From 339fbe93f29fe31809aa463dc3f63a2c4900ff96 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 10:51:24 -0400 Subject: [PATCH 1/4] OSAC-4049: Extract GitHub Actions workflow job/needs/uses under --code-only graphify has no YAML support at all (.yaml/.yml are DOC_EXTENSIONS with no extractor), so .github/workflows/*.yaml content is invisible to the graph regardless of corpus scope, and our CI's first-run path (graphify extract --code-only) has no LLM backend to even give it the semantic-pass reading. Adds a scoped GitHub Actions workflow extractor (job nodes, needs -> depends_on edges, job-level and step-level uses -> action/reusable-workflow edges, cross-file shared-action hub collapsing) via tree-sitter-yaml, behind a new optional [yaml] extra. Traversal helpers and the extraction approach are adapted, with attribution, from the unmerged Graphify-Labs/graphify PR #2541 (read via the GitHub API, not executed -- unreviewed third-party code) -- but that PR only registers an extractor for the semantic pass; YAML stays DOC-classified either way, so it can't help --code-only regardless of merge status. This also carves out .github/workflows/*.yml|yaml as FileType.CODE in detect.classify_file(), mirroring the existing package-manifest precedent (apm.yml/pyproject.toml are already special-cased to CODE by path before the generic extension lookup), which is the actual missing piece. Scoped narrowly to GitHub Actions workflow shapes specifically -- Docker Compose (also in PR #2541) is deliberately not ported, and every other .yaml/.yml (Helm values, k8s manifests, OpenAPI specs) keeps its existing, correct semantic-pass classification untouched (verified empirically, see test plan). Full test suite: 4355 passed, 0 failures (unrelated pre-existing openai- extra gap aside, confirmed passing separately with --extra ollama). --- graphify/detect.py | 15 ++ graphify/extract.py | 5 + graphify/extractors/github_actions.py | 318 ++++++++++++++++++++++++++ pyproject.toml | 8 +- tests/test_github_actions.py | 262 +++++++++++++++++++++ uv.lock | 24 +- 6 files changed, 630 insertions(+), 2 deletions(-) create mode 100644 graphify/extractors/github_actions.py create mode 100644 tests/test_github_actions.py diff --git a/graphify/detect.py b/graphify/detect.py index c51ea916ee..7a389d8c42 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -508,6 +508,21 @@ 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). Path-only check, no file read: content is + # validated inside extract_github_actions() itself, which returns an + # empty result for anything at this path that isn't actually workflow- + # shaped rather than this function guessing from a peek (OSAC-4049). + # 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 + if is_github_actions_workflow_path(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 1822c556fb..7a97e099d7 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -55,6 +55,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 @@ -4846,6 +4847,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, @@ -4873,6 +4876,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", diff --git a/graphify/extractors/github_actions.py b/graphify/extractors/github_actions.py new file mode 100644 index 0000000000..8cc865a746 --- /dev/null +++ b/graphify/extractors/github_actions.py @@ -0,0 +1,318 @@ +"""GitHub Actions workflow extractor. + +Scoped to GitHub Actions workflow YAML only (job nodes, ``needs``/``uses`` +edges) -- OSAC-4049. 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 + +from pathlib import Path + +from graphify.extractors.base import _file_stem, _make_id + + +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, zero-I/O signal usable at + classify_file() time, before any content is read. Content is still + validated separately inside extract_github_actions() itself (a + malformed/non-workflow file at this path returns an empty result rather + than being misclassified retroactively). + """ + if path.suffix.lower() not in (".yml", ".yaml"): + return False + return path.parent.name == "workflows" and path.parent.parent.name == ".github" + + +# 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: + return {"nodes": [], "edges": [], "error": "tree_sitter_yaml not installed. Run: pip install tree-sitter-yaml"} + + 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"} + language = Language(tsyaml.language()) + parser = Parser(language) + 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 72be356a10..a9966f1a38 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 (OSAC-4049). 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 @@ -88,7 +94,7 @@ terraform = ["tree-sitter-hcl"] # tree-sitter-ocaml ships prebuilt abi3 wheels for every platform, so no C # toolchain is needed; kept optional because OCaml is a niche corpus language. ocaml = ["tree-sitter-ocaml"] -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"] +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"] [project.scripts] graphify = "graphify.__main__:main" diff --git a/tests/test_github_actions.py b/tests/test_github_actions.py new file mode 100644 index 0000000000..4528b1b469 --- /dev/null +++ b/tests/test_github_actions.py @@ -0,0 +1,262 @@ +"""Tests for the GitHub Actions extractor (graphify/extractors/github_actions.py) +and its detect.classify_file() carve-out (OSAC-4049). + +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 ticket, 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 ─────────────────── + +def test_workflow_path_classified_as_code(): + assert classify_file(Path(".github/workflows/ci.yml")) == FileType.CODE + assert classify_file(Path(".github/workflows/nightly-build.yaml")) == FileType.CODE + + +def test_workflow_path_classified_as_code_absolute(): + assert classify_file(Path("/repo/osac/.github/workflows/ci.yml")) == FileType.CODE + + +def test_nested_workflows_dir_is_not_reclassified(): + # GitHub only recognizes workflow files directly in .github/workflows/, + # not nested deeper -- so neither does this carve-out. + assert classify_file(Path(".github/workflows/nested/ci.yml")) == FileType.DOCUMENT + + +def test_composite_action_yml_is_not_reclassified(): + # .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. + assert classify_file(Path(".github/actions/setup/action.yml")) == FileType.DOCUMENT + + +def test_other_yaml_still_classified_as_document(): + # 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(Path("charts/myapp/values.yaml")) == FileType.DOCUMENT + assert classify_file(Path("k8s/deployment.yaml")) == FileType.DOCUMENT + assert classify_file(Path("openapi.yaml")) == FileType.DOCUMENT + assert classify_file(Path("docker-compose.yml")) == 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") diff --git a/uv.lock b/uv.lock index 279516c114..1a3ac362cb 100644 --- a/uv.lock +++ b/uv.lock @@ -1150,6 +1150,7 @@ all = [ { name = "tree-sitter-ocaml" }, { name = "tree-sitter-pascal" }, { name = "tree-sitter-sql" }, + { name = "tree-sitter-yaml" }, { name = "watchdog" }, { name = "yt-dlp" }, ] @@ -1230,6 +1231,9 @@ video = [ watch = [ { name = "watchdog" }, ] +yaml = [ + { name = "tree-sitter-yaml" }, +] [package.dev-dependencies] dev = [ @@ -1332,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", "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 = [ @@ -4932,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 774738132bb4c858515b48b9b7b7908b5eae2451 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 16:50:50 -0400 Subject: [PATCH 2/4] OSAC-4049: fix 8 real CodeRabbit review findings on PR #2 - classify_file()/_get_extractor() required only a workflow PATH, not workflow-shaped content -- a stray non-workflow YAML at .github/workflows/ got routed to CODE, extracted as empty, and never reached the semantic pass at all (real content loss, not just noise). Added a cheap tree-sitter-free content sniff (looks_like_workflow_shape) and gated both classify_file() and extract._get_extractor() on it. - extract_github_actions() conflated "tree_sitter_yaml not installed" with "installed but failed to load" the same way extractors/sql.py already distinguishes them; applied the same importlib.util.find_spec check, and extended it to Language()/Parser() init failures too. - cache.py's semantic-cache read only caught json.JSONDecodeError; a truncated write can raise UnicodeDecodeError first, so it was never counted as a corrupt entry. - ARCHITECTURE.md's statelessness claim didn't match extract() actually raising the recursion limit, clearing module caches, and warning to stderr -- reworded to describe the dict/graph boundary instead. - README.md MD028 (blank line inside a blockquote), os.access(W_OK) being unreliable as a writability probe under root, and missing platform gates on os.mkfifo/AF_UNIX/symlink tests. Skipped (not real / out of scope, reasoning on the PR thread): - tests/test_extract.py E702 semicolons: pre-existing code untouched by this PR's diff, not part of this repo's committed Ruff `select` set, and the same pattern repeats elsewhere in the same file. - detect.py count_words() FIFO-before-regular-file-check: pre-existing, unrelated to this PR's diff; worth its own follow-up. Full suite: 4390 passed, 0 failures. ruff check: clean. --- ARCHITECTURE.md | 2 +- README.md | 2 +- graphify/cache.py | 6 +- graphify/detect.py | 26 ++++--- graphify/extract.py | 13 ++++ graphify/extractors/github_actions.py | 64 +++++++++++++--- tests/test_cache.py | 30 ++++++++ tests/test_github_actions.py | 104 ++++++++++++++++++++++---- tests/test_install_references.py | 9 ++- tests/test_non_regular_files.py | 16 ++++ 10 files changed, 233 insertions(+), 39 deletions(-) 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 b5ec879b6f..13ce604537 100644 --- a/README.md +++ b/README.md @@ -866,7 +866,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 bdb6cbd1e0..54821847c7 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -918,11 +918,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 (OSAC-4049 review). _corrupt_cache_entries += 1 return None except OSError: diff --git a/graphify/detect.py b/graphify/detect.py index 7a389d8c42..b71b17db21 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -512,16 +512,22 @@ def classify_file(path: Path) -> FileType | None: # 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). Path-only check, no file read: content is - # validated inside extract_github_actions() itself, which returns an - # empty result for anything at this path that isn't actually workflow- - # shaped rather than this function guessing from a peek (OSAC-4049). - # 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 - if is_github_actions_workflow_path(path): + # 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 OSAC-4049's 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"): diff --git a/graphify/extract.py b/graphify/extract.py index 7a97e099d7..dd6c422806 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -5027,6 +5027,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", #OSAC-4049 + # review round 3). 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 index 8cc865a746..0b63985fb2 100644 --- a/graphify/extractors/github_actions.py +++ b/graphify/extractors/github_actions.py @@ -18,10 +18,13 @@ """ 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 @@ -29,17 +32,42 @@ def is_github_actions_workflow_path(path: Path) -> bool: 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, zero-I/O signal usable at - classify_file() time, before any content is read. Content is still - validated separately inside extract_github_actions() itself (a - malformed/non-workflow file at this path returns an empty result rather - than being misclassified retroactively). + 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, #OSAC-4049 review round 3). """ 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 caught in OSAC-4049's review (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"}) @@ -211,16 +239,34 @@ def extract_github_actions(path: Path) -> dict: try: import tree_sitter_yaml as tsyaml from tree_sitter import Language, Parser - except ImportError: - return {"nodes": [], "edges": [], "error": "tree_sitter_yaml not installed. Run: pip install tree-sitter-yaml"} + 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"} - language = Language(tsyaml.language()) - parser = Parser(language) tree = parser.parse(source) root = tree.root_node except Exception as e: diff --git a/tests/test_cache.py b/tests/test_cache.py index 67135b3932..149ec80591 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -1486,3 +1486,33 @@ def test_corrupt_semantic_entry_warns_and_is_a_miss(tmp_path): # The corrupt entry is a miss, so the file is re-dispatched for extraction. assert nodes == [] assert uncached == [str(f)] + + +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 + (OSAC-4049 review).""" + from graphify.cache import ( + check_semantic_cache, + save_semantic_cache, + cache_dir, + ) + + f = tmp_path / "doc.md" + f.write_text("# Doc\n\nBody.\n") + save_semantic_cache([{"id": "n", "source_file": "doc.md"}], [], root=tmp_path) + + 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") + + with pytest.warns(RuntimeWarning, match="corrupt"): + nodes, _, _, uncached = check_semantic_cache([str(f)], root=tmp_path) + + assert nodes == [] + assert uncached == [str(f)] diff --git a/tests/test_github_actions.py b/tests/test_github_actions.py index 4528b1b469..604931ec27 100644 --- a/tests/test_github_actions.py +++ b/tests/test_github_actions.py @@ -213,37 +213,57 @@ def test_empty_and_comment_only_files_are_safe(tmp_path): # ── classify_file() carve-out: the actual --code-only fix ─────────────────── - -def test_workflow_path_classified_as_code(): - assert classify_file(Path(".github/workflows/ci.yml")) == FileType.CODE - assert classify_file(Path(".github/workflows/nightly-build.yaml")) == FileType.CODE +# +# 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_workflow_path_classified_as_code_absolute(): - assert classify_file(Path("/repo/osac/.github/workflows/ci.yml")) == 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(): +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. - assert classify_file(Path(".github/workflows/nested/ci.yml")) == FileType.DOCUMENT + p = _write(tmp_path, ".github/workflows/nested/ci.yml", WORKFLOW) + assert classify_file(p) == FileType.DOCUMENT -def test_composite_action_yml_is_not_reclassified(): +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. - assert classify_file(Path(".github/actions/setup/action.yml")) == FileType.DOCUMENT + p = _write(tmp_path, ".github/actions/setup/action.yml", WORKFLOW) + assert classify_file(p) == FileType.DOCUMENT -def test_other_yaml_still_classified_as_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(Path("charts/myapp/values.yaml")) == FileType.DOCUMENT - assert classify_file(Path("k8s/deployment.yaml")) == FileType.DOCUMENT - assert classify_file(Path("openapi.yaml")) == FileType.DOCUMENT - assert classify_file(Path("docker-compose.yml")) == FileType.DOCUMENT + 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): @@ -260,3 +280,57 @@ def test_workflow_extracted_under_code_only_semantics(tmp_path): 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..29a3ecd538 100644 --- a/tests/test_install_references.py +++ b/tests/test_install_references.py @@ -544,5 +544,10 @@ 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 + # (OSAC-4049 review). + 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..3b40de9425 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,16 @@ 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 +# (OSAC-4049 review). +_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 +45,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 +54,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 +71,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 +79,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 +89,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") From 5d0f83bba8ffea755757d4a075006410b2b8bb46 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 16:52:23 -0400 Subject: [PATCH 3/4] OSAC-4049: resolve git executable via shutil.which() in export._git_head Ruff S607 (partial executable path): correcting a mistaken skip-reply on this finding -- I initially claimed this was already fixed without re-checking the live code first. shutil.which("git") or "git" is the same fallback pattern already used for claude/gws/graphify elsewhere in this codebase. watch.py's _git_head has the identical bare-"git" pattern but wasn't flagged by this review; leaving it for a separate pass rather than scope-creeping this fix. --- graphify/export.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/graphify/export.py b/graphify/export.py index 50ed388a41..9cb8432922 100644 --- a/graphify/export.py +++ b/graphify/export.py @@ -180,10 +180,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 From 2597034e8daf326dd8c0f4f92ea7c1d1dcc260aa Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Fri, 14 Aug 2026 17:34:53 -0400 Subject: [PATCH 4/4] OSAC-4049: fix extractor-precedence and comment-in-sequence bugs Two more real findings, verified against current code: - _get_extractor() checked is_package_manifest_path (filename-only, e.g. apm.yml) before the GitHub Actions workflow-shape gate, so a real workflow file coincidentally named .github/workflows/apm.yml lost its job/needs/uses extraction to the manifest extractor instead. Moved the workflow check first -- its own path check already scopes it to .github/workflows/, so it can never misfire for a real manifest sitting where manifests actually live. - _item_value() took the first is_named child of a block_sequence_item to find its value, but a comment on its own line before the value is also is_named in tree-sitter-yaml's grammar (confirmed empirically) -- a needs:/uses: item like "-\n # note\n lint" would read the comment text as the dependency name. Now explicitly skips comment children. Full suite: 4519 passed (1 pre-existing flaky test unrelated to this branch, confirmed passing in isolation). ruff check: clean. --- graphify/extract.py | 34 +++++++++++++-------- graphify/extractors/github_actions.py | 6 +++- tests/test_github_actions.py | 43 +++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/graphify/extract.py b/graphify/extract.py index dd6c422806..1db5119239 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -4994,6 +4994,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 @@ -5027,19 +5038,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", #OSAC-4049 - # review round 3). 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" (#OSAC-4049 review round 3). 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 0b63985fb2..36c5f12e60 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 604931ec27..a7d1ce3704 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