Two changes that move the delta path toward O(change) without touching
the parallel-resolve contract.
Proxy narrowing. The delta executor pre-loaded every project node so
resolution could find cross-file targets by qualified name. Nodes that
resolution can never look up are pure load cost, and on the kernel they
dominate: six of its 8.5M nodes are Macro. The set is narrowed by
EXCLUSION rather than an inclusion list, deliberately -- an earlier
inclusion list was disproven by counterexamples it did not anticipate
(synthetic Decorator nodes, then Macro), and excluding a short list of
labels that are never lookup targets fails safe where guessing the full
inclusion set did not.
Patch-time identity mapping. A resolver that upserts a symbol the
narrowed set did not pre-load now produces a stand-in node; the patch
maps it back onto its existing row by qualified name instead of raising
the UNIQUE violation the previous patch would have. Nodes from CHANGED
files cannot collide here -- the purge removed them -- so repaired files
still receive fresh rows. The map is a sorted array searched by
bisection, not a CBMHashTable: that table stores key POINTERS without
copying them, which a stack-formatted integer key cannot satisfy.
NOT attempted here, and recorded instead: making the proxy load itself
lazy. cbm_parallel_resolve documents main_gbuf as READ-ONLY during its
worker phase and its workers do call cbm_gbuf_find_by_qn on it, so a
find-time materializer would mutate a buffer under concurrent readers.
A safe version needs the materialization hoisted ahead of the worker
phase; that is a separate change with its own verification.
Kernel one-file warm: 50.8s -> 42.7s wall, peak RSS 13.2 -> 10.8GB,
proxies 8.5M -> 2.5M (preseed 18.3s -> 9.7s), and the run reports
remapped=0 -- no resolver needed a symbol the narrowing dropped. 535
pipeline/incremental/store/cross-repo/integration tests green, including
the full convergence matrix.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
closure_probe_surfaces runs cbm_parallel_extract to compute the changed
files' fresh surfaces, and parallel extraction builds the process-global
package map as a side effect. Both real extraction paths release it at an
explicit ownership boundary; the probe borrowed the machinery without
inheriting that contract, leaking one map per probed run.
Found by the macOS leak lane added earlier in this branch -- the lane
catching a defect introduced after it, which is the point of having it.
Verified: the incremental suite is clean under LSan with the fix, and the
same allocation site (cbm_pkgmap_build via merge_pkg_entries) no longer
appears.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Two more measured blocks out of the delta path, closing the
optimization arc:
Parallel rehydration. The base-def decode (2.37M defs from ~89k
surface rows at kernel scale) fans out across workers with per-worker
arenas that live exactly as long as the resolve borrows them; assembly
stays in row order, so the registration input is byte-identical to the
serial loop's. Measured 315ms for the block; the arc also disproved an
earlier attribution -- the planner including its full surface load is
608ms, no projection needed.
Known-healthy finalize. prepare_existing_generation_for_replace runs
PRAGMA quick_check over the ENTIRE outgoing generation to choose
replace-vs-quarantine -- 35.5s of full-database page scan at kernel
scale. The delta route cloned that same file and ran complete
transactions against the clone minutes earlier; a corrupt live
database cannot reach the delta finalize because every earlier step
fails it into the dump path, whose finalize keeps the check and the
quarantine semantics unchanged (as its corruption tests continue to
prove). Sidecars are still removed on the fast path -- a replaced
database must never inherit the old generation's WAL. Delta publish
total: 35.7s -> 224ms.
finalize/publish timing brackets stay as durable telemetry.
Kernel one-file warm across today's arc: 306s (binary routing) -> 223s
(closure via dump) -> 121.7s (delta) -> 85.3s (query plans + shadow
gate) -> 50.8s wall / 34.3s worker; peak RSS 33.6 -> 13.2GB. Remaining
named blocks: proxy preseed 18.3s and repair/mdi 11.1s, both
serial-bound gbuf/index builds -- recorded follow-ups, not mysteries.
437 pipeline/incremental tests green throughout.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The cov_timing_mark instrumentation existed to locate one block (it
found the shadow rebuild); the previous commit shipped with it still in
place, including a call after a return that cppcheck rightly flagged as
unreachable — that commit went out with the lint gate RED because the
push was chained without depending on the gate result. The scaffolding
is gone; the durable publish.timing brackets in pipeline.c remain.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Two measured pathologies out of the delta path, both shared-code fixes
that help every route:
Join order. The inbound-edge snapshot and the dependent-files lookup
let the planner start from EDGES, walking every project edge through
the url_path index prefix — 14.6s for a one-file closure against the
kernel's 16.5M edges. CROSS JOIN pins nodes-first (idx_nodes_file →
idx_edges_target → primary key): measured 4ms for the same query, 23ms
for the full snapshot+purge step.
Coverage shadow graph. cbm_store_coverage_replace_ex rebuilt the
miss-graph shadow view wholesale inside every publish — wipe plus tens
of thousands of node/edge upserts probing the full-size nodes index,
23.4s at kernel scale — even when the failure-row set it derives from
was byte-identical. The rebuild is now gated on a sha256 fingerprint of
the failure rows persisted in store_meta: unchanged set, provable
no-op, skipped. Measured 84ms steady-state; the rebuild still fires
whenever the set actually changes, and the shadow output is untouched.
The benchmark probe also stops appending a trailing comment that
happened to break bootp.c's parse — a probe that mutates the failure
set on every run measures the shadow rebuild, not the repair. It now
edits inside the license-header comment.
Kernel one-file warm, steady state: 111.6s -> 85.3s wall (71.1s
worker). Remaining measured blocks: base-def rehydration ~35s, preseed
18.8s, repair 10.8s — the parallelization targets. 554 store/pipeline/
incremental tests green.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Three defects the kernel and django corpora exposed in the delta
executor, each caught by its own fail-closed design and each fixed at
the root:
Global id watermark. MAX(id) was project-scoped while node ids are one
keyspace for the whole database; a fresh node collided with a row
outside the project filter (UNIQUE nodes.id on django). The watermark
now clears every row.
Full preseed. The label-filtered proxy set immediately met its
counterexamples: synthetic Decorator nodes on django failed the patch
via the QN constraint, and Macro -- six million of the kernel's 8.5M
nodes -- was absent entirely, which would have silently dropped
cross-file macro edges rather than failing. Curating an
edge-endpoint-label list is guessing; every project node is now a
proxy. The load stays edge-free and property-free, which is where the
old full load actually spent its time (preseed measures 14.1s against
the 38.3s gbuf load it replaces, plus that load's 16.5M edges).
In-place surfaces. publish rewrote every lsp_surface row on each delta
(delete-all plus re-upsert of ~89k serialized def sets); the patch now
deletes exactly the purged files' rows and upserts the repaired files'
fresh ones inside its own transaction, and publish skips the wholesale
rewrite behind generation->surfaces_in_place. Measured 0.12s for the
whole write block at kernel scale.
publish_staged gains per-block timing logs; they located the next
optimization targets precisely (23.8s in the meta/coverage section at
kernel scale, integrity and seal effectively free).
Measured end-to-end on the kernel corpus: one-file warm reindex
306s (binary routing) -> 223s (closure via dump) -> 121.7s (delta),
peak RSS 33.6 -> 21.5 -> 13.6GB. django delta repair: 1.6s worker time.
Convergence suite and 437 pipeline/incremental tests green throughout.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The closure route stops loading and rewriting the world. Its executor is
now a dedicated subsystem (pipeline_delta.c + orchestration): CLONE the
live generation (copy-on-write where the filesystem offers it), repair
the closure against the clone, PATCH exactly the repaired node/edge set
in one transaction, and publish through the same sealed-staging finalize
leg as the dump path. No full graph load, no full dump, and the general
indexing pipeline is untouched -- the profiled kernel run put those two
at 187s of a 238s one-file repair whose actual resolution work was 0.6s.
Id discipline carries the design. Node ids are AUTOINCREMENT and never
reused; the small in-RAM graph is pre-seeded with PROXY nodes carrying
their real database ids (SELECT ... ORDER BY id with the id watermark
pinned before each insert), and fresh nodes are numbered above the
previous generation's MAX(id) -- so "id > max_db_id" is the complete,
marker-free definition of what the patch inserts, and every edge
endpoint id is database-valid by construction. The inbound-edge snapshot
and its QN-keyed re-link become indexed SQL; a re-link whose endpoint no
longer exists matches no row, which is full-reindex semantics for
deleted symbols.
Fail-closed throughout: an unexpected reference to an unseeded label
surfaces as a UNIQUE-constraint violation that fails the patch
transaction, and EVERY delta failure discards the stage and returns
FORCE_FULL_REINDEX -- the live database is never touched, so a full
rebuild always self-heals whatever the delta could not do.
FTS policy: nodes_fts is contentless, so purged rows cannot be deleted
individually on existing databases; their rowids can never alias a live
node again (AUTOINCREMENT) and dead entries drop out of the rowid join.
The patch inserts rows for exactly the new nodes through the same
cbm_camel_split tokenizer the wholesale rebuild uses.
The legacy gbuf-based tail reverts to serving only the test-only
force_legacy_partial route; the closure orchestration owns its own
coverage merge, publication race gate, surface-row merge and committed
counts, and publishes with fts_wholesale=false.
Gate: the full convergence suite runs against this executor unchanged --
body-edit graph equality with a fresh full index, removed-definition
dropping the dependent's stale edge, tsconfig-alias retargeting, the
decline matrix, and 510 pipeline/incremental/store/integration tests.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Two foundations for the delta-merge incremental subsystem (a dedicated
copy->patch->rename executor for the closure route; the general dump
pipeline is untouched):
cbm_clone_or_copy_file (foundation/compat_fs): stage a database by
copy-on-write clone where the filesystem has one -- clonefile(2) on
APFS, FICLONE on Linux reflink filesystems -- with a streamed copy as
the portable fallback. Verified byte-identical and write-independent.
For a multi-GB generation this is the difference between milliseconds
and seconds of staging cost.
cbm_pipeline_finalize_staged_generation: the final leg of publication
(sidecar removal, previous-generation quarantine, atomic rename with
rollback on every failure) extracted, behavior-preserving, from
cbm_pipeline_publish_generation so a patched staging copy can publish
through the exact same crash-safety tail as a dump-built one. The
FTS-rebuild and integrity-check policy deliberately stays OUTSIDE the
shared tail: the dump path rebuilds wholesale, while the delta path
will write row-level FTS inserts (safe against stale entries because
node ids are AUTOINCREMENT and never reused, so dead rowids drop out
of the join).
All 437 pipeline/incremental/publication tests green, unchanged.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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>
itoa_buf recycles a four-slot thread-local ring; the closure_plan line
passed six conversions in one call, so two fields printed corrupted --
the kernel-scale profile showed surface_changed reporting the elapsed-ms
value. Split into two calls of at most four conversions each.
The same profile run, for the record, answered the incremental cost
question with data (kernel corpus, one-file closure, worker total
238.5s): dump/publish 149.1s (62.5%), graph load 38.3s (16.1%),
wholesale semantic-edges post-pass 18.1s, manifest hashing ~20s by
residual -- while the repair itself (extract + resolve + registry
rehydration) is 0.57s. The closure algorithm is effectively free at
every scale; the remaining cost is generational I/O, which is the
delta-merge (copy -> patch -> rename) follow-up's target.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The shared cross registries are an amortization: one build over every
def so that tens of thousands of per-file resolves become O(1). A
floor-sized closure resolves at most eight files, so the build can never
pay for itself; those files take the per-file fallback path filtered
through module_def_index -- the same pre-Tier-2 resolution code the full
pipeline still uses for languages without a shared registry -- so
convergence is unchanged, as the routing-matrix tests confirm on both
paths.
Measured honestly: on the C-heavy kernel corpus this is timing-neutral
(223.3s vs 227.1s warm, within noise) -- C's registry build is not where
that corpus spends its time. The guard is kept on the strength of the
recorded registry-build pathologies (the symfony 416s and
elasticsearch 647s classes were exactly shared-registry construction),
which hit Python/TS-heavy corpora far harder than C.
Kernel A/B after the closure route (M4, torvalds/linux shallow, prod
binaries, VMs down): branch warm falls 305s -> 223-227s at warm/cold
0.73 vs main's 0.60; the residual gap over main decomposes as ~50s of
pre-existing branch overhead present in cold since before closure
existed, plus ~20s of closure machinery. Peak warm RSS across the
process tree is 21.5GB, BELOW the ~33.6GB cold peak -- the closure path
adds no memory regression.
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>
clang-format violation at pipeline.c:1430 from the forward declaration
added in 6c22338. Caught by CI rather than locally because I pushed
without running `make -f Makefile.cbm lint-ci` first, which is the whole
point of having that target.
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>
Apple clang 15 (Xcode 15.4, the macOS CI image) rejects
static _Atomic cbm_log_sink_fn g_log_sink = NULL;
with "initializer element is not a compile-time constant": NULL expands
to ((void*)0), and the implicit void*-to-function-pointer conversion is
not a constant expression there. Casting to the function-pointer type
makes it an address constant, which is what a static initializer needs.
The local ladder could not have caught this. The macOS host here runs
Apple clang 21, which accepts the uncast form; the rejecting compiler
exists only on the CI image. Recording that plainly because it is a real
gap in what local verification can promise for macOS, not a slip in how
this batch was checked.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The level, format, sink and sink-mode globals are written by whichever
thread configures logging and read by every thread that logs: daemon
connection workers, pipeline workers, the watcher. They were plain
globals.
For the sink this is not a benign stale-value race. emit_line() read the
global function pointer, tested it, then called it -- so a concurrent
cbm_log_set_sink_ex() could turn a checked pointer into a call through a
NULL or partially-written one. The load is now done once into a local,
which closes the test-then-call window as well as the tear.
Ordering is relaxed: each value is an independent scalar with nothing to
publish alongside it, and the log path has to stay cheap enough that no
caller is tempted to route around it. cbm_log_set_sink_ex stores the mode
before the sink, so a reader that sees the new sink cannot then read the
mode belonging to the previous one.
Found by ThreadSanitizer once the daemon_runtime suite was added to the
TSan set. That suite had been excluded from the lane, which is precisely
what kept this hidden.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Two follow-ups to the MSan lane, both from running it for real.
1. compat_thread.c gains a sanitized-build-only stack FLOOR
(CBM_THREAD_STACK_MB). Thread stacks here are sized in code, so a
sanitizer lane cannot raise them with ulimit -- RLIMIT_STACK at 8/64/256
MiB provably had no effect. The first version overrode only the DEFAULT
size, which silently did nothing for worker_pool/runtime/main because they
all pass an explicit size; it is now a floor applied to every thread.
Shipping builds are untouched (the whole hook is behind
CBM_SANITIZED_BUILD).
2. Two grammar-corpus suites are EXCLUDED from the lane, with the full
rationale, evidence, and everything tried recorded at the exclusion site
per O10 -- including that the floor above does NOT fix them, which narrows
the next person's search to a thread creator outside cbm_thread_create.
The exclusion list now names SUITES (an earlier version named a TEST and
therefore excluded nothing) and fails loudly on an entry that matches no
suite, so that silent-no-op cannot recur.
Also restores the exec bit on scripts/msan.sh, which the image ENTRYPOINT
needs.
worker_pool + parallel + pipeline + mcp on macOS: 526 passed, 2 skipped.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The memory-diagnostics report's priority-4 lane (path-sensitive clang-analyzer,
memory checks only) run over all 111 production files. 21 findings triaged;
the real ones, all cold-path (none can explain #581's per-query residual):
LEAKS
- mcp get_architecture: scope_path leaked on the missing-store early return
(REQUIRE_STORE frees only `project`); allocate after the gate.
- pass_definitions: cancellation mid-extraction leaked the pass-owned result
cache including already-extracted entries; mirror the end-of-pass cleanup.
- store package-boundary scan: the row-scan abort path freed the node arrays
but not the boundary accumulators or their duplicated package strings.
- cbm quarantine set: a duplicate path line leaked the replaced value (and a
fresh key copy -- the table borrows key pointers); a partial strdup failure
leaked the surviving half. Reuse the stored key for duplicates.
- pass_githistory: unchecked malloc/strdup -- an OOM dereferenced NULL and a
failed strdup leaked the index cell. Allocate before claiming the slot.
NULL/UB
- cli config subcommand: NULL argv with nonzero argc slipped the guard (the
inner `argv &&` shielded only the help comparison) into argv[0].
- store bfs_multi: a negative max_results broke out before any row was
written, then freed fields of an unwritten negative-index slot. Clamp.
- pass_calls emit_http_async_edge: the service-pattern call sites pass a NULL
target behind a hand-duplicated URL predicate; a drift between the copies
turned target->id into a null deref. The callee is now total.
- sqlite_writer: both leaf-array OOM paths left leaf_count stale with a NULL
array, walking pb_finalize_* into leaves[0]; consistent empty state routes
them to the existing root=0 failure return.
HARDENED (invariants true but invisible to path-sensitive analysis)
- Leiden CSR + aggregate arrays, SCC adjacency: calloc + endpoint guards, so
a future degree/collection miscount degrades benignly instead of UB.
- SCC cycle fill: the ncyc==0 no-slot invariant made local.
RECORDED FALSE POSITIVES (no code change)
- yaml sequence starts (loop bound == alloc bound), cypher agg arrays (same
count both sides), mcp read_message ch (assigned by fgetc each iteration),
pkgmap clean buffer, mcp csize (Tarjan: ncomp>=1 when nverts>=1), vendored
verstable x2.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
GATE + LANES (user decision: runner cost accepted)
- make lint-mem (local triage) and lint-mem-ci (gating: vendored-filtered,
any remaining finding fails). The gate is green because every false
positive above was restructured for provability -- calloc'd fill-cursor
arrays, explicit Tarjan invariant, zeroed buffer tails, min-1-element
allocations -- never suppressed.
- make diag: pinned newest-LLVM ASan/UBSan lane with straighter stacks.
- CI: lint-mem job (_lint.yml) and test-diag job (_test.yml), both on the
pinned LLVM 22 apt toolchain. Cost disclosure: roughly +25-40 min and
+25-60 min (ccache-warm) per push respectively.
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>
resolve_db_path returns a strdup the function owns, but neither the
publish-failure return nor the success tail freed it --
cbm_pipeline_refresh_artifact only borrows the pointer. Every pipeline run
leaked one path string; LeakSanitizer on the Linux leg aborted the pipeline,
index_resilience and mcp suites over exactly this pair of exits (every leaked
allocation across the leg traced to this single strdup). macOS stayed green
because this setup has no leak detection there, which is precisely why the
Linux leg exists.
Linux container, same three suites under LSan: green after the fix.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
make lint-ci was red on this branch before any of the rebase work.
cppcheck reported all four halves of two ULARGE_INTEGER values in
cbm_path_info_utf8 as assigned-but-never-read. The code is correct -- it is a
union, and .QuadPart reads exactly what .LowPart/.HighPart wrote -- but cppcheck
does not model that aliasing, so it cannot see the read.
Composing the two 64-bit values arithmetically says the same thing without the
union, so the checker needs no exception. That is the repository's stated
preference: refactor first, adjust the rule second, suppress only as a rare
justified exception -- and a suppression here would have to be re-justified by
every future reader.
Also applies clang-format to the lines this rebase touched in pipeline.c and
extract_usages.c, plus one pre-existing violation in pipeline_incremental.c.
Formatting only; the file set is limited to what LINT_SRCS/LINT_HDRS actually
covers, so no unrelated whole-file reflow rides along.
make -f Makefile.cbm lint-ci: passes.
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>
Two fixes on top of the main merge.
1. try_incremental_or_delete_db removed the existing database unconditionally.
On main those two lines are only REACHED on the reindex route, because the
incremental path returns early. This branch restructured the function so there
is no early return, and the merge carried main's cleanup in verbatim -- so the
database was deleted on every route, including the no-op and the successful
incremental publish.
The failure mode was badly misleading: the pipeline genuinely succeeded and
logged pipeline.done nodes=17, and cleanup then destroyed the result, so every
later reader reported dump.verify reason=store_missing. Because the probe
suites all share ~/.cache/codebase-memory-mcp and all have the shape
"index a snippet, open the store, count nodes", one suite that indexed twice
destroyed the shared database and every probe suite after it failed at its
store-open gate.
Full suite went from 6539 passed / 836 failed to 7354 passed / 21 failed. The
narrow arena that isolates it -- test-runner mcp node_creation_probe -- went
from 83 failed to 269 passed, which is exactly main's number for that pairing.
2. pass_parallel.c sized its usage-properties buffer at 256 bytes while the
sequential twin in pass_usages.c uses 512.
esc_ref holds up to 255 bytes and the {"callee":"..."} wrapper adds 13, so a
long callable identifier truncated mid-string, cutting the closing quote-brace
and persisting malformed JSON into edge properties. The two paths must also
agree: the same repository indexed in parallel and sequentially has to produce
byte-identical edge properties. This is RED item 4 from the PR description, and
it was a live defect on main rather than only on this branch.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Wires the registry-driven generators into install, plan and uninstall.
pi gets ~/.pi/agent/extensions/cbmem.ts, its only route to the graph -- pi has
no MCP client. OpenCode gets ~/.config/opencode/plugins/cbm-augment.ts, which
adds no tools (it already reaches all of them over MCP) and supplies only the
automatic graph lookup before a grep/glob that other clients get from their own
hook configuration. OpenCode has none; its plugin system is the only extension
point, verified against their plugin documentation.
Both go in as a MARKED BLOCK rather than a whole-file write. Those directories
are auto-loaded, so a user may legitimately keep their own module there, and an
install routine that clobbers it is destroying user content. Uninstall removes
only our block and leaves the rest of the file, matching every other uninstall
path here.
A generator returning NULL is a hard error rather than a skip. A silently absent
extension is precisely the failure that left the earlier proposal a no-op for six
weeks, so it must surface as an install error instead of a quiet success.
Note for reviewers: the generated body deliberately carries NO ownership
markers. cbm_text_upsert_managed_block adds them itself and rejects content that
already contains them -- an existing guard test (test_cli.c:6018) caught that
integration mistake before CI did, which is the test doing exactly its job.
Reimplements the ideas from two community PRs, both closed as superseded by this
work, with credit to their authors:
- #534 (@Tensorboyalive) proposed the pi extension. It embedded 380 lines of
TypeScript as C string literals and registered 7 of the 15 registry tools;
this generates from the registry so the surface cannot drift, and ships no .ts
in the repository. Their marker-delimited editing, their uninstall test
asserting the markers are GONE, and their defensive stdout parsing (take the
last line that parses, not the first) are all carried forward here.
- #616 (@ZeR020) proposed the OpenCode plugin. Its payload omitted
hook_event_name, which hook-augment requires, so it produced zero bytes; and
its path template rejected the double quote but not the backslash, so a
Windows home like C:\Users\urs\bin produced an invalid unicode escape and the
auto-loaded plugin failed to parse. Both are closed by construction here and
pinned by tests. Their hook-point research is what made the OpenCode side
possible at all.
Tests: install writes both extensions and the dry run does not; uninstall
removes our block while preserving a user's own content in the same file.
Co-Authored-By: Tensorboyalive <Tensorboyalive@users.noreply.github.com>
Co-Authored-By: ZeR020 <ZeR020@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
pi has no MCP client, and OpenCode has no declarative hook configuration --
verified against OpenCode's own plugin documentation, which states hooks are
available only through JavaScript/TypeScript plugin modules. For those two
clients a module is the only extension point.
We do not want to ship such a module as a repository asset. #534 proposed one
embedding 380 lines of TypeScript as C string literals and registering 7 of the
15 registry tools; every tool added afterwards would have been silently missing
for that client. Hand-maintained copies of the tool list have already produced
defects here (#1361, and the smoke-invariants count).
So the module is GENERATED from the live registry instead. cbm_client_adapter_pi
walks cbm_mcp_tool_count()/cbm_mcp_tool_name() and registers every tool, which
makes drift structurally impossible rather than merely discouraged: adding a
tool to TOOLS[] adds it to every generated adapter with no second edit. Nothing
in the repository is a .ts file.
Both emitters wrap their output in ownership markers so a caller can rewrite its
own block and leave a user-authored file alone.
Two defects from the prior proposals are closed by construction:
- Path escaping. #616's template rejected the double quote but not the
backslash, so a Windows home like C:\Users\urs\bin produced an invalid
unicode escape and the whole auto-loaded plugin failed to parse, which is
worse than an absent plugin. cbm_client_adapter_escape_js escapes backslash,
quote, newline and CR, and fails closed rather than emitting a truncated
literal; generation aborts if the path cannot be escaped.
- The silent no-op. #616's payload omitted hook_event_name, which hook-augment
requires and without which it accepts nothing, so the plugin emitted zero
bytes for six weeks. The generated payload carries it and a test pins it.
Recorded honestly: the OpenCode emitter hooks tool.execute.after, whose ability
to modify a tool's output is NOT part of OpenCode's documented plugin contract
(only tool.execute.before's argument mutation is). If they change it the
augmentation stops with no error. That risk is accepted deliberately and the
emitter says so in a comment, so a future reader does not have to rediscover it.
This commit adds the generator and its tests only; wiring into the install and
uninstall routines follows separately so the two are reviewable apart.
Tests: every registry tool appears in the pi module (revert-checked by
simulating the 7-of-15 subset, which fails); Windows/quote/newline escaping and
its truncation boundary; the OpenCode payload carries hook_event_name and
registers no tools; NULL/empty binary paths generate nothing.
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>
Martin Vogel