fix(registry): score a candidate on the extractor's test verdict, not its name - #2427
Open
BobbieBarker wants to merge 1 commit into
Open
BobbieBarker wants to merge 1 commit into
BobbieBarker wants to merge 1 commit into
Conversation
… its name cbm_registry_add discarded the caller's knowledge of whether a symbol is test code and recovered it from the qualified name: is_test_qn scanned the QN for "test", "Test", "mock", "stub", "spec", "fake" or "Fixture" anywhere in the string. REG_TEST_PENALTY then keeps non-test candidates ahead of test ones, so that scan decides which definition an unresolved call resolves to. An unanchored substring match over a path is wrong in both directions, and the false positives are not exotic. On one 970-file Elixir tree the scan calls 794 of 20,882 Function and Method nodes test code. 516 of those match on "spec", and 268 match ONLY because "inspect" contains it: forge_symphony.pipeline.impl .inspect_optional, .pr_tracker.continuation_intent.inspect_record and every other symbol under an operator_inspect module is ordinary production code that was being ranked below real test code. "latest" and "contest" match "test" the same way. In the other direction a test helper under a directory that spells none of those words is scored as production. The extractor already knows. CBMDefinition.is_test is set from the file's test-ness (cbm_is_test_file) OR the definition's own attributes, and it is the value the store filters on. cbm_registry_add now takes it and stores it beside the QN, and is_test_qn is deleted. Three things follow from the verdict no longer being recomputable: candidate_score loses its tri-state `int is_test` and its `-1: ask` fallback, because there is nothing left to ask. qn_test_flags stops returning NULL. Both filtered resolve paths have to carry the verdicts with the candidates. resolve_multi_with_imports already copied a parallel array but guarded it on `flags ?`; filter_import_reachable carried none at all, so the fuzzy path passed NULL whenever it had filtered and silently fell back to the QN scan. index_under_name drops an entry whose verdict it cannot store, rather than leaving the flag array short. A missing verdict scores as non-test, which would let an unranked candidate outrank real code; one missing candidate is the smaller error. pipeline_incremental.c seeds the registry from persisted nodes and has no CBMDefinition, so it reads the same flag out of properties_json through cbm_node_is_test, exported from pass_tests.c. Its comment requires the incremental registry to mirror the full-index one exactly, and both now answer from the same value. Measured by indexing four corpora with a binary built from this commit's parent and with this one, and diffing the `edges` rows keyed on (source QN, target QN, type): corpus moved retargeted lost (resolve) lost (similarity) Elixir 970-file 920 919 1 0 ripgrep 14.1.1 353 193 30 130 tokio 1.40.0 1,548 1,426 86 36 rust-analyzer 5,160 4,817 219 124 Almost every moved edge is the same call site landing on a different target. 336 resolution edges are lost across the four, where the new winner is refused by a check the old one passed. The similarity column is SEMANTICALLY_RELATED, which the post-passes rebuild wholesale over a drifting corpus and which moves on its own. This is a behaviour change across every indexed project, not a signature change, and the gating suite does not see it: 8,236 tests pass with the scan and without it. REG_TEST_PENALTY had no direct coverage, because none of the 95 cbm_registry_add call sites in the suite registered a symbol the scan would flag. tests/test_registry.c: three tests. registry_prefers_non_test_over_test_ candidate pins the penalty itself. registry_test_verdict_comes_from_the_caller _not_the_qn pins where the verdict comes from, using proj.latest.parse (production code the scan reads as a test) against proj.harness.parse (a test the scan reads as production); it fails with the scan restored and the signature kept. registry_sole_test_candidate_still_resolves pins that the penalty orders candidates without vetoing them. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
|
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. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was written by an AI agent working on my behalf. I certify the DCO sign-off on the commit and answer for the change.
Follow-up to #2370. The registry scores candidates on
CBMDefinition.is_testinstead of scanning the qualified name, andis_test_qnis deleted.is_test_qnmatchedtest,Test,mock,stub,spec,fakeorFixtureanywhere in the QN, andREG_TEST_PENALTYuses that to pick which definition an unresolved call resolves to. On one 970-file Elixir tree it calls 794 of 20,882 Function and Method nodes test code. 516 match onspec, 268 of those only becauseinspectcontains it:inspect_optional,inspect_record,inspection_actions.latestandcontestmatchtestthe same way, and a test helper under a directory spelling none of those words is scored as production.Cost
Four corpora, a binary from this commit's parent and one from this branch,
edgesdiffed on (source QN, target QN, type):336 resolution edges are lost across the four, where the new winner is refused by a check the old one passed. The similarity column is
SEMANTICALLY_RELATED, which the post-passes rebuild wholesale and which moves on its own.acceptance_spec_runneris a typical retarget:resultresolved tocritique_report.resultand now resolves tooperator_inspect.reader.result, the function it calls.This is a behaviour change in every indexed project, not a signature change.
Ripples
candidate_scoreloses its-1: asktri-state andqn_test_flagsstops returning NULL, since there is nothing left to recompute from.Both filtered resolve paths now carry verdicts with their candidates.
resolve_multi_with_importscopied a parallel array but guarded it onflags ?;filter_import_reachablecarried none, so the fuzzy path passed NULL whenever it had filtered and fell back to the scan silently.index_under_namedrops an entry whose verdict it cannot store rather than leaving the array short, because a missing verdict scores as non-test and would outrank real code.pipeline_incremental.cseeds from persisted nodes with noCBMDefinition, so it reads the flag out ofproperties_jsonviacbm_node_is_test, exported frompass_tests.c.Tests
REG_TEST_PENALTYhad no direct coverage: of the 95cbm_registry_addcall sites in the suite, none registered a symbol the scan would flag, so the suite passes with the scan and without it.registry_test_verdict_comes_from_the_caller_not_the_qndiscriminates:proj.latest.parseagainstproj.harness.parse. With the scan restored and the signature kept it fails on"proj.harness.parse" != "proj.latest.parse". The other two pin that the penalty exists and that it orders candidates without vetoing them.144 of 144 suites, 8,254 passed, 0 failed, 10 skipped.