From 0d4f019e3187b885687fe20cb9d7558b607a40e0 Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Mon, 29 Jun 2026 20:09:20 +0200 Subject: [PATCH 1/7] Update graphify-py submodule to 544f95e --- graphify-py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphify-py b/graphify-py index 92e682f..544f95e 160000 --- a/graphify-py +++ b/graphify-py @@ -1 +1 @@ -Subproject commit 92e682f1de69a717785373fd8d84e113e400402a +Subproject commit 544f95efa65c56af2cdc22cad14750839005e76d From 52c53fcda0eeaea606193a0aa1c1cd54d614c7d0 Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Mon, 29 Jun 2026 22:29:43 +0200 Subject: [PATCH 2/7] Resync with graphify-py @ 544f95e Port the v0.9.0..v0.9.2 changes from the `graphify-py` reference into the Rust workspace. Extraction (`graphify-extract`): - tsconfig `paths` keep all targets, trying each on disk (#1531) - package.json `exports` subpath map with condition priority, a single `./*` wildcard, and a path-escape guard (#1308) - alias/workspace import targets remap via the absolute-resolved id form so relative-input runs do not orphan symbol edges (#1529) - skip Java type-parameter references; emit record-component references (#1518, #1519) - type-aware Ruby member-call resolution: `Class.new` and typed `var.method` resolve by receiver type, dispatched through a small resolver registry (#1499) - Swift type-qualified static calls are EXTRACTED, locally-inferred instance calls stay INFERRED (#1533) - ObjC: blank `NS_ASSUME_NONNULL` macros, resolve a quoted `#import` to the real header, emit `[[Foo alloc] init]` type references (#1475) - salt separator-colliding node ids with a path hash; repoint ObjC header imports to the disambiguated `.h` node (#1522, #1475) - record `origin_file` on the Go reference stub; drop `origin_file` from extract output so no absolute path leaks (#1515, #1516) Other crates: - `prune_semantic_cache` sweeps orphaned content-hash entries, wired into the extract command (#1527) - GraphML export pins null-attribute coercion to the empty string (#1502) - `GRAPHIFY_MAX_RETRIES` plus a 429 retry loop around the LLM HTTP sends; the secondary dispatch path keeps its timeout (#1523, #1442) - `graphify update` prunes edges owned by a re-extracted file (#1521) - skill and AGENTS install strings drop the host-specific `skill` tool reference (#1530) - `save-result --answer-file` reads the answer from a file (#1502) Bump the workspace version to 0.9.2 in lockstep with the submodule pointer and document `--answer-file` and the retry env vars in `USAGE.md`. Glory to the Omnissiah --- Cargo.lock | 64 +-- Cargo.toml | 2 +- USAGE.md | 5 + crates/graphify-cache/src/lib.rs | 4 +- crates/graphify-cache/src/semantic.rs | 48 +++ crates/graphify-cache/tests/parity.rs | 145 ++++++- crates/graphify-export/tests/parity.rs | 32 ++ crates/graphify-extract/Cargo.toml | 2 + .../src/extractors/dm/source.rs | 1 + .../graphify-extract/src/extractors/elixir.rs | 1 + .../src/extractors/go/calls.rs | 1 + .../src/extractors/go/refs.rs | 8 +- .../src/extractors/multi/cache.rs | 8 + .../src/extractors/multi/mod.rs | 123 +++--- .../src/extractors/multi/resolvers.rs | 99 +++++ .../src/extractors/multi/ruby.rs | 158 ++++++++ .../src/extractors/multi/swift.rs | 17 +- .../graphify-extract/src/extractors/objc.rs | 146 +++++-- .../src/extractors/powershell/mod.rs | 1 + .../src/extractors/rust_lang/calls.rs | 1 + .../graphify-extract/src/extractors/svelte.rs | 35 +- crates/graphify-extract/src/extractors/zig.rs | 1 + crates/graphify-extract/src/generic/calls.rs | 33 ++ .../graphify-extract/src/generic/js_extra.rs | 10 +- crates/graphify-extract/src/generic/mod.rs | 15 + .../src/generic/references/java.rs | 82 +++- crates/graphify-extract/src/generic/ruby.rs | 99 +++++ crates/graphify-extract/src/generic/walk.rs | 67 ++++ .../graphify-extract/src/import_handlers.rs | 2 +- crates/graphify-extract/src/lang_configs.rs | 2 +- crates/graphify-extract/src/postprocess.rs | 118 +++++- crates/graphify-extract/src/tsconfig.rs | 70 +++- crates/graphify-extract/src/types.rs | 6 + crates/graphify-extract/src/workspace.rs | 111 +++++- .../tests/cross_file_multi.rs | 13 - crates/graphify-extract/tests/id_collision.rs | 179 +++++++++ .../tests/java_type_resolution.rs | 37 +- .../tests/js_alias_relative_paths.rs | 128 ++++++ .../tests/js_workspace_resolution.rs | 365 ++++++++++++++++++ .../graphify-extract/tests/objc_resolution.rs | 256 ++++++++++++ crates/graphify-extract/tests/parity.rs | 2 +- .../tests/parity_languages.rs | 121 ++++++ .../tests/parity_postprocess.rs | 1 + .../graphify-extract/tests/ruby_resolution.rs | 202 ++++++++++ .../tests/swift_member_calls.rs | 91 +++-- .../src/platform/common/markdown.rs | 2 +- .../src/platform/common/skills.rs | 8 +- crates/graphify-hooks/tests/parity.rs | 24 ++ crates/graphify-llm/src/azure.rs | 20 +- crates/graphify-llm/src/call.rs | 16 +- crates/graphify-llm/src/claude.rs | 16 +- crates/graphify-llm/src/kimi.rs | 20 +- crates/graphify-llm/src/openai_compat.rs | 70 +++- .../tests/openai_compat_helpers.rs | 21 +- crates/graphify-llm/tests/per_backend_http.rs | 61 +++ .../src/rebuild/pipeline_helpers.rs | 38 +- .../graphify-watch/tests/rebuild_pipeline.rs | 60 +++ src/cli/args.rs | 6 +- src/cli/dispatch.rs | 11 + src/cli/extract.rs | 26 ++ tests/cli_commands.rs | 47 +++ 61 files changed, 3064 insertions(+), 294 deletions(-) create mode 100644 crates/graphify-extract/src/extractors/multi/resolvers.rs create mode 100644 crates/graphify-extract/src/extractors/multi/ruby.rs create mode 100644 crates/graphify-extract/src/generic/ruby.rs create mode 100644 crates/graphify-extract/tests/id_collision.rs create mode 100644 crates/graphify-extract/tests/js_alias_relative_paths.rs create mode 100644 crates/graphify-extract/tests/objc_resolution.rs create mode 100644 crates/graphify-extract/tests/ruby_resolution.rs diff --git a/Cargo.lock b/Cargo.lock index 369f0ee..17f7381 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1581,7 +1581,7 @@ dependencies = [ [[package]] name = "graphify" -version = "0.9.0" +version = "0.9.2" dependencies = [ "anyhow", "assert_cmd", @@ -1621,7 +1621,7 @@ dependencies = [ [[package]] name = "graphify-affected" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "graphify-security", @@ -1634,7 +1634,7 @@ dependencies = [ [[package]] name = "graphify-analyze" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "graphify-cluster", @@ -1646,7 +1646,7 @@ dependencies = [ [[package]] name = "graphify-benchmark" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "graphify-security", @@ -1659,7 +1659,7 @@ dependencies = [ [[package]] name = "graphify-build" -version = "0.9.0" +version = "0.9.2" dependencies = [ "caseless", "graphify-security", @@ -1676,7 +1676,7 @@ dependencies = [ [[package]] name = "graphify-cache" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-security", "hex", @@ -1692,7 +1692,7 @@ dependencies = [ [[package]] name = "graphify-cluster" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "indexmap", @@ -1705,7 +1705,7 @@ dependencies = [ [[package]] name = "graphify-dedup" -version = "0.9.0" +version = "0.9.2" dependencies = [ "caseless", "indexmap", @@ -1720,7 +1720,7 @@ dependencies = [ [[package]] name = "graphify-detect" -version = "0.9.0" +version = "0.9.2" dependencies = [ "calamine", "graphify-google", @@ -1746,7 +1746,7 @@ dependencies = [ [[package]] name = "graphify-diagnostics" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "graphify-security", @@ -1760,7 +1760,7 @@ dependencies = [ [[package]] name = "graphify-export" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-build", @@ -1784,7 +1784,7 @@ dependencies = [ [[package]] name = "graphify-extract" -version = "0.9.0" +version = "0.9.2" dependencies = [ "flate2", "glob", @@ -1792,6 +1792,7 @@ dependencies = [ "graphify-cache", "graphify-detect", "graphify-security", + "hex", "indexmap", "native-tls", "postgres", @@ -1801,6 +1802,7 @@ dependencies = [ "regex", "serde", "serde_json", + "sha1 0.11.0", "tempfile", "thiserror 2.0.18", "toml", @@ -1839,7 +1841,7 @@ dependencies = [ [[package]] name = "graphify-global" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-build", @@ -1855,7 +1857,7 @@ dependencies = [ [[package]] name = "graphify-google" -version = "0.9.0" +version = "0.9.2" dependencies = [ "hex", "regex", @@ -1868,7 +1870,7 @@ dependencies = [ [[package]] name = "graphify-hooks" -version = "0.9.0" +version = "0.9.2" dependencies = [ "regex", "serde_json", @@ -1881,7 +1883,7 @@ dependencies = [ [[package]] name = "graphify-html" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-build", @@ -1898,7 +1900,7 @@ dependencies = [ [[package]] name = "graphify-ingest" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-security", @@ -1916,7 +1918,7 @@ dependencies = [ [[package]] name = "graphify-llm" -version = "0.9.0" +version = "0.9.2" dependencies = [ "aws-config", "aws-sdk-bedrockruntime", @@ -1943,14 +1945,14 @@ dependencies = [ [[package]] name = "graphify-manifest" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-detect", ] [[package]] name = "graphify-multigraph-compat" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "indexmap", @@ -1961,7 +1963,7 @@ dependencies = [ [[package]] name = "graphify-prs" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-security", @@ -1974,7 +1976,7 @@ dependencies = [ [[package]] name = "graphify-reflect" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-ingest", @@ -1987,7 +1989,7 @@ dependencies = [ [[package]] name = "graphify-report" -version = "0.9.0" +version = "0.9.2" dependencies = [ "chrono", "graphify-analyze", @@ -2001,7 +2003,7 @@ dependencies = [ [[package]] name = "graphify-scip" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-security", "hex", @@ -2016,7 +2018,7 @@ dependencies = [ [[package]] name = "graphify-security" -version = "0.9.0" +version = "0.9.2" dependencies = [ "ipnet", "mockito", @@ -2031,7 +2033,7 @@ dependencies = [ [[package]] name = "graphify-semantic" -version = "0.9.0" +version = "0.9.2" dependencies = [ "indexmap", "regex", @@ -2043,7 +2045,7 @@ dependencies = [ [[package]] name = "graphify-serve" -version = "0.9.0" +version = "0.9.2" dependencies = [ "axum", "chrono", @@ -2064,7 +2066,7 @@ dependencies = [ [[package]] name = "graphify-transcribe" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-security", "hex", @@ -2077,7 +2079,7 @@ dependencies = [ [[package]] name = "graphify-validate" -version = "0.9.0" +version = "0.9.2" dependencies = [ "serde_json", "thiserror 2.0.18", @@ -2085,7 +2087,7 @@ dependencies = [ [[package]] name = "graphify-watch" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-analyze", "graphify-build", @@ -2107,7 +2109,7 @@ dependencies = [ [[package]] name = "graphify-wiki" -version = "0.9.0" +version = "0.9.2" dependencies = [ "graphify-build", "indexmap", diff --git a/Cargo.toml b/Cargo.toml index e320f53..939ca45 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,7 @@ license = "Apache-2.0" publish = false repository = "https://github.com/bunkerlab-net/graphify" rust-version = "1.95" -version = "0.9.0" +version = "0.9.2" [workspace.dependencies] anyhow = "1" diff --git a/USAGE.md b/USAGE.md index c7b03df..45ec26b 100644 --- a/USAGE.md +++ b/USAGE.md @@ -323,6 +323,9 @@ Save a Q&A result back into `graphify-out/memory/` so it gets re-extracted into Pass `--outcome useful|dead_end|corrected` (and `--correction ""` for the `corrected` case) to record a work-memory signal that `graphify reflect` later aggregates into `LESSONS.md`. An out-of-set `--outcome` is rejected. +The answer can be passed inline with `--answer ""` or read from a file with `--answer-file `; the +latter avoids fragile shell quoting for long or multiline answers (#1502). Exactly one of the two is required. + ```bash graphify save-result \ --question "how is auth scoped" \ @@ -703,6 +706,8 @@ completes the feature.) | `GRAPHIFY_CLUSTER_BACKEND` | `leiden` (default) or `louvain` to force the fallback. | | `GRAPHIFY_ALLOW_LOCAL_PROVIDERS` | Opt in to loading a project-local `.graphify/providers.json` (ignored by default; see Custom providers). | | `OLLAMA_BASE_URL` | Ollama endpoint (default `http://localhost:11434/v1`); a link-local/cloud-metadata host is refused, a general non-loopback host warns. | +| `GRAPHIFY_API_TIMEOUT` | LLM HTTP request timeout in seconds (default 600); bounds a runaway connection during semantic extraction. | +| `GRAPHIFY_MAX_RETRIES` | Times a rate-limited (HTTP 429) LLM request is retried before its chunk is dropped (default 6); `0` disables retries (#1523). | ### LLM backends diff --git a/crates/graphify-cache/src/lib.rs b/crates/graphify-cache/src/lib.rs index 31f8ae2..d38fab4 100644 --- a/crates/graphify-cache/src/lib.rs +++ b/crates/graphify-cache/src/lib.rs @@ -29,7 +29,9 @@ mod store; pub use error::CacheError; pub use hash::{body_content, file_hash}; pub use paths::{EXTRACTOR_VERSION, cache_dir, cache_dir_versioned}; -pub use semantic::{SemanticCacheSplit, check_semantic_cache, save_semantic_cache}; +pub use semantic::{ + SemanticCacheSplit, check_semantic_cache, prune_semantic_cache, save_semantic_cache, +}; pub use stat_index::{ _reset_stat_index_for_tests, ensure_atexit_flush_registered, flush_stat_index, }; diff --git a/crates/graphify-cache/src/semantic.rs b/crates/graphify-cache/src/semantic.rs index 3221f92..661ac5a 100644 --- a/crates/graphify-cache/src/semantic.rs +++ b/crates/graphify-cache/src/semantic.rs @@ -1,5 +1,6 @@ //! Semantic-extraction cache: per-source-file nodes/edges/hyperedges. +use std::collections::HashSet; use std::path::{Path, PathBuf}; use indexmap::IndexMap; @@ -121,3 +122,50 @@ pub fn save_semantic_cache( } Ok(saved) } + +/// Remove orphaned semantic cache entries, returning the count pruned. +/// +/// The semantic cache is content-hash-keyed (`{file_hash}.json` under +/// `cache/semantic/`) and deliberately UNVERSIONED — entries are produced by the +/// LLM from file contents, so invalidating them on every release would re-bill +/// extraction. Because it is unversioned it is never swept by the AST +/// version-cleanup, so every content change or file deletion leaves a permanent +/// orphan that accumulates unbounded (#1527). +/// +/// This sweeps `cache/semantic/*.json` and deletes any entry whose stem (the +/// content hash) is not in `live_hashes` — the hashes of the current live +/// document set. `*.tmp` atomic-write temporaries are skipped, and only this +/// directory is touched (never `cache/ast/**`). Best-effort: each unlink failure +/// is ignored (worst case is a surviving orphan, never wrong output). Mirrors +/// Python `prune_semantic_cache`. +#[must_use] +pub fn prune_semantic_cache( + root: &Path, + live_hashes: &HashSet, +) -> usize { + let semantic_dir = crate::paths::out_base(root).join("cache").join("semantic"); + if !semantic_dir.is_dir() { + return 0; + } + let Ok(entries) = std::fs::read_dir(&semantic_dir) else { + return 0; + }; + let mut pruned = 0; + for entry in entries.flatten() { + let path = entry.path(); + // Only `*.json`; `*.tmp` atomic-write temporaries are left untouched. + if path.extension().and_then(|e| e.to_str()) != Some("json") { + continue; + } + let Some(stem) = path.file_stem().and_then(|s| s.to_str()) else { + continue; + }; + if live_hashes.contains(stem) { + continue; + } + if std::fs::remove_file(&path).is_ok() { + pruned += 1; + } + } + pruned +} diff --git a/crates/graphify-cache/tests/parity.rs b/crates/graphify-cache/tests/parity.rs index fd57c8c..33904df 100644 --- a/crates/graphify-cache/tests/parity.rs +++ b/crates/graphify-cache/tests/parity.rs @@ -1,13 +1,14 @@ //! Parity tests against `graphify-py/tests/test_cache.py`. #![allow(clippy::expect_used)] +use std::collections::HashSet; use std::fs; use std::path::Path; use graphify_cache::{ _reset_stat_index_for_tests, body_content, cache_dir, cache_dir_versioned, cached_files, clear_cache, ensure_atexit_flush_registered, file_hash, load_cached, load_cached_versioned, - save_cached, save_cached_versioned, + prune_semantic_cache, save_cached, save_cached_versioned, }; use serde_json::{Value, json}; use serial_test::serial; @@ -607,3 +608,145 @@ fn dir_has_json(dir: &Path) -> bool { .any(|e| e.path().extension().and_then(|x| x.to_str()) == Some("json")) }) } + +#[test] +#[serial] +fn semantic_prune_removes_orphan_entries() { + // Changing a file's content leaves the old content-hash entry orphaned; + // pruning against the new live hash removes the stale entry, keeps the current. + _reset_stat_index_for_tests(); + let tmp = tempfile::tempdir().expect("tempdir"); + let f = tmp.path().join("doc.md"); + write_text(&f, "# A\n\nContent A.\n"); + let h_a = file_hash(&f, tmp.path()).expect("h_a"); + save_cached( + &f, + &json!({"nodes": [{"id": "a"}], "edges": []}), + tmp.path(), + "semantic", + ) + .expect("save a"); + + write_text(&f, "# B\n\nContent B.\n"); + _reset_stat_index_for_tests(); + bump_mtime(&f); + let h_b = file_hash(&f, tmp.path()).expect("h_b"); + save_cached( + &f, + &json!({"nodes": [{"id": "b"}], "edges": []}), + tmp.path(), + "semantic", + ) + .expect("save b"); + assert_ne!(h_a, h_b, "content change must produce a new hash"); + + let semantic_dir = cache_dir(tmp.path(), "semantic").expect("cache_dir"); + assert!(semantic_dir.join(format!("{h_a}.json")).exists()); + assert!(semantic_dir.join(format!("{h_b}.json")).exists()); + + let pruned = prune_semantic_cache(tmp.path(), &HashSet::from([h_b.clone()])); + assert_eq!(pruned, 1); + assert!(!semantic_dir.join(format!("{h_a}.json")).exists()); + assert!(semantic_dir.join(format!("{h_b}.json")).exists()); +} + +#[test] +#[serial] +fn semantic_prune_keeps_live_unchanged_entries() { + // Pruning against the FULL live set must keep every live entry — guards the + // trap of pruning against an incremental changed-subset. + _reset_stat_index_for_tests(); + let tmp = tempfile::tempdir().expect("tempdir"); + let mut live_hashes: HashSet = HashSet::new(); + for i in 0..5 { + let f = tmp.path().join(format!("doc{i}.md")); + write_text(&f, &format!("# Doc {i}\n\nBody {i}.\n")); + save_cached( + &f, + &json!({"nodes": [{"id": i.to_string()}], "edges": []}), + tmp.path(), + "semantic", + ) + .expect("save"); + live_hashes.insert(file_hash(&f, tmp.path()).expect("hash")); + } + let semantic_dir = cache_dir(tmp.path(), "semantic").expect("cache_dir"); + let count = || { + fs::read_dir(&semantic_dir) + .expect("read_dir") + .filter_map(Result::ok) + .filter(|e| e.path().extension().and_then(|x| x.to_str()) == Some("json")) + .count() + }; + assert_eq!(count(), 5); + assert_eq!(prune_semantic_cache(tmp.path(), &live_hashes), 0); + assert_eq!(count(), 5); +} + +#[test] +#[serial] +fn semantic_prune_handles_deleted_file() { + // An entry for a file that no longer exists (dropped from the live set) is pruned. + _reset_stat_index_for_tests(); + let tmp = tempfile::tempdir().expect("tempdir"); + let f = tmp.path().join("gone.md"); + write_text(&f, "# Gone\n\nWill be deleted.\n"); + let h = file_hash(&f, tmp.path()).expect("hash"); + save_cached( + &f, + &json!({"nodes": [{"id": "g"}], "edges": []}), + tmp.path(), + "semantic", + ) + .expect("save"); + let semantic_dir = cache_dir(tmp.path(), "semantic").expect("cache_dir"); + assert!(semantic_dir.join(format!("{h}.json")).exists()); + + fs::remove_file(&f).expect("unlink"); + let pruned = prune_semantic_cache(tmp.path(), &HashSet::new()); + assert_eq!(pruned, 1); + assert!(!semantic_dir.join(format!("{h}.json")).exists()); +} + +#[test] +#[serial] +fn semantic_prune_ignores_ast_and_tmp() { + // Prune touches only cache/semantic/*.json: AST entries and atomic-write + // *.tmp temporaries are left untouched. + _reset_stat_index_for_tests(); + let tmp = tempfile::tempdir().expect("tempdir"); + let f = tmp.path().join("doc.md"); + write_text(&f, "# Doc\n\nBody.\n"); + // AST entry (different subtree) must survive. + save_cached( + &f, + &json!({"nodes": [{"id": "ast"}], "edges": []}), + tmp.path(), + "ast", + ) + .expect("save ast"); + let ast_dir = cache_dir(tmp.path(), "ast").expect("ast dir"); + let ast_json = |d: &Path| { + fs::read_dir(d) + .expect("read_dir") + .filter_map(Result::ok) + .filter(|e| e.path().extension().and_then(|x| x.to_str()) == Some("json")) + .count() + }; + assert_eq!(ast_json(&ast_dir), 1); + + // A semantic orphan .json (to be pruned) plus a .tmp temporary (to survive). + let semantic_dir = cache_dir(tmp.path(), "semantic").expect("semantic dir"); + write_text( + &semantic_dir.join("deadbeef.json"), + "{\"nodes\": [], \"edges\": []}", + ); + let tmp_entry = semantic_dir.join("deadbeef.tmp"); + write_text(&tmp_entry, "partial"); + + let pruned = prune_semantic_cache(tmp.path(), &HashSet::new()); + assert_eq!(pruned, 1); + assert!(!semantic_dir.join("deadbeef.json").exists()); + assert!(tmp_entry.exists(), "*.tmp temporaries must not be swept"); + assert_eq!(ast_json(&ast_dir), 1, "AST entries must not be touched"); +} diff --git a/crates/graphify-export/tests/parity.rs b/crates/graphify-export/tests/parity.rs index a9a9ae5..9129e88 100644 --- a/crates/graphify-export/tests/parity.rs +++ b/crates/graphify-export/tests/parity.rs @@ -203,6 +203,38 @@ fn test_to_graphml_has_community_attribute() { ); } +#[test] +fn test_to_graphml_tolerates_none_attribute_values() { + // A null attribute value must coerce to "" so a node/edge with a null field + // still exports (no crash). graphify-py needs this because nx.write_graphml + // raises ValueError on None (#1502); the hand-written Rust GraphML already + // renders null as empty, so this pins that contract. + let mut g = make_graph(); + let communities = make_communities(); + // Inject a null-valued attribute on one node... + let (nid, mut nattrs) = { + let (id, attrs) = g.nodes().next().expect("graph has at least one node"); + (id.clone(), attrs.clone()) + }; + nattrs.insert("nullable_field".to_string(), Value::Null); + g.add_node(&nid, nattrs); + // ...and on one edge. + let edge_info = g + .edges() + .next() + .map(|e| (e.source.clone(), e.target.clone(), e.attrs.clone())); + if let Some((src, tgt, mut eattrs)) = edge_info { + eattrs.insert("nullable_field".to_string(), Value::Null); + g.add_edge(&src, &tgt, eattrs); + } + + let tmp = tempdir().expect("tempdir"); + let out = tmp.path().join("graph.graphml"); + to_graphml(&g, &communities, &out).expect("to_graphml must not fail on null attrs"); + let content = std::fs::read_to_string(&out).expect("read graphml"); + assert!(content.contains(" { source_file: self.str_path.to_string(), source_location: format!("L{line}"), receiver: None, + receiver_type: None, }); } diff --git a/crates/graphify-extract/src/extractors/elixir.rs b/crates/graphify-extract/src/extractors/elixir.rs index 421d6a3..664fa57 100644 --- a/crates/graphify-extract/src/extractors/elixir.rs +++ b/crates/graphify-extract/src/extractors/elixir.rs @@ -532,6 +532,7 @@ fn walk_calls_elixir( source_file: str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver: None, + receiver_type: None, }); } } diff --git a/crates/graphify-extract/src/extractors/go/calls.rs b/crates/graphify-extract/src/extractors/go/calls.rs index eb2abb7..734a41f 100644 --- a/crates/graphify-extract/src/extractors/go/calls.rs +++ b/crates/graphify-extract/src/extractors/go/calls.rs @@ -100,6 +100,7 @@ pub(super) fn walk_calls_go( source_file: str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver: None, + receiver_type: None, }); } } diff --git a/crates/graphify-extract/src/extractors/go/refs.rs b/crates/graphify-extract/src/extractors/go/refs.rs index 4edddf7..0555bf3 100644 --- a/crates/graphify-extract/src/extractors/go/refs.rs +++ b/crates/graphify-extract/src/extractors/go/refs.rs @@ -129,9 +129,9 @@ impl GoRefCtx<'_> { /// The stub carries no `source_file` so the corpus-level rewire can collapse /// it onto the real definition; a sourced stub would bake the referencing /// file's path (extension and all) into the id and block the rewire — the - /// phantom-duplicate-node bug (#1500/#1402). Unlike the generic-walker stub, - /// no `origin_file` is recorded: same-package Go refs resolve to the single - /// canonical type node rather than splitting per referencing file. + /// phantom-duplicate-node bug (#1500/#1402); the referencing file is recorded + /// as `origin_file` so same-label cross-file stubs split into distinct ids + /// (#1462/#1515), matching the generic `ensure_named_node`. fn ensure_named_node(&mut self, name: &str) -> String { let nid1 = make_id(&[self.pkg_scope, name]); if self.seen_ids.contains(&nid1) { @@ -146,7 +146,7 @@ impl GoRefCtx<'_> { source_file: String::new(), source_location: Some(String::new()), metadata: None, - origin_file: None, + origin_file: Some(self.str_path.to_string()), }); } nid2 diff --git a/crates/graphify-extract/src/extractors/multi/cache.rs b/crates/graphify-extract/src/extractors/multi/cache.rs index 176eb4f..89acf0a 100644 --- a/crates/graphify-extract/src/extractors/multi/cache.rs +++ b/crates/graphify-extract/src/extractors/multi/cache.rs @@ -32,6 +32,7 @@ fn file_result_to_value(result: &FileResult) -> Value { "source_file": rc.source_file, "source_location": rc.source_location, "receiver": rc.receiver, + "receiver_type": rc.receiver_type, }) }) .collect(); @@ -99,6 +100,13 @@ fn value_to_file_result(v: &Value) -> FileResult { .get("receiver") .and_then(Value::as_str) .map(str::to_string), + // `receiver_type` (#1499) reads back as `None` when + // absent; same version-namespaced invalidation as + // `receiver` above keeps a pre-field entry from surfacing. + receiver_type: rc + .get("receiver_type") + .and_then(Value::as_str) + .map(str::to_string), }) }) .collect() diff --git a/crates/graphify-extract/src/extractors/multi/mod.rs b/crates/graphify-extract/src/extractors/multi/mod.rs index c73cd13..795aeec 100644 --- a/crates/graphify-extract/src/extractors/multi/mod.rs +++ b/crates/graphify-extract/src/extractors/multi/mod.rs @@ -17,6 +17,8 @@ mod csharp; mod java; mod js; mod python; +mod resolvers; +mod ruby; mod swift; use crate::extractors::{ @@ -37,14 +39,11 @@ use cache::extract_single_file; use csharp::resolve_csharp_type_references; use java::{resolve_cross_file_java_imports, resolve_java_type_references}; use js::{resolve_js_default_imports, resolve_js_reexport_imports}; -use python::{ - resolve_cross_file_python_imports, resolve_python_member_calls, resolve_python_reexport_imports, -}; +use python::{resolve_cross_file_python_imports, resolve_python_reexport_imports}; use rayon::prelude::*; use serde_json::Value; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use swift::resolve_swift_member_calls; const PARALLEL_THRESHOLD: usize = 20; @@ -188,6 +187,13 @@ fn relativise_under_root(path: &Path, root: &Path) -> Option { .and_then(|c| c.strip_prefix(root).map(Path::to_path_buf).ok()) } +/// `true` when `id` begins with `prefix` followed by a `_` segment boundary — +/// i.e. `id == "{prefix}_{suffix}"`. IDs are `make_id` output (lowercase word +/// chars + `_`), so the byte index is always on a char boundary. +fn prefix_segment_match(id: &str, prefix: &str) -> bool { + id.len() > prefix.len() && id.starts_with(prefix) && id.as_bytes()[prefix.len()] == b'_' +} + /// Extract AST nodes and edges from a list of code files. /// /// Two-pass process: @@ -329,7 +335,7 @@ pub fn extract(paths: &[PathBuf], cache_root: Option<&Path>) -> ExtractOutput { // way, gated by `source_file` so two files sharing a prefix can't // cross-contaminate. Keyed by the path string the extractor recorded in // `source_file` → (old_prefix, new_prefix). - let mut prefix_remap: HashMap = HashMap::new(); + let mut prefix_remap: HashMap> = HashMap::new(); for path in paths { let old_id = make_id1(&path.to_string_lossy()); // Resolve relative-to-root; a lexical strip can fail (path is relative, or @@ -353,9 +359,26 @@ pub fn extract(paths: &[PathBuf], cache_root: Option<&Path>) -> ExtractOutput { id_remap.entry(canon_id).or_insert_with(|| new_id.clone()); } } + // Each file maps from up to TWO old prefixes: the input-form + // `file_node_id(path)` and the absolute-resolved-form + // `file_node_id(canonicalize(path))`. Alias/workspace imports resolve + // specifiers through the canonical absolute path, so their symbol-edge + // targets are keyed off the ABSOLUTE file stem; with relative inputs the + // two forms differ and absolute-derived targets would otherwise orphan + // (#1529). Stored as a list so the symbol-prefix remap can try both. + let mut old_prefs: Vec<(String, String)> = Vec::new(); let old_pref = crate::ids::file_node_id(path); if old_pref != new_id { - prefix_remap.insert(path.to_string_lossy().into_owned(), (old_pref, new_id)); + old_prefs.push((old_pref.clone(), new_id.clone())); + } + if let Ok(canon) = path.canonicalize() { + let old_pref_abs = crate::ids::file_node_id(&canon); + if old_pref_abs != new_id && old_pref_abs != old_pref { + old_prefs.push((old_pref_abs, new_id.clone())); + } + } + if !old_prefs.is_empty() { + prefix_remap.insert(path.to_string_lossy().into_owned(), old_prefs); } } if !id_remap.is_empty() { @@ -391,18 +414,34 @@ pub fn extract(paths: &[PathBuf], cache_root: Option<&Path>) -> ExtractOutput { { continue; } - let Some((old_pref, new_pref)) = prefix_remap.get(&n.source_file) else { + let Some(entry) = prefix_remap.get(&n.source_file) else { continue; }; - // IDs are make_id output (lowercase word chars + `_`), so slicing at - // a byte offset is always on a char boundary. - if n.id.len() > old_pref.len() - && n.id.starts_with(old_pref.as_str()) - && n.id.as_bytes()[old_pref.len()] == b'_' - { - let new_nid = format!("{new_pref}{}", &n.id[old_pref.len()..]); - if new_nid != n.id { - sym_remap.insert(n.id.clone(), new_nid); + // The node may carry the input-form OR the absolute-form prefix + // (#1529); try each, first match wins (source_file gating above + // prevents cross-file contamination). + let mut matched = false; + for (old_pref, new_pref) in entry { + if prefix_segment_match(&n.id, old_pref) { + let new_nid = format!("{new_pref}{}", &n.id[old_pref.len()..]); + if new_nid != n.id { + sym_remap.insert(n.id.clone(), new_nid); + } + matched = true; + break; + } + } + // When the node is already canonical, also map its absolute-form id + // variant → the canonical id, so an alias/workspace import edge that + // targeted the absolute prefix (its file resolved via the canonical + // path) reconnects instead of dangling (#1529). The absolute prefix + // is a full-path stem, so it cannot collide with a canonical id. + if !matched { + for (old_pref, new_pref) in entry { + if old_pref != new_pref && prefix_segment_match(&n.id, new_pref) { + let abs_id = format!("{old_pref}{}", &n.id[new_pref.len()..]); + sym_remap.entry(abs_id).or_insert_with(|| n.id.clone()); + } } } } @@ -623,31 +662,28 @@ pub fn extract(paths: &[PathBuf], cache_root: Option<&Path>) -> ExtractOutput { }); } - // Cross-file Swift member-call resolution (#1356): after the shared call pass - // (node ids and caller_nids final) and before source_file relativisation (the - // type-table re-parse keys on the absolute paths nodes/raw_calls still carry). - let swift_paths: Vec = paths - .iter() - .filter(|p| p.extension().is_some_and(|e| e == "swift")) - .cloned() - .collect(); - if !swift_paths.is_empty() { - resolve_swift_member_calls(&swift_paths, &all_nodes, &mut all_edges, &all_raw_calls); - } - - // Cross-file Python qualified class-method calls (#1446): same timing as the - // Swift pass — after the shared call pass (ids final), before relativisation. - let has_python = paths - .iter() - .any(|p| p.extension().is_some_and(|e| e == "py" || e == "pyi")); - if has_python { - resolve_python_member_calls(&all_nodes, &mut all_edges, &all_raw_calls); - } + // Cross-file, language-specific member-call resolution (#1356 Swift, #1446 + // Python, #1499 Ruby). Runs after the shared call pass — node ids and + // caller_nids are final — and before source_file relativisation (Swift's + // type-table re-parse keys on the absolute paths nodes/raw_calls still + // carry). Each pass is suffix-gated and additive; a new language registers + // one resolver instead of editing this body. + let resolver_set = resolvers::default_resolvers(); + resolvers::run_language_resolvers( + paths, + &all_nodes, + &mut all_edges, + &all_raw_calls, + &resolver_set, + ); - // Relativise source_file (and the #1462 origin_file) so persisted paths are - // portable across machines (#555). graphify-py relativizes only source_file - // (extract.py), leaking absolute origin_file paths into graph JSON — fix that - // determinism gap here too. + // Relativise source_file so persisted paths are portable across machines + // (#555), then drop the internal origin_file hint entirely. The colliding-id + // pass above already consumed it (#1462); keeping it would ship an absolute, + // machine-specific path into graph.json — the same "no absolute paths in + // output" contract that relativises source_file (#1516, #932). The per-file + // AST cache keeps its own copy, which the colliding-id pass reads on a cache + // hit. for n in &mut all_nodes { let sf_path = PathBuf::from(&n.source_file); if sf_path.is_absolute() @@ -655,12 +691,7 @@ pub fn extract(paths: &[PathBuf], cache_root: Option<&Path>) -> ExtractOutput { { n.source_file = rel.to_string_lossy().into_owned(); } - if let Some(of_path) = n.origin_file.as_deref().map(PathBuf::from) - && of_path.is_absolute() - && let Some(rel) = relativise_under_root(&of_path, &root) - { - n.origin_file = Some(rel.to_string_lossy().into_owned()); - } + n.origin_file = None; } for e in &mut all_edges { let sf_path = PathBuf::from(&e.source_file); diff --git a/crates/graphify-extract/src/extractors/multi/resolvers.rs b/crates/graphify-extract/src/extractors/multi/resolvers.rs new file mode 100644 index 0000000..10ca76b --- /dev/null +++ b/crates/graphify-extract/src/extractors/multi/resolvers.rs @@ -0,0 +1,99 @@ +//! Cross-file, language-specific member-call resolver dispatch (#1499). +//! +//! Formalizes the previously hand-wired sequence of suffix-gated resolution +//! passes (`if !swift_paths.is_empty() { resolve_swift(...) }` …) so a new +//! language plugs in by adding one [`LanguageResolver`] to [`default_resolvers`] +//! instead of editing `extract`'s body. Mirrors the observable contract of +//! graphify-py's `resolver_registry`: suffix gating and ordered execution. +//! +//! Divergence from graphify-py: the Python registry wraps each pass in +//! `try/except` and logs-and-continues on failure. The Rust resolvers are +//! infallible by construction — they guard internally (god-node checks, +//! single-definition requirements) and return `()` — so there is no panic to +//! isolate, and no `catch_unwind` is used (a panic here is a bug, not a +//! recoverable per-language failure). + +use std::collections::HashSet; +use std::path::PathBuf; + +use crate::types::{Edge, Node, RawCall}; + +/// A resolver pass: reads the corpus `nodes` / `raw_calls` and the input +/// `paths`, mutating `edges` in place. `paths` lets a pass that re-parses source +/// (Swift) find its files; name-only passes (Python, Ruby) ignore it. +type ResolveFn = fn(&[PathBuf], &[Node], &mut Vec, &[RawCall]); + +/// One cross-file, language-specific resolution pass. `suffixes` (dotted, e.g. +/// `.rb`) gates activation: the pass runs only when the corpus contains at least +/// one file with one of these extensions. Mirrors graphify-py +/// `resolver_registry.LanguageResolver`. +pub(super) struct LanguageResolver { + /// Dotted file suffixes that activate the pass (e.g. `.rb`). + pub suffixes: &'static [&'static str], + /// The pass itself. + pub resolve: ResolveFn, +} + +/// Run every resolver whose suffix appears in `paths`, in registration order. +/// Behaviorally identical to the prior hand-wired sequence of suffix-gated +/// passes: same activation rule (suffix present) and execution order. Mirrors +/// graphify-py `run_language_resolvers`. +pub(super) fn run_language_resolvers( + paths: &[PathBuf], + nodes: &[Node], + edges: &mut Vec, + raw_calls: &[RawCall], + resolvers: &[LanguageResolver], +) { + let present: HashSet = paths + .iter() + .filter_map(|p| { + p.extension() + .and_then(|e| e.to_str()) + .map(|e| format!(".{e}")) + }) + .collect(); + for resolver in resolvers { + if resolver.suffixes.iter().any(|s| present.contains(*s)) { + (resolver.resolve)(paths, nodes, edges, raw_calls); + } + } +} + +/// The default ordered resolver set: Swift (#1356), then Python (#1446), then +/// Ruby (#1499). Order preserves the prior inlined wiring. +pub(super) fn default_resolvers() -> [LanguageResolver; 3] { + [ + LanguageResolver { + suffixes: &[".swift"], + resolve: swift_pass, + }, + LanguageResolver { + suffixes: &[".py", ".pyi"], + resolve: python_pass, + }, + LanguageResolver { + suffixes: &[".rb"], + resolve: ruby_pass, + }, + ] +} + +fn swift_pass(paths: &[PathBuf], nodes: &[Node], edges: &mut Vec, raw_calls: &[RawCall]) { + // The Swift resolver re-parses each `.swift` file for its local type table, + // so it needs the swift-only path subset (not the whole corpus). + let swift_paths: Vec = paths + .iter() + .filter(|p| p.extension().is_some_and(|e| e == "swift")) + .cloned() + .collect(); + super::swift::resolve_swift_member_calls(&swift_paths, nodes, edges, raw_calls); +} + +fn python_pass(_paths: &[PathBuf], nodes: &[Node], edges: &mut Vec, raw_calls: &[RawCall]) { + super::python::resolve_python_member_calls(nodes, edges, raw_calls); +} + +fn ruby_pass(_paths: &[PathBuf], nodes: &[Node], edges: &mut Vec, raw_calls: &[RawCall]) { + super::ruby::resolve_ruby_member_calls(nodes, edges, raw_calls); +} diff --git a/crates/graphify-extract/src/extractors/multi/ruby.rs b/crates/graphify-extract/src/extractors/multi/ruby.rs new file mode 100644 index 0000000..7173320 --- /dev/null +++ b/crates/graphify-extract/src/extractors/multi/ruby.rs @@ -0,0 +1,158 @@ +//! Type-aware cross-file resolution for Ruby member calls (#1499). +//! +//! Ruby has no type annotations and reuses method names heavily, so resolving +//! `obj.method()` by globally-unique name is both lossy (drops on collision) and +//! unsafe (can attach to the wrong same-named method). This pass instead uses the +//! receiver's *type*, inferred at extraction time from local `var = ClassName.new` +//! bindings and carried on each member-call `RawCall` as `receiver_type`. +//! +//! It resolves two shapes, both at EXTRACTED (1.0) confidence and only when the +//! target is certain (single owning class, single owned method) — bail otherwise: +//! +//! * `Processor.new` -> a `calls` edge to the `Processor` class +//! * `p.run` where `p` is a `Processor` -> a `calls` edge to `Processor#run` +//! +//! Runs after id-disambiguation, so node ids and raw-call caller ids are final. + +use std::collections::{HashMap, HashSet}; + +use crate::types::{Edge, Node, RawCall}; + +/// Normalise a class/method label to a comparison key (drop punctuation, fold). +/// Mirrors Python `_key`. +fn key(label: &str) -> String { + label + .chars() + .filter(char::is_ascii_alphanumeric) + .collect::() + .to_lowercase() +} + +/// The single class node id owning `name`, or `None` when absent or ambiguous +/// (god-node guard). Mirrors Python `_unique_class`. +fn unique_class<'a>( + class_def_nids: &'a HashMap>, + name: &str, +) -> Option<&'a str> { + match class_def_nids.get(&key(name)) { + Some(nids) if nids.len() == 1 => nids.first().map(String::as_str), + _ => None, + } +} + +/// Append a `calls` edge unless it is a self-loop or already present. Mirrors +/// Python `_emit`. +fn push_edge( + caller: &str, + target: &str, + rc: &RawCall, + existing: &mut HashSet<(String, String)>, + out: &mut Vec, +) { + if caller.is_empty() || target.is_empty() || caller == target { + return; + } + if !existing.insert((caller.to_string(), target.to_string())) { + return; + } + out.push(Edge { + external: false, + source: caller.to_string(), + target: target.to_string(), + relation: "calls".to_string(), + confidence: "EXTRACTED".to_string(), + source_file: rc.source_file.clone(), + source_location: Some(rc.source_location.clone()), + weight: 1.0, + context: Some("call".to_string()), + confidence_score: Some(1.0), + }); +} + +/// Resolve Ruby `Class.new` and typed `var.method` calls by receiver type. +/// +/// Purely additive: only emits edges the shared (name-based) call pass skips +/// because they are member calls. Each emission requires a single owning class +/// (god-node guard) so an ambiguous class name resolves to nothing rather than a +/// wrong edge. Mirrors Python `resolve_ruby_member_calls`. +pub(super) fn resolve_ruby_member_calls( + all_nodes: &[Node], + all_edges: &mut Vec, + all_raw_calls: &[RawCall], +) { + let node_by_id: HashMap<&str, &Node> = all_nodes.iter().map(|n| (n.id.as_str(), n)).collect(); + + // class label key -> [class node ids]; (class_node_id, method_key) -> method id. + let mut class_def_nids: HashMap> = HashMap::new(); + let mut method_index: HashMap<(String, String), String> = HashMap::new(); + for e in all_edges.iter() { + if e.relation != "method" { + continue; + } + if let Some(cnode) = node_by_id.get(e.source.as_str()) { + class_def_nids + .entry(key(&cnode.label)) + .or_default() + .push(e.source.clone()); + } + if let Some(tnode) = node_by_id.get(e.target.as_str()) { + method_index.insert((e.source.clone(), key(&tnode.label)), e.target.clone()); + } + } + for nids in class_def_nids.values_mut() { + nids.sort(); + nids.dedup(); + } + + let mut existing_pairs: HashSet<(String, String)> = all_edges + .iter() + .map(|e| (e.source.clone(), e.target.clone())) + .collect(); + + let mut new_edges: Vec = Vec::new(); + for rc in all_raw_calls { + // Scope to Ruby raw_calls (mirrors Python `_ruby_raw_calls`); only member + // calls are resolved here (the shared pass already handled the rest). + if !rc.source_file.ends_with(".rb") || !rc.is_member_call { + continue; + } + if rc.caller_nid.is_empty() || rc.callee.is_empty() { + continue; + } + + // `Processor.new` -> instantiation edge to the class. + if rc.callee == "new" + && let Some(receiver) = rc.receiver.as_deref() + && receiver.chars().next().is_some_and(char::is_uppercase) + { + if let Some(class_nid) = unique_class(&class_def_nids, receiver) { + push_edge( + &rc.caller_nid, + class_nid, + rc, + &mut existing_pairs, + &mut new_edges, + ); + } + continue; + } + + // `p.run` where p's type is known -> edge to that class's method. + let Some(receiver_type) = rc.receiver_type.as_deref() else { + continue; + }; + let Some(class_nid) = unique_class(&class_def_nids, receiver_type) else { + continue; + }; + if let Some(method_nid) = method_index.get(&(class_nid.to_string(), key(&rc.callee))) { + push_edge( + &rc.caller_nid, + method_nid, + rc, + &mut existing_pairs, + &mut new_edges, + ); + } + } + all_edges.extend(new_edges); +} diff --git a/crates/graphify-extract/src/extractors/multi/swift.rs b/crates/graphify-extract/src/extractors/multi/swift.rs index c0c5019..2e0343f 100644 --- a/crates/graphify-extract/src/extractors/multi/swift.rs +++ b/crates/graphify-extract/src/extractors/multi/swift.rs @@ -82,8 +82,9 @@ fn collect_swift_type_table( /// `is_member_call` (a bare method name collides across the corpus); this pass /// types the receiver via the file's local type table (or treats an upper-cased /// receiver as a type itself), then emits an edge ONLY when the type name -/// resolves to exactly one definition (god-node guard). Everything it adds is -/// INFERRED (type inference, not an explicit import). +/// resolves to exactly one definition (god-node guard). A type-qualified call +/// (`Type.staticMethod()`) is EXTRACTED — the type is named explicitly in source; +/// an instance call typed via local inference (`obj.method()`) is INFERRED (#1533). #[allow(clippy::too_many_lines)] // linear: re-parse type tables, build indexes, resolve each member call pub(super) fn resolve_swift_member_calls( swift_paths: &[PathBuf], @@ -173,7 +174,8 @@ pub(super) fn resolve_swift_member_calls( // An upper-cased receiver is itself a type (`Type.staticMethod()`, // `Singleton.shared.x()`); otherwise look it up in the declaring file's // local type table. - let type_name = if receiver.chars().next().is_some_and(char::is_uppercase) { + let type_qualified = receiver.chars().next().is_some_and(char::is_uppercase); + let type_name = if type_qualified { receiver.to_string() } else if let Some(t) = type_table_by_file .get(&rc.source_file) @@ -203,12 +205,17 @@ pub(super) fn resolve_swift_member_calls( source: rc.caller_nid.clone(), target, relation: relation.to_string(), - confidence: "INFERRED".to_string(), + confidence: if type_qualified { + "EXTRACTED" + } else { + "INFERRED" + } + .to_string(), source_file: rc.source_file.clone(), source_location: Some(rc.source_location.clone()), weight: 1.0, context: Some("call".to_string()), - confidence_score: Some(0.8), + confidence_score: Some(if type_qualified { 1.0 } else { 0.8 }), }); } all_edges.extend(new_edges); diff --git a/crates/graphify-extract/src/extractors/objc.rs b/crates/graphify-extract/src/extractors/objc.rs index ee12a6a..d0c452b 100644 --- a/crates/graphify-extract/src/extractors/objc.rs +++ b/crates/graphify-extract/src/extractors/objc.rs @@ -11,6 +11,24 @@ fn read_text<'a>(node: tree_sitter::Node<'_>, source: &'a [u8]) -> &'a str { std::str::from_utf8(&source[node.start_byte()..node.end_byte()]).unwrap_or("") } +/// Overwrite every occurrence of `needle` in `haystack` with equal-length +/// spaces, preserving byte offsets and line numbers so tree-sitter spans stay +/// valid. Mirrors Python's `source.replace(macro, b" " * len(macro))` (#1475). +fn blank_bytes(haystack: &mut [u8], needle: &[u8]) { + if needle.is_empty() || needle.len() > haystack.len() { + return; + } + let mut i = 0; + while i + needle.len() <= haystack.len() { + if &haystack[i..i + needle.len()] == needle { + haystack[i..i + needle.len()].fill(b' '); + i += needle.len(); + } else { + i += 1; + } + } +} + /// Collect every `type_identifier` name under a property's type node, descending /// through `generic_specifier`/`type_name` so `NSArray` yields both /// `NSArray` and the element type `Product` (#1475). @@ -36,8 +54,9 @@ fn collect_objc_type_names<'a>( /// Extract interfaces, implementations, protocols, methods, and imports from `.m`/`.mm`/`.h` files. #[must_use] +#[allow(clippy::too_many_lines)] // linear orchestration: read+blank source, parse, structural walk, call walk, reconcile pub fn extract_objc(path: &Path) -> FileResult { - let source = match std::fs::read(path) { + let mut source = match std::fs::read(path) { Ok(b) => b, Err(e) => { return FileResult { @@ -49,6 +68,18 @@ pub fn extract_objc(path: &Path) -> FileResult { } }; + // tree-sitter-objc cannot expand these argument-less annotation macros (no + // trailing `;`), and their presence before `@interface` makes the parser + // fail to emit a class_interface node, swallowing the whole interface + // (#1475). Blank them to equal-length spaces so byte offsets and line + // numbers are preserved and the interface parses. + for macro_bytes in [ + b"NS_ASSUME_NONNULL_BEGIN".as_slice(), + b"NS_ASSUME_NONNULL_END".as_slice(), + ] { + blank_bytes(&mut source, macro_bytes); + } + let mut parser = tree_sitter::Parser::new(); if parser .set_language(&tree_sitter_objc::LANGUAGE.into()) @@ -116,8 +147,11 @@ pub fn extract_objc(path: &Path) -> FileResult { { let mut call_ctx = ObjcCallCtx { str_path: &str_path, + stem: &stem, all_method_nids: &all_method_nids, + nodes: &mut nodes, edges: &mut edges, + seen_ids: &mut seen_ids, seen_calls: &mut seen_calls, }; for (caller_nid, body_start, body_end) in &method_bodies { @@ -156,6 +190,37 @@ struct ObjcWalkCtx<'a> { method_bodies: &'a mut Vec<(String, usize, usize)>, } +/// Return the NID for a named type, creating a SOURCELESS placeholder stub when +/// no file-qualified node exists. The stub carries no `source_file` so a real +/// cross-file definition can be rewired onto it (#1402); `origin_file` records +/// the referencing file to disambiguate same-label stubs (#1462). Mirrors Python +/// objc `ensure_named_node`. +fn ensure_objc_named_node( + stem: &str, + str_path: &str, + name: &str, + nodes: &mut Vec, + seen_ids: &mut HashSet, +) -> String { + let nid1 = make_id(&[stem, name]); + if seen_ids.contains(&nid1) { + return nid1; + } + let nid2 = make_id1(name); + if seen_ids.insert(nid2.clone()) { + nodes.push(Node { + id: nid2.clone(), + label: name.to_string(), + file_type: "code".to_string(), + source_file: String::new(), + source_location: Some(String::new()), + metadata: None, + origin_file: Some(str_path.to_string()), + }); + } + nid2 +} + impl ObjcWalkCtx<'_> { /// Return the NID for a named type, creating a SOURCELESS placeholder stub /// when no file-qualified node exists. Mirrors Python objc `ensure_named_node` @@ -164,23 +229,7 @@ impl ObjcWalkCtx<'_> { /// the referencing file is recorded as `origin_file` to disambiguate /// same-label stubs (#1462), matching the generic `ensure_named_node`. fn ensure_named_node(&mut self, name: &str) -> String { - let nid1 = make_id(&[self.stem, name]); - if self.seen_ids.contains(&nid1) { - return nid1; - } - let nid2 = make_id1(name); - if self.seen_ids.insert(nid2.clone()) { - self.nodes.push(Node { - id: nid2.clone(), - label: name.to_string(), - file_type: "code".to_string(), - source_file: String::new(), - source_location: Some(String::new()), - metadata: None, - origin_file: Some(self.str_path.to_string()), - }); - } - nid2 + ensure_objc_named_node(self.stem, self.str_path, name, self.nodes, self.seen_ids) } } @@ -224,10 +273,25 @@ fn walk_objc( loop { if sc.node().kind() == "string_content" { let raw = read_text(sc.node(), source); - let module = - raw.split('/').next_back().unwrap_or("").replace(".h", ""); - if !module.is_empty() { - let tgt_nid = make_id1(&module); + // Resolve the quoted include to a real file so the + // target id matches the (possibly disambiguated) + // node id _make_id gives that file; the bare-stem id + // never survives id-collision splitting when a + // `.h`/`.m` pair exists, so the edge dangled (#1475). + let tgt_nid = crate::import_handlers::resolve_c_include_path( + raw, + ctx.str_path, + ) + .map(|resolved| make_id1(&resolved.to_string_lossy())) + .or_else(|| { + let module = raw + .split('/') + .next_back() + .unwrap_or("") + .replace(".h", ""); + (!module.is_empty()).then(|| make_id1(&module)) + }); + if let Some(tgt_nid) = tgt_nid { ctx.edges.push(Edge { external: false, source: ctx.file_nid.to_string(), @@ -655,8 +719,11 @@ fn walk_objc( /// Shared state threaded through every [`walk_calls_objc`] recursion. struct ObjcCallCtx<'a> { str_path: &'a str, + stem: &'a str, all_method_nids: &'a HashSet, + nodes: &'a mut Vec, edges: &'a mut Vec, + seen_ids: &'a mut HashSet, seen_calls: &'a mut HashSet<(String, String)>, } @@ -669,13 +736,48 @@ fn walk_calls_objc( body_end: usize, ) { let str_path = ctx.str_path; + let stem = ctx.stem; let all_method_nids = ctx.all_method_nids; + let nodes = &mut *ctx.nodes; let edges = &mut *ctx.edges; + let seen_ids = &mut *ctx.seen_ids; let seen_calls = &mut *ctx.seen_calls; if node.start_byte() >= body_end || node.end_byte() <= body_start { return; } if node.kind() == "message_expression" { + // `[[Foo alloc] init]` — the inner `[Foo alloc]` is a message_expression + // whose method is the identifier `alloc` and whose receiver is the bare + // class identifier `Foo`; resolve that class and emit a `references` edge + // so the allocating method links to the allocated type. ensure_named_node + // emits a sourceless stub for an unknown name, which the corpus rewire + // collapses only when exactly one real class of that name exists, so an + // unknown/ambiguous class produces no false resolved edge (#1475). + if let (Some(meth), Some(recv)) = ( + node.child_by_field_name("method"), + node.child_by_field_name("receiver"), + ) && meth.kind() == "identifier" + && read_text(meth, source) == "alloc" + && recv.kind() == "identifier" + { + let tname = read_text(recv, source); + let ref_line = node.start_position().row + 1; + let type_nid = ensure_objc_named_node(stem, str_path, tname, nodes, seen_ids); + if type_nid != caller_nid { + edges.push(Edge { + external: false, + source: caller_nid.to_string(), + target: type_nid, + relation: "references".to_string(), + confidence: "EXTRACTED".to_string(), + source_file: str_path.to_string(), + source_location: Some(format!("L{ref_line}")), + weight: 1.0, + context: Some("type".to_string()), + confidence_score: None, + }); + } + } // `[receiver sel]` and `[receiver kw1:a kw2:b]` both parse to a // message_expression whose selector parts carry the field name "method" // (one for a simple selector, several for a compound one); the receiver diff --git a/crates/graphify-extract/src/extractors/powershell/mod.rs b/crates/graphify-extract/src/extractors/powershell/mod.rs index b1d50a4..d487323 100644 --- a/crates/graphify-extract/src/extractors/powershell/mod.rs +++ b/crates/graphify-extract/src/extractors/powershell/mod.rs @@ -786,6 +786,7 @@ fn walk_calls_ps( source_file: str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver: None, + receiver_type: None, }); } } diff --git a/crates/graphify-extract/src/extractors/rust_lang/calls.rs b/crates/graphify-extract/src/extractors/rust_lang/calls.rs index 70d8be6..a4bcef9 100644 --- a/crates/graphify-extract/src/extractors/rust_lang/calls.rs +++ b/crates/graphify-extract/src/extractors/rust_lang/calls.rs @@ -90,6 +90,7 @@ pub(super) fn walk_calls_rust( source_file: ctx.str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver: None, + receiver_type: None, }); } } diff --git a/crates/graphify-extract/src/extractors/svelte.rs b/crates/graphify-extract/src/extractors/svelte.rs index ccb9142..1f385ea 100644 --- a/crates/graphify-extract/src/extractors/svelte.rs +++ b/crates/graphify-extract/src/extractors/svelte.rs @@ -15,7 +15,7 @@ use regex::Regex; use crate::generic::{extract_generic, extract_generic_with_source}; use crate::ids::make_id1; use crate::lang_configs; -use crate::tsconfig::{load_tsconfig_aliases, resolve_js_module_path}; +use crate::tsconfig::{load_tsconfig_aliases, resolve_js_module_path, resolve_tsconfig_alias}; use crate::types::{Edge, FileResult, Node}; // ── Regex patterns ──────────────────────────────────────────────────────────── @@ -72,28 +72,7 @@ fn resolve_import_id(raw: &str, path: &Path) -> (String, String) { } else { // Check tsconfig aliases let aliases = load_tsconfig_aliases(path.parent().unwrap_or(path)); - let mut resolved_alias: Option = None; - for (alias_prefix, alias_base) in &aliases { - if raw == alias_prefix || raw.starts_with(&format!("{alias_prefix}/")) { - let rest = raw[alias_prefix.len()..].trim_start_matches('/'); - let joined = std::path::Path::new(alias_base).join(rest); - let normalised = - joined - .components() - .fold(std::path::PathBuf::new(), |mut acc, c| { - match c { - std::path::Component::ParentDir => { - acc.pop(); - } - std::path::Component::CurDir => {} - other => acc.push(other), - } - acc - }); - resolved_alias = Some(resolve_js_module_path(&normalised)); - break; - } - } + let resolved_alias = resolve_tsconfig_alias(raw, &aliases); if let Some(alias_path) = resolved_alias { let stub = alias_path.to_string_lossy().into_owned(); (make_id1(&stub), stub) @@ -139,15 +118,7 @@ fn fixup_static_relative(raw: &str, path: &Path) -> (String, String) { (make_id1(&stub), stub) } else { let aliases = load_tsconfig_aliases(path.parent().unwrap_or(path)); - let mut resolved_alias: Option = None; - for (alias_prefix, alias_base) in &aliases { - if raw == alias_prefix || raw.starts_with(&format!("{alias_prefix}/")) { - let rest = raw[alias_prefix.len()..].trim_start_matches('/'); - let joined = std::path::Path::new(alias_base).join(rest); - resolved_alias = Some(joined); - break; - } - } + let resolved_alias = resolve_tsconfig_alias(raw, &aliases); if let Some(alias_path) = resolved_alias { // Route the aliased path through `resolve_js_module_path` so the // same `.js` → `.ts` / `.jsx` → `.tsx` fallback used for diff --git a/crates/graphify-extract/src/extractors/zig.rs b/crates/graphify-extract/src/extractors/zig.rs index 25b6b01..3659c07 100644 --- a/crates/graphify-extract/src/extractors/zig.rs +++ b/crates/graphify-extract/src/extractors/zig.rs @@ -486,6 +486,7 @@ fn walk_calls_zig( source_file: str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver: None, + receiver_type: None, }); } } diff --git a/crates/graphify-extract/src/generic/calls.rs b/crates/graphify-extract/src/generic/calls.rs index cb2b1d1..8b420ec 100644 --- a/crates/graphify-extract/src/generic/calls.rs +++ b/crates/graphify-extract/src/generic/calls.rs @@ -34,6 +34,10 @@ pub(super) struct CallWalkCtx<'a> { pub edges: &'a mut Vec, pub raw_calls: &'a mut Vec, pub seen_ref_pairs: &'a mut HashSet<(String, String, String)>, + /// Per-method `var -> ClassName` table from `var = Const.new` bindings, used + /// to attach `receiver_type` to Ruby member-call `raw_calls` so the cross-file + /// pass resolves `var.method` by type (#1499). Empty for non-Ruby files. + pub ruby_var_types: &'a HashMap>>, } /// Stops descending into nested function definitions (`function_boundary_types`) @@ -123,6 +127,18 @@ pub(super) fn walk_calls( } } } else if !crate::builtins::is_language_builtin_global(&callee) { + // Ruby: attach the receiver's inferred type from the method's + // local `var = Const.new` bindings, when unambiguously known (#1499). + let receiver_type = if ctx.config.lang_id == LangId::Ruby { + receiver.as_deref().and_then(|r| { + ctx.ruby_var_types + .get(caller_nid) + .and_then(|m| m.get(r).cloned()) + .flatten() + }) + } else { + None + }; ctx.raw_calls.push(RawCall { caller_nid: caller_nid.to_string(), callee: callee.clone(), @@ -130,6 +146,7 @@ pub(super) fn walk_calls( source_file: ctx.str_path.to_string(), source_location: format!("L{}", node.start_position().row + 1), receiver, + receiver_type, }); } @@ -371,6 +388,22 @@ fn extract_callee( } } } + LangId::Ruby => { + // Ruby's `call` node carries `receiver` and `method` as direct fields + // (no intermediate accessor node), so the generic accessor model + // doesn't apply. Read them directly and capture a simple receiver (`p` + // in `p.run`, `Processor` in `Processor.new`) so the cross-file pass + // can resolve member calls by the receiver's type (#1499). + if let Some(meth) = node.child_by_field_name("method") { + callee_name = Some(read_text_owned(meth, source)); + } + if let Some(recv) = node.child_by_field_name("receiver") { + is_member_call = true; + if matches!(recv.kind(), "identifier" | "constant") { + receiver = Some(read_text_owned(recv, source)); + } + } + } _ => { // Generic: use call_function_field if !config.call_function_field.is_empty() diff --git a/crates/graphify-extract/src/generic/js_extra.rs b/crates/graphify-extract/src/generic/js_extra.rs index 3ffaff4..6b210b2 100644 --- a/crates/graphify-extract/src/generic/js_extra.rs +++ b/crates/graphify-extract/src/generic/js_extra.rs @@ -459,14 +459,8 @@ pub fn resolve_js_import_target(raw: &str, str_path: &str) -> (String, Option = Vec::new(); let mut seen_ref_pairs: HashSet<(String, String, String)> = HashSet::new(); + // Ruby: per-method `var -> ClassName` table from `var = Const.new` bindings, + // populated before walk_calls so member-call raw_calls carry a `receiver_type` + // for type-based cross-file resolution (#1499). Empty for non-Ruby files. + let ruby_var_types: HashMap>> = + if config.lang_id == LangId::Ruby { + function_bodies + .iter() + .map(|(nid, body)| (nid.clone(), ruby::ruby_local_class_bindings(*body, source))) + .collect() + } else { + HashMap::new() + }; + { let mut call_ctx = super::generic::calls::CallWalkCtx { config, @@ -168,6 +182,7 @@ pub(crate) fn extract_generic_with_source( edges: &mut edges, raw_calls: &mut raw_calls, seen_ref_pairs: &mut seen_ref_pairs, + ruby_var_types: &ruby_var_types, }; for (caller_nid, body_node) in &function_bodies { walk_calls(&mut call_ctx, *body_node, caller_nid, source); diff --git a/crates/graphify-extract/src/generic/references/java.rs b/crates/graphify-extract/src/generic/references/java.rs index 226288a..dbc8c13 100644 --- a/crates/graphify-extract/src/generic/references/java.rs +++ b/crates/graphify-extract/src/generic/references/java.rs @@ -1,16 +1,81 @@ //! Java type-reference and annotation collectors. +use std::collections::HashSet; + use tree_sitter::Node; use super::{RefRole, role_of}; use crate::generic::names::read_text_owned; -#[allow(clippy::too_many_lines)] // single recursive dispatch over tree-sitter Java type kinds +/// Declaration kinds that can introduce Java type parameters (``). +const JAVA_TYPE_PARAMETER_SCOPE_DECLARATIONS: [&str; 5] = [ + "class_declaration", + "interface_declaration", + "record_declaration", + "method_declaration", + "constructor_declaration", +]; + +/// Type-parameter names visible from `node` — the `` / `` declared on any +/// enclosing class/interface/record/method/constructor. A reference to one of +/// these names is a type variable, not a real type, so it must emit neither a +/// `references` edge nor a sourceless stub node (#1518). +fn java_type_parameters_in_scope(node: Node<'_>, source: &[u8]) -> HashSet { + let mut names = HashSet::new(); + let mut scope = Some(node); + while let Some(s) = scope { + if JAVA_TYPE_PARAMETER_SCOPE_DECLARATIONS.contains(&s.kind()) + && let Some(params) = s.child_by_field_name("type_parameters") + { + let mut cur = params.walk(); + if cur.goto_first_child() { + loop { + let param = cur.node(); + if param.kind() == "type_parameter" { + let mut pcur = param.walk(); + if pcur.goto_first_child() { + loop { + if pcur.node().kind() == "type_identifier" { + names.insert(read_text_owned(pcur.node(), source)); + break; + } + if !pcur.goto_next_sibling() { + break; + } + } + } + } + if !cur.goto_next_sibling() { + break; + } + } + } + } + scope = s.parent(); + } + names +} + +/// Walk a Java type expression, appending `(name, role)` references. Type- +/// parameter names in scope are skipped (#1518); the scope is computed once +/// from `node` and threaded through the recursion. pub(crate) fn java_collect_type_refs( node: Node<'_>, source: &[u8], generic: bool, out: &mut Vec<(String, RefRole)>, +) { + let skip = java_type_parameters_in_scope(node, source); + java_collect_type_refs_inner(node, source, generic, out, &skip); +} + +#[allow(clippy::too_many_lines)] // single recursive dispatch over tree-sitter Java type kinds +fn java_collect_type_refs_inner( + node: Node<'_>, + source: &[u8], + generic: bool, + out: &mut Vec<(String, RefRole)>, + skip: &HashSet, ) { let t = node.kind(); if matches!( @@ -21,7 +86,7 @@ pub(crate) fn java_collect_type_refs( } if t == "type_identifier" { let name = read_text_owned(node, source); - if !name.is_empty() { + if !name.is_empty() && !skip.contains(&name) { let role = role_of(generic); out.push((name, role)); } @@ -44,7 +109,12 @@ pub(crate) fn java_collect_type_refs( if matches!(child.kind(), "type_identifier" | "scoped_type_identifier") { let text = read_text_owned(child, source); let tail = text.rsplit('.').next().unwrap_or(&text); - if !tail.is_empty() { + // A bare `type_identifier` that names a type parameter is + // skipped; a `scoped_type_identifier` (e.g. `a.b.C`) is never a + // type parameter and is always kept (#1518). + if !tail.is_empty() + && (child.kind() == "scoped_type_identifier" || !skip.contains(tail)) + { let role = role_of(generic); out.push((tail.to_string(), role)); } @@ -64,7 +134,7 @@ pub(crate) fn java_collect_type_refs( if acur.goto_first_child() { loop { if acur.node().is_named() { - java_collect_type_refs(acur.node(), source, true, out); + java_collect_type_refs_inner(acur.node(), source, true, out, skip); } if !acur.goto_next_sibling() { break; @@ -84,7 +154,7 @@ pub(crate) fn java_collect_type_refs( if cur.goto_first_child() { loop { if cur.node().is_named() { - java_collect_type_refs(cur.node(), source, generic, out); + java_collect_type_refs_inner(cur.node(), source, generic, out, skip); } if !cur.goto_next_sibling() { break; @@ -98,7 +168,7 @@ pub(crate) fn java_collect_type_refs( if cur.goto_first_child() { loop { if cur.node().is_named() { - java_collect_type_refs(cur.node(), source, generic, out); + java_collect_type_refs_inner(cur.node(), source, generic, out, skip); } if !cur.goto_next_sibling() { break; diff --git a/crates/graphify-extract/src/generic/ruby.rs b/crates/graphify-extract/src/generic/ruby.rs new file mode 100644 index 0000000..fc9e256 --- /dev/null +++ b/crates/graphify-extract/src/generic/ruby.rs @@ -0,0 +1,99 @@ +//! Ruby local type inference for member-call resolution (#1499). +//! +//! Ruby has no type annotations, so a member call `obj.method()` can only be +//! resolved by name unless we know `obj`'s type. We infer it from local +//! `var = ClassName.new` bindings within a single method body and carry it on +//! each member-call `RawCall` as `receiver_type`, letting the cross-file pass +//! resolve `var.method` by type rather than by globally-unique method name. + +use std::collections::HashMap; +use std::collections::hash_map::Entry; + +use tree_sitter::Node; + +use super::names::read_text_owned; + +/// Return `ClassName` if `node` is a `ClassName.new(...)` call, else `None`. +/// +/// Only a bare capitalized constant receiver counts (`Processor.new`); +/// namespaced (`A::B.new`) and dynamic receivers are intentionally ignored so +/// the binding stays unambiguous. Mirrors Python `_ruby_new_class_name`. +fn ruby_new_class_name(node: Node<'_>, source: &[u8]) -> Option { + if node.kind() != "call" { + return None; + } + let recv = node.child_by_field_name("receiver")?; + let meth = node.child_by_field_name("method")?; + if recv.kind() != "constant" || read_text_owned(meth, source) != "new" { + return None; + } + Some(read_text_owned(recv, source)) +} + +/// Map `local_var -> ClassName` for `var = ClassName.new` within one Ruby method +/// body, not descending into nested method definitions. +/// +/// 100%-confidence contract: a variable assigned more than once, or to anything +/// other than a single `Constant.new`, maps to `None` (ambiguous) so callers +/// never resolve it. Only the certain single-binding case carries a type. +/// Mirrors Python `_ruby_local_class_bindings`. +#[must_use] +pub(super) fn ruby_local_class_bindings( + body_node: Node<'_>, + source: &[u8], +) -> HashMap> { + let mut bindings: HashMap> = HashMap::new(); + visit(body_node, source, &mut bindings); + bindings +} + +fn visit(node: Node<'_>, source: &[u8], bindings: &mut HashMap>) { + let mut cur = node.walk(); + if !cur.goto_first_child() { + return; + } + loop { + let child = cur.node(); + // A nested method has its own scope — don't descend into it. + if matches!(child.kind(), "method" | "singleton_method") { + if !cur.goto_next_sibling() { + break; + } + continue; + } + if child.kind() == "assignment" + && let Some(left) = child.child_by_field_name("left") + && left.kind() == "identifier" + { + let var = read_text_owned(left, source); + let cls = child + .child_by_field_name("right") + .and_then(|right| ruby_new_class_name(right, source)); + match cls { + // Assigned to something we can't type: poison only if it was + // already typed (matches Python — an untyped var stays absent). + None => { + if let Entry::Occupied(mut e) = bindings.entry(var) { + e.insert(None); + } + } + Some(c) => match bindings.entry(var) { + Entry::Occupied(mut e) => { + // Reassigned to a different class → ambiguous (poison); + // an identical re-binding keeps the type. + if e.get().as_deref() != Some(c.as_str()) { + e.insert(None); + } + } + Entry::Vacant(e) => { + e.insert(Some(c)); + } + }, + } + } + visit(child, source, bindings); + if !cur.goto_next_sibling() { + break; + } + } +} diff --git a/crates/graphify-extract/src/generic/walk.rs b/crates/graphify-extract/src/generic/walk.rs index 4baaab7..b4577c6 100644 --- a/crates/graphify-extract/src/generic/walk.rs +++ b/crates/graphify-extract/src/generic/walk.rs @@ -382,6 +382,67 @@ fn emit_member_type_refs( } } +/// Emit `references` edges for a Java record's header components +/// (`record Order(Payload p, List items, Attachment... rest)`), mirroring +/// field-type references. Type-parameter components are skipped by the +/// collector (#1519). +fn emit_java_record_component_refs( + ctx: &mut WalkCtx<'_, '_>, + record_node: Node<'_>, + class_nid: &str, + source: &[u8], +) { + let Some(components) = record_node.child_by_field_name("parameters") else { + return; + }; + let mut cur = components.walk(); + if !cur.goto_first_child() { + return; + } + loop { + let component = cur.node(); + let type_node = match component.kind() { + "formal_parameter" => component.child_by_field_name("type"), + "spread_parameter" => { + // `Attachment... rest`: the type is the first named child that is + // not the `modifiers` annotation block or the binder declarator. + let mut found = None; + let mut scur = component.walk(); + if scur.goto_first_child() { + loop { + let child = scur.node(); + if child.is_named() + && !matches!(child.kind(), "modifiers" | "variable_declarator") + { + found = Some(child); + break; + } + if !scur.goto_next_sibling() { + break; + } + } + } + found + } + _ => None, + }; + if let Some(type_node) = type_node { + let component_line = component.start_position().row as u32 + 1; + emit_member_type_refs( + ctx, + type_node, + class_nid, + component_line, + source, + super::references::java_collect_type_refs, + ); + } + if !cur.goto_next_sibling() { + break; + } + } +} + // ── Structural walk ─────────────────────────────────────────────────────────── /// Shared state threaded through every structural-walk recursion. @@ -573,6 +634,12 @@ pub(super) fn walk<'tree>( ); } } + // Java record components (the `record Order(Payload p, List + // items)` header parameters) -> references, mirroring field types + // (#1519). Type-parameter components are skipped by the collector. + if t == "record_declaration" { + emit_java_record_component_refs(ctx, node, &class_nid, source); + } } // C++ base_class_clause diff --git a/crates/graphify-extract/src/import_handlers.rs b/crates/graphify-extract/src/import_handlers.rs index 42b8205..87cf73f 100644 --- a/crates/graphify-extract/src/import_handlers.rs +++ b/crates/graphify-extract/src/import_handlers.rs @@ -562,7 +562,7 @@ pub fn import_c( } /// Resolve a quoted C `#include` path relative to the including file, returning the canonical path if it exists. -fn resolve_c_include_path(raw: &str, str_path: &str) -> Option { +pub(crate) fn resolve_c_include_path(raw: &str, str_path: &str) -> Option { if raw.is_empty() { return None; } diff --git a/crates/graphify-extract/src/lang_configs.rs b/crates/graphify-extract/src/lang_configs.rs index 1d80b93..c84b4ee 100644 --- a/crates/graphify-extract/src/lang_configs.rs +++ b/crates/graphify-extract/src/lang_configs.rs @@ -291,7 +291,7 @@ pub static RUBY: LazyLock = LazyLock::new(|| LangConfig { call_accessor_node_types: &[], call_accessor_field: "", function_boundary_types: &["method", "singleton_method"], - lang_id: LangId::Other, + lang_id: LangId::Ruby, import_handler: None, resolve_function_name: None, helper_fn_names: &[], diff --git a/crates/graphify-extract/src/postprocess.rs b/crates/graphify-extract/src/postprocess.rs index 1b64066..6155bcf 100644 --- a/crates/graphify-extract/src/postprocess.rs +++ b/crates/graphify-extract/src/postprocess.rs @@ -20,6 +20,19 @@ use serde_json::Value; static NON_ALNUM: LazyLock = LazyLock::new(|| Regex::new(r"[^a-zA-Z0-9]+").expect("static label-key regex")); +/// Header file suffixes (without the dot): a C/ObjC/C++ quoted include always +/// targets the header, so an import edge dangling on a salted-away bare id is +/// repointed to the header variant of the colliding id (#1475). +const HEADER_SUFFIXES: [&str; 4] = ["h", "hpp", "hh", "hxx"]; + +/// First 6 hex chars of the SHA-1 of `s` — an injective-enough salt to split +/// node ids whose naive disambiguator still collides (#1522). Matches Python's +/// `hashlib.sha1(...).hexdigest()[:6]`. +fn sha1_hex6(s: &str) -> String { + use sha1::{Digest, Sha1}; + hex::encode(Sha1::digest(s.as_bytes()))[..6].to_string() +} + /// Canonical form of `source_file` used for disambiguating colliding /// node IDs. Mirrors `_source_key` in the Python source. #[must_use] @@ -50,6 +63,87 @@ fn node_disambiguation_source_key(node: &Node, root: &Path) -> String { } } +/// Salt every node id in one collision group (`old_id` shared across distinct +/// source files) with its source path, recording `(old_id, source_key) -> new_id` +/// in `remap` and rewriting the node ids. When the naive salt +/// `make_id(source_key, old_id)` itself collides (separator-vs-punctuation paths, +/// #1522), a short sha1 of the raw source path is appended so the colliders split. +fn salt_collision_group( + old_id: &str, + group: &[usize], + source_keys: &HashSet, + nodes: &mut [Node], + root: &Path, + remap: &mut HashMap<(String, String), String>, +) { + let mut naive: HashMap = HashMap::new(); + for sk in source_keys { + if !sk.is_empty() { + naive.insert(sk.clone(), make_id(&[sk, old_id])); + } + } + let mut naive_counts: HashMap<&str, usize> = HashMap::new(); + for nid in naive.values() { + *naive_counts.entry(nid.as_str()).or_default() += 1; + } + let needs_hash: HashSet = naive + .iter() + .filter(|(_, nid)| naive_counts.get(nid.as_str()).copied().unwrap_or(0) > 1) + .map(|(sk, _)| sk.clone()) + .collect(); + for &idx in group { + let sk = node_disambiguation_source_key(&nodes[idx], root); + if sk.is_empty() { + continue; + } + let new_id = if needs_hash.contains(&sk) { + make_id(&[&sk, old_id, &sha1_hex6(&sk)]) + } else { + naive + .get(&sk) + .cloned() + .unwrap_or_else(|| make_id(&[&sk, old_id])) + }; + remap.insert((old_id.to_string(), sk), new_id.clone()); + if new_id != *old_id { + nodes[idx].id = new_id; + } + } +} + +/// Build `old_id -> header-variant new_id` for colliding ids whose group includes +/// a header file (`.h`/`.hpp`/…), so a quoted-include import edge dangling on the +/// salted-away bare id is repointed to the header variant (#1475). +fn build_header_remaps( + ambiguous_ids: &HashSet, + by_id: &HashMap>, + nodes: &[Node], + root: &Path, + remap: &HashMap<(String, String), String>, +) -> HashMap { + let mut header_remaps: HashMap = HashMap::new(); + for old_id in ambiguous_ids { + let Some(group) = by_id.get(old_id) else { + continue; + }; + for &idx in group { + let sk = node_disambiguation_source_key(&nodes[idx], root); + let is_header = Path::new(&sk) + .extension() + .and_then(|e| e.to_str()) + .is_some_and(|e| HEADER_SUFFIXES.contains(&e.to_lowercase().as_str())); + if !sk.is_empty() + && is_header + && let Some(new_id) = remap.get(&(old_id.clone(), sk)) + { + header_remaps.insert(old_id.clone(), new_id.clone()); + break; + } + } + } + header_remaps +} + /// Rewrite only node IDs that collide across two or more *distinct* /// source files, using the source path as the disambiguator. /// @@ -96,17 +190,7 @@ pub fn disambiguate_colliding_node_ids( continue; } ambiguous_ids.insert(old_id.clone()); - for &idx in group { - let sk = node_disambiguation_source_key(&nodes[idx], root); - if sk.is_empty() { - continue; - } - let new_id = make_id(&[&sk, old_id]); - remap.insert((old_id.clone(), sk), new_id.clone()); - if new_id != *old_id { - nodes[idx].id = new_id; - } - } + salt_collision_group(old_id, group, &source_keys, nodes, root, &mut remap); } if remap.is_empty() { @@ -140,6 +224,8 @@ pub fn disambiguate_colliding_node_ids( } } + let header_remaps = build_header_remaps(&ambiguous_ids, &by_id, nodes, root, &remap); + for edge in edges.iter_mut() { let edge_source_key = source_key(&edge.source_file, root); let source_key_tuple = (edge.source.clone(), edge_source_key.clone()); @@ -149,7 +235,15 @@ pub fn disambiguate_colliding_node_ids( } else if let Some(new_id) = unambiguous_remaps.get(&edge.source) { edge.source.clone_from(new_id); } - if let Some(new_id) = remap.get(&target_key_tuple) { + // imports/imports_from always target a header file, so they must resolve + // to the header variant BEFORE the same-source-file salt is considered. + // Keying the import target by the importer's own source file mis-points a + // `.m` importing its own `.h` back at itself (self-loop) (#1475). + if matches!(edge.relation.as_str(), "imports" | "imports_from") + && let Some(new_id) = header_remaps.get(&edge.target) + { + edge.target.clone_from(new_id); + } else if let Some(new_id) = remap.get(&target_key_tuple) { edge.target.clone_from(new_id); } else if let Some(new_id) = unambiguous_remaps.get(&edge.target) { edge.target.clone_from(new_id); diff --git a/crates/graphify-extract/src/tsconfig.rs b/crates/graphify-extract/src/tsconfig.rs index 18eeecc..1195f89 100644 --- a/crates/graphify-extract/src/tsconfig.rs +++ b/crates/graphify-extract/src/tsconfig.rs @@ -33,8 +33,12 @@ pub fn strip_jsonc(text: &str) -> String { TRAILING_COMMA_RE.replace_all(&stripped, "$1").into_owned() } +/// Alias prefix → ordered list of resolved base dirs. tsc tries each target in +/// declared order until one resolves on disk (#1531). +pub type AliasMap = IndexMap>; + // Cache: tsconfig path string → alias map -static ALIAS_CACHE: LazyLock>>> = +static ALIAS_CACHE: LazyLock>> = LazyLock::new(|| Mutex::new(IndexMap::new())); /// Recursively read path aliases from a tsconfig, following `extends` chains. @@ -44,7 +48,7 @@ pub fn read_tsconfig_aliases( tsconfig: &Path, base_dir: &Path, seen: &mut HashSet, -) -> IndexMap { +) -> AliasMap { let key = tsconfig.to_string_lossy().into_owned(); if seen.contains(&key) { return IndexMap::new(); @@ -61,7 +65,7 @@ pub fn read_tsconfig_aliases( Err(_) => return IndexMap::new(), }; - let mut aliases: IndexMap = IndexMap::new(); + let mut aliases: AliasMap = IndexMap::new(); // Follow extends chain. TypeScript 5.0 allows `extends` to be either a // string or an array of paths; for an array, parents are processed in @@ -119,17 +123,24 @@ pub fn read_tsconfig_aliases( if arr.is_empty() { continue; } - let Some(first) = arr[0].as_str() else { - continue; - }; let alias_prefix = alias.trim_end_matches("/*"); - let target_base = first.trim_end_matches("/*"); - aliases.insert( - alias_prefix.to_string(), - normpath(&paths_base.join(target_base)) - .to_string_lossy() - .into_owned(), - ); + // Keep ALL targets in declared order — tsc tries each until one + // resolves on disk. Discarding the fallbacks (#1531) misresolved or + // dropped imports whose file lived at a non-first target. + // Empty / non-string entries are skipped. + let target_bases: Vec = arr + .iter() + .filter_map(serde_json::Value::as_str) + .filter(|t| !t.is_empty()) + .map(|t| { + normpath(&paths_base.join(t.trim_end_matches("/*"))) + .to_string_lossy() + .into_owned() + }) + .collect(); + if !target_bases.is_empty() { + aliases.insert(alias_prefix.to_string(), target_bases); + } } } @@ -139,7 +150,7 @@ pub fn read_tsconfig_aliases( /// Lexically normalise a path (collapse `.`, resolve `..` where possible), /// mirroring Python's `os.path.normpath`. Used so a `baseUrl` like `./src` /// does not leave a `.` component in the resolved alias target. -fn normpath(p: &Path) -> PathBuf { +pub(crate) fn normpath(p: &Path) -> PathBuf { use std::path::Component; let mut out = PathBuf::new(); for comp in p.components() { @@ -176,7 +187,7 @@ fn normpath(p: &Path) -> PathBuf { /// Panics if the internal alias cache mutex is poisoned (which would indicate /// a prior panic in another thread holding the lock). #[must_use] -pub fn load_tsconfig_aliases(start_dir: &Path) -> IndexMap { +pub fn load_tsconfig_aliases(start_dir: &Path) -> AliasMap { let current = match start_dir.canonicalize() { Ok(c) => c, Err(_) => start_dir.to_path_buf(), @@ -210,6 +221,35 @@ pub fn load_tsconfig_aliases(start_dir: &Path) -> IndexMap { IndexMap::new() } +/// Resolve `raw` against tsconfig path aliases (#1531). Try each target in +/// declared order; return the first whose candidate resolves to a real file on +/// disk (tsc parity). If none exist, return the first candidate (no false edge +/// is fabricated; the prior single-target behaviour is preserved). Returns +/// `None` when no alias prefix matches. +/// +/// Mirrors Python `_resolve_tsconfig_alias`. +#[must_use] +pub fn resolve_tsconfig_alias(raw: &str, aliases: &AliasMap) -> Option { + for (alias_prefix, alias_bases) in aliases { + if raw == alias_prefix || raw.starts_with(&format!("{alias_prefix}/")) { + let rest = raw[alias_prefix.len()..].trim_start_matches('/'); + let mut first: Option = None; + for base in alias_bases { + let cand = normpath(&Path::new(base).join(rest)); + let resolved = resolve_js_module_path(&cand); + if resolved.is_file() { + return Some(resolved); + } + if first.is_none() { + first = Some(cand); + } + } + return first; + } + } + None +} + /// Resolve a JS/TS-style import specifier path to an actual file on disk. /// /// Mirrors Python `_resolve_js_module_path`. diff --git a/crates/graphify-extract/src/types.rs b/crates/graphify-extract/src/types.rs index 36d6a0c..98dfade 100644 --- a/crates/graphify-extract/src/types.rs +++ b/crates/graphify-extract/src/types.rs @@ -84,6 +84,12 @@ pub struct RawCall { /// by cross-file member-call resolution (#1356). `None` for other languages /// and non-member calls. pub receiver: Option, + /// For Ruby member calls (`var.method()`), the receiver's inferred type from + /// local `var = ClassName.new` bindings, when unambiguously known. Lets the + /// cross-file pass resolve the call by the receiver's *type* rather than by + /// globally-unique method name (#1499). `None` for other languages, non-member + /// calls, and receivers whose type is unknown or ambiguous. + pub receiver_type: Option, } /// Result of extracting a single file. diff --git a/crates/graphify-extract/src/workspace.rs b/crates/graphify-extract/src/workspace.rs index a384250..1e23a0c 100644 --- a/crates/graphify-extract/src/workspace.rs +++ b/crates/graphify-extract/src/workspace.rs @@ -325,13 +325,62 @@ fn walk_subdirs(base: &Path, out: &mut Vec) { } } +/// Condition keys consulted when resolving an `exports` target, in priority +/// order. `default` is Node's catch-all and must be consulted LAST so a more +/// specific condition (source/import/module/etc.) wins when several match. +/// Mirrors Python `_EXPORT_CONDITION_PRIORITY`. +const EXPORT_CONDITION_PRIORITY: [&str; 7] = [ + "source", "import", "module", "svelte", "types", "require", "default", +]; + +/// Resolve an `exports` map value (string or condition object) to a relative +/// target string, honouring [`EXPORT_CONDITION_PRIORITY`] for objects and +/// recursing into nested condition objects. Mirrors Python `_resolve_export_target`. +fn resolve_export_target(value: &Value) -> Option { + if let Some(s) = value.as_str() { + return Some(s.to_string()); + } + let obj = value.as_object()?; + for cond in EXPORT_CONDITION_PRIORITY { + match obj.get(cond) { + Some(Value::String(s)) => return Some(s.clone()), + Some(nested @ Value::Object(_)) => { + if let Some(found) = resolve_export_target(nested) { + return Some(found); + } + } + _ => {} + } + } + None +} + +/// Guard against `exports` targets that escape the package directory (e.g. +/// `"./evil": "../../../etc/passwd"`). `true` only when `package_dir/target` +/// stays within `package_dir`. Mirrors Python `_contained_in_package` +/// (`Path.resolve().is_relative_to(...)`): symlinks are followed when the +/// candidate exists, with a lexical `..`-collapse fallback for a not-yet-created +/// target so an escaping symlink can't slip through. +fn contained_in_package(target: &str, package_dir: &Path) -> bool { + let Ok(pkg_canon) = package_dir.canonicalize() else { + return false; + }; + let candidate = pkg_canon.join(target); + let resolved = candidate + .canonicalize() + .unwrap_or_else(|_| crate::tsconfig::normpath(&candidate)); + resolved.starts_with(&pkg_canon) +} + /// Pick the most likely entry-point file for a workspace package. /// -/// When `subpath` is non-empty (i.e. the import was `pkg/foo/bar`), -/// return `package_dir/subpath` directly. Otherwise read `package.json` -/// and prefer (in order) `exports["."]` (string or object), then -/// `svelte` / `module` / `main` / `types`, then `src/index` / `index` -/// fallbacks. Mirrors `_package_entry_candidates` in Python. +/// When `subpath` is non-empty (i.e. the import was `pkg/foo/bar`), the +/// package's `exports` subpath map is consulted first (`./foo` conditions or a +/// single `./*` wildcard, with a path-escape guard, #1308), falling back to +/// `package_dir/subpath`. Otherwise read `package.json` and prefer (in order) +/// `exports["."]` (string or condition object), then `svelte` / `module` / +/// `main` / `types`, then `src/index` / `index` fallbacks. Mirrors +/// `_package_entry_candidates` in Python. #[must_use] pub fn package_entry_candidates(package_dir: &Path, subpath: &str) -> Vec { let manifest_path = package_dir.join("package.json"); @@ -341,6 +390,46 @@ pub fn package_entry_candidates(package_dir: &Path, subpath: &str) -> Vec conditions -> file, plus single + // wildcard "./*" patterns. Targets that escape the package dir are + // rejected; resolution then falls through to the bare path. + if let Some(exports) = manifest_data.get("exports").and_then(Value::as_object) { + let subpath_key = format!("./{subpath}"); + if let Some(target) = exports.get(&subpath_key).and_then(resolve_export_target) { + if contained_in_package(&target, package_dir) { + return vec![package_dir.join(target)]; + } + } else { + for (pattern, pattern_value) in exports { + if pattern.matches('*').count() != 1 { + continue; + } + let mut parts = pattern.splitn(2, '*'); + let prefix = parts.next().unwrap_or(""); + let suffix = parts.next().unwrap_or(""); + if !(subpath_key.starts_with(prefix) + && (suffix.is_empty() || subpath_key.ends_with(suffix))) + { + continue; + } + let end = subpath_key.len().saturating_sub(suffix.len()); + let matched = if prefix.len() <= end { + &subpath_key[prefix.len()..end] + } else { + "" + }; + if let Some(resolved) = resolve_export_target(pattern_value) + && resolved.contains('*') + { + let target = resolved.replace('*', matched); + if contained_in_package(&target, package_dir) { + return vec![package_dir.join(target)]; + } + } + } + } + } return vec![package_dir.join(subpath)]; } @@ -350,17 +439,9 @@ pub fn package_entry_candidates(package_dir: &Path, subpath: &str) -> Vec = path_nodes - .iter() - .filter_map(|n| lookup_str(n, "origin_file")) - .collect(); - assert_eq!( - origins.len(), - 2, - "Path stubs must carry distinct origin_file: {origins:?}" - ); - assert!(origins.iter().any(|o| o.ends_with("a.py")), "{origins:?}"); - assert!(origins.iter().any(|o| o.ends_with("b.py")), "{origins:?}"); } #[test] diff --git a/crates/graphify-extract/tests/id_collision.rs b/crates/graphify-extract/tests/id_collision.rs new file mode 100644 index 0000000..6495b24 --- /dev/null +++ b/crates/graphify-extract/tests/id_collision.rs @@ -0,0 +1,179 @@ +//! Parity tests for node-id collision salting (#1522), ported from +//! `graphify-py/tests/test_extract.py`. +#![allow( + clippy::expect_used, + clippy::unwrap_used, + clippy::case_sensitive_file_extension_comparisons +)] + +use std::collections::HashSet; +use std::path::Path; + +use graphify_extract::{extract, file_stem, make_id}; +use serde_json::Value; + +fn write(path: &Path, text: &str) { + std::fs::create_dir_all(path.parent().expect("parent")).expect("create_dir_all"); + std::fs::write(path, text).expect("write"); +} + +#[test] +fn separator_collision_paths_get_distinct_ids() { + // Two distinct paths whose only difference is a separator-vs-punctuation swap + // (`foo/bar_baz.py` vs `foo_bar/baz.py`) normalize to the same stem; the + // disambiguation pass salts the colliders with a stable path hash so they stay + // distinct instead of silently merging (#1522). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write(&root.join("foo/bar_baz.py"), "class Widget:\n pass\n"); + write(&root.join("foo_bar/baz.py"), "class Gadget:\n pass\n"); + + let out = extract( + &[root.join("foo/bar_baz.py"), root.join("foo_bar/baz.py")], + Some(root), + ); + let file_ids: HashSet<&str> = out + .nodes + .iter() + .filter(|n| { + n.get("label") + .and_then(Value::as_str) + .is_some_and(|l| l.ends_with(".py")) + }) + .filter_map(|n| n.get("id").and_then(Value::as_str)) + .collect(); + let file_count = out + .nodes + .iter() + .filter(|n| { + n.get("label") + .and_then(Value::as_str) + .is_some_and(|l| l.ends_with(".py")) + }) + .count(); + assert_eq!(file_count, 2, "both .py files must survive as nodes"); + assert_eq!( + file_ids.len(), + 2, + "file ids must stay distinct: {file_ids:?}" + ); +} + +#[test] +fn non_colliding_path_id_is_not_salted() { + // The collision hash must touch only actual colliders — a path with no + // collision keeps its plain full-path stem id (no hash suffix). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write( + &root.join("src/auth/session.py"), + "class Session:\n pass\n", + ); + + let out = extract(&[root.join("src/auth/session.py")], Some(root)); + let file_id = out + .nodes + .iter() + .find(|n| n.get("source_location").and_then(Value::as_str) == Some("L1")) + .and_then(|n| n.get("id").and_then(Value::as_str)) + .expect("file node with L1 source_location"); + assert_eq!(file_id, "src_auth_session"); + assert_eq!( + file_id, + make_id(&[&file_stem(Path::new("src/auth/session.py"))]) + ); +} + +#[test] +fn origin_file_is_not_serialized_into_extract_output() { + // origin_file is an internal disambiguation hint (#1462) consumed only by the + // colliding-id pass; it must not survive into the returned nodes (#1516), + // where it would ship an absolute, machine-specific path (#555, #932). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write( + &root.join("pkg/a.py"), + "from pathlib import Path\ndef use_a(p: Path):\n return p\n", + ); + write( + &root.join("pkg/b.py"), + "from pathlib import Path\ndef use_b(p: Path):\n return p\n", + ); + let out = extract(&[root.join("pkg/a.py"), root.join("pkg/b.py")], Some(root)); + + // The internal field is gone from every node... + assert!( + out.nodes.iter().all(|n| n.get("origin_file").is_none()), + "origin_file leaked into output nodes" + ); + // ...so no node leaks the absolute sandbox path in any string value. + let root_str = root.to_string_lossy(); + let leaked: Vec<_> = out + .nodes + .iter() + .flat_map(|n| n.iter()) + .filter(|(_, v)| v.as_str().is_some_and(|s| s.contains(root_str.as_ref()))) + .collect(); + assert!( + leaked.is_empty(), + "absolute paths leaked into nodes: {leaked:?}" + ); + // ...yet the colliding-id pass still kept the two cross-file Path stubs distinct. + let path_ids: HashSet<&str> = out + .nodes + .iter() + .filter(|n| n.get("label").and_then(Value::as_str) == Some("Path")) + .filter_map(|n| n.get("id").and_then(Value::as_str)) + .collect(); + assert_eq!( + path_ids.len(), + 2, + "two distinct Path stubs expected: {path_ids:?}" + ); +} + +#[test] +fn go_imported_type_stubs_do_not_collide_across_source_files() { + // #1462 for the dedicated extractors: same-label cross-file stubs must stay + // distinct per file while keeping source_file empty so the #1402 rewire still + // collapses them onto a real definition when one exists. + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + write( + &root.join("a/use_a.go"), + "package a\n\nimport \"ext\"\n\nfunc UseA(w ext.Widget) {}\n", + ); + write( + &root.join("b/use_b.go"), + "package b\n\nimport \"ext\"\n\nfunc UseB(w ext.Widget) {}\n", + ); + let out = extract( + &[root.join("a/use_a.go"), root.join("b/use_b.go")], + Some(root), + ); + let widgets: Vec<(&str, &str)> = out + .nodes + .iter() + .filter(|n| n.get("label").and_then(Value::as_str) == Some("Widget")) + .filter_map(|n| { + Some(( + n.get("id")?.as_str()?, + n.get("source_file").and_then(Value::as_str).unwrap_or(""), + )) + }) + .collect(); + assert_eq!(widgets.len(), 2, "expected 2 Widget stubs: {widgets:?}"); + assert_eq!( + widgets + .iter() + .map(|(id, _)| *id) + .collect::>() + .len(), + 2, + "Widget stub ids must be distinct: {widgets:?}" + ); + assert!( + widgets.iter().all(|(_, sf)| sf.is_empty()), + "Widget stubs must stay sourceless: {widgets:?}" + ); +} diff --git a/crates/graphify-extract/tests/java_type_resolution.rs b/crates/graphify-extract/tests/java_type_resolution.rs index 5d4ba07..4d76f00 100644 --- a/crates/graphify-extract/tests/java_type_resolution.rs +++ b/crates/graphify-extract/tests/java_type_resolution.rs @@ -3,7 +3,7 @@ //! `graphify-py/tests/test_java_type_resolution.py`. #![allow(clippy::expect_used, clippy::unwrap_used)] -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use graphify_build::build_from_json; @@ -261,3 +261,38 @@ fn java_cross_file_constructor_call_resolves() { let g = build_from_json(serde_json::to_value(&res).unwrap(), false, None).expect("build"); assert!(g.contains_node(foo_id), "Foo node missing after build"); } + +#[test] +fn java_type_parameters_do_not_resolve_to_real_class() { + // #1518: a generic field `List` must not emit a references edge to a real + // same-named class `T` — `T` is a type variable, not a type. + let tmp = tempfile::tempdir().unwrap(); + let real_type = write_file(tmp.path(), "T.java", "public class T {}\n"); + let generic = write_file( + tmp.path(), + "Generic.java", + "public class Generic { java.util.List values; }\n", + ); + let res = extract(&[real_type, generic], Some(tmp.path())); + + let id_to_label: HashMap<&str, &str> = res + .nodes + .iter() + .filter_map(|n| Some((n.get("id")?.as_str()?, n.get("label")?.as_str()?))) + .collect(); + let has_generic_t_ref = res.edges.iter().any(|e| { + e.get("relation").and_then(|v| v.as_str()) == Some("references") + && e.get("source") + .and_then(|v| v.as_str()) + .and_then(|s| id_to_label.get(s).copied()) + == Some("Generic") + && e.get("target") + .and_then(|v| v.as_str()) + .and_then(|t| id_to_label.get(t).copied()) + == Some("T") + }); + assert!( + !has_generic_t_ref, + "type parameter T must not resolve to the real T class" + ); +} diff --git a/crates/graphify-extract/tests/js_alias_relative_paths.rs b/crates/graphify-extract/tests/js_alias_relative_paths.rs new file mode 100644 index 0000000..5b0032b --- /dev/null +++ b/crates/graphify-extract/tests/js_alias_relative_paths.rs @@ -0,0 +1,128 @@ +//! #1529: alias/workspace import targets orphaned by the full-path id migration. +//! +//! Ports `test_alias_import_edge_resolves_with_relative_input_paths` from +//! `graphify-py/tests/test_js_import_resolution.py`. The bug only reproduces with +//! RELATIVE input paths (so the input-form file id differs from the +//! absolute-resolved form an alias import keys its edge target off of); absolute +//! / `tmp_path` inputs hide it because the two forms coincide. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use graphify_extract::{ExtractOutput, extract, file_node_id, file_stem, make_id}; +use serde_json::Value; +use tempfile::tempdir; + +type TestResult = Result<(), Box>; + +/// Restore the process CWD on drop so this chdir-based test can't leak into +/// other tests (nextest also isolates each test in its own process). +struct CwdGuard(PathBuf); + +impl Drop for CwdGuard { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } +} + +fn write(path: &Path, text: &str) -> TestResult { + std::fs::create_dir_all(path.parent().ok_or("path has no parent")?)?; + std::fs::write(path, text)?; + Ok(()) +} + +fn edge_targets_from(out: &ExtractOutput, source_id: &str, relation: &str) -> Vec { + out.edges + .iter() + .filter(|e| { + e.get("source").and_then(Value::as_str) == Some(source_id) + && e.get("relation").and_then(Value::as_str) == Some(relation) + }) + .filter_map(|e| e.get("target").and_then(Value::as_str).map(str::to_string)) + .collect() +} + +/// Mirrors Python `_has_symbol_edge`: a named-symbol `imports` edge from +/// `source` to `make_id(file_stem(target_file), symbol)`. +fn has_symbol_edge(out: &ExtractOutput, source: &str, target_file: &str, symbol: &str) -> bool { + let s = file_node_id(Path::new(source)); + let t = make_id(&[&file_stem(Path::new(target_file)), symbol]); + out.edges.iter().any(|e| { + e.get("source").and_then(Value::as_str) == Some(s.as_str()) + && e.get("target").and_then(Value::as_str) == Some(t.as_str()) + && e.get("relation").and_then(Value::as_str) == Some("imports") + }) +} + +#[test] +fn alias_import_edge_resolves_with_relative_input_paths() -> TestResult { + let tmp = tempdir()?; + let root = tmp.path().canonicalize()?; + write( + &root.join("tsconfig.json"), + r#"{"compilerOptions": {"baseUrl": ".", "paths": {"@/*": ["src/*"]}}}"#, + )?; + write( + &root.join("src/lib/utils.ts"), + "export function formatDate(d) { return d }\n", + )?; + write( + &root.join("src/components/Button.tsx"), + "import { formatDate } from '@/lib/utils'\nexport function Button() { return formatDate(1) }\n", + )?; + + let original = std::env::current_dir()?; + let _guard = CwdGuard(original); + std::env::set_current_dir(&root)?; + + // CRUCIAL: relative input paths. Alias imports resolve specifiers through an + // absolute path, so the import-target id is keyed off the ABSOLUTE form; with + // relative inputs the id-remap (keyed on the input form) must also cover the + // absolute form or the edge orphans and is dropped (#1529). + let rel_paths = [ + PathBuf::from("src/lib/utils.ts"), + PathBuf::from("src/components/Button.tsx"), + ]; + let out = extract(&rel_paths, Some(Path::new("."))); + + let node_ids: HashSet<&str> = out + .nodes + .iter() + .filter_map(|n| n.get("id").and_then(Value::as_str)) + .collect(); + let target_id = file_node_id(Path::new("src/lib/utils.ts")); + let button_id = file_node_id(Path::new("src/components/Button.tsx")); + + // The file-level imports_from edge must target the REAL utils file node. + let import_targets = edge_targets_from(&out, &button_id, "imports_from"); + assert!( + node_ids.contains(target_id.as_str()), + "real utils file node missing; nodes: {node_ids:?}" + ); + assert!( + import_targets.iter().any(|t| t == &target_id), + "imports_from must target the real utils node; targets: {import_targets:?}" + ); + + // No surviving edge target may carry an absolute-path prefix from the sandbox. + let abs_prefix = file_node_id(&Path::new("src/lib/utils.ts").canonicalize()?); + assert!( + import_targets + .iter() + .all(|t| !t.starts_with(&format!("{abs_prefix}_")) && t != &abs_prefix), + "an import target kept an absolute prefix ({abs_prefix}); targets: {import_targets:?}" + ); + + // The named-symbol edge to formatDate must resolve to the real symbol node. + assert!( + has_symbol_edge( + &out, + "src/components/Button.tsx", + "src/lib/utils.ts", + "formatDate" + ), + "named-symbol import edge to formatDate did not resolve; edges: {:?}", + out.edges + ); + Ok(()) +} diff --git a/crates/graphify-extract/tests/js_workspace_resolution.rs b/crates/graphify-extract/tests/js_workspace_resolution.rs index b878dcd..56a16c8 100644 --- a/crates/graphify-extract/tests/js_workspace_resolution.rs +++ b/crates/graphify-extract/tests/js_workspace_resolution.rs @@ -221,3 +221,368 @@ fn malformed_inner_workspaces_does_not_shadow_real_root() -> TestResult { ); Ok(()) } + +// ── #1531: tsconfig path-alias fallback targets ────────────────────────────── + +#[test] +fn tsconfig_alias_resolves_second_target_when_first_missing() -> TestResult { + // tsc tries each `paths` target in declared order until one resolves on + // disk. The file lives only at the SECOND target, so keeping only the first + // entry (#1531) dropped the edge. + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("tsconfig.json"), + r#"{"compilerOptions": {"baseUrl": ".", "paths": {"$lib/*": ["generated/*", "src/lib/*"]}}}"#, + )?; + write(&root.join("src/lib/utils.ts"), "export const helper = 1\n")?; + write( + &root.join("src/routes/page.ts"), + "import { helper } from '$lib/utils'\nconsole.log(helper)\n", + )?; + + let out = extract( + &[ + root.join("src/lib/utils.ts"), + root.join("src/routes/page.ts"), + ], + Some(root), + ); + assert!( + has_imports_from(&out, "src/routes/page.ts", "src/lib/utils.ts"), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn tsconfig_alias_first_target_wins_when_both_exist() -> TestResult { + // When the file exists at BOTH targets, tsc resolves to the FIRST. The edge + // must target the generated/ copy, not src/lib. + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("tsconfig.json"), + r#"{"compilerOptions": {"baseUrl": ".", "paths": {"$lib/*": ["generated/*", "src/lib/*"]}}}"#, + )?; + write( + &root.join("generated/utils.ts"), + "export const helper = 1\n", + )?; + write(&root.join("src/lib/utils.ts"), "export const helper = 2\n")?; + write( + &root.join("src/routes/page.ts"), + "import { helper } from '$lib/utils'\nconsole.log(helper)\n", + )?; + + let out = extract( + &[ + root.join("generated/utils.ts"), + root.join("src/lib/utils.ts"), + root.join("src/routes/page.ts"), + ], + Some(root), + ); + assert!( + has_imports_from(&out, "src/routes/page.ts", "generated/utils.ts"), + "edges: {:?}", + out.edges + ); + assert!( + !has_imports_from(&out, "src/routes/page.ts", "src/lib/utils.ts"), + "first target must win; edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn tsconfig_alias_none_exist_creates_no_false_edge() -> TestResult { + // The file exists at neither target; no concrete imports_from edge to either + // candidate may be fabricated (it stays an external/phantom target). + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("tsconfig.json"), + r#"{"compilerOptions": {"baseUrl": ".", "paths": {"$lib/*": ["generated/*", "src/lib/*"]}}}"#, + )?; + write(&root.join("src/routes/other.ts"), "export const x = 1\n")?; + write( + &root.join("src/routes/page.ts"), + "import { helper } from '$lib/utils'\nconsole.log(helper)\n", + )?; + + let out = extract( + &[ + root.join("src/routes/other.ts"), + root.join("src/routes/page.ts"), + ], + Some(root), + ); + assert!( + !has_imports_from(&out, "src/routes/page.ts", "generated/utils.ts"), + "edges: {:?}", + out.edges + ); + assert!( + !has_imports_from(&out, "src/routes/page.ts", "src/lib/utils.ts"), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +// ── #1308: workspace subpath `exports` map resolution ──────────────────────── + +#[test] +fn workspace_subpath_export_string_resolves() -> TestResult { + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a", "exports": {".": "./src/index.ts", "./browser": "./src/browser.ts"}}"#, + )?; + write( + &root.join("packages/pkg-a/src/browser.ts"), + "export const value = \"ok\"\n", + )?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { value } from '@example/pkg-a/browser'\nexport const v = value\n", + )?; + + let out = extract( + &[ + root.join("packages/pkg-a/src/browser.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/src/browser.ts" + ), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn workspace_subpath_export_condition_object_resolves() -> TestResult { + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a", "exports": {"./browser": {"source": "./src/browser.ts", "import": "./dist/esm/browser.js", "require": "./dist/cjs/browser.js", "types": "./dist/types/browser.d.ts"}}}"#, + )?; + write( + &root.join("packages/pkg-a/src/browser.ts"), + "export const value = \"ok\"\n", + )?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { value } from '@example/pkg-a/browser'\nexport const v = value\n", + )?; + + let out = extract( + &[ + root.join("packages/pkg-a/src/browser.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/src/browser.ts" + ), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn workspace_subpath_export_wildcard_resolves() -> TestResult { + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a", "exports": {"./*": {"source": "./src/*.ts"}}}"#, + )?; + write( + &root.join("packages/pkg-a/src/utils.ts"), + "export function add(a: number, b: number) { return a + b }\n", + )?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { add } from '@example/pkg-a/utils'\nexport const sum = add(1, 2)\n", + )?; + + let out = extract( + &[ + root.join("packages/pkg-a/src/utils.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/src/utils.ts" + ), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn workspace_subpath_export_falls_back_to_filesystem() -> TestResult { + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a"}"#, + )?; + write( + &root.join("packages/pkg-a/browser.ts"), + "export const value = \"ok\"\n", + )?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { value } from '@example/pkg-a/browser'\nexport const v = value\n", + )?; + + let out = extract( + &[ + root.join("packages/pkg-a/browser.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/browser.ts" + ), + "edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn workspace_subpath_export_rejects_path_escape() -> TestResult { + // An exports target that escapes the package dir must NOT resolve to the + // outside path (path-containment guard). Resolution falls through to the + // bare-path fallback, which has no real file here, so no edge lands. + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a", "exports": {"./evil": "../../../../secret.ts"}}"#, + )?; + write(&root.join("secret.ts"), "export const leak = \"secret\"\n")?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { leak } from '@example/pkg-a/evil'\nexport const v = leak\n", + )?; + + let out = extract( + &[ + root.join("secret.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + !has_imports_from(&out, "apps/web/src/consumer.ts", "secret.ts"), + "escaped export must not resolve; edges: {:?}", + out.edges + ); + Ok(()) +} + +#[test] +fn workspace_subpath_export_default_consulted_last() -> TestResult { + // When both `default` and an earlier condition match, the earlier condition + // (import) must win — `default` is Node's catch-all. + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("pnpm-workspace.yaml"), + "packages:\n - 'apps/*'\n - 'packages/*'\n", + )?; + write( + &root.join("packages/pkg-a/package.json"), + r#"{"name": "@example/pkg-a", "exports": {"./browser": {"default": "./src/default-entry.ts", "import": "./src/import-entry.ts"}}}"#, + )?; + write( + &root.join("packages/pkg-a/src/import-entry.ts"), + "export const value = \"import\"\n", + )?; + write( + &root.join("packages/pkg-a/src/default-entry.ts"), + "export const value = \"default\"\n", + )?; + write( + &root.join("apps/web/src/consumer.ts"), + "import { value } from '@example/pkg-a/browser'\nexport const v = value\n", + )?; + + let out = extract( + &[ + root.join("packages/pkg-a/src/import-entry.ts"), + root.join("packages/pkg-a/src/default-entry.ts"), + root.join("apps/web/src/consumer.ts"), + ], + Some(root), + ); + assert!( + has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/src/import-entry.ts" + ), + "import condition must win; edges: {:?}", + out.edges + ); + assert!( + !has_imports_from( + &out, + "apps/web/src/consumer.ts", + "packages/pkg-a/src/default-entry.ts" + ), + "default must not win over import; edges: {:?}", + out.edges + ); + Ok(()) +} diff --git a/crates/graphify-extract/tests/objc_resolution.rs b/crates/graphify-extract/tests/objc_resolution.rs new file mode 100644 index 0000000..f0a7677 --- /dev/null +++ b/crates/graphify-extract/tests/objc_resolution.rs @@ -0,0 +1,256 @@ +//! Parity tests for Objective-C macro handling, quoted-import resolution, and +//! alloc/init type references (#1475), ported from `graphify-py/tests/test_languages.py`. +#![allow( + clippy::expect_used, + clippy::unwrap_used, + clippy::case_sensitive_file_extension_comparisons +)] + +use std::collections::{HashMap, HashSet}; + +use graphify_extract::{ExtractOutput, FileResult, extract, extract_objc}; +use serde_json::Value; + +/// `(source_label, target_label)` pairs for `relation` edges of a single-file +/// `FileResult`. +fn fr_label_edges(r: &FileResult, relation: &str) -> HashSet<(String, String)> { + let id2label: HashMap<&str, &str> = r + .nodes + .iter() + .map(|n| (n.id.as_str(), n.label.as_str())) + .collect(); + r.edges + .iter() + .filter(|e| e.relation == relation) + .filter_map(|e| { + Some(( + (*id2label.get(e.source.as_str())?).to_string(), + (*id2label.get(e.target.as_str())?).to_string(), + )) + }) + .collect() +} + +/// `(source_label, target_label)` pairs for `relation` edges of a multi-file +/// `ExtractOutput`. +fn eo_label_edges(r: &ExtractOutput, relation: &str) -> HashSet<(String, String)> { + let id2label: HashMap<&str, &str> = r + .nodes + .iter() + .filter_map(|n| Some((n.get("id")?.as_str()?, n.get("label")?.as_str()?))) + .collect(); + r.edges + .iter() + .filter(|e| e.get("relation").and_then(Value::as_str) == Some(relation)) + .filter_map(|e| { + let s = e.get("source").and_then(Value::as_str)?; + let t = e.get("target").and_then(Value::as_str)?; + Some(( + (*id2label.get(s)?).to_string(), + (*id2label.get(t)?).to_string(), + )) + }) + .collect() +} + +#[test] +fn objc_ns_assume_nonnull_macro_does_not_break_parsing() { + // `NS_ASSUME_NONNULL_BEGIN` before `@interface` made tree-sitter-objc fail to + // emit a class_interface node, swallowing the whole interface; blanking the + // argument-less macro restores it (#1475). + let tmp = tempfile::tempdir().unwrap(); + let p = tmp.path().join("AlertManager.h"); + std::fs::write( + &p, + "#import \n\ + NS_ASSUME_NONNULL_BEGIN\n\ + @class Other;\n\ + @interface AlertManager : NSObject\n\ + - (void)show;\n\ + @end\n\ + NS_ASSUME_NONNULL_END\n", + ) + .unwrap(); + let r = extract_objc(&p); + let labels: HashSet<&str> = r.nodes.iter().map(|n| n.label.as_str()).collect(); + assert!(labels.contains("AlertManager"), "{labels:?}"); + assert!( + fr_label_edges(&r, "inherits").contains(&("AlertManager".into(), "NSObject".into())), + "AlertManager should inherit NSObject" + ); + // `@class Other;` is only a forward declaration; it must not mint a class node. + assert!(!labels.contains("Other"), "{labels:?}"); +} + +#[test] +fn objc_macro_free_header_unchanged() { + // A macro-free header still parses exactly as before (regression). + let tmp = tempfile::tempdir().unwrap(); + let p = tmp.path().join("Plain.h"); + std::fs::write(&p, "@interface Plain : NSObject\n- (void)go;\n@end\n").unwrap(); + let r = extract_objc(&p); + let labels: HashSet<&str> = r.nodes.iter().map(|n| n.label.as_str()).collect(); + assert!(labels.contains("Plain"), "{labels:?}"); + assert!(fr_label_edges(&r, "inherits").contains(&("Plain".into(), "NSObject".into()))); +} + +#[test] +fn objc_alloc_init_emits_type_reference() { + // `[[Foo alloc] init]` must emit a `references` edge to the project class Foo (#1475). + let tmp = tempfile::tempdir().unwrap(); + std::fs::write( + tmp.path().join("Foo.h"), + "@interface Foo : NSObject\n@end\n", + ) + .unwrap(); + std::fs::write( + tmp.path().join("Foo.m"), + "#import \"Foo.h\"\n@implementation Foo\n@end\n", + ) + .unwrap(); + let user = tmp.path().join("User.m"); + std::fs::write( + &user, + "#import \"Foo.h\"\n\ + @implementation User\n\ + - (void)build { Foo *x = [[Foo alloc] init]; }\n\ + @end\n", + ) + .unwrap(); + let r = extract( + &[tmp.path().join("Foo.h"), tmp.path().join("Foo.m"), user], + Some(tmp.path()), + ); + assert!( + eo_label_edges(&r, "references").contains(&("-build".into(), "Foo".into())), + "expected -build -> Foo reference; edges: {:?}", + r.edges + ); +} + +#[test] +fn objc_alloc_init_unknown_class_no_resolved_edge() { + // `[[Unknown alloc] init]` with no such class must not produce a resolved + // reference edge (the sourceless stub is collapsed only when a real class exists). + let tmp = tempfile::tempdir().unwrap(); + let p = tmp.path().join("Caller.m"); + std::fs::write( + &p, + "@implementation Caller\n\ + - (void)build { id x = [[Unknown alloc] init]; }\n\ + - (void)other { [self build]; [x doStuff]; }\n\ + @end\n", + ) + .unwrap(); + let r = extract_objc(&p); + let sourced_ids: HashSet<&str> = r + .nodes + .iter() + .filter(|n| !n.source_file.is_empty()) + .map(|n| n.id.as_str()) + .collect(); + for e in r.edges.iter().filter(|e| e.relation == "references") { + assert!( + !sourced_ids.contains(e.target.as_str()), + "unexpected resolved ref: {} -> {}", + e.source, + e.target + ); + } +} + +#[test] +fn objc_quoted_import_edges_resolve_to_real_nodes() { + // Quoted `#import "X.h"` edges must target the real (disambiguated) header + // file node, not the bare stem, which gets salted away when a `.h`/`.m` pair + // exists and left the import edge dangling (#1475). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + for (name, body) in [ + ("Product.h", "@interface Product : NSObject\n@end\n"), + ( + "Product.m", + "#import \"Product.h\"\n@implementation Product\n@end\n", + ), + ("Order.h", "@interface Order : NSObject\n@end\n"), + ( + "Order.m", + "#import \"Order.h\"\n@implementation Order\n@end\n", + ), + ( + "ConsumerA.m", + "#import \"Product.h\"\n@implementation ConsumerA\n@end\n", + ), + ( + "ConsumerB.m", + "#import \"Order.h\"\n@implementation ConsumerB\n@end\n", + ), + ] { + std::fs::write(root.join(name), body).unwrap(); + } + let files: Vec<_> = [ + "Product.h", + "Product.m", + "Order.h", + "Order.m", + "ConsumerA.m", + "ConsumerB.m", + ] + .iter() + .map(|n| root.join(n)) + .collect(); + let r = extract(&files, Some(root)); + + let node_ids: HashSet<&str> = r + .nodes + .iter() + .filter_map(|n| n.get("id").and_then(Value::as_str)) + .collect(); + let id_to_label: HashMap<&str, &str> = r + .nodes + .iter() + .filter_map(|n| Some((n.get("id")?.as_str()?, n.get("label")?.as_str()?))) + .collect(); + let import_edges: Vec<(&str, &str)> = r + .edges + .iter() + .filter(|e| { + matches!( + e.get("relation").and_then(Value::as_str), + Some("imports" | "imports_from") + ) + }) + .filter_map(|e| Some((e.get("source")?.as_str()?, e.get("target")?.as_str()?))) + .collect(); + assert!(!import_edges.is_empty(), "no import edges"); + for (src, tgt) in &import_edges { + // No dangling targets, no self-loops, and every quoted import targets a + // header (.h) file node. + assert!(node_ids.contains(tgt), "dangling import target: {tgt}"); + assert_ne!(src, tgt, "self-loop import edge: {src} -> {tgt}"); + let tgt_label = id_to_label.get(tgt).copied().unwrap_or(""); + assert!( + tgt_label.ends_with(".h"), + "import target is not a header node: {tgt} -> {tgt_label}" + ); + } + // Product.m -> Product.h specifically lands on the .h variant (not salted + // back to the importing .m). + let prod_imports: Vec<_> = import_edges + .iter() + .filter(|(s, _)| { + id_to_label + .get(s) + .copied() + .unwrap_or("") + .ends_with("Product.m") + }) + .collect(); + assert!(!prod_imports.is_empty(), "no Product.m import edge"); + assert!( + prod_imports + .iter() + .all(|(_, t)| id_to_label.get(t).copied() == Some("Product.h")), + "Product.m should import the Product.h node" + ); +} diff --git a/crates/graphify-extract/tests/parity.rs b/crates/graphify-extract/tests/parity.rs index ea176aa..907f27d 100644 --- a/crates/graphify-extract/tests/parity.rs +++ b/crates/graphify-extract/tests/parity.rs @@ -897,7 +897,7 @@ fn tsconfig_alias_excess_parent_dirs_clamp_at_root() { ) .expect("write tsconfig"); let aliases = graphify_extract::tsconfig::load_tsconfig_aliases(root); - let target = aliases.get("@top").expect("@top alias present"); + let target = &aliases.get("@top").expect("@top alias present")[0]; assert!( !target.contains(".."), "excess `..` left a stray parent component: {target}" diff --git a/crates/graphify-extract/tests/parity_languages.rs b/crates/graphify-extract/tests/parity_languages.rs index 73a7ea5..549e65e 100644 --- a/crates/graphify-extract/tests/parity_languages.rs +++ b/crates/graphify-extract/tests/parity_languages.rs @@ -786,6 +786,127 @@ fn java_field_type_references_have_field_context() -> Result<(), Box` / `` / `` are type variables, not real types — no `references` edge +/// and no sourceless stub node, while real types (Base, Payload) survive. +#[test] +fn java_type_parameters_do_not_emit_references() -> Result<(), Box> { + let tmp = tempfile::tempdir()?; + let source = tmp.path().join("TypeParameters.java"); + std::fs::write( + &source, + "class Payload {}\n\ + class Base {}\n\ + class Box extends Base {\n\ + \x20 T value;\n\ + \x20 List values;\n\ + \x20 U convert(T input, List mapped, List retained) {\n\ + \x20 return null;\n\ + \x20 }\n\ + \x20 Box(V value) {}\n\ + }\n", + )?; + let result = extract_java(&source); + let references = edge_label_pairs(&result, "references", None); + assert!( + !references + .iter() + .any(|(_, t)| matches!(t.as_str(), "T" | "U" | "V")), + "type-parameter references leaked: {references:?}" + ); + assert!( + !result + .nodes + .iter() + .any(|n| matches!(n.label.as_str(), "T" | "U" | "V") && n.source_file.is_empty()), + "sourceless type-parameter stub node leaked" + ); + let inherits = edge_label_pairs(&result, "inherits", None); + assert!( + inherits.contains(&("Box".into(), "Base".into())), + "{inherits:?}" + ); + let generics = edge_label_pairs(&result, "references", Some("generic_arg")); + assert!( + generics.contains(&("convert".into(), "Payload".into())), + "{generics:?}" + ); + Ok(()) +} + +/// Ports `test_languages.py::test_java_record_component_type_references` (#1519): +/// a record's header components emit `field` / `generic_arg` references like fields. +#[test] +fn java_record_component_type_references() -> Result<(), Box> { + let tmp = tempfile::tempdir()?; + let source = tmp.path().join("RecordComponents.java"); + std::fs::write( + &source, + "class Payload {}\n\ + class Item {}\n\ + class Attachment {}\n\ + record Order(Payload payload, List items, int count, Attachment... attachments) {}\n", + )?; + let result = extract_java(&source); + let fields = edge_label_pairs(&result, "references", Some("field")); + let generics = edge_label_pairs(&result, "references", Some("generic_arg")); + assert!( + fields.contains(&("Order".into(), "Payload".into())), + "{fields:?}" + ); + assert!( + fields.contains(&("Order".into(), "List".into())), + "{fields:?}" + ); + assert!( + generics.contains(&("Order".into(), "Item".into())), + "{generics:?}" + ); + assert!( + fields.contains(&("Order".into(), "Attachment".into())), + "{fields:?}" + ); + Ok(()) +} + +/// Ports `test_languages.py::test_java_record_components_skip_type_parameters` (#1519): +/// a generic record's type parameters are skipped in its component references. +#[test] +fn java_record_components_skip_type_parameters() -> Result<(), Box> { + let tmp = tempfile::tempdir()?; + let source = tmp.path().join("GenericRecord.java"); + std::fs::write( + &source, + "class Payload {}\n\ + class Box {}\n\ + record Batch(T value, Box boxed, Box retained) {}\n", + )?; + let result = extract_java(&source); + let references = edge_label_pairs(&result, "references", None); + assert!( + !references.contains(&("Batch".into(), "T".into())), + "{references:?}" + ); + assert!( + !result + .nodes + .iter() + .any(|n| n.label == "T" && n.source_file.is_empty()), + "sourceless T stub node leaked" + ); + let fields = edge_label_pairs(&result, "references", Some("field")); + let generics = edge_label_pairs(&result, "references", Some("generic_arg")); + assert!( + fields.contains(&("Batch".into(), "Box".into())), + "{fields:?}" + ); + assert!( + generics.contains(&("Batch".into(), "Payload".into())), + "{generics:?}" + ); + Ok(()) +} + /// Ports `test_languages.py::test_java_type_annotations_have_attribute_context` (#1487). #[test] fn java_type_annotations_have_attribute_context() -> Result<(), Box> { diff --git a/crates/graphify-extract/tests/parity_postprocess.rs b/crates/graphify-extract/tests/parity_postprocess.rs index af30af0..e8ae02a 100644 --- a/crates/graphify-extract/tests/parity_postprocess.rs +++ b/crates/graphify-extract/tests/parity_postprocess.rs @@ -45,6 +45,7 @@ fn raw(caller: &str, callee: &str, source_file: &str) -> RawCall { source_file: source_file.to_string(), source_location: String::new(), receiver: None, + receiver_type: None, } } diff --git a/crates/graphify-extract/tests/ruby_resolution.rs b/crates/graphify-extract/tests/ruby_resolution.rs new file mode 100644 index 0000000..7d553fe --- /dev/null +++ b/crates/graphify-extract/tests/ruby_resolution.rs @@ -0,0 +1,202 @@ +//! Parity tests for type-aware Ruby call-graph resolution (#1499), ported from +//! `graphify-py/tests/test_ruby_resolution.py`. +//! +//! Member calls capture their receiver (extraction); `var = ClassName.new` local +//! bindings give the receiver a type (extraction); the cross-file resolver turns +//! `var.method` into a precise edge BY TYPE, not by globally-unique name — so it +//! survives name collisions and never emits a false positive when the type is +//! unknown (resolution). Every resolved edge must be EXTRACTED confidence. +#![allow(clippy::expect_used, clippy::unwrap_used)] + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use graphify_extract::{ExtractOutput, FileResult, RawCall, extract, extract_ruby}; +use serde_json::Value; +use tempfile::tempdir; + +const HELPER_RB: &str = "def transform(data)\n data.upcase\nend\n\n\ +class Processor\n def run(items)\n items.map { |i| transform(i) }\n end\nend\n"; + +const MAIN_RB: &str = "require_relative \"helper\"\n\n\ +def handle(values)\n transform(values)\nend\n\n\ +def process_all(items)\n p = Processor.new\n p.run(items)\nend\n"; + +const WORKER_RB: &str = "class Worker\n def run(jobs)\n jobs.each { |j| j }\n end\nend\n"; + +fn write(dir: &Path, name: &str, body: &str) -> PathBuf { + let p = dir.join(name); + std::fs::write(&p, body).expect("write fixture"); + p +} + +fn find_raw_call<'a>(r: &'a FileResult, callee: &str) -> Option<&'a RawCall> { + r.raw_calls.iter().find(|rc| rc.callee == callee) +} + +fn label_map(graph: &ExtractOutput) -> HashMap<&str, &str> { + graph + .nodes + .iter() + .filter_map(|n| Some((n.get("id")?.as_str()?, n.get("label")?.as_str()?))) + .collect() +} + +/// Return `(target_id, confidence)` for the `calls` edge whose source/target +/// labels contain the given substrings, or `None`. Mirrors Python `_has_call_edge`. +fn call_edge<'a>( + graph: &'a ExtractOutput, + labels: &HashMap<&str, &str>, + src_sub: &str, + tgt_sub: &str, +) -> Option<(&'a str, &'a str)> { + graph.edges.iter().find_map(|e| { + if e.get("relation").and_then(Value::as_str) != Some("calls") { + return None; + } + let s = e.get("source").and_then(Value::as_str)?; + let t = e.get("target").and_then(Value::as_str)?; + if labels.get(s).copied().unwrap_or("").contains(src_sub) + && labels.get(t).copied().unwrap_or("").contains(tgt_sub) + { + Some((t, e.get("confidence").and_then(Value::as_str).unwrap_or(""))) + } else { + None + } + }) +} + +// ── extraction level ──────────────────────────────────────────────────────── + +#[test] +fn member_call_captures_receiver() { + let tmp = tempdir().unwrap(); + let main = write(tmp.path(), "main.rb", MAIN_RB); + let r = extract_ruby(&main); + let rc = find_raw_call(&r, "run").expect("p.run should produce a raw_call with callee 'run'"); + assert!(rc.is_member_call); + assert_eq!(rc.receiver.as_deref(), Some("p")); +} + +#[test] +fn local_binding_gives_receiver_a_type() { + let tmp = tempdir().unwrap(); + let main = write(tmp.path(), "main.rb", MAIN_RB); + let r = extract_ruby(&main); + let rc = find_raw_call(&r, "run").unwrap(); + // `p = Processor.new` in the same method => p has type Processor. + assert_eq!(rc.receiver_type.as_deref(), Some("Processor")); +} + +#[test] +fn ambiguous_binding_yields_no_type() { + let tmp = tempdir().unwrap(); + let main = write( + tmp.path(), + "main.rb", + "def process_all(items)\n p = Processor.new\n p = Worker.new\n p.run(items)\nend\n", + ); + let r = extract_ruby(&main); + let rc = find_raw_call(&r, "run").unwrap(); + // reassigned to a different class => not certain => no type attached. + assert_eq!(rc.receiver_type, None); +} + +// ── resolution level ────────────────────────────────────────────────────────── + +#[test] +fn resolves_member_call_by_type() { + let tmp = tempdir().unwrap(); + write(tmp.path(), "helper.rb", HELPER_RB); + let main = write(tmp.path(), "main.rb", MAIN_RB); + let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); + let labels = label_map(&graph); + let edge = call_edge(&graph, &labels, "process_all", "run"); + assert!( + edge.is_some(), + "process_all should resolve a call to Processor#run" + ); + assert_eq!(edge.unwrap().1, "EXTRACTED"); +} + +#[test] +fn resolution_is_type_based_not_name_luck() { + // The differentiator: adding an unrelated Worker#run must NOT break the edge. + // A name-match resolver drops this (two `run` defs => ambiguous); a type-based + // resolver keeps resolving p.run -> Processor#run, never Worker#run. + let tmp = tempdir().unwrap(); + write(tmp.path(), "helper.rb", HELPER_RB); + write(tmp.path(), "worker.rb", WORKER_RB); + let main = write(tmp.path(), "main.rb", MAIN_RB); + let graph = extract( + &[ + main, + tmp.path().join("helper.rb"), + tmp.path().join("worker.rb"), + ], + Some(tmp.path()), + ); + let labels = label_map(&graph); + let edge = call_edge(&graph, &labels, "process_all", "run"); + assert!(edge.is_some(), "edge must survive the name collision"); + let (tgt_id, conf) = edge.unwrap(); + assert_eq!(conf, "EXTRACTED"); + // And it must be the RIGHT run: the target node id is prefixed by its owning + // class (helper_processor_run), so it must mention processor, never worker. + assert!( + tgt_id.to_lowercase().contains("processor"), + "expected Processor#run, got {tgt_id}" + ); + assert!(!tgt_id.to_lowercase().contains("worker")); +} + +#[test] +fn no_false_positive_when_type_unknown() { + // A member call on a receiver with no known type must NOT be resolved. + let tmp = tempdir().unwrap(); + write(tmp.path(), "helper.rb", HELPER_RB); + let main = write( + tmp.path(), + "main.rb", + "require_relative \"helper\"\n\ndef process_all(thing)\n thing.run(1)\nend\n", + ); + let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); + let labels = label_map(&graph); + // `thing` is a parameter of unknown type => no precise target => no edge. + assert!(call_edge(&graph, &labels, "process_all", "run").is_none()); +} + +#[test] +fn class_new_creates_instantiation_edge() { + // `p = Processor.new` should link the caller to the Processor class. + let tmp = tempdir().unwrap(); + write(tmp.path(), "helper.rb", HELPER_RB); + let main = write(tmp.path(), "main.rb", MAIN_RB); + let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); + let labels = label_map(&graph); + let edge = call_edge(&graph, &labels, "process_all", "Processor"); + assert!( + edge.is_some(), + "Processor.new should resolve a call to the Processor class" + ); + assert_eq!(edge.unwrap().1, "EXTRACTED"); +} + +#[test] +fn reassignment_to_untyped_value_clears_type() { + // The 100%-confidence contract: a variable reassigned to anything other than + // a single `Constant.new` (here a plain method call) becomes ambiguous, so no + // type is carried and the member call is never resolved by type. + let tmp = tempdir().unwrap(); + let main = write( + tmp.path(), + "main.rb", + "def process_all(items)\n p = Processor.new\n p = items.first\n p.run(items)\nend\n", + ); + let result = extract_ruby(&main); + let rc = find_raw_call(&result, "run").unwrap(); + assert_eq!( + rc.receiver_type, None, + "reassign to an untyped value poisons the type" + ); +} diff --git a/crates/graphify-extract/tests/swift_member_calls.rs b/crates/graphify-extract/tests/swift_member_calls.rs index 4470f8d..dff0b60 100644 --- a/crates/graphify-extract/tests/swift_member_calls.rs +++ b/crates/graphify-extract/tests/swift_member_calls.rs @@ -98,7 +98,11 @@ fn swift_cross_file_member_calls_resolve() { } #[test] -fn swift_cross_file_member_calls_are_inferred_and_resolve_to_real_nodes() { +fn swift_cross_file_member_calls_have_correct_confidence_and_resolve() { + // Instance calls typed via local inference (vm.update(), self.svc.fetch()) are + // INFERRED; type-qualified static calls (SessionType.staticMethod(), + // Singleton.shared.method()) name the receiver type explicitly in source, so + // they are EXTRACTED (#1533). All must land on real definition nodes. let tmp = tempfile::tempdir().unwrap(); let files = issue_fixture(&tmp.path().join("src")); let res = extract(&files, Some(&tmp.path().join("cache"))); @@ -108,46 +112,71 @@ fn swift_cross_file_member_calls_are_inferred_and_resolve_to_real_nodes() { .iter() .filter_map(|n| n.get("id").and_then(|v| v.as_str())) .collect(); - let member_targets: HashSet<&str> = - [".update()", ".fetch()", ".staticMethod()", ".method()"].into(); - let mut seen: HashSet = HashSet::new(); - let mut inferred_calls = 0; + let inferred_targets: HashSet<&str> = [".update()", ".fetch()"].into(); + let extracted_targets: HashSet<&str> = [".staticMethod()", ".method()"].into(); + let mut seen_inferred: HashSet = HashSet::new(); + let mut seen_extracted: HashSet = HashSet::new(); for e in &res.edges { - let rel = e.get("relation").and_then(|v| v.as_str()).unwrap_or(""); - let conf = e.get("confidence").and_then(|v| v.as_str()).unwrap_or(""); - if rel == "calls" && conf == "INFERRED" { - inferred_calls += 1; + if e.get("relation").and_then(|v| v.as_str()) != Some("calls") { + continue; } let tgt = e.get("target").and_then(|v| v.as_str()).unwrap_or(""); let tgt_label = label_of(&res, tgt); - if rel == "calls" && member_targets.contains(tgt_label) { + let conf = e.get("confidence").and_then(|v| v.as_str()).unwrap_or(""); + let score = e + .get("confidence_score") + .and_then(serde_json::Value::as_f64); + let source_backed = res + .nodes + .iter() + .find(|n| n.get("id").and_then(|v| v.as_str()) == Some(tgt)) + .and_then(|n| n.get("source_file").and_then(|v| v.as_str())) + .is_some_and(|sf| !sf.is_empty()); + if inferred_targets.contains(tgt_label) { assert_eq!(conf, "INFERRED"); - assert_eq!( - e.get("confidence_score") - .and_then(serde_json::Value::as_f64), - Some(0.8) - ); - assert!(node_ids.contains(tgt), "member target not a node: {tgt}"); - let sf = res - .nodes - .iter() - .find(|n| n.get("id").and_then(|v| v.as_str()) == Some(tgt)) - .and_then(|n| n.get("source_file").and_then(|v| v.as_str())) - .unwrap_or(""); - assert!(!sf.is_empty(), "member target not source-backed: {tgt}"); - seen.insert(tgt_label.to_string()); + assert_eq!(score, Some(0.8)); + assert!(node_ids.contains(tgt) && source_backed, "unresolved: {tgt}"); + seen_inferred.insert(tgt_label.to_string()); + } else if extracted_targets.contains(tgt_label) { + assert_eq!(conf, "EXTRACTED"); + assert_eq!(score, Some(1.0)); + assert!(node_ids.contains(tgt) && source_backed, "unresolved: {tgt}"); + seen_extracted.insert(tgt_label.to_string()); } } - let want: HashSet = member_targets.iter().map(|s| (*s).to_string()).collect(); - assert_eq!(seen, want); - assert!( - inferred_calls >= 5, - "expected >=5 INFERRED calls, got {inferred_calls}" + assert_eq!( + seen_inferred, + inferred_targets + .iter() + .map(|s| (*s).to_string()) + .collect::>() + ); + assert_eq!( + seen_extracted, + extracted_targets + .iter() + .map(|s| (*s).to_string()) + .collect::>() ); - // Edges survive graph construction (targets are real nodes). + // Resolved member calls survive graph construction (targets are real nodes). + let surviving = res + .edges + .iter() + .filter(|e| { + e.get("relation").and_then(|v| v.as_str()) == Some("calls") + && matches!( + e.get("confidence").and_then(|v| v.as_str()), + Some("INFERRED" | "EXTRACTED") + ) + }) + .count(); + assert!( + surviving >= 5, + "expected >=5 resolved calls, got {surviving}" + ); let g = build_from_json(serde_json::to_value(&res).unwrap(), true, None).expect("build"); - for t in &member_targets { + for t in inferred_targets.iter().chain(extracted_targets.iter()) { let nid = res .nodes .iter() diff --git a/crates/graphify-hooks/src/platform/common/markdown.rs b/crates/graphify-hooks/src/platform/common/markdown.rs index 9704967..3b81f22 100644 --- a/crates/graphify-hooks/src/platform/common/markdown.rs +++ b/crates/graphify-hooks/src/platform/common/markdown.rs @@ -33,7 +33,7 @@ pub const AGENTS_MD_SECTION: &str = "\ This project has a knowledge graph at graphify-out/ with god nodes, community structure, and cross-file relationships. -When the user types `/graphify`, invoke the `skill` tool with `skill: \"graphify\"` before doing anything else. +When the user types `/graphify`, use the installed graphify skill or instructions before doing anything else. Rules: - For codebase questions, first run `graphify query \"\"` when graphify-out/graph.json exists. Use `graphify path \"\" \"\"` for relationships and `graphify explain \"\"` for focused concepts. These return a scoped subgraph, usually much smaller than GRAPH_REPORT.md or raw grep output. diff --git a/crates/graphify-hooks/src/platform/common/skills.rs b/crates/graphify-hooks/src/platform/common/skills.rs index 4f3af71..46572f1 100644 --- a/crates/graphify-hooks/src/platform/common/skills.rs +++ b/crates/graphify-hooks/src/platform/common/skills.rs @@ -45,8 +45,8 @@ pub(in crate::platform) const SKILL_WINDOWS_MD: &str = SKILL_MD; pub(in crate::platform) const SKILL_REGISTRATION: &str = "\n# graphify\n\ - **graphify** (`~/.claude/skills/graphify/SKILL.md`) \ - any input to knowledge graph. Trigger: `/graphify`\n\ -When the user types `/graphify`, invoke the Skill tool \ -with `skill: \"graphify\"` before doing anything else.\n"; +When the user types `/graphify`, use the installed graphify skill \ +or instructions before doing anything else.\n"; /// Skill registration text appended to `~/.codebuddy/CODEBUDDY.md` (#1136). /// @@ -56,5 +56,5 @@ with `skill: \"graphify\"` before doing anything else.\n"; pub(in crate::platform) const CODEBUDDY_REGISTRATION: &str = "\n# graphify\n\ - **graphify** (`~/.codebuddy/skills/graphify/SKILL.md`) \ - any input to knowledge graph. Trigger: `/graphify`\n\ -When the user types `/graphify`, invoke the Skill tool \ -with `skill: \"graphify\"` before doing anything else.\n"; +When the user types `/graphify`, use the installed graphify skill \ +or instructions before doing anything else.\n"; diff --git a/crates/graphify-hooks/tests/parity.rs b/crates/graphify-hooks/tests/parity.rs index 55da1f6..bf6c3b6 100644 --- a/crates/graphify-hooks/tests/parity.rs +++ b/crates/graphify-hooks/tests/parity.rs @@ -1736,6 +1736,30 @@ fn test_agents_section_does_not_skip_dirty_graph_output() { assert!(AGENTS_MD_SECTION.contains("not a reason to skip graphify")); } +#[test] +fn test_agents_section_uses_generic_graphify_instruction() { + // #1530: the AGENTS.md section must not name a host-specific `skill` tool. + assert!(!AGENTS_MD_SECTION.contains("`skill` tool")); + assert!(!AGENTS_MD_SECTION.contains("skill: \"graphify\"")); + assert!(AGENTS_MD_SECTION.contains("use the installed graphify skill")); +} + +#[test] +#[serial(home_env)] +fn test_skill_registration_uses_host_generic_instruction() { + // #1530: the CLAUDE.md skill registration must use the host-generic + // instruction, not the literal `skill: "graphify"` / "Skill tool". + let dir = tempfile::tempdir().expect("tempdir"); + install_skill_to(dir.path(), "claude"); + let content = dir.path().join(".claude/CLAUDE.md").read_to_string_unwrap(); + assert!( + content.contains("use the installed graphify skill or instructions"), + "{content:?}" + ); + assert!(!content.contains("skill: \"graphify\""), "{content:?}"); + assert!(!content.contains("Skill tool"), "{content:?}"); +} + // --------------------------------------------------------------------------- // install_upgrade parity (test_install_upgrade.py) // --------------------------------------------------------------------------- diff --git a/crates/graphify-llm/src/azure.rs b/crates/graphify-llm/src/azure.rs index 4df5e37..aa31c32 100644 --- a/crates/graphify-llm/src/azure.rs +++ b/crates/graphify-llm/src/azure.rs @@ -252,13 +252,15 @@ fn send_azure_request(api_key: &str, url: &str, body: &Value) -> Result) -> Result, body: &Value) -> Result u32 { default } +/// How many times a transient HTTP send (notably an HTTP 429 rate limit) is +/// retried before giving up. Reads `GRAPHIFY_MAX_RETRIES`; defaults to 6. `0` +/// disables retries; a negative or non-numeric value falls back to the default. +/// Mirrors Python `_resolve_max_retries` (#1523). +#[must_use] +pub fn resolve_max_retries() -> u32 { + let raw = std::env::var("GRAPHIFY_MAX_RETRIES").unwrap_or_default(); + let raw = raw.trim(); + if !raw.is_empty() + && let Ok(v) = raw.parse::() + { + return v; + } + 6 +} + +/// Run `send` (a `ureq` POST), retrying on a rate-limit (429) or transient (5xx) +/// status up to [`resolve_max_retries`] times with exponential backoff. Mirrors +/// the SDK `max_retries` behaviour graphify-py relies on (#1523): a rate-limited +/// request waits out the window instead of dropping the chunk. +/// +/// Divergence from graphify-py: the provider SDKs honour the `Retry-After` +/// header; `ureq`'s `StatusCode` error does not expose response headers, so we +/// back off exponentially instead. The observable behaviour — retrying a +/// rate-limited request rather than failing it — matches. +pub(crate) fn send_json_with_retry(mut send: F) -> Result +where + F: FnMut() -> Result, +{ + let max = resolve_max_retries(); + let mut attempt: u32 = 0; + loop { + match send() { + Ok(resp) => return Ok(resp), + Err(err) => { + // #1523 is specifically about rate limits: retry only HTTP 429, + // leaving 4xx/5xx to fail fast (5xx flows to the adaptive-retry / + // error paths, and exhausting backoff on a hard 5xx is pointless). + let rate_limited = matches!(&err, ureq::Error::StatusCode(code) if *code == 429); + if !rate_limited || attempt >= max { + return Err(err); + } + let base_ms = std::env::var("GRAPHIFY_RETRY_BASE_MS") + .ok() + .and_then(|s| s.trim().parse::().ok()) + .unwrap_or(500); + let backoff = std::time::Duration::from_millis( + base_ms.saturating_mul(1u64 << attempt.min(6)), + ); + std::thread::sleep(backoff); + attempt += 1; + } + } + } +} + /// Cap raw response at [`LLM_JSON_MAX_BYTES`] then parse the JSON. /// /// Returns an empty fragment `{nodes:[], edges:[], hyperedges:[]}` on failure. diff --git a/crates/graphify-llm/tests/openai_compat_helpers.rs b/crates/graphify-llm/tests/openai_compat_helpers.rs index 62ab65f..6e6dc6c 100644 --- a/crates/graphify-llm/tests/openai_compat_helpers.rs +++ b/crates/graphify-llm/tests/openai_compat_helpers.rs @@ -4,8 +4,8 @@ use graphify_llm::openai_compat::{ OpenAiRequest, api_timeout, call_openai_compat, derive_ollama_num_ctx, extraction_messages, - model_requires_default_temperature, plain_messages, resolve_max_tokens, resolve_temperature, - safe_parse_response, + model_requires_default_temperature, plain_messages, resolve_max_retries, resolve_max_tokens, + resolve_temperature, safe_parse_response, }; use serde_json::json; use std::time::Duration; @@ -314,3 +314,20 @@ fn resolve_temperature_env_var_invalid_falls_back() { assert_eq!(resolve_temperature(Some(0.0), "gpt-4.1-mini"), Some(0.0)); assert_eq!(resolve_temperature(Some(0.0), "o3-mini"), None); } + +// ── resolve_max_retries ────────────────────────────────────────────────────── + +#[test] +#[serial_test::serial(env)] +fn resolve_max_retries_default_and_env() { + // Default retry count is generous (so 429s are absorbed, #1523); env overrides. + let mut g = EnvGuard::new(); + g.remove("GRAPHIFY_MAX_RETRIES"); + assert!(resolve_max_retries() >= 5, "default should be generous"); + g.set("GRAPHIFY_MAX_RETRIES", "10"); + assert_eq!(resolve_max_retries(), 10); + g.set("GRAPHIFY_MAX_RETRIES", "0"); + assert_eq!(resolve_max_retries(), 0, "disable is allowed"); + g.set("GRAPHIFY_MAX_RETRIES", "bogus"); + assert!(resolve_max_retries() >= 5, "invalid -> default"); +} diff --git a/crates/graphify-llm/tests/per_backend_http.rs b/crates/graphify-llm/tests/per_backend_http.rs index 4d1a083..95d646d 100644 --- a/crates/graphify-llm/tests/per_backend_http.rs +++ b/crates/graphify-llm/tests/per_backend_http.rs @@ -99,6 +99,67 @@ fn openai_plain_via_mock() { assert_eq!(out, "plain answer"); } +#[test] +fn openai_retries_rate_limited_request() { + // A 429 must be retried (SDK max_retries parity, #1523): the first response is + // a rate limit, the retry succeeds, so the call resolves instead of dropping + // the chunk. Sequential mocks: 429 once, then 200. + let mut server = mockito::Server::new(); + let _rl = server + .mock("POST", "/chat/completions") + .with_status(429) + .with_body("rate limited") + .expect(1) + .create(); + let _ok = server + .mock("POST", "/chat/completions") + .with_status(200) + .with_body(json_body("{\"nodes\":[{\"id\":\"x\"}],\"edges\":[]}")) + .expect_at_least(1) + .create(); + + let mut g = allow_private(); + g.set("GRAPHIFY_OPENAI_BASE_URL", &server.url()); + g.set("GRAPHIFY_MAX_RETRIES", "3"); + g.set("GRAPHIFY_RETRY_BASE_MS", "0"); // no backoff sleep under test + + let resp = call_openai( + "key", + "gpt-test", + &[json!({"role":"user","content":"hi"})], + 128, + ) + .expect("429 should be retried and then resolve"); + assert_eq!(resp.nodes.len(), 1); +} + +#[test] +fn openai_gives_up_when_retries_disabled() { + // GRAPHIFY_MAX_RETRIES=0 disables retries: a 429 fails immediately. + let mut server = mockito::Server::new(); + let _rl = server + .mock("POST", "/chat/completions") + .with_status(429) + .with_body("rate limited") + .create(); + + let mut g = allow_private(); + g.set("GRAPHIFY_OPENAI_BASE_URL", &server.url()); + g.set("GRAPHIFY_MAX_RETRIES", "0"); + g.set("GRAPHIFY_RETRY_BASE_MS", "0"); + + assert!( + call_openai( + "key", + "gpt-test", + &[json!({"role":"user","content":"hi"})], + 128 + ) + .is_err(), + "with retries disabled, a 429 must fail" + ); +} + // ── gemini ───────────────────────────────────────────────────────────────── #[test] diff --git a/crates/graphify-watch/src/rebuild/pipeline_helpers.rs b/crates/graphify-watch/src/rebuild/pipeline_helpers.rs index 37fae27..5c21a04 100644 --- a/crates/graphify-watch/src/rebuild/pipeline_helpers.rs +++ b/crates/graphify-watch/src/rebuild/pipeline_helpers.rs @@ -430,6 +430,42 @@ pub(crate) fn merge_with_existing_graph( ) .collect(); + // An edge is OWNED by the file it was extracted from (its `source_file`). When + // that file is re-extracted, its prior edges must not be carried forward — the + // fresh extraction re-emits whichever ones still exist. Preserving by endpoint + // membership alone keeps a removed import's edge alive forever whenever both + // endpoint nodes survive (e.g. `a` no longer imports `b`, but both survive), + // producing phantom circular dependencies (#1521). So drop preserved edges + // whose source_file was re-extracted this run (or deleted). Unlike the + // node-level evict set, this MUST cover the full-rebuild case too: there every + // file is re-extracted but `evict_sources` only lists deleted files, so a + // removed import in a surviving file would otherwise never be pruned. + let mut edge_evict_sources = evict_sources.clone(); + for p in extract_targets { + for root in [project_root, watch_root] { + edge_evict_sources.insert(norm_source_file( + &p.to_string_lossy(), + Some(&root.to_string_lossy()), + )); + } + } + let edge_evicted = |e: &Value| -> bool { + if edge_evict_sources.is_empty() { + return false; + } + let Some(sf) = e.get("source_file").and_then(Value::as_str) else { + return false; + }; + if sf.is_empty() { + return false; + } + if edge_evict_sources.contains(sf) { + return true; + } + let norm = norm_source_file(sf, Some(&project_root_str)); + !norm.is_empty() && edge_evict_sources.contains(&norm) + }; + let preserved_edges: Vec = existing .get("links") .or_else(|| existing.get("edges")) @@ -439,7 +475,7 @@ pub(crate) fn merge_with_existing_graph( .filter(|e| { let src = e.get("source").and_then(Value::as_str).unwrap_or(""); let tgt = e.get("target").and_then(Value::as_str).unwrap_or(""); - all_ids.contains(src) && all_ids.contains(tgt) + all_ids.contains(src) && all_ids.contains(tgt) && !edge_evicted(e) }) .cloned() .collect() diff --git a/crates/graphify-watch/tests/rebuild_pipeline.rs b/crates/graphify-watch/tests/rebuild_pipeline.rs index 85f1688..70e3eff 100644 --- a/crates/graphify-watch/tests/rebuild_pipeline.rs +++ b/crates/graphify-watch/tests/rebuild_pipeline.rs @@ -608,3 +608,63 @@ fn no_cluster_incremental_reextract_does_not_duplicate_edges() { "re-extracting an unchanged file must not duplicate edges (#1317)" ); } + +#[test] +fn rebuild_code_prunes_a_removed_imports_edge() { + // #1521: when an import is deleted from a file, a full `update` must prune the + // edge it produced — preserving it (keyed only on endpoint membership) left a + // stale edge that drove phantom circular-dependency findings. + let tmp = tempfile::tempdir().expect("tempdir"); + let corpus = tmp.path(); + let pkg = corpus.join("pkg"); + fs::create_dir_all(&pkg).expect("mkdir pkg"); + fs::write(pkg.join("b.py"), "def helper():\n return 1\n").expect("write b.py"); + fs::write( + pkg.join("a.py"), + "from pkg.b import helper\ndef use():\n return helper()\n", + ) + .expect("write a.py"); + + let opts = RebuildOptions { + force: false, + no_cluster: true, + lock: LockPolicy::None, + }; + assert!(rebuild_code(corpus, None, opts).expect("first rebuild")); + + let graph_path = corpus.join("graphify-out").join("graph.json"); + let a_import_edges = |path: &Path| -> usize { + let v: serde_json::Value = + serde_json::from_slice(&fs::read(path).expect("read graph.json")).expect("parse"); + v.get("links") + .or_else(|| v.get("edges")) + .and_then(|e| e.as_array()) + .map_or(0, |edges| { + edges + .iter() + .filter(|e| { + matches!( + e.get("relation").and_then(|r| r.as_str()), + Some("imports" | "imports_from") + ) && e + .get("source_file") + .and_then(|s| s.as_str()) + .is_some_and(|s| s.ends_with("a.py")) + }) + .count() + }) + }; + assert!( + a_import_edges(&graph_path) > 0, + "expected an import edge from a.py initially" + ); + + // Remove the import, then run a full update (changed_paths = None). + fs::write(pkg.join("a.py"), "def use():\n return 1\n").expect("rewrite a.py"); + assert!(rebuild_code(corpus, None, opts).expect("second rebuild")); + assert_eq!( + a_import_edges(&graph_path), + 0, + "removed import's edge owned by a.py must be pruned" + ); +} diff --git a/src/cli/args.rs b/src/cli/args.rs index a8f5f1d..dcba381 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -220,7 +220,11 @@ pub(crate) enum Command { #[arg(long)] question: String, #[arg(long)] - answer: String, + answer: Option, + /// Read the answer from a file (alternative to `--answer`, for long or + /// multiline answers that are awkward to pass inline, #1502). + #[arg(long = "answer-file")] + answer_file: Option, #[arg(long = "type", default_value = "query")] query_type: String, #[arg(long, num_args = 0..)] diff --git a/src/cli/dispatch.rs b/src/cli/dispatch.rs index 347a914..35d5229 100644 --- a/src/cli/dispatch.rs +++ b/src/cli/dispatch.rs @@ -259,6 +259,7 @@ fn dispatch_save_result(cmd: Command) -> Result<()> { let Command::SaveResult { question, answer, + answer_file, query_type, nodes, memory_dir, @@ -268,6 +269,16 @@ fn dispatch_save_result(cmd: Command) -> Result<()> { else { unreachable!("dispatch_save_result invoked with wrong variant") }; + // `--answer-file` lets callers pass a long/multiline answer via a file instead + // of a fragile inline arg (Windows/PowerShell quoting), #1502. It wins over + // `--answer`; with neither, fail with a message naming both flags. + let answer = match answer_file { + Some(path) => std::fs::read_to_string(&path) + .map_err(|e| anyhow::anyhow!("--answer-file {}: {e}", path.display()))? + .trim() + .to_string(), + None => answer.ok_or_else(|| anyhow::anyhow!("--answer or --answer-file is required"))?, + }; cli::save_result::cmd_save_result( &question, &answer, diff --git a/src/cli/extract.rs b/src/cli/extract.rs index 8ed29b3..acd5d44 100644 --- a/src/cli/extract.rs +++ b/src/cli/extract.rs @@ -553,6 +553,12 @@ fn run_semantic_phase( } save_semantic_cache_safe(&sem_result, path); + // Prune orphaned semantic cache entries against the FULL live document set + // (sem_paths), NOT the incremental cache-miss subset, which would delete every + // unchanged doc's valid entry. The semantic cache is content-hash-keyed and + // unversioned, so it is never swept by the AST version-cleanup: every content + // change or file deletion leaves a permanent orphan otherwise (#1527). + prune_semantic_cache_safe(path, &sem_paths); merge_semantic_with_cache_and_ast(&mut sem_result, cache_split, extraction); let extraction_json = serde_json::json!({ "nodes": sem_result.nodes, @@ -581,6 +587,26 @@ fn save_semantic_cache_safe(sem_result: &graphify_llm::LlmResponse, path: &std:: } } +/// Best-effort prune of orphaned semantic cache entries (#1527). Builds the live +/// content-hash set from the semantic files that still exist on disk (a deleted +/// file is left out so its stale entry is swept), then sweeps everything else. A +/// prune failure only costs one re-extraction on a future run, never wrong output. +fn prune_semantic_cache_safe(root: &std::path::Path, sem_paths: &[String]) { + let live: std::collections::HashSet = sem_paths + .iter() + .filter_map(|p| { + let fp = std::path::Path::new(p); + fp.is_file() + .then(|| graphify_cache::file_hash(fp, root).ok()) + .flatten() + }) + .collect(); + let pruned = graphify_cache::prune_semantic_cache(root, &live); + if pruned > 0 { + eprintln!(" pruned {pruned} orphaned semantic cache entries"); + } +} + /// Prepend cached semantic entries to `sem_result`, then append AST nodes/edges. /// /// Semantic entries win any deduplication that happens inside `build_from_json`. diff --git a/tests/cli_commands.rs b/tests/cli_commands.rs index d02e7df..7bb5c08 100644 --- a/tests/cli_commands.rs +++ b/tests/cli_commands.rs @@ -1166,3 +1166,50 @@ fn save_result_rejects_bad_outcome() { .assert() .failure(); } + +#[test] +fn save_result_reads_answer_from_file() { + // #1502: --answer-file lets callers pass a long/multiline answer via a file + // instead of a fragile inline arg (Windows/PowerShell quoting). + let dir = tempfile::tempdir().unwrap(); + let ans = dir.path().join("answer.txt"); + fs::write(&ans, "line one\nline two with a \"quote\"\n").unwrap(); + cli() + .current_dir(dir.path()) + .env_remove("GRAPHIFY_OUT") + .args([ + "save-result", + "--question", + "how does auth work?", + "--answer-file", + ans.to_str().unwrap(), + "--outcome", + "useful", + ]) + .assert() + .success(); + let memory = dir.path().join("graphify-out").join("memory"); + let docs: Vec<_> = fs::read_dir(&memory) + .unwrap() + .filter_map(Result::ok) + .filter(|e| e.path().extension().and_then(|x| x.to_str()) == Some("md")) + .collect(); + assert!(!docs.is_empty(), "save-result wrote no memory doc"); + let body = fs::read_to_string(docs[0].path()).unwrap(); + assert!( + body.contains("line one") && body.contains("line two"), + "{body}" + ); +} + +#[test] +fn save_result_requires_answer_or_answer_file() { + // #1502: neither --answer nor --answer-file -> clean error, not a crash. + let dir = tempfile::tempdir().unwrap(); + cli() + .current_dir(dir.path()) + .args(["save-result", "--question", "q", "--outcome", "useful"]) + .assert() + .failure() + .stderr(contains("--answer")); +} From 93745b729c1846e334b18139e815b0faf8130d12 Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Mon, 29 Jun 2026 22:50:36 +0200 Subject: [PATCH 3/7] Address CodeRabbit review on the resync Apply the actionable findings from the first CodeRabbit pass. - `contained_in_package` canonicalises the nearest existing ancestor of the candidate so a symlink that crosses the package boundary is rejected, not just a lexical `..` escape (closer to Python's `Path.resolve()`). - correct the `send_json_with_retry` doc: only HTTP 429 is retried, 5xx fails fast. - assert the exact default retry count (6) instead of a lower bound. - mark the env-mutating 429 retry tests `#[serial(env)]`. - route aliased imports through `resolve_js_module_path` in the Svelte and JS import branches so a `.js` specifier backed by a `.ts` source hashes to the same node id; add a regression test. - select the file node by label in the non-salted id test so it is order-independent. - `USAGE.md`: `--answer-file` takes precedence when both answer inputs are given; at least one is required. By the will of the Machine God --- USAGE.md | 3 +- .../graphify-extract/src/extractors/svelte.rs | 6 +++- .../graphify-extract/src/generic/js_extra.rs | 6 +++- crates/graphify-extract/src/workspace.rs | 29 ++++++++++++++----- crates/graphify-extract/tests/id_collision.rs | 8 +++-- .../tests/js_workspace_resolution.rs | 29 +++++++++++++++++++ crates/graphify-llm/src/openai_compat.rs | 4 +-- .../tests/openai_compat_helpers.rs | 4 +-- crates/graphify-llm/tests/per_backend_http.rs | 2 ++ 9 files changed, 74 insertions(+), 17 deletions(-) diff --git a/USAGE.md b/USAGE.md index 45ec26b..05d85ea 100644 --- a/USAGE.md +++ b/USAGE.md @@ -324,7 +324,8 @@ Pass `--outcome useful|dead_end|corrected` (and `--correction ""` f a work-memory signal that `graphify reflect` later aggregates into `LESSONS.md`. An out-of-set `--outcome` is rejected. The answer can be passed inline with `--answer ""` or read from a file with `--answer-file `; the -latter avoids fragile shell quoting for long or multiline answers (#1502). Exactly one of the two is required. +latter avoids fragile shell quoting for long or multiline answers (#1502). At least one is required, and +`--answer-file` takes precedence when both are given. ```bash graphify save-result \ diff --git a/crates/graphify-extract/src/extractors/svelte.rs b/crates/graphify-extract/src/extractors/svelte.rs index 1f385ea..133052d 100644 --- a/crates/graphify-extract/src/extractors/svelte.rs +++ b/crates/graphify-extract/src/extractors/svelte.rs @@ -74,7 +74,11 @@ fn resolve_import_id(raw: &str, path: &Path) -> (String, String) { let aliases = load_tsconfig_aliases(path.parent().unwrap_or(path)); let resolved_alias = resolve_tsconfig_alias(raw, &aliases); if let Some(alias_path) = resolved_alias { - let stub = alias_path.to_string_lossy().into_owned(); + // Route the alias hit through `resolve_js_module_path` so a `.js` + // specifier backed by a `.ts` source resolves identically to the + // relative-import path before the id is hashed. + let resolved = resolve_js_module_path(&alias_path); + let stub = resolved.to_string_lossy().into_owned(); (make_id1(&stub), stub) } else { // External module: use last segment diff --git a/crates/graphify-extract/src/generic/js_extra.rs b/crates/graphify-extract/src/generic/js_extra.rs index 6b210b2..eac5d4b 100644 --- a/crates/graphify-extract/src/generic/js_extra.rs +++ b/crates/graphify-extract/src/generic/js_extra.rs @@ -459,7 +459,11 @@ pub fn resolve_js_import_target(raw: &str, str_path: &str) -> (String, Option Option { /// Guard against `exports` targets that escape the package directory (e.g. /// `"./evil": "../../../etc/passwd"`). `true` only when `package_dir/target` /// stays within `package_dir`. Mirrors Python `_contained_in_package` -/// (`Path.resolve().is_relative_to(...)`): symlinks are followed when the -/// candidate exists, with a lexical `..`-collapse fallback for a not-yet-created -/// target so an escaping symlink can't slip through. +/// (`Path.resolve().is_relative_to(...)`): a lexical `..`-collapse rejects the +/// obvious escape, then the nearest existing ancestor is canonicalised so a +/// symlink that crosses the package boundary can't slip through either. fn contained_in_package(target: &str, package_dir: &Path) -> bool { let Ok(pkg_canon) = package_dir.canonicalize() else { return false; }; - let candidate = pkg_canon.join(target); - let resolved = candidate - .canonicalize() - .unwrap_or_else(|_| crate::tsconfig::normpath(&candidate)); - resolved.starts_with(&pkg_canon) + let candidate = crate::tsconfig::normpath(&pkg_canon.join(target)); + if !candidate.starts_with(&pkg_canon) { + return false; + } + // Canonicalise the nearest existing ancestor to follow any symlink on the + // path; a not-yet-created tail keeps the lexical result above. + let mut probe = candidate.as_path(); + loop { + if std::fs::symlink_metadata(probe).is_ok() { + return probe + .canonicalize() + .is_ok_and(|resolved| resolved.starts_with(&pkg_canon)); + } + let Some(parent) = probe.parent() else { + return false; + }; + probe = parent; + } } /// Pick the most likely entry-point file for a workspace package. diff --git a/crates/graphify-extract/tests/id_collision.rs b/crates/graphify-extract/tests/id_collision.rs index 6495b24..b88cd68 100644 --- a/crates/graphify-extract/tests/id_collision.rs +++ b/crates/graphify-extract/tests/id_collision.rs @@ -74,9 +74,13 @@ fn non_colliding_path_id_is_not_salted() { let file_id = out .nodes .iter() - .find(|n| n.get("source_location").and_then(Value::as_str) == Some("L1")) + .find(|n| { + n.get("label") + .and_then(Value::as_str) + .is_some_and(|l| l.ends_with(".py")) + }) .and_then(|n| n.get("id").and_then(Value::as_str)) - .expect("file node with L1 source_location"); + .expect("file node"); assert_eq!(file_id, "src_auth_session"); assert_eq!( file_id, diff --git a/crates/graphify-extract/tests/js_workspace_resolution.rs b/crates/graphify-extract/tests/js_workspace_resolution.rs index 56a16c8..bc4e9da 100644 --- a/crates/graphify-extract/tests/js_workspace_resolution.rs +++ b/crates/graphify-extract/tests/js_workspace_resolution.rs @@ -586,3 +586,32 @@ fn workspace_subpath_export_default_consulted_last() -> TestResult { ); Ok(()) } + +#[test] +fn tsconfig_alias_js_specifier_resolves_to_ts_source() -> TestResult { + // An aliased `.js` specifier backed by a `.ts` source must hash to the same + // file node as the real `.ts` file — the `.js`->`.ts` fallback applies to + // alias hits too, matching relative imports. + let tmp = tempdir()?; + let root = tmp.path(); + write( + &root.join("tsconfig.json"), + r#"{"compilerOptions": {"baseUrl": ".", "paths": {"@/*": ["src/*"]}}}"#, + )?; + write(&root.join("src/foo.ts"), "export const x = 1\n")?; + write( + &root.join("src/app.ts"), + "import { x } from '@/foo.js'\nconsole.log(x)\n", + )?; + + let out = extract( + &[root.join("src/foo.ts"), root.join("src/app.ts")], + Some(root), + ); + assert!( + has_imports_from(&out, "src/app.ts", "src/foo.ts"), + "aliased `.js` specifier should resolve to the `.ts` source; edges: {:?}", + out.edges + ); + Ok(()) +} diff --git a/crates/graphify-llm/src/openai_compat.rs b/crates/graphify-llm/src/openai_compat.rs index 399a0ff..d2cfb5b 100644 --- a/crates/graphify-llm/src/openai_compat.rs +++ b/crates/graphify-llm/src/openai_compat.rs @@ -385,8 +385,8 @@ pub fn resolve_max_retries() -> u32 { 6 } -/// Run `send` (a `ureq` POST), retrying on a rate-limit (429) or transient (5xx) -/// status up to [`resolve_max_retries`] times with exponential backoff. Mirrors +/// Run `send` (a `ureq` POST), retrying on a rate-limited (429) response up to +/// [`resolve_max_retries`] times with exponential backoff. Mirrors /// the SDK `max_retries` behaviour graphify-py relies on (#1523): a rate-limited /// request waits out the window instead of dropping the chunk. /// diff --git a/crates/graphify-llm/tests/openai_compat_helpers.rs b/crates/graphify-llm/tests/openai_compat_helpers.rs index 6e6dc6c..4a9fb1a 100644 --- a/crates/graphify-llm/tests/openai_compat_helpers.rs +++ b/crates/graphify-llm/tests/openai_compat_helpers.rs @@ -323,11 +323,11 @@ fn resolve_max_retries_default_and_env() { // Default retry count is generous (so 429s are absorbed, #1523); env overrides. let mut g = EnvGuard::new(); g.remove("GRAPHIFY_MAX_RETRIES"); - assert!(resolve_max_retries() >= 5, "default should be generous"); + assert_eq!(resolve_max_retries(), 6, "default should be generous"); g.set("GRAPHIFY_MAX_RETRIES", "10"); assert_eq!(resolve_max_retries(), 10); g.set("GRAPHIFY_MAX_RETRIES", "0"); assert_eq!(resolve_max_retries(), 0, "disable is allowed"); g.set("GRAPHIFY_MAX_RETRIES", "bogus"); - assert!(resolve_max_retries() >= 5, "invalid -> default"); + assert_eq!(resolve_max_retries(), 6, "invalid -> default"); } diff --git a/crates/graphify-llm/tests/per_backend_http.rs b/crates/graphify-llm/tests/per_backend_http.rs index 95d646d..5b8c88b 100644 --- a/crates/graphify-llm/tests/per_backend_http.rs +++ b/crates/graphify-llm/tests/per_backend_http.rs @@ -100,6 +100,7 @@ fn openai_plain_via_mock() { } #[test] +#[serial_test::serial(env)] fn openai_retries_rate_limited_request() { // A 429 must be retried (SDK max_retries parity, #1523): the first response is // a rate limit, the retry succeeds, so the call resolves instead of dropping @@ -134,6 +135,7 @@ fn openai_retries_rate_limited_request() { } #[test] +#[serial_test::serial(env)] fn openai_gives_up_when_retries_disabled() { // GRAPHIFY_MAX_RETRIES=0 disables retries: a 429 fails immediately. let mut server = mockito::Server::new(); From ceb807b9e12cbed696ce9b35f8c86ade4a2cf655 Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Tue, 30 Jun 2026 07:39:20 +0200 Subject: [PATCH 4/7] Harden node-id disambiguation collision handling CodeRabbit flagged three latent bugs in the colliding-node-id pass. The `graphify-py` reference has the same gaps, but per AGENTS.md a bug in the reference is not a requirement, and each violates an observable contract in a collision corner case. The divergences are documented in code. - restrict the header-variant repoint to C-family importers. A non-C `imports_from` whose target merely collides with a C header id was silently rewritten to the header, mis-targeting the edge. - only repoint to a header when the colliding group holds exactly one header candidate; the reference picks the first in node order, which is arbitrary when two same-stem headers (`foo.h`/`foo.hpp`) collide. - guard a salted id against colliding with an id surviving outside its group, widening the salt until globally unique. `occupied` holds the surviving ids (non-ambiguous, plus ambiguous groups with an empty-key member that keeps the bare id), so the guard never over-hashes against an id that is itself about to be rewritten. Extract `rewrite_edge_endpoints` to keep the pass under the line limit; add regression tests for the non-C import and the cross-group salt. By the will of the Machine God --- crates/graphify-extract/src/postprocess.rs | 146 ++++++++++++++---- .../tests/parity_postprocess.rs | 57 +++++++ 2 files changed, 174 insertions(+), 29 deletions(-) diff --git a/crates/graphify-extract/src/postprocess.rs b/crates/graphify-extract/src/postprocess.rs index 6155bcf..9e5a5af 100644 --- a/crates/graphify-extract/src/postprocess.rs +++ b/crates/graphify-extract/src/postprocess.rs @@ -25,6 +25,15 @@ static NON_ALNUM: LazyLock = /// repointed to the header variant of the colliding id (#1475). const HEADER_SUFFIXES: [&str; 4] = ["h", "hpp", "hh", "hxx"]; +/// C-family source/header suffixes (without the dot). Only an importer whose own +/// file is C-family emits `#include` edges that should resolve to a header +/// variant; restricting the header remap to these importers stops a non-C +/// `imports_from` edge whose target merely collides with a header id from being +/// silently mis-pointed at the header (#1475). graphify-py omits this guard. +const C_FAMILY_SUFFIXES: [&str; 11] = [ + "c", "cc", "cpp", "cxx", "c++", "h", "hpp", "hh", "hxx", "m", "mm", +]; + /// First 6 hex chars of the SHA-1 of `s` — an injective-enough salt to split /// node ids whose naive disambiguator still collides (#1522). Matches Python's /// `hashlib.sha1(...).hexdigest()[:6]`. @@ -74,6 +83,7 @@ fn salt_collision_group( source_keys: &HashSet, nodes: &mut [Node], root: &Path, + occupied: &HashSet, remap: &mut HashMap<(String, String), String>, ) { let mut naive: HashMap = HashMap::new(); @@ -96,14 +106,27 @@ fn salt_collision_group( if sk.is_empty() { continue; } - let new_id = if needs_hash.contains(&sk) { + let naive_id = naive + .get(&sk) + .cloned() + .unwrap_or_else(|| make_id(&[&sk, old_id])); + // Divergence from graphify-py (#1522): the reference only de-dupes within + // the group. Hash when the naive id collides in-group OR with an id + // surviving OUTSIDE this group (a salted `src_a_foo` can clash with a real + // node already named that). `occupied` holds surviving non-ambiguous ids, + // so this never over-hashes against an ambiguous id about to be rewritten. + let mut new_id = if needs_hash.contains(&sk) || occupied.contains(&naive_id) { make_id(&[&sk, old_id, &sha1_hex6(&sk)]) } else { - naive - .get(&sk) - .cloned() - .unwrap_or_else(|| make_id(&[&sk, old_id])) + naive_id }; + // If even the hashed candidate is occupied, widen with a numeric suffix + // until the id is globally unique (terminates: `occupied` is finite). + let mut bump = 1u32; + while occupied.contains(&new_id) { + new_id = make_id(&[&sk, old_id, &sha1_hex6(&sk), &bump.to_string()]); + bump += 1; + } remap.insert((old_id.to_string(), sk), new_id.clone()); if new_id != *old_id { nodes[idx].id = new_id; @@ -112,8 +135,12 @@ fn salt_collision_group( } /// Build `old_id -> header-variant new_id` for colliding ids whose group includes -/// a header file (`.h`/`.hpp`/…), so a quoted-include import edge dangling on the -/// salted-away bare id is repointed to the header variant (#1475). +/// exactly one header file (`.h`/`.hpp`/…), so a quoted-include import edge +/// dangling on the salted-away bare id is repointed to the header variant +/// (#1475). Divergence from graphify-py: the reference picks the *first* header +/// in node order, which is arbitrary when a group holds two same-stem headers +/// (e.g. `foo.h` and `foo.hpp`); we only remap when the header target is +/// unambiguous and otherwise leave the edge to the normal per-source-file remap. fn build_header_remaps( ambiguous_ids: &HashSet, by_id: &HashMap>, @@ -126,6 +153,7 @@ fn build_header_remaps( let Some(group) = by_id.get(old_id) else { continue; }; + let mut header_ids: HashSet = HashSet::new(); for &idx in group { let sk = node_disambiguation_source_key(&nodes[idx], root); let is_header = Path::new(&sk) @@ -136,10 +164,16 @@ fn build_header_remaps( && is_header && let Some(new_id) = remap.get(&(old_id.clone(), sk)) { - header_remaps.insert(old_id.clone(), new_id.clone()); - break; + header_ids.insert(new_id.clone()); } } + // Only remap when exactly one header variant exists; multiple headers + // make the import target ambiguous, so we leave it untouched. + if header_ids.len() == 1 + && let Some(new_id) = header_ids.into_iter().next() + { + header_remaps.insert(old_id.clone(), new_id); + } } header_remaps } @@ -179,20 +213,53 @@ pub fn disambiguate_colliding_node_ids( } } - let mut remap: HashMap<(String, String), String> = HashMap::new(); let mut ambiguous_ids: HashSet = HashSet::new(); for (old_id, group) in &by_id { let source_keys: HashSet = group .iter() .map(|&idx| node_disambiguation_source_key(&nodes[idx], root)) .collect(); - if group.len() < 2 || source_keys.len() < 2 { - continue; + if group.len() >= 2 && source_keys.len() >= 2 { + ambiguous_ids.insert(old_id.clone()); } - ambiguous_ids.insert(old_id.clone()); - salt_collision_group(old_id, group, &source_keys, nodes, root, &mut remap); } + // Ids that survive disambiguation: a salted id must not collide with one of + // these. A non-ambiguous id always survives; an ambiguous id survives only + // when one of its nodes has an empty disambiguation source key, since + // `salt_collision_group` skips those and leaves the bare id intact. Built + // before salting, so the guard never targets an ambiguous id that is itself + // fully rewritten (which would cause needless over-hashing). + let occupied: HashSet = by_id + .iter() + .filter(|(id, group)| { + !ambiguous_ids.contains(*id) + || group + .iter() + .any(|&idx| node_disambiguation_source_key(&nodes[idx], root).is_empty()) + }) + .map(|(id, _)| id.clone()) + .collect(); + + let mut remap: HashMap<(String, String), String> = HashMap::new(); + for old_id in &ambiguous_ids { + let Some(group) = by_id.get(old_id) else { + continue; + }; + let source_keys: HashSet = group + .iter() + .map(|&idx| node_disambiguation_source_key(&nodes[idx], root)) + .collect(); + salt_collision_group( + old_id, + group, + &source_keys, + nodes, + root, + &occupied, + &mut remap, + ); + } if remap.is_empty() { return; } @@ -226,6 +293,29 @@ pub fn disambiguate_colliding_node_ids( let header_remaps = build_header_remaps(&ambiguous_ids, &by_id, nodes, root, &remap); + rewrite_edge_endpoints(edges, &remap, &unambiguous_remaps, &header_remaps, root); + + for call in raw_calls.iter_mut() { + let call_source_key = source_key(&call.source_file, root); + let caller_tuple = (call.caller_nid.clone(), call_source_key); + if let Some(new_id) = remap.get(&caller_tuple) { + call.caller_nid.clone_from(new_id); + } else if let Some(new_id) = unambiguous_remaps.get(&call.caller_nid) { + call.caller_nid.clone_from(new_id); + } + } +} + +/// Rewrite edge endpoints onto disambiguated node ids. Source endpoints take the +/// per-source-file salt remap then the single-candidate remap; target endpoints +/// additionally resolve a C-family `#include` to the header variant first (#1475). +fn rewrite_edge_endpoints( + edges: &mut [Edge], + remap: &HashMap<(String, String), String>, + unambiguous_remaps: &HashMap, + header_remaps: &HashMap, + root: &Path, +) { for edge in edges.iter_mut() { let edge_source_key = source_key(&edge.source_file, root); let source_key_tuple = (edge.source.clone(), edge_source_key.clone()); @@ -235,11 +325,19 @@ pub fn disambiguate_colliding_node_ids( } else if let Some(new_id) = unambiguous_remaps.get(&edge.source) { edge.source.clone_from(new_id); } - // imports/imports_from always target a header file, so they must resolve - // to the header variant BEFORE the same-source-file salt is considered. - // Keying the import target by the importer's own source file mis-points a - // `.m` importing its own `.h` back at itself (self-loop) (#1475). - if matches!(edge.relation.as_str(), "imports" | "imports_from") + // A C-family `#include "foo.h"` whose bare id was salted away resolves to + // the header variant BEFORE the same-source-file salt is considered, so a + // `.m` including its own `.h` points at the header, not back at itself. + // Restrict to C-family importers: a non-C `imports_from` whose target + // merely collides with a header id must NOT be rewritten to the header + // (#1475). graphify-py applies this to every imports/imports_from edge and + // can mis-target non-C imports — fixed here per the parity-bug rule. + let importer_is_c_family = Path::new(&edge.source_file) + .extension() + .and_then(|e| e.to_str()) + .is_some_and(|e| C_FAMILY_SUFFIXES.contains(&e.to_lowercase().as_str())); + if importer_is_c_family + && matches!(edge.relation.as_str(), "imports" | "imports_from") && let Some(new_id) = header_remaps.get(&edge.target) { edge.target.clone_from(new_id); @@ -249,16 +347,6 @@ pub fn disambiguate_colliding_node_ids( edge.target.clone_from(new_id); } } - - for call in raw_calls.iter_mut() { - let call_source_key = source_key(&call.source_file, root); - let caller_tuple = (call.caller_nid.clone(), call_source_key); - if let Some(new_id) = remap.get(&caller_tuple) { - call.caller_nid.clone_from(new_id); - } else if let Some(new_id) = unambiguous_remaps.get(&call.caller_nid) { - call.caller_nid.clone_from(new_id); - } - } } /// Map any unresolved no-source-file stub node to a unique real diff --git a/crates/graphify-extract/tests/parity_postprocess.rs b/crates/graphify-extract/tests/parity_postprocess.rs index e8ae02a..b035fd3 100644 --- a/crates/graphify-extract/tests/parity_postprocess.rs +++ b/crates/graphify-extract/tests/parity_postprocess.rs @@ -144,3 +144,60 @@ fn rewire_does_not_rewire_to_dotted_label() { rewire_unique_stub_nodes(&mut nodes, &mut edges); assert!(nodes.iter().any(|n| n.id == "stub")); } + +#[test] +fn header_remap_skips_non_c_family_importers() { + // #1475 parity-bug fix: a header-variant repoint applies only to a C-family + // importer's `#include`. A Python `imports_from` whose target id merely + // collides with a C header must NOT be silently rewritten to the header. + let mut nodes = vec![ + n("foo", "foo", "src/foo.py"), // python module + n("foo", "foo", "include/foo.h"), // C header, same bare id + ]; + let mut edges = vec![ + e("consumer", "foo", "src/consumer.py", "imports_from"), // non-C importer + e("caller", "foo", "lib/util.c", "imports_from"), // C importer + ]; + let mut raw_calls: Vec = Vec::new(); + disambiguate_colliding_node_ids(&mut nodes, &mut edges, &mut raw_calls, Path::new(".")); + let header_id = nodes + .iter() + .find(|nd| nd.source_file == "include/foo.h") + .map(|nd| nd.id.clone()) + .expect("header node present"); + assert_ne!( + header_id, "foo", + "header should be salted away from the bare id" + ); + assert_eq!( + edges[1].target, header_id, + "a C `#include` should resolve to the header variant" + ); + assert_ne!( + edges[0].target, header_id, + "a non-C import must not be repointed at the header variant" + ); +} + +#[test] +fn salted_id_does_not_collide_with_existing_node() { + // #1522 hardening: when a salted id would equal an id already occupied + // outside the collision group, it must be disambiguated further so no two + // nodes share an id (graphify-py only de-dupes within the group). + let naive = graphify_extract::make_id(&["src/a/foo.py", "foo"]); + let mut nodes = vec![ + n("foo", "foo", "src/a/foo.py"), + n("foo", "foo", "src/b/foo.py"), + n(&naive, "other", "src/c/other.py"), // already occupies the naive salt + ]; + let mut edges: Vec = Vec::new(); + let mut raw_calls: Vec = Vec::new(); + disambiguate_colliding_node_ids(&mut nodes, &mut edges, &mut raw_calls, Path::new(".")); + let ids: Vec<&str> = nodes.iter().map(|nd| nd.id.as_str()).collect(); + let unique: std::collections::HashSet<&str> = ids.iter().copied().collect(); + assert_eq!( + unique.len(), + ids.len(), + "all node ids must be distinct: {ids:?}" + ); +} From 92ea34c9cf4831b1af2113382caa5e6d242b2b17 Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Tue, 30 Jun 2026 10:26:31 +0200 Subject: [PATCH 5/7] Address further CodeRabbit review findings Resolve the second review round; divergences from `graphify-py` are documented in code comments (where the reviewer reads them). Resolution accuracy: - Ruby: index class-definition nodes from `contains` (not only `method` edges) so a method-less class still resolves `Class.new`; defer Ruby member calls before the same-file bare-name lookup so a typed `p.run` resolves by receiver type instead of being swallowed by a local `run`; skip nested `class`/`module` scopes in the local-binding walk. - Swift: a receiver bound as a local resolves via its inferred type and is INFERRED even when upper-cased, so an upper-cased local is no longer mis-marked as a type-qualified EXTRACTED call. Id disambiguation and resolution: - postprocess: track ids minted within a pass so a later collision group can't reuse an earlier group's salted id, while still collapsing same-file same-id nodes onto one remap entry. - tsconfig: match the longest alias prefix so `@/feature/*` wins over a broad `@/*`. - workspace: resolve a bare conditional-`exports` object (no `.` key) as the package entry, and reject `exports` targets that escape the package directory on the no-subpath path too. CLI / cache: - `--answer-file` content is preserved exactly (no trim), matching inline `--answer`. - the semantic-cache prune resolves relative paths against `root`, as the cache lookup does, so a valid entry isn't pruned on relative input. Tests and docs: - cover the negative `GRAPHIFY_MAX_RETRIES` fallback and assert the no-retry mock is hit exactly once; assert the full save-result requirement message; document `GRAPHIFY_RETRY_BASE_MS`. - regression tests for the method-less Ruby class and the cross-group salt collision; `#[must_use]` on `default_resolvers`; new tests use `?`-returning signatures. By the will of the Machine God --- USAGE.md | 1 + crates/graphify-export/tests/parity.rs | 11 +- .../src/extractors/multi/resolvers.rs | 1 + .../src/extractors/multi/ruby.rs | 26 +++- .../src/extractors/multi/swift.rs | 24 ++-- crates/graphify-extract/src/generic/calls.rs | 50 ++++--- crates/graphify-extract/src/generic/ruby.rs | 12 +- crates/graphify-extract/src/postprocess.rs | 76 +++++++---- crates/graphify-extract/src/tsconfig.rs | 38 +++--- crates/graphify-extract/src/workspace.rs | 26 +++- .../tests/java_type_resolution.rs | 5 +- .../tests/parity_postprocess.rs | 24 ++++ .../graphify-extract/tests/ruby_resolution.rs | 127 +++++++++++------- .../tests/openai_compat_helpers.rs | 2 + crates/graphify-llm/tests/per_backend_http.rs | 4 +- src/cli/dispatch.rs | 9 +- src/cli/extract.rs | 11 +- tests/cli_commands.rs | 2 +- 18 files changed, 297 insertions(+), 152 deletions(-) diff --git a/USAGE.md b/USAGE.md index 05d85ea..55c188b 100644 --- a/USAGE.md +++ b/USAGE.md @@ -709,6 +709,7 @@ completes the feature.) | `OLLAMA_BASE_URL` | Ollama endpoint (default `http://localhost:11434/v1`); a link-local/cloud-metadata host is refused, a general non-loopback host warns. | | `GRAPHIFY_API_TIMEOUT` | LLM HTTP request timeout in seconds (default 600); bounds a runaway connection during semantic extraction. | | `GRAPHIFY_MAX_RETRIES` | Times a rate-limited (HTTP 429) LLM request is retried before its chunk is dropped (default 6); `0` disables retries (#1523). | +| `GRAPHIFY_RETRY_BASE_MS` | Base backoff delay in milliseconds for the 429 retry path; the wait grows exponentially per attempt (default 500). `0` disables the sleep (#1523). | ### LLM backends diff --git a/crates/graphify-export/tests/parity.rs b/crates/graphify-export/tests/parity.rs index 9129e88..d5ae3da 100644 --- a/crates/graphify-export/tests/parity.rs +++ b/crates/graphify-export/tests/parity.rs @@ -204,7 +204,7 @@ fn test_to_graphml_has_community_attribute() { } #[test] -fn test_to_graphml_tolerates_none_attribute_values() { +fn test_to_graphml_tolerates_none_attribute_values() -> Result<(), Box> { // A null attribute value must coerce to "" so a node/edge with a null field // still exports (no crash). graphify-py needs this because nx.write_graphml // raises ValueError on None (#1502); the hand-written Rust GraphML already @@ -213,7 +213,7 @@ fn test_to_graphml_tolerates_none_attribute_values() { let communities = make_communities(); // Inject a null-valued attribute on one node... let (nid, mut nattrs) = { - let (id, attrs) = g.nodes().next().expect("graph has at least one node"); + let (id, attrs) = g.nodes().next().ok_or("graph has at least one node")?; (id.clone(), attrs.clone()) }; nattrs.insert("nullable_field".to_string(), Value::Null); @@ -228,11 +228,12 @@ fn test_to_graphml_tolerates_none_attribute_values() { g.add_edge(&src, &tgt, eattrs); } - let tmp = tempdir().expect("tempdir"); + let tmp = tempdir()?; let out = tmp.path().join("graph.graphml"); - to_graphml(&g, &communities, &out).expect("to_graphml must not fail on null attrs"); - let content = std::fs::read_to_string(&out).expect("read graphml"); + to_graphml(&g, &communities, &out)?; + let content = std::fs::read_to_string(&out)?; assert!(content.contains(" [LanguageResolver; 3] { [ LanguageResolver { diff --git a/crates/graphify-extract/src/extractors/multi/ruby.rs b/crates/graphify-extract/src/extractors/multi/ruby.rs index 7173320..f1b05e5 100644 --- a/crates/graphify-extract/src/extractors/multi/ruby.rs +++ b/crates/graphify-extract/src/extractors/multi/ruby.rs @@ -16,6 +16,7 @@ use std::collections::{HashMap, HashSet}; +use super::java::is_type_like_definition; use crate::types::{Edge, Node, RawCall}; /// Normalise a class/method label to a comparison key (drop punctuation, fold). @@ -82,9 +83,32 @@ pub(super) fn resolve_ruby_member_calls( ) { let node_by_id: HashMap<&str, &Node> = all_nodes.iter().map(|n| (n.id.as_str(), n)).collect(); - // class label key -> [class node ids]; (class_node_id, method_key) -> method id. + // Index class-definition nodes independently of method ownership: a Ruby + // class is a `contains` target whose label is a Constant (upper-cased) + // type-like definition. This lets a method-less class (`class Config; end`) + // still resolve `Config.new`. Divergence from graphify-py, which builds the + // class index solely from `method` edges (extract.py:9621), so a class with + // no methods is invisible there. let mut class_def_nids: HashMap> = HashMap::new(); let mut method_index: HashMap<(String, String), String> = HashMap::new(); + let contained: HashSet<&str> = all_edges + .iter() + .filter(|e| e.relation == "contains") + .map(|e| e.target.as_str()) + .collect(); + for n in all_nodes { + if contained.contains(n.id.as_str()) + && is_type_like_definition(n) + && n.label.chars().next().is_some_and(char::is_uppercase) + { + class_def_nids + .entry(key(&n.label)) + .or_default() + .push(n.id.clone()); + } + } + // (class_node_id, method_key) -> method id, from `method` edges. The edge + // source also confirms a class node (belt-and-braces with the index above). for e in all_edges.iter() { if e.relation != "method" { continue; diff --git a/crates/graphify-extract/src/extractors/multi/swift.rs b/crates/graphify-extract/src/extractors/multi/swift.rs index 2e0343f..49fbbf8 100644 --- a/crates/graphify-extract/src/extractors/multi/swift.rs +++ b/crates/graphify-extract/src/extractors/multi/swift.rs @@ -171,17 +171,21 @@ pub(super) fn resolve_swift_member_calls( let Some(receiver) = rc.receiver.as_deref() else { continue; }; - // An upper-cased receiver is itself a type (`Type.staticMethod()`, - // `Singleton.shared.x()`); otherwise look it up in the declaring file's - // local type table. - let type_qualified = receiver.chars().next().is_some_and(char::is_uppercase); - let type_name = if type_qualified { - receiver.to_string() - } else if let Some(t) = type_table_by_file + // A receiver bound as a local (property/parameter) in this file resolves + // via its inferred type and is INFERRED — even when the local's name is + // upper-cased, which would otherwise look type-qualified. Only a receiver + // with NO local binding is treated as a bare type name + // (`Type.staticMethod()`), which is EXTRACTED because the type is named + // explicitly in source (#1533). Divergence from graphify-py + // (extract.py:9553), which keys solely on receiver casing and so mis-marks + // an upper-cased local as EXTRACTED. + let local_type = type_table_by_file .get(&rc.source_file) - .and_then(|tbl| tbl.get(receiver)) - { - t.clone() + .and_then(|tbl| tbl.get(receiver)); + let (type_name, type_qualified) = if let Some(t) = local_type { + (t.clone(), false) + } else if receiver.chars().next().is_some_and(char::is_uppercase) { + (receiver.to_string(), true) } else { continue; }; diff --git a/crates/graphify-extract/src/generic/calls.rs b/crates/graphify-extract/src/generic/calls.rs index 8b420ec..5bae4b5 100644 --- a/crates/graphify-extract/src/generic/calls.rs +++ b/crates/graphify-extract/src/generic/calls.rs @@ -91,17 +91,35 @@ pub(super) fn walk_calls( // real local symbol is a genuine call and must be kept. Only drop // built-ins when they DON'T resolve, so they can't become cross-file // god-nodes via the raw-call pass (#726). - // A capitalized-receiver Python member call (`ClassName.method()`) - // defers to receiver-based cross-file resolution: the bare method - // name can collide with an in-file node — even the calling method - // itself — which would match `tgt == caller` and silently drop the - // call. `resolve_python_member_calls` resolves it via the receiver - // (#1446). Gated to Python so Swift's own resolver is unaffected. + // Ruby: the receiver's inferred type from the method's local + // `var = Const.new` bindings, when unambiguously known (#1499). + // Computed up-front so it can also gate member-call deferral below. + let receiver_type = if ctx.config.lang_id == LangId::Ruby { + receiver.as_deref().and_then(|r| { + ctx.ruby_var_types + .get(caller_nid) + .and_then(|m| m.get(r).cloned()) + .flatten() + }) + } else { + None + }; + let receiver_upper = receiver + .as_deref() + .is_some_and(|r| r.chars().next().is_some_and(char::is_uppercase)); + // Defer to receiver-based cross-file resolution rather than a bare + // same-file `label_to_nid` match (which can collide with an in-file + // symbol — even the caller — and drop or mis-link the call): + // * Python `ClassName.method()` (upper-cased receiver), #1446; + // * Ruby `Const.new` / `Const.method()` (upper-cased receiver), and a + // typed instance call `var.method()` whose `var` has a known type + // (#1499). graphify-py only defers upper-cased receivers + // (extract.py:3899); deferring the typed-instance case too keeps a + // `p.run` from being swallowed by a same-file `run`. let defer_member = is_member_call - && ctx.config.lang_id == LangId::Python - && receiver - .as_deref() - .is_some_and(|r| r.chars().next().is_some_and(char::is_uppercase)); + && ((ctx.config.lang_id == LangId::Python && receiver_upper) + || (ctx.config.lang_id == LangId::Ruby + && (receiver_upper || receiver_type.is_some()))); let tgt_nid = if defer_member { None } else { @@ -127,18 +145,6 @@ pub(super) fn walk_calls( } } } else if !crate::builtins::is_language_builtin_global(&callee) { - // Ruby: attach the receiver's inferred type from the method's - // local `var = Const.new` bindings, when unambiguously known (#1499). - let receiver_type = if ctx.config.lang_id == LangId::Ruby { - receiver.as_deref().and_then(|r| { - ctx.ruby_var_types - .get(caller_nid) - .and_then(|m| m.get(r).cloned()) - .flatten() - }) - } else { - None - }; ctx.raw_calls.push(RawCall { caller_nid: caller_nid.to_string(), callee: callee.clone(), diff --git a/crates/graphify-extract/src/generic/ruby.rs b/crates/graphify-extract/src/generic/ruby.rs index fc9e256..78d49da 100644 --- a/crates/graphify-extract/src/generic/ruby.rs +++ b/crates/graphify-extract/src/generic/ruby.rs @@ -54,8 +54,16 @@ fn visit(node: Node<'_>, source: &[u8], bindings: &mut HashMap, nodes: &mut [Node], root: &Path, - occupied: &HashSet, + taken: &mut HashSet, remap: &mut HashMap<(String, String), String>, ) { let mut naive: HashMap = HashMap::new(); @@ -106,27 +106,39 @@ fn salt_collision_group( if sk.is_empty() { continue; } - let naive_id = naive - .get(&sk) - .cloned() - .unwrap_or_else(|| make_id(&[&sk, old_id])); - // Divergence from graphify-py (#1522): the reference only de-dupes within - // the group. Hash when the naive id collides in-group OR with an id - // surviving OUTSIDE this group (a salted `src_a_foo` can clash with a real - // node already named that). `occupied` holds surviving non-ambiguous ids, - // so this never over-hashes against an ambiguous id about to be rewritten. - let mut new_id = if needs_hash.contains(&sk) || occupied.contains(&naive_id) { - make_id(&[&sk, old_id, &sha1_hex6(&sk)]) + // Same-file same-id nodes share a `(old_id, sk)` key and must collapse to + // one disambiguated id; if a prior node in this group already minted it, + // reuse it rather than minting (and bumping) a second id, which would + // split them and corrupt the remap. + let new_id = if let Some(existing) = remap.get(&(old_id.to_string(), sk.clone())) { + existing.clone() } else { - naive_id + let naive_id = naive + .get(&sk) + .cloned() + .unwrap_or_else(|| make_id(&[&sk, old_id])); + // Divergence from graphify-py (#1522): the reference only de-dupes + // within the group. Hash when the naive id collides in-group or with + // an id already claimed — a surviving non-ambiguous id (a salted + // `src_a_foo` can clash with a real node already named that) or one + // minted earlier in this pass. `taken` is seeded with surviving ids + // (never an ambiguous id about to be rewritten), so this never + // over-hashes. + let mut candidate = if needs_hash.contains(&sk) || taken.contains(&naive_id) { + make_id(&[&sk, old_id, &sha1_hex6(&sk)]) + } else { + naive_id + }; + // If the hashed candidate is also taken, widen with a numeric suffix + // until globally unique (terminates: `taken` is finite). + let mut bump = 1u32; + while taken.contains(&candidate) { + candidate = make_id(&[&sk, old_id, &sha1_hex6(&sk), &bump.to_string()]); + bump += 1; + } + taken.insert(candidate.clone()); + candidate }; - // If even the hashed candidate is occupied, widen with a numeric suffix - // until the id is globally unique (terminates: `occupied` is finite). - let mut bump = 1u32; - while occupied.contains(&new_id) { - new_id = make_id(&[&sk, old_id, &sha1_hex6(&sk), &bump.to_string()]); - bump += 1; - } remap.insert((old_id.to_string(), sk), new_id.clone()); if new_id != *old_id { nodes[idx].id = new_id; @@ -224,13 +236,15 @@ pub fn disambiguate_colliding_node_ids( } } - // Ids that survive disambiguation: a salted id must not collide with one of - // these. A non-ambiguous id always survives; an ambiguous id survives only - // when one of its nodes has an empty disambiguation source key, since - // `salt_collision_group` skips those and leaves the bare id intact. Built - // before salting, so the guard never targets an ambiguous id that is itself - // fully rewritten (which would cause needless over-hashing). - let occupied: HashSet = by_id + // Ids already claimed, which a salted id must avoid: every surviving id plus + // every id minted during this pass. A non-ambiguous id always survives; an + // ambiguous id survives only when one of its nodes has an empty disambiguation + // source key (`salt_collision_group` skips those, leaving the bare id intact). + // Seeded before salting, so it never holds an ambiguous id about to be + // rewritten (which would needlessly over-hash); `salt_collision_group` adds + // each minted id so a later group can't reuse an earlier group's salted form + // (possible when two old ids normalise to the same salted id). + let mut taken: HashSet = by_id .iter() .filter(|(id, group)| { !ambiguous_ids.contains(*id) @@ -242,7 +256,11 @@ pub fn disambiguate_colliding_node_ids( .collect(); let mut remap: HashMap<(String, String), String> = HashMap::new(); - for old_id in &ambiguous_ids { + // Iterate in sorted order so the `taken`-set resolution is deterministic + // regardless of `ambiguous_ids` hash order. + let mut ambiguous_sorted: Vec<&String> = ambiguous_ids.iter().collect(); + ambiguous_sorted.sort(); + for old_id in ambiguous_sorted { let Some(group) = by_id.get(old_id) else { continue; }; @@ -256,7 +274,7 @@ pub fn disambiguate_colliding_node_ids( &source_keys, nodes, root, - &occupied, + &mut taken, &mut remap, ); } diff --git a/crates/graphify-extract/src/tsconfig.rs b/crates/graphify-extract/src/tsconfig.rs index 1195f89..6aa44b9 100644 --- a/crates/graphify-extract/src/tsconfig.rs +++ b/crates/graphify-extract/src/tsconfig.rs @@ -230,24 +230,32 @@ pub fn load_tsconfig_aliases(start_dir: &Path) -> AliasMap { /// Mirrors Python `_resolve_tsconfig_alias`. #[must_use] pub fn resolve_tsconfig_alias(raw: &str, aliases: &AliasMap) -> Option { - for (alias_prefix, alias_bases) in aliases { - if raw == alias_prefix || raw.starts_with(&format!("{alias_prefix}/")) { - let rest = raw[alias_prefix.len()..].trim_start_matches('/'); - let mut first: Option = None; - for base in alias_bases { - let cand = normpath(&Path::new(base).join(rest)); - let resolved = resolve_js_module_path(&cand); - if resolved.is_file() { - return Some(resolved); - } - if first.is_none() { - first = Some(cand); - } + // Collect every alias prefix that matches `raw`, longest first: tsc resolves + // the most specific (longest) prefix, so `@/feature/*` wins over `@/*` for + // `@/feature/x`. Divergence from graphify-py (extract.py:291), which returns + // the first match in declaration order and so lets a broad prefix shadow a + // more specific one. The first-candidate fallback is kept, but taken from the + // longest matching prefix. + let mut matched: Vec<(&String, &Vec)> = aliases + .iter() + .filter(|(prefix, _)| raw == prefix.as_str() || raw.starts_with(&format!("{prefix}/"))) + .collect(); + matched.sort_by_key(|m| std::cmp::Reverse(m.0.len())); + let mut first: Option = None; + for (alias_prefix, alias_bases) in matched { + let rest = raw[alias_prefix.len()..].trim_start_matches('/'); + for base in alias_bases { + let cand = normpath(&Path::new(base).join(rest)); + let resolved = resolve_js_module_path(&cand); + if resolved.is_file() { + return Some(resolved); + } + if first.is_none() { + first = Some(cand); } - return first; } } - None + first } /// Resolve a JS/TS-style import specifier path to an actual file on disk. diff --git a/crates/graphify-extract/src/workspace.rs b/crates/graphify-extract/src/workspace.rs index 5053e7f..df4d874 100644 --- a/crates/graphify-extract/src/workspace.rs +++ b/crates/graphify-extract/src/workspace.rs @@ -447,12 +447,26 @@ pub fn package_entry_candidates(package_dir: &Path, subpath: &str) -> Vec resolve_export_target(dot), + None => resolve_export_target(exports), + } + } else { + None + }; + if let Some(target) = target + && contained_in_package(&target, package_dir) { return vec![package_dir.join(target)]; } diff --git a/crates/graphify-extract/tests/java_type_resolution.rs b/crates/graphify-extract/tests/java_type_resolution.rs index 4d76f00..a66b54f 100644 --- a/crates/graphify-extract/tests/java_type_resolution.rs +++ b/crates/graphify-extract/tests/java_type_resolution.rs @@ -263,10 +263,10 @@ fn java_cross_file_constructor_call_resolves() { } #[test] -fn java_type_parameters_do_not_resolve_to_real_class() { +fn java_type_parameters_do_not_resolve_to_real_class() -> Result<(), Box> { // #1518: a generic field `List` must not emit a references edge to a real // same-named class `T` — `T` is a type variable, not a type. - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempfile::tempdir()?; let real_type = write_file(tmp.path(), "T.java", "public class T {}\n"); let generic = write_file( tmp.path(), @@ -295,4 +295,5 @@ fn java_type_parameters_do_not_resolve_to_real_class() { !has_generic_t_ref, "type parameter T must not resolve to the real T class" ); + Ok(()) } diff --git a/crates/graphify-extract/tests/parity_postprocess.rs b/crates/graphify-extract/tests/parity_postprocess.rs index b035fd3..a39eec7 100644 --- a/crates/graphify-extract/tests/parity_postprocess.rs +++ b/crates/graphify-extract/tests/parity_postprocess.rs @@ -201,3 +201,27 @@ fn salted_id_does_not_collide_with_existing_node() { "all node ids must be distinct: {ids:?}" ); } + +#[test] +fn salted_ids_unique_across_colliding_groups() { + // Two distinct old ids that normalise to the same salted form under the same + // source key (`foo-bar`/`foo_bar` both -> `shared_rb_foo_bar`): a live + // minted-set keeps the two `shared.rb` nodes from reusing one id. Without it + // the second group reassigns the first group's salted id. + let mut nodes = vec![ + n("foo-bar", "FooBar", "shared.rb"), // group "foo-bar" + n("foo-bar", "FooBar", "a.rb"), // makes "foo-bar" ambiguous + n("foo_bar", "FooBar", "shared.rb"), // group "foo_bar" + n("foo_bar", "FooBar", "b.rb"), // makes "foo_bar" ambiguous + ]; + let mut edges: Vec = Vec::new(); + let mut raw_calls: Vec = Vec::new(); + disambiguate_colliding_node_ids(&mut nodes, &mut edges, &mut raw_calls, Path::new(".")); + let ids: Vec<&str> = nodes.iter().map(|nd| nd.id.as_str()).collect(); + let unique: std::collections::HashSet<&str> = ids.iter().copied().collect(); + assert_eq!( + unique.len(), + ids.len(), + "all node ids must be distinct: {ids:?}" + ); +} diff --git a/crates/graphify-extract/tests/ruby_resolution.rs b/crates/graphify-extract/tests/ruby_resolution.rs index 7d553fe..ef05bb6 100644 --- a/crates/graphify-extract/tests/ruby_resolution.rs +++ b/crates/graphify-extract/tests/ruby_resolution.rs @@ -6,15 +6,17 @@ //! `var.method` into a precise edge BY TYPE, not by globally-unique name — so it //! survives name collisions and never emits a false positive when the type is //! unknown (resolution). Every resolved edge must be EXTRACTED confidence. -#![allow(clippy::expect_used, clippy::unwrap_used)] use std::collections::HashMap; +use std::error::Error; use std::path::{Path, PathBuf}; use graphify_extract::{ExtractOutput, FileResult, RawCall, extract, extract_ruby}; use serde_json::Value; use tempfile::tempdir; +type TestResult = Result<(), Box>; + const HELPER_RB: &str = "def transform(data)\n data.upcase\nend\n\n\ class Processor\n def run(items)\n items.map { |i| transform(i) }\n end\nend\n"; @@ -24,10 +26,10 @@ def process_all(items)\n p = Processor.new\n p.run(items)\nend\n"; const WORKER_RB: &str = "class Worker\n def run(jobs)\n jobs.each { |j| j }\n end\nend\n"; -fn write(dir: &Path, name: &str, body: &str) -> PathBuf { +fn write(dir: &Path, name: &str, body: &str) -> std::io::Result { let p = dir.join(name); - std::fs::write(&p, body).expect("write fixture"); - p + std::fs::write(&p, body)?; + Ok(p) } fn find_raw_call<'a>(r: &'a FileResult, callee: &str) -> Option<&'a RawCall> { @@ -69,65 +71,66 @@ fn call_edge<'a>( // ── extraction level ──────────────────────────────────────────────────────── #[test] -fn member_call_captures_receiver() { - let tmp = tempdir().unwrap(); - let main = write(tmp.path(), "main.rb", MAIN_RB); +fn member_call_captures_receiver() -> TestResult { + let tmp = tempdir()?; + let main = write(tmp.path(), "main.rb", MAIN_RB)?; let r = extract_ruby(&main); - let rc = find_raw_call(&r, "run").expect("p.run should produce a raw_call with callee 'run'"); + let rc = find_raw_call(&r, "run").ok_or("p.run should produce a raw_call with callee 'run'")?; assert!(rc.is_member_call); assert_eq!(rc.receiver.as_deref(), Some("p")); + Ok(()) } #[test] -fn local_binding_gives_receiver_a_type() { - let tmp = tempdir().unwrap(); - let main = write(tmp.path(), "main.rb", MAIN_RB); +fn local_binding_gives_receiver_a_type() -> TestResult { + let tmp = tempdir()?; + let main = write(tmp.path(), "main.rb", MAIN_RB)?; let r = extract_ruby(&main); - let rc = find_raw_call(&r, "run").unwrap(); + let rc = find_raw_call(&r, "run").ok_or("missing run raw_call")?; // `p = Processor.new` in the same method => p has type Processor. assert_eq!(rc.receiver_type.as_deref(), Some("Processor")); + Ok(()) } #[test] -fn ambiguous_binding_yields_no_type() { - let tmp = tempdir().unwrap(); +fn ambiguous_binding_yields_no_type() -> TestResult { + let tmp = tempdir()?; let main = write( tmp.path(), "main.rb", "def process_all(items)\n p = Processor.new\n p = Worker.new\n p.run(items)\nend\n", - ); + )?; let r = extract_ruby(&main); - let rc = find_raw_call(&r, "run").unwrap(); + let rc = find_raw_call(&r, "run").ok_or("missing run raw_call")?; // reassigned to a different class => not certain => no type attached. assert_eq!(rc.receiver_type, None); + Ok(()) } // ── resolution level ────────────────────────────────────────────────────────── #[test] -fn resolves_member_call_by_type() { - let tmp = tempdir().unwrap(); - write(tmp.path(), "helper.rb", HELPER_RB); - let main = write(tmp.path(), "main.rb", MAIN_RB); +fn resolves_member_call_by_type() -> TestResult { + let tmp = tempdir()?; + write(tmp.path(), "helper.rb", HELPER_RB)?; + let main = write(tmp.path(), "main.rb", MAIN_RB)?; let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); let labels = label_map(&graph); - let edge = call_edge(&graph, &labels, "process_all", "run"); - assert!( - edge.is_some(), - "process_all should resolve a call to Processor#run" - ); - assert_eq!(edge.unwrap().1, "EXTRACTED"); + let (_, conf) = call_edge(&graph, &labels, "process_all", "run") + .ok_or("process_all should resolve a call to Processor#run")?; + assert_eq!(conf, "EXTRACTED"); + Ok(()) } #[test] -fn resolution_is_type_based_not_name_luck() { +fn resolution_is_type_based_not_name_luck() -> TestResult { // The differentiator: adding an unrelated Worker#run must NOT break the edge. // A name-match resolver drops this (two `run` defs => ambiguous); a type-based // resolver keeps resolving p.run -> Processor#run, never Worker#run. - let tmp = tempdir().unwrap(); - write(tmp.path(), "helper.rb", HELPER_RB); - write(tmp.path(), "worker.rb", WORKER_RB); - let main = write(tmp.path(), "main.rb", MAIN_RB); + let tmp = tempdir()?; + write(tmp.path(), "helper.rb", HELPER_RB)?; + write(tmp.path(), "worker.rb", WORKER_RB)?; + let main = write(tmp.path(), "main.rb", MAIN_RB)?; let graph = extract( &[ main, @@ -137,9 +140,8 @@ fn resolution_is_type_based_not_name_luck() { Some(tmp.path()), ); let labels = label_map(&graph); - let edge = call_edge(&graph, &labels, "process_all", "run"); - assert!(edge.is_some(), "edge must survive the name collision"); - let (tgt_id, conf) = edge.unwrap(); + let (tgt_id, conf) = call_edge(&graph, &labels, "process_all", "run") + .ok_or("edge must survive the collision")?; assert_eq!(conf, "EXTRACTED"); // And it must be the RIGHT run: the target node id is prefixed by its owning // class (helper_processor_run), so it must mention processor, never worker. @@ -148,55 +150,76 @@ fn resolution_is_type_based_not_name_luck() { "expected Processor#run, got {tgt_id}" ); assert!(!tgt_id.to_lowercase().contains("worker")); + Ok(()) } #[test] -fn no_false_positive_when_type_unknown() { +fn no_false_positive_when_type_unknown() -> TestResult { // A member call on a receiver with no known type must NOT be resolved. - let tmp = tempdir().unwrap(); - write(tmp.path(), "helper.rb", HELPER_RB); + let tmp = tempdir()?; + write(tmp.path(), "helper.rb", HELPER_RB)?; let main = write( tmp.path(), "main.rb", "require_relative \"helper\"\n\ndef process_all(thing)\n thing.run(1)\nend\n", - ); + )?; let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); let labels = label_map(&graph); // `thing` is a parameter of unknown type => no precise target => no edge. assert!(call_edge(&graph, &labels, "process_all", "run").is_none()); + Ok(()) } #[test] -fn class_new_creates_instantiation_edge() { +fn class_new_creates_instantiation_edge() -> TestResult { // `p = Processor.new` should link the caller to the Processor class. - let tmp = tempdir().unwrap(); - write(tmp.path(), "helper.rb", HELPER_RB); - let main = write(tmp.path(), "main.rb", MAIN_RB); + let tmp = tempdir()?; + write(tmp.path(), "helper.rb", HELPER_RB)?; + let main = write(tmp.path(), "main.rb", MAIN_RB)?; let graph = extract(&[main, tmp.path().join("helper.rb")], Some(tmp.path())); let labels = label_map(&graph); - let edge = call_edge(&graph, &labels, "process_all", "Processor"); - assert!( - edge.is_some(), - "Processor.new should resolve a call to the Processor class" - ); - assert_eq!(edge.unwrap().1, "EXTRACTED"); + let (_, conf) = call_edge(&graph, &labels, "process_all", "Processor") + .ok_or("Processor.new should resolve a call to the Processor class")?; + assert_eq!(conf, "EXTRACTED"); + Ok(()) } #[test] -fn reassignment_to_untyped_value_clears_type() { +fn class_new_resolves_for_method_less_class() -> TestResult { + // A method-less class still resolves `X.new`: the Ruby class index is built + // from `contains` nodes, not only `method` edges, so `class Config; end` + // (which emits no `method` edge) is still found. + let tmp = tempdir()?; + write(tmp.path(), "config.rb", "class Config\nend\n")?; + let main = write( + tmp.path(), + "main.rb", + "require_relative \"config\"\n\ndef boot\n c = Config.new\n c\nend\n", + )?; + let graph = extract(&[main, tmp.path().join("config.rb")], Some(tmp.path())); + let labels = label_map(&graph); + let (_, conf) = call_edge(&graph, &labels, "boot", "Config") + .ok_or("Config.new should resolve to the method-less Config class")?; + assert_eq!(conf, "EXTRACTED"); + Ok(()) +} + +#[test] +fn reassignment_to_untyped_value_clears_type() -> TestResult { // The 100%-confidence contract: a variable reassigned to anything other than // a single `Constant.new` (here a plain method call) becomes ambiguous, so no // type is carried and the member call is never resolved by type. - let tmp = tempdir().unwrap(); + let tmp = tempdir()?; let main = write( tmp.path(), "main.rb", "def process_all(items)\n p = Processor.new\n p = items.first\n p.run(items)\nend\n", - ); + )?; let result = extract_ruby(&main); - let rc = find_raw_call(&result, "run").unwrap(); + let rc = find_raw_call(&result, "run").ok_or("missing run raw_call")?; assert_eq!( rc.receiver_type, None, "reassign to an untyped value poisons the type" ); + Ok(()) } diff --git a/crates/graphify-llm/tests/openai_compat_helpers.rs b/crates/graphify-llm/tests/openai_compat_helpers.rs index 4a9fb1a..b2bc03f 100644 --- a/crates/graphify-llm/tests/openai_compat_helpers.rs +++ b/crates/graphify-llm/tests/openai_compat_helpers.rs @@ -330,4 +330,6 @@ fn resolve_max_retries_default_and_env() { assert_eq!(resolve_max_retries(), 0, "disable is allowed"); g.set("GRAPHIFY_MAX_RETRIES", "bogus"); assert_eq!(resolve_max_retries(), 6, "invalid -> default"); + g.set("GRAPHIFY_MAX_RETRIES", "-1"); + assert_eq!(resolve_max_retries(), 6, "negative -> default"); } diff --git a/crates/graphify-llm/tests/per_backend_http.rs b/crates/graphify-llm/tests/per_backend_http.rs index 5b8c88b..6aab3ee 100644 --- a/crates/graphify-llm/tests/per_backend_http.rs +++ b/crates/graphify-llm/tests/per_backend_http.rs @@ -139,10 +139,11 @@ fn openai_retries_rate_limited_request() { fn openai_gives_up_when_retries_disabled() { // GRAPHIFY_MAX_RETRIES=0 disables retries: a 429 fails immediately. let mut server = mockito::Server::new(); - let _rl = server + let rl = server .mock("POST", "/chat/completions") .with_status(429) .with_body("rate limited") + .expect(1) .create(); let mut g = allow_private(); @@ -160,6 +161,7 @@ fn openai_gives_up_when_retries_disabled() { .is_err(), "with retries disabled, a 429 must fail" ); + rl.assert(); // exactly one request: GRAPHIFY_MAX_RETRIES=0 means no retry } // ── gemini ───────────────────────────────────────────────────────────────── diff --git a/src/cli/dispatch.rs b/src/cli/dispatch.rs index 35d5229..3da098f 100644 --- a/src/cli/dispatch.rs +++ b/src/cli/dispatch.rs @@ -271,12 +271,13 @@ fn dispatch_save_result(cmd: Command) -> Result<()> { }; // `--answer-file` lets callers pass a long/multiline answer via a file instead // of a fragile inline arg (Windows/PowerShell quoting), #1502. It wins over - // `--answer`; with neither, fail with a message naming both flags. + // `--answer`; with neither, fail with a message naming both flags. The file + // content is preserved exactly (indentation, trailing newlines) to match + // inline `--answer`, which is unstripped — diverging from graphify-py + // (__main__.py:2982), which `.strip()`s the file. let answer = match answer_file { Some(path) => std::fs::read_to_string(&path) - .map_err(|e| anyhow::anyhow!("--answer-file {}: {e}", path.display()))? - .trim() - .to_string(), + .map_err(|e| anyhow::anyhow!("--answer-file {}: {e}", path.display()))?, None => answer.ok_or_else(|| anyhow::anyhow!("--answer or --answer-file is required"))?, }; cli::save_result::cmd_save_result( diff --git a/src/cli/extract.rs b/src/cli/extract.rs index acd5d44..f5e1f9a 100644 --- a/src/cli/extract.rs +++ b/src/cli/extract.rs @@ -595,9 +595,16 @@ fn prune_semantic_cache_safe(root: &std::path::Path, sem_paths: &[String]) { let live: std::collections::HashSet = sem_paths .iter() .filter_map(|p| { - let fp = std::path::Path::new(p); + // Resolve a relative path against `root` exactly as + // `check_semantic_cache` does (semantic.rs), so the live-set hashes + // key the same entries the cache lookup did; otherwise a relative CLI + // input would hash differently here and prune a valid entry. + let mut fp = std::path::PathBuf::from(p); + if !fp.is_absolute() { + fp = root.join(&fp); + } fp.is_file() - .then(|| graphify_cache::file_hash(fp, root).ok()) + .then(|| graphify_cache::file_hash(&fp, root).ok()) .flatten() }) .collect(); diff --git a/tests/cli_commands.rs b/tests/cli_commands.rs index 7bb5c08..9d29caf 100644 --- a/tests/cli_commands.rs +++ b/tests/cli_commands.rs @@ -1211,5 +1211,5 @@ fn save_result_requires_answer_or_answer_file() { .args(["save-result", "--question", "q", "--outcome", "useful"]) .assert() .failure() - .stderr(contains("--answer")); + .stderr(contains("--answer").and(contains("--answer-file"))); } From f451a978c6c8821254fe76eae7477fce20cdea9a Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Tue, 30 Jun 2026 10:39:59 +0200 Subject: [PATCH 6/7] Clarify the non-C importer assertion in the header-remap test CodeRabbit asked the test to assert the non-C import resolves to the Python variant, but the salt remap is keyed by the importer's own file and cannot infer the target module for a bare ambiguous import when the importer differs from the definition (the graphify-py reference doesn't either). Document that in the test and assert the gate the fix actually guarantees -- a non-C import is never repointed at the header variant -- rather than pinning a dangling id. By the will of the Machine God --- crates/graphify-extract/tests/parity_postprocess.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/graphify-extract/tests/parity_postprocess.rs b/crates/graphify-extract/tests/parity_postprocess.rs index a39eec7..743b844 100644 --- a/crates/graphify-extract/tests/parity_postprocess.rs +++ b/crates/graphify-extract/tests/parity_postprocess.rs @@ -169,10 +169,17 @@ fn header_remap_skips_non_c_family_importers() { header_id, "foo", "header should be salted away from the bare id" ); + // A C-family `#include` resolves to the header variant... assert_eq!( edges[1].target, header_id, "a C `#include` should resolve to the header variant" ); + // ...while a non-C importer's edge must NOT be redirected to the header. We + // assert only the negative here: the salt remap is keyed by the importer's own + // file, and `consumer.py` matches neither colliding definition, so the pass + // has no information to resolve a bare ambiguous import to the Python module. + // Pinning a positive target would either codify a dangling id or assume a + // module-inference step this pass (and the graphify-py reference) never does. assert_ne!( edges[0].target, header_id, "a non-C import must not be repointed at the header variant" From ec1c325912d7b826f158a86943d7c3722e11a66b Mon Sep 17 00:00:00 2001 From: Robbie Blaine Date: Tue, 30 Jun 2026 10:56:15 +0200 Subject: [PATCH 7/7] Resolve Swift uppercase receivers as types, not bindings The file-wide local type table is not scope-aware, so consulting it before the receiver's syntax could demote a genuine `Type.staticMethod()` to an INFERRED call on whatever type a same-named local was bound to elsewhere in the file. Resolve an upper-cased receiver as the named type (EXTRACTED) and consult the table only for lower-cased receivers, which re-aligns with graphify-py (extract.py:9553) and removes the divergence. Add a regression test for the pollution case (an upper-cased parameter binding alongside a real type-qualified call). The companion review note about `member_call_captures_receiver` missing `#[test]` is a false positive: the attribute is present and the test runs (confirmed via `nextest -E 'test(member_call_captures_receiver)'`). By the will of the Machine God --- .../src/extractors/multi/swift.rs | 30 ++++----- .../tests/swift_member_calls.rs | 63 +++++++++++++++++++ 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/crates/graphify-extract/src/extractors/multi/swift.rs b/crates/graphify-extract/src/extractors/multi/swift.rs index 49fbbf8..cc83585 100644 --- a/crates/graphify-extract/src/extractors/multi/swift.rs +++ b/crates/graphify-extract/src/extractors/multi/swift.rs @@ -171,21 +171,23 @@ pub(super) fn resolve_swift_member_calls( let Some(receiver) = rc.receiver.as_deref() else { continue; }; - // A receiver bound as a local (property/parameter) in this file resolves - // via its inferred type and is INFERRED — even when the local's name is - // upper-cased, which would otherwise look type-qualified. Only a receiver - // with NO local binding is treated as a bare type name - // (`Type.staticMethod()`), which is EXTRACTED because the type is named - // explicitly in source (#1533). Divergence from graphify-py - // (extract.py:9553), which keys solely on receiver casing and so mis-marks - // an upper-cased local as EXTRACTED. - let local_type = type_table_by_file + // An upper-cased receiver is named as a type in source + // (`Type.staticMethod()`, `Singleton.shared.x()`) and is EXTRACTED. A + // lower-cased receiver is a local; resolve its type via the file's local + // table and mark the call INFERRED (#1533). The table is file-wide, not + // scope-aware, so it is consulted ONLY for lower-cased receivers: using it + // to override an upper-cased (conventionally a type) receiver could wrongly + // demote a genuine `Type.staticMethod()` to INFERRED when the same name is + // a local in another scope of the file. Keys on casing like graphify-py + // (extract.py:9553). + let type_qualified = receiver.chars().next().is_some_and(char::is_uppercase); + let type_name = if type_qualified { + receiver.to_string() + } else if let Some(t) = type_table_by_file .get(&rc.source_file) - .and_then(|tbl| tbl.get(receiver)); - let (type_name, type_qualified) = if let Some(t) = local_type { - (t.clone(), false) - } else if receiver.chars().next().is_some_and(char::is_uppercase) { - (receiver.to_string(), true) + .and_then(|tbl| tbl.get(receiver)) + { + t.clone() } else { continue; }; diff --git a/crates/graphify-extract/tests/swift_member_calls.rs b/crates/graphify-extract/tests/swift_member_calls.rs index dff0b60..a0c85f0 100644 --- a/crates/graphify-extract/tests/swift_member_calls.rs +++ b/crates/graphify-extract/tests/swift_member_calls.rs @@ -258,3 +258,66 @@ fn swift_unknown_receiver_emits_no_edge() { "unknown receiver should not resolve: {edges:?}" ); } + +#[test] +fn swift_uppercase_local_does_not_shadow_a_real_type_receiver() { + // Regression: the file's local type table is file-wide, not scope-aware. An + // upper-cased local binding (here a parameter `SessionType: OtherType`) must + // NOT demote a genuine `SessionType.staticMethod()` to an INFERRED call on + // OtherType — an upper-cased receiver is resolved as the named type + // (EXTRACTED), ignoring the table. A table-first resolver would mis-resolve it + // to OtherType.staticMethod. + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("src"); + let mut files = vec![ + write_file( + &base, + "Core/SessionType.swift", + "enum SessionType {\n static func staticMethod() {}\n}\n", + ), + write_file( + &base, + "Core/OtherType.swift", + "class OtherType {\n func staticMethod() {}\n}\n", + ), + write_file( + &base, + "Views/Poller.swift", + "class Poller {\n\ + \x20 func bind(SessionType: OtherType) {}\n\n\ + \x20 func go() {\n\ + \x20 SessionType.staticMethod()\n\ + \x20 }\n\ + }\n", + ), + ]; + files.sort(); + let res = extract(&files, Some(&tmp.path().join("cache"))); + let edge = res + .edges + .iter() + .find(|e| { + e.get("relation").and_then(|v| v.as_str()) == Some("calls") + && label_of(&res, e.get("source").and_then(|v| v.as_str()).unwrap_or("")) == ".go()" + && label_of(&res, e.get("target").and_then(|v| v.as_str()).unwrap_or("")) + == ".staticMethod()" + }) + .expect("go() should call a staticMethod"); + assert_eq!( + edge.get("confidence").and_then(|v| v.as_str()), + Some("EXTRACTED"), + "an upper-cased receiver is type-qualified, not table-resolved" + ); + // ...and it must target SessionType's method, not OtherType's. + let tgt_id = edge.get("target").and_then(|v| v.as_str()).unwrap_or(""); + let tgt_sf = res + .nodes + .iter() + .find(|n| n.get("id").and_then(|v| v.as_str()) == Some(tgt_id)) + .and_then(|n| n.get("source_file").and_then(|v| v.as_str())) + .unwrap_or(""); + assert!( + tgt_sf.contains("SessionType"), + "must resolve to SessionType.staticMethod, got source_file {tgt_sf}" + ); +}