Conversation
|
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. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you — making trace output agree with itself about what a test is fixes a real inconsistency in #1593, and the two tests pin it well.
One change of approach before it leaves draft: please reuse the existing classifier rather than add a third one. cbm_is_test_path() in src/pipeline/pass_tests.c is the shared rule (already used by pass_importance.c) and already covers .spec.ts. Combined with the node's is_test property, it gives trace output the same answer as the rest of the graph. We have decided to accept whatever that shared rule says everywhere: if it stops hiding names such as tests.py, test.c, testing/ or test_helpers/, the fix for those belongs in cbm_is_test_path itself, so every consumer changes together.
Two practical notes:
- Please rebase onto
main. Your CI reds were a Windows startup race fixed on main by #2275, and an MSan bootstrap timing test that our open #2272 fixes; neither is yours. - #2305 adds new calls to
is_test_file()in the same trace code this PR removes, so whichever lands second will need a small rebase; we will sequence them.
Thank you again.
Use test metadata and anchored path components for trace filtering, totals and test markers. Recognize .spec files and preserve filtering before pagination without pruning traversal. Add regression coverage for metadata, render modes and pagination. Refs DeusData#1593 Signed-off-by: Anton Standrik <astandrik@yandex-team.ru>
Review on DeusData#2294 asked not to add a third test-path rule. trace_node_is_test() now asks cbm_is_test_path(), the rule that TESTS edges and importance already use, and still honours a stored boolean is_test=true. The prototype moves from pipeline_internal.h to pipeline.h, next to cbm_parse_hunks, so mcp.c can call it. The regression table follows the shared rule. Java *Test.java and Ruby *_spec.rb rows are added: the removed matcher kept them visible. Rows only that matcher handled are dropped: backslash paths, _test.c and my__tests__/. Refs DeusData#1593 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Anton Standrik <astandrik@yandex-team.ru>
54c143a to
6e3d8c2
Compare
|
Written with Claude Code on behalf of @astandrik. Rebased onto
Checked on a Linux x86_64 builder in the project containers. The new table fails against the old matcher and passes here. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @astandrik. This is exactly the shape we asked for, and the evidence is excellent. trace_node_is_test() now defers to the one shared rule (cbm_is_test_path or the stored is_test), so trace output can no longer disagree with the rest of the tools about what counts as a test. The #1593 table now pins that shared rule rather than the old matcher's private answers. The real-index pass over eight projects, where every changed row is one the shared rule decides, is what makes this safe to land.
A note on the overlap you flagged: #2305 adds new is_test_file(node->file_path) calls, so whichever PR lands second will switch those to trace_node_is_test(node). We'll handle that on our side.
Approved. It merges once CI is green on this head. Thank you again!
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Aligns trace_path test-node filtering and labeling with the graph’s canonical test classifier so trace output is consistent with TESTS edges/importance and respects stored is_test metadata.
Changes:
- Replaces
trace_path’s bespokeis_test_file()matcher withcbm_is_test_path()+is_testproperty parsing. - Makes
cbm_is_test_path()publicly available viapipeline.h(removed frompipeline_internal.h). - Adds regression tests covering classification, filtering-after-BFS behavior, and updates an existing paging test fixture.
| File | Description |
|---|---|
| tests/test_mcp.c | Adds/updates tests to validate consistent test classification in trace output and that filtering doesn’t break traversal/paging. |
| src/pipeline/pipeline_internal.h | Removes cbm_is_test_path prototype from internal header (no longer declared here). |
| src/pipeline/pipeline.h | Exposes cbm_is_test_path publicly so MCP trace code can use the shared classifier. |
| src/mcp/mcp.c | Removes trace-only test matcher and uses shared classifier + is_test property for filtering/totals/renderers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* Same rule as TESTS edges and importance, plus the node's is_test property. */ | ||
| static bool trace_node_is_test(const cbm_node_t *node) { | ||
| return cbm_is_test_path(node->file_path) || | ||
| (node->properties_json && cbm_mcp_get_bool_arg(node->properties_json, "is_test")); |
There was a problem hiding this comment.
It does re-parse. With include_tests=false the classifier runs up to six times per production node (filter, totals, then the renderers' count loops and cells), and every run parses the node's properties, about 1 KB for a Function.
The cost is small. Worst case, both legs at the 5000-node ceiling, is 8-15 ms on a default page and up to about 45 ms with limit=5000 and the tree format. Typical traces lose a few milliseconds at most, and on the eight real indexes from the description the timing matched main within noise.
This PR stays the classification fix. Follow-up after merge: classify each visited node once, right after traversal, with a test that counts one parse per node.
| if (matches && visible && include_tests) { | ||
| yyjson_val *test = yyjson_arr_get(row, views[v].risk ? 3 : 2); | ||
| matches = yyjson_is_bool(test) && | ||
| yyjson_get_bool(test) == cases[i].is_test; | ||
| } |
There was a problem hiding this comment.
The offset follows the renderer's column order: name, hop, then risk and test when requested, then strategy, confidence, args. The view matrix already runs mode:"data_flow" and include_evidence, and neither moves test because both append after it.
Reading the index from the leg's cols array is better, agreed. That goes into the same follow-up rather than another CI round on the approved head.
|
Thank you, @astandrik, for answering both Copilot notes with real numbers. Measuring the re-parse cost at the 5000-node ceiling and checking it on eight real indexes is exactly the evidence that lets us keep this PR scoped to the classification fix. We agree with both deferrals. Classifying each visited node once, with a test that counts one parse per node, and reading the |
What does this PR do?
trace_pathcan include.spec.tscallers withinclude_tests=falseand mark nodes as non-tests even when their storedis_testproperty is true. Trace output now uses the graph's own test rule for filtering, totals and thetestcolumn in every renderer:cbm_is_test_path(), which TESTS edges and importance already use, or a stored booleanis_test=true. The trace-onlyis_test_file()matcher is removed. Thecbm_is_test_pathprototype moves frompipeline_internal.htopipeline.hsomcp.ccan call it.Compared with
main,.spec.ts, Java*Test.javaand Ruby*_spec.rbfiles are hidden, and names such assrc/testimonials.tsorsrc/test_helpers/are visible. Changes to what counts as a test belong incbm_is_test_path.Filtering stays after BFS and before pagination, so hidden nodes do not consume page limits or block the walk to visible nodes. No extra database queries and no index change: the new binary reads indexes built by
main.Partial fix for #1593. The PHP resolver and the incorrect graph edges described there are outside this PR; merging this change should leave the issue open.
Validation
Base
64c23fab. Two commits: the original change rebased, then the switch to the shared classifier. Everything below ran on a Linux x86_64 builder in the project's containers (test-infrastructure/Dockerfile*) against the tree of the second commit.tool_trace_test_classification_issue1593; the new tests againstmain's matcher fail that test,tool_trace_test_filter_preserves_walk_issue1593and the paging test. With the fix,mcpandimportancepass (335 tests, 4 platform skips).scripts/test.sh(gcc 13.3, default CI lane): 180 suites, 8232 passed, 0 failed, 9 skipped; the incremental suite ran all 163 tests against the FastAPI 0.99.1 fixture.scripts/lint.sh --ci(clang-format-20, cppcheck 2.20.0): clean. clang-tidy 20.1.8 oversrc/mcp/mcp.c: 954 findings before and after, none added or removed.lint-mem-cion the same file: identical, gate clean.scripts/check-dco.sh,git diff --check: clean.ASAN_OPTIONS): 180 suites, 8232 passed, 0 failed, 9 skipped. MSan (clang 21.1.8, the lane's default suite list): 7484 passed, 9 skipped, no MemorySanitizer report; onedaemon_runtimetest failed onexecveEACCES inside the non-root container, not a sanitizer finding.main's binary against this branch's binary on the same index,trace_pathfor 14 functions per repository, both directions, with and withoutinclude_tests: nest v10.4.15, ripgrep 14.1.1, fastapi 0.99.1, gson 2.11.0, cobra 1.8.1, sinatra 4.1.1, Slim 4.14.0 and this repository. Every row that changes is decided bycbm_is_test_pathoris_test: nest hidesintegration/**/e2e/*.spec.tscallers and shows**/dto/test.dto.ts,test.controller.ts,packages/testing/testing-module.ts; ripgrep hides inline#[test]functions and showstestutil.rs; this repository showsscripts/test.shsymbols; the other five repositories do not change. Thetestcolumn follows the rule in every row.trace_pathat depth 3 on this repository: 3.30 s median with either binary (5 runs). The same call through the MCP stdio server returns the CLI totals.Not run here: macOS LSan, Windows guards, ARM64 (CI).
Checklist
git commit -s)make -f Makefile.cbm test), as the fullscripts/test.shrun abovemake -f Makefile.cbm lint-ci), in the CI lint containerWritten with Claude Code on behalf of Anton Standrik (@astandrik). The first commit was written with OpenAI Codex.