fix(mcp): expose possible override targets in trace_path - #2374
lorenzozanee wants to merge 1 commit into
Conversation
Signed-off-by: lorenzozanee <wyz0707@proton.me>
DeusData
left a comment
There was a problem hiding this comment.
Thank you so much, @lorenzozanee! This is a careful, well-aimed change. You picked up exactly the representation argued for in #1271:
- declared
CALLStargets 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:
- Memory-core ratchet (
make lint-cifails).scripts/lint-memory-core.pyreportssrc/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 throughcbm_free? - Let
possible_calleesgive way first undermax_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. Thetrace_pathcontract is that optional diagnostics yield before primary rows. Treating it like the other optional fields keeps that promise: drop it first, then reportpossible_callees_omitted: true. - 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. - 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(instore.h) keeps the cost proportional to what was actually reached. - Follow deeper hierarchies. The indexer's OVERRIDE edges point only at the immediate base (
pass_semantic.c), so forC : B : Aa call toA.mlistsB.mbut notC.m. Walking inbound OVERRIDE edges transitively, with a visited set, returns the full possible set, which is what the issue asks for. - 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?
- Smaller things:
- Please split the single
ASSERT_TRUE(valid)into separate asserts, so a failure says what broke. - Please add cases for
include_testsfiltering, 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.
- Please split the single
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!
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
What does this PR do?
trace_pathcan optionally expand an outbound call to implementations of its declared target throughOVERRIDEedges. Possible implementations appear separately underpossible_callees, so declaredCALLStargets 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
git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)