fix: resolve URL-builder helper calls to Route/HTTP_CALLS

[CharlesQueiroz]

Fixes #1009. Stacked on #1008 (includes its commit; rebases cleanly once #1008 lands).

Root cause

URL-shaped literals returned from small builder functions never reach a call argument, so client(buildPath(id)) had no first_string_arg: no HTTP_CALLS edge, no Route node, static and template literals alike. lookup_string_constant only covers module-level consts.

Fix

• handle_url_builders() (extract_unified.c, runs in the unified walk): when a return statement (or an arrow-function expression body) yields a URL-shaped literal, records builderName -> url in the same per-file constant map used for module-level consts. Ambiguous builders (two different URLs for one name) are tombstoned so lookups miss instead of guessing.
• extract_url_or_topic_arg() (extract_calls.c): a call_expression argument whose callee is a plain identifier resolves through that map, giving the call its first_string_arg; the existing pipeline then mints the Route node and the HTTP_CALLS edge from the real HTTP caller (the hook), not from the builder.
• Composed builders: the builder-body template flatten is map-aware: a ${...} substitution that is a bare identifier or a call to an already-recorded name inlines that value, everything else becomes {}, and the result is truncated at the first ? (query strings are not part of a route's identity). This covers the real-world TanStack shape:

function activityPath(id: string) { return `/api/v1/team-members/${id}/activity` }
function buildPath(id: string, cursor: string) {
  const params = new URLSearchParams()
  return `${activityPath(id)}?${params.toString()}`
}
apiFetch(buildPath(id, cursor))   // -> HTTP_CALLS -> /api/v1/team-members/{}/activity

Same-file scope, document order (same constraints the const map already has).

Validation

• New regression tests extract_ts_url_builder_issue1009 (return + arrow bodies) and extract_ts_url_builder_composed_issue1009 (composition + query truncation).
• Full suite: 5988 passed, 1 skipped, 0 failures.
• End-to-end fixture: queryFn -HTTP_CALLS-> /api/v1/team-members/{}/activity now exists for the builder shape, joining the server-side __route__GET__/api/v1/team-members/{}/activity exactly.

[CharlesQueiroz]

Pushed a follow-up commit: detect_url_in_args() (the arg_url HTTP_CALLS emitter used when the callee is a local client wrapper rather than a known HTTP library) reads per-arg values, not first_string_arg. The builder-map resolution and template flattening are now applied there too, so wrapper clients like apiFetch(buildPath(id), opts) emit the edge. Validated end-to-end on a real TanStack codebase: the parameterized hooks now produce HTTP_CALLS -> /api/v1/team-members/{}/activity, joining the server route exactly. Suite still 5988 passed / 0 failed.

[DeusData]

Thanks for sending this follow-up. I have triaged it into 0.9.1-rc as the fix candidate for #1009. Review focus will be the stacked dependency on #1008, builder ambiguity handling, and avoiding speculative cross-repo routes that could create false positives.

[CharlesQueiroz]

Rebased onto the updated #1008 with sign-offs on both commits; no content changes. Full suite passes locally on the stacked branch.

[CharlesQueiroz]

Rebased the stack onto the updated #1008, which now sits on current main. The only conflict was the adjacent handler registration from another merged PR in extract_unified.c, resolved keeping both. Full suite on the stacked branch: 6360 passed, 1 skipped, 0 failures. lint-format clean. Once #1008 lands this rebases onto main trivially.

[DeusData]

Reviewed in depth. The mechanism is genuinely well built and the direction is right — but it needs one gate before it can merge, because as written it fires on C and Go too.

What is good, and I want to be specific. The ambiguity tombstone — two different literal URLs turning a builder into a deliberate miss rather than a guess — is exactly the right instinct for this codebase. So is truncating at ? for route identity, and reusing the existing per-file constant map instead of inventing a new resolution subsystem. The composed-builder inlining solves the real TanStack pattern from #1009 end to end, and keeping it same-file and document-order means there is no multithreading or cross-file ordering hazard. All three tests are binding and fail without the fix.

I also checked the things that usually go wrong here and found them handled: Route QNs are deterministic (__route__ANY__<canon> upserted by QN, so two call sites reaching one route produce one node, not two), resolution terminates trivially because builder_template_text only inlines names already recorded earlier in document order, fully-dynamic templates fail the / gate and are declined rather than guessed, and there is no quadratic pattern — the map is per-file and capped.

The blocker: handle_url_builders has no language gate.

The predicate accepts any absolute pathname, not just routes — url[0] == '/' plus cbm_classify_string. And the handler runs in the unified walk for every language: return_statement + string_literal covers C and C++, interpreted_string_literal covers Go, and both use call_expression at the resolution points. Only the tests are TypeScript.

So on a C codebase:

static const char *cfg_path(void) { return "/etc/myapp/conf.d"; }
...
parse_config(cfg_path());

cfg_path gets recorded as a URL builder, the new call_expression branch sets ca->value to /etc/myapp/conf.d, and the pre-existing detect_url_in_args — which runs unconditionally for every emitted call — accepts it and mints a Route node __route__ANY__/etc/myapp/conf.d plus an HTTP_CALLS edge. A filesystem path is not an HTTP route.

That shape is everywhere in exactly the C codebases we showcase: /etc/…, /proc/self/…, /dev/…, /sys/… returners. It would show up in the kernel benchmark and in cbm's own self-graph. There is some pre-existing exposure for literal arguments (open("/etc/x/y")), but this extends it to the far more common path-returning-helper shape, which is a real change in false-positive class for non-web languages.

Under our rule that a wrong edge is worse than a missing one, that is a request-for-changes rather than a nit.

The fix is small: gate the recording (and ideally the call_expression resolution branches) to CBM_LANG_JAVASCRIPT / TYPESCRIPT / TSX — which is what the issue, your description, and your tests all already assume. All of the value, none of the blast radius.

Two narrower false-positive shapes worth fixing while you are there:

1. Mixed literal/computed builders. The tombstone only fires on two literal returns, so:

   function pathFor(kind: string) {
     if (kind === 'user') return '/api/users';
     return computePath(kind);      // non-literal — does not tombstone
   }

   Every call site, including ones taking the computed branch, gets attributed to /api/users. Tombstoning when a function has any non-literal return would close it.
2. Builders resolving as function references. The entries live in the same map the pre-existing identifier branch consults, so ids.map(thingPath) resolves the reference to the URL string and mints an HTTP_CALLS edge from whatever does the mapping. Building a URL is not making a request, and callbacks and route tables are idiomatic JS. A separate map, or a flag so only call_expression arguments resolve builders, would fix it.

Related and smaller: recording keys on the bare last QN component means a class method path() and an imported module-level path can capture each other.

On tests: the three you added are good. What is missing is the negative side, which for a feature whose entire risk is false positives is the side that matters — nothing asserts that a helper returning a non-URL string does not populate first_string_arg, and nothing asserts the tombstone actually misses. A non-TS case would also lock in the gate once you add it.

One thing you should know about, which is not a defect: this PR physically contains #1008's commit a029b3a — the same SHA as that PR's head. Merging this merges #1008 as well. Your combined tree is green so there is no hidden red, but that is a decision we want to make deliberately rather than as a side effect, and I have noted it on #1008 too.

Add the language gate and a negative test or two, and I would like to land this — the underlying idea is right and the implementation quality is high.

[DeusData]

Re-checked this while working through the backlog, and I can now confirm the cross-language concern from the July review as a verified finding rather than a suspicion.

handle_url_builders() is wired unconditionally into cbm_extract_unified()'s per-node dispatch. There is no CBM_LANG_TYPESCRIPT/CBM_LANG_JAVASCRIPT check anywhere in the new path, so it fires on return_statement/arrow_function nodes for every vendored grammar — all 158 of them.

The concrete failure that produces: a C or Go helper returning any absolute pathname — /etc/..., /proc/..., a config path — that then reaches a call_expression argument gets misclassified as a Route node with an HTTP_CALLS edge. That's a fabricated edge in a language that has nothing to do with HTTP, and it would look entirely plausible in the graph.

Your three tests are good and genuinely binding — they assert first_string_arg values the old code could not produce. What's missing is the negative direction: a non-JS/TS fixture returning a path-shaped string, asserting no Route node appears. That test is what would have caught this, and it's the one I'd most want before this lands.

So the asks from the July review still stand, and the first is now confirmed rather than precautionary:

1. Gate the dispatch to JS/TS. Confirmed necessary.
2. Negative test for the cross-language case.
3. The two narrower shapes previously raised (mixed literal/computed builder tombstoning, builder-as-callback misattribution).

The branch also conflicts with main now, and its tree still includes #1008's commit verbatim.

None of this is a rejection — resolving URL-builder helpers to Route edges is a real gap and your approach to it is sound. It just cannot land while it can invent HTTP routes in C. If you'd rather hand it off at this point, say so and I'll finish it with you credited as co-author.

[DeusData]

A correction to my own review, in your favour — and a smaller remaining scope than I described.

The example I gave you was wrong. I said a C helper returning something like /etc/myapp/conf.d would mint a fabricated Route. It would not: is_filesystem_path (internal/cbm/helpers.c:1570) already rejects /usr/, /bin/, /etc/, /var/, /tmp/, /opt/, /home/, /dev/, /sys/ and /proc/ before is_rest_path ever sees the string, and that guard predates this PR. If you had gone looking for that case you would have found the code already handles it, and wasted the trip.

The underlying concern is still real, just narrower than I made it sound. That prefix list is a fixed ten. An absolute path outside it — /srv/app/data, /data/exports, /mnt/share/x, /run/state, /media/…, /config/… — still passes the classifier, and handle_url_builders is wired unconditionally into the per-node dispatch in extract_unified.c with no language check. is_string_node covers C/C++/Rust string_literal and Go interpreted_string_literal, and return_statement exists in all of those grammars. So a Go or Rust helper returning /srv/cache/objects would be recorded as a URL builder and mint a Route node plus an HTTP_CALLS edge that describes nothing real.

A wrong edge is worse than a missing one here, so the JS/TS language gate is still the fix I want — I just want you chasing the actual failure shape rather than the one I invented.

Two things are now easier than when I last wrote.

1. The stacked-commit problem has resolved itself. I flagged that this PR physically contains fix: flatten JS/TS template-literal URLs to {} placeholders #1008's commit, so merging one merged both. fix: flatten JS/TS template-literal URLs to {} placeholders #1008 landed on 31 July, and since this repo never squashes, a029b3a6 is now an ancestor of main — a rebase simply drops it. What remains is +244/−0 across three files: extract_calls.c, extract_unified.c and the two builder tests. The helpers.c/helpers.h changes are already on main under your name.
2. One of your three tests is already merged too. extract_ts_template_string_url_issue1006 lives at tests/test_extraction.c:3376 on main. Only the two builder tests are new.

The gap itself is confirmed still open. handle_url_builders, builder_template_text, record_url_builder and url_builder_literal_text have zero occurrences anywhere in internal/cbm/, and neither URL-arg resolution site has a call_expression branch — so client(buildPath(id)) still yields no first_string_arg, no edge, no Route. #1009 is still open.

I also re-confirmed the thing I liked most about the design: you never mint a Route or an edge yourself. The change stops at populating first_string_arg during extraction, and the existing route_edge_visitor in pass_route_nodes.c does the minting with its canonical deterministic QN. That is why two call sites reaching one route upsert a single node, and it is the reason this integrates rather than duplicates.

So the remaining list is: gate the dispatch to JS/TS; add a negative test with a non-JS/TS fixture returning a path-shaped string outside those ten prefixes, asserting no Route appears; address the two narrower false-positive shapes (a builder with one literal and one computed return does not tombstone, so every call site is attributed to the literal; and a builder entry in the shared map means a bare function reference used as a callback resolves to the URL); and rebase.

You have been quiet for five weeks and I offered yesterday to finish this with you credited as co-author. That offer stands and there is no wrong answer — but I would rather have a "go ahead" or a "not for me" than keep guessing. If I do not hear back, I will take the hand-off route and credit you, because the fix is worth having and #1009 has been open a long time.

[CharlesQueiroz]

All three asks are in, plus the rebase onto current main.

The dispatch is gated: handle_url_builders returns immediately unless the file is JavaScript, TypeScript or TSX. Rather than leave the resolution side trusting that one gate, builder entries now carry their own flag in the per-file constant map, so lookup_string_constant skips them and only a call_expression argument resolves one. A language that records no builders has nothing to resolve, and handing a builder to a callback no longer inherits the route.

Mixed builders are declined rather than tombstoned. A builder is recorded only if every return in the enclosing function yields a route-shaped literal, so the outcome does not depend on the order the returns appear in, and no map entry is spent on a function that will never resolve. That same flag also stops a builder from taking over a name a module-level const already owns, which closes the collision half of your last-QN-component note.

Five negative tests: the C helper returning /etc/myapp/conf.d, a builder mixing a literal return with a computed one, the ambiguity tombstone, a builder handed to map, and a helper returning an ordinary string. The helper they share checks both mechanisms the resolution feeds, first_string_arg and the per-arg value, since a false positive in either fabricates the Route node. I confirmed they have teeth: with the guards reverted and this same test file, all five go red and the two positive tests stay green. The tombstone and URL-shape cases were proven by mutation instead, because reverting leaves them passing vacuously.

[CharlesQueiroz]

The one red check, test-lsan-macos, is not this change. It failed on an assertion in watcher_unwatch_drains_pending_free: the poll after appending to a tracked file saw no index callback, so the count was zero where the test wants one. The leak summary below it is the consequence, not the cause, since the aborted test left its sqlite store open. That test shells out to git and depends on filesystem timing, and the job runs the suite in three parallel processes under ASan on a macOS runner.

Attribution: this diff touches only URL-builder extraction and never reaches the watcher, the store or the daemon. The new map field lives in a designated-initializer struct, so it is zero-filled and cannot be read uninitialized. The watcher suite passes locally, seventy-two of seventy-two, three runs in a row, and the full suite is green at 7576 passed.

Could you re-run that job? I get a 403 on the rerun endpoint.

[DeusData]

Approving and merging — and apologies for the wait on the rerun. You were right that it was the flake: I re-ran the failed leg and it went green. test-lsan-macos / watcher_unwatch_drains_pending_free is a known recurring failure on that leg specifically, independent of the diff (it has shown up on PRs touching only install.ps1 and only the CLI), and this change touches nothing outside extraction and tests.

I re-verified the three hardening asks rather than taking them on trust, and all three are genuinely closed in the code:

• Language gate — handle_url_builders returns unless JS/TS/TSX, and resolution is transitively gated because only gated recording can populate a builder entry in the first place. That is the right place to put it.
• Mixed returns — builder_returns_only_urls declines any builder with a non-route return, order-independently, and correctly excludes nested functions' returns via ts_node_eq on the enclosing function. That was the subtle half.
• Builder-as-callback — the new is_url_builder flag means lookup_string_constant skips builder entries, so ids.map(thingPath) no longer resolves. Only call arguments do.

The ambiguity tombstone and the ? truncation both behave as described, and I like that the resolution mints nothing itself — it only populates values, leaving the existing route visitor to mint the Route by canonical QN, so client and server sides join instead of duplicating. Misses over guesses throughout, which is the right instinct for this subsystem.

One thing I am fixing on our side rather than asking you for another round. The gate test extract_c_url_builder_gated_issue1009 does not actually bind the gate. Its fixture returns /etc/myapp/conf.d, and is_filesystem_path() rejects /etc/ before is_rest_path() is ever consulted — so the value is never classified as a URL and never recorded, gate or no gate. Remove the language gate entirely and that test still passes.

That is my fault as much as anything: it is the example I used myself in July, and I only corrected it to "use a path outside those ten prefixes" on the 21st, a few hours before you pushed. I am landing a one-word follow-up (/srv/myapp/conf.d, which is not in the prefix list and so is genuinely URL-shaped) so the gate has real coverage. Nothing about your implementation changes — only the fixture.

Two smaller notes, neither blocking and neither needing action from you:

• builder_returns_only_urls re-walks the enclosing function body for every URL-shaped return, so a function with K such returns does K walks. It is bounded per-file and cannot go corpus-quadratic, but a visited-function memo would be a cheap improvement if you ever revisit this.
• While reviewing I noticed something pre-existing and unrelated to your change: the per-arg minting path detect_url_in_args → is_junk_url appears to have no filesystem-path guard of its own. That is on my list to look at separately; your values are protected by the classify chain.

Thank you for the patience through a long review and for doing the hardening properly rather than arguing the FP shapes were unlikely. The negative tests in particular are the reason this was reviewable.