diff --git a/CHANGELOG.md b/CHANGELOG.md index f87cb0404..626fac385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Full release notes with details on each version: [GitHub Releases](https://githu ## Unreleased +- Fix: a malformed semantic chunk no longer crashes `extract` and discards every successful chunk (#1631, thanks @ssazy). When an LLM returned a well-formed object whose `edges` (or `nodes`/`hyperedges`) array carried a stray non-dict entry — a nested list where an edge object belongs — the AST+semantic merge and the semantic-cache write both called `.get()` per entry and raised `AttributeError: 'list' object has no attribute 'get'`. On a 34-chunk run where 33 succeeded, that meant no `graph.json` was written and the cache write failed too, so a re-run re-extracted everything. `_parse_llm_json` now sanitizes each fragment at the single parse chokepoint (keeping only dict entries and coercing a non-list value to `[]`), so the cache writer, the adaptive-retry merge, and the CLI merge are all protected in one place. +- Fix: an unresolved bare npm import no longer aliases onto an unrelated same-named local file (#1638, thanks @EveX1). `import colors from "tailwindcss/colors"` in a `.tsx` file emitted an `imports_from` edge to the bare id `colors`, and build.py's pre-migration alias index (which registers every local file's bare stem) then remapped it onto an unrelated `backend/utils/colors.py` — a confident (`EXTRACTED`) cross-language phantom edge, and one per `.tsx` file sharing the import. In a real monorepo eight unrelated `.tsx` files all landed on a single Python module. Common package subpaths (`colors`, `utils`, `types`, `config`, `client`) collide this way constantly. The external-import fallback now namespaces its target with the `ref` prefix (the same J-4 convention used for tsconfig `extends`/`$ref` externals), so it can never collapse to a local file/symbol id; the ref-namespaced target has no node, so build drops it as an external reference — the correct outcome for a third-party import. +- Fix: `graph.json` node/edge ordering is now stable run-to-run for document/semantic corpora (#1632, thanks @umeshpsatwe). With a parallel LLM backend, `extract_corpus_parallel` merged chunk results in completion order, so which network call happened to return first reordered the nodes and edges even when the model returned identical content — churning `graph.json` between otherwise-identical runs. Chunks are now merged in deterministic submission order after the pool drains (matching the serial path); the progress callback still fires in completion order so long local runs aren't silent. Note: the semantic content the LLM extracts is itself nondeterministic run-to-run — this fix removes the pipeline's own ordering churn, not the model's variance. + - Fix: `graphify export obsidian` no longer crashes in `to_canvas` on a dangling community member (#1236 follow-up, thanks @swells808). The original #1236 fix guarded `to_obsidian` but not `to_canvas`, so a community member id with no backing node in the graph still raised `KeyError` while writing `graph.canvas` — after the notes had exported, leaving a partial mirror. `to_canvas` now applies the same dangling-member filter (`m in G and m in node_filenames`) in both the box-sizing and card-layout loops. - Feat: TS/JS member calls on a local `new` binding or a type-annotated parameter now resolve (#1630, thanks @DanielC000). `const s = new Svc(); s.doThing()` and a call on a typed param — including inside a returned closure (`(svc: Svc) => () => svc.doThing()`) — now emit `calls` edges to the receiver type's method, so `affected` no longer silently under-reports. Extends the #1316 `this.field` resolver: the per-file type table now also learns local `new` bindings and bare-typed parameters, and `walk_calls` descends into inline/returned closures (attributing their calls to the enclosing function) instead of stopping at the arrow boundary. Resolution keeps the single-definition guard; an untyped or non-bare-typed (array/union/generic) receiver produces no edge. diff --git a/graphify/extract.py b/graphify/extract.py index eb3d24aab..f1cb548d6 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -1771,7 +1771,17 @@ def _resolve_js_import_target(raw: str, str_path: str) -> "tuple[str, Path | Non module_name = raw.split("/")[-1] if not module_name: return None - return _make_id(module_name), None + # Unresolved: relative/absolute, tsconfig-alias and workspace resolution have + # all run and failed, so this is an external package (or a dangling local + # path). Namespace the id with the "ref" prefix — the J-4 convention already + # used for tsconfig `extends`/`$ref` externals — so it can NEVER collapse to + # the same _make_id as a local file/symbol node. Without it, the bare + # last-segment id (e.g. "tailwindcss/colors" -> "colors") collides with any + # unrelated local file of that stem via build.py's pre-migration alias index, + # producing a confident (EXTRACTED) cross-language phantom imports_from edge + # (#1638). The ref-namespaced target has no node, so build drops it as an + # external reference — the correct outcome for a third-party import. + return _make_id("ref", raw), None def _import_js(node, source: bytes, file_nid: str, stem: str, edges: list, str_path: str, scope_stack: list[str] | None = None) -> None: diff --git a/graphify/llm.py b/graphify/llm.py index 44e98a8ca..16644e6dc 100644 --- a/graphify/llm.py +++ b/graphify/llm.py @@ -759,6 +759,30 @@ def _bedrock_content(user_message: str, refs: list[_ImageRef]) -> list[dict]: _LLM_JSON_MAX_BYTES = 10 * 1024 * 1024 # 10 MB hard cap before json.loads (F-016) +def _sanitize_fragment(parsed: dict) -> dict: + """Force ``nodes``/``edges``/``hyperedges`` to lists of dicts, in place. + + A model can return a well-formed top-level object whose ``edges`` (or + ``nodes``/``hyperedges``) array contains a stray non-dict entry — most often + a nested list where an edge object belongs, or the whole value being a bare + array/scalar instead of a list. Those entries slip past JSON parsing but + blow up every downstream consumer that calls ``.get()`` per entry + (semantic-cache write and the AST+semantic merge both did — #1631, crashing + with ``'list' object has no attribute 'get'`` and discarding all successful + chunks). Sanitizing here, at the single parse chokepoint, protects the cache + writer, the adaptive-retry merge, and the CLI merge in one place. + """ + for key in ("nodes", "edges", "hyperedges"): + value = parsed.get(key) + if value is None: + continue + if not isinstance(value, list): + parsed[key] = [] + continue + parsed[key] = [entry for entry in value if isinstance(entry, dict)] + return parsed + + def _parse_llm_json(raw: str) -> dict: """Strip optional markdown fences and parse JSON. Returns empty fragment on failure. @@ -792,7 +816,7 @@ def _parse_llm_json(raw: str) -> dict: try: parsed = json.loads(stripped) if isinstance(parsed, dict): - return parsed + return _sanitize_fragment(parsed) # Top-level array/scalar (common LLM output) is not a usable graph # fragment; fall through to the next strategy rather than returning a # non-dict that callers will try to subscript (e.g. result["input_tokens"]). @@ -827,7 +851,7 @@ def _parse_llm_json(raw: str) -> dict: try: parsed = json.loads(stripped[start : i + 1]) if isinstance(parsed, dict): - return parsed + return _sanitize_fragment(parsed) break except json.JSONDecodeError: break @@ -1862,6 +1886,14 @@ def extract_corpus_parallel( if callable(on_chunk_done): on_chunk_done(idx, total, result) else: + # Merge in deterministic submission order, NOT completion order. Merging + # as chunks finish makes the node/edge ordering in the returned corpus + # (and therefore graph.json) depend on which network call happened to + # return first — so identical input churned run-to-run (#1632). Collect + # results keyed by chunk index and merge in sorted order after the pool + # drains; this matches the serial path's order. The progress callback + # still fires in completion order so long local runs aren't silent. + results_by_idx: dict[int, dict] = {} with ThreadPoolExecutor(max_workers=workers) as pool: futures = [pool.submit(_run_one, idx, chunk) for idx, chunk in enumerate(chunks)] for future in as_completed(futures): @@ -1874,9 +1906,11 @@ def extract_corpus_parallel( merged["failed_chunks"] += 1 continue assert result is not None - _merge_into(merged, result) + results_by_idx[idx] = result if callable(on_chunk_done): on_chunk_done(idx, total, result) + for idx in sorted(results_by_idx): + _merge_into(merged, results_by_idx[idx]) # Loud failure summary — surface chunk failures at end so they're never # buried mid-log. Exit 0 preserved for caller compatibility; the diff --git a/tests/test_chunking.py b/tests/test_chunking.py index 087464ab8..21b28eec4 100644 --- a/tests/test_chunking.py +++ b/tests/test_chunking.py @@ -201,6 +201,53 @@ def test_corpus_parallel_sequential_when_max_concurrency_is_one(tmp_path): assert call_order == [("f0.py",), ("f1.py",), ("f2.py",)] +def test_corpus_parallel_merge_order_is_submission_order_not_completion(tmp_path): + """#1632: merged node/edge order must be deterministic (submission order), + not the order chunks' network calls happen to finish. We skew latencies so + the first-submitted chunk finishes LAST; the merged result must still be in + file/submission order so graph.json is stable run-to-run.""" + from graphify.llm import extract_corpus_parallel + + files = [] + for i in range(4): + f = tmp_path / f"f{i}.py"; f.write_text("x") + files.append(f) + + def latency_skewed(chunk, **kwargs): + # chunk is a single file (chunk_size=1). Earlier files sleep longer, so + # completion order is the reverse of submission order. + name = chunk[0].name # f0.py .. f3.py + idx = int(name[1]) + time.sleep(0.05 * (4 - idx)) # f0 sleeps 0.20s, f3 sleeps 0.05s + return { + "nodes": [{"id": f"node_from_{name}"}], + "edges": [{"source": f"node_from_{name}", "target": "t"}], + "hyperedges": [], + "input_tokens": 1, + "output_tokens": 1, + } + + with patch("graphify.llm.extract_files_direct", side_effect=latency_skewed): + result = extract_corpus_parallel( + files, backend="kimi", token_budget=None, chunk_size=1, max_concurrency=4 + ) + + node_ids = [n["id"] for n in result["nodes"]] + assert node_ids == [ + "node_from_f0.py", + "node_from_f1.py", + "node_from_f2.py", + "node_from_f3.py", + ], f"merge order not deterministic: {node_ids}" + edge_srcs = [e["source"] for e in result["edges"]] + assert edge_srcs == [ + "node_from_f0.py", + "node_from_f1.py", + "node_from_f2.py", + "node_from_f3.py", + ], f"edge merge order not deterministic: {edge_srcs}" + + def test_corpus_parallel_continues_after_chunk_failure(tmp_path, capsys): """A single chunk raising should be logged but not abort the run. Other chunks' results should still be merged.""" diff --git a/tests/test_phantom_external_import.py b/tests/test_phantom_external_import.py new file mode 100644 index 000000000..c30f334ab --- /dev/null +++ b/tests/test_phantom_external_import.py @@ -0,0 +1,116 @@ +"""#1638 — an unresolved bare npm import must not alias onto an unrelated +same-named local file, producing a confident cross-language phantom edge. + +`import colors from "tailwindcss/colors"` in a .tsx file used to emit an +`imports_from` edge to the bare id ``colors``. build.py's pre-migration alias +index registers every local file's bare stem (``backend/utils/colors.py`` -> +alias ``colors``), so the dangling ``colors`` target was remapped onto the +Python file — an EXTRACTED-confidence edge between two files in different +languages with no real relationship. + +The fix namespaces the external-import fallback id with the ``ref`` prefix (the +J-4 convention), so it can never collide with a local file/symbol node id. +""" +from __future__ import annotations + +from pathlib import Path + +from graphify.build import build_from_json +from graphify.extract import _make_id, _resolve_js_import_target, extract + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +# ── unit: the resolver never returns a bare local-shaped id for an external ── + + +def test_unresolved_bare_import_is_ref_namespaced(): + tgt, resolved_path = _resolve_js_import_target( + "tailwindcss/colors", "frontend/src/SomeChart.tsx" + ) + assert resolved_path is None + # Must not be the bare last-segment id that collides with a local `colors` file. + assert tgt != _make_id("colors") + assert tgt != _make_id("colors.py") + assert tgt.startswith("ref") + + +def test_scoped_package_import_is_ref_namespaced(): + tgt, resolved_path = _resolve_js_import_target( + "@scope/utils", "src/thing.ts" + ) + assert resolved_path is None + assert tgt != _make_id("utils") + assert tgt.startswith("ref") + + +# ── end-to-end: the reporter's exact synthetic monorepo ───────────────────── + + +def test_no_phantom_edge_from_tsx_to_unrelated_python_file(tmp_path: Path): + py = _write( + tmp_path / "backend/utils/colors.py", + "def hex_to_rgb(value):\n return (0, 0, 0)\n", + ) + tsx = _write( + tmp_path / "frontend/src/SomeChart.tsx", + 'import colors from "tailwindcss/colors";\n\n' + "export const CHART_COLOR = colors.blue[500];\n", + ) + + result = extract([py, tsx], cache_root=tmp_path / "graphify-out") + G = build_from_json(result, root=str(tmp_path)) + + # Find the python file node. + py_ids = [ + n for n, d in G.nodes(data=True) + if str(d.get("source_file", "")).endswith("colors.py") + ] + assert py_ids, "colors.py should have produced at least one node" + + # No edge from the TSX file (or any TS symbol) should land on the python file + # as an imports_from relationship. + for u, v, d in G.edges(data=True): + if d.get("relation") != "imports_from": + continue + endpoints = {u, v} + if endpoints & set(py_ids): + other = (endpoints - set(py_ids)) or endpoints + srcfiles = {str(G.nodes[e].get("source_file", "")) for e in other} + assert not any(sf.endswith((".tsx", ".ts")) for sf in srcfiles), ( + f"phantom cross-language imports_from edge onto colors.py: " + f"{u} -> {v} ({d})" + ) + + +def test_multiple_tsx_files_do_not_all_alias_onto_one_python_file(tmp_path: Path): + # The real-world symptom: N unrelated .tsx files all doing the same bare + # import showed up as N imports_from sources on one python module. + _write( + tmp_path / "backend/utils/colors.py", + "def hex_to_rgb(value):\n return (0, 0, 0)\n", + ) + for i in range(3): + _write( + tmp_path / f"frontend/src/Chart{i}.tsx", + 'import colors from "tailwindcss/colors";\n' + f"export const C{i} = colors.blue;\n", + ) + + paths = list((tmp_path).rglob("*.py")) + list((tmp_path / "frontend").rglob("*.tsx")) + result = extract(paths, cache_root=tmp_path / "graphify-out") + G = build_from_json(result, root=str(tmp_path)) + + py_ids = { + n for n, d in G.nodes(data=True) + if str(d.get("source_file", "")).endswith("colors.py") + } + phantom = [ + (u, v) for u, v, d in G.edges(data=True) + if d.get("relation") == "imports_from" and ({u, v} & py_ids) + ] + assert not phantom, f"phantom edges onto colors.py: {phantom}" diff --git a/tests/test_semantic_fragment_sanitize.py b/tests/test_semantic_fragment_sanitize.py new file mode 100644 index 000000000..5ab2f9204 --- /dev/null +++ b/tests/test_semantic_fragment_sanitize.py @@ -0,0 +1,71 @@ +"""#1631 — a malformed LLM chunk (a stray non-dict entry in edges/nodes) must +not crash the merge/cache-write and discard all successful chunks. + +`sem_result["edges"]` could contain a bare list where an edge object belongs +(a JSON array slipping past parse). Downstream code calls ``.get()`` per entry +(the AST+semantic merge at __main__.py and the semantic-cache writer both did), +raising ``AttributeError: 'list' object has no attribute 'get'`` and losing all +33 successful chunks. `_parse_llm_json` now sanitizes the fragment at the single +parse chokepoint so every consumer only ever sees lists of dicts. +""" +from __future__ import annotations + +import json + +from graphify.llm import _parse_llm_json, _sanitize_fragment + + +def test_sanitize_drops_non_dict_edge_entries(): + frag = { + "nodes": [{"id": "a"}, ["not", "a", "dict"], "bare-string", {"id": "b"}], + "edges": [{"source": "a", "target": "b"}, ["stray", "list"], 42], + "hyperedges": [{"id": "h"}, None], + } + out = _sanitize_fragment(frag) + assert out["nodes"] == [{"id": "a"}, {"id": "b"}] + assert out["edges"] == [{"source": "a", "target": "b"}] + assert out["hyperedges"] == [{"id": "h"}] + + +def test_sanitize_coerces_non_list_values_to_empty(): + frag = {"nodes": {"id": "oops"}, "edges": "nope", "hyperedges": None} + out = _sanitize_fragment(frag) + assert out["nodes"] == [] + assert out["edges"] == [] + # None is left as-is (absent key semantics) — the guard only fixes lists/values + assert out.get("hyperedges") is None + + +def test_parse_llm_json_sanitizes_stray_list_in_edges(): + raw = json.dumps({ + "nodes": [{"id": "a"}], + "edges": [{"source": "a", "target": "b"}, ["malformed"]], + "hyperedges": [], + }) + parsed = _parse_llm_json(raw) + # Every entry that survives must be a dict so downstream .get() is safe. + for key in ("nodes", "edges", "hyperedges"): + assert all(isinstance(x, dict) for x in parsed.get(key, [])) + assert parsed["edges"] == [{"source": "a", "target": "b"}] + + +def test_parse_llm_json_fenced_response_is_sanitized(): + raw = ( + "Here you go:\n\n```json\n" + + json.dumps({"nodes": [["bad"], {"id": "ok"}], "edges": []}) + + "\n```\n" + ) + parsed = _parse_llm_json(raw) + assert parsed["nodes"] == [{"id": "ok"}] + + +def test_merge_after_sanitize_does_not_raise_on_source_file_access(): + # Mirrors the __main__.py comprehension that crashed: e.get("source_file", ""). + parsed = _parse_llm_json(json.dumps({ + "nodes": [{"id": "a", "source_file": "d.md"}], + "edges": [{"source": "a", "target": "b"}, ["oops"]], + })) + # This is exactly the pattern at __main__.py:4858-4860. + seen = {n.get("source_file", "") for n in parsed.get("nodes", [])} + seen |= {e.get("source_file", "") for e in parsed.get("edges", [])} + assert "d.md" in seen diff --git a/tests/test_ts_import_require.py b/tests/test_ts_import_require.py index 4612e6c23..43f186116 100644 --- a/tests/test_ts_import_require.py +++ b/tests/test_ts_import_require.py @@ -54,7 +54,11 @@ def test_import_require_single_quotes(tmp_path: Path): assert _has_edge(result, "src/main.ts", "src/util.ts") -def test_import_require_bare_module_targets_stub(tmp_path: Path): +def test_import_require_bare_module_targets_ref_stub(tmp_path: Path): + # A bare module (`require("fs")`) is external, so it emits an imports_from + # edge to a ref-namespaced stub — NOT the bare `_make_id("fs")` id, which + # would collide with any local file named fs.* via build.py's alias index + # (#1638). Parity with the ESM external path (test_external_module_unchanged). importer = _write( tmp_path / "src/io.ts", 'import fs = require("fs");\nexport const data = fs.readFileSync("x");\n', @@ -63,11 +67,15 @@ def test_import_require_bare_module_targets_stub(tmp_path: Path): result = extract([importer], cache_root=tmp_path) src = _file_node_id(Path("src/io.ts")) - tgt = _make_id("fs") - assert any( - e["source"] == src and e["target"] == tgt and e["relation"] == "imports_from" - for e in result["edges"] - ), "bare-module import-equals should target the module-name stub, like ESM" + import_targets = { + e["target"] for e in result["edges"] + if e["source"] == src and e["relation"] == "imports_from" + } + # An external stub edge still exists... + assert import_targets, "bare-module import-equals should still emit an external stub edge" + # ...but it is ref-namespaced and never the bare, collision-prone id. + assert _make_id("fs") not in import_targets + assert any(t.startswith("ref") for t in import_targets), import_targets def test_import_require_parity_with_namespace_import(tmp_path: Path):