Skip to content

fix(extraction): a TS/JS call through a host-global chain emits no ref (#1707) - #1710

Open
bompus wants to merge 1 commit into
colbymchenry:mainfrom
bompus:fix/chained-receiver-drop
Open

fix(extraction): a TS/JS call through a host-global chain emits no ref (#1707)#1710
bompus wants to merge 1 commit into
colbymchenry:mainfrom
bompus:fix/chained-receiver-drop

Conversation

@bompus

@bompus bompus commented Sep 5, 2026

Copy link
Copy Markdown

Fixes the host-namespace half of #1707.

The bug

extractCall keeps a receiver only when it is a plain identifier. Every deeper chain collapses to the bare method name, which then exact-matches whatever project symbol shares it. In a Chrome extension that means every chrome.storage.local.get(key) inside a storage wrapper binds to the wrapper's own get — a self-edge that is not in the source, plus a fabricated caller for every other call site.

What this does

Emit nothing when the receiver is a member/subscript chain whose root identifier is a host object a project never declares: chrome, browser, document, navigator, performance, console, localStorage, sessionStorage, indexedDB, crypto, globalThis, process, Math, JSON, Object, Array, Reflect, Promise, Intl. A chain rooted there ends in a platform API, so the bare name could only ever match an unrelated symbol. Same trade the literal-receiver gate already makes (#1230): a silent miss instead of a wrong edge.

window is deliberately not a host root — window.MyNamespace.doThing() reaches a project symbol.

Why it is this narrow

I first tried the general version: drop the ref for every receiver the extractor cannot name, keeping only a plain identifier and this.<field>. It kills the bug and costs far too much. On a 588-file extension it removed 64 correct project edges, and it broke 10 tests in this repo's own suite:

  • object-literal-methods — resolves callers of store actions across files (destructured + chained getState())
  • ui-steps-api-servers (3), ui-steps-cross-tier (2), nextjs (2), react-native-bridge (1)

The real-world losses had the same shape: window.MyNs.doThing(), Vue's intelligence.value.evaluatePlayer(p), obs.liveSources.ingest(row). The bare-name fallback is load-bearing for chained receivers, so a blanket drop is not the fix.

Measured

Same repo, same baseline index, line-insensitive call-edge diff (keyed on source/target/refName with multiplicities, so a call that only moved lines is not counted as a loss):

before after
fabricated calls edges 153 0
real edges lost 0
false self-edges 3 1
import-resolved edges 4,797 4,797
instance-method edges 498 498
suite 4,052 pass / 32 env-known 4,052 pass / 32 env-known / 0 fresh

The 32 known failures are environmental on this host (Windows) and identical before and after.

Test

__tests__/ts-chained-receiver.test.ts — fails without the change, passes with it:

  1. a storage wrapper's get does not call itself through chrome.storage.local.get
  2. document.body.querySelector(s) does not bind to a project function named querySelector
  3. a chain rooted at a project value still resolves — window.MyNs.ping() and this.<field>.ping()

Scope left open

this.map.get(k) and rows[0].text() still bare-name match. Those need the receiver's type, which is #1496's job. The host-global gate is the subset fixable without one.

Caveat on the Rust side

The kernel's extract_call (tsjs/extractors.rs) mirrors the gate, which it has to since TS/JS is routed to the kernel by default. That half is unverified: my host has no cargo toolchain and no prebuilt .node, so only the TypeScript path was executed. Please build and run the kernel tests before merging, or tell me and I will drop the Rust hunk into a follow-up.

@bompus

bompus commented Sep 5, 2026

Copy link
Copy Markdown
Author

The Rust half is now verified. I installed a toolchain and built the kernel, so the caveat in the description is resolved.

Setup: cargo build --release via scripts/build-kernel.sh on Windows x64, MSVC, rustc 1.98.1, staged at codegraph-kernel/prebuilds/win32-x64/codegraph-kernel.node. kernel-grammar-parity passes 19/19, so the locally built grammars match the vendored wasm.

I verified by ablation rather than by the parity suite, because it isolates this change:

Kernel binary ts-chained-receiver.test.ts
built with the hunk 3/3 pass
rebuilt with only the hunk disabled (if false && self.is_host_global_chain(r)) 2 fail, exactly the wasm-path failures

The TypeScript side was byte-identical in both runs and CODEGRAPH_KERNEL_EXPECT=1 was set, so the kernel is provably what executed and the Rust gate is what produces the behaviour. The two failures in the ablated run are the same two assertions that fail on the wasm path without the fix: the storage wrapper calling itself through chrome.storage.local.get, and document.body.querySelector binding to a same-named project function.

One thing worth flagging for anyone reproducing this: I could not use kernel-tsjs-parity.test.ts as the gate, because it fails on my branch for reasons unrelated to this PR. My fork carries two extraction features that exist only in the TypeScript extractor and were never mirrored into the kernel — a literal-seeding pass of my own, and interface-member indexing from #1638. Both apply to routed languages, so kernel and wasm legitimately disagree on my branch. On a clean main that suite should be the gate, and I would expect it to pass, since this change is a pure subtraction of refs that the wasm side also stops emitting.

Happy to squash, split the Rust hunk out, or adjust the host-global list if you would rather it were narrower or configurable.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Map of my seven open PRs — posting it here because this is the one with an ordering constraint

Seven of these are open and unreviewed, and they're mostly independent, so I don't want the pile to read as one big thing to work through. Here's the map, smallest first, and then the only real dependency — which involves this PR, which is why the comment is here rather than scattered across all seven.

Independent, small, no ordering constraints:

PR size what it fixes
#1695 +7/-1 prompt-hook injection exceeded Claude Code's 10k inline hook-output limit — 16,360 → 9,348 chars. Independently reproduced and confirmed by @danusha2345.
#1697 +86/-6 codegraph_explore was being deferred rather than eagerly loaded in Claude Code and Copilot CLI. @danusha2345 verified alwaysLoad over stdio.
#1706 +169/-1 an import naming the emitted .js extension now resolves to its .ts source
#1709 +78/-1 fuzzy matching no longer links a reference to a nested function it cannot reach

#1695 and #1697 are the two I'd merge first — they're tiny, they're user-visible in Claude Code, and they each have a second pair of eyes on them already.

Larger, still independent:

PR size what it adds
#1699 +2134/-33 markdown index (#361) with a section-first doc tier
#1702 +947/-20 codegraph sessions / codegraph_sessions — search a project's earlier agent transcripts
#1710 +5990/-223 a TS/JS call through a host-global chain emitted no ref (#1707)

The one dependency worth knowing about. This PR (#1710) and @maxmilian's #1686 touch the same two files: src/extraction/tree-sitter.ts and — for #1686's kernel half — codegraph-kernel/src/tsjs/extractors.rs. Whichever lands second wants a rebase; they don't conflict semantically.

More important than the ordering: #1686 needs a kernel half to land with it, not after it. It indexes TypeScript interface members on the TS/wasm path only, and the kernel is what DEFAULT_ROUTED selects for typescript/tsx — so on its own it moves scripts/kernel-parity.mjs from 452/626 to a wider gap (2,386 property and 1,174 method nodes present in wasm, absent in the kernel), and #1686's own new test fails on a kernel build. I ported it in maxmilian#1, which takes parity to 619/626. That's for @maxmilian to pull in, but it's the thing to check before #1686 merges.

All seven are MERGEABLE with no conflicts. Nothing here is blocked on me — I'm not waiting on an answer to any of them, so if the useful thing is to merge the two small ones and leave the rest, that's a fine outcome. Happy to split, rebase, or close any of them on request.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Correction to the map above: #1709 is now a draft — don't merge it.

I ran it against vitejs/vite after posting that comment. It removes 12 false-positive edges, all correct, and adds 59 new wrong ones (52 calls, 7 imports) — import { resolve } from 'node:path' in the playground configs now links to pluginContainer.ts:resolve, because filtering the unreachable nested candidates leaves exactly one survivor and the matcher commits to it without checking the binding is internal. Detail and the fix direction: #1709 (comment).

#1699 also picked up a real-repo finding — the markdown index itself is solid on vite, but several natural doc queries, including a bare CONTRIBUTING.md, return no markdown at all. It's still fine to review; the note is on that PR.

The rest of the map stands: #1695 and #1697 are still the two I'd merge first.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Real-repo run: the commit itself is clean — but this PR is stacked on #1709, which is not

Ran this against vitejs/vite, with the Rust kernel built from each arm's own sources — this PR touches codegraph-kernel/src/tsjs/extractors.rs, and DEFAULT_ROUTED sends typescript/tsx/javascript/jsx to the kernel, so a wasm-only run would measure a path users don't take. Confirmed the kernel was loaded, not silently skipped.

Because the branch is 47 commits off main, the comparison is 8993c83 against its own parent df88e20, not against main. Anything wider would credit this PR with everyone else's changes.

The host-global commit: 10 false positives removed, 0 added

Edges 31,002 → 30,992. Ten lost, none gained, and every one is a call through a host global that was previously bound to a same-named project function:

call site what it really is had been linked to
build.ts:1123 process.stdout.clearLine(0) build.ts:clearLine
utils.ts:1928 process.stdin.on('end', …) shared/hmr.ts:on
utils.ts:1941 process.stdin.off('end', …) shared/hmr.ts:off
cli/__tests__/serve.ts:29-30 process.stdout.on('data', …) shared/hmr.ts:on
ssr-html.spec.ts:139 process.version.match(/^v(\d+)\./) a match in importAnalysisBuild.spec.ts

process.stdin.on resolving to vite's HMR emitter is the kind of edge that quietly pollutes a call path, so removing ten of them with nothing gained is the result I wanted.

Worth noting for parity: at main, the kernel and wasm arms produce identical graphs on vite — 9,354 nodes, 27,778 edges either way.

But do not merge this as it stands

This branch carries #1709's change verbatimdf88e20 is that commit, and isLexicallyReachable is present in src/resolution/name-matcher.ts at this head. I measured #1709 separately and it adds 59 wrong edges on vite (52 calls, 7 imports): import { resolve } from 'node:path' in the playground configs gets linked to pluginContainer.ts:resolve. Same numbers on a kernel-extracted graph as on wasm, so it isn't a routing artifact.

It also carries 35efb48, which is #1706 — that one measured clean (one edge gained, zero lost).

So merging this today would bring in #1709's regression along with this fix. I've put #1709 back to draft. This one should be rebased off df88e20 — or wait for the fixed #1709 to land first.

Correction to my stack map above

I wrote that #1710 and #1686 are independent and would only "want a rebase". That was wrong about the dependency that matters: #1710 is stacked on #1709 and #1706, so it is not independently mergeable at all. I should have checked the branch composition before writing the map rather than reading the PR list at face value.

@danusha2345

Copy link
Copy Markdown
Contributor

Reviewed the host-global-root change itself and it is the right cut: a chain rooted at chrome / document / Object ends in a platform API, so dropping the ref is the same trade the literal-receiver gate (#1230) makes, and leaving window out is correct. It is also complementary to #1692 rather than overlapping — that one keeps a call receiver (d.setdefault(k, []).append(v), make().run()) in both arms, this one handles a member chain with a host root; neither covers the other's shape.

One thing keeps me from taking it into the local integration build: the branch is stacked on the rest of the fork. Against main it is 47 commits / 69 files (+5,990), carrying the markdown index, codegraph sessions, the #1686 port and its kernel changes, so the host-root fix cannot be merged, bisected or reverted on its own. The fix proper is a handful of lines in extractCall plus the kernel mirror in tsjs/extractors.rs and a torture.tsx line. If you rebase just that onto main I will re-verify on my repos and take it; as it stands I can only review the diff, not run it standalone.

`chrome.storage.local.get(key)` and `document.body.querySelector(s)` end in
a platform API, but the extractor emitted the bare method name for them. That
name then exact-matched whatever project symbol shared it: in a Chrome
extension every `chrome.storage.local.get/set` inside a storage wrapper bound
to the wrapper's own `get`/`set`, giving two self-edges that are not in the
source and 153 fabricated call edges (colbymchenry#1707).

A member chain whose root identifier is a host object the project never
declares now emits nothing — a silent miss instead of a wrong edge, the same
trade the literal-receiver gate makes (colbymchenry#1230). `window` is deliberately not a
host root: `window.MyNs.doThing()` reaches a project symbol. A chain rooted at
a project value keeps the bare name, so `store.getState().act()`, `ref.value
.m()` and `this.<field>.m()` are untouched.

Measured on a 588-file extension: 153 false edges removed, no real edge lost
(line-insensitive edge diff), false self-edges 3 -> 1.

The Rust kernel mirrors the same gate. It is unverified: this host has no
cargo toolchain and no prebuilt .node, so only the TS path was executed.
@bompus
bompus force-pushed the fix/chained-receiver-drop branch from 8993c83 to 04320a0 Compare September 6, 2026 07:34
@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Rebased — the branch is now just the fix, on top of main.

fix/chained-receiver-drop is one commit (04320a0), 3 files, +176/−0, and GitHub reports the PR mergeable:

  • src/extraction/tree-sitter.ts (+53)
  • codegraph-kernel/src/tsjs/extractors.rs (+32, the kernel mirror)
  • __tests__/ts-chained-receiver.test.ts (+91)

The markdown index, codegraph sessions, the #1686 port and the rest of the fork are gone from the diff; it bisects and reverts on its own now.

One correction to your scoping guess: there is no torture.tsx line. The change carries its own dedicated test file instead, which is the +91 above (three cases: a chrome.* chain drops, a document.* chain drops, a project-rooted chain still keeps the bare name).

Verified before pushing, same runner both ways on a clean clone, upstream/main as the baseline:

test files passed failed
main 236 3105 894
main + 04320a0 237 3108 894

The delta is exactly the new file and its three tests, and the failure count is identical. Those 894 are pre-existing on untouched main on this host, not something the commit introduces — 672 are EBUSY: resource busy or locked, rm on Windows temp dirs and most of the rest are process.chdir() is not supported in workers, i.e. my runner, not the tree. Worth knowing that the suite is not green on Windows for reasons unrelated to this PR; I did not chase it here. The new test file on its own passes 3/3.

The kernel caveat from the commit message still stands: this host has no cargo toolchain and no prebuilt .node, so tsjs/extractors.rs is a mirror of the TS gate that I have not executed. That is the piece worth your re-verification with the native kernel on.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Correction to my previous comment, and to the commit message: this host does have a Rust toolchain. I have now built the kernel and executed the mirror.

The caveat was true when the commit was written and I repeated it without re-checking. cargo/rustc 1.98.1 on stable-x86_64-pc-windows-msvc were installed 30 minutes after 04320a0 was authored; ~/.cargo/bin is not on my shell's PATH, so a naive command -v cargo still reports nothing, which is how the stale line survived into the comment. Sorry for sending you to re-verify something I could have verified myself.

What the kernel actually does with the gate. scripts/build-kernel.sh builds clean here in 41s and stages a 34M prebuilds/win32-x64/codegraph-kernel.node; the loader confirms it is the arm in use, with TypeScript among the routed languages:

[codegraph-kernel] loaded ...\prebuilds\win32-x64\codegraph-kernel.node
  (languages: typescript, tsx, javascript, jsx, java, python, go, c, cpp, rust, ...)

Since TS/JS is in DEFAULT_ROUTED, staging the .node makes the kernel the default arm with no env var — which also means my earlier numbers in this thread were the wasm path throughout. Re-run both arms, upstream/main rebuilt from source as the baseline each time:

arm tree test files passed failed
kernel main 236 3273 899
kernel main + 04320a0 237 3276 899
wasm main 236 3105 894
wasm main + 04320a0 237 3108 894

In both arms the delta is exactly the new test file and its three tests, and the failure count is unchanged. __tests__/ts-chained-receiver.test.ts passes 3/3 on the kernel arm specifically, so the is_host_global_chain walk behaves the same on the kernel's nodes as the TS tsJsChainRoot walk does on tree-sitter's. I also diffed the two root lists directly: HOST_GLOBAL_ROOTS in tsjs/extractors.rs and TS_JS_HOST_GLOBAL_ROOTS in tree-sitter.ts are the same 19 identifiers in the same order.

So the "unverified mirror" caveat in the commit message no longer holds, and the PR does not need that re-verification from you.

One thing that is not about this PR: the suite is not green on Windows on either arm, including on untouched main, so read the table above as deltas and not as absolutes. 679 of the 899 failures are EBUSY: resource busy or locked, rm on mkdtemp directories and most of the rest are process.chdir() is not supported in workers; the failure-class histogram is identical on main and on this branch. __tests__/c-fnptr-kernel-sweep.test.ts is the one file that fails on the kernel arm but not the wasm arm, and that is only because it skips without a staged .node — every one of its failures is EBUSY, none is an assertion, and it fails identically on baseline main. I am looking into those Windows failures separately and will raise them on their own rather than here.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Retraction: the "899 failures" table above is wrong. The real number is 23, and all three causes were mine.

The test-results table in my earlier comment reported 899 failures on the kernel arm and 894 on wasm, on main as well as on this branch. Those numbers are worthless. They measured my own broken setup, not this repository. Nobody should act on them, and I'm sorry for putting a number that alarming on a public thread without earning it first.

The actual figure on an untouched upstream/main on this box, run correctly, is 23 failures — and every one is a Windows-only test-teardown problem, not a product defect.

What I did wrong — all three, in order

1. I ran the entire suite under Bun. engines.node is >=20.0.0 <25.0.0; this project asks for Node and I gave it something else. The result was a flood of EBUSY errors that had nothing to do with any code under test.

To be precise about why, because I want to be fair to Bun here: this is not Bun faking node:sqlite. Bun 1.4.2 implements it properly. The defect is narrower — DatabaseSync.close() does not finalize outstanding prepared statements, so the SQLite file handle survives the close and Windows then refuses to remove the file. exec() alone is fine; prepare() is the trigger; forcing a GC before close() releases it, which pins the mechanism to unfinalized statement handles. A control with plain files, explicit fds and nested directories removes cleanly under Bun, so it is not fs.rmSync and not Windows generally.

That is a known, vendor-reproduced bug: oven-sh/bun#40001, with fix PR #40005 still open, which is why a current 1.4.2 still reproduces it. Note also that bun:sqlite is unaffected — its close(true) finalizes statements and is documented — so this is specific to the node:sqlite compatibility surface, where Node's close() takes no argument and there is no escape hatch.

None of which excuses the run. Every test in this suite that prepares a statement was going to fail, and choosing an unsupported runtime is what put me there.

2. I forced --pool=threads. vitest was dying under Bun, so I overrode the pool to get it running — against a config that deliberately leaves the default at forks. That manufactured 9 more failures reading process.chdir() is not supported in workers. The override was me working around error #1, and it invented a category of failure that does not exist in a correct run.

3. I skipped npm run build. The documented order is build:kernelbuildtest. I ran the first and the third. Every test that spawns dist/bin/codegraph.js had no dist/ to spawn, which accounts for roughly 67 failures across the CLI and MCP suites.

Stack those together and you get 899. Remove them and you get 23.

I compounded it by presenting the numbers in a tidy four-row table with a main baseline, which made the result look controlled. The baseline was broken in exactly the same three ways as the branch arm, so the two columns agreeing told me nothing and I read it as confirmation.

What this does and does not change about this PR

It does not change the finding this PR rests on. The host-global measurement — vite, 31,002 → 30,992 edges, ten false positives removed and none added — was a graph diff, not a test run, and it reproduces. The kernel-vs-wasm parity check on main (9,354 nodes / 27,778 edges either way) also stands.

What it changes is that my "899 failures on main too" line should never have been read as "this tree is broken." It isn't. It's 23, they're all Windows teardown, and they're present on a clean checkout.

The 23, and a fix

They fall into two causes, both Windows-only and both invisible on CI because POSIX unlinks an open file happily where Windows refuses:

  • An open handle across fs.rmSync — a CodeGraph (or, in a couple of places, the test's own second DatabaseConnection.open) still holding the database when the temp tree is removed.
  • An unawaited childchild.kill() returns before the process is gone, and Windows will not delete a live process's cwd.

I have a test-only branch that fixes both and I'll open it as its own PR shortly. I'm still chasing the last of it: the MCP suites can spawn a detached daemon, which is not a tracked child and has no exit event to wait on, so a few teardowns still intermittently lose a race with the OS releasing its files. I'd rather send that when it's actually clean than repeat today's mistake of publishing a number I haven't earned.

Separately, and for the same reason: measurements I post from this box should be assumed to be from a Windows host running Node 26.8.1, which is outside the supported range and which the CLI correctly refuses. Where that matters to a result, I'll say so up front from now on rather than after someone asks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants