The manifest's per-file sha256 loop was single-threaded -- tens of
thousands of file reads in sequence, the second-largest block of a
kernel-scale incremental run after publication (~20s by residual). The
hash helper is pure per-file work, so files now fan out across
cbm_default_worker_count workers on a stride; ASSEMBLY stays serial and
in discovery order, so the manifest bytes are identical to the serial
build's -- the exactness doctrine is untouched, only the wall clock
moves. Repos under 64 files keep the serial path outright.
A worker that fails to spawn leaves its stride to the calling thread,
so every index is hashed exactly once regardless of thread-creation
failures.
Pinned by a threshold-crossing test: a 72-file repo must route NOOP on
unchanged bytes -- which stands entirely on two parallel builds
producing byte-identical manifests -- and still classify a single edit
into the closure route.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Path-alias configs (tsconfig/jsconfig class) stop declining the closure
route. A config delta re-routes RESOLUTION for the files it governs while
touching none of their bytes, so those files join the closure directly:
they re-extract and re-resolve under the freshly loaded alias collection,
their surfaces come out unchanged, and propagation stops -- the depth-1
argument holds exactly as it does for source edits. Governed means every
discovered file under the config's directory; over-inclusion from nested
scopes is deliberate (safe direction), and the existing budget still
bounds the total, so a root config on a large repo correctly concedes to
a full rebuild.
Classification is now explicit rather than incidental: synthetic
manifest digests (git context, extension configs) decline as
semantic_input_changed; package-control files decline as
control_file_changed (pkgmap is global -- governed repair is unsound
there); alias configs -- recognized by exact match against the loaded
collection plus a basename fallback so a REMOVED config still
classifies -- seed the governed closure, whether changed, added, or
removed.
The existing tsconfig-alias convergence test becomes the proof: no
source file changes, the route asserts CLOSURE_REPAIR, and the caller's
CALL_REFERENCE must move from target_a.ts to target_b.ts to match the
fresh-full reference -- the exact case the legacy partial route silently
corrupted and binary routing paid a full rebuild for.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The routing follow-through on the +92% warm-reindex finding: a semantic
manifest delta no longer unconditionally rebuilds the world. The planner
recomputes exactly the changed files plus the recorded consumers of any
changed SURFACE, and the executor resolves them against cross registries
rehydrated from the persisted per-file surfaces -- the same registration
code a full build feeds from fresh parses, which is what makes the output
converge instead of drift.
Routing, in order: exact manifest match stays a no-op; a delta first
offers itself to the closure planner; every uncertain case declines to
the full rebuild that was yesterday's only behaviour. Declines: virtual/
config manifest entries, new files, ADDED definition names (yesterday's
graph cannot know who would resolve to a name that did not exist -- the
write-the-caller-first flow and shadowing both live here), missing or
undecodable surface rows, dependents outside discovery, and a budget of
30% of files with an 8-file floor (a percentage alone starves small
repos: 1 changed file in 3 is 33%).
Two structural facts carry the correctness argument. Per-file extraction
is a pure function of file content, so an unchanged dependent can never
be surface-changed in turn -- the closure is depth-1 by construction, no
fixpoint. And a body edit reserializes to the identical surface bytes,
so its closure is the file itself. The dependent set comes from one
indexed query over the previous generation's edges (structural
Folder/Project containment excluded -- a container is not a consumer).
The executor is the existing partial machinery, parameterized: re-parse
list = closure; inbound-edge snapshot/re-link keeps only sources OUTSIDE
the closure (sound because every referencer of a surface-changed file is
inside it by construction); cbm_parallel_resolve now receives real cross
registries built from stored-surface defs plus this run's fresh parses;
publication merges surviving surface rows with the re-parsed files'
fresh ones inside the same generation. The legacy test-only route
publishes no surface rows at all -- a stale row that satisfies a future
closure plan with yesterday's surface would be worse than the full
rebuild an empty table forces.
Tests pin route AND convergence together (route equality matters because
a full rebuild satisfies any convergence assertion vacuously): body edit
routes CLOSURE_REPAIR with node/edge/CALL_REFERENCE counts equal to a
fresh full index; REMOVING a definition keeps the closure route and
drops the dependent's stale CALL_REFERENCE -- the assertion the legacy
QN-keyed re-link could never pass; added-name, new-file and budget cases
decline; the existing Go content-change test now routes CLOSURE_REPAIR
with its convergence assertions unchanged, making it the Go-language
proof of the same machinery.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Second piece of closure repair. At the collect_all_defs seam -- the only
moment the per-file result cache is alive -- both drivers (parallel and
sequential) now serialize each file's CBMLSPDef slice to canonical JSON,
hash it, and hand the rows to the pipeline; cbm_pipeline_publish_generation
writes them into the staging store next to the manifest, so surface data
and graph always belong to the same generation.
Canonical bytes are the point: every field is written in fixed order with
an explicit null for absent strings (NULL and "" differ in the CBMLSPDef
contract -- receiver_type NULL means "not a method"), so byte equality IS
surface equality and the sha over the bytes is the early-cutoff key.
Registry-only labels that pxc_map_label drops but the name registry serves
(Field) are folded into the hash as a separate "reg" array, or renaming
one would slip past the cutoff.
Behaviour pinned in SUITE(pipeline): a fresh full index persists a
versioned surface row per file; a BODY edit republishes the identical
surface_sha; a SIGNATURE edit changes it. That pair of properties is what
the routing layer will stand on.
cbm_pxc_collect_all_defs gains an optional per-file prefix array -- the
flat all_defs[] otherwise loses the file boundaries the serializer needs.
CORRECTION to 6c22338's scope note: it claimed cbm_pipeline_publish_
generation was reachable only behind CBM_INCREMENTAL_TEST_API. Wrong --
dump_and_persist_hashes calls it on every production full index
(pipeline.c:1863); the grep that "verified" test-only reachability had
excluded pipeline.c itself. The predictable staging name WAS in the
production publish path, which makes that fix a real production hardening,
not a test-path cleanup.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
First piece of the closure-repair incremental route (the follow-through
on the +92% warm-reindex finding): a per-file lsp_surface row holding
the file's serialized cross-file definition set -- exactly what
pass_lsp_cross registration consumes -- plus the metadata the routing
decision needs: the surface sha (early-cutoff key: a body edit leaves it
unchanged, so no dependent recomputation is owed), a referenced-name
bloom (added-symbol trigger), and a governing-config context hash.
The store treats defs_json and the bloom as opaque; the codec lives with
pass_lsp_cross, which is their only writer and reader. A project with no
rows reads back as OK/0 -- callers treat that as "no surface data" and
route to a full rebuild, which is also how databases written before this
table existed upgrade themselves.
Table appears via the CREATE IF NOT EXISTS schema on store open, so the
raw dump writer needs no change: publication opens the staging DB with
the store right after the dump, which applies the schema.
Round-trip covered in store_nodes: batch upsert, ordering, binary bloom
with embedded NUL, NULL bloom, whole-row conflict replacement including
bloom removal, project-scoped delete, and the empty-project signal.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cbm_pipeline_publish_generation built its staging database name by hand as
"<db>.stage.<pid>.<counter>", unlinked it, then wrote it. Any local process
can compute that name in advance, so a symlink planted between the unlink
and the write redirects the write to a target of the attacker's choosing —
an arbitrary-file clobber when the database sits in a world-writable
directory.
The same file already solves this correctly elsewhere: create_staging_path()
mints the name with mkstemp, so the file is created O_EXCL and we only ever
write one we made ourselves. Publication now shares it. The unlink-first
step goes away with the predictable name — it existed to clear a leftover at
a name we might reuse, and a freshly minted name cannot collide, nor can its
sidecars pre-exist.
SCOPE, stated precisely because the PR description overstates it: the only
caller of cbm_pipeline_publish_generation sits behind
CBM_INCREMENTAL_TEST_API, which is set in CFLAGS_TEST and never in
CFLAGS_PROD. The predictable name was therefore not reachable in a shipped
binary — production publication already went through create_staging_path.
This is removing a bad pattern from a test-only path before it can be
promoted, not patching a live user-facing vulnerability.
The regression test calls the function directly, because no pipeline entry
point reaches it in a production build. It does not try to win the race — a
test that has to win a race is a coin flip, not a gate. It asserts the
property that removes the race: canaries occupy every name the old scheme
could have chosen and all must survive publication. Verified both ways
rather than green-only: against the old code it reports "survived == 31,
expected PREDICTABLE_CANARIES == 32", exactly one canary consumed; with the
fix the suite goes 18 passed/1 failed -> 19 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
scripts/ci/lint-mem.sh and scripts/ci/msan-lane.sh were committed at mode
100644, so the workflow step that runs them directly died with
"Permission denied" (exit 126). scripts/lint-mem-gate.py gets the same
treatment: it is invoked through python3 today, but it carries a shebang
and should not depend on that.
The exec-bit contract already exists to catch precisely this, and it did
not, because it derives its candidate set from `git ls-files -s '*.sh'`
-- tracked files only. A brand-new script is invisible there until it is
committed, so the check passes on the run where the defect is introduced
and only starts failing on the run that ships it. The window where the
contract is most useful was the one window it could not see.
It now also considers not-yet-tracked scripts by their filesystem mode.
Verified against the real defect rather than in the abstract: with
lint-mem.sh untracked and non-executable the contract reports
"_lint.yml:76 executes scripts/ci/lint-mem.sh directly, but its committed
mode is 100644", and passes once the bit is set.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
runtime_test_spawn_blocked_executable() used fork() + execl(). fork()
duplicates the parent's address space, and under a sanitizer that address
space includes a very large shadow mapping. On macOS the duplicate trips
the per-process memory limit and jetsam SIGKILLs the child before exec
ever gets to replace the image, so the child is already dead by the time
the parent checks it.
The visible effect was daemon_runtime_process_fingerprint_never_hashes_
replacement_path failing at ASSERT(setup) under TSan while passing in
every non-sanitized and ASan build. The diagnosis is direct rather than
inferred: waitpid() reported the child as WIFSIGNALED with WTERMSIG 9,
and DYLD_INSERT_LIBRARIES was unset, ruling out library-validation
refusal of the copied system binary.
posix_spawn() never copies the parent address space, so the child is
never charged for the shadow mapping. The file actions reproduce the
child-side setup exactly (close the ready-read and input-write ends,
dup2 the input-read end onto stdin, close the original), and exec
failure is now reported by posix_spawn itself rather than over the ready
pipe. The FD_CLOEXEC-on-successful-exec EOF that the parent uses to
detect a live child is unchanged.
This keeps the test running under TSan on macOS instead of dropping the
suite from the lane. Verified: daemon_runtime 43 passed under TSan and
under the ordinary build, and the full macOS TSan set is 940 passed /
3 skipped / 0 races.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
setenv/unsetenv do not exist on Windows, so the bug-repro runner failed to
COMPILE there -- this is the 'runner did not execute' failure on the
repro-windows CI job and the local Windows board alike. The compat layer's
cbm_setenv/cbm_unsetenv (already included via repro_harness.h) are the repo
idiom; test_pipeline.c uses them for the same variable.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
c_lsp_process_file carried __attribute__((no_sanitize("address"))) over the
entire C LSP orchestration, excluding every direct memory access in it from
ASan -- placed for a stack-pointer-to-registry lifetime bug whose witness test
also had its assertion discarded ((void)find_resolved), so nothing could ever
prove the bug gone. Flagged by the memory-diagnostics standards report
(private/C_MEMORY_DIAGNOSTICS_STANDARDS_REPORT.md, priority 2) as the single
highest-value ASan coverage gap in the repository.
This branch's C LSP registry rework resolved the underlying lifetime: with the
suppression removed, the full c_lsp suite passes under ASan+UBSan (760 tests),
including the formerly-failing template field type resolution, whose test now
asserts the resolved call instead of discarding it.
Honest limit: the original bug predates the branch and its fix is the branch's
broad registry rework, not an isolatable commit -- so the revert-proof is
main-vs-branch (main: suppression + discarded assertion; branch: neither),
not a single-change revert-check.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The sequential lsp_cross pass builds its shared per-language cross registries
in ctx->seq_cross_arena, which DELIBERATELY outlives the pass -- resolved_calls
and the registries carry borrowed strings that pass_calls still reads, and the
arena is destroyed only after all passes (the earlier freeing-here bug was a
pass_calls use-after-free, says the comment at the arena's creation).
But the per-file module-QN strings (def_modules[], malloc'd in
cbm_pxc_collect_all_defs and handed to every registrar as def_module_qn) were
freed at the END OF THE PASS -- the exact mistake the arena comment warns
about, one level down. Any registry-reachable structure holding one of those
pointers read freed memory in pass_calls.
AddressSanitizer caught it as a heap-use-after-free (strcmp in
cbm_pipeline_pass_calls on a string freed by the pass-end cleanup) on the
first-ever run of the real-repo determinism tier (linux/fs/xfs, 355 files) --
a tier no CI runner can execute because the corpus is local-only, which is why
it survived: bisect shows it predates today's commits (b020748 reproduces),
and main is clean on the identical suite.
Ownership now transfers to the ctx at the end of the pass and the strings are
released beside the arena, in the pipeline teardown and in test_parallel's
direct-drive harness. The parallel path is unchanged: it already destroys its
registries and module strings together, before any later pass.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Second LSan round from the Linux leg, both verified green in the container
(532 passed, 0 failed under LSan):
- dump_and_persist_hashes' two semantic-manifest abort returns leaked BOTH of
the function's strdups (db_path and db_dir, the latter otherwise freed only
further down). Same ownership rule as the previous fix: every exit releases
what the function allocated.
- test_parallel's sequential harness drives the passes directly and never
destroyed ctx->seq_cross_arena, which the cross pass fills with the shared
per-language registries (stdlib registrations included -- ~20MB per test).
Production's run_sequential_pipeline destroys it after all passes; the
harness now does the same.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Maintainer decision (option B) closing the Kotlin property-reference repro's
platform divergence at its root instead of tie-breaking the fallback.
`holder::handler` (parsed as navigation by the vendored grammar) produced a
USAGE edge whose target was chosen by the name-only registry fallback: with a
same-named property (Holder.handler) and function (Functions.handler) in the
project, the winner was registration order -- readdir order -- so Windows
(lexicographic NTFS) borrowed the FUNCTION while macOS happened to pick the
property. The graph's answer must not depend on directory enumeration.
The occurrence is now claimed by exact, receiver-typed resolution. Five links,
each of which was missing:
1. extract side: a Kotlin navigation member read is a semantic-reference
candidate at the member occurrence, so an LSP row can claim it. With no row
the join finds nothing -- but a candidate no longer falls back to the
name-only registry guess, which is the fail-closed direction this PR is
built on.
2. kotlin cross resolution: a property READ with a proven receiver emits a
CALL_REFERENCE row against the property. The property is not a callable
target, so the join can only produce USAGE, never a fabricated
CALL_REFERENCE. Value reads only; calls stay with the invocation machinery.
3. receiver typing across files: kotlin_resolve_class_name composed
<this module>.<name> for an unimported cross-file type, which can never
name a type defined in another file. A per-call unique-short-name map
(built from the project defs, ambiguous names fail closed) resolves the
annotation to the real registered QN. Hash lookup, no scans.
4. cross registry fields: pxc_map_label dropped Variable defs entirely, so no
cross registry ever saw a property. They now flow through (every language's
registrar filters by explicit label, so only Kotlin consumes them) and the
Kotlin registrar attaches them as fields of their receiver type,
hash-bucketed -- a per-type scan would be the registry-tail-scan quadratic
pattern.
5. def side: class-body variables now record their declaring class
(parent_class) -- previously only methods did. The QN stays module-level,
so this is additive metadata: the only structural parent_class consumer is
Method-gated (DEFINES_METHOD), verified in pass_definitions/pass_parallel/
pipeline_incremental.
Emission targets each field's REAL def QN carried through the field map:
kotlin class properties are minted with module-level QNs (proj.Holder.handler,
not proj.Holder.Holder.handler), so the composed form would name a node that
does not exist and the join would silently drop the edge.
The repro now proves the property edge on every platform for the same reason,
not by racing readdir.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
write_temp_file used fopen(path, "w"): text mode on Windows rewrites \n as
\r\n on disk, so the fixture bytes stop matching the source string the test
reasons about. Three Windows-only failures follow: the two semantic-manifest
tests compare cbm_sha256_hex(<string>) against the pipeline's hash of the
file, and the quarantine test compares byte sizes (17 != 18). test_helpers.h
and repro_harness.h already write "wb" for exactly this reason; this brings
the one holdout in line (cbm_fopen + "wb").
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
pipeline_test_set_mtime used utimensat(AT_FDCWD, ...), which does not exist on
Windows, so the whole Windows test build failed to compile. Set the same
instant through SetFileTime instead: FILETIME is 100ns ticks since 1601 -- the
representation cbm_path_info_utf8 reads back -- so the round-trip loses
nothing the incremental pipeline can observe. POSIX keeps utimensat.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Maintainer decision closing the last open RED from PR item 2.
`callback = handler` contains a genuine value occurrence of handler, and its
exact callable identity is proven -- the alias binding below could not be
created otherwise. The policy of this PR is proven exact value =>
CALL_REFERENCE, so that occurrence now emits a resolved reference row instead
of remaining an ordinary USAGE. Bare identifier RHS only, and the usage-carrier
rule in extract_usages.c is extended in lockstep, Python-gated -- a candidate
the LSP never matches would change nothing, but the asymmetry would be a trap.
The strategy distinction mirrors the argument path: a source name that is
itself a local alias claims lsp_callable_alias, so the local-shadow guard
admits exactly what it admits for arguments and refuses everything else.
In the graph the two occurrences inside `argument` -- the RHS and the alias
use in accept(callback) -- surface as ONE CALL_REFERENCE edge and zero USAGE
edges, because edge dedup is by (src, tgt, type) and collapsing multiple sites
between the same pair is its existing, intended behavior. That is precisely
what the fixture always asserted, so it needed no relaxation, only the comment
recording the decision.
Also: GCC rejects repro_call_argument_matrix_b's _Static_assert comparing
enumerators of two different anonymous enums (-Werror=enum-compare); clang
accepts it, so only the Linux leg saw it. Cast both sides to int.
repro_reference_precision + call_reference_contract: 100 passed, 0 failed.
This closes all five item-2 REDs.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
PR item 2. Four of its five REDs; the fifth is a policy question left open.
THREE SHADOW REDS -- a rebound name kept looking exactly bound.
An imported `callback` that Python has already rebound must lose exact callable
proof, or the graph reports a reference to a callable the name no longer refers
to. Three binders were invisible:
(callback := 0) module-level walrus
[(callback := 0) for _ in (0,)] walrus inside a comprehension
type callback = int PEP 695 type alias
The first two because a module-level expression statement is routed to call
resolution and never reaches the binding scanner at all, so no binder inside one
was ever seen. The third because the scanner had no branch for
type_alias_statement, and its generic recursion only descends -- an identifier
alone binds nothing.
The comprehension case needed care in the other direction: the whole node was
skipped, correctly, because its iteration variables are private to it. But an
assignment expression binds in the CONTAINING scope (PEP 572), which is exactly
what separates it from every other binder a comprehension can hold. It is now
scanned for walrus targets only.
PARENTHESISED CALLABLE ARGUMENT -- `accept((handler))` produced no reference.
The semantic row was recorded on the outermost parenthesised argument while the
usage carrier stopped at the parentheses and was never marked a callable-value
candidate at all, so the occurrence-exact join had nothing to join. A bare
identifier now climbs to the same wrapper python_direct_callable_attribute_site
already returns for the bound-method form, so `accept((handler))` and
`accept((service.handler))` agree on one occurrence instead of disagreeing.
That climb uses the cursor the caller already holds and checks the parent kind
before doing anything. Walking up with ts_node_parent instead costs one slow
fallback per identifier in the file, which the linearity guard in
test_extraction.c rejects -- correctly, and it caught exactly that here.
The fixture's expected span was also wrong: "((handler))" is the argument LIST,
one byte wider on each side than either party, and per argument it would collide
for any call with more than one argument. It now names "(handler)" -- the
occurrence both sides actually use. Production was genuinely broken; the fixture
was additionally wrong about where.
LOCAL CALLABLE ALIAS -- a proven alias was refused by the shadow guard.
`callback = handler` makes callback a local, so the usage is a local shadow, and
a shadowed usage may only claim a semantic reference through the alias strategy.
Python emitted lsp_callable_value_reference for every callable argument, so the
one case the guard exists to admit could never satisfy it.
A lexical binding that does not simply name the module symbol of the same
spelling is an alias introduced in this body, and only that now claims the alias
strategy. A module function referenced by its own name keeps the strategy it has
always had, so the guard still refuses everything it refused before.
STILL OPEN: the same fixture asserts no ordinary USAGE from `argument`, and the
right-hand side of `callback = handler` is a second genuine occurrence of
handler as a value. Whether that should become a CALL_REFERENCE (this PR's own
proven-value policy, making the count 2) or emit nothing is a graph-content
decision, not a defect, and is left for the maintainer.
repro_reference_precision, call_reference_contract, extraction: 371 passed,
1 failed (that fixture).
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
main and this branch each grew their own atomic publication while the branch was
outstanding, and neither knew about the other:
main cbm_pipeline_run() copies the live database into an mkstemp staging
file, points the whole run at that copy, and renames it over the
destination at the end. That is what stops an INCREMENTAL run from
mutating the live database in place.
branch cbm_pipeline_publish_generation() builds a generation in its own stage
file, validates it (integrity check, FTS rebuild, seal), quarantines a
corrupt destination to a fresh .corrupt name, then renames.
The rebase kept both, nested, so the branch's layer ran against main's staging
file rather than the real database. Two consequences, both silent:
1. The quarantine became a no-op. Its "existing destination" was a staging file
we had just created, so a genuinely corrupt destination reached
cbm_rename_replace and was overwritten -- the one copy of the bytes that
would explain the corruption, destroyed by the recovery path.
2. Every publish failure collapsed to CBM_NOT_FOUND, because the outer wrapper
returned a bare -1 for its own errors, a cancellation, and a failed persist
alike. A caller could not tell "aborted, your data is intact" from "the
persist failed", which is the only distinction that matters at that moment.
Rather than pick one implementation, put each concern at the layer that owns it.
The outer wrapper owns the real destination's lifecycle, so it now owns the
quarantine: prepare_publish_destination() calls the branch's existing
prepare_existing_generation_for_replace() when the destination could not be
copied, which moves it aside only when it is verifiably not a readable SQLite
database. A destination that is valid (the backup failed for some other reason)
is sealed and replaced as before, never renamed away, so a good database is
never mislabelled .corrupt. main's guard that refuses to drop sidecars holding
uncommitted pages is kept ahead of it. A failed rename rolls the quarantine
back, so a caller is never left with no database at all.
The wrapper also stops flattening. Everything it does happens before the
publishing rename, so an abort there is genuinely non-destructive and can say
so: cancellation reports CBM_PIPELINE_ABORT_PRESERVE_DB, a failed seal reports
CBM_PIPELINE_PERSIST_FAILED, and a status from the inner publish propagates
unchanged. Both MCP call sites test only `rc == 0` and are unaffected; the codes
stay in pipeline_internal.h beside the stages that raise them, and pipeline.h no
longer claims a -1 it does not return.
Quarantining is now also correctly refused at the inner layer, which was the
same confusion in the other direction. publish_generation's destination is the
staging file the wrapper created moments earlier, so when that file was not a
readable database it was being parked as
`<db>.stage.<random>.corrupt` -- debris named after a temp file, which nothing
collects and no one can interpret. It only surfaced once the status codes stopped
collapsing, because the assertions that count leftover stage artifacts sat behind
the return-code assertions that failed first. The inner caller now discards an
unreadable staging file instead, and only the wrapper, which owns the user's real
database, ever quarantines.
Two of main's own tests asserted the bare -1. Both are named
cancelled_*_reindex_preserves_committed_db and now assert
CBM_PIPELINE_ABORT_PRESERVE_DB -- the value whose name is their subject. That
tightens the assertion rather than relaxing it: -2 answers their question and
-1 did not.
Closes the last 10 pipeline failures from the rebase.
pipeline, incremental, store_nodes, mcp: 662 passed, 2 skipped.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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>
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>
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>
Martin Vogel