Skip to content

fix(mcp): expose possible override targets in trace_path - #2374

Open
lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:fix/trace-path-override-targets
Open

lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:fix/trace-path-override-targets

Conversation

@lorenzozanee

Copy link
Copy Markdown
Contributor

What does this PR do?

trace_path can optionally expand an outbound call to implementations of its declared target through OVERRIDE edges. Possible implementations appear separately under possible_callees, so declared CALLS targets remain distinct and default output is unchanged.

Focused validation: bash scripts/test.sh --suites mcp (321 passed, 4 skipped).

Fixes #1271

Prepared with AI assistance. The submitting human retains DCO accountability.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

Signed-off-by: lorenzozanee <wyz0707@proton.me>

@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 so much, @lorenzozanee! This is a careful, well-aimed change. You picked up exactly the representation argued for in #1271:

  • declared CALLS targets stay what they are;
  • possible runtime implementations are listed separately, under a name that says "possible";
  • the feature is opt-in, and the default output is unchanged.

It's also good to see the test pinning that no edges are written and that the default response doesn't change. And this one is nicely focused: it does just what the title says. We'd like to land it, and have a few requests first, each with the reason:

  1. Memory-core ratchet (make lint-ci fails). scripts/lint-memory-core.py reports src/mcp/mcp.c: grew by 7 (784 -> 791): six frees in the new error path and one at cleanup. We route allocations through one core so leaks and waste stay measurable project-wide, and a file's raw-allocator count may never go up. Could the error path reuse the handler's existing cleanup, or free through cbm_free?
  2. Let possible_callees give way first under max_output_tokens. It's appended after the rows but isn't one of the optional fields. So when the byte ceiling is hit, the budget search drops graph rows to make room for it, and if it's large on its own the response falls to the floor. The trace_path contract is that optional diagnostics yield before primary rows. Treating it like the other optional fields keeps that promise: drop it first, then report possible_callees_omitted: true.
  3. Scope it to the emitted page. It's built from the whole traversal (up to 5000 nodes), so it repeats in full on every cursor page and can name contracts that aren't on the page. Computing it only for the rows actually emitted keeps pages exactly-once and bounded by limit.
  4. Query per contract instead of loading every OVERRIDE edge. cbm_store_find_edges_by_type(..., "OVERRIDE") loads and sorts the project's whole override set on every call, even at depth 1. On large Java/C# graphs that's a lot of work per trace. cbm_store_find_edges_by_target_type (in store.h) keeps the cost proportional to what was actually reached.
  5. Follow deeper hierarchies. The indexer's OVERRIDE edges point only at the immediate base (pass_semantic.c), so for C : B : A a call to A.m lists B.m but not C.m. Walking inbound OVERRIDE edges transitively, with a visited set, returns the full possible set, which is what the issue asks for.
  6. Tree format. The default tree output embeds a raw JSON array. Could it use the same tree-table form as the other sections, so clients parsing the tree format see one shape?
  7. Smaller things:
    • Please split the single ASSERT_TRUE(valid) into separate asserts, so a failure says what broke.
    • Please add cases for include_tests filtering, a two-level hierarchy, and the budget and paging behaviour.
    • Please keep the dropped description sentence ("Rows keep qn/hop with explicit totals, relations, and continuations."). Clients read it as guidance.

Heads-up, not a request. #2294 moves trace test filtering to trace_node_is_test(node), and #2305 touches the same part of handle_trace_call_path. Whichever lands first, a rebase will be needed, and the implementation filter should then use the same test classifier. The inbound half of #1271 (callers reached through the contract) is fine as a follow-up; we'll keep the issue open for it.

Thanks again. This closes a real gap in dispatch-aware tracing!

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

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

Development

Successfully merging this pull request may close these issues.

Python polymorphic call is collapsed to one low-confidence concrete target instead of override set

2 participants