From 07df314e12c00aa5ae17ceffe44114b56931bfce Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:35:35 +0200 Subject: [PATCH] fix(cpg): prove the frontend can read the source before building, and check the graph always Two permanent guards, and a correction that motivates both. THE CORRECTION. There is no php2cpg defect. Every PHP build on this machine ran through a local `php` shim proxying stdio through `docker run`, and php2cpg depends on the INTERLEAVING of its parser's stderr banners with its stdout JSON to attribute each document to a file. The proxy scrambles it: streaming shim drops 72-88 767KB / 969KB / 1.2MB different every run buffered shim drops 0 5,703 bytes empty, and silent NATIVE php drops 0 2,872,996 bytes identical x3 The 637-file tree with native php: 0 drops, a 4,329,471-byte CPG, 20.0s, 1.9GB peak -- which is exactly what task 5.10 said before I started "correcting" it. No intermittent failures, no malformed graph, no dataflow assertion, no scale limit. GUARD 1: the interpreter must prove it can read the tree, before anything is built. Every instrument failure this project has had takes one shape -- the input was unreadable and the tool reported a plausible ZERO -- and it has cost four wrong diagnoses in a day: a php that could not see ~/.cache -> "cannot handle 637 files" a php whose stdio was proxied -> "drops half its batches" a container missing a bind mount -> "0 dropped", 5,703-byte graph None announce themselves: file_exists() is false, the parser writes nothing, the frontend exits 0, and "no errors" reads exactly like success. Checking the output cannot separate that from an empty repository. Checking that the input arrived can, and costs one exec. GUARD 2: the graph census is taken on EVERY build, not only one the frontend warned about. A graph holding nothing arrives with no warnings at all -- silence is the symptom that has no symptom -- and the shortfall is now reported as "the frontend reported no failures but the graph holds N of M source files". `cpg_build_failed` carries that detail through to the report instead of being a bare reason. Also adds tests/test_cpg_queries_compile.py: the queries are 900 lines of Scala that nothing compiled. A rowsFor signature change shipped with 850 tests passing and broke every query at runtime, found by a measurement failing hours later. Each query is now compiled AND run against a real graph wherever joern exists, and must return a parseable payload. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 24 ++++++- benchmarks/repos/README.md | 9 +++ src/openultrasast/cpg/backend.py | 85 +++++++++++++++++++++--- src/openultrasast/model/scan.py | 2 +- src/openultrasast/reports.py | 2 +- tests/test_cpg_backend.py | 96 +++++++++++++++++++++++++++ tests/test_cpg_queries_compile.py | 59 ++++++++++++++++ 7 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 tests/test_cpg_queries_compile.py diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index d6458d9..99d5b81 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -675,7 +675,29 @@ - The next measurement is therefore the per-request cost itself, and specifically what the two-stage joins cost at scale: they were 1.6x on a slice (62s -> 97s) and are plausibly the dominant term here. Until that is known, raising the timeout would only buy a slower way to find out. - - Still open: that measurement; and nothing here recovers a cross-shard flow. + - **ALL OF THE ABOVE ABOUT php2cpg IS WRONG. There is no frontend defect. 2026-09-10.** Every PHP build on + this machine ran through a local `php` shim that proxies stdio through `docker run`, and php2cpg depends + on the INTERLEAVING of its parser's stderr banners with its stdout JSON to attribute each document to a + file. The proxy scrambles that ordering. Measured on the same 177-file tree: + + streaming shim drops 72-88 767KB / 969KB / 1.2MB different every run + buffered shim drops 0 5,703 bytes empty, and silent about it + NATIVE php drops 0 2,872,996 bytes byte-identical across three runs + + So: no intermittent batch failure, no ~50% drop rate, no malformed CPG, no dataflow assertion, and no + scale limit. The `AssertionError: astParent ... has two parents` was a graph the shim had corrupted. + - **The real 637-file envelope, native php:** `drops=0`, a **4,329,471-byte CPG**, **20.0s wall**, **1.9GB + peak RSS**. Which is what the ORIGINAL note in this task said -- "php2cpg builds a 4.3MB CPG for a + 637-file plugin with no errors" -- and it was right; everything written against it since was measured + through a broken instrument. + - Three of today's mechanisms were therefore built against an artifact: `_build_with_retries`, the sharded + build with its island CPG, and the census-before-split guard. They are inert on a correct instrument (no + drops means no retry and no split) and would still help against a genuine frontend failure, so they + stay -- but the evidence offered for them in their commit messages was not real, and this is the record + of that. + - Still open, and now measurable for the first time: whether the queries survive a well-formed 637-file + graph, what they cost, and what they find. Nothing here recovers a cross-shard flow, but on a correct + instrument there are no shards. - Still open: wiring that exclusion loop, the 637-file build with it in place, and whether the REST handler two calls away can reach the sink -- the slice found it through a same-function flow, not across the object. diff --git a/benchmarks/repos/README.md b/benchmarks/repos/README.md index 206bd6b..8978ed3 100644 --- a/benchmarks/repos/README.md +++ b/benchmarks/repos/README.md @@ -218,6 +218,15 @@ obvious question is whether any of it survives on the whole plugin. Measured on The build is affordable; a CI runner can do this without a vertical-scale machine, which was the open question. The scan is not. +**Correction, and it invalidates the paragraph below.** Every PHP build behind these numbers ran through a +local `php` shim that proxies stdio through `docker run`, and php2cpg depends on the interleaving of its +parser's stderr banners with its stdout JSON to attribute documents to files. The proxy scrambles that +ordering: 72–88 dropped files per build, a different graph every run, and a corrupted CPG that makes Joern's +dataflow overlay assert. With a **native** php the same 637-file tree builds in **20.0s** to a +**4,329,471-byte CPG** with **zero** drops, byte-identical across runs. There is no frontend defect, no scale +limit, and the whole-repository scan has never actually been measured here. What follows is kept as the +record of a wrong diagnosis, not as a result. + It is worth being precise about *why*, because the obvious answers are all wrong. It is not memory: at 2 GB and at 4 GB the CPG never finishes loading, with no `OutOfMemoryError` in either case, both runs stopping in the same place — `Braintree\Util`. It is not the request payload either. What costs is Joern's dataflow diff --git a/src/openultrasast/cpg/backend.py b/src/openultrasast/cpg/backend.py index d1bfb69..fbb135b 100644 --- a/src/openultrasast/cpg/backend.py +++ b/src/openultrasast/cpg/backend.py @@ -80,6 +80,11 @@ _FRONTENDS = { _PREFER_FRONTEND = frozenset({"php"}) # The extensions that count as source for a language, used to ask whether a graph is missing anything. _LANGUAGE_EXTENSIONS: dict[str, tuple[str, ...]] = {"php": (".php",)} +# Frontends that shell out to a separate interpreter, and the command that proves it can read a file. +# `php2cpg` drives PHP-Parser through whatever `php` is on PATH, so the frontend is only as good as that +# interpreter's view of the filesystem -- and a `php` that cannot see the tree does not fail, it produces an +# empty graph with exit status 0 and no warnings. +_INTERPRETERS: dict[str, str] = {"php": "php"} FRONTEND_BUILD_ATTEMPTS = 4 # Request fields that describe the REPOSITORY rather than the region, and are therefore identical in every @@ -184,6 +189,8 @@ class JoernBackend: query_timeout: int = QUERY_TIMEOUT_SECONDS heap_mb: int = 0 # 0 means read the environment, then fall back to CPG_HEAP_MB queries_dir: Path = field(default_factory=lambda: QUERIES_DIR) + # Why the last build refused, so the driver can name it instead of reporting a bare `cpg_build_failed`. + last_failure: str = "" def available(self) -> bool: from .capability import has_cpg @@ -202,6 +209,23 @@ class JoernBackend: scratch = Path(tempfile.mkdtemp(prefix="ousast-cpg-")) cpg_path = scratch / "cpg.bin" + # THE PRECONDITION. Before anything is built, make the toolchain open one file from the tree and say + # how big it is. Every instrument failure this project has had takes the same shape -- the input was + # unreadable and the tool reported a plausible ZERO -- and it has cost four wrong diagnoses: + # + # a php that could not see ~/.cache -> "php2cpg cannot handle 637 files" (it read none of them) + # a php whose stdio was proxied -> "php2cpg drops half its batches" (it dropped none) + # a container missing a bind mount -> "0 files dropped", 5,703-byte graph + # + # None of those announced themselves. `file_exists()` returns false, the parser writes nothing, the + # frontend exits 0, and "no errors" reads exactly like success. Checking the OUTPUT cannot separate + # those from a genuinely empty repository; checking that the INPUT arrived can, and costs one exec. + unreadable = self._interpreter_cannot_read(root, language) + if unreadable: + logger.error("cpg build refused for %s: %s", root, unreadable) + self.last_failure = unreadable + return None + # A frontend whose failures are visible and retryable is worth more than one whose are not. if language.lower() in _PREFER_FRONTEND: shards, unparsed = self._build_sharded(root, scratch, language) @@ -235,6 +259,36 @@ class JoernBackend: unparsed=unparsed, ) + def _interpreter_cannot_read(self, root: Path, language: str) -> str: + """``""`` when the language's interpreter can read the tree, else why not. + + Deliberately narrow: it proves one file is openable and non-empty FROM THE INTERPRETER'S OWN VIEW, + which is the thing that differs when `php` is a wrapper, a container without the right bind mount, or + a tree of symlinks pointing somewhere the sandbox cannot follow. Nothing else here can see that + difference, because every other signal is downstream of the parse that never happened. + """ + interpreter = _INTERPRETERS.get(language.lower()) + extensions = _LANGUAGE_EXTENSIONS.get(language.lower()) + if not interpreter or not extensions: + return "" + binary = shutil.which(interpreter) + if binary is None: + return "" # absent is a different failure, and the frontend reports it plainly + sample = next((path for path in sorted(root.rglob("*")) if path.suffix.lower() in extensions and path.is_file()), None) + if sample is None: + return "" # nothing of this language to read; not the interpreter's fault + probe = "$f = $argv[1]; if (!is_readable($f) || filesize($f) < 1) { exit(3); } exit(0);" + completed = self._run([binary, "-r", probe, str(sample)], timeout=60) + if completed is None: + return f"the {interpreter} interpreter did not respond when asked to read {sample.name}" + if completed.returncode != 0: + return ( + f"the {interpreter} interpreter cannot read {sample}, so the frontend would parse nothing and " + f"write an empty graph. Check that {interpreter} is a real interpreter with access to this " + f"tree -- a container or wrapper without the right mount fails exactly this way." + ) + return "" + def _build_sharded(self, root: Path, scratch: Path, language: str) -> tuple[tuple[Path, ...], tuple[str, ...]]: """Build the tree, and give the files it refuses a CPG of their OWN rather than losing them. @@ -254,6 +308,12 @@ class JoernBackend: if dropped is None: return (), () if not dropped: + # A build that warned about nothing is exactly the build that needs checking, because a graph + # holding nothing arrives with no warnings at all. Silence is the symptom that has no symptom. + census = self._graph_census(main, root, language) + if census is not None and census[0] < census[1]: + self.last_failure = f"the frontend reported no failures but the graph holds {census[0]} of {census[1]} source files" + logger.error("cpg build for %s: %s", root, self.last_failure) return (main,), () # A warning is a symptom, not a verdict. `php2cpg` logs `Failed to process` for files that are @@ -282,26 +342,31 @@ class JoernBackend: return tuple(shards), tuple(still) return tuple(shards), dropped - def _graph_is_complete(self, cpg_path: Path, root: Path, language: str) -> bool: - """Does this graph hold at least as many files as the tree has sources? + def _graph_census(self, cpg_path: Path, root: Path, language: str) -> tuple[int, int] | None: + """``(files_in_graph, files_expected)``, or ``None`` when the question cannot be asked. - Conservative in the only direction that matters: when the census cannot be taken, or the language - has no declared extensions, the answer is "no" and the caller does the careful thing. + Always asked, not only when the frontend warned. The dangerous build is the SILENT one: a graph that + holds nothing, reported with no errors at all, which is what an unreadable tree produces and what a + genuinely clean scan of an empty repository looks like. A warning is a symptom; this is the evidence. """ extensions = _LANGUAGE_EXTENSIONS.get(language.lower()) if not extensions: - return False + return None expected = sum(1 for path in root.rglob("*") if path.suffix.lower() in extensions and path.is_file()) if not expected: - return False + return None payload = self.query(cpg_path, "census", {}) if not isinstance(payload, Mapping): - return False + return None try: - files = int(str(payload.get("files", "0"))) + return int(str(payload.get("files", "0"))), expected except ValueError: - return False - return files >= expected + return None + + def _graph_is_complete(self, cpg_path: Path, root: Path, language: str) -> bool: + """Conservative: when the census cannot be taken, the answer is "no" and the caller is careful.""" + census = self._graph_census(cpg_path, root, language) + return census is not None and census[0] >= census[1] def _island_root(self, root: Path, scratch: Path, files: Sequence[str]) -> Path | None: """A tree holding only ``files``, at their original relative paths. diff --git a/src/openultrasast/model/scan.py b/src/openultrasast/model/scan.py index 55f5a69..a37a693 100644 --- a/src/openultrasast/model/scan.py +++ b/src/openultrasast/model/scan.py @@ -123,7 +123,7 @@ def scan_repository( regions_unjudged=len(regions), seconds=round(time.monotonic() - started, 2), build_seconds=build_seconds, - degradations=({"stage": "model", "reason": "cpg_build_failed"},), + degradations=({"stage": "model", "reason": "cpg_build_failed", "detail": getattr(backend, "last_failure", "")},), ) # A CPG the frontend built with files missing is not the repository the caller asked about, and no diff --git a/src/openultrasast/reports.py b/src/openultrasast/reports.py index be66bd7..9f019a5 100644 --- a/src/openultrasast/reports.py +++ b/src/openultrasast/reports.py @@ -427,7 +427,7 @@ def _append_coverage(lines: list[str], coverage: Sequence[Mapping[str, object]], # silence in the table above is narrower than it looks. _COVERAGE_DEGRADATIONS: dict[str, str] = { "cpg_empty": "The code graph came back with no methods in it, so **nothing above was analysed at all**.", - "cpg_build_failed": "The code graph could not be built, so nothing above was analysed.", + "cpg_build_failed": "The code graph could not be built, so nothing above was analysed. {detail}", "cpg_sharded": ( "The graph had to be built in {shards} parts, because the frontend could not parse the whole tree at " "once. Each part was queried, but **a flow whose source is in one part and whose sink is in another " diff --git a/tests/test_cpg_backend.py b/tests/test_cpg_backend.py index b6c504f..a067c14 100644 --- a/tests/test_cpg_backend.py +++ b/tests/test_cpg_backend.py @@ -402,6 +402,8 @@ def test_php_builds_through_the_frontend_and_retries_while_files_are_dropped(tmp warning = "WARN AstCreationPass Failed to process '/src/big.php'\n" def runner(command, **kwargs): # type: ignore[no-untyped-def] + if "-r" in command: + return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr="") tried.append(Path(command[0]).name) Path(command[command.index("-o") + 1]).write_text("cpg") # Drops a file on the first two attempts, clean on the third. @@ -428,6 +430,8 @@ def test_a_php_build_that_never_stops_dropping_files_reports_them(tmp_path: Path def runner(command, **kwargs): # type: ignore[no-untyped-def] nonlocal attempts + if "-r" in command: + return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr="") attempts += 1 Path(command[command.index("-o") + 1]).write_text("cpg") return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr="WARN Failed to process '/src/stubborn.php'\n") @@ -456,6 +460,8 @@ def test_files_the_frontend_refuses_get_a_cpg_of_their_own(tmp_path: Path, monke def runner(command, **kwargs): # type: ignore[no-untyped-def] commands.append(list(command)) + if "-r" in command: + return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr="") # the input probe if "--script" in command: # The census, asked because the frontend warned. Here the warning is real: the graph is short a # file, so the caller should go on to exclude it and build it separately. @@ -535,6 +541,8 @@ def test_a_warning_about_a_file_the_graph_actually_holds_does_not_split_it(tmp_p def runner(command, **kwargs): # type: ignore[no-untyped-def] commands.append(list(command)) + if "-r" in command: + return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr="") # the input probe if "--script" in command: body = '---OUSAST-CPG-BEGIN---\n{"files": "2", "methods": "9"}\n---OUSAST-CPG-END---\n' return subprocess.CompletedProcess(args=command, returncode=0, stdout=body, stderr="") @@ -549,3 +557,91 @@ def test_a_warning_about_a_file_the_graph_actually_holds_does_not_split_it(tmp_p assert result is not None assert result.unparsed == (), "the graph holds both files, so nothing is missing" assert not any("--exclude" in command for command in commands), "and it was not split" + + +def test_a_build_is_refused_when_the_interpreter_cannot_read_the_tree(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The failure this project keeps having: the input is unreadable and the tool reports a plausible ZERO. + + `php2cpg` drives PHP-Parser through whatever `php` is on PATH. Give it a `php` that cannot see the tree + -- a wrapper, a container missing a bind mount, symlinks pointing outside a sandbox -- and it does not + fail. It parses nothing, writes a graph with no methods in it, and exits 0 with no warnings. That has + cost this project four wrong diagnoses, including "php2cpg cannot handle 637 files" from a run that read + none of them. + + Checking the output cannot tell that from an empty repository. Checking that the input arrived can. + """ + import subprocess + + from openultrasast.cpg.backend import JoernBackend + + (tmp_path / "a.php").write_text(" None: + """The probe must not stand in the way of the ordinary case.""" + import subprocess + + from openultrasast.cpg.backend import JoernBackend + + (tmp_path / "a.php").write_text(" None: + """A build that warns about nothing is exactly the build that needs checking. + + A graph holding nothing arrives with no warnings at all -- silence is the symptom that has no symptom -- + so the census is taken on every build, not only on one the frontend complained about. + """ + import subprocess + + from openultrasast.cpg.backend import JoernBackend + + for name in ("a.php", "b.php", "c.php"): + (tmp_path / name).write_text(" str | None: + import shutil + + return shutil.which("joern") + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +@pytest.mark.parametrize("name", ["taint", "dominance", "config", "census"]) +def test_each_query_compiles_and_answers(name: str, tmp_path: Path) -> None: + """Compile and RUN each query against a graph, and require a fenced payload back. + + Compiling alone would miss a runtime arity error inside a branch, so the query has to answer -- and the + answer has to parse, because a script that prints a stack trace still exits 0 often enough to matter. + """ + from openultrasast.cpg.backend import extract_payload + + script = QUERIES / f"{name}.sc" + assert script.is_file(), f"{script} is missing" + + source = tmp_path / "sample.php" + source.write_text("query($x);\n}\n") + cpg = tmp_path / "cpg.bin" + frontend = __import__("shutil").which("php2cpg") + if frontend is None: + pytest.skip("php2cpg is not installed") + built = subprocess.run([frontend, str(tmp_path), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + + command = [_joern() or "joern", "--script", str(script.resolve()), "--param", f"cpgFile={cpg}"] + if name != "census": + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"0": {"sources": "$_GET", "sinks": "$wpdb->query", "function": "handler"}})) + command += ["--param", f"requestsFile={requests}"] + done = subprocess.run(command, capture_output=True, text=True, timeout=900, check=False) + + assert done.returncode == 0, f"{name}.sc did not run: {(done.stderr or done.stdout or '')[-500:]}" + assert extract_payload(done.stdout or "") is not None, f"{name}.sc produced no parseable payload"