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>
LABEL_GOLDENS pins the node labels each grammar emits. The nix case is
`{ foo = 1; bar = 2; }` — two module-level scalar bindings — and its golden was
Module:1, recorded when no Nix binding could be named and none was minted.
Both are Variables now, so the golden is Module:1,Variable:2. The mismatch CI
reported is the intended behaviour change, not a regression.
Extraction, pipeline, grammar_labels, grammar_regression and lang_contract:
534 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
nix_module_level_bindings_mint_variables
a `let` binding and an attrset binding both mint Variables, and the QN
carries the attrpath (services.nginx.enable).
nix_nested_bindings_are_not_module_level
a binding two attrsets deep mints nothing, and neither `deep` nor `nested`
becomes a Variable since both are scopes.
nix_lambda_binding_is_function_not_variable
a lambda-valued binding is a Function and NOT a Variable; a scalar binding
is the reverse.
Every absence assertion is paired with a positive one on the same predicate in
the same result — `topLevel` in the nesting test, `val` in the split test — so
none of them can pass by extracting nothing at all. Several false "clean" results
in this area came from predicates that could not have matched.
Signed-off-by: Jason Bowman <jason@json64.dev>
`nix_var_types` has declared `binding` since the language was added, but no Nix
case existed in extract_var_names and no path reached a binding in the first
place, so the Variable count for Nix was unconditionally zero.
Two things were in the way. extract_variables iterates only the file root's
DIRECT children, and a Nix file's root child is its header lambda
(`{ pkgs, lib, ... }:`), so the generic loop saw nothing — the same shape as the
definition bug. And extract_var_names had no Nix branch, so a binding could not
have been named even if reached.
Scope follows the rule the other languages already use rather than inventing one.
extract_variables mints FILE scope and never locals: a C++ declaration inside a
function body is not a Variable. For Nix, file scope is the `let` bindings and
the returned attrset — a let binding is file scope in the same sense a C++
file-static is, and the attrset is the exported surface. A binding in a deeper
attrset is not.
That bound is load-bearing. Every Nix binding's parent is a binding_set at any
depth, so admitting them all would mint a node per `enable = true` in a NixOS
module's settings tree — the per-leaf flood the Helm values.yaml case in this
same function already exists to avoid.
Bindings whose value is a lambda or an attribute set are skipped: the first is
already minted as a Function by the def walk, the second is a scope
(is_namespace_scope_kind), so minting either here would double-count it.
Names follow the attrpath convention from the previous commit — leaf as the name,
whole path in the qualified name, so `services.nginx.enable = true` is name
`enable`, QN proj.file.services.nginx.enable. push_var_def grows a _qn variant
for that; the plain form delegates to it and is unchanged for every other
language.
Extraction and pipeline suites: 494 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
Four extraction cases and one pipeline case, each asserting a distinct claim so a
regression names what it broke:
nix_attrset_scope_disambiguates_leaf_names
two attrsets each holding `dup` produce two definitions with distinct QNs.
Unqualified they shared one QN and the second was discarded.
nix_dotted_attrpath_qualifies_like_nested
`wrap.deep.fn = …` and `wrap = { deep = { fn = …; }; }` produce the SAME
QN. This is the equality that makes attrpath qualification a correctness
fix rather than a preference — the two are the same expression.
nix_quoted_attr_name_strips_quotes
`"kebab-case"` is named kebab-case, and asserts the quoted form is absent
rather than merely that the bare form is present.
nix_interpolated_attr_mints_no_def
`"${dynamic}" = …` mints nothing, and the sibling binding still does — so
the test cannot pass by extracting nothing at all.
pipeline_nix_scoped_binding_calls_resolve (tests/test_pipeline.c)
a call inside a scoped binding reaches its target through the store, for
both routes into a qualified name: an enclosing attrset and a dotted
attrpath.
The pipeline case has to live at that level. Definition QNs and call-scope QNs
come from two separate functions, and if they disagree the edge is dropped at
write with no error — every extraction-level assertion still passes while the
graph quietly loses edges. Only the store shows it.
Adds has_def_qn alongside the existing has_def. find_def_by_name returns the
first match by NAME and so cannot tell two same-named definitions in different
scopes apart, which is exactly what these tests exist to separate.
Extraction and pipeline suites: 491 passed, 0 failed.
Signed-off-by: Jason Bowman <jason@json64.dev>
A Nix binding's name is a PATH, and the resolver took only its first segment
(`child_by_field_name(attrpath, "attr")`). Three defects followed from that one
modelling gap:
- `setA = { dup = …; }` and `setB = { dup = …; }` both minted the qualified
name proj.file.dup. The second definition, and every CALLS edge sourced from
it, was silently discarded at write.
- `a.b.fn = …` minted a definition named `a`, colliding with every other
binding whose path began `a`.
- `"kebab-case" = …` minted a name with the quote characters in it, so every
consumer keying on the name had to know to re-quote.
Adopts the convention already used for C++ namespaces — name is the leaf
segment, qualified_name is enclosing scope plus leaf, so `ns::serialize` is name
`serialize` and QN proj.file.ns.serialize. For Nix that means:
- the resolver returns the LAST attrpath segment;
- leading segments become scope, so `a.b.fn = …` and the nested spelling
`a = { b = { fn = …; }; }` produce the same QN, as they must — one is sugar
for the other;
- a binding whose value is an attribute set is a scope, mirroring
is_namespace_scope_kind's treatment of a C++ namespace. `let` bindings are
deliberately NOT scopes: they are lexical, and C++ does not qualify by block
scope either;
- a quoted segment is unquoted, and an interpolated one (`"${x}" = …`) mints
nothing at all, since it has no statically knowable name — the same call the
Makefile dot-prefix guard makes.
The two scope sources compose: `setA = { a.b.fn = …; }` is
proj.file.setA.a.b.fn.
Definition QNs and call-scope QNs are computed by two different functions —
extract_defs.c's compute_class_qn and extract_unified.c's own. If they disagree
by one segment the CALLS edge names a source node that was never minted and is
dropped at write, with no error and no failing extraction assertion. Rather than
implement the rule twice, the Nix computation lives in helpers.c and both call
it, which makes that class of drift impossible instead of merely unlikely. Note
also that the def-side enclosing-QN gate is language-gated while the call-side is
not, so widening one without the other would have produced exactly that mismatch.
Signed-off-by: Jason Bowman <jason@json64.dev>
The acknowledgement text described a state of the world that ended when
0.9.1-rc.1 published: it told every new contributor we were still finishing
release-critical work and that "review resumes once the release path is clear".
Posting that now would be false on the day it starts running.
This is the obligation the PR description already set for itself -- the
review-status section is a public promise and has to be edited when the
situation it describes changes. Doing that before the workflow ever posts,
rather than after someone reads a stale freeze notice, is the point.
The queue is still real, so the section stays rather than being removed: the
freeze is over, a large backlog is not, and PRs are being read oldest-first.
Saying that is more useful to someone opening a PR today than either the old
text or silence, because it explains why a recent PR sits behind older ones.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
CodeQL flagged two high-severity alerts in the previous commit: the help-list
builder accumulated snprintf's return value, which is the length it WOULD have
written rather than what it did. Once len passes cap, the next `cap - len`
underflows to a huge size_t and the following write lands outside the buffer.
That is the same class the Cypher UNWIND list builder was just fixed for in
#1173, where blen advanced past a 2KB buffer the same way.
The capacity computed here is in fact sufficient for the current registry, so
nothing overflows today -- which makes this the more dangerous shape, not the
safer one. The code was correct only by an argument made ten lines away in the
cap loop, so renaming a tool or changing the wrap rule would have turned it into
an out-of-bounds write with nothing in the function to notice.
help_append() returns the bytes ACTUALLY written, clamped to the space left, so
`len <= cap - 1` becomes a local invariant that holds no matter what the
registry or the wrap column later become. Also adds the <stdarg.h> the variadic
helper needs.
Verified: builds clean under -Werror; --help still renders all 15 tools in
registry order with the same wrapping; mcp + cli suites 437 passed, 0 failed;
make -f Makefile.cbm lint-ci clean.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The top-level --help listed 14 hand-maintained tool names and omitted
check_index_coverage; MCP tools/list correctly advertised all 15. The
help string was a separate copy of the registry, so it drifted silently
when the tool was added.
Render the Tools block from the TOOLS registry itself (new
cbm_mcp_tools_help_list, plus cbm_mcp_tool_count/cbm_mcp_tool_name
accessors) so the two lists share one source of truth and cannot
diverge again. A unit test pins the render against the registry:
every advertised tool appears, cardinality matches, and lines stay
80-column safe.
Fixes#1361
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
13 soak directories and 4 memlab files -- 78 files of daemon logs, latency and
metrics CSVs, and summaries -- were committed to the repository root in 7808eee
("revert(daemon): drop the connection thread pool"). They are local run output
from manual soak legs: no source, no tests, nothing anyone needs from a clone,
and they sit in the root listing above scripts/ where they are the first thing a
visitor sees.
Untracked, not deleted: every file is preserved locally under private/ (already
gitignored), so the measurements remain available for comparison without being
part of the published tree.
.gitignore is widened so this cannot recur. It already carried `soak-results/`
and `soak-results-query-leak/`, but every directory that actually landed was
hand-named for its run -- soak-mac2, soak-ql-mac, soak30-linux -- so none of them
matched. The new patterns are ROOT-ANCHORED (`/soak*/`, `/memlab-*`) rather than
bare globs, verified so scripts/soak-legs.sh, scripts/soak-test.sh and
tests/test_soak_daemon_recovery_contract.sh stay tracked; only run directories at
the repo root are caught.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Run 30504814537 built all eight platforms successfully with skip_tests=true, and
reported:
test: skipped (intended)
smoke: skipped (NOT intended)
soak: skipped (NOT intended)
release-draft: running
GitHub propagates "skipped" TRANSITIVELY down the needs graph. `build` carried
`!cancelled() && !failure()` so it overrode the skipped `test` and ran; `smoke`
had no `if:` and `soak` only checked soak_level, so both inherited the skip.
Nothing failed. Nothing announced it. The run then proceeded toward publishing
binaries that had never been smoke-tested or soaked -- the exact opposite of what
skip_tests was for, which was to skip a phase already proven green, not to
quietly stop testing the artifacts.
The second half of the defect is worse than the first: `release-draft` guarded
itself with a bare `!cancelled() && !failure()`, which is fail-OPEN. A skipped
job is neither cancelled nor failed, so a skipped smoke sails straight through
it. The draft was one gate away from being cut from unverified binaries, and the
only reason it was not is that the run was cancelled by hand.
Fixes, both directions:
- smoke and soak get `!cancelled() && !failure()`, so a deliberately skipped
ancestor no longer skips them;
- release-draft now requires `needs.smoke.result == 'success'` and enumerates
`needs.soak.result` explicitly, so a SKIPPED smoke BLOCKS the draft, and a
legitimately skipped soak (soak_level=none) stays distinguishable from a soak
that never ran.
tests/test_release_gate_chain_contract.sh pins both properties: every job
downstream of the optional phase must carry the override, and the draft must
demand smoke success rather than smoke non-failure. Verified in both directions
-- it passes on this workflow and fails with 2 violations when the previous
smoke/soak conditions are restored.
Nothing was published: the release remained a draft (isDraft=true,
publishedAt=null) and the run is cancelled.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Completes the previous commit, which carried only the two mode changes because
the call-site edits were not staged when it landed (--amend is denied in this
repo, so this is additive rather than a rewrite).
Call sites now go through `bash`, which is what most of this repo already does and
which cannot break if a mode bit is lost to a patch application or a non-POSIX
checkout:
scripts/package-release.sh -> bash scripts/ci/check-binary-composition.sh
.github/workflows/release.yml -> bash scripts/ci/append-vt-notes.sh
With the 100755 modes from the previous commit, both sides are now correct, and
either alone would have been sufficient.
tests/test_script_exec_bit_contract.sh pins the class so it cannot recur: any
tracked .sh whose COMMITTED mode is non-executable must not appear as the first
word of a command in workflows, scripts, test-infrastructure or the Makefiles.
The committed mode is the thing that matters and the thing no local run can
check -- the working copy having the bit is exactly why this shipped.
Verified in both directions: passes on this tree, and fails on the exact defect
when the mode and the call site are reverted together. It joins backslash
continuations before analysing, because its own first draft reported a false
positive on
... && bash \
test-infrastructure/vm/vm-run-tests.sh --soak
where the interpreter sits on the preceding line. A contract that cries wolf
teaches people to ignore contracts, so that had to be right before it could be
useful.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Every unix build leg of release run 30499236230 died during packaging:
scripts/package-release.sh: line 190: scripts/ci/check-binary-composition.sh: Permission denied
The composition gate was committed 100644 while being invoked as a command. It
passed every local check because my WORKING COPY had the exec bit -- only the
committed mode was wrong, and nothing you can run locally reveals that.
scripts/ci/append-vt-notes.sh had the identical defect waiting in the verify
step, the last step of the release, so this would have failed a second time after
two hours of tests, build, smoke and soak.
Fixed on BOTH sides, because either alone suffices and the pair is mode-proof:
the two scripts are now 100755, and their call sites invoke them through `bash`,
which is what most of this repo already does and which cannot break if a mode bit
is ever lost to a patch application or a non-POSIX checkout.
tests/test_script_exec_bit_contract.sh pins the class: any tracked .sh whose
COMMITTED mode is non-executable must not appear as the first word of a command
in workflows, scripts, test-infrastructure or the Makefiles. Verified in both
directions -- it passes on this tree, and fails on the exact defect when the mode
and the call site are reverted. It also joins backslash continuations before
analysing, because its own first draft reported a false positive on
... && bash \
test-infrastructure/vm/vm-run-tests.sh --soak
and a contract that cries wolf teaches people to ignore contracts.
Product code is untouched: the test phase of the failed run was 27/27 green on
this exact tree, and a file mode cannot change a test outcome.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cppcheck blocked the release run, correctly:
src/main.c:283: Condition '!main_test_worker_project_lock_marker(mutation)' is always false
src/main.c:2049: Condition '!worker_start_watchdog_test_descendant()' is always false
Compiling the seams out left behind stubs that returned true unconditionally, so
negating them was provably constant. Refactored rather than suppressed (repo
rule): the stubs are deleted and the CALL SITES are guarded, so a release build
does not contain the call at all -- which is what "the seam is not in the
artifact" should have meant in the first place.
The worker path needed care. The probe was one term in a five-term `||` chain,
and its POSITION is load-bearing: it must fork AFTER the isolated process group
exists (it inherits that group) and BEFORE the parent-death watchdog thread
starts, because forking a multithreaded process is the bug that ordering avoids.
Hoisting the call out of the chain would compile, pass tests, and silently break
that invariant. So the chain is split into three ordered steps -- process group,
then (test builds only) the probe, then the watchdog -- sharing one
worker_containment_unavailable() failure path so every step still fails
identically, with write()/_exit() rather than stdio and atexit handlers.
Verified in BOTH directions, since a guard that is always off is as wrong as one
always on: a TEST_SEAMS=1 build still contains the probe and
tests/test_worker_watchdog.sh passes against it, while the release build carries
no seam string. `make -f Makefile.cbm lint-ci` now passes locally -- the step
skipped before the last push, and the reason CI found this instead of me.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The release build blocked at Layer 8 (vendored dependency integrity):
MISMATCH: vendored/mimalloc/src/options.c
expected: 96ef01e4...
actual: 192ce06a...
That is the integrity check working exactly as designed. The hardening pass
patched mimalloc's version banner to stop baking __DATE__/__TIME__ into every
binary, which is an intentional and reviewed change to vendored code -- but an
intentional change is indistinguishable from a supply-chain edit until someone
records it, which is the entire point of the manifest. Updating the recorded
checksum is the review being written down.
Scope verified before committing: the manifest diff is exactly ONE line, the
options.c hash, and it matches the hash CI computed. No other vendored file
moved. scripts/security-vendored.sh's other layers still pass on the updated
tree -- no subprocess, network or dangerous calls in vendored code, dlopen
confined to sqlite3.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Martin Vogel