Skip to content

fix(pipeline): expose unresolved call coverage - #2305

Open
pcristin wants to merge 2 commits into
DeusData:mainfrom
pcristin:fix/issue-2265-unresolved-call-coverage
Open

pcristin wants to merge 2 commits into
DeusData:mainfrom
pcristin:fix/issue-2265-unresolved-call-coverage

Conversation

@pcristin

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #2265.

TypeScript and JavaScript receiver calls can be emitted as lsp_unresolved without a CALLS edge. This patch records those unresolved call sites as per-file index coverage, including the caller, method name, source byte span, and reason. check_index_coverage exposes the records. Incremental indexing preserves them, and CALLS trace totals are reported as unknown when unresolved sites or older metadata make an exact total impossible. It does not invent CALLS edges for unresolved targets.

The CLI host fixture timeout discovered while validating this change is addressed separately in #2304, following the contributing guide's one-issue-per-PR rule.

Validation

  • The issue-only scripts/test.sh run completed all 143 suites: 8,176 passed, 1 failed, 8 skipped. The only failure was the existing CLI fixture host expiring on this slow machine; test(cli): keep fixture host until released #2304 fixes that fixture.
  • With test(cli): keep fixture host until released #2304's fixture fix applied during validation, the same 143 suites reported 8,177 passed, 0 failed, 8 skipped. The script then stopped in a production watcher guard because another CBM process was active, so the final script exit status was nonzero.
  • Relevant MCP, pipeline, and incremental suites passed. clang-format-20 --dry-run --Werror, the memory-core lint, no-skips check, DCO check, and focused cppcheck 2.20.0 on changed production C sources passed.
  • Full scripts/lint.sh --ci did not finish within 45 minutes in cppcheck; it produced no findings before being stopped.

OpenAI Codex assisted with the investigation, implementation, testing, and PR preparation. Pcristin reviewed and certified the signed-off commit.

Checklist

  • DCO sign-off on every commit
  • New behavior covered by MCP and pipeline tests
  • Full test script completes locally (suite results above; later process guard encountered an active CBM process)
  • Full lint script completes locally (cppcheck exceeded 45 minutes)

Signed-off-by: Pcristin <xxxokzxxx@protonmail.com>
@pcristin
pcristin requested a review from DeusData as a code owner September 24, 2026 20:45
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@BryanQuiceno

Copy link
Copy Markdown

Thank you @pcristin, both for picking this up so quickly and for keeping it focused on the misleading zero. That was exactly the part that hurt us. I built the PR head and ran it against the repro from #2265, plus one extra case. The inbound side works well. I found three rough edges and traced each one to the source, so I hope this saves you some time. Please take whatever is useful.

All line links below point to the PR head 69b84ca.

How I tested

Linux x86_64, gcc, scripts/build.sh on the PR branch. I used a throwaway HOME so it wouldn't touch my regular index:

export HOME=$(mktemp -d)
B=./build/c/codebase-memory-mcp
R=/path/to/repro            # the 9 files from #2265 (+ anidada.js below)
echo "{\"repo_path\":\"$R\",\"mode\":\"full\"}" | $B cli index_repository
P=<project name printed above>
q() { echo "{\"project\":\"$P\",\"function_name\":\"$1\",\"direction\":\"$2\",\"format\":\"json\"}" | $B cli trace_path; }

What works

query before with #2305
q buscar inbound 0, eq 0, unknown ✅
q buscarPlano inbound 0, eq 0, unknown ✅
q procesarConClase outbound (class method) 0, eq 0, unknown ✅

check_index_coverage with paths: ["servicio.js"] returns status: "unresolved_calls" and recommended_action: "read_source_and_verify_calls", plus caller, leaf, span and reason. For an agent, that is the right nudge: go read the source before trusting the count. 🙌


1. Outbound: nested functions inside a factory still report an exact zero

Repro (anidada.js, added next to the original files):

import { ayudante } from "./ayudante.js";

export function crearAnidada({ cliente }) {
  function interna(id) {
    ayudante(id);              // resolved → CALLS edge from `interna`
    return cliente.buscar(id); // unresolved → recorded with caller `crearAnidada`
  }
  return { interna };
}
q interna outbound      → callees_total: 1, callees_total_relation: "eq"       ❌ (buscar is missing, but it says exact)
q crearAnidada outbound → callees_total: 0, callees_total_relation: "unknown"  (the site isn't really its own)

The original repro shows the same thing: procesar, procesarTipado and procesarPlano (outbound) all stay at 0 / eq.

Why: the two passes disagree on who the caller is.

  • The extractor attributes a call to the innermost function: cbm_enclosing_func_qn → cbm_find_enclosing_func (helpers.c#L1187-L1233). That gives …anidada.interna, which is the node trace_path looks up.
  • The TS LSP walk only sets enclosing_func_qn in process_function_body (ts_lsp.c#L3652), i.e. for top-level functions and class methods. For nested function_declaration / function_expression / arrow_function, the branch at ts_lsp.c#L3551-L3574 pushes a fresh scope but keeps the outer enclosing_func_qn ("Nested functions just get a fresh scope"). So ts_emit_unresolved_call_at (#L290) stores caller = …anidada.crearAnidada.
  • The new outbound check in trace_path matches on that caller QN (mcp.c#L9426). interna is never found, so the relation stays eq. Inbound works because it matches on leaf, which doesn't depend on the caller.

I checked this with a temporary fprintf inside ts_emit_unresolved_call_at: every site in the repro is emitted with the outer factory as caller.

Possible directions (you know the codebase much better, so these are only ideas):

  • (a) In the nested-function branch, set enclosing_func_qn to the same QN the def walk produces for that node, and restore it on exit. That would also help the resolved ts_emit_resolved_call_at paths (#L254, #L274) join with the extractor's caller, the same kind of mismatch the comment at helpers.c#L1198-L1205 describes for nested classes. Wider blast radius, though.
  • (b) Keep this PR narrow: in trace_path, match an unresolved site to the traced function by file + source range (the site's span inside the function's range) instead of by caller QN.

This is the case that made me open #2265. Our backend builds services with factory functions and injected dependencies, so almost every method we'd trace outbound is a nested function.

2. A resolved call is also recorded as unresolved, twice

In directo.js, usarDirecto → ayudante resolves (heuristic, 0.95), but coverage also lists it as unresolved, twice, with the same span:

[{"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"},
 {"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"}]

As a result, q ayudante inbound and q usarDirecto outbound report unknown even though the count is complete.

Why: the TS LSP runs twice per file, and each run gives a different callee QN for the same site. My trace shows:

per-file  (cbm.c#L2324 cbm_run_ts_lsp)            callee=./ayudante.js.ayudante                    reason=import_symbol_not_in_registry
cross-file (pass_lsp_cross.c#L1356 cbm_run_ts_lsp_cross) callee=<project>.ayudante.ayudante        reason=import_symbol_not_in_registry

pxc_append_results dedupes on kind + caller + callee + span + origin (pass_lsp_cross.c#L1090). The callee strings differ, so both records survive. cbm_pipeline_record_unresolved_calls then reduces the callee to its leaf (pipeline.c#L466-L470), and the two become identical rows. The method-call sites (buscar) are emitted twice too, but with the same callee string, so they are deduped correctly.

Possible fixes:

  • Dedupe by (caller, leaf, start_byte, end_byte) when building the coverage rows. Cheap and local to this PR.
  • Skip unresolved sites whose span already produced a CALLS edge by another strategy. Otherwise, in a real TS/JS repo nearly every file with an import may end up flagged, and the signal loses its value.

(Separately, and maybe worth its own issue: the cross-file pass has the correct module QN <project>.ayudante.ayudante and still reports import_symbol_not_in_registry for a plain one-argument exported function. I didn't dig into why.)

3. index_repository lists these files as skipped

"skipped_count": 5,
"skipped": {"files":[{"path":"clase.js","reason":"[{\"caller\":…}]","phase":"unresolved_calls"}, …]}

These files were indexed. add_skipped_summary only filters parse_partial/parse_unusable through is_parse_coverage (mcp.c#L10195), so unresolved_calls falls into skipped[]. The comment right above that function says it well: "a reader who sees a file there believes it is absent from the graph entirely." Adding unresolved_calls to that predicate, and perhaps an unresolved_calls_count in the summary, would match what you already did in the coverage summary.

Minor

A 1-based line next to start_byte/end_byte would save agents a conversion step when they go to read the source.


A proposal, if it works for you

I'd like to help with code, not only with reports:

  • (2) and (3): both are small and live inside this PR's scope. If you're OK with it, I'll open a PR against your branch (fix/issue-2265-unresolved-call-coverage) with those two fixes and tests, signed off. You can take them, adapt them or ignore them, so this PR stays yours and stays one claim.
  • (1): the nested-function attribution lives in the TS LSP walk and predates this PR, and it likely affects resolved LSP calls too. To keep fix(pipeline): expose unresolved call coverage #2305 focused, I'd open a separate issue for it with this repro and follow with its own PR, unless you'd rather handle it here.

Either way, I'm happy to re-run the same repro on any follow-up commit. Thanks again for the careful work on this!

@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.

Thank you. This is the follow-up we agreed on in #1682, and persisting unresolved call sites so coverage and trace_path can admit what they do not know is exactly the direction we want. We accept the "unknown" relation value.

@BryanQuiceno, thank you too for that test report. You traced each rough edge to the line that causes it, and that saved us real time. Your three findings overlap with what we were going to ask for, so here they are in one list.

Before it merges:

  1. Remove the false "unknown"s. Two sources drain the signal:

    • Bryan's finding 2: one call site that resolved (usarDirecto → ayudante) is also recorded as unresolved, twice, because the per-file and cross-file TS passes use different callee strings.
    • Inbound matching on any short name: any unresolved site whose leaf matches a visited node's short name marks that trace "unknown". Common names (get, run, init) would then almost always read "unknown".

    Please dedupe coverage rows on (caller, leaf, start_byte, end_byte), drop an unresolved site whose span already produced a CALLS edge, and link an unresolved site to a traced node only through the caller or a leaf the resolver would actually have considered for it.

  2. Outbound on nested functions must not claim an exact zero (Bryan's finding 1). A factory-built function (crearAnidada → interna) still reports callees_total_relation: "eq" while a call is missing. Correcting that "eq" is what this PR is for. Matching the site to the traced function by file and source range, as Bryan suggests in (b), keeps this PR narrow. The deeper fix in the TS LSP walk, setting the enclosing function for nested functions, affects resolved calls too and deserves its own issue and PR, as Bryan proposes.

  3. Files with unresolved calls must not appear in skipped[] (Bryan's finding 3). add_skipped_summary should treat unresolved_calls like the parse-coverage phases, since those files were indexed.

  4. Real-corpus numbers. Our rule is that a change is shown working on real input before it merges. Please report:

    • index time
    • peak RSS
    • the number and total size of unresolved_calls rows
    • the share of trace_path calls that come back "unknown"

    Please measure on a Go repository and on the Linux kernel, taken after items 1–3, so they show the signal we will actually ship.

  5. A parallel-path test. The two tests use two files, which is below MIN_FILES_FOR_PARALLEL (50), so only the sequential path runs. Please add a fixture past 50 files, ideally one that forces a result spill, to cover the path most real indexes take.

  6. yyjson_mut_doc_new(NULL) in cbm_pipeline_record_unresolved_calls uses libc's allocator. Please pass the core-backed allocator the rest of the PR already uses.

Bryan has offered to open a PR against your branch with items 1 (dedupe part) and 3, plus tests. That is welcome from our side, and it is your call whether to take it. The 1-based line next to the byte offsets is a good small addition if you have room.

We will call out one behaviour in the release notes on our side: every index built before upgrading reads "unknown" on call traces until it is re-indexed. Also a heads-up: #2294 removes is_test_file() from mcp.c, which this PR calls in new places, so whichever lands second will need a small rebase.

Thank you both. This closes a real honesty gap in the tools.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

3 participants