process_edges and expand_var_length carried the expansion budget in the LOOP
condition (`ei < edge_count && *new_count < max_new`). Once new_count reached
max_new they stopped iterating entirely, so match_count was never incremented
for the remaining sources -- even though those sources had real neighbours.
expand_pattern_rels' OPTIONAL fallback is ungated (it has a reserved slot per
binding since the #1177 sizing fix), so it saw match_count == 0 and emitted an
unbound row for every such source.
The result is a false negative that reads as a positive assertion:
MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(c) WHERE c IS NULL
reports a function as having no callers when it has callers, once an earlier
function in the same scan saturated the budget. That is a dead-code query
telling a user that live code is dead.
The budget caps MATERIALISATION, never detection. Both loops now iterate the
full candidate set, increment match_count whenever a real neighbour passes the
filters, and gate only the write into new_bindings. The bound proof from #1177
is unchanged -- writes are still capped at max_new and fallbacks are still at
most one per binding -- so allocation safety does not depend on this change.
Cost: a saturated source still performs its node lookups and filter checks, the
same ones the unsaturated path performs, and materialises nothing.
Reported by @SEPURI-SAI-KRISHNA while working on the sibling bound-terminal path
in #1378; this is the same defect in expand_pattern_rels, which was already live
on main rather than introduced there.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Three items from the #1372 review.
The Windows shard failed in the fixture, not the fix: `system("cd '<dir>' &&
git init …")` used POSIX single quotes, which cmd.exe cannot parse. Switched
to the portable `git -C "<dir>"` shape already used by wt_git in
tests/test_watcher.c, with identity, default branch and signing passed via -c
so the fixture no longer depends on the machine's global git config.
A changed file with hunks but no overlapping definition now keeps whole-file
seeding instead of dropping out. Import-only edits, module-level constants and
anything above the first definition land outside every definition's line range,
so scoping alone would have removed the file from the seed set entirely —
strictly worse recall than the behavior being replaced.
The overlap probe has to apply the same label filter as the seeding loop: a
Module node spans lines 1..EOF, so probing raw nodes reports an overlap for
every hunk and defeats the fallback. Extracting detect_is_seedable_label lets
both sites share one definition. The new regression test caught exactly this —
it failed with seed_symbols 0 until the probe was filtered.
A filled hunk buffer is now treated as a truncated diff: hunks past HUNK_CAP
are gone, so files captured only partially would still look scoped and silently
under-seed. Scoping is dropped for the whole request and the decision logged,
rather than under-reporting a large refactor.
Documented the mixed coordinate systems in the fetch block: base...HEAD hunks
are HEAD-side lines, worktree hunks are worktree-side, node lines come from the
indexed snapshot. They agree while the index is fresh; a stale index plus
earlier-in-file insertions can mis-scope, bounded by the zero-overlap fallback.
Signed-off-by: lishixiang <lishixiang@gmail.com>
test-runner, test-repro-runner, test-runner-tsan, codebase-memory-mcp and
test-foundation are each built in a single compiler invocation from their
sources, and their rules listed only .c files as prerequisites. Editing a
header therefore changed nothing make could see, so the target was not
rebuilt and an incremental build silently kept testing the OLD header value.
Found while revert-checking the label-allowlist change in this branch: the
check passed, which was wrong. Shrinking CBM_SQL_TYPE_LIKE_LABELS should have
turned the pin test red and did not, because nothing had recompiled. Forcing a
rebuild produced the expected failure. A revert-check that cannot fail is worse
than no revert-check, because it reports confidence it did not earn.
This is the same class as the lsp_all.o staleness that #662 fixed with an
explicit dep list; that fix was per-object, this one covers the whole-program
targets.
PROJECT_HDRS globs src/, internal/cbm/ and tests/ headers and is added to the
five affected targets. Vendored trees are deliberately excluded: they are
pinned, they already carry explicit dep lists (LSP_UNITY_DEPS,
TS_RUNTIME_DEPS), and globbing tens of thousands of grammar headers would cost
more than it protects.
Verified: editing only src/foundation/constants.h, with no source file touched,
now rebuilds and turns the pin test red -- the exact edit that was invisible to
make before.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cbm.h documents cbm_label_is_type_like() as the single source of truth for
type-like labels, "so adding a new type-like label (e.g. Struct for
Rust/Go/Swift/D structs) updates them all at once instead of scattering
|| strcmp(label,"Struct")==0 across the tree".
A SQL string literal cannot call it. Four queries in store.c and the BM25
ranking in mcp.c carried their own hardcoded label lists, so they silently
opted out of that contract and stopped matching the moment Struct, Interface,
Enum, Type and Trait began being emitted:
store.c:5279 arch_boundaries ('Function','Method','Class')
store.c:5422 arch_packages_from_qn ('Function','Method','Class')
store.c:6689 arch_clusters ('Function','Method','Class')
store.c:7742 cbm_store_vector_search ('Function','Method','Class')
mcp.c:2759 BM25 ranking ('Class','Interface','Type','Enum')
Effect on main today: get_architecture (boundaries, packages, clusters) and
vector search drop every Struct, Interface, Enum, Type and Trait node in the
project, and search_code under-ranks structs -- for Rust, Go, Swift and D,
where struct is the primary type declaration. The BM25 list had drifted
differently again, omitting Struct and Trait but not Interface.
The fix is not a wider literal. CBM_SQL_TYPE_LIKE_LABELS and
CBM_SQL_CALLABLE_OR_TYPE_LABELS live next to each other in constants.h and are
pinned to cbm_label_is_type_like() by a test that checks both directions:
every label the C predicate accepts appears in the SQL fragment, and nothing it
rejects is smuggled in. Adding a type-like label without updating the SQL now
fails that test instead of quietly shrinking query results.
No new labels are introduced. Every label added here is one the extractors
already emit.
Reported by @win4r in #568, whose audit found four of the five sites. Their
patch widened the literals in place and added Actor, a label main does not
emit; this routes through the canonical set instead and also covers Interface,
Type and Trait, which that patch still omitted.
Co-Authored-By: win4r <win4r@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The indexer already records how every CALLS edge was resolved --
pass_calls.c:355 writes {callee, confidence, strategy, candidates} into the
edge properties, and cbm_store_traverse reads it back -- but no tool ever
surfaced it. An agent could see THAT A calls B, never how confident the
resolver was, or whether the edge came from an LSP or from a name heuristic.
trace_path / trace_call_path gain include_evidence (default false). When set,
each row carries two extra columns: the resolution strategy CLASS and the
resolver's confidence.
Two decisions worth recording:
Opt-in, not default. Two extra columns on every row is exactly the inflation
the tree format exists to avoid, so the default response is byte-identical to
before.
A closed class, not the raw strategy. Production emits ~20 internal strategy
names (lsp_trait_dispatch, php_self_static, callee_suffix, ...) and the set
grows with every language. Publishing them verbatim would make each internal
resolver name public API by accident. cbm_mcp_edge_strategy_class maps them to
a fixed vocabulary -- lsp | language_rule | heuristic | unresolved -- so adding
lsp_foo_dispatch maps automatically while a genuinely new KIND of resolution
fails the pinning test and forces a deliberate decision. lsp_unresolved
classifies as unresolved, not lsp: the caller's question is whether the edge is
trustworthy, and "we tried LSP and it did not resolve" answers no.
Evidence is looked up against the PAGINATED view, so it is emitted only for the
rows on the current page -- it does not bypass limit, the cursor, or the
MCP_BFS_LIMIT_MAX guard.
Tests: the vocabulary pin (every strategy production can emit maps to a known
class), and an end-to-end test binding both halves -- absent by default,
present when asked for, and the raw internal name never reaching the client.
Both fail without this change.
Distilled from #559 by @vvenegasv, who found that we already store this and
never show it. The freshness half of that PR is not included: it is
non-functional in production (both pipelines persist through the direct SQLite
writer, which records a hardcoded NULL) and its ALTER TABLE migration never
runs for the read-only opens the MCP server uses.
Co-Authored-By: vvenegasv <vvenegasv@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The bare-node-carry check in with_agg_find_or_create only tested
!property && variable, so labels(n)/id(n)/keys(n)/properties(n) aliases
(variable set, property NULL, func set) were also tagged with the source
node's id. A later alias.property then hit node_prop's stub re-fetch
heuristic and silently returned the source node's real property instead
of empty for the non-node alias.
Adds cypher_issue1111_with_scalar_func_alias_no_node_leak, which fails
on the prior code (returns the source node's file_path) and passes with
this fix.
Addresses DeusData's review on #1221.
Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
RETURN/WITH aggregation classified each projected column with a bare
`if (item->func)` check to decide group-key vs. aggregate value. That
check is true for any function call, so type()/labels()/id()/keys()/
properties() were routed into the aggregate branch alongside real
aggregates and formatted via format_agg_value's default case, which
emits the row count instead of the function's actual value.
Swap the five call sites (ret_agg_build_key, ret_agg_emit_row,
with_agg_build_key, with_agg_find_or_create, with_agg_accumulate) to
is_aggregate_func(), the predicate already used correctly one level up
to decide whether a query needs aggregation at all. Group-key columns
carrying a non-aggregate function now project through the existing
project_item() helper instead of binding_get_virtual(), since they
need to evaluate the function rather than read a raw property.
Fixes#1111 (the type(r)/count(*) half; the inline-property-map half
of that issue does not reproduce on current HEAD)
Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
detect_collect_seeds treated every definition in a changed file as a
BFS seed, regardless of which lines actually changed. A one-line edit
inside a single method seeded every other definition in the file too,
producing an impact report an order of magnitude larger than what the
edit actually touched.
Fetch `git diff --unified=0` hunks (cbm_parse_hunks already existed in
pass_gitdiff.c but had no caller) alongside the existing file list, and
scope seeds to definitions whose line range overlaps a hunk. Any
failure fetching hunks (new/untracked files, a transient git error)
falls back to the previous whole-file behavior, so this is a precision
improvement with no new failure mode.
Verified on microsoft/qlib (an unrelated third-party repo): a same-
line-count edit inside one method now seeds 2 symbols (the method +
its containing class) instead of 25 (every definition in the 24-def
file), matching the equivalent output of a different code-graph tool
(GitNexus) on the identical scenario. Reproduced on a second file with
a different definition count (42 defs -> seeds 43 before the fix, 2
after) to rule out coincidence.
cbm_changed_hunk_t / cbm_parse_hunks move from pipeline_internal.h
(explicitly not a public header) to pipeline.h, since detect_changes
now needs them from src/mcp/mcp.c. cbm_detect_node_in_hunks is exposed
non-static via mcp_internal.h, matching this file's existing white-box
test-hook pattern, so the overlap logic has a direct unit test
independent of the git/subprocess/index plumbing around it.
Fixes#1363
Signed-off-by: lishixiang <lishixiang@gmail.com>
LABEL_GOLDENS pins the node labels each grammar emits. The nix case is
`{ foo = 1; bar = 2; }` — two module-level scalar bindings — and its golden was
Module:1, recorded when no Nix binding could be named and none was minted.
Both are Variables now, so the golden is Module:1,Variable:2. The mismatch CI
reported is the intended behaviour change, not a regression.
Extraction, pipeline, grammar_labels, grammar_regression and lang_contract:
534 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
nix_module_level_bindings_mint_variables
a `let` binding and an attrset binding both mint Variables, and the QN
carries the attrpath (services.nginx.enable).
nix_nested_bindings_are_not_module_level
a binding two attrsets deep mints nothing, and neither `deep` nor `nested`
becomes a Variable since both are scopes.
nix_lambda_binding_is_function_not_variable
a lambda-valued binding is a Function and NOT a Variable; a scalar binding
is the reverse.
Every absence assertion is paired with a positive one on the same predicate in
the same result — `topLevel` in the nesting test, `val` in the split test — so
none of them can pass by extracting nothing at all. Several false "clean" results
in this area came from predicates that could not have matched.
Signed-off-by: Jason Bowman <jason@json64.dev>
`nix_var_types` has declared `binding` since the language was added, but no Nix
case existed in extract_var_names and no path reached a binding in the first
place, so the Variable count for Nix was unconditionally zero.
Two things were in the way. extract_variables iterates only the file root's
DIRECT children, and a Nix file's root child is its header lambda
(`{ pkgs, lib, ... }:`), so the generic loop saw nothing — the same shape as the
definition bug. And extract_var_names had no Nix branch, so a binding could not
have been named even if reached.
Scope follows the rule the other languages already use rather than inventing one.
extract_variables mints FILE scope and never locals: a C++ declaration inside a
function body is not a Variable. For Nix, file scope is the `let` bindings and
the returned attrset — a let binding is file scope in the same sense a C++
file-static is, and the attrset is the exported surface. A binding in a deeper
attrset is not.
That bound is load-bearing. Every Nix binding's parent is a binding_set at any
depth, so admitting them all would mint a node per `enable = true` in a NixOS
module's settings tree — the per-leaf flood the Helm values.yaml case in this
same function already exists to avoid.
Bindings whose value is a lambda or an attribute set are skipped: the first is
already minted as a Function by the def walk, the second is a scope
(is_namespace_scope_kind), so minting either here would double-count it.
Names follow the attrpath convention from the previous commit — leaf as the name,
whole path in the qualified name, so `services.nginx.enable = true` is name
`enable`, QN proj.file.services.nginx.enable. push_var_def grows a _qn variant
for that; the plain form delegates to it and is unchanged for every other
language.
Extraction and pipeline suites: 494 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
Four extraction cases and one pipeline case, each asserting a distinct claim so a
regression names what it broke:
nix_attrset_scope_disambiguates_leaf_names
two attrsets each holding `dup` produce two definitions with distinct QNs.
Unqualified they shared one QN and the second was discarded.
nix_dotted_attrpath_qualifies_like_nested
`wrap.deep.fn = …` and `wrap = { deep = { fn = …; }; }` produce the SAME
QN. This is the equality that makes attrpath qualification a correctness
fix rather than a preference — the two are the same expression.
nix_quoted_attr_name_strips_quotes
`"kebab-case"` is named kebab-case, and asserts the quoted form is absent
rather than merely that the bare form is present.
nix_interpolated_attr_mints_no_def
`"${dynamic}" = …` mints nothing, and the sibling binding still does — so
the test cannot pass by extracting nothing at all.
pipeline_nix_scoped_binding_calls_resolve (tests/test_pipeline.c)
a call inside a scoped binding reaches its target through the store, for
both routes into a qualified name: an enclosing attrset and a dotted
attrpath.
The pipeline case has to live at that level. Definition QNs and call-scope QNs
come from two separate functions, and if they disagree the edge is dropped at
write with no error — every extraction-level assertion still passes while the
graph quietly loses edges. Only the store shows it.
Adds has_def_qn alongside the existing has_def. find_def_by_name returns the
first match by NAME and so cannot tell two same-named definitions in different
scopes apart, which is exactly what these tests exist to separate.
Extraction and pipeline suites: 491 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
A Nix binding's name is a PATH, and the resolver took only its first segment
(`child_by_field_name(attrpath, "attr")`). Three defects followed from that one
modelling gap:
- `setA = { dup = …; }` and `setB = { dup = …; }` both minted the qualified
name proj.file.dup. The second definition, and every CALLS edge sourced from
it, was silently discarded at write.
- `a.b.fn = …` minted a definition named `a`, colliding with every other
binding whose path began `a`.
- `"kebab-case" = …` minted a name with the quote characters in it, so every
consumer keying on the name had to know to re-quote.
Adopts the convention already used for C++ namespaces — name is the leaf
segment, qualified_name is enclosing scope plus leaf, so `ns::serialize` is name
`serialize` and QN proj.file.ns.serialize. For Nix that means:
- the resolver returns the LAST attrpath segment;
- leading segments become scope, so `a.b.fn = …` and the nested spelling
`a = { b = { fn = …; }; }` produce the same QN, as they must — one is sugar
for the other;
- a binding whose value is an attribute set is a scope, mirroring
is_namespace_scope_kind's treatment of a C++ namespace. `let` bindings are
deliberately NOT scopes: they are lexical, and C++ does not qualify by block
scope either;
- a quoted segment is unquoted, and an interpolated one (`"${x}" = …`) mints
nothing at all, since it has no statically knowable name — the same call the
Makefile dot-prefix guard makes.
The two scope sources compose: `setA = { a.b.fn = …; }` is
proj.file.setA.a.b.fn.
Definition QNs and call-scope QNs are computed by two different functions —
extract_defs.c's compute_class_qn and extract_unified.c's own. If they disagree
by one segment the CALLS edge names a source node that was never minted and is
dropped at write, with no error and no failing extraction assertion. Rather than
implement the rule twice, the Nix computation lives in helpers.c and both call
it, which makes that class of drift impossible instead of merely unlikely. Note
also that the def-side enclosing-QN gate is language-gated while the call-side is
not, so widening one without the other would have produced exactly that mismatch.
Signed-off-by: Jason Bowman <jason@json64.dev>
The acknowledgement text described a state of the world that ended when
0.9.1-rc.1 published: it told every new contributor we were still finishing
release-critical work and that "review resumes once the release path is clear".
Posting that now would be false on the day it starts running.
This is the obligation the PR description already set for itself -- the
review-status section is a public promise and has to be edited when the
situation it describes changes. Doing that before the workflow ever posts,
rather than after someone reads a stale freeze notice, is the point.
The queue is still real, so the section stays rather than being removed: the
freeze is over, a large backlog is not, and PRs are being read oldest-first.
Saying that is more useful to someone opening a PR today than either the old
text or silence, because it explains why a recent PR sits behind older ones.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
CodeQL flagged two high-severity alerts in the previous commit: the help-list
builder accumulated snprintf's return value, which is the length it WOULD have
written rather than what it did. Once len passes cap, the next `cap - len`
underflows to a huge size_t and the following write lands outside the buffer.
That is the same class the Cypher UNWIND list builder was just fixed for in
#1173, where blen advanced past a 2KB buffer the same way.
The capacity computed here is in fact sufficient for the current registry, so
nothing overflows today -- which makes this the more dangerous shape, not the
safer one. The code was correct only by an argument made ten lines away in the
cap loop, so renaming a tool or changing the wrap rule would have turned it into
an out-of-bounds write with nothing in the function to notice.
help_append() returns the bytes ACTUALLY written, clamped to the space left, so
`len <= cap - 1` becomes a local invariant that holds no matter what the
registry or the wrap column later become. Also adds the <stdarg.h> the variadic
helper needs.
Verified: builds clean under -Werror; --help still renders all 15 tools in
registry order with the same wrapping; mcp + cli suites 437 passed, 0 failed;
make -f Makefile.cbm lint-ci clean.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
King Star