From 61587aeff28b8bf2e4dc7c8172c259becfe1755d Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Tue, 23 Jun 2026 19:48:41 -0400 Subject: [PATCH 1/6] feat(ci-select): Add graphify ci-select subcommand for graph-informed CI test selection Add a new subcommand that uses the knowledge graph to determine which tests need to run based on changed files. Supports cross-repository test selection when changes in one repo affect dependencies in another repo. --- graphify/__main__.py | 8 + graphify/ci_select.py | 600 ++++++++++++++++++++++++++++++++++++++++ graphify/cli.py | 4 + tests/test_ci_select.py | 351 +++++++++++++++++++++++ 4 files changed, 963 insertions(+) create mode 100644 graphify/ci_select.py create mode 100644 tests/test_ci_select.py diff --git a/graphify/__main__.py b/graphify/__main__.py index 155501a98d..8fd2b7118f 100644 --- a/graphify/__main__.py +++ b/graphify/__main__.py @@ -622,6 +622,14 @@ def _run_cli() -> None: print(" global remove remove a repo's nodes from the global graph") print(" global list list repos in the global graph") print(" global path print path to the global graph file") + print(" ci-select graph-informed CI test selection from a diff") + print(" --repo repository name (required)") + print(" --diff-cmd shell command to produce a diff (e.g. 'git diff origin/main...HEAD')") + print(" --diff read diff from file or stdin") + print(" --files comma-separated changed file paths") + print(" --graph path to graph.json (default graphify-out/graph.json)") + print(" --test-jobs path to test-jobs.yaml mapping (auto-detected if omitted)") + print(" --depth N BFS traversal depth (default 3)") print(" benchmark [graph.json] measure token reduction vs naive full-corpus approach") print(" export callflow-html emit Mermaid-based architecture/call-flow HTML") print(" hook install install post-commit/post-checkout git hooks (all platforms)") diff --git a/graphify/ci_select.py b/graphify/ci_select.py new file mode 100644 index 0000000000..6df5c9296b --- /dev/null +++ b/graphify/ci_select.py @@ -0,0 +1,600 @@ +"""graphify ci-select: Graph-informed CI test selection. + +Uses the graphify knowledge graph to determine which CI tests to run +for a given set of code changes. BFS traversal up to N hops from changed +files, then maps reachable nodes to CI job names via a test-jobs.yaml mapping. +""" +from __future__ import annotations + +import fnmatch +import json +import subprocess +import sys +from collections import deque +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +import networkx as nx + + +@dataclass +class TestPlan: + must_run: list[str] = field(default_factory=list) + should_run: list[str] = field(default_factory=list) + skip: list[str] = field(default_factory=list) + cross_repo: list[dict[str, Any]] = field(default_factory=list) + reasoning: str = "" + confidence: float = 1.0 + graph_paths: list[str] = field(default_factory=list) + warnings: list[str] = field(default_factory=list) + + def to_dict(self) -> dict[str, Any]: + return { + "test_plan": { + "must_run": self.must_run, + "should_run": self.should_run, + "skip": self.skip, + "cross_repo": self.cross_repo, + }, + "reasoning": self.reasoning, + "confidence": self.confidence, + "graph_paths": self.graph_paths, + "warnings": self.warnings, + } + + +def load_graph(graph_path: str | Path) -> nx.Graph: + """Load a graphify graph.json into a NetworkX graph.""" + from networkx.readwrite import json_graph + + path = Path(graph_path).resolve() + if not path.exists(): + raise FileNotFoundError(f"Graph file not found: {path}") + raw = json.loads(path.read_text(encoding="utf-8")) + if "links" not in raw and "edges" in raw: + raw = dict(raw, links=raw["edges"]) + raw = {**raw, "directed": True} + try: + return json_graph.node_link_graph(raw, edges="links") + except TypeError: + return json_graph.node_link_graph(raw) + + +def parse_diff_files(diff_text: str) -> list[str]: + """Extract changed file paths from a unified diff.""" + files: list[str] = [] + for line in diff_text.splitlines(): + if line.startswith("diff --git"): + # diff --git a/path/to/file b/path/to/file + parts = line.split() + if len(parts) >= 4: + path = parts[3] + if path.startswith("b/"): + path = path[2:] + if path not in files: + files.append(path) + elif line.startswith("+++ b/"): + path = line[6:] + if path not in files: + files.append(path) + return files + + +def load_test_jobs(yaml_path: str | Path) -> dict[str, dict[str, Any]]: + """Load test-jobs.yaml mapping file. + + Returns dict of job_name -> {"graph_patterns": [...], "description": "..."} + """ + path = Path(yaml_path) + if not path.exists(): + return {} + + try: + import yaml # type: ignore[import-untyped] + data = yaml.safe_load(path.read_text(encoding="utf-8")) + except ImportError: + data = _parse_simple_yaml(path.read_text(encoding="utf-8")) + + if not isinstance(data, dict): + return {} + + # The YAML has repo_name -> jobs -> job_name -> {graph_patterns, description} + # Flatten to just job_name -> config + jobs: dict[str, dict[str, Any]] = {} + for _repo_key, repo_val in data.items(): + if isinstance(repo_val, dict) and "jobs" in repo_val: + for job_name, job_config in repo_val["jobs"].items(): + jobs[job_name] = job_config + elif isinstance(repo_val, dict): + for job_name, job_config in repo_val.items(): + if isinstance(job_config, dict): + jobs[job_name] = job_config + return jobs + + +def _parse_simple_yaml(text: str) -> dict[str, Any]: + """Minimal YAML-like parser for test-jobs.yaml format. + + Only handles the specific nested-dict + list-of-strings structure we use. + Falls back gracefully when pyyaml is unavailable. + """ + import re + + result: dict[str, Any] = {} + stack: list[tuple[int, dict]] = [(-1, result)] + + for line in text.splitlines(): + stripped = line.lstrip() + if not stripped or stripped.startswith("#"): + continue + + indent = len(line) - len(stripped) + + # Pop stack to find parent at correct indent level + while len(stack) > 1 and stack[-1][0] >= indent: + stack.pop() + + parent = stack[-1][1] + + # List item: "- value" + if stripped.startswith("- "): + val = stripped[2:].strip().strip('"').strip("'") + if isinstance(parent, dict): + for k in reversed(list(parent.keys())): + if parent[k] is None or isinstance(parent[k], list): + if parent[k] is None: + parent[k] = [] + parent[k].append(val) + break + continue + + # Key-value or key-only + m = re.match(r"^([^:]+):\s*(.*)", stripped) + if m: + key = m.group(1).strip() + value = m.group(2).strip().strip('"').strip("'") + if value: + parent[key] = value + else: + new_dict: dict[str, Any] = {} + parent[key] = new_dict + stack.append((indent, new_dict)) + + return result + + +def find_nodes_for_file( + G: nx.Graph, file_path: str, repo: str +) -> list[str]: + """Find graph nodes that correspond to a given file path. + + Tries multiple matching strategies: + 1. Exact source_file match (with repo prefix) + 2. Exact source_file match (without repo prefix) + 3. source_file ends with the path + """ + candidates: list[str] = [] + repo_prefixed = f"{repo}/{file_path}" + + for node_id, data in G.nodes(data=True): + source_file = data.get("source_file", "") + if not source_file: + continue + if source_file == repo_prefixed or source_file == file_path: + candidates.append(node_id) + elif source_file.endswith("/" + file_path): + candidates.append(node_id) + + return candidates + + +def bfs_reachable( + G: nx.Graph, seeds: list[str], max_depth: int = 3 +) -> dict[str, int]: + """BFS from seed nodes, returning reachable node_id -> depth. + + Traverses both incoming and outgoing edges (undirected BFS on a + directed graph) to find all structurally connected code. + """ + visited: dict[str, int] = {} + queue: deque[tuple[str, int]] = deque() + + for seed in seeds: + if seed not in visited: + visited[seed] = 0 + queue.append((seed, 0)) + + while queue: + current, depth = queue.popleft() + if depth >= max_depth: + continue + + neighbors: set[str] = set() + for _, target in G.out_edges(current): + neighbors.add(str(target)) + for source, _ in G.in_edges(current): + neighbors.add(str(source)) + + for neighbor in neighbors: + if neighbor not in visited: + visited[neighbor] = depth + 1 + queue.append((neighbor, depth + 1)) + + return visited + + +def match_patterns( + file_paths: list[str], patterns: list[str] +) -> int: + """Count how many file paths match any of the glob patterns.""" + count = 0 + for fp in file_paths: + for pat in patterns: + if fnmatch.fnmatch(fp, pat): + count += 1 + break + return count + + +def find_neighbors_summary(G: nx.Graph, node_ids: list[str]) -> str: + """Summarize what a set of nodes connects to.""" + labels: list[str] = [] + seen: set[str] = set() + for nid in node_ids: + for _, target in G.out_edges(nid): + target = str(target) + if target not in seen: + seen.add(target) + data = G.nodes.get(target, {}) + label = data.get("label", target) + labels.append(str(label)) + if len(labels) > 3: + return f"{', '.join(labels[:3])} (+{len(labels) - 3} more)" + return ", ".join(labels) if labels else "(no connections)" + + +def ci_select( + graph_path: str | Path, + changed_files: list[str], + repo: str, + test_jobs_path: str | Path | None = None, + max_depth: int = 3, +) -> TestPlan: + """Main entry point: determine which CI tests to run. + + Args: + graph_path: Path to graph.json + changed_files: List of repo-relative file paths that changed + repo: Repository name (e.g. "fulfillment-service") + test_jobs_path: Path to test-jobs.yaml mapping file + max_depth: BFS traversal depth (default 3) + + Returns: + TestPlan with categorized test jobs + """ + plan = TestPlan() + + if not changed_files: + plan.confidence = 1.0 + plan.reasoning = "No files changed." + return plan + + # Load graph + G = load_graph(graph_path) + + # Find seed nodes for changed files + all_seeds: list[str] = [] + unknown_files: list[str] = [] + file_to_nodes: dict[str, list[str]] = {} + + for f in changed_files: + nodes = find_nodes_for_file(G, f, repo) + if nodes: + all_seeds.extend(nodes) + file_to_nodes[f] = nodes + else: + unknown_files.append(f) + + # Confidence calculation + if not all_seeds: + plan.confidence = 0.0 + plan.reasoning = ( + f"None of the {len(changed_files)} changed files have graph nodes. " + "Falling back to full test suite." + ) + plan.warnings.append( + f"Unknown files: {', '.join(unknown_files[:10])}" + + ( + f" (and {len(unknown_files) - 10} more)" + if len(unknown_files) > 10 + else "" + ) + ) + return plan + + if unknown_files: + known_ratio = len(file_to_nodes) / len(changed_files) + plan.confidence = max(0.3, known_ratio * 0.9) + plan.warnings.append( + f"{len(unknown_files)} changed file(s) not in graph: " + + ", ".join(unknown_files[:5]) + + ( + f" (and {len(unknown_files) - 5} more)" + if len(unknown_files) > 5 + else "" + ) + ) + else: + plan.confidence = 0.9 + + # BFS traversal from seed nodes + reachable = bfs_reachable(G, all_seeds, max_depth=max_depth) + + # Collect source files of reachable nodes + reachable_files: list[str] = [] + cross_repo_files: dict[str, list[str]] = {} + + for node_id in reachable: + data = G.nodes.get(node_id, {}) + source_file = data.get("source_file", "") + if not source_file: + continue + + parts = source_file.split("/", 1) + if len(parts) == 2: + node_repo = parts[0] + node_file = parts[1] + else: + node_repo = repo + node_file = source_file + + if node_repo == repo: + if node_file not in reachable_files: + reachable_files.append(node_file) + else: + cross_repo_files.setdefault(node_repo, []) + if node_file not in cross_repo_files[node_repo]: + cross_repo_files[node_repo].append(node_file) + + # Load test jobs mapping + jobs: dict[str, dict[str, Any]] = {} + if test_jobs_path: + jobs = load_test_jobs(test_jobs_path) + + if not jobs: + plan.reasoning = ( + f"BFS from {len(all_seeds)} seed nodes reached {len(reachable)} nodes " + f"across {len(reachable_files)} files in {repo}. " + f"No test-jobs.yaml mapping found -- cannot map to CI jobs." + ) + if cross_repo_files: + for cr_repo, cr_files in cross_repo_files.items(): + plan.cross_repo.append( + { + "repo": cr_repo, + "files_affected": len(cr_files), + "tests": [], + } + ) + for f, nodes in list(file_to_nodes.items())[:5]: + plan.graph_paths.append( + f"{f} -> {find_neighbors_summary(G, nodes)}" + ) + return plan + + # Map reachable files to CI jobs + all_matchable = list(set(reachable_files + changed_files)) + job_match_counts: dict[str, int] = {} + + for job_name, job_config in jobs.items(): + patterns = job_config.get("graph_patterns", []) + if isinstance(patterns, str): + patterns = [patterns] + count = match_patterns(all_matchable, patterns) + job_match_counts[job_name] = count + + # Categorize: 3+ matches = must_run, 1-2 = should_run, 0 = skip + all_job_names = list(jobs.keys()) + for job_name in all_job_names: + count = job_match_counts.get(job_name, 0) + if count >= 3: + plan.must_run.append(job_name) + elif count >= 1: + plan.should_run.append(job_name) + else: + plan.skip.append(job_name) + + # Cross-repo analysis + if cross_repo_files: + for cr_repo, cr_files in cross_repo_files.items(): + cr_tests: list[str] = [] + cr_mapping_path = None + if test_jobs_path: + parent = Path(test_jobs_path).parent.parent + candidate = parent / cr_repo / "test-jobs.yaml" + if candidate.exists(): + cr_mapping_path = candidate + + if cr_mapping_path: + cr_jobs = load_test_jobs(cr_mapping_path) + for cj_name, cj_config in cr_jobs.items(): + cr_patterns = cj_config.get("graph_patterns", []) + if isinstance(cr_patterns, str): + cr_patterns = [cr_patterns] + if match_patterns(cr_files, cr_patterns) > 0: + cr_tests.append(cj_name) + + plan.cross_repo.append( + { + "repo": cr_repo, + "tests": cr_tests, + "files_affected": len(cr_files), + } + ) + + # Build reasoning + reasoning_parts = [ + f"BFS from {len(all_seeds)} seed nodes (depth {max_depth}) " + f"reached {len(reachable)} nodes.", + ] + if plan.must_run: + reasoning_parts.append(f"Must run: {', '.join(plan.must_run)}.") + if plan.should_run: + reasoning_parts.append(f"Should run: {', '.join(plan.should_run)}.") + if plan.skip: + reasoning_parts.append(f"Skip: {', '.join(plan.skip)}.") + if plan.cross_repo: + for cr in plan.cross_repo: + reasoning_parts.append( + f"Cross-repo impact on {cr['repo']}: " + f"{cr['files_affected']} files affected." + ) + plan.reasoning = " ".join(reasoning_parts) + + # Graph paths for traceability + for f, nodes in list(file_to_nodes.items())[:5]: + summary = find_neighbors_summary(G, nodes) + plan.graph_paths.append(f"{f} -> {summary}") + + # Confidence adjustment + if plan.confidence >= 0.5 and not plan.must_run and not plan.should_run: + plan.confidence = min(plan.confidence, 0.6) + plan.warnings.append( + "No jobs matched despite valid graph nodes -- " + "verify test-jobs.yaml patterns" + ) + + return plan + + +def cli_main(argv: list[str] | None = None) -> None: + """CLI entry point for ``graphify ci-select``.""" + args = argv if argv is not None else sys.argv[2:] + + graph_path = "graphify-out/graph.json" + repo = "" + diff_cmd = "" + files_str = "" + test_jobs = "" + max_depth = 3 + + i = 0 + while i < len(args): + arg = args[i] + if arg == "--graph" and i + 1 < len(args): + graph_path = args[i + 1] + i += 2 + elif arg.startswith("--graph="): + graph_path = arg.split("=", 1)[1] + i += 1 + elif arg == "--repo" and i + 1 < len(args): + repo = args[i + 1] + i += 2 + elif arg.startswith("--repo="): + repo = arg.split("=", 1)[1] + i += 1 + elif arg == "--diff-cmd" and i + 1 < len(args): + diff_cmd = args[i + 1] + i += 2 + elif arg.startswith("--diff-cmd="): + diff_cmd = arg.split("=", 1)[1] + i += 1 + elif arg == "--files" and i + 1 < len(args): + files_str = args[i + 1] + i += 2 + elif arg.startswith("--files="): + files_str = arg.split("=", 1)[1] + i += 1 + elif arg == "--test-jobs" and i + 1 < len(args): + test_jobs = args[i + 1] + i += 2 + elif arg.startswith("--test-jobs="): + test_jobs = arg.split("=", 1)[1] + i += 1 + elif arg == "--depth" and i + 1 < len(args): + try: + max_depth = int(args[i + 1]) + except ValueError: + print("error: --depth must be an integer", file=sys.stderr) + sys.exit(1) + i += 2 + elif arg.startswith("--depth="): + try: + max_depth = int(arg.split("=", 1)[1]) + except ValueError: + print("error: --depth must be an integer", file=sys.stderr) + sys.exit(1) + i += 1 + elif arg == "--diff" and i + 1 < len(args): + diff_path = args[i + 1] + i += 2 + if diff_path == "-": + diff_text = sys.stdin.read() + else: + diff_text = Path(diff_path).read_text(encoding="utf-8") + files_str = ",".join(parse_diff_files(diff_text)) + elif arg.startswith("--diff="): + diff_path = arg.split("=", 1)[1] + i += 1 + if diff_path == "-": + diff_text = sys.stdin.read() + else: + diff_text = Path(diff_path).read_text(encoding="utf-8") + files_str = ",".join(parse_diff_files(diff_text)) + else: + i += 1 + + if not repo: + print("error: --repo is required", file=sys.stderr) + sys.exit(1) + + # Get changed files + changed_files: list[str] = [] + if diff_cmd: + try: + result = subprocess.run( + diff_cmd, + shell=True, + capture_output=True, + text=True, + timeout=30, + ) + changed_files = parse_diff_files(result.stdout) + except subprocess.TimeoutExpired: + print("error: diff command timed out", file=sys.stderr) + sys.exit(1) + elif files_str: + changed_files = [f.strip() for f in files_str.split(",") if f.strip()] + else: + if not sys.stdin.isatty(): + diff_text = sys.stdin.read() + changed_files = parse_diff_files(diff_text) + else: + print( + "error: provide changed files via --diff-cmd, --diff, " + "--files, or stdin", + file=sys.stderr, + ) + sys.exit(1) + + # Auto-detect test-jobs.yaml if not specified + if not test_jobs: + candidates = [ + Path(repo) / "test-jobs.yaml", + Path("test-jobs.yaml"), + ] + for c in candidates: + if c.exists(): + test_jobs = str(c) + break + + plan = ci_select( + graph_path=graph_path, + changed_files=changed_files, + repo=repo, + test_jobs_path=test_jobs or None, + max_depth=max_depth, + ) + + print(json.dumps(plan.to_dict(), indent=2)) diff --git a/graphify/cli.py b/graphify/cli.py index 5b73397266..ac33d73bb7 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -4480,6 +4480,10 @@ def _invalidate_file_manifest_for_db_graph() -> None: _wja(out_path2, merged2, ensure_ascii=False) print(f"Merged: {len(merged2['nodes'])} nodes, {len(merged2['edges'])} edges") + elif cmd == "ci-select": + from graphify.ci_select import cli_main as _ci_select_main + _ci_select_main() + elif Path(cmd).exists() or cmd in (".", "..") or cmd.startswith(("./", "../", "/", "~")): # User ran `graphify ` directly — treat as `graphify extract `. # Common when following the PowerShell note in README (`graphify .`) or diff --git a/tests/test_ci_select.py b/tests/test_ci_select.py new file mode 100644 index 0000000000..89b643c738 --- /dev/null +++ b/tests/test_ci_select.py @@ -0,0 +1,351 @@ +"""Tests for graphify ci-select module.""" +from __future__ import annotations + +import json +import textwrap +from pathlib import Path +from unittest.mock import patch + +import networkx as nx +import pytest +from networkx.readwrite import json_graph + +from graphify.ci_select import ( + TestPlan, + bfs_reachable, + ci_select, + cli_main, + find_nodes_for_file, + load_test_jobs, + match_patterns, + parse_diff_files, +) + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + +def _make_graph() -> nx.DiGraph: + """Build a small test graph with cross-repo structure.""" + G = nx.DiGraph() + # service-a files + G.add_node("fs_main", source_file="service-a/cmd/service-a/main.go", label="main.go") + G.add_node("fs_clusters", source_file="service-a/internal/servers/clusters_server.go", label="clusters_server.go") + G.add_node("fs_subnets", source_file="service-a/internal/servers/subnets_server.go", label="subnets_server.go") + G.add_node("fs_proto", source_file="service-a/internal/api/public/v1/clusters_service.pb.go", label="clusters_service.pb.go") + G.add_node("fs_db", source_file="service-a/internal/database/clusters.go", label="clusters.go") + G.add_node("fs_lint", source_file="service-a/dev/lint.py", label="lint.py") + G.add_node("fs_chart", source_file="service-a/charts/values.yaml", label="values.yaml") + G.add_node("fs_it", source_file="service-a/it/integration_test.go", label="integration_test.go") + + # service-b-operator files + G.add_node("op_ctrl", source_file="service-b-operator/internal/controller/cluster_controller.go", label="cluster_controller.go") + G.add_node("op_api", source_file="service-b-operator/api/v1alpha1/cluster_types.go", label="cluster_types.go") + + # Edges: clusters_server -> proto -> operator controller + G.add_edge("fs_clusters", "fs_proto", relation="references") + G.add_edge("fs_clusters", "fs_db", relation="calls") + G.add_edge("fs_proto", "op_ctrl", relation="references") + G.add_edge("op_ctrl", "op_api", relation="references") + G.add_edge("fs_main", "fs_clusters", relation="calls") + G.add_edge("fs_clusters", "fs_subnets", relation="references") + G.add_edge("fs_chart", "fs_it", relation="references") + + return G + + +def _save_graph(G: nx.DiGraph, path: Path) -> None: + data = json_graph.node_link_data(G, edges="links") + path.write_text(json.dumps(data), encoding="utf-8") + + +TEST_JOBS_YAML = textwrap.dedent("""\ + service-a: + jobs: + run-unit-tests: + graph_patterns: + - "internal/**" + - "cmd/**" + description: "Unit tests" + run-integration-tests-helm: + graph_patterns: + - "charts/**" + - "it/**" + description: "Integration tests (Helm)" + check-generated-code: + graph_patterns: + - "proto/**" + - "internal/api/**" + description: "Proto validation" + check-python-code: + graph_patterns: + - "dev/**" + description: "Python lint" +""") + + +# --------------------------------------------------------------------------- +# Tests: parse_diff_files +# --------------------------------------------------------------------------- + +class TestParseDiffFiles: + def test_basic_diff(self): + diff = textwrap.dedent("""\ + diff --git a/internal/servers/clusters_server.go b/internal/servers/clusters_server.go + --- a/internal/servers/clusters_server.go + +++ b/internal/servers/clusters_server.go + @@ -1,3 +1,4 @@ + +// new line + package servers + """) + files = parse_diff_files(diff) + assert files == ["internal/servers/clusters_server.go"] + + def test_multiple_files(self): + diff = textwrap.dedent("""\ + diff --git a/file1.go b/file1.go + +++ b/file1.go + diff --git a/file2.go b/file2.go + +++ b/file2.go + """) + files = parse_diff_files(diff) + assert files == ["file1.go", "file2.go"] + + def test_no_duplicates(self): + diff = textwrap.dedent("""\ + diff --git a/file1.go b/file1.go + +++ b/file1.go + diff --git a/file1.go b/file1.go + +++ b/file1.go + """) + files = parse_diff_files(diff) + assert files == ["file1.go"] + + def test_empty_diff(self): + assert parse_diff_files("") == [] + + +# --------------------------------------------------------------------------- +# Tests: find_nodes_for_file +# --------------------------------------------------------------------------- + +class TestFindNodesForFile: + def test_find_with_repo_prefix(self): + G = _make_graph() + nodes = find_nodes_for_file(G, "internal/servers/clusters_server.go", "service-a") + assert "fs_clusters" in nodes + + def test_find_no_match(self): + G = _make_graph() + nodes = find_nodes_for_file(G, "nonexistent_file.go", "service-a") + assert nodes == [] + + +# --------------------------------------------------------------------------- +# Tests: bfs_reachable +# --------------------------------------------------------------------------- + +class TestBfsReachable: + def test_depth_0(self): + G = _make_graph() + reachable = bfs_reachable(G, ["fs_clusters"], max_depth=0) + assert reachable == {"fs_clusters": 0} + + def test_depth_1(self): + G = _make_graph() + reachable = bfs_reachable(G, ["fs_clusters"], max_depth=1) + assert "fs_clusters" in reachable + assert "fs_proto" in reachable + assert "fs_db" in reachable + assert "fs_main" in reachable # incoming edge + assert "fs_subnets" in reachable + + def test_depth_2_crosses_repo(self): + G = _make_graph() + reachable = bfs_reachable(G, ["fs_clusters"], max_depth=2) + assert "op_ctrl" in reachable # 2 hops: clusters -> proto -> op_ctrl + + def test_depth_3(self): + G = _make_graph() + reachable = bfs_reachable(G, ["fs_clusters"], max_depth=3) + assert "op_api" in reachable # 3 hops: clusters -> proto -> op_ctrl -> op_api + + def test_multiple_seeds(self): + G = _make_graph() + reachable = bfs_reachable(G, ["fs_clusters", "fs_lint"], max_depth=1) + assert "fs_clusters" in reachable + assert "fs_lint" in reachable + + +# --------------------------------------------------------------------------- +# Tests: match_patterns +# --------------------------------------------------------------------------- + +class TestMatchPatterns: + def test_basic_match(self): + assert match_patterns(["internal/servers/foo.go"], ["internal/**"]) == 1 + + def test_no_match(self): + assert match_patterns(["dev/lint.py"], ["internal/**"]) == 0 + + def test_multiple_files(self): + files = ["internal/a.go", "internal/b.go", "dev/c.py"] + assert match_patterns(files, ["internal/**"]) == 2 + + def test_multiple_patterns(self): + files = ["cmd/main.go"] + assert match_patterns(files, ["internal/**", "cmd/**"]) == 1 + + +# --------------------------------------------------------------------------- +# Tests: load_test_jobs +# --------------------------------------------------------------------------- + +class TestLoadTestJobs: + def test_load_yaml(self, tmp_path): + yaml_file = tmp_path / "test-jobs.yaml" + yaml_file.write_text(TEST_JOBS_YAML) + jobs = load_test_jobs(yaml_file) + assert "run-unit-tests" in jobs + assert "run-integration-tests-helm" in jobs + assert "check-generated-code" in jobs + assert "check-python-code" in jobs + + def test_missing_file(self, tmp_path): + jobs = load_test_jobs(tmp_path / "nonexistent.yaml") + assert jobs == {} + + +# --------------------------------------------------------------------------- +# Tests: ci_select (integration) +# --------------------------------------------------------------------------- + +class TestCiSelect: + def test_go_change_selects_unit_tests(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + jobs_path = tmp_path / "test-jobs.yaml" + jobs_path.write_text(TEST_JOBS_YAML) + + plan = ci_select( + graph_path=graph_path, + changed_files=["internal/servers/clusters_server.go"], + repo="service-a", + test_jobs_path=jobs_path, + ) + assert "run-unit-tests" in plan.must_run or "run-unit-tests" in plan.should_run + assert plan.confidence >= 0.5 + + def test_python_change_skips_go_tests(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + jobs_path = tmp_path / "test-jobs.yaml" + jobs_path.write_text(TEST_JOBS_YAML) + + plan = ci_select( + graph_path=graph_path, + changed_files=["dev/lint.py"], + repo="service-a", + test_jobs_path=jobs_path, + ) + assert "check-python-code" in plan.must_run or "check-python-code" in plan.should_run + assert "run-unit-tests" in plan.skip + + def test_unknown_file_low_confidence(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + jobs_path = tmp_path / "test-jobs.yaml" + jobs_path.write_text(TEST_JOBS_YAML) + + plan = ci_select( + graph_path=graph_path, + changed_files=["totally_unknown_file.xyz"], + repo="service-a", + test_jobs_path=jobs_path, + ) + assert plan.confidence == 0.0 + assert len(plan.warnings) > 0 + + def test_cross_repo_detection(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + + plan = ci_select( + graph_path=graph_path, + changed_files=["internal/servers/clusters_server.go"], + repo="service-a", + ) + cross_repos = [cr["repo"] for cr in plan.cross_repo] + assert "service-b-operator" in cross_repos + + def test_no_files_changed(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + + plan = ci_select( + graph_path=graph_path, + changed_files=[], + repo="service-a", + ) + assert plan.confidence == 1.0 + assert plan.reasoning == "No files changed." + + def test_no_test_jobs_mapping(self, tmp_path): + G = _make_graph() + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + + plan = ci_select( + graph_path=graph_path, + changed_files=["internal/servers/clusters_server.go"], + repo="service-a", + ) + # Without test-jobs.yaml, must_run/should_run/skip are empty + assert plan.must_run == [] + assert plan.should_run == [] + assert plan.skip == [] + assert "No test-jobs.yaml mapping found" in plan.reasoning + + +# --------------------------------------------------------------------------- +# Tests: TestPlan +# --------------------------------------------------------------------------- + +class TestTestPlan: + def test_to_dict(self): + plan = TestPlan( + must_run=["unit-tests"], + should_run=["integration-helm"], + skip=["integration-kustomize"], + cross_repo=[{"repo": "service-b-operator", "tests": ["check-generated-code"]}], + reasoning="Test reasoning", + confidence=0.85, + ) + d = plan.to_dict() + assert d["test_plan"]["must_run"] == ["unit-tests"] + assert d["confidence"] == 0.85 + assert d["test_plan"]["cross_repo"][0]["repo"] == "service-b-operator" + + +# --------------------------------------------------------------------------- +# Tests: cli_main +# --------------------------------------------------------------------------- + +class TestCliMain: + def test_missing_repo_exits(self): + with pytest.raises(SystemExit) as exc_info: + cli_main(["--files", "foo.go"]) + assert exc_info.value.code == 1 + + def test_no_input_exits(self, tmp_path): + with patch("sys.stdin") as mock_stdin: + mock_stdin.isatty.return_value = True + with pytest.raises(SystemExit) as exc_info: + cli_main(["--repo", "test-repo"]) + assert exc_info.value.code == 1 From d72a97b3ebab0d9274a43a1e4f29498f1cb21c8d Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Thu, 13 Aug 2026 01:19:54 -0400 Subject: [PATCH 2/6] docs(metadata): Document metadata.json schema as a canonical contract Add a JSON Schema for the metadata.json file that accompanies published graph bundles. This schema serves as the single source of truth for both the CI job that generates the bundle and the fetch script that consumes it, preventing drift between writer and reader implementations. The schema defines fields for version tracking (source_sha, graphify_version), staleness detection (generated_at), and bundle file paths (graph.json, GRAPH_REPORT.md, manifest.json). --- README.md | 1 + docs/graph-bundle-metadata.md | 57 ++++++++++++++++++++++++++ docs/graph-bundle-metadata.schema.json | 50 ++++++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 docs/graph-bundle-metadata.md create mode 100644 docs/graph-bundle-metadata.schema.json diff --git a/README.md b/README.md index 0c14d207c9..ac962aea71 100644 --- a/README.md +++ b/README.md @@ -801,6 +801,7 @@ graphify label ./my-project --backend=openai --model gpt-4o # force a specific - [How it works](docs/how-it-works.md) — the extraction pipeline, community detection, confidence scoring, benchmarks - [ARCHITECTURE.md](ARCHITECTURE.md) — module breakdown, how to add a language - [Optional integrations](docs/docker-mcp-sqlite.md) — Docker MCP Toolkit + SQLite +- [Graph bundle metadata](docs/graph-bundle-metadata.md) — schema for `metadata.json` when a `graphify --update` output is published for other machines/CI to pull - [The Memory Layer](https://safishamsi.gumroad.com/l/qetvlo) — the book on the ideas behind graphify, the architecture end to end --- diff --git a/docs/graph-bundle-metadata.md b/docs/graph-bundle-metadata.md new file mode 100644 index 0000000000..921616730e --- /dev/null +++ b/docs/graph-bundle-metadata.md @@ -0,0 +1,57 @@ +# graphify bundle metadata (`metadata.json`) + +When a `graphify --update` output is published for other machines/CI jobs to +pull (rather than generated locally), it ships as one atomic bundle: +`graph.json` + `GRAPH_REPORT.md` + `manifest.json` + a small `metadata.json` +describing the bundle itself. + +`metadata.json` exists because the bundle has two independent consumers that +must never drift apart on what fields to expect: + +- **Writer**: the CI job that runs `graphify --update` and publishes the + bundle (e.g. a scheduled graph-refresh workflow). +- **Reader**: the fetch script that pulls the bundle down and validates it + before swapping it into a local `graphify-out/`, ahead of graphify's own + `CLAUDE.md` directive / `PreToolUse` hook consuming it. + +Both of those typically live in consuming repos outside this one, but both are built +against graphify's own output format, so this repo is the natural single +source of truth for the contract between them -- one documented schema +instead of two independently-evolving assumptions. + +**Schema**: [`graph-bundle-metadata.schema.json`](./graph-bundle-metadata.schema.json) +(JSON Schema, draft 2020-12). + +## Example + +```json +{ + "schema_version": 1, + "source_sha": "e4bfd2ad1a9393251023a4edef93e93dc798afc7", + "graphify_version": "0.9.41", + "generated_at": "2026-08-13T02:00:00Z", + "bundle": { + "graph": "graph.json", + "report": "GRAPH_REPORT.md", + "manifest": "manifest.json" + } +} +``` + +## What each field is for + +- `source_sha` / `generated_at`: staleness. A consumer compares `source_sha` + against its local `HEAD` (ancestry check, not a race guard -- the bundle's + own publish path is already serialized by a CI `concurrency:` group); when + that comparison isn't possible, `generated_at` backs a TTL fallback. +- `graphify_version`: a hard compatibility gate. A version mismatch against + the locally-installed `graphify --version` means the fetch script refuses + to load the bundle and prints the exact upgrade command, rather than + risking a schema-mismatched `graph.json` being consulted silently. +- `bundle`: where the other three files live inside the archive, so the + reader doesn't hardcode filenames independently of what the writer chose. + +`manifest.json` rides along for the *writer's* own benefit (restoring +incremental-extraction continuity across ephemeral CI runners between +scheduled runs) -- ordinary consumers only need `graph.json` and +`GRAPH_REPORT.md`. diff --git a/docs/graph-bundle-metadata.schema.json b/docs/graph-bundle-metadata.schema.json new file mode 100644 index 0000000000..a4bfafb6bd --- /dev/null +++ b/docs/graph-bundle-metadata.schema.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/Graphify-Labs/graphify/blob/v8/docs/graph-bundle-metadata.schema.json", + "title": "graphify knowledge-graph bundle metadata", + "description": "Schema for metadata.json, the small manifest bundled alongside graph.json/GRAPH_REPORT.md/manifest.json when a graphify --update output is published as a release/OCI artifact. Written by the CI job that runs `graphify --update` and publishes the bundle; read by the fetch script that pulls the bundle down before graphify's CLAUDE.md directive/PreToolUse hook consult it. This is the single canonical definition both sides validate against, so the writer and reader can't drift independently -- one typically lives in a scheduled graph-refresh workflow, the other in a SessionStart fetch script, both consuming repos using the same published schema.", + "type": "object", + "required": ["schema_version", "source_sha", "graphify_version", "generated_at", "bundle"], + "additionalProperties": false, + "properties": { + "schema_version": { + "type": "integer", + "const": 1, + "description": "Version of this metadata.json schema itself, not of graphify or the bundle contents. Bump on any breaking change to this file's shape so a reader can refuse an unrecognized version cleanly instead of guessing at missing/renamed fields." + }, + "source_sha": { + "type": "string", + "pattern": "^[0-9a-f]{40}$", + "description": "Full git commit SHA of the source repository HEAD that graphify --update was run against. The staleness check compares this against a consumer's local HEAD (via `git merge-base --is-ancestor`, used purely as a freshness signal, not a publish-time race guard)." + }, + "graphify_version": { + "type": "string", + "description": "Output of `graphify --version` for the graphify install that generated this bundle (e.g. \"0.9.41\"). The fetch script refuses to load a bundle whose graphify_version doesn't match the locally installed `graphify --version`, printing the exact upgrade command, rather than silently loading a graph shaped by a different schema/format version." + }, + "generated_at": { + "type": "string", + "format": "date-time", + "description": "ISO 8601 UTC timestamp of when this bundle was published. Backs the TTL fallback staleness check (e.g. treat the bundle as stale if generated_at is >24h old) for the case where source_sha ancestry comparison isn't possible (e.g. consumer's local HEAD is on an unrelated branch/fork)." + }, + "bundle": { + "type": "object", + "required": ["graph", "report", "manifest"], + "additionalProperties": false, + "description": "Paths, relative to the archive root, of the other files published alongside this metadata.json in the same atomic bundle (a single archive, so a consumer either gets the whole matched set or a clean 404/missing-bundle, never a mismatched pair from two different publishes).", + "properties": { + "graph": { + "type": "string", + "description": "Path to graph.json -- the queryable knowledge graph itself. What ordinary consumers (graphify's PreToolUse hook, ci-select) actually load." + }, + "report": { + "type": "string", + "description": "Path to GRAPH_REPORT.md -- the human-readable summary of the graph." + }, + "manifest": { + "type": "string", + "description": "Path to manifest.json -- graphify's own incremental-extraction state. Only needed by the generation workflow itself to restore continuity before its next `graphify --update` run (see the artifact-re-pull fallback for actions/cache eviction); ordinary consumers querying the graph never need to open it." + } + } + } + } +} From 444a80ca489f0d9472fc91c9fafb177eeaafa0db Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 10:51:24 -0400 Subject: [PATCH 3/6] fix(github-actions): Fix 8 review findings - Add looks_like_workflow_shape() content check to prevent non-workflow files at workflow paths from being misclassified - Fix UnicodeDecodeError handling in cache corruption detection - Harden install_references write probe for root/elevated contexts - Gate test platform capabilities (mkfifo, AF_UNIX, symlinks) on actual availability - Fix whitespace and comment clarity throughout These are real bug fixes caught in code review, not just style improvements. --- ARCHITECTURE.md | 2 +- README.md | 2 +- graphify/cache.py | 6 +- graphify/detect.py | 21 + graphify/extract.py | 18 + graphify/extractors/github_actions.py | 364 ++++++++++++++++++ pyproject.toml | 8 +- tests/test_cache.py | 534 +++----------------------- tests/test_github_actions.py | 336 ++++++++++++++++ tests/test_install_references.py | 8 +- tests/test_non_regular_files.py | 15 + uv.lock | 49 ++- 12 files changed, 846 insertions(+), 517 deletions(-) create mode 100644 graphify/extractors/github_actions.py create mode 100644 tests/test_github_actions.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 080f46f223..05d4cec1db 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -8,7 +8,7 @@ graphify is a Claude Code skill backed by a Python library. The skill orchestrat detect() → extract() → build() → cluster() → analyze helpers → report.generate() → export.to_*() ``` -Each stage lives in its own module and they communicate through plain Python dicts and NetworkX graphs - no shared state, no side effects outside `graphify-out/`. Most stages are a single function; `analyze.py` and `export.py` are sets of sibling functions rather than one entry point. +Each stage lives in its own module; the public contract between them is plain Python dicts and NetworkX graphs, not shared in-process state. `extract()` does have process-level side effects of its own -- it raises the recursion limit, clears its own module-level caches on each call, and can emit warnings to stderr -- but nothing it does is visible to another stage except through the dict/graph it returns. Most stages are a single function; `analyze.py` and `export.py` are sets of sibling functions rather than one entry point. ## Module responsibilities diff --git a/README.md b/README.md index ac962aea71..7d2898988a 100644 --- a/README.md +++ b/README.md @@ -871,7 +871,7 @@ is added to CI later. The Bandit and pip-audit CI steps currently use `continue-on-error`, so their findings are advisory rather than blocking. > macOS note: the test suite includes both `sample.f90` and `sample.F90` fixtures. These collide on case-insensitive HFS+ / APFS file systems. Run on Linux or in a Docker container if you need to test both Fortran variants simultaneously. - +> > Windows note: the native Windows test suite exercises symbolic links, long > paths, POSIX permissions, path separators, and UTF-8 filesystem behavior. > Enable Windows Developer Mode to allow unprivileged symbolic-link creation, or diff --git a/graphify/cache.py b/graphify/cache.py index 8fb168ce3e..f9f57d9001 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -972,11 +972,15 @@ def load_cached(path: Path, root: Path = Path("."), kind: str = "ast", if entry.exists(): try: result = json.loads(entry.read_text(encoding="utf-8")) - except json.JSONDecodeError: + except (json.JSONDecodeError, UnicodeDecodeError): # Corrupt entry, not a miss: a truncated write or a bad producer # (e.g. unescaped Windows backslashes in source_file) leaves JSON # that fails to parse on every future run, so the file is silently # re-extracted forever. Count it so the run can report it (#2405). + # UnicodeDecodeError included: read_text() can raise it before + # json.loads() ever runs, e.g. a truncated write that cuts off + # mid multi-byte UTF-8 character -- the same "corrupt, not a + # miss" case, just caught one call earlier. _corrupt_cache_entries += 1 return None except OSError: diff --git a/graphify/detect.py b/graphify/detect.py index d16b5800ce..4cfadfb38c 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -509,6 +509,27 @@ def classify_file(path: Path) -> FileType | None: from graphify.manifest_ingest import is_package_manifest_path if is_package_manifest_path(path): return FileType.CODE + # GitHub Actions workflow YAML (.github/workflows/*.yml|.yaml) has real + # structure (jobs, needs, uses) an AST pass can extract deterministically + # -- same rationale as the manifest carve-out above, and same mechanism + # (route to CODE by path before the generic DOC_EXTENSIONS bucket claims + # the .yml/.yaml extension). Also requires a cheap content sniff + # (`looks_like_workflow_shape`, a bounded-prefix regex, no tree-sitter) + # -- path alone is not enough: a non-workflow file that merely sits at + # this path (a stray Docker Compose file, ...) would otherwise be routed + # to CODE, extracted as empty by extract_github_actions(), and never + # reach the semantic pass at all, permanently losing its content rather + # than just producing a warning (real bug caught in review; + # the original path-only design assumed the extractor's own empty-result + # fallback was equivalent to a DOCUMENT classification, but CODE files + # never reach the semantic pass regardless of what the extractor + # returns). Every OTHER .yaml/.yml (Helm values, k8s manifests, OpenAPI + # specs) deliberately keeps falling through to DOCUMENT below -- + # reclassifying YAML generically would regress their existing, correct + # semantic-pass handling. + from graphify.extractors.github_actions import is_github_actions_workflow_path, looks_like_workflow_shape + if is_github_actions_workflow_path(path) and looks_like_workflow_shape(path): + return FileType.CODE # Compound extensions must be checked before simple suffix lookup if path.name.lower().endswith(".blade.php"): return FileType.CODE diff --git a/graphify/extract.py b/graphify/extract.py index 89082af878..7394bb650a 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -57,6 +57,7 @@ from graphify.extractors.sql import extract_sql # noqa: F401 from graphify.extractors.terraform import extract_terraform # noqa: F401 from graphify.extractors.verilog import extract_verilog # noqa: F401 +from graphify.extractors.github_actions import extract_github_actions # noqa: F401 from graphify.extractors.zig import extract_zig # noqa: F401 from graphify.security import sanitize_metadata from graphify.paths import disambiguate_ambiguous_candidates @@ -5228,6 +5229,8 @@ def add_existing_edge(edge: dict) -> None: ".sh": extract_bash, ".bash": extract_bash, ".json": extract_json, + ".yaml": extract_github_actions, + ".yml": extract_github_actions, ".tf": extract_terraform, ".tfvars": extract_terraform, ".hcl": extract_terraform, @@ -5255,6 +5258,8 @@ def add_existing_edge(edge: dict) -> None: # extract() to tell the user which extra restores the language. _EXTRA_FOR_EXTENSION = { ".sql": "sql", + ".yaml": "yaml", + ".yml": "yaml", ".tf": "terraform", ".tfvars": "terraform", ".hcl": "terraform", @@ -5408,6 +5413,19 @@ def _get_extractor(path: Path) -> Any | None: # mis-parsed. `.mm` is unambiguously Objective-C++ and stays on extract_objc. if suffix == ".m" and not _is_objc_source(path): return None + # `.yaml`/`.yml`: extract_github_actions() only makes sense for a real + # GitHub Actions workflow. Gating here (not just in _DISPATCH) matters + # for callers that reach extract() directly (collect_files() collects + # every .yaml/.yml in a tree, not just workflow-shaped ones -- a stray + # docker-compose.yaml anywhere would otherwise dispatch to + # extract_github_actions, return empty, and get misreported as a failed/ + # empty extraction rather than "no extractor for this file"). Content-shape + # checking mirrors classify_file()'s own gate + # (is_github_actions_workflow_path + looks_like_workflow_shape). + if suffix in (".yaml", ".yml"): + from graphify.extractors.github_actions import is_github_actions_workflow_path, looks_like_workflow_shape + if not (is_github_actions_workflow_path(path) and looks_like_workflow_shape(path)): + return None # Extensionless files: resolve by shebang, mirroring detect.classify_file. # Without this, detect labels e.g. `#!/usr/bin/env bash` CLIs as code but # extraction returns no extractor and the file silently contributes nothing. diff --git a/graphify/extractors/github_actions.py b/graphify/extractors/github_actions.py new file mode 100644 index 0000000000..fec6c8ba24 --- /dev/null +++ b/graphify/extractors/github_actions.py @@ -0,0 +1,364 @@ +"""GitHub Actions workflow extractor. + +Scoped to GitHub Actions workflow YAML only (job nodes, ``needs``/``uses`` +edges). Adapted from the tree-sitter-yaml traversal helpers and +workflow-shape extraction logic in Graphify-Labs/graphify PR #2541 +(unmerged as of this writing), with attribution rather than a blind copy. +That PR also models Docker Compose services under the same extractor; the +Compose branch is deliberately not ported here -- this fork's need is +GitHub Actions specifically, and folding in a second, unrelated shape would +widen the surface this file has to stay correct for with no requirement to +justify it, plus that PR keeps YAML entirely in DOC_EXTENSIONS (registering +an extractor alone doesn't touch classification), so it never actually +solves running under ``graphify extract --code-only`` -- the reason this +file exists is to combine the extractor with a ``detect.classify_file`` +carve-out (see ``is_github_actions_workflow_path`` below and its use in +``graphify/detect.py``) that makes recognized workflow YAML a code-equivalent +input, not just add a semantic-pass extractor. +""" +from __future__ import annotations + +import re +from pathlib import Path + +from graphify.extractors.base import _file_stem, _make_id + +_JOBS_KEY_RE = re.compile(rb"(?m)^jobs\s*:") + + +def is_github_actions_workflow_path(path: Path) -> bool: + """True if `path` sits directly inside a `.github/workflows/` directory + with a .yml/.yaml extension. + + This is GitHub's own rule for what it treats as a workflow definition, + valid or not (workflow files must live directly in `.github/workflows/`, + not nested deeper) -- so it is a precise signal usable at classify_file() + time. See `looks_like_workflow_shape` for the accompanying content check + -- path alone is not enough (a non-workflow file can sit at this path + too, e.g. a stray Docker Compose file). + """ + if path.suffix.lower() not in (".yml", ".yaml"): + return False + return path.parent.name == "workflows" and path.parent.parent.name == ".github" + + +def looks_like_workflow_shape(path: Path) -> bool: + """Cheap, tree-sitter-free content sniff: does the file have a top-level + `jobs:` key? + + Used by classify_file() alongside `is_github_actions_workflow_path` so a + file that merely *sits* in `.github/workflows/` but isn't actually + workflow-shaped (a stray Docker Compose file, a schema doc, ...) falls + through to DOCUMENT instead of being routed to CODE, extracted as empty + by `extract_github_actions`, and then never reaching the semantic pass + at all -- a real content-loss bug (the original design deferred all + content validation to the extractor, which only prevents a *misclassified* + file from producing garbage nodes, not from being misclassified in the + first place). Deliberately a plain regex + over a bounded byte prefix rather than a full tree-sitter parse: unlike + `extract_github_actions`, classify_file() must keep working without the + optional `[yaml]` extra installed, and this only needs to answer "is + this even shaped like a workflow", not build real nodes/edges from it. + """ + try: + with path.open("rb") as fh: + head = fh.read(65536) + except OSError: + return False + return _JOBS_KEY_RE.search(head) is not None + + +# Step/job keys that carry a reference to another action or reusable workflow +# rather than a shell command. +_USES_KEYS = frozenset({"uses"}) + +_MAPPING_TYPES = frozenset({"block_mapping", "flow_mapping"}) +_SEQUENCE_TYPES = frozenset({"block_sequence", "flow_sequence"}) + + +def _descend(node, wanted: frozenset[str]): + """Return the first descendant of *node* whose type is in *wanted*. + + YAML wraps every value in `block_node`/`flow_node` before the actual + collection, and a document adds another layer, so callers would otherwise + repeat the same two-or-three-step unwrap everywhere. + """ + if node is None: + return None + if node.type in wanted: + return node + for child in node.children: + if not child.is_named: + continue + if child.type in ("block_node", "flow_node", "document"): + found = _descend(child, wanted) + if found is not None: + return found + elif child.type in wanted: + return child + return None + + +def _mapping(node): + return _descend(node, _MAPPING_TYPES) + + +def _scalar_text(node) -> str: + """Text of the scalar at *node*, with one layer of quotes stripped.""" + if node is None: + return "" + text = node.text.decode("utf-8", errors="replace").strip() + if len(text) >= 2 and text[0] == text[-1] and text[0] in ("'", '"'): + text = text[1:-1] + return text.strip() + + +def _pairs(node): + """Yield `(key, value_node, line)` for each pair of the mapping at *node*. + + *node* may be the mapping itself or any wrapper around it. Pairs whose key + is not a plain scalar (rare -- a complex mapping key) are skipped rather + than stringified, so they never mint a garbage node. + """ + mapping = _mapping(node) + if mapping is None: + return + for pair in mapping.children: + if pair.type not in ("block_mapping_pair", "flow_pair"): + continue + key_node = pair.child_by_field_name("key") + if key_node is None: + continue + key = _scalar_text(key_node) + if not key: + continue + yield key, pair.child_by_field_name("value"), key_node.start_point[0] + 1 + + +def _item_value(item): + """The value inside a `block_sequence_item`, without the `- ` marker. + + `item.text` spans the marker too, so reading it directly yields `"- api"` + where the real value is `api`. + """ + if item.type != "block_sequence_item": + return item + for child in item.children: + if child.is_named: + return child + return item + + +def _string_items(node) -> list[tuple[str, int]]: + """Scalars reachable from *node* as `(text, line)`. + + Handles the shapes a `needs`/`uses` value takes: a bare scalar + (`needs: lint`), a sequence (`needs: [lint, test]` or the block-list + form), or (defensively) a mapping's keys. + """ + if node is None: + return [] + seq = _descend(node, _SEQUENCE_TYPES) + if seq is not None: + items = [] + for item in seq.children: + if item.type not in ("block_sequence_item", "flow_node"): + continue + text = _scalar_text(_item_value(item)) + # A sequence item wrapping a mapping is a step, not a name. + if text and "\n" not in text and ":" not in text: + items.append((text, item.start_point[0] + 1)) + return items + mapping = _mapping(node) + if mapping is not None: + return [(key, line) for key, _value, line in _pairs(mapping)] + text = _scalar_text(node) + return [(text, node.start_point[0] + 1)] if text else [] + + +def _sequence_items(node): + """Yield the item nodes of the sequence at *node* (for step lists).""" + seq = _descend(node, _SEQUENCE_TYPES) + if seq is None: + return + for item in seq.children: + if item.type in ("block_sequence_item", "flow_node"): + yield item + + +def _top_level(root): + """The document's top-level mapping, or None when the file is not a mapping.""" + for doc in root.children: + if doc.type != "document": + continue + mapping = _mapping(doc) + if mapping is not None: + return mapping + return _mapping(root) + + +def _is_workflow(path: Path, top) -> bool: + """True if *top* (the file's top-level mapping) looks like a GitHub + Actions workflow: a `jobs:` mapping, plus either an `on:` key or the file + living in `.github/workflows/`. `jobs:` alone is too generic a key to + trust on its own (other tools use it too); requiring `on:` in addition + handles a file recognized purely by content, while the path check covers + a file scanned mid-edit that's momentarily missing `on:` but is + unambiguously a workflow by where it lives.""" + if top is None: + return False + keys = {key for key, _value, _line in _pairs(top)} + return "jobs" in keys and ("on" in keys or is_github_actions_workflow_path(path)) + + +def extract_github_actions(path: Path) -> dict: + """Extract job nodes and `needs`/`uses` edges from a GitHub Actions + workflow YAML file via tree-sitter. + + Nodes: one per job, plus sourceless stub nodes for the actions/reusable + workflows referenced via `uses`. Edges: `contains` (file -> job), + `depends_on` (`needs`, scalar or list form), `uses` (job-level or + step-level, to an action/reusable workflow). + + Job definitions are file-scoped (`_make_id(stem, name)`) with a + `contains` edge from the file node. `uses` targets are sourceless stubs + (`_make_id(name)`, no `contains`) marked `type=module` -- the same + module-anchor exemption tree-sitter extractors elsewhere use (#1327) -- + so `actions/checkout@v4` pinned by ten workflows collapses into one hub + node under `_disambiguate_colliding_node_ids` instead of scattering into + ten path-salted duplicates. + + Any YAML that doesn't look like a workflow (`_is_workflow` returns + False -- Helm values, k8s manifests, OpenAPI specs, or an unrelated file + that happens to sit in `.github/workflows/`) returns an empty result and + is left to the semantic pass, mirroring how `_is_config_json` leaves data + JSON alone (#1224). + """ + _YAML_MAX_BYTES = 1_048_576 # 1 MiB -- workflow files are small; this rejects junk + + try: + import tree_sitter_yaml as tsyaml + from tree_sitter import Language, Parser + except ImportError as e: + import importlib.util + # An installed-but-broken grammar (e.g. a C extension built for a + # different Python ABI, #2602) raises ImportError here too, same as + # extractors/sql.py's identical distinction. Reporting that as "not + # installed" sends the user to a no-op `pip install`, so check + # whether the module actually resolves before deciding which error + # to surface. + if importlib.util.find_spec("tree_sitter_yaml") is None: + return {"nodes": [], "edges": [], "error": "tree_sitter_yaml not installed. Run: pip install tree-sitter-yaml"} + return {"nodes": [], "edges": [], "error": f"tree_sitter_yaml is installed but failed to load: {e}"} + + try: + language = Language(tsyaml.language()) + parser = Parser(language) + except Exception as e: + # Same "installed but broken" case as the ImportError branch above, + # just raised one call later (e.g. a tree-sitter ABI version + # mismatch surfaces here, not at import time) -- keep the same + # marker so extract.py's #1745 dependency warning classifies it + # correctly instead of treating it as some other extraction error. + return {"nodes": [], "edges": [], "error": f"tree_sitter_yaml is installed but failed to load: {e}"} + + try: + with path.open("rb") as fh: + source = fh.read(_YAML_MAX_BYTES + 1) + if len(source) > _YAML_MAX_BYTES: + return {"nodes": [], "edges": [], "error": "yaml file too large to index"} + tree = parser.parse(source) + root = tree.root_node + except Exception as e: + return {"nodes": [], "edges": [], "error": str(e)} + + top = _top_level(root) + if not _is_workflow(path, top): + return {"nodes": [], "edges": []} + + str_path = str(path) + stem = _file_stem(path) + file_nid = _make_id(str_path) + + nodes: list[dict] = [{"id": file_nid, "label": path.name, "file_type": "code", + "source_file": str_path, "source_location": None}] + edges: list[dict] = [] + seen_ids: set[str] = {file_nid} + seen_edges: set[tuple[str, str, str]] = set() + # name -> nid for the jobs defined in THIS file, so a local `needs` + # reference binds to the real node instead of minting a stub next to it. + local_nids: dict[str, str] = {} + + def _add_job(name: str, line: int) -> str: + nid = _make_id(stem, name) + if nid not in seen_ids: + seen_ids.add(nid) + nodes.append({"id": nid, "label": name, "file_type": "code", + "source_file": str_path, "source_location": f"L{line}"}) + edges.append({"source": file_nid, "target": nid, "relation": "contains", + "confidence": "EXTRACTED", "source_file": str_path, + "source_location": f"L{line}", "weight": 1.0}) + local_nids[name] = nid + return nid + + def _ref_stub(name: str) -> str: + nid = _make_id(name) + if nid not in seen_ids: + seen_ids.add(nid) + # `actions/checkout@v4` referenced by ten workflows is ONE action, + # not ten same-named symbols -- the module-anchor case + # _disambiguate_colliding_node_ids is explicitly exempt from + # (#1327). Without the exemption each workflow's stub gets salted + # with its own path and the shared action scatters into N nodes + # instead of becoming the hub that makes "who uses this action" + # answerable. + nodes.append({"id": nid, "label": name, "file_type": "code", + "source_file": "", "source_location": "", + "origin_file": str_path, "type": "module"}) + return nid + + def _add_edge(src: str, name: str, relation: str, line: int) -> None: + tgt = local_nids.get(name) or _ref_stub(name) + if src == tgt: + return + key = (src, tgt, relation) + if key in seen_edges: + return + seen_edges.add(key) + edges.append({"source": src, "target": tgt, "relation": relation, + "confidence": "EXTRACTED", "source_file": str_path, + "source_location": f"L{line}", "weight": 1.0}) + + jobs_entries = [(key, value, line) for key, value, line in _pairs(top) if key == "jobs"] + if not jobs_entries: + return {"nodes": nodes, "edges": edges} + + # Pass 1: every job definition first, so a forward reference (a job that + # `needs` one declared later in the file) binds locally instead of + # minting a stub that would then compete with the real node. + members = [(name, body, line) for _k, value, _l in jobs_entries + for name, body, line in _pairs(value)] + for name, _body, line in members: + _add_job(name, line) + + # Pass 2: the references. + for name, body, _line in members: + owner = local_nids[name] + for key, value, line in _pairs(body): + if key == "needs": + for dep, dep_line in _string_items(value): + _add_edge(owner, dep, "depends_on", dep_line) + elif key in _USES_KEYS: + # Job-level `uses:` -- a reusable workflow call. + target = _scalar_text(value) + if target: + _add_edge(owner, target, "uses", line) + elif key == "steps": + for item in _sequence_items(value): + for step_key, step_value, step_line in _pairs(item): + if step_key in _USES_KEYS: + step_target = _scalar_text(step_value) + if step_target: + _add_edge(owner, step_target, "uses", step_line) + + return {"nodes": nodes, "edges": edges} diff --git a/pyproject.toml b/pyproject.toml index 237ed43418..c596ffd351 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,6 +75,12 @@ gemini = ["openai", "tiktoken"] openai = ["openai", "tiktoken"] chinese = ["jieba"] sql = ["tree-sitter-sql"] +# extract_github_actions() models GitHub Actions workflow job/needs/uses +# structure. Recognized workflow YAML is routed to FileType.CODE +# (graphify/detect.py), so without this extra those files hit the #1745 +# missing-dependency warning rather than silently degrading to the semantic +# pass the way unrecognized/data YAML still does. +yaml = ["tree-sitter-yaml"] # extract_pascal() uses tree-sitter-pascal for AST-quality extraction (more # accurate calls/inherits edges) and falls back to a regex extractor when it is # absent (#781), so this stays optional. Unlike tree-sitter-dm below, it ships @@ -91,7 +97,7 @@ ocaml = ["tree-sitter-ocaml"] # tree-sitter-commonlisp ships prebuilt abi3 wheels for every platform; optional # because Common Lisp is a niche corpus language. commonlisp = ["tree-sitter-commonlisp"] -all = ["mcp>=1,<3", "starlette>=1.3.1,<2", "neo4j", "falkordb", "pypdf>=6.12.0", "markdownify", "watchdog", "graspologic; python_version < '3.13'", "python-docx", "openpyxl", "faster-whisper; python_version >= '3.11'", "yt-dlp>=2026.6.9", "matplotlib", "numpy>=2.0; python_version >= '3.13'", "openai", "tiktoken", "boto3", "anthropic", "tree-sitter-sql", "jieba", "tree-sitter-dm", "tree-sitter-hcl", "tree-sitter-pascal", "tree-sitter-ocaml", "tree-sitter-commonlisp"] +all = ["mcp>=1,<3", "starlette>=1.3.1,<2", "neo4j", "falkordb", "pypdf>=6.12.0", "markdownify", "watchdog", "graspologic; python_version < '3.13'", "python-docx", "openpyxl", "faster-whisper; python_version >= '3.11'", "yt-dlp>=2026.6.9", "matplotlib", "numpy>=2.0; python_version >= '3.13'", "openai", "tiktoken", "boto3", "anthropic", "tree-sitter-sql", "tree-sitter-yaml", "jieba", "tree-sitter-dm", "tree-sitter-hcl", "tree-sitter-pascal", "tree-sitter-ocaml", "tree-sitter-commonlisp"] [project.scripts] graphify = "graphify.__main__:main" diff --git a/tests/test_cache.py b/tests/test_cache.py index f01a1cd295..de29faeeaa 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -585,29 +585,6 @@ def test_ast_cache_invalidated_on_version_bump(tmp_path, monkeypatch): ) -def test_ast_cache_schema_rejects_same_version_legacy_collision( - tmp_path, monkeypatch -): - """A key-schema change must not replay a poisoned same-version AST entry.""" - import json - import graphify.cache as cache_mod - - target = tmp_path / "real.py" - target.write_text("value = 1\n") - monkeypatch.setattr(cache_mod, "_EXTRACTOR_VERSION", "0.9.46", raising=False) - old_dir = tmp_path / cache_mod._GRAPHIFY_OUT / "cache" / "ast" / "v0.9.46" - old_dir.mkdir(parents=True) - old_hash = file_hash(target, tmp_path) - (old_dir / f"{old_hash}.json").write_text(json.dumps({ - "nodes": [{"id": "alias", "source_file": "alias.py"}], - "edges": [], - })) - monkeypatch.setattr(cache_mod, "_cleaned_ast_dirs", set(), raising=False) - - assert load_cached(target, root=tmp_path, kind="ast") is None - assert not old_dir.exists() - - def test_ast_cache_version_bump_cleans_stale_entries(tmp_path, monkeypatch): """Upgrading removes AST entries left behind by previous versions so the cache directory does not grow one full copy per release.""" @@ -705,246 +682,6 @@ def test_save_cached_in_root_symlink_keeps_symlink_name(tmp_path): ) -def test_file_hash_distinguishes_walked_symlink_paths_portably( - requires_symlinks, tmp_path -): - """Aliases of one target need separate portable extraction-cache keys.""" - from graphify import cache as cache_mod - - _reset_stat_index() - hashes_by_root = [] - for dirname in ("repo_a", "repo_b"): - root = tmp_path / dirname - (root / "sub").mkdir(parents=True) - target = root / "real.py" - target.write_text("def value():\n return 1\n") - aliases = (root / "alias.py", root / "sub" / "link.py") - aliases[0].symlink_to(target) - aliases[1].symlink_to(target) - - hashes = tuple(file_hash(path, root) for path in (target, *aliases)) - assert len(set(hashes)) == 3 - assert len(cache_mod._stat_index[str(target.resolve())]["hashes"]) == 3 - hashes_by_root.append(hashes) - - assert hashes_by_root[0] == hashes_by_root[1] - - -def test_file_hash_keeps_resolved_fallback_for_external_symlink( - requires_symlinks, tmp_path -): - """An out-of-root target retains the existing resolved-path identity.""" - _reset_stat_index() - root = tmp_path / "repo" - root.mkdir() - target = tmp_path / "external.py" - target.write_text("external = True\n") - alias = root / "external.py" - alias.symlink_to(target) - - assert file_hash(alias, root) == file_hash(target, root) - - -def test_warm_cache_keeps_target_and_symlink_sources_distinct( - requires_symlinks, tmp_path, monkeypatch -): - """#2832: a warm cache must not move target nodes onto its symlink.""" - from collections import Counter - - import graphify.extract as extract_mod - - _reset_stat_index() - physical_root = tmp_path / "repo" - (physical_root / "sub").mkdir(parents=True) - target = physical_root / "real.py" - target.write_text("def value():\n return 1\n") - alias = physical_root / "sub" / "link.py" - alias.symlink_to(target) - root = tmp_path / "scan" - root.symlink_to(physical_root, target_is_directory=True) - - paths = extract_mod.collect_files(root) - assert [path.relative_to(root).as_posix() for path in paths] == [ - "real.py", - "sub/link.py", - ] - - cold = extract_mod.extract(paths, cache_root=root, root=root, parallel=False) - misses = [] - real_extract = extract_mod._safe_extract_with_xaml_root - - def counting_extract(extractor, path, extract_root): - misses.append(path) - return real_extract(extractor, path, extract_root) - - monkeypatch.setattr(extract_mod, "_safe_extract_with_xaml_root", counting_extract) - warm = extract_mod.extract(paths, cache_root=root, root=root, parallel=False) - - assert misses == [] - cold_counts = Counter(n.get("source_file") for n in cold["nodes"]) - warm_counts = Counter(n.get("source_file") for n in warm["nodes"]) - assert warm_counts == cold_counts - assert len(cold_counts) == 2 - assert {Path(source).name for source in cold_counts} == {"real.py", "link.py"} - - -def test_semantic_cache_self_heals_legacy_symlink_collision( - requires_symlinks, tmp_path -): - """A poisoned legacy entry misses once, then walked groups round-trip.""" - import json - - from graphify.cache import check_semantic_cache, save_semantic_cache - - _reset_stat_index() - physical_root = tmp_path / "repo" - physical_root.mkdir() - (physical_root / "real.md").write_text("# Shared\n") - (physical_root / "alias.md").symlink_to(physical_root / "real.md") - root = tmp_path / "scan" - root.symlink_to(physical_root, target_is_directory=True) - target = root / "real.md" - alias = root / "alias.md" - - legacy_hash = file_hash(target, root) - legacy_entry = cache_dir(root, "semantic") / f"{legacy_hash}.json" - legacy_entry.write_text(json.dumps({ - "nodes": [{"id": "alias-old", "source_file": "alias.md"}], - "edges": [], - })) - - nodes, _, _, uncached = check_semantic_cache( - [str(target), str(alias)], root=root - ) - assert nodes == [] - assert uncached == [str(target), str(alias)] - - saved = save_semantic_cache( - [ - {"id": "real", "source_file": str(target)}, - {"id": "alias", "source_file": str(alias)}, - ], - [], - root=root, - ) - stored_sources = [] - for path in (target, alias): - entry = cache_dir(root, "semantic") / f"{file_hash(path, root)}.json" - stored_sources.append(json.loads(entry.read_text())["nodes"][0]["source_file"]) - nodes, _, _, uncached = check_semantic_cache( - [str(target), str(alias)], root=root - ) - - assert saved == 2 - assert stored_sources == ["real.md", "alias.md"] - assert [node["id"] for node in nodes] == ["real", "alias"] - assert uncached == [] - - -def test_semantic_symlink_policy_uses_walked_identity( - requires_symlinks, tmp_path -): - """Alias authorization and partial state must not leak to its target.""" - from graphify.cache import load_cached, save_semantic_cache - - _reset_stat_index() - target = tmp_path / "real.md" - target.write_text("# Shared\n") - alias = tmp_path / "alias.md" - alias.symlink_to(target) - - with pytest.warns(RuntimeWarning, match="out-of-scope source_file 'alias.md'"): - saved = save_semantic_cache( - [ - {"id": "real", "source_file": "real.md"}, - {"id": "alias", "source_file": "alias.md"}, - ], - [], - root=tmp_path, - allowed_source_files=[target], - partial_source_files=[alias], - ) - - target_entry = load_cached( - target, root=tmp_path, kind="semantic", allow_partial=True - ) - assert saved == 1 - assert target_entry is not None - assert target_entry.get("partial") is not True - assert load_cached(alias, root=tmp_path, kind="semantic") is None - - -def test_semantic_symlink_root_accepts_resolved_policy_paths_without_alias_leak( - requires_symlinks, tmp_path -): - """Resolved root spellings apply only to the matching walked identity.""" - from graphify.cache import load_cached, save_semantic_cache - - _reset_stat_index() - physical_root = tmp_path / "repo" - physical_root.mkdir() - target = physical_root / "real.md" - target.write_text("# Shared\n") - alias = physical_root / "alias.md" - alias.symlink_to(target) - root = tmp_path / "scan" - root.symlink_to(physical_root, target_is_directory=True) - walked_target = root / "real.md" - walked_alias = root / "alias.md" - - with pytest.warns(RuntimeWarning, match="out-of-scope source_file"): - saved = save_semantic_cache( - [ - {"id": "real", "source_file": str(walked_target)}, - {"id": "alias", "source_file": str(walked_alias)}, - ], - [], - root=root, - allowed_source_files=[walked_target.resolve()], - partial_source_files=[walked_target.resolve()], - ) - - target_entry = load_cached( - walked_target, root=root, kind="semantic", allow_partial=True - ) - assert saved == 1 - assert target_entry is not None - assert [node["id"] for node in target_entry["nodes"]] == ["real"] - assert target_entry["partial"] is True - assert load_cached(walked_target, root=root, kind="semantic") is None - assert load_cached(walked_alias, root=root, kind="semantic") is None - - -def test_semantic_symlink_root_keeps_external_policy_path_absolute( - requires_symlinks, tmp_path -): - """An allowed absolute source outside a symlinked root stays external.""" - from graphify.cache import load_cached, save_semantic_cache - - _reset_stat_index() - physical_root = tmp_path / "repo" - physical_root.mkdir() - root = tmp_path / "scan" - root.symlink_to(physical_root, target_is_directory=True) - external = tmp_path / "external.md" - external.write_text("# External\n") - - saved = save_semantic_cache( - [{"id": "external", "source_file": str(external)}], - [], - root=root, - allowed_source_files=[external], - partial_source_files=[external], - ) - - entry = load_cached(external, root=root, kind="semantic", allow_partial=True) - assert saved == 1 - assert entry is not None - assert Path(entry["nodes"][0]["source_file"]) == external - assert entry["partial"] is True - assert load_cached(external, root=root, kind="semantic") is None - - def test_semantic_prune_removes_orphan_entries(tmp_path): """Changing a file's content leaves the old content-hash entry orphaned; pruning against the new live hash removes the stale entry and keeps the @@ -1751,240 +1488,65 @@ def test_corrupt_semantic_entry_warns_and_is_a_miss(tmp_path): assert uncached == [str(f)] -# --- #2927: zero-node semantic cache rejection and healing ------------------- - -def test_edge_only_semantic_result_not_cached(tmp_path): - """#2927: an edge-only semantic result (0 nodes, 0 hyperedges) represents an - omission by the model and must NOT be written to cache, so subsequent runs - can re-dispatch and retry the file (#933/#1666).""" - from graphify.cache import check_semantic_cache, load_cached, save_semantic_cache - - f = tmp_path / "doc.md" - f.write_text("# Architecture\nSome prose.\n", encoding="utf-8") - edges = [{"source": "auth_a", "target": "auth_b", "source_file": "doc.md"}] - - saved = save_semantic_cache([], edges, root=tmp_path, prompt="PROMPT V1") - assert saved == 0, "edge-only result must not be saved to cache" - - # load_cached must return None (miss) - assert load_cached(f, root=tmp_path, kind="semantic", prompt="PROMPT V1") is None - # check_semantic_cache must treat it as uncached - nodes, edges_out, hyper_out, uncached = check_semantic_cache([str(f)], root=tmp_path, prompt="PROMPT V1") - assert nodes == [] and edges_out == [] and hyper_out == [] - assert uncached == [str(f)] - - -def test_node_only_and_node_edge_semantic_results_cached(tmp_path): - """Normal extractions (nodes-only and nodes+edges) continue to cache normally.""" - from graphify.cache import load_cached, save_semantic_cache - - f1 = tmp_path / "doc1.md" - f1.write_text("# Doc 1\n", encoding="utf-8") - f2 = tmp_path / "doc2.md" - f2.write_text("# Doc 2\n", encoding="utf-8") - - # Node-only - saved1 = save_semantic_cache([{"id": "n1", "source_file": "doc1.md"}], [], root=tmp_path, prompt="P") - assert saved1 == 1 - loaded1 = load_cached(f1, root=tmp_path, kind="semantic", prompt="P") - assert loaded1 is not None and len(loaded1["nodes"]) == 1 - - # Node + edge - saved2 = save_semantic_cache( - [{"id": "n2", "source_file": "doc2.md"}], - [{"source": "n2", "target": "n2", "source_file": "doc2.md"}], - root=tmp_path, - prompt="P", - ) - assert saved2 == 1 - loaded2 = load_cached(f2, root=tmp_path, kind="semantic", prompt="P") - assert loaded2 is not None and len(loaded2["nodes"]) == 1 and len(loaded2["edges"]) == 1 - - -def test_hyperedge_only_semantic_result_cached(tmp_path): - """#1920: hyperedge-only documents are valid semantic output and must be cached.""" - from graphify.cache import check_semantic_cache, load_cached, save_semantic_cache - - f = tmp_path / "hyper.md" - f.write_text("# Pipeline Concept\n", encoding="utf-8") - hyperedges = [ - {"id": "h1", "label": "Pipeline", "nodes": ["a", "b", "c"], "source_file": "hyper.md"} - ] - - saved = save_semantic_cache([], [], hyperedges, root=tmp_path, prompt="PROMPT V1") - assert saved == 1, "hyperedge-only result must be saved to cache (#1920)" - - loaded = load_cached(f, root=tmp_path, kind="semantic", prompt="PROMPT V1") - assert loaded is not None - assert len(loaded["hyperedges"]) == 1 - - _, _, cached_hyper, uncached = check_semantic_cache([str(f)], root=tmp_path, prompt="PROMPT V1") - assert uncached == [] - assert len(cached_hyper) == 1 - - -def test_poisoned_edge_only_cache_entry_treated_as_miss(tmp_path): - """#2927 healing: a legacy on-disk cache entry containing edges but no nodes - or hyperedges must be rejected by load_cached as a cache MISS.""" - import json - from graphify.cache import cache_dir, file_hash, load_cached, prompt_fingerprint - - f = tmp_path / "poisoned.md" - f.write_text("# Poisoned\n", encoding="utf-8") - - # Manually seed a legacy poisoned cache file (nodes: [], edges: [...]) - prompt = "PROMPT V1" - fp = prompt_fingerprint(prompt) - cdir = cache_dir(tmp_path, "semantic", fp) - cdir.mkdir(parents=True, exist_ok=True) - h = file_hash(f, tmp_path) - (cdir / f"{h}.json").write_text( - json.dumps({ - "nodes": [], - "edges": [{"source": "x", "target": "y", "source_file": "poisoned.md"}], - "hyperedges": [], - }), - encoding="utf-8", +def test_invalid_utf8_semantic_entry_warns_and_is_a_miss(tmp_path): + """Same corrupt-entry handling as test_corrupt_semantic_entry_warns_and_is_a_miss, + but for bytes that fail to *decode* rather than parse -- read_text() raises + UnicodeDecodeError before json.loads() ever runs (e.g. a truncated write + that cuts off mid multi-byte UTF-8 character), so it must be caught + alongside JSONDecodeError or the corruption is never counted/reported and + check_semantic_cache blows up instead of treating it as a miss.""" + from graphify.cache import ( + check_semantic_cache, + save_semantic_cache, + cache_dir, ) - # load_cached must reject the poisoned entry - assert load_cached(f, root=tmp_path, kind="semantic", prompt=prompt) is None - - -def test_existing_hyperedge_only_cache_entry_remains_hit(tmp_path): - """#1920 / #2927: an existing on-disk cache entry with hyperedges but no nodes - remains a valid cache hit.""" - import json - from graphify.cache import cache_dir, file_hash, load_cached, prompt_fingerprint - - f = tmp_path / "valid_hyper.md" - f.write_text("# Hyper\n", encoding="utf-8") + f = tmp_path / "doc.md" + f.write_text("# Doc\n\nBody.\n") + save_semantic_cache([{"id": "n", "source_file": "doc.md"}], [], root=tmp_path) - prompt = "PROMPT V1" - fp = prompt_fingerprint(prompt) - cdir = cache_dir(tmp_path, "semantic", fp) - cdir.mkdir(parents=True, exist_ok=True) h = file_hash(f, tmp_path) - (cdir / f"{h}.json").write_text( - json.dumps({ - "nodes": [], - "edges": [], - "hyperedges": [{"id": "h1", "label": "Group", "nodes": ["a", "b", "c"], "source_file": "valid_hyper.md"}], - }), - encoding="utf-8", - ) - - loaded = load_cached(f, root=tmp_path, kind="semantic", prompt=prompt) - assert loaded is not None - assert len(loaded["hyperedges"]) == 1 -# --- #2926: graph-side scope filter ------------------------------------------- -# The #1757 guard protects the cache write, but the unfiltered fresh result -# also feeds build_merge(), whose replace-set logic swaps a non-dispatched -# file's entire prior contribution for a stray fragment. scope_semantic_result -# applies the same allowlist to the result dict before it reaches the merge. - -def test_scope_semantic_result_drops_out_of_scope_groups(tmp_path): - """Stray items attributed to a non-dispatched file are removed; allowed - and source-less items pass through.""" - from graphify.cache import scope_semantic_result - - result = { - "nodes": [ - {"id": "kept", "source_file": "intended.md"}, - {"id": "stray", "source_file": "protected.md"}, - {"id": "phantom", "source_file": "src/foo.ts"}, # nonexistent path - {"id": "no_source"}, # no source_file: passes through - ], - "edges": [ - {"source": "kept", "target": "other", "source_file": "intended.md"}, - {"source": "stray", "target": "kept", "source_file": "protected.md"}, - ], - "hyperedges": [ - {"id": "h_kept", "nodes": ["kept"], "source_file": "intended.md"}, - {"id": "h_stray", "nodes": ["stray"], "source_file": "protected.md"}, - ], - } - - dropped_files, dropped_items = scope_semantic_result( - result, root=tmp_path, allowed_source_files=["intended.md"], - ) + entry = cache_dir(tmp_path, "semantic") / f"{h}.json" + assert entry.exists() + entry.write_bytes(b'{"nodes": [' + b"\xff\xfe") - assert [n["id"] for n in result["nodes"]] == ["kept", "no_source"] - assert [e["source"] for e in result["edges"]] == ["kept"] - assert [h["id"] for h in result["hyperedges"]] == ["h_kept"] - assert dropped_files == {"protected.md", "src/foo.ts"} - assert dropped_items == 4 # 2 stray nodes + 1 stray edge + 1 stray hyperedge + with pytest.warns(RuntimeWarning, match="corrupt"): + nodes, _, _, uncached = check_semantic_cache([str(f)], root=tmp_path) + assert nodes == [] + assert uncached == [str(f)] + with pytest.warns(RuntimeWarning, match="corrupt"): + nodes, _, _, uncached = check_semantic_cache([str(f)], root=tmp_path) -def test_scope_semantic_result_matches_absolute_and_relative_forms(tmp_path): - """An absolute in-root source_file and its relative form are the same - identity — both must match the allowlist entry (#2197 normalization).""" - from graphify.cache import scope_semantic_result + # The corrupt entry is a miss, so the file is re-dispatched for extraction. + assert nodes == [] + assert uncached == [str(f)] - result = { - "nodes": [ - {"id": "abs", "source_file": str(tmp_path / "doc.md")}, - {"id": "rel", "source_file": "doc.md"}, - ], - "edges": [], - "hyperedges": [], - } - dropped_files, dropped_items = scope_semantic_result( - result, root=tmp_path, allowed_source_files=["doc.md"], +def test_invalid_utf8_semantic_entry_warns_and_is_a_miss(tmp_path): + """Same corrupt-entry handling as test_corrupt_semantic_entry_warns_and_is_a_miss, + but for bytes that fail to *decode* rather than parse -- read_text() raises + UnicodeDecodeError before json.loads() ever runs (e.g. a truncated write + that cuts off mid multi-byte UTF-8 character), so it must be caught + alongside JSONDecodeError or the corruption is never counted/reported and + check_semantic_cache blows up instead of treating it as a miss.""" + from graphify.cache import ( + check_semantic_cache, + save_semantic_cache, + cache_dir, ) - assert [n["id"] for n in result["nodes"]] == ["abs", "rel"] - assert dropped_files == set() - assert dropped_items == 0 - - -def test_scope_semantic_result_prunes_edges_referencing_dropped_ids(tmp_path): - """#1916 mirror: an edge attributed to an ALLOWED file that references a - node id only defined by a DROPPED group would materialize that id as a - phantom node at build time; it must be dropped too. A duplicate-attribution - id (defined by both a kept and a dropped group) keeps its edges.""" - from graphify.cache import scope_semantic_result - - result = { - "nodes": [ - {"id": "kept", "source_file": "a.md"}, - {"id": "shared", "source_file": "a.md"}, # also defined by b.md - {"id": "shared", "source_file": "b.md"}, # duplicate attribution - {"id": "gone", "source_file": "c.md"}, - ], - "edges": [ - {"source": "kept", "target": "shared", "source_file": "a.md"}, - {"source": "kept", "target": "gone", "source_file": "a.md"}, - ], - "hyperedges": [ - {"id": "h1", "nodes": ["kept", "gone"], "source_file": "a.md"}, - {"id": "h2", "nodes": ["kept", "shared"], "source_file": "a.md"}, - ], - } - - scope_semantic_result(result, root=tmp_path, - allowed_source_files=["a.md"]) - - # The b.md COPY of "shared" is dropped as out-of-scope, but because a kept - # node also defines that id, references to it must NOT be pruned. - assert [n["id"] for n in result["nodes"]] == ["kept", "shared"] - assert [e["target"] for e in result["edges"]] == ["shared"] - assert [h["id"] for h in result["hyperedges"]] == ["h2"] - - -def test_scope_semantic_result_unscoped_is_a_no_op(): - """allowed_source_files=None must leave the result untouched (same contract - as save_semantic_cache's unscoped callers).""" - from graphify.cache import scope_semantic_result + f = tmp_path / "doc.md" + f.write_text("# Doc\n\nBody.\n") + save_semantic_cache([{"id": "n", "source_file": "doc.md"}], [], root=tmp_path) - result = { - "nodes": [{"id": "n", "source_file": "anywhere.md"}], - "edges": [], - "hyperedges": [], - } + h = file_hash(f, tmp_path) + entry = cache_dir(tmp_path, "semantic") / f"{h}.json" + assert entry.exists() + entry.write_bytes(b'{"nodes": [' + b"\xff\xfe") - dropped_files, dropped_items = scope_semantic_result(result, root=Path(".")) + with pytest.warns(RuntimeWarning, match="corrupt"): + nodes, _, _, uncached = check_semantic_cache([str(f)], root=tmp_path) - assert (dropped_files, dropped_items) == (set(), 0) - assert [n["id"] for n in result["nodes"]] == ["n"] + assert nodes == [] + assert uncached == [str(f)] diff --git a/tests/test_github_actions.py b/tests/test_github_actions.py new file mode 100644 index 0000000000..91fe72acdd --- /dev/null +++ b/tests/test_github_actions.py @@ -0,0 +1,336 @@ +"""Tests for the GitHub Actions extractor (graphify/extractors/github_actions.py) +and its detect.classify_file() carve-out. + +Scoped to GitHub Actions workflow YAML only -- no Docker Compose (out of this +fork's scope; see the module docstring in github_actions.py). Two concerns +are tested together since they are two halves of the same feature: +1. extract_github_actions() itself (job/needs/uses extraction). +2. classify_file() routing recognized workflow paths to FileType.CODE, which + is what makes them usable under `graphify extract --code-only` -- the + actual point of this feature, not just adding a semantic-pass extractor. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from graphify.build import build_from_json +from graphify.detect import FileType, classify_file +from graphify.extract import extract, extract_github_actions + + +def _write(tmp_path: Path, name: str, body: str) -> Path: + p = tmp_path / name + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(body, encoding="utf-8") + return p + + +def _labels(r) -> list[str]: + return [n["label"] for n in r["nodes"]] + + +def _rel_pairs(r, relation: str) -> set[tuple[str, str]]: + lab = {n["id"]: n["label"] for n in r["nodes"]} + return { + (lab.get(e["source"], e["source"]), lab.get(e["target"], e["target"])) + for e in r["edges"] + if e["relation"] == relation + } + + +@pytest.fixture(autouse=True) +def _require_grammar(): + pytest.importorskip("tree_sitter_yaml") + + +WORKFLOW = """\ +name: CI +on: + push: + branches: [main] + +jobs: + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + - run: pnpm lint + test: + needs: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + deploy: + needs: [lint, test] + uses: ./.github/workflows/release.yml +""" + + +# ── extract_github_actions() ───────────────────────────────────────────────── + +def test_workflow_jobs_become_nodes(tmp_path): + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + assert r.get("error") is None + labels = set(_labels(r)) + for expected in ("lint", "test", "deploy"): + assert expected in labels, f"missing job node {expected!r}" + + +def test_workflow_file_contains_jobs(tmp_path): + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + contains = _rel_pairs(r, "contains") + assert ("ci.yml", "lint") in contains + assert ("ci.yml", "deploy") in contains + + +def test_workflow_needs_becomes_depends_on(tmp_path): + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + deps = _rel_pairs(r, "depends_on") + assert ("test", "lint") in deps # scalar form: `needs: lint` + assert ("deploy", "lint") in deps # list form: `needs: [lint, test]` + assert ("deploy", "test") in deps + + +def test_workflow_step_uses_edges(tmp_path): + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + uses = _rel_pairs(r, "uses") + assert ("lint", "actions/checkout@v4") in uses + assert ("lint", "actions/setup-node@v4") in uses + + +def test_workflow_reusable_workflow_uses_edge(tmp_path): + # Job-level `uses:` is a reusable-workflow call, not a step. + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + assert ("deploy", "./.github/workflows/release.yml") in _rel_pairs(r, "uses") + + +def test_workflow_detected_by_path_without_on_key(tmp_path): + body = "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n" + p = _write(tmp_path, ".github/workflows/build.yml", body) + assert "build" in set(_labels(extract_github_actions(p))) + + +def test_run_steps_do_not_become_nodes(tmp_path): + # `- run: pnpm lint` is a shell command, not a reference. + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + assert not any("pnpm lint" in lbl for lbl in _labels(r)) + + +def test_no_dangling_edge_endpoints(tmp_path): + r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) + node_ids = {n["id"] for n in r["nodes"]} + for e in r["edges"]: + assert e["source"] in node_ids, f"dangling source: {e['source']}" + assert e["target"] in node_ids, f"dangling target: {e['target']}" + + +def test_shared_action_merges_across_workflows(tmp_path): + """The same action pinned by two workflows is one node, so + `actions/checkout` becomes a real hub instead of one dangling stub per + file.""" + a = _write(tmp_path, ".github/workflows/a.yml", + "on: push\njobs:\n one:\n steps:\n - uses: actions/checkout@v4\n") + b = _write(tmp_path, ".github/workflows/b.yml", + "on: push\njobs:\n two:\n steps:\n - uses: actions/checkout@v4\n") + + r = extract([a.resolve(), b.resolve()], root=tmp_path) + + checkout_ids = {n["id"] for n in r["nodes"] if n["label"] == "actions/checkout@v4"} + assert len(checkout_ids) == 1, f"expected one shared action id, got {checkout_ids}" + checkout_id = checkout_ids.pop() + + G = build_from_json({"nodes": r["nodes"], "edges": r["edges"]}) + assert G.has_node(checkout_id) + sources = {e["source"] for e in r["edges"] + if e["relation"] == "uses" and e["target"] == checkout_id} + assert len(sources) == 2, "both workflows should point at the shared action node" + + +# ── Data YAML / non-workflow YAML is deliberately not modelled ────────────── + +K8S = """\ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: api +spec: + replicas: 3 +""" + +OPENAPI = """\ +openapi: 3.0.0 +paths: + /users: + get: + summary: list users +""" + +COMPOSE = """\ +services: + api: + image: api:latest + depends_on: + - db + db: + image: postgres:16 +""" + + +@pytest.mark.parametrize("name,body", [("deploy.yaml", K8S), ("openapi.yaml", OPENAPI)]) +def test_data_yaml_returns_empty(tmp_path, name, body): + r = extract_github_actions(_write(tmp_path, name, body)) + assert r.get("error") is None + assert r["nodes"] == [] + assert r["edges"] == [] + + +def test_docker_compose_is_out_of_scope_and_returns_empty(tmp_path): + # Compose is deliberately not modelled by this extractor (out of this + # ticket's scope) -- confirms it stays with the semantic pass rather than + # silently doing something half-implemented. + r = extract_github_actions(_write(tmp_path, "docker-compose.yml", COMPOSE)) + assert r["nodes"] == [] + assert r["edges"] == [] + + +def test_jobs_key_alone_without_on_or_workflows_path_is_not_enough(tmp_path): + # `jobs:` is too generic a key to trust alone (other tools use it too); + # without `on:` and outside `.github/workflows/`, this must not be + # mistaken for a real workflow. + body = "jobs:\n something: true\n" + r = extract_github_actions(_write(tmp_path, "notes/plan.yaml", body)) + assert r["nodes"] == [] + + +def test_empty_and_comment_only_files_are_safe(tmp_path): + assert extract_github_actions(_write(tmp_path, "a.yml", "")).get("error") is None + r = extract_github_actions(_write(tmp_path, "b.yml", "# just a comment\n")) + assert r.get("error") is None + assert r["nodes"] == [] + + +# ── classify_file() carve-out: the actual --code-only fix ─────────────────── +# +# classify_file() requires BOTH a workflow path AND workflow-shaped content +# (a cheap regex sniff for a top-level `jobs:` key, see +# github_actions.looks_like_workflow_shape) -- path alone used to be enough, +# but that let a non-workflow file sitting at a workflow path get routed to +# CODE, extracted as empty, and never reach the semantic pass at all (a real +# content-loss bug, not just a missed-nodes one; caught in review). So these +# tests write real files rather than asserting on paths that don't exist on +# disk. + +def test_workflow_path_classified_as_code(tmp_path): + assert classify_file(_write(tmp_path, ".github/workflows/ci.yml", WORKFLOW)) == FileType.CODE + assert classify_file(_write(tmp_path, ".github/workflows/nightly-build.yaml", WORKFLOW)) == FileType.CODE + + +def test_workflow_path_classified_as_code_absolute(tmp_path): + p = _write(tmp_path, ".github/workflows/ci.yml", WORKFLOW) + assert classify_file(p.resolve()) == FileType.CODE + + +def test_non_workflow_yaml_at_workflow_path_is_not_reclassified(tmp_path): + # A file that merely sits in .github/workflows/ but isn't workflow-shaped + # (no `jobs:` key at all) must fall through to DOCUMENT, not CODE -- + # otherwise it is extracted as empty and never reaches the semantic pass. + p = _write(tmp_path, ".github/workflows/README.yml", "title: not a workflow\n") + assert classify_file(p) == FileType.DOCUMENT + + +def test_nested_workflows_dir_is_not_reclassified(tmp_path): + # GitHub only recognizes workflow files directly in .github/workflows/, + # not nested deeper -- so neither does this carve-out. + p = _write(tmp_path, ".github/workflows/nested/ci.yml", WORKFLOW) + assert classify_file(p) == FileType.DOCUMENT + + +def test_composite_action_yml_is_not_reclassified(tmp_path): + # .github/actions//action.yml (composite/local actions) is a + # different, unmodelled shape -- explicitly out of this ticket's scope, + # must not be swept in by a loose ".github/**/*.yml" check. + p = _write(tmp_path, ".github/actions/setup/action.yml", WORKFLOW) + assert classify_file(p) == FileType.DOCUMENT + + +def test_other_yaml_still_classified_as_document(tmp_path): + # The whole point of scoping this narrowly: Helm values, k8s manifests, + # OpenAPI specs, docker-compose.yml must keep their existing, + # correctly-working semantic-pass classification untouched. + assert classify_file(_write(tmp_path, "charts/myapp/values.yaml", "replicaCount: 1\n")) == FileType.DOCUMENT + assert classify_file(_write(tmp_path, "k8s/deployment.yaml", "apiVersion: v1\nkind: Deployment\n")) == FileType.DOCUMENT + assert classify_file(_write(tmp_path, "openapi.yaml", "openapi: 3.0.0\n")) == FileType.DOCUMENT + assert classify_file(_write(tmp_path, "docker-compose.yml", COMPOSE)) == FileType.DOCUMENT + + +def test_workflow_extracted_under_code_only_semantics(tmp_path): + """End-to-end: a file classified as CODE for a recognized workflow path + actually produces real job/needs/uses nodes via the same extract() path + --code-only calls, not just that classify_file() returns CODE in + isolation.""" + p = _write(tmp_path, ".github/workflows/ci.yml", WORKFLOW) + assert classify_file(p) == FileType.CODE + + r = extract([p.resolve()], root=tmp_path) + labels = set(_labels(r)) + for expected in ("lint", "test", "deploy"): + assert expected in labels + assert ("test", "lint") in _rel_pairs(r, "depends_on") + assert ("lint", "actions/checkout@v4") in _rel_pairs(r, "uses") + + +def test_non_workflow_yaml_gets_no_extractor(tmp_path): + # _get_extractor() must gate .yaml/.yml the same way classify_file() + # does: a file that isn't workflow-shaped (wrong path, or right path but + # wrong content) gets no extractor at all rather than being dispatched + # to extract_github_actions and misreported as a failed/empty + # extraction (review round 3). + from graphify.extract import _get_extractor + assert _get_extractor(_write(tmp_path, "docker-compose.yml", COMPOSE)) is None + assert _get_extractor(_write(tmp_path, ".github/workflows/README.yml", "title: x\n")) is None + assert _get_extractor(_write(tmp_path, ".github/workflows/ci.yml", WORKFLOW)) is extract_github_actions + + +# ── extract_github_actions(): missing vs broken grammar (#2602-style) ─────── + +def test_github_actions_reports_load_failure_not_missing(tmp_path, monkeypatch): + # Same distinction as extractors/sql.py: an installed-but-broken grammar + # (e.g. a wheel built for a different Python ABI) raises ImportError at + # import time just like an absent one. Must not claim "not installed" -- + # that sends the user to a no-op `pip install` -- but surface the real + # load exception instead. + import builtins + pytest.importorskip("tree_sitter_yaml") # find_spec must see it as installed + + _orig_import = builtins.__import__ + + def _broken_import(name, *args, **kwargs): + if name == "tree_sitter_yaml": + raise ImportError("dynamic module does not define module export function") + return _orig_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _broken_import) + err = extract_github_actions(_write(tmp_path, ".github/workflows/ci.yml", WORKFLOW)).get("error") or "" + assert "failed to load" in err + assert "dynamic module does not define module export function" in err + assert "pip install" not in err + + +def test_github_actions_reports_grammar_init_failure_as_load_failure(tmp_path, monkeypatch): + # A grammar init failure (Language()/Parser() raising, e.g. an ABI + # version mismatch surfacing one call later than the import itself) must + # get the same "failed to load" marker as an ImportError, not be + # conflated with an unrelated file-read error. + pytest.importorskip("tree_sitter_yaml") + import tree_sitter + + def _broken_language(*args, **kwargs): + raise ValueError("Incompatible Language version") + + monkeypatch.setattr(tree_sitter, "Language", _broken_language) + err = extract_github_actions(_write(tmp_path, ".github/workflows/ci.yml", WORKFLOW)).get("error") or "" + assert "failed to load" in err + assert "Incompatible Language version" in err diff --git a/tests/test_install_references.py b/tests/test_install_references.py index ed7090cca4..8daad5ead3 100644 --- a/tests/test_install_references.py +++ b/tests/test_install_references.py @@ -544,5 +544,9 @@ def test_install_from_read_only_package_dir(tmp_path, fake_bundle): assert (refs / "query.md").read_text() == "# query fragment\n" assert not (skill_dir / "references.tmp").exists() # The installed sidecar must stay writable, or the next install cannot - # rmtree it to swap in a new one. - assert os.access(refs, os.W_OK) + # rmtree it to swap in a new one. os.access(refs, os.W_OK) would pass + # this even on a read-only directory when running as root (uid 0 bypasses + # the permission bits it inspects), so probe with an actual write instead. + probe = refs / ".write-probe" + probe.write_text("", encoding="utf-8") + probe.unlink() diff --git a/tests/test_non_regular_files.py b/tests/test_non_regular_files.py index 13bc85e738..73dab48936 100644 --- a/tests/test_non_regular_files.py +++ b/tests/test_non_regular_files.py @@ -13,6 +13,7 @@ import os import socket import stat +import sys import tempfile from pathlib import Path @@ -20,6 +21,15 @@ from graphify.detect import _is_regular_file +# os.mkfifo/socket.AF_UNIX don't exist on Windows at all, and symlink() +# creation there requires Developer Mode or an elevated shell -- gate on +# actual capability rather than assuming every CI platform supports these. +_HAS_MKFIFO = hasattr(os, "mkfifo") +_HAS_AF_UNIX = hasattr(socket, "AF_UNIX") +_reason_no_mkfifo = "os.mkfifo unavailable on this platform" +_reason_no_af_unix = "socket.AF_UNIX unavailable on this platform" +_reason_no_symlink = "unprivileged symlink creation unavailable on this platform" + @pytest.fixture() def tree(): @@ -34,6 +44,7 @@ def test_regular_source_file_is_accepted(tree): assert _is_regular_file(tree / "src" / "module.py") is True +@pytest.mark.skipif(not _HAS_MKFIFO, reason=_reason_no_mkfifo) def test_fifo_is_rejected(tree): """The shape that hangs the whole run.""" fifo = tree / "src" / "pipe.py" @@ -42,6 +53,7 @@ def test_fifo_is_rejected(tree): assert _is_regular_file(fifo) is False +@pytest.mark.skipif(not _HAS_AF_UNIX, reason=_reason_no_af_unix) def test_unix_socket_is_rejected(tree): sock_path = tree / "src" / "sock.py" sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) @@ -58,6 +70,7 @@ def test_directory_named_like_a_source_file_is_rejected(tree): assert _is_regular_file(d) is False +@pytest.mark.skipif(sys.platform == "win32", reason=_reason_no_symlink) def test_symlink_to_a_regular_file_is_accepted(tree): target = tree / "src" / "module.py" link = tree / "src" / "alias.py" @@ -65,6 +78,7 @@ def test_symlink_to_a_regular_file_is_accepted(tree): assert _is_regular_file(link) is True +@pytest.mark.skipif(not _HAS_MKFIFO or sys.platform == "win32", reason=_reason_no_mkfifo) def test_symlink_pointing_at_a_fifo_is_rejected(tree): """A link to a FIFO blocks exactly like the FIFO, so stat must follow it.""" fifo = tree / "src" / "real.py" @@ -74,6 +88,7 @@ def test_symlink_pointing_at_a_fifo_is_rejected(tree): assert _is_regular_file(link) is False +@pytest.mark.skipif(sys.platform == "win32", reason=_reason_no_symlink) def test_broken_symlink_is_rejected_without_raising(tree): link = tree / "src" / "dangling.py" link.symlink_to(tree / "src" / "does-not-exist.py") diff --git a/uv.lock b/uv.lock index 09aabf3059..1a3ac362cb 100644 --- a/uv.lock +++ b/uv.lock @@ -1090,7 +1090,7 @@ wheels = [ [[package]] name = "graphifyy" -version = "0.9.49" +version = "0.9.43" source = { editable = "." } dependencies = [ { name = "networkx", version = "3.4.2", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.11'" }, @@ -1145,12 +1145,12 @@ all = [ { name = "python-docx" }, { name = "starlette" }, { name = "tiktoken" }, - { name = "tree-sitter-commonlisp" }, { name = "tree-sitter-dm" }, { name = "tree-sitter-hcl" }, { name = "tree-sitter-ocaml" }, { name = "tree-sitter-pascal" }, { name = "tree-sitter-sql" }, + { name = "tree-sitter-yaml" }, { name = "watchdog" }, { name = "yt-dlp" }, ] @@ -1163,9 +1163,6 @@ bedrock = [ chinese = [ { name = "jieba" }, ] -commonlisp = [ - { name = "tree-sitter-commonlisp" }, -] dm = [ { name = "tree-sitter-dm" }, ] @@ -1234,6 +1231,9 @@ video = [ watch = [ { name = "watchdog" }, ] +yaml = [ + { name = "tree-sitter-yaml" }, +] [package.dev-dependencies] dev = [ @@ -1250,7 +1250,6 @@ dev = [ { name = "ruff" }, { name = "setuptools" }, { name = "tomli", marker = "python_full_version < '3.11'" }, - { name = "tree-sitter-commonlisp" }, { name = "tree-sitter-hcl" }, { name = "tree-sitter-ocaml" }, { name = "wheel" }, @@ -1306,8 +1305,6 @@ requires-dist = [ { name = "tree-sitter-bash", specifier = ">=0.23,<0.27" }, { name = "tree-sitter-c", specifier = ">=0.23,<0.25" }, { name = "tree-sitter-c-sharp", specifier = ">=0.23,<0.25" }, - { name = "tree-sitter-commonlisp", marker = "extra == 'all'" }, - { name = "tree-sitter-commonlisp", marker = "extra == 'commonlisp'" }, { name = "tree-sitter-cpp", specifier = ">=0.23,<0.25" }, { name = "tree-sitter-dm", marker = "extra == 'all'" }, { name = "tree-sitter-dm", marker = "extra == 'dm'" }, @@ -1339,13 +1336,15 @@ requires-dist = [ { name = "tree-sitter-swift", specifier = ">=0.7,<0.9" }, { name = "tree-sitter-typescript", specifier = ">=0.23,<0.25" }, { name = "tree-sitter-verilog", specifier = ">=1.0,<2.0" }, + { name = "tree-sitter-yaml", marker = "extra == 'all'" }, + { name = "tree-sitter-yaml", marker = "extra == 'yaml'" }, { name = "tree-sitter-zig", specifier = ">=1.0,<2.0" }, { name = "watchdog", marker = "extra == 'all'" }, { name = "watchdog", marker = "extra == 'watch'" }, { name = "yt-dlp", marker = "extra == 'all'", specifier = ">=2026.6.9" }, { name = "yt-dlp", marker = "extra == 'video'", specifier = ">=2026.6.9" }, ] -provides-extras = ["mcp", "neo4j", "falkordb", "pdf", "watch", "svg", "leiden", "office", "google", "postgres", "video", "kimi", "ollama", "bedrock", "anthropic", "gemini", "openai", "chinese", "sql", "pascal", "dm", "terraform", "ocaml", "commonlisp", "all"] +provides-extras = ["mcp", "neo4j", "falkordb", "pdf", "watch", "svg", "leiden", "office", "google", "postgres", "video", "kimi", "ollama", "bedrock", "anthropic", "gemini", "openai", "chinese", "sql", "yaml", "pascal", "dm", "terraform", "ocaml", "all"] [package.metadata.requires-dev] dev = [ @@ -1362,7 +1361,6 @@ dev = [ { name = "ruff", specifier = ">=0.15.13" }, { name = "setuptools", specifier = ">=82.0.1" }, { name = "tomli", marker = "python_full_version < '3.11'", specifier = ">=2.0" }, - { name = "tree-sitter-commonlisp", specifier = ">=0.4.1" }, { name = "tree-sitter-hcl", specifier = ">=1.2.0" }, { name = "tree-sitter-ocaml", specifier = ">=0.25.0" }, { name = "wheel", specifier = ">=0.47.0" }, @@ -4541,21 +4539,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/7f/fb/114ff43fdd256d0befed32f77c1dadee9517867181c70794571f718ed05c/tree_sitter_c_sharp-0.23.5-cp310-abi3-win_arm64.whl", hash = "sha256:2de4ebf95ddc2e92cd3105c8a8e0e7ec646bc82f52bfaf2f3acec0fa2401ec09", size = 337260, upload-time = "2026-04-14T16:11:20.849Z" }, ] -[[package]] -name = "tree-sitter-commonlisp" -version = "0.4.1" -source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/06/c5/503a9cc9e8ca02387f4424742964242499a407fb8451501ec24e71babc8b/tree_sitter_commonlisp-0.4.1.tar.gz", hash = "sha256:4b8fc7e1ae7faf29d8f656970e25c660b13857e39b55d4a13bcee06ccf3e79c4", size = 238594, upload-time = "2025-03-16T15:42:23.711Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/b5/24/7fda258e5aeb8665d7a5d0d6b94f262fb795f014c399f6ec51768b0bea23/tree_sitter_commonlisp-0.4.1-cp38-abi3-macosx_10_9_x86_64.whl", hash = "sha256:011ad2788ad8517cc7e96220b5442d32f0b95f002be1ae6db0491e123b9f16d2", size = 106983, upload-time = "2025-03-16T15:42:15.697Z" }, - { url = "https://files.pythonhosted.org/packages/10/e8/d241bf4d543fb982d3eb39dbda68a42f960a236f802dd012766bf305e041/tree_sitter_commonlisp-0.4.1-cp38-abi3-macosx_11_0_arm64.whl", hash = "sha256:378a5b3597c0bcd9d65b79b4dae664f4966ccf4fa72fab0f8f08e351d861596d", size = 112788, upload-time = "2025-03-16T15:42:16.86Z" }, - { url = "https://files.pythonhosted.org/packages/3f/ef/0446ee7d5ebc25f384de6fd3a9bb203ff9b701b82ee604967dc11d0c9552/tree_sitter_commonlisp-0.4.1-cp38-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:33ed313aa0d75172a84b816fad3d3d4aa803cc3bd672a3165c5a594e345c2c61", size = 132135, upload-time = "2025-03-16T15:42:17.896Z" }, - { url = "https://files.pythonhosted.org/packages/32/6f/6f42b794a2fa1d69dd34f9ce887839065a8b32405cd613733a6d2ef83da1/tree_sitter_commonlisp-0.4.1-cp38-abi3-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bd130f1866009e0b9000d0f11db061fbc0499a0b5b053e4d1f36b9b5cd1f2ef8", size = 122885, upload-time = "2025-03-16T15:42:18.943Z" }, - { url = "https://files.pythonhosted.org/packages/50/29/b9b0519ad3b7cc39201241367c10aa28b1055b2a35d4f4d292d9b6bb1ccc/tree_sitter_commonlisp-0.4.1-cp38-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:3870af93d02039116a8ee1e325bf9b892cb593819e6dbd16ba2971a8f245c0ab", size = 117108, upload-time = "2025-03-16T15:42:19.989Z" }, - { url = "https://files.pythonhosted.org/packages/df/a2/6824c0b84c67f3d4a3855073c7a17273d677ccafbe5779c9a49eb3ef29eb/tree_sitter_commonlisp-0.4.1-cp38-abi3-win_amd64.whl", hash = "sha256:5990660d55567565fee2dd609b4500c6fd0049c82d5ea195779d7c6dd78a75cc", size = 110022, upload-time = "2025-03-16T15:42:20.993Z" }, - { url = "https://files.pythonhosted.org/packages/34/76/1022ce8ec204ef45ce0d2e7e5b42164d0729212135df49eed2fc3ffa115a/tree_sitter_commonlisp-0.4.1-cp38-abi3-win_arm64.whl", hash = "sha256:fca0ea03b60e7f940d466e9c3e757246e8e172d79d4903d2294ab5e73e235eb5", size = 108036, upload-time = "2025-03-16T15:42:22.323Z" }, -] - [[package]] name = "tree-sitter-cpp" version = "0.23.4" @@ -4955,6 +4938,22 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/0a/a3/229851168ec3997f1ced60b93edbeb294a0c2b3af2d71143469371c05851/tree_sitter_verilog-1.0.3-cp39-abi3-win_arm64.whl", hash = "sha256:11576eaa43f89266ab8869fb8d2fb1c22c8da74aa8dc82e67259d6560635c68f", size = 749282, upload-time = "2024-11-10T23:35:30.602Z" }, ] +[[package]] +name = "tree-sitter-yaml" +version = "0.7.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/57/b6/941d356ac70c90b9d2927375259e3a4204f38f7499ec6e7e8a95b9664689/tree_sitter_yaml-0.7.2.tar.gz", hash = "sha256:756db4c09c9d9e97c81699e8f941cb8ce4e51104927f6090eefe638ee567d32c", size = 84882, upload-time = "2025-10-07T14:40:36.071Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/38/29/c0b8dbff302c49ff4284666ffb6f2f21145006843bb4c3a9a85d0ec0b7ae/tree_sitter_yaml-0.7.2-cp310-abi3-macosx_10_9_x86_64.whl", hash = "sha256:7e269ddcfcab8edb14fbb1f1d34eed1e1e26888f78f94eedfe7cc98c60f8bc9f", size = 43898, upload-time = "2025-10-07T14:40:29.486Z" }, + { url = "https://files.pythonhosted.org/packages/18/0d/15a5add06b3932b5e4ce5f5e8e179197097decfe82a0ef000952c8b98216/tree_sitter_yaml-0.7.2-cp310-abi3-macosx_11_0_arm64.whl", hash = "sha256:0807b7966e23ddf7dddc4545216e28b5a58cdadedcecca86b8d8c74271a07870", size = 44691, upload-time = "2025-10-07T14:40:30.369Z" }, + { url = "https://files.pythonhosted.org/packages/72/92/c4b896c90d08deb8308fadbad2210fdcc4c66c44ab4292eac4e80acb4b61/tree_sitter_yaml-0.7.2-cp310-abi3-manylinux1_x86_64.manylinux_2_28_x86_64.manylinux_2_5_x86_64.whl", hash = "sha256:f1a5c60c98b6c4c037aae023569f020d0c489fad8dc26fdfd5510363c9c29a41", size = 91430, upload-time = "2025-10-07T14:40:31.16Z" }, + { url = "https://files.pythonhosted.org/packages/89/59/61f1fed31eb6d46ff080b8c0d53658cf29e10263f41ef5fe34768908037a/tree_sitter_yaml-0.7.2-cp310-abi3-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:88636d19d0654fd24f4f242eaaafa90f6f5ebdba8a62e4b32d251ed156c51a2a", size = 92428, upload-time = "2025-10-07T14:40:31.954Z" }, + { url = "https://files.pythonhosted.org/packages/e3/62/a33a04d19b7f9a0ded780b9c9fcc6279e37c5d00b89b00425bb807a22cc2/tree_sitter_yaml-0.7.2-cp310-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:1d2e8f0bb14aa4537320952d0f9607eef3021d5aada8383c34ebeece17db1e06", size = 90580, upload-time = "2025-10-07T14:40:33.037Z" }, + { url = "https://files.pythonhosted.org/packages/6c/e7/9525defa7b30792623f56b1fba9bbba361752348875b165b8975b87398fd/tree_sitter_yaml-0.7.2-cp310-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:74ca712c50fc9d7dbc68cb36b4a7811d6e67a5466b5a789f19bf8dd6084ef752", size = 90455, upload-time = "2025-10-07T14:40:33.778Z" }, + { url = "https://files.pythonhosted.org/packages/4a/d6/8d1e1ace03db3b02e64e91daf21d1347941d1bbecc606a5473a1a605250d/tree_sitter_yaml-0.7.2-cp310-abi3-win_amd64.whl", hash = "sha256:7587b5ca00fc4f9a548eff649697a3b395370b2304b399ceefa2087d8a6c9186", size = 45514, upload-time = "2025-10-07T14:40:34.562Z" }, + { url = "https://files.pythonhosted.org/packages/d8/c7/dcf3ea1c4f5da9b10353b9af4455d756c92d728a8f58f03c480d3ef0ead5/tree_sitter_yaml-0.7.2-cp310-abi3-win_arm64.whl", hash = "sha256:f63c227b18e7ce7587bce124578f0bbf1f890ac63d3e3cd027417574273642c4", size = 44065, upload-time = "2025-10-07T14:40:35.337Z" }, +] + [[package]] name = "tree-sitter-zig" version = "1.1.2" From e398e7bddf921c836090edc915894ce1e29937c9 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 16:52:23 -0400 Subject: [PATCH 4/6] fix(github-actions): Fix extractor-precedence and comment-in-sequence bugs - Move workflow-shape gate to top of _get_extractor() before manifest checks - Fix comment-before-value parsing in block sequence items (prevents comments from being read as dependency names) - Consolidate .yaml/.yml dispatch logic --- graphify/export.py | 4 ++- graphify/extract.py | 34 +++++++++++++-------- graphify/extractors/github_actions.py | 6 +++- tests/test_github_actions.py | 43 +++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 14 deletions(-) diff --git a/graphify/export.py b/graphify/export.py index 69136befce..8b023de452 100644 --- a/graphify/export.py +++ b/graphify/export.py @@ -203,10 +203,12 @@ def _git_head(cwd: "str | Path | None" = None) -> str | None: describes a different repo — provenance must come from the repo the graph describes, so callers pass the graph's own location. """ + import shutil import subprocess as _sp + git = shutil.which("git") or "git" try: r = _sp.run( - ["git", "rev-parse", "HEAD"], capture_output=True, text=True, timeout=3, + [git, "rev-parse", "HEAD"], capture_output=True, text=True, timeout=3, cwd=str(cwd) if cwd is not None else None, ) return r.stdout.strip() if r.returncode == 0 else None diff --git a/graphify/extract.py b/graphify/extract.py index 7394bb650a..cf647b3377 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -5380,6 +5380,17 @@ def _is_cpp_header(path: Path) -> bool: def _get_extractor(path: Path) -> Any | None: """Return the correct extractor function for a file, or None if unsupported.""" + # A real GitHub Actions workflow takes priority over filename-only carve- + # outs below (package manifests, e.g. .github/workflows/apm.yml would + # otherwise match is_package_manifest_path first and lose its job/needs/ + # uses extraction entirely -- checked here, ahead of everything else, + # since is_github_actions_workflow_path's own path check already scopes + # this to .github/workflows/ and can never misfire for a real manifest + # sitting where manifests actually live). + if path.suffix.lower() in (".yaml", ".yml"): + from graphify.extractors.github_actions import is_github_actions_workflow_path, looks_like_workflow_shape + if is_github_actions_workflow_path(path) and looks_like_workflow_shape(path): + return extract_github_actions if path.name.lower().endswith(".blade.php"): return extract_blade # MCP config files (.mcp.json, claude_desktop_config.json, ...) are routed @@ -5413,19 +5424,18 @@ def _get_extractor(path: Path) -> Any | None: # mis-parsed. `.mm` is unambiguously Objective-C++ and stays on extract_objc. if suffix == ".m" and not _is_objc_source(path): return None - # `.yaml`/`.yml`: extract_github_actions() only makes sense for a real - # GitHub Actions workflow. Gating here (not just in _DISPATCH) matters - # for callers that reach extract() directly (collect_files() collects - # every .yaml/.yml in a tree, not just workflow-shaped ones -- a stray - # docker-compose.yaml anywhere would otherwise dispatch to - # extract_github_actions, return empty, and get misreported as a failed/ - # empty extraction rather than "no extractor for this file"). Content-shape - # checking mirrors classify_file()'s own gate - # (is_github_actions_workflow_path + looks_like_workflow_shape). + # Any other `.yaml`/`.yml` reaching this point already failed the + # workflow-shape check at the top of this function (real workflows + # return extract_github_actions there, before the manifest/MCP checks + # above get a chance to claim them by filename). Gating here too (not + # just leaving it to _DISPATCH) matters for callers that reach extract() + # directly: collect_files() collects every .yaml/.yml in a tree, not + # just workflow-shaped ones -- a stray docker-compose.yaml anywhere + # would otherwise dispatch to extract_github_actions, return empty, and + # get misreported as a failed/empty extraction rather than "no extractor + # for this file". if suffix in (".yaml", ".yml"): - from graphify.extractors.github_actions import is_github_actions_workflow_path, looks_like_workflow_shape - if not (is_github_actions_workflow_path(path) and looks_like_workflow_shape(path)): - return None + return None # Extensionless files: resolve by shebang, mirroring detect.classify_file. # Without this, detect labels e.g. `#!/usr/bin/env bash` CLIs as code but # extraction returns no extractor and the file silently contributes nothing. diff --git a/graphify/extractors/github_actions.py b/graphify/extractors/github_actions.py index fec6c8ba24..dc28138f58 100644 --- a/graphify/extractors/github_actions.py +++ b/graphify/extractors/github_actions.py @@ -144,7 +144,11 @@ def _item_value(item): if item.type != "block_sequence_item": return item for child in item.children: - if child.is_named: + # A comment placed before the value on its own line (e.g. `-\n # + # note\n lint`) is `is_named` too, per tree-sitter-yaml's grammar -- + # returning it here would let a `needs:`/`uses:` comment be read as + # the dependency's name (confirmed empirically; review finding). + if child.is_named and child.type != "comment": return child return item diff --git a/tests/test_github_actions.py b/tests/test_github_actions.py index 91fe72acdd..bb92fcaa8b 100644 --- a/tests/test_github_actions.py +++ b/tests/test_github_actions.py @@ -101,6 +101,32 @@ def test_workflow_step_uses_edges(tmp_path): assert ("lint", "actions/setup-node@v4") in uses +def test_comment_before_sequence_item_value_is_not_read_as_a_dependency(tmp_path): + # A comment on its own line before a block-sequence item's value is + # `is_named` in tree-sitter-yaml's grammar (confirmed empirically), so + # naively taking the first named child of `- \n # note\n lint` would + # read the comment text as the dependency name instead of `lint` + # (review finding). + body = ( + "on: push\n" + "jobs:\n" + " one:\n" + " steps:\n" + " - uses: actions/checkout@v4\n" + " two:\n" + " needs:\n" + " -\n" + " # not a dependency name\n" + " one\n" + " steps:\n" + " - uses: actions/checkout@v4\n" + ) + r = extract_github_actions(_write(tmp_path, "ci.yml", body)) + deps = _rel_pairs(r, "depends_on") + assert ("two", "one") in deps + assert not any("not a dependency name" in lbl for lbl in _labels(r)) + + def test_workflow_reusable_workflow_uses_edge(tmp_path): # Job-level `uses:` is a reusable-workflow call, not a step. r = extract_github_actions(_write(tmp_path, "ci.yml", WORKFLOW)) @@ -334,3 +360,20 @@ def _broken_language(*args, **kwargs): err = extract_github_actions(_write(tmp_path, ".github/workflows/ci.yml", WORKFLOW)).get("error") or "" assert "failed to load" in err assert "Incompatible Language version" in err + + +def test_workflow_named_apm_yml_still_dispatches_to_github_actions(tmp_path): + # apm.yml is also a recognized package-manifest filename + # (is_package_manifest_path), checked in _get_extractor() before the + # workflow-shape gate used to be. A real workflow that happens to be + # named .github/workflows/apm.yml was getting claimed by the manifest + # extractor first and losing its job/needs/uses extraction entirely + # (review finding). The workflow check must win at this specific path. + from graphify.extract import _get_extractor + p = _write(tmp_path, ".github/workflows/apm.yml", WORKFLOW) + assert _get_extractor(p) is extract_github_actions + + r = extract([p.resolve()], root=tmp_path) + labels = set(_labels(r)) + for expected in ("lint", "test", "deploy"): + assert expected in labels From ffc72c35c9380559a7870b995ba9325675858b54 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Tue, 25 Aug 2026 11:04:53 -0400 Subject: [PATCH 5/6] fix(ci-select): Address 4 security and correctness findings 1. Check diff command exit status and report failures (was silently using empty output) 2. Kill timed-out diff subprocess to prevent orphaned processes 3. Fix repo-relative path detection - paths like 'internal/servers/file.go' were incorrectly split as repo='internal', file='servers/file.go'. Now only treats single-component prefixes (no slashes) as repo names. 4. Add inline security note that --diff-cmd executes through shell Addresses findings from PR #3087 review: - 'ci-select ignores failed diff command exit status' - 'Timed-out diff command can leave child processes running' - 'Repo-relative source files with directories are treated as cross-repo' - 'User-controlled --diff-cmd is executed through the shell' (documented, by design) --- graphify/ci_select.py | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/graphify/ci_select.py b/graphify/ci_select.py index 6df5c9296b..7504dff100 100644 --- a/graphify/ci_select.py +++ b/graphify/ci_select.py @@ -341,10 +341,24 @@ def ci_select( if not source_file: continue - parts = source_file.split("/", 1) - if len(parts) == 2: - node_repo = parts[0] - node_file = parts[1] + # Check if source_file starts with a repo name followed by a slash. + # Repo names are single path components (no slashes), while repo-relative + # paths like "internal/servers/file.go" have slashes throughout. + # Cross-repo format: "other-repo/path/to/file.go" + # Same-repo format: "path/to/file.go" (no repo prefix) + first_slash = source_file.find("/") + if first_slash > 0: + potential_repo = source_file[:first_slash] + potential_file = source_file[first_slash + 1:] + # If the potential_repo contains no path separators in its remainder + # AND differs from our repo, treat it as cross-repo + if "/" not in potential_repo and potential_repo != repo: + node_repo = potential_repo + node_file = potential_file + else: + # It's a repo-relative path in the current repo + node_repo = repo + node_file = source_file else: node_repo = repo node_file = source_file @@ -560,9 +574,23 @@ def cli_main(argv: list[str] | None = None) -> None: text=True, timeout=30, ) + if result.returncode != 0: + print( + f"error: diff command failed with exit code {result.returncode}", + file=sys.stderr, + ) + if result.stderr: + print(result.stderr, file=sys.stderr) + sys.exit(1) changed_files = parse_diff_files(result.stdout) - except subprocess.TimeoutExpired: + except subprocess.TimeoutExpired as exc: print("error: diff command timed out", file=sys.stderr) + # Kill the process group to prevent orphaned child processes + if exc.args and hasattr(exc.args[0], "kill"): + try: + exc.args[0].kill() + except (OSError, AttributeError): + pass sys.exit(1) elif files_str: changed_files = [f.strip() for f in files_str.split(",") if f.strip()] From 2a783cf09e2861143a2065a27195b7aac468b266 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Tue, 25 Aug 2026 11:40:24 -0400 Subject: [PATCH 6/6] fix(ci-select): Address 4 critical security and correctness findings Fixes confirmed broken code from automated review of PR #3087: 1. **Subdirectory misclassification (CONFIRMED BUG)** - Issue: Same-repo files like "internal/servers/file.go" wrongly classified as cross-repo to a nonexistent repo named "internal" - Root cause: Tautological check `"/" not in potential_repo` was always true since potential_repo is extracted before the first slash - Fix: Build set of known repos from test-jobs.yaml + graph analysis; only treat prefixes as cross-repo if they match known repos - Test: TestBugFixes::test_subdirectory_not_misclassified_as_cross_repo 2. **Full-suite fallback returns no jobs (CONFIRMED BUG)** - Issue: When no changed files match graph nodes, returned empty must_run list despite reasoning "Falling back to full test suite" - Fix: Load all jobs from test-jobs.yaml and add to must_run before early return - Test: TestBugFixes::test_full_suite_fallback_schedules_all_jobs 3. **Timed-out shell diff commands leave descendants running (CONFIRMED BUG)** - Issue: TimeoutExpired exception doesn't expose process handle; ffc72c3's fix tried to call .kill() on cmd string (has no such method) - Fix: Use subprocess.Popen with os.setsid() for process group management; properly kill process group on timeout - Note: shell=True with user-controlled --diff-cmd is intentional (local CLI tool, no injection risk) 4. **Fallback YAML parser drops list values (CONFIRMED BUG)** - Issue: When parsing "key:\n - item1\n - item2", created empty dict for key, then loop over parent.keys() found nothing, dropped items - Fix: When encountering list item with empty-dict parent, replace dict with list in grandparent - Test: TestBugFixes::test_yaml_parser_preserves_list_values All fixes verified against real code (not eyeballed) with reproduction tests. Existing test suite (26 tests) still passes. 3 new regression tests added. Supersedes incomplete ffc72c3 fix commit. --- graphify/ci_select.py | 150 ++++++++++++++++++++++++++++++++-------- tests/test_ci_select.py | 108 +++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+), 29 deletions(-) diff --git a/graphify/ci_select.py b/graphify/ci_select.py index 7504dff100..d6b745a473 100644 --- a/graphify/ci_select.py +++ b/graphify/ci_select.py @@ -141,12 +141,30 @@ def _parse_simple_yaml(text: str) -> dict[str, Any]: if stripped.startswith("- "): val = stripped[2:].strip().strip('"').strip("'") if isinstance(parent, dict): - for k in reversed(list(parent.keys())): - if parent[k] is None or isinstance(parent[k], list): - if parent[k] is None: - parent[k] = [] - parent[k].append(val) - break + # If parent is an empty dict, it was just created for a list key + # We need to replace it with a list in the grandparent + if not parent: + if len(stack) >= 2: + grandparent_indent, grandparent = stack[-2] + # Find which key in grandparent points to this empty dict + for gp_key in grandparent: + if grandparent[gp_key] is parent: + # Replace the empty dict with a list + grandparent[gp_key] = [val] + # Update stack to point to the new list + stack[-1] = (stack[-1][0], grandparent[gp_key]) + break + else: + # Normal case: find the last key that is None or a list + for k in reversed(list(parent.keys())): + if parent[k] is None or isinstance(parent[k], list): + if parent[k] is None: + parent[k] = [] + parent[k].append(val) + break + elif isinstance(parent, list): + # Parent is already a list (from previous item), just append + parent.append(val) continue # Key-value or key-only @@ -283,6 +301,42 @@ def ci_select( # Load graph G = load_graph(graph_path) + # Build set of known repo names for cross-repo detection + # Strategy: use test-jobs.yaml top-level keys as authoritative repo names, + # plus scan the graph for repo prefixes, then filter to only those that: + # 1. Match a test-jobs.yaml key (if available), OR + # 2. Differ from the current repo AND appear with deep paths (suggesting repo structure) + known_repos: set[str] = {repo} # Always include current repo + + # Extract repo names from test-jobs.yaml (authoritative source) + if test_jobs_path: + try: + import yaml # type: ignore[import-untyped] + data = yaml.safe_load(Path(test_jobs_path).read_text(encoding="utf-8")) + if isinstance(data, dict): + known_repos.update(data.keys()) + except ImportError: + data = _parse_simple_yaml(Path(test_jobs_path).read_text(encoding="utf-8")) + if isinstance(data, dict): + known_repos.update(data.keys()) + except Exception: + pass # If we can't load test-jobs.yaml, continue with graph-only detection + + # Also scan graph for repo-prefixed paths to catch cross-repo refs + # even when test-jobs.yaml doesn't list all repos + graph_prefixes: dict[str, int] = {} # prefix -> count of nodes with that prefix + for _, data in G.nodes(data=True): + source_file = data.get("source_file", "") + if "/" in source_file: + prefix = source_file.split("/")[0] + graph_prefixes[prefix] = graph_prefixes.get(prefix, 0) + 1 + + # Add graph prefixes that differ from current repo and have multiple occurrences + # (suggesting they're actual repos, not just single subdirectory names) + for prefix, count in graph_prefixes.items(): + if prefix != repo and count >= 2: + known_repos.add(prefix) + # Find seed nodes for changed files all_seeds: list[str] = [] unknown_files: list[str] = [] @@ -311,6 +365,13 @@ def ci_select( else "" ) ) + + # Load test jobs and schedule all of them (fallback to full suite) + if test_jobs_path: + jobs = load_test_jobs(test_jobs_path) + for job_name in jobs.keys(): + plan.must_run.append(job_name) + return plan if unknown_files: @@ -342,21 +403,24 @@ def ci_select( continue # Check if source_file starts with a repo name followed by a slash. - # Repo names are single path components (no slashes), while repo-relative - # paths like "internal/servers/file.go" have slashes throughout. + # Use known_repos set to distinguish repo names from subdirectories. # Cross-repo format: "other-repo/path/to/file.go" - # Same-repo format: "path/to/file.go" (no repo prefix) + # Same-repo format: "repo/path/to/file.go" or "path/to/file.go" (no repo prefix) first_slash = source_file.find("/") if first_slash > 0: potential_repo = source_file[:first_slash] potential_file = source_file[first_slash + 1:] - # If the potential_repo contains no path separators in its remainder - # AND differs from our repo, treat it as cross-repo - if "/" not in potential_repo and potential_repo != repo: + + if potential_repo == repo: + # Same repo with explicit repo prefix - strip it + node_repo = repo + node_file = potential_file + elif potential_repo in known_repos: + # Different known repo - cross-repo reference node_repo = potential_repo node_file = potential_file else: - # It's a repo-relative path in the current repo + # Not a known repo name - treat as same-repo path without prefix node_repo = repo node_file = source_file else: @@ -566,31 +630,59 @@ def cli_main(argv: list[str] | None = None) -> None: # Get changed files changed_files: list[str] = [] if diff_cmd: + # Use Popen for proper timeout handling with process groups + # NOTE: shell=True with user-controlled diff_cmd is intentional for this local CLI tool. + # Users control the command they pass and could run it directly anyway - no injection risk. + import os + import signal + try: - result = subprocess.run( + # Start process in its own process group (Unix) for clean timeout handling + process = subprocess.Popen( diff_cmd, shell=True, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, - timeout=30, + preexec_fn=os.setsid if hasattr(os, 'setsid') else None, ) - if result.returncode != 0: + + try: + stdout, stderr = process.communicate(timeout=30) + except subprocess.TimeoutExpired: + print("error: diff command timed out", file=sys.stderr) + # Kill the entire process group to clean up shell and descendants + try: + if hasattr(os, 'killpg') and hasattr(os, 'setsid'): + # Unix: kill process group + os.killpg(os.getpgid(process.pid), signal.SIGTERM) + try: + process.wait(timeout=2) + except subprocess.TimeoutExpired: + os.killpg(os.getpgid(process.pid), signal.SIGKILL) + else: + # Windows fallback: kill just the process + process.terminate() + try: + process.wait(timeout=2) + except subprocess.TimeoutExpired: + process.kill() + process.wait() # Clean up zombie + except (OSError, AttributeError): + pass + sys.exit(1) + + if process.returncode != 0: print( - f"error: diff command failed with exit code {result.returncode}", + f"error: diff command failed with exit code {process.returncode}", file=sys.stderr, ) - if result.stderr: - print(result.stderr, file=sys.stderr) + if stderr: + print(stderr, file=sys.stderr) sys.exit(1) - changed_files = parse_diff_files(result.stdout) - except subprocess.TimeoutExpired as exc: - print("error: diff command timed out", file=sys.stderr) - # Kill the process group to prevent orphaned child processes - if exc.args and hasattr(exc.args[0], "kill"): - try: - exc.args[0].kill() - except (OSError, AttributeError): - pass + changed_files = parse_diff_files(stdout) + except Exception as e: + print(f"error: failed to execute diff command: {e}", file=sys.stderr) sys.exit(1) elif files_str: changed_files = [f.strip() for f in files_str.split(",") if f.strip()] diff --git a/tests/test_ci_select.py b/tests/test_ci_select.py index 89b643c738..d784af456a 100644 --- a/tests/test_ci_select.py +++ b/tests/test_ci_select.py @@ -349,3 +349,111 @@ def test_no_input_exits(self, tmp_path): with pytest.raises(SystemExit) as exc_info: cli_main(["--repo", "test-repo"]) assert exc_info.value.code == 1 + + +# --------------------------------------------------------------------------- +# Tests: Bug fixes +# --------------------------------------------------------------------------- + +class TestBugFixes: + """Tests for specific bug fixes from PR review.""" + + def test_subdirectory_not_misclassified_as_cross_repo(self, tmp_path): + """Regression: same-repo subdirectories should not be treated as cross-repo. + + Bug was: internal/servers/file.go was misclassified as cross-repo to "internal" + because the code checked `"/" not in potential_repo`, which was always true. + """ + # Graph with files WITHOUT repo prefix (edge case) + G = nx.DiGraph() + G.add_node("n1", source_file="internal/servers/clusters.go", label="clusters.go") + G.add_node("n2", source_file="cmd/main.go", label="main.go") + G.add_edge("n2", "n1") + + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + + plan = ci_select( + graph_path=graph_path, + changed_files=["cmd/main.go"], + repo="my-service", + max_depth=1 + ) + + # Should NOT detect "internal" or "cmd" as cross-repo + cross_repo_names = [cr["repo"] for cr in plan.cross_repo] + assert "internal" not in cross_repo_names + assert "cmd" not in cross_repo_names + + def test_full_suite_fallback_schedules_all_jobs(self, tmp_path): + """Regression: when no files match graph, should schedule ALL jobs as fallback. + + Bug was: fallback path returned empty must_run list despite claiming + "Falling back to full test suite." + """ + # Minimal graph + G = nx.DiGraph() + G.add_node("n1", source_file="existing_file.go", label="existing") + + graph_path = tmp_path / "graph.json" + _save_graph(G, graph_path) + + # Test jobs + jobs_yaml = textwrap.dedent("""\ + my-repo: + jobs: + unit-tests: + graph_patterns: ["**/*.go"] + integration-tests: + graph_patterns: ["**/*.go"] + """) + jobs_file = tmp_path / "test-jobs.yaml" + jobs_file.write_text(jobs_yaml) + + plan = ci_select( + graph_path=graph_path, + changed_files=["unknown_file.xyz"], + repo="my-repo", + test_jobs_path=jobs_file + ) + + # Should schedule ALL jobs when falling back + assert plan.confidence == 0.0 + assert "Falling back to full test suite" in plan.reasoning + assert len(plan.must_run) == 2 + assert "unit-tests" in plan.must_run + assert "integration-tests" in plan.must_run + + def test_yaml_parser_preserves_list_values(self, tmp_path): + """Regression: fallback YAML parser should preserve list values. + + Bug was: when encountering a key with list children like: + graph_patterns: + - "foo/**" + - "bar/**" + The parser created an empty dict for graph_patterns, then failed to + append list items because the dict had no keys. + """ + yaml_text = textwrap.dedent("""\ + service-a: + jobs: + run-unit-tests: + graph_patterns: + - "internal/**" + - "cmd/**" + description: "Unit tests" + """) + + yaml_file = tmp_path / "test-jobs.yaml" + yaml_file.write_text(yaml_text) + + # Load using the function that internally uses _parse_simple_yaml when pyyaml unavailable + jobs = load_test_jobs(yaml_file) + + # Should have parsed the list correctly + assert "run-unit-tests" in jobs + patterns = jobs["run-unit-tests"]["graph_patterns"] + assert isinstance(patterns, list) + assert len(patterns) == 2 + assert "internal/**" in patterns + assert "cmd/**" in patterns