Skip to content

fix(pipeline): refuse same_module for import-bound or builtin JS/TS receivers - #1128

Merged
DeusData merged 4 commits into
DeusData:mainfrom
sahil-mangla:fix/registry-resolution
Nov 10, 2026
Merged

DeusData merged 4 commits into
DeusData:mainfrom
sahil-mangla:fix/registry-resolution

Conversation

@sahil-mangla

Copy link
Copy Markdown
Contributor

Description

This PR tightens the call-resolution core in the registry to prevent false-positive CALLS edge resolutions when dealing with qualified receivers, improving precision across all supported languages.

Changes

  • Restricted resolve_same_module on receivers: The resolve_same_module strategy now validates the receiver prefix. It will correctly resolve self-receivers (self.get), exact module prefixes (proj.pkg.service.get), and last-segment namespace matches (service.get). It actively rejects resolving to local functions when the receiver is clearly unrelated (e.g., axios.get or _get_store().get), preventing spurious edges to same-named functions within the current module.
  • Restricted resolve_name_lookup for qualified callees: Added a qn_ends_with_qualified check. If a callee name is explicitly qualified (e.g., contains . or ::), the resolution candidate's qualified name must properly terminate with that exact qualification (unless the candidate is explicitly labeled as a Method), preventing cross-namespace leakage on generic method names.

Impact

  • Precision vs Recall: This change heavily favors precision, eliminating wide blast-radius false positives on common method names (like get, add, update) when invoked on unrelated imported or delegated receiver objects.
  • Testing: Added regression tests for resolve_same_module_only_on_self_receiver and verified the full test suite runs cleanly.

@sahil-mangla
sahil-mangla requested a review from DeusData as a code owner July 16, 2026 10:33
@DeusData DeusData added the bug Something isn't working label Jul 16, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 16, 2026
@DeusData DeusData added parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 16, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for taking on the receiver false-positive problem. The exact head is security-clean and DCO passes, but it is not correct enough to merge:

  1. Every full Unix/macOS/Windows test job fails deterministically at cp_enum_method_java: valid Java enum calls such as d.label() and d.isWeekend() become unresolved. This existing convergence guard must remain green.
  2. The blanket Method/type-like-parent exemption preserves the main false-positive class: an unrelated axios.get can still resolve through unique_name when the sole candidate is labeled Method. The new negative test uses a Function, so it does not exercise that common path.
  3. Add direct coverage for the unique and multi-candidate paths, imported receivers, . and :: qualification, and candidates labeled Method.
  4. Use the canonical cbm_label_is_type_like() policy rather than duplicating the label set in the registry.

The precision goal is valid, but the registry currently lacks enough receiver/language/type evidence for this global heuristic: it is simultaneously too strict for valid Java dispatch and too permissive for unrelated Method candidates. Please redesign the suppression around receiver-aware evidence, preserve the Java enum convergence test, and update the PR's “full suite clean” statement after the complete matrix is actually green.

@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch 2 times, most recently from 25457d2 to 7c78413 Compare July 16, 2026 16:59
@sahil-mangla
sahil-mangla marked this pull request as draft July 16, 2026 17:01
@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from 7c78413 to 0b22112 Compare July 16, 2026 19:00
@sahil-mangla
sahil-mangla marked this pull request as ready for review July 16, 2026 19:03
@sahil-mangla
sahil-mangla marked this pull request as draft July 17, 2026 10:27
@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from ab4e7e5 to e6e5377 Compare July 19, 2026 06:08
@sahil-mangla
sahil-mangla marked this pull request as ready for review July 19, 2026 07:21
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData the registry fix it ready to be reviewed!

@DeusData

Copy link
Copy Markdown
Owner

Thanks for the update. I see current head e6e537759e176e8a23d06828ea2808b7cff16fc8 and a green matrix. The exact-head re-review will focus on the prior Java enum-dispatch regression and Method-labelled receiver false positives before we give further technical feedback. No additional contributor action is requested in the meantime.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — the false-positive class you are killing is real, and your tests demonstrate it convincingly. axios.get resolving to an unrelated same-named method on main today is exactly the kind of wrong edge that erodes trust in the graph. I want to be specific about what I verified, because two things need your attention before a merge decision.

What holds up. The registry logic is sound and the new tests bind. Restricting suffix matching on receivers is a defensible precision improvement, and the thread-local _import_map_cache use is fine.

1. A regression the tests cannot see — Java enum constants.

The diff rewrites find_class_body to descend enum_body → enum_body_declarations. But extract_enum_members walks the direct children of whatever find_class_body() returns, looking for enum_constant nodes. In tree-sitter-java, enum constants are children of enum_body — they are siblings of enum_body_declarations, not inside it.

The consequence: for any Java enum that also declares methods, the constants (MON, TUE, …) stop being extracted as Variable nodes. They just disappear. Your java_enum_method test asserts only the Enum and its Methods, so CI stays green while the constants are lost.

Worth adding a test that asserts the constants and the methods on the same enum — that would have caught this and will stop it recurring.

2. Scope — there is a second feature in here.

The description covers the registry precision change. The diff also rewrites find_class_body, adds a Java identifier-fallback for class names, and extends push_class_body_children for enum_body/enum_body_declarations. That is Java enum extraction work, and I suspect it was added to make the receiver-awareness test's Java case pass.

We hold to atomic PRs, so I would like to see this split: the Java-enum extraction improvement is independently valuable and should land on its own merits (with the constants bug fixed), and the registry change should stand or fall on its own.

3. One question that is above my pay grade, so I have referred it to the maintainer.

This changes the precision/recall balance of the resolution core for every language at once, and I could not find evidence of what it costs. The self-receiver list is hardcoded {self, this, cls, @self} — that misses $this in PHP and Me in VB, and C++ ns::helper() calls will stop resolving where namespaces are not part of the qualified name. Killing false positives is good; losing true CALLS edges is a findability regression, and we care about that more.

We have a per-language benchmark and evaluation harness precisely for this kind of trade-off. The maintainer will decide whether this needs to go through it first. That is not a criticism of your work — it is a question about how much evidence a change to the resolution core needs, and it is his call rather than mine.

Points 1 and 2 are worth acting on regardless of how point 3 lands. Thanks again for digging into a genuinely hard part of the system.

@DeusData

Copy link
Copy Markdown
Owner

Follow-up with more detail, now that I have also reviewed your #893 and closed it in favour of this PR (nothing unique was lost — the comparison is in that thread).

Reviewing the two together produced a sharper picture of the recall risk I flagged earlier. This is not a request to change anything yet, since the direction call is the maintainer's — but the specifics are worth having on the record, because they are the evidence that decision will rest on.

Where the receiver heuristic loses true edges. The self-receiver list is {self, this, cls, @self}, matched against raw callee node text. Concretely:

  • PHP emits $this.helper with the sigil preserved, so "$this" is not in the list.
  • Perl convention is $self.
  • VB.NET is Me.
  • Rust associated calls are Self::new.

PHP and Rust have LSP cover, but the registry is their fallback path; VB-style languages have neither. This matters more than it might look, because cbm_registry_resolve splits at the first separator, so for roughly 150 non-LSP languages this heuristic is the primary resolver.

Two further recall paths I had not previously named:

  1. Multi-segment module-qualified callees. The prefix check uses only the first segment, so pkg.service.get inside module proj.pkg.service fails Strategy 2 ("pkg" ≠ module tail "service"), while the less qualified service.get passes — the more precise reference resolves worse than the vaguer one. Strategy 3 can rescue it via the qualified tail, but only below REG_MAX_CANDIDATES; at our scale the hot names (get, add, update) blow straight past that cap and come back empty. Strategy 2's module fallback was the scale-proof path for exactly those names.
  2. Alias receivers. The resolve_name_lookup enforcement drops true edges for Function candidates behind an alias: Elixir alias Foo.Bar, as: B; B.helper() yields a winner proj.foo.bar.helper that does not end in B.helper, so it resolves to nothing where it previously resolved correctly. The same shape hits Ruby constant aliases and CommonJS const u = require('./utils'); u.helper() whenever import extraction misses the alias. Elixir aliasing in particular is idiomatic essentially everywhere.

Your PR description is candid that the change "heavily favours precision", and that honesty is appreciated — it is also precisely why this needs a maintainer decision rather than a reviewer's. This project's stated priority runs the other way: a false edge is noise, but a lost true edge is a thing an agent can no longer find. Killing axios.get false positives is a real win, and I do not want to lose it; the question is what it costs across the other languages, and that is measurable with the per-language bench and eval harness rather than arguable.

Also worth noting independently of all of the above: there is no test anywhere in the suite that asserts Java enum constants. That is why the enum defect I raised earlier stays green in CI on this PR, on #893, and on #984. Worth adding regardless of which way the direction question lands.

Nothing for you to do on the direction question — that is ours to answer, and I will come back to you. The enum constants fix and the Java gating from my earlier comment are still the concrete asks.

@DeusData

Copy link
Copy Markdown
Owner

An update that makes this easier to settle, and it replaces prose with a test.

The regression I described is now pinned on main. tests/test_extraction.c:482-483, inside java_enum_dedup_preserves_calls_issue1234, now asserts:

ASSERT(has_def(r, "Variable", "MON"));
ASSERT(has_def(r, "Variable", "SUN"));

That is the guard for enum constants surviving alongside enum methods, and it is exactly what the find_class_body change bundled into this branch breaks. So rather than taking my word for it, rebase onto current main and run build/c/test-runner extraction — you will see it go red, and you will see precisely which behaviour changed. That is better evidence than my description, and if it turns out I have misread your change, the test will show that too.

To be clear about why this matters beyond one test: dropping enum constants is a graph-quality regression that no consumer can see. The nodes simply are not there, so nothing errors — searches just quietly return less. That is the failure mode this project treats as the worst kind.

The bigger question is still open and it is the one blocking merge. Restricting same-module and unique-name suffix matching genuinely improves precision — I am not disputing the premise, and over-eager suffix matching does mint wrong edges. But my read is that it also costs recall in PHP, Perl, VB.NET and Rust, where the resolver leans on suffix matching more heavily than it does in the languages you tested against.

I do not want to decide that from either of our intuitions. What would settle it is measured per-language edge-count deltas: index the same corpus before and after your change and report CALLS-edge counts per language. If precision rises without meaningful recall loss outside the cases you targeted, that is a merge. If PHP or Rust drops a large fraction of its call edges, we need a narrower rule.

If gathering that is more than you want to take on, say so — it is a fair amount of work, and I would rather know than have this sit for another month. I can run the measurement myself if you would prefer to hand it over.

One process note for whenever you next push here: the find_class_body change is not mentioned in the PR description, which claims only the registry suffix-match restriction. A reviewer looking for what you described would not go looking in the class-body walker. Whatever we conclude on the main question, please split that out or describe it — a bundled change is the one thing that reliably makes a review miss something.

@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Open since 16 July with no reply from us at all. Seven weeks of silence on 378 lines of careful work is not acceptable, and I am sorry. Here is a real review.

The finding that decides it

Test Case D and Test Case E are right — a qualified receiver (Service.get, Service::get) should resolve. But Test Case C asserts the exact false positive this PR sets out to remove:

/* Test Case C: Receiver that is an unregistered/instance variable (e.g. "d.get") */
{
    cbm_resolution_t res = cbm_registry_resolve(r, "d.get", "proj.app", NULL, NULL, 0);
    ASSERT_STR_EQ(res.qualified_name, "proj.pkg.Service.get");
}

d is an unregistered receiver and the import map is empty (NULL, NULL, 0). Nothing connects d to proj.pkg.Service — yet the test requires the resolver to bind them. That is the axios.get → project.get shape, written down as desired behaviour.

So the suite documents the change failing its own stated goal. Whatever else is right here, a reviewer cannot accept a precision fix whose tests demand the imprecision, and I would rather tell you that plainly than let it sit for another seven weeks.

The architectural constraint you could not have known

This is the part that is genuinely on us for not saying in July.

Receiver-precision fixes in this project are written as per-language suppressors, gated on the file's language, and wired at both pass_calls.c and pass_parallel.c — not as changes to the strategies inside cbm_registry_resolve. All three existing guards follow that shape, and every one deliberately preserves the same_module path.

The reason is concrete. Go receiver methods carry a flat qualified name (pkg.Area, not pkg.Circle.Area), so c.Area() resolves only through the same_module suffix branch that a global restriction gates off. And roughly 150 languages without an LSP use this chain as their only resolver, so the blast radius is far wider than the languages anyone tests. A blocked high-confidence match does not vanish either — it is demoted from same_module (0.90) into suffix_match (0.55), which picks a winner by import distance project-wide. A global restriction can therefore lose a correct edge and mint a wrong one in the same step.

Your PR is the most global member of this family: it changes strategies inside cbm_registry_resolve itself, where the others add suppressors downstream.

A global change is not categorically refused, but it has to carry an unusually strong case: that the upstream strategies are provably untouched, a measured per-language blast radius on real codebases with the refusals hand-read, no per-language seam available at that point, and a demonstration that confidence scoring cannot do the job instead. That bar has been met once, by a PR that measured three real codebases and hand-read every refusal.

Where that leaves this

I am not closing it — that is the maintainer's call and I have not asked. But as it stands it cannot merge, for the test reason above independently of the architectural one.

If you want to take it further, the shape most likely to land is: drop Test Case C or invert it to assert the non-resolution, move the restriction into a per-language suppressor gated on the file language and wired at both call sites, and bring a measurement of what it removes and keeps on a real repository.

It is also long since conflicted with main. Please do not spend time rebasing until you have decided whether you want to pursue the reshaped version — I would rather not cost you more work on top of the wait.

Thank you for the report and the work, and again, sorry for the silence.

@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from e6e5377 to d4b7622 Compare October 18, 2026 09:40
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Hi @DeusData,

Thank you for the detailed feedback. I have rebased the PR cleanly onto main and addressed your review notes:

  1. Uncoupled from Java Enum Extraction: The bundled find_class_body changes were dropped since they were already resolved upstream in 5eae0a43. This PR now focuses purely on receiver precision.
  2. Inverted Test Case C: Inverted d.get with an empty import map to assert non-resolution (NULL), matching the intended precision goal and eliminating the false positive.
  3. Preserved Go & Qualified Suffix Dispatch: Maintained Go flat receiver dispatch (c.Area()) and allowed qualified attribute chains in the same module while strictly preventing unrelated bare receivers (like axios.get or _get_store().get) from resolving to local functions.
  4. Builtins Compatibility: Builtin types like builtins.dict.get remain resolvable as unique-name matches, while foreign project methods and functions require matching receiver qualifications.

All 94 tests in SUITE(registry) are passing cleanly.

… receivers

Tighten call resolution precision in the registry:
- Validate receiver prefixes in resolve_same_module: allow self receivers,
  exact module names, and namespace prefixes, while rejecting unrelated
  local function bindings (e.g. axios.get).
- Preserve Go flat receiver dispatch (e.g. c.Area()) and qualified
  attribute chains in the same module.
- In unique-name and suffix-match strategies, reject receiver mismatches
  when the callee targets a Function (not a Method), or when targeting
  a Method and the receiver is an imported alias or known Module/Class/Type
  that contradicts the candidate target.
- Preserve resolution for untyped instance receivers (e.g. vm.load, c.val())
  and builtins (e.g. builtins.dict.get).
- Add unit test coverage in test_registry.c.

Signed-off-by: sahil-mangla <[email protected]>
@sahil-mangla
sahil-mangla force-pushed the fix/registry-resolution branch from d4b7622 to f3bcb25 Compare October 18, 2026 14:28
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Hi @DeusData,

Refined the implementation in line with your review notes and precision guidance:

  1. Dropped Test Case C: Completely removed the unregistered instance variable test case (d.get) to avoid imposing an artificial global restriction that penalizes untyped instance dispatch across non-typed languages.
  2. Context-Aware Receiver Restriction in Name Lookup:
    • For Function candidates (free functions): qualification is strictly required (qn_ends_with_qualified), preventing arbitrary receiver bindings (e.g. unrelated.compute does not bind helper.compute).
    • For Method candidates: receiver mismatches are rejected only when the receiver prefix is an imported alias (is_imported_receiver) or a known Type/Class/Module/Namespace in the registry (is_known_type_or_namespace), preventing cross-namespace collisions (axios.get and other_service.get do not resolve to Service.get).
    • Untyped local instance receivers (e.g. vm.load, c.val(), client.get()) are preserved without regressions.
  3. Same-Module Precision: Preserved Go flat receiver dispatch (c.Area()) and self/module qualifications, while preventing unrelated receivers (axios.get, _get_store().get) from resolving to local module functions.

All changes are formatted with clang-format -i and signed off with DCO.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sahil-mangla, thank you for coming back to this, and for doing it so thoroughly - the rebase onto current main, dropping the bundled find_class_body work now that #1234's fix (5eae0a4) covers it, and removing Test Case C are exactly the right moves, and the PR is much easier to review for it. The axios.get false positive you are going after is real, and I want this to land in a shape that keeps it fixed.

The CI run on your current head (f3bcb25) is red on all four test legs (Linux, macOS, Windows, TSan) with the same three failures in the pipeline suite. They are deterministic, not flakes, and they are worth reading closely, because they show the problem with doing this inside the resolver:

  1. pipeline_c_member_access_binds_field_by_object_type and its _parallel twin (tests/test_pipeline.c): a C call b.notify(3), where b is declared as Beta, no longer produces the CALLS edge to Beta.notify. On main the registry first returns a notify Field, and cbm_call_onto_field_policy (in pass_calls.c and pass_parallel.c) then re-binds it to the Field of the object's own type. With this change should_reject_receiver refuses every candidate that is not labelled Method - the function-pointer member is a Field - and Strategy 2 refuses the receiver b, so the resolver returns nothing and the typed re-binding never runs. That is a true, type-checked edge lost.

  2. python_typed_field_external_import_stays_unresolved: this test feeds unittest.TestCase through cbm_registry_resolve and requires the raw same_module candidate to still appear, so that it proves the downstream field-type guard is what keeps the external import unresolved. Your change removes the raw candidate, so the guard is no longer exercised. The outcome happens to be right here, but the test exists to stop that guard from going untested, so it cannot simply be relaxed.

Both tests exist on the base you rebased onto, so build/c/test-runner registry pipeline reproduces them locally; registry alone does not reach them.

Why this keeps happening, and the shape that avoids it. cbm_registry_resolve_lang is not only the call resolver. It also resolves type references and field types (pass_usages.c, pass_semantic.c), reads and writes, SQL lineage (cbm_registry_resolve_lineage), and the cross-LSP pass. A rule added to Strategies 2-4 therefore changes all of those, for every language at once, which is what the two failures above are. That is the reason for the request on 2 September, which this version does not yet follow: receiver-precision rules here are written as per-language suppressors at the call sites, gated on the file's language and applied identically in pass_calls.c and pass_parallel.c (the two must match, or the sequential and parallel paths produce different graphs). The existing ones are good templates: cbm_suppress_weak_member_match and cbm_perl_suppress_generic_match in pass_calls.c, mirrored in pass_parallel.c.

Part of what this PR does is in fact already on main in that form: for Python, JavaScript, TypeScript/TSX, ArkTS and script blocks in HTML/Vue/Svelte/Astro, a member call that only matched by unique_name, suffix_match, field_type_hint or fuzzy is already dropped (#592/#606/#1276). What that guard deliberately keeps is same_module - and that is exactly your headline case: axios.get() in a file that defines its own get still binds to it at 0.90 on main. So the piece with the clearest value is narrow and well defined.

Concretely, what would get this to merge:

  1. Leave the strategy chain in registry.c unchanged (both failing tests then pass as they are).
  2. Add a suppressor for a member call that resolved by same_module while its receiver is neither a self receiver nor the module itself, wired at both call sites with one shared language list. The self-receiver spellings then become per-language facts (self/cls in Python, this in JS/TS, $this in PHP, $self in Perl, Me in VB, Self in Rust) instead of one list applied everywhere - as written, a JavaScript variable named me counts as a self receiver.
  3. Start with the languages you can show it is right for; the JS/TS family is the natural first set, since axios.get is the motivating case. Go must stay out, for the flat receiver-method names you already preserved.
  4. Wherever you need an "is this a type" test, use cbm_label_is_type_like() rather than a local list of labels, so the two cannot drift.
  5. Keep your resolve_same_module_only_on_self_receiver cases as the positive and negative controls, move them to the suppressor's level, and add one pipeline-level test per language in the set that is red before the fix, with positive controls that must still resolve (this.get(), a module-qualified call, and in CommonJS exports.get()).
  6. A before/after count of CALLS edges per language on one real repository, with the removed edges spot-read. With the rule scoped this narrowly that should be a short, readable list. If you would rather hand that part over, say so and we will run it.
  7. Please update the PR description to the new scope and to the suites you actually ran.

Thank you again - finding this class of edge and sticking with it through a long review is genuinely useful work for the graph.

@DeusData

Copy link
Copy Markdown
Owner

@sahil-mangla, a quick follow-up to our review from yesterday, before you put time into it: we ran the suppressor shape we asked for (items 2 and 3) on two real JavaScript/TypeScript repositories, and it removes too many correct edges. In JS/TS, methods of an object literal (const context = { push() {...} }, as in Vue's createCodegenContext) and functions assigned onto an object (Translator.create = function create() {...}) are stored as plain module-level functions, so context.push() reaching the module's push through the same-module lookup is usually right. On vuejs/core about half of the removed edges were correct ones like that; on NodeBB about one in four. That is the same trap as Go's flat receiver methods, one level over, and the miss was in our ask, not in your code.

So please hold off on items 2, 3 and 6 for now: we are working out a narrower shape and will come back to you with it. Item 1 of the review (leave the strategy chain in registry.c unchanged) still stands; with that alone the three failing pipeline tests pass.

Thank you for your patience with this one, and for finding the class in the first place.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for your patience and the receiver-resolution work. Our later October 19 correction still applies: please hold items 2, 3 and 6 while we settle the narrower approach; the request to leave the registry strategy chain unchanged stands. Sorry for the delay on our follow-up—I’ll come back to you soon.

@DeusData

Copy link
Copy Markdown
Owner

@sahil-mangla, a short update so this does not look forgotten: we have settled the narrower shape. The same-module lookup will be refused only for a member call whose receiver is bound to an import (an ES import, a CommonJS require, or an AMD dependency) or is a builtin global such as console or Math - exactly your axios.get case - while calls like context.push() on an object-literal method keep their edges. Since you allowed maintainer edits, we will make that change on your branch ourselves: it replaces the registry.c strategy changes (item 1 of the review) and keeps your test cases as the positive and negative controls, and we will add a before/after edge count on real JavaScript/TypeScript repositories and explain every commit here. Nothing is needed from you in the meantime. Thank you for finding this class of wrong edge and for your patience while we got the shape right.

The same-module guard for DeusData#1128 needs to know, from the caller's own
source, whether a member call's receiver is bound to another module.
Two facts were missing:

- A JS/TS import statement that binds no local name (side-effect
  `import './x'`, a re-export, a bare or destructured `require('x')`,
  `require('x').y`) still records the path's last segment as its
  local_name, so `require('node:stream').Readable` made every `stream`
  parameter look import-bound. CBMImport.local_name_synthesized marks
  those stand-ins. A lazy `x = require('x')` and
  `const x = require('x').member` do bind that very name and stay
  unmarked. local_name itself is unchanged, so IMPORTS edges and the
  import map are untouched.
- AMD `define([...], function (dep) {...})` / `require([...], fn)` bind
  each factory parameter to the dependency at the same position. That
  binding is local to the factory and is not an import (no IMPORTS
  edge), so the unified walk carries it like the Python parameter
  bindings: the loader call notes its factory, the factory's frame binds
  the dependency parameters, the frame's pop unwinds them. The
  CommonJS-wrapper pseudo dependencies (`exports`, `module`, `require`)
  stay unbound. O(1) per node, no ancestor walk. The JS member-call
  branch reads it into CBMCall.receiver_is_amd_dependency.

Nothing reads either fact yet; the graph is unchanged by this commit.

Signed-off-by: Martin Vogel <[email protected]>
…eceivers

Strategy 2 (same_module) looks a member callee up a second time without
its receiver, so `axios.get(url)` in a file that defines its own `get`
became a 0.90 CALLS edge to that local function. The weak-member guard
deliberately keeps same_module, so this class was unguarded (DeusData#1128).

Restore the registry strategy chain to main (review of 2026-10-19,
item 1): the chain also resolves type references, field types,
reads/writes, SQL lineage and the cross-LSP pass for every language,
and the chain-level restriction broke the C typed-field and Python
external-import pipeline tests. Instead add a call-site suppressor,
wired identically in pass_calls.c and pass_parallel.c with the language
list inside the helper: for the JS/TS family and the <script> hosts it
drops the plain-CALLS fall-through only when the receiver-dropping
lookup produced the hit AND the receiver is bound to an ES import or a
CommonJS require of the file, to an AMD dependency, or is a builtin
global of the JS runtime (console, Math, JSON, Object, process, ...).
Every other receiver keeps its edge: object-literal methods and
functions assigned onto an object are stored at module level, so
`context.push()` reaching the module's `push` is usually right. A
whole-callee hit (`search.init()` -> <module>.search.init) keeps the
receiver in the match and is never refused. Service and route edges are
unaffected.

Measured with a fresh full index, sequential and parallel identical,
runs repeatable: vuejs/core same_module CALLS 1309 -> 1304, NodeBB
1721 -> 1680, no other strategy moves and no edge is added; every one
of the 49 removed call sites was read against the source and is a wrong
edge. helm (Go) is byte-identical.

The contributor's registry cases move to the guard as positive and
negative controls; pipeline tests cover JS, TS, TSX, ArkTS, Vue,
Svelte, Astro and HTML plus CommonJS and AMD, sequential and parallel,
and fail on main for every language.

Signed-off-by: Martin Vogel <[email protected]>
Brings the branch up to main (0d835c5). Two conflicts, both the
`drop_plain_call` expression in pass_calls.c and pass_parallel.c: main
appended the external-import-shadow and PHP receiver clauses where this
branch appended the DeusData#1128 same-module bound-receiver clause. Resolved as
main's clauses followed by `||` and the DeusData#1128 clause; clang-format then
wraps main's PHP line, which the added `||` pushed past the column limit.
No other change.

Signed-off-by: Martin Vogel <[email protected]>
@DeusData

Copy link
Copy Markdown
Owner

@sahil-mangla, thank you for sticking with this one. You found a real class of wrong edge - axios.get() binding to whatever get the calling file happens to define - and your test cases pin it down precisely. As announced earlier today, we have now pushed the narrower shape onto your branch (thank you for allowing maintainer edits). Here is what each commit does and why.

  1. feat(extract): record JS/TS receiver binding facts for imports and AMD (34c64bf)
    The guard has to know, from the calling file's own source, whether a receiver is bound to another module. Two facts were missing. First, an import statement that binds no name (a side-effect import './x', a re-export, require('x').y) still records the last segment of its path as its local name, so require('node:stream') made every parameter called stream look like an import. Those entries are now marked as synthesized; the import map and the IMPORTS edges are unchanged. Second, AMD: define(['api'], function (api) { ... }) binds api to a dependency with no import statement at all. The extractor now carries that binding through its walk, scoped to the factory function, the same way it already tracks Python parameters, and records it on the call. The CommonJS-wrapper dependencies exports, module and require stay unbound, because they are the module itself. This commit on its own does not change the graph.

  2. fix(pipeline): refuse same_module for import-bound or builtin JS/TS receivers (51c57c4)
    This restores the strategy chain in registry.c to main (item 1 of the 19 October review) and adds a call-site suppressor instead, applied identically in pass_calls.c and pass_parallel.c, with the language list inside the helper: JavaScript, TypeScript, TSX, ArkTS, and the script bodies of Vue, Svelte, Astro and HTML files. A member call keeps its same_module edge unless the lookup had to drop the receiver to find the target AND that receiver is an ES import or a CommonJS require of the file, an AMD dependency, or a builtin global (console, Math, JSON, Object, Reflect, process, and a few more). Every other receiver keeps its edge, including context.push() on an object-literal method, this.get() and exports.get(). A call such as search.init() that matched <module>.search.init as a whole also keeps its edge, because there the receiver is part of the match. Your two registry tests are kept as the positive and negative controls, now at the level of the guard. One case changed sides: _get_store().get is now a positive control, because a call result carries no binding fact, so under the agreed shape it keeps its edge. New pipeline tests (sequential and parallel) cover all eight languages plus CommonJS and AMD, and two extraction tests cover the new facts.

  3. Merge of current main (9f5923c). The only conflicts were the suppression expressions in the two resolver files, where main had added two clauses next to ours; all of them are kept.

Before and after on two real repositories (fresh full index on current main; sequential and parallel indexing give identical graphs, and repeated runs are identical):

repository CALLS before CALLS after same_module before -> after other strategies
vuejs/core 6532 6527 1276 -> 1271 unchanged
NodeBB 3907 3866 1721 -> 1680 unchanged
helm (Go, control) 6720 6720 unchanged whole graph byte-identical

We read every one of the 49 removed call sites (6 in vuejs/core, 43 in NodeBB) against the source, and each one was a wrong edge. Some examples: Object.defineProperty(...) bound to a proxy handler's own defineProperty, Reflect.get(...) bound to a collection handler's get, nconf.get(...) bound to a controller's get, winston.info(...) bound to a CLI command named info, and the AMD dependency categorySelector.init(...) bound to the calling module's own init. No new caller/target pair appears. The edge count drops by slightly less than the number of call sites, because three of the pairs are still reached through another, correct call in the same function.

The new pipeline tests fail on main for every language and pass with these commits. The registry, pipeline, extraction, TypeScript LSP and complexity suites pass under ASan/UBSan.

Since we pushed to your branch, please pull before pushing again. Thank you again for your patience with this one, and for finding the class in the first place.

@DeusData DeusData changed the title fix(registry): restrict same-module and unique-name suffix matches on receivers fix(pipeline): refuse same_module for import-bound or builtin JS/TS receivers Nov 10, 2026
@DeusData
DeusData merged commit 5b87320 into DeusData:main Nov 10, 2026
26 checks passed
@DeusData

Copy link
Copy Markdown
Owner

Merged - thank you very much, @sahil-mangla, for finding this class of wrong edge and for your patience while we worked out the right shape.

A member call whose receiver is an ES import, a CommonJS require, an AMD dependency or a builtin global such as console or Math no longer gets a same_module edge to whatever same-named function the calling file happens to define, while calls on local objects keep their edges. On vuejs/core and NodeBB every one of the 49 edges this removes was a wrong one, such as Object.defineProperty(...) landing on a proxy handler's own defineProperty, and no new edge appeared. Your test cases now guard the rule as its positive and negative controls. Thanks again!

chidev added a commit to lev-os/codebase-memory-mcp that referenced this pull request Nov 14, 2026
…nt upstream

Keep the fork current while adopting upstream's Swift callable identity,
scoped inheritance, annotation lookup and initializer validation. Retain
JVM overloads, barrel resolution, secret-file exclusion, receiver filtering,
bounded MCP refusal and fork-only CI. Pass every new policy/import/ranking
field through the retained code paths. Keep both parents' behavioral tests.

The namespace-export and qualified Swift annotation regressions were red
before their integration fixes. Upstream's super.init regression also caught
an early rewrite that bypassed its validator; delegation now owns that case.
The SwiftUI fixture declares its intended shared target, preserving upstream
extension visibility rather than restoring cross-module name guessing.

Unwind table (all 36 non-merge fork patches):
c93b82e | kept as is | Explicit alias extensions are stripped once; upstream still double-strips dotted stems.
ada9aa8 | kept as is | Non-JS aliases retain module/file identity and bundler suffix handling.
4b2f1bd | kept as is | Fork skill location and compatibility symlink remain local.
3b3955d | kept as is | Fork-management skill remains local.
b053dae | adapted | Keep five-second refusal within upstream endpoint/frontend lifetime.
5d2a30b | adapted | Keep shared-barrel complexity counters alongside upstream Swift extension populations.
679ea8d | kept as is | Keep member/alias, long-chain, parallel and incremental barrel regressions.
3021f2a | adapted | Shared barrel registry composes with upstream Java registry and delta/test metadata.
30fe44a | kept as is | Re-export records still bind no local name; upstream DeusData#1128 adds a separate bound-receiver guard.
24b79f2 | adapted | Keep the MCP refusal responder; release endpoints only on their owning return paths.
274a68a | adapted | Retained tests use upstream Swift signature identities.
9baadf3 | adapted | Keep proxy re-export edges; unwind duplicate supertypes JSON now covered by upstream 38df8e5/6eddf54a.
dd89a93 | adapted | Retain formatting intent while composing current upstream interfaces.
0b9d75f | adapted | Keep cached casts/non-view chains; unwind annotation rewriting covered by upstream a9ae13e and DeusData#2061.
25d2897 | adapted | Keep JVM overload proxy shapes; upstream d8a845e/DeusData#2061 owns Swift signatures/defaults.
6541132 | kept as is | JVM doc links still follow fork-split overload nodes.
e6308a0 | adapted | Filter before ranking, passing upstream language and caller-test flags; f3bcb25 still picks then rejects.
c5c8070 | unwound | Upstream d33c6ac/38df8e5a/6eddf54a resolves typed inherited members with module scope; graph regressions retained.
acb98db | adapted | Keep cast/super/view spelling; upstream 71ab015 owns super.init validation; preserve scoped SwiftPM extensions.
c6e8672 | kept as is | Kotlin external receivers remain protected; upstream 8081009 is Java-only.
8782ce7 | adapted | Keep Java/Kotlin split identities; unwind Swift machinery covered by upstream d8a845e and DeusData#2061 follow-ups.
cb1e370 | kept as is | Kotlin companion methods remain graph nodes under the enclosing class.
844751a | adapted | Prefer the caller's Java module while retaining upstream proven-external targets.
0521d82 | kept as is | Per-name re-export lookup remains in the fork's later shared-registry implementation.
4c16d9e | kept as is | Fork release skips only macOS LeakSanitizer.
bceebbd | kept as is | Namespace-import calls still follow re-exported members.
b6e807d | kept as is | jsconfig paths fallback is not supplied by upstream.
26e6e9d | adapted | Java build-root selection composes with upstream LSP receiver checks in both resolver paths.
cf70a88 | kept as is | lev remains a prerelease-only fork workflow.
901ac86 | adapted | Preserve secret policy; unwind hard-coded worker-scope object in favor of upstream a45858f's shared helper.
7912fdd | adapted | Keep secret exclusion; upstream de16152 adds resource limits but no equivalent secret policy.
9ee111c | adapted | Keep named/star re-exports; set upstream local_name_synthesized and preserve source-only export dependencies.
60d96ab | kept as is | Dotted aliases and constructor-property fixtures remain; upstream alias path still uses fqn_module.
0274f69 | kept as is | Keep barrel-folder/index module resolution.
ff54949 | kept as is | Fork upstream-sync workflow remains local.
c8e7a3f | kept as is | Bare external JS/TS imports cannot name-guess project symbols; retain workspace source/subpath exports.

Validation covers the complete compiled suite list. The long Git group was
split below the ten-minute ceiling; its exact 77-case union is verified.
One unrelated upstream test remains red: regex_compile_refuses_overlong_pattern
expects success at 32768 bytes, but macOS returns 12. The exact upstream test
body and implementation from 0e4d160 reproduce it. Upstream vendor patch
whitespace is unchanged; the fork diff against upstream passes diff --check.

Constraint: Local merge only; shared heavy gate; no sanitizer or scripts/test.sh leg
Tested: 197 suites, 10515 passed, 1 proven upstream failure, 12 skipped
Tested: Isolated worker scope and six-second MCP conflict refusal
Not-tested: Release build, sanitizers, hosted CI and Windows execution
Directive: When upstream enables JVM callable identity, retire the matching fork splitter before changing qn_sig_off handling
Confidence: high
Scope-risk: broad
Signed-off-by: chidev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants