Since #1360 routed ordinary malloc/new through mimalloc on Linux, the
arena policy governs every allocation in the process rather than just the
bound sqlite/tree_sitter populations. cbm sets arena_eager_commit=0, so
mimalloc commits sub-ranges with mprotect(PROT_READ|PROT_WRITE) over a
PROT_NONE reservation, and each partial commit SPLITS the reserved VMA.
Measured on the Go corpus, Linux arm64, shipped binaries:
v0.9.0 10 mappings, at ANY worker count
v0.10.5 ~22k mappings, peak; the count tracks CONCURRENCY
(999 at 1 worker, 8460 at 4, 11965 at 18)
Two consequences, both of which #1654 reported from a 96-CPU/376 GB host:
the mmap/mprotect churn serialises on the kernel's per-process mmap_lock,
and the VMA count climbs toward vm.max_map_count, after which mmap fails
for ANY size -- so mimalloc reported it could not allocate 10 KB while
`free -g` still showed 246 GB available.
mimalloc's own default for this option is 2, meaning "eager-commit arenas
only on an OS that overcommits (i.e. linux)", precisely because commit is
free there until pages are touched. Overriding it to 0 opted Linux out of
the default written for Linux. Restore it on Linux only; every other
platform keeps the lazy setting, where commit is NOT free and the
upfront-memory reason still holds (Windows especially, #581).
Measured effect, same corpus and host, baseline build vs this build:
mappings 22450 -> 17312 (-23%)
wall 92.4s -> 92.6s (unchanged)
peak RSS 19.14 -> 19.22 GB (unchanged)
This is a partial mitigation, not a cure: the remaining ~17k mappings are
individual 64 KB-3 MB extraction buffers, each taking its own mmap (the
worker reserves ~40 GB of address space for ~19 GB of RSS). Pooling those
is the durable fix and is deliberately left out of this change.
Guard: mem_arena_eager_commit_follows_platform_commit_cost pins the
platform split so the Linux default cannot be silently opted out again.
Reproduction and controlled 2x2 (only vm.max_map_count varied) are
recorded on #1654.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
LeakSanitizer on CI caught all five new tests leaking their CBMFileResult:
Indirect leak of 24 byte(s) ... ts_tree_new
cbm_extract_file_ex cbm.c:1256
do_extract test_parse_coverage.c:39
test_dockerfile_missing_final_newline_not_flagged_issue1610:272
SUMMARY: AddressSanitizer: 706504 byte(s) leaked in 189 allocation(s)
Every pre-existing test in this suite calls cbm_free_result before PASS; the new
ones did not. The local run could not have found it - LeakSanitizer reports
"detect_leaks is not supported on this platform" on macOS arm64, so this class
of defect is CI-only here.
Each test now captures what it asserts, frees, and only then decides, so the
early-FAIL paths do not leak either. The cross-grammar loop prints its
diagnostic before freeing so the failure message keeps naming the grammar.
While correcting the guard, a first attempt left ASSERT_TRUE(flagged ||
has_ranges || true) in real_error_before_eof_still_flagged - always true, and it
would have silently disarmed the guard that stops the EOF suppression from being
over-broad. Removed. The guard is re-proven binding: forcing
cbm_is_eof_terminator_miss to return true makes EIGHT tests fail, including both
guards, and restoring it returns the suite to green.
parse_coverage 14 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
A file that does not end with a newline leaves the grammar's mandatory line
terminator MISSING. cbm_collect_error_regions counted that node, so the file was
reported parse_partial with the last line as its error range.
It is not a miss. The node is ZERO-WIDTH and sits at EOF: the parser consumed no
source for it, so by construction nothing was dropped - no construct can live in
a zero-byte span - and every real instruction above it parsed normally. Proven
by dumping the tree: the reporter's two-line Dockerfile yields
(source_file (from_instruction ...) (entrypoint_instruction ...) (MISSING "\n"))
with both instructions intact and the MISSING node spanning bytes 73-73.
It was never Dockerfile-specific. Stripping the trailing newline from the 156
linkable grammar fixtures flips 13 of them to has_error, and SIX produce regions:
dockerfile, tcl, fish, gomod, hyprlang - and makefile, which is a genuinely
different case (its ERROR has WIDTH; the recipe really is lost).
Worse, the ones that stayed silent did so for no principled reason. ini, fsharp,
beancount, requirements, gitignore, sshconfig and kconfig omit the same
terminator, but theirs is a HIDDEN node and hidden nodes are invisible to
ts_node_child(). Whether a user was told their file was partially parsed came
down to whether that grammar's author declared the terminator visible.
The cost was not cosmetic: a phantom parse_partial writes a "<project>::missed"
shadow row, and until #1609 that row made the project fail cross-repo validation
as BOTH source and target. A single absent byte could remove an entire
repository from cross-repo intelligence with no error shown anywhere.
The suppression is deliberately narrow - zero-width AND at EOF. A MISSING or
ERROR node with width still counts even at EOF, and anything before EOF is
untouched. Both callers pass the raw root, so one source_len is correct for
both; verified rather than assumed, since root is bound once and never
reassigned.
Reported by @vitaliy-shatskiy, who could not share the original file and instead
rebuilt the property from scratch with a byte-exact script - an editor would
have silently re-added the newline and hidden it. Their isolation matrix ruled
out BOM, CRLF vs LF, exec-form vs shell-form and file length before we looked at
it once.
Reproduce-first, revert-checked: the Dockerfile and cross-grammar tests fail on
the previous tree and pass with the fix; forcing the new predicate to return
false brings the identical REDs back. Two guards pin the boundary and hold in
both directions - a width-bearing failure at EOF (makefile) and a real
mid-file ERROR in a file that ALSO lacks its final newline (built from
C_IFDEF_SPLIT, the fixture this suite already proves is flagged).
parse_coverage 14, extraction 276, language 217, infrascan 3,
grammar_regression 1 - 511 passed, 0 failed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
`update` hands off to install.sh (install.ps1 on Windows) and prints the command
to run. It built that command from cbm_detect_self_path - the BINARY's directory
- and treated "I resolved my own location" as "the installer is beside me".
Those are different questions. install.sh is placed beside the binary by
install.sh itself, but a binary that was moved, packaged by a distro, or built
from source has no installer next to it. We printed the path anyway:
bash "/home/<user>/.local/bin/install.sh"
/usr/bin/bash: /home/<user>/.local/bin/install.sh: No such file or directory
Reported on discussion #1560 (#1632) by a user who was already three releases
deep in install trouble and had just been told, by us, to run a file that does
not exist.
`update` exists to tell someone how to proceed. Ending the interaction on a
command that cannot run is the one outcome it must not produce - and the
fallback text was already there and already correct, naming install.sh as
shipping in the release archive without asserting a path.
The probe goes through cbm_path_info_utf8 so a non-ASCII install directory
resolves on Windows, and rejects a DIRECTORY of that name, because `bash <dir>`
is not a command either. A symlink still counts: it is reported rather than
followed, and the shell runs it perfectly well.
The Windows branch gets the same treatment; it had the identical assumption
about install.ps1.
Reproduce-first and revert-checked: with the probe forced to return true - the
old behaviour - the new test fails with "a directory with no installer must not
be named as one", and passes once it is restored. cli: 276 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Indexing writes an internal "<name>::missed" miss-graph row into the SAME db as
the primary project whenever a file parses partially. cr_store_has_exact_project
required `count == 1` over ALL rows returned by cbm_store_list_projects, which
does not filter those rows - so any project that had ever recorded a parse miss
failed validation, as SOURCE and as TARGET, and the whole feature reported:
project is not indexed
for a project that plainly was. There is no user-level workaround: a partial
parse is not something the operator controls, and re-indexing reproduces the
shadow row.
This is the same defect mcp.c fixed for list_projects in #1044 ("requiring
n == 1 over ALL rows made every project with a miss graph vanish"); the
cross-repo site never learned it. The fix ports that primary-row filter.
The single-primary requirement itself is deliberately kept: it is what proves
the db belongs to the project we were asked about rather than being a shared or
mislabelled store. Only "::" shadow rows stop counting toward it.
Reported by vitaliy-shatskiy in #1609, whose diagnosis named the exact function
and the exact reason.
Reproduce-first, and revert-checked both ways:
- the new test fails on origin/main with `ASSERT(!(result.failed))`
(tests/test_cross_repo.c), for the behaviour under test rather than a setup
error;
- it passes with the fix;
- reverting ONLY src/pipeline/pass_cross_repo.c and keeping the test brings
the identical RED back, so the test binds to the production change.
The existing pair without shadow rows is the control: those tests already prove
that path returns edges, so this cannot pass vacuously on a fixture that never
matched.
cross_repo 8 passed; pipeline 249 passed; store_edges 25 and store_nodes 67
passed - no collateral change.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
`codebase-memory-mcp uninstall --help` removed the binary and every agent
configuration (#1038). It did the destructive thing to someone asking what the
command does.
The top-level dispatcher matches the subcommand at argv[1] and forwards the rest,
so its own --help check at src/main.c:1047 never sees argv[2]. Nothing downstream
looked either, and cbm_cmd_uninstall went straight to parse_auto_answer.
The guard is checked FIRST, before parse_auto_answer, so a `-y` elsewhere on the
line cannot auto-confirm the destruction we are trying to prevent. It prints real
usage, states plainly that the command is destructive, and points at --dry-run.
--help is the flag a person types precisely BECAUSE they are unsure what a
command does. It must never be the thing that destroys their install.
cli suite: 271 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The daemon/CLI rendezvous directory is created under %LOCALAPPDATA% (Windows) or
/tmp -- /private/tmp on macOS -- and every ancestor of it must pass the
private-directory walk. That ancestry is not always acceptable, and when it is
not, EVERY invocation fails, `config list` included, so the settings surface
cannot be reached either:
codebase-memory-mcp: secure daemon endpoint could not be created
#1623 narrowed the Windows side of this by admitting AppContainer package and
capability SIDs on ancestors, and named the remainder explicitly: a live local
group, Authenticated Users inherited from a secondary volume root, and orphaned
unresolvable SIDs still refuse, and "those need CBM_RUNTIME_DIR or a separate
change". #1621 is the POSIX shape of the same dead end -- /private/tmp/cbm-daemon-<uid>
refused with no way to move it.
There was no way to move it in a shipped build. The only relocation hook,
CBM_TEST_DAEMON_RUNTIME_PARENT, is compiled out unless CBM_ENABLE_TEST_SEAMS is
defined, so a test build started while the shipped build did not; CBM_CACHE_DIR
is no help either, because it moves the cache and never the rendezvous.
CBM_RUNTIME_DIR names the parent directory the rendezvous is created under. It
does NOT relax the check: the directory it names goes through exactly the same
validation as the default -- ancestors owned by you or root, not world-writable,
no allow-ACL; the rendezvous directory itself still forced to owner-only -- and a
value that fails is refused rather than silently replaced by the default. The
operator only chooses an ancestry that passes. cbm_safe_getenv never truncates,
so no half of an over-long value can become a runtime parent.
The override is resolved in cbm_daemon_bootstrap_endpoint_new(), the one function
every product endpoint goes through: the daemon, the MCP client, the local CLI,
the index worker, and the install/update/uninstall activation path in cli.c. No
call site can silently keep the default, and the detached daemon inherits the
value with the rest of its environment. An explicit parent still wins, so the
compile-time test seam and the lifecycle guards' isolated namespace behave
exactly as before.
Approach and variable name from #1576 by Leonardo trindade miranda, resolved one
layer lower so the activation path is covered too.
Refs #1574
Refs #1621
Co-Authored-By: Leonardo trindade miranda <tmonestudio@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Six reports describe the same dead end: the indexing worker dies, the
supervisor points at `logs/.worker-<pid>.log`, and the file is 0 bytes.
Nothing was ever flushed, so not one of #1070, #1130, #1132, #1133,
#1145 or #1450 is reproducible or attributable -- the hint says "crashed
on a file" and the file is never named.
Root cause is buffering, not the crash. The worker's stderr is redirected
to that log file by the supervisor, and every diagnostic goes through
`fprintf(stderr, ...)` with no flush. The C standard only promises stderr
is "not fully buffered"; the Windows CRT gives a redirected stderr FULL
buffering, which is why five of the six reports are Windows. A worker
that aborts, is SIGKILLed by the OOM killer, or is terminated after a
hang takes its whole buffer with it.
- cbm_log_set_crash_durable(): setvbuf(_IONBF) plus a per-line flush in
emit_line. Both, deliberately -- setvbuf covers every writer to the
stream including the plain fprintf startup errors that explain a
worker which never got as far as logging, and the flush covers the
case where setvbuf is refused because the stream was already written
to. Enabled for the worker role only, claimed in main() before the
process writes anything.
- cbm_index_worker_log_begin(): a startup header written and flushed as
the worker's first act -- version, build fingerprint, pid, repo path,
and the worker's own arguments. A control record rather than an info
line, so CBM_LOG_LEVEL cannot restore the 0-byte log.
- Under a crash-durable log the parallel extract pass logs every file
it starts, not just the first two rounds of workers, so the log ends
with the files that were in flight when the worker died. One line per
file, never per node; unchanged for every non-worker caller.
This fixes no crash. It converts six unreproducible reports into reports
we can act on, and every future one arrives with the run identified.
Test: a worker started with a fully buffered stderr -- the state the
Windows CRT hands it, forced on POSIX so the contract binds on all three
legs -- writes diagnostics and is then SIGKILLed. The retained log must
be non-empty, carry the header with the version, pid, repo path and args,
and still hold the line written after it. Reverting the production change
leaves that log at 0 bytes.
The probe kills rather than aborts because Darwin's abort() runs the
stdio cleanup handler and flushes the very buffer the repro depends on
stranding: under abort the reverted build still produced a populated log
and the test was quietly toothless. SIGKILL runs no cleanup, and is
literally #1070's death (signal=9).
Refs #1070
Refs #1130
Refs #1132
Refs #1133
Refs #1145
Refs #1450
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
`cli <tool>` resolved its JSON arguments from stdin whenever stdin was not a
terminal and no --args-file, raw-JSON positional or --flag form was supplied,
and cli_slurp_stream reads to EOF. An ordinary automation caller never sends
that EOF: Node's child_process.spawn defaults to stdio ['pipe','pipe','pipe']
and the parent must call child.stdin.end() explicitly, which almost nobody does
for a command it is not writing to. fd 0 then stays open with no writer and the
read never returns. The reporter's shell script was still parked in fread(0)
fourteen minutes later.
`list_projects` advertises "properties":{} — stdin could never have carried
anything it accepts, so the read was pure deadlock with nothing to gain. Gate
the stdin path on the tool's input_schema actually declaring properties, in a
seam (cbm_cli_args_from_stdin_allowed) that main.c's resolution chain calls.
An unknown tool also stops blocking: dispatch rejects it by name and no stdin
content can change that verdict. Tools that do declare properties keep the
documented `echo '<json>' | cli <tool>` channel untouched, and interactive runs
are unchanged because isatty(0) already short-circuited them.
Measured on the pre-fix binary, stdin held open by a writer that never writes,
15-second watchdog: `cli list_projects` was killed at 15s with 0 bytes of
output, while `cli list_projects < /dev/null` returned in 3s with 90 bytes.
After the fix the same open-pipe invocation returns in 3s with 90 bytes.
Regression tests (tests/test_cli.c): the reported hang, the piped-argument
channel that must survive, the interactive path, unknown/NULL tool names, and a
sweep asserting the gate tracks the advertised schema across the whole tool
table so a future zero-argument tool cannot reintroduce this silently.
Revert-check: with the schema gate reverted to the pre-fix `return
!stdin_is_tty`, both tests fail on the list_projects assertion and the suite
reports 2 failed; with the gate applied, 272 passed / 0 failed.
Known limit, not addressed here: a tool that DOES declare properties, invoked
with its required flag omitted (`cli index_status` without --project), still
blocks on an open pipe. Distinguishing "arguments are coming" from "nobody will
ever write" needs a bounded wait, which is a public-surface decision (deadline
value, and the cross-platform pipe-readiness probe Windows would need) rather
than a mechanical fix.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
OpenCode writes "enabled": true beside the "command" and "type" we write. Our
ownership check required the entry's key set to match EXACTLY
(config_json_like.c: member_count != found_count), so a three-key entry with two
recognised keys was classified as FOREIGN - and we refused to touch an entry we
had written ourselves. install then failed with:
error: agent_config agent=OpenCode op=mcp_install path=.../opencode.json
Confirmed on two independent configs: Linux (#1630) and Windows (#1582). In
gotspatel's file EVERY MCP server carries the key - mssql, forgetful,
chrome-devtools and ours - so this is OpenCode's normal shape, not an unusual
hand-edit. Anyone who has ever toggled a server on or off in the UI was hit.
Two of my own hypotheses were wrong before the reporters' files settled it: it is
not JSONC comment parsing, and it is not the .jsonc targeting that #1575 fixed.
#1575 fixed WHICH file we open; this happens after, on what we find inside.
The distinction now reported is MATCH_WITH_EXTRAS, and the caller treats it as
ALREADY SATISFIED - success, without touching the file. That is deliberate and it
is the safe half of the fix: cbm_json_like_upsert_entry REPLACES an entry
wholesale, so writing our canonical shape over an annotated entry would silently
delete the client's keys. A refusal the user can see beats a deletion they
cannot. Doing nothing is also correct on the merits: the entry already names this
binary with the right type, which is the entire content of the install.
Merging our fields into an annotated entry while preserving the rest is the
fuller fix and stays tracked in #1630. This makes the common case work without
risking anyone's configuration tonight.
Ownership is NOT loosened otherwise: an entry whose command points at a different
binary is still foreign and still refused, byte-identically, and that direction
is pinned by its own test.
Tests use the reporters' actual entry shape. cli suite: 272 passed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
yaml_range_has_unsupported rejected `&` and `*` anywhere in a plain scalar, with
no positional test at all. So a value containing prose asterisks - `use
*emphasis* here`, a kaomoji, a glob inside a description - was refused as if it
were an alias.
That is one of four constructs that made a real 16 KB hand-maintained Hermes
config permanently un-editable by us (#1631). It is not YAML we cannot model; it
is YAML we declined to read.
An indicator only counts where a NODE begins. The test is now positional and
judged by what PRECEDES the character, because the scanned range covers a whole
line including its key: in `command: &shared` the `&` is an anchor even though
`command:` came first. A node begins at the range start, after a mapping colon,
or after a block-sequence dash. Everything else is text.
`{` and `}` keep their existing treatment. Empty flow mappings (`key: {}`) are
the next item in #1631 and carry their own semantics; bundling them here would
mix a positional correction with a value-semantics change.
Root-caused by @rg6304, who reproduced it in isolation, read this source, and
corrected the maintainer's hypothesis - the constructs I had guessed (inline
comments, anchors, `---` separators) appear nowhere in the failing file.
TESTING NOTE, recorded because it nearly went wrong: the first version of the
regression test put the asterisk in an untouched foreign section and passed with
the fix REVERTED - the editor only validates the range it writes, so the test
proved nothing. Rewritten onto the entry block, it now fails without the fix
(`== -1, expected 0`) and passes with it. The companion test pins the other
direction: a LEADING `*` is still an alias and still refused, byte-identically.
config_yaml_edit + yaml suites: 117 passed, 0 failed.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
CI lint flagged two continuation lines from the previous commit. Formatting
collapsed the runtime-directory call onto one line, so the contract pin that
asserted ancestor=false there stopped matching; the pin now keys on the
formatted text. Both revert directions re-verified: capability-only fails, and
relaxing the runtime directory to ancestor=true fails.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Six independent machines in #1533 and #1574 cannot run cbm at all, and none of
them is exotic: a domain-joined UAC-filtered admin, a secondary volume, orphaned
ACEs from an uninstalled application, and an AppContainer package SID belonging
to a shipping desktop application.
The ancestor walk was identity-blind to all of it. win_directory_component_secure
demanded that no ancestor DACL entry grant any private-mutation bit to any SID
outside the trusted set, so a single ACE anywhere up %LOCALAPPDATA% refused the
endpoint before logging started. Every mode failed, config list included, so the
product could not even be reconfigured out of it, and CBM_CACHE_DIR does not help
because the runtime directory is %LOCALAPPDATA%\cbm-daemon-<hash>, whose ancestor
chain relocating the cache never touches.
Ancestor components now also tolerate AppContainer identities: package SIDs
(S-1-15-2-*) and capability SIDs (S-1-15-3-*), under identifier authority 15.
The boundary is ancestor-only. The private runtime directory is validated with
ancestor=false and keeps demanding the exact current user with a protected DACL;
the flag is threaded explicitly through win_file_security_secure and
win_file_acl_secure rather than inferred, so the strict path cannot acquire the
tolerance by accident.
Why these identities are admissible on an ancestor: a process cannot choose which
AppContainer it runs in. The identity is stamped by the OS at process creation
from the package it was launched from, so such an ACE cannot be exercised by
arbitrary local code the way a live local group can. What it does permit is the
packaged application itself — that is the residual risk this accepts, and it is
the same trust already extended to whoever installed that package.
BOTH forms are covered deliberately. The most common real ACE of this shape is
S-1-15-2-*, a package SID; on reported machines it resolves through the registry
AppContainer mappings to Anthropic Claude Desktop, which many of our users run and
cannot be asked to uninstall. Covering only capability SIDs leaves exactly that
case failing.
This narrows a deliberate policy: the strict gate was chosen on purpose and a
middle ground was previously declined. It is reopened here by explicit maintainer
decision, and narrowed as far as the evidence allows rather than relaxed wholesale
— a live local group, Authenticated Users on a secondary volume, and orphaned
unresolvable SIDs all still refuse. Those need CBM_RUNTIME_DIR or a separate
change; orphan tolerance in particular needs LookupAccountSid bound first and has
an offline-domain-controller caveat, so it is not bundled in here.
Approach and the ancestor-only boundary follow @mlandolfi90's PR #1447, extended
from capability SIDs to package SIDs.
Co-Authored-By: mlandolfi90 <mlandolfi90@users.noreply.github.com>
Contract-pinned in tests/test_windows_bundle_contract.sh and revert-checked in
both directions: reducing it to capability-only fails, and passing ancestor=true
for the runtime directory fails.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Release notes now carry one row per product — the bytes actually published, with
their VirusTotal verdict — instead of a column per candidate.
Several candidates are scanned so the selector has an alternative when an opaque
classifier flags one, but a reader installing cbm cares about what they receive,
not about what we discarded. The rejected candidates' verdicts remain in the
published evidence TSVs for anyone auditing the selection, and they stay useful
to US as a development signal; in a changelog they are noise, and listing a
"microsoft-ml" verdict next to a binary nobody can download invites exactly the
wrong conclusion.
Also removes a claim that had become FALSE: the section asserted that every scan
had a minimum of 50 decisive engines. That floor was removed deliberately (it is
VirusTotal fleet availability, not a property of our binary) and the last release
observed 31. Published notes would have stated something untrue. The observed
range is still reported, now over the shipped binaries.
The disclosure of a tolerated Microsoft `!ml` is preserved and narrowed to the
binaries that ship. The contract test now pins both directions: a flagged binary
that SHIPS must be disclosed, and flagged candidates that were rejected must not
appear at all. Its fixture gained a target that draws the tolerated verdict on
all three candidates, so the disclosure branch is actually exercised — previously
every flagged candidate was rejected in favour of a clean sibling, leaving that
path untested.
README and SECURITY.md updated: they promised notes "link every candidate result".
All five release/VT contract tests pass.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Four places ask "is this binary instrumented?" and each asked it differently.
One of them, the C# LSP bench, only recognised ASan, so TSan and MSan measured
an instrumented parse against the NATIVE 200ms budget. The other three carried
a hand-copied list of `__SANITIZE_*__` macros that nobody kept in sync.
That drift is what the TSan gap was made of. CFLAGS_TSAN never passed
SANITIZED_DEFINE, and the per-site conditions could not cover for it: they test
`__SANITIZE_THREAD__`, which is GCC's spelling. Clang — the compiler that leg
uses — announces thread instrumentation through `__has_feature(thread_sanitizer)`
only, and no site consulted it. The claim in 0a163d4f that compiler probes could
not have helped is true of the probes we had, not of the one clang actually
offers.
src/foundation/sanitized.h now answers the question once, as CBM_SANITIZED,
from two sources with distinct jobs:
- CBM_SANITIZED_BUILD from the build system stays the source of truth, and is
the ONLY thing that can answer for UBSan and trap-UBSan: undefined-behaviour
instrumentation leaves no macro and no __has_feature bit to probe.
- The clang and GCC probes are the backstop for the three sanitizers that do
announce themselves, so a lane that forgets the define still gets correct
budgets instead of native ones on an instrumented binary.
Deliberately no #error when a probe fires without the define: promoting leaves
the binary correct while the lane gets fixed, and it does not break an
out-of-tree `make CFLAGS_EXTRA=-fsanitize=address` that never went near
Makefile.cbm.
__has_feature is defined away where it does not exist rather than guarded with
`#elif defined(__has_feature)`. The guarded form compiles everywhere but fails
cppcheck, which walks every configuration and rejects the file with "failed to
evaluate #if condition, undefined function-like macro invocation". The
define-away idiom is what clang documents and what tests/test_mem.c already
uses; `defined(...) && __has_feature(...)` is not an option at all, since && does
not spare a preprocessor without the builtin from parsing `0 (0)`.
Verified the resolution rather than assuming it (clang 22, -dM -E):
native CBM_SANITIZED 0
-DCBM_SANITIZED_BUILD=1 CBM_SANITIZED 1
-fsanitize=address CBM_SANITIZED 1
-fsanitize=thread (linux target) CBM_SANITIZED 1
-fsanitize=memory (linux target) CBM_SANITIZED 1
-fsanitize=undefined CBM_SANITIZED 0 <- define-only, as designed
The third row is the one that matters: the TSan failure this header is named
after would have self-healed.
Also wired the define into the instrumented flag sets of our own code that
still lacked it — CXXFLAGS_TSAN (preprocessor.cpp is ours, and CXXFLAGS_TEST
already had it), GRAMMAR_CFLAGS_TEST and GRAMMAR_CFLAGS_TSAN. Neither tree can
include the header today (no -Isrc), so this is the build system keeping its
own promise rather than a behaviour change. Vendored flag sets are untouched:
mimalloc, sqlite3, tre, zstd, lz4 and tree-sitter read no macro of ours.
Behaviour change worth naming: test_cs_lsp_bench now allows 2000ms on the TSan
and MSan lanes instead of 200ms. It loosens a bound that was being applied to
an instrumented binary by accident; it never tightens one.
Not verified locally: this machine has no POSIX-target compiler, so the POSIX
half of subprocess.c was not compiled here. The full Windows test-runner builds
clean with -Werror and the subprocess suite is green (14 passed, 17 skipped);
clang-format clean; macro matrix as above.
Signed-off-by: Mauricio Offermann <mauricio.offermann@gocode.cl>
The v0.10.4 release (run 31755436916) failed at the very last step of `verify`,
after all 69 other jobs had passed — full test matrix, all eight builds, smoke
and soak:
publish-vt-evidence: wrong evidence marker in binaries/vt-results.tsv
The results format was bumped to `cbm-virustotal-results-v2` in #1596, but
publish-vt-evidence.sh still demanded `-v1`. It was the only straggler: the
gate, the selector, the notes renderer and all three contract fixtures were
already on v2.
It survived because that PR had removed the script's only caller, leaving it
dead code that nothing exercised. Restoring the full-surface scan restored the
caller too, and the stale expectation surfaced in the worst possible place — at
the end of a real release rather than in a dry-run, since `verify` does not run
in dry-runs.
Also adds the guard that would have caught it: every marker publish-vt-evidence.sh
validates must be one the gate actually writes. Revert-checked — reintroducing
v1 fails with "publisher expects marker never written by the gate:
cbm-virustotal-results-v1".
The scan itself passed. This is purely the evidence-publishing step.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Two things, both found by the dry-run and by sweeping what it exposed.
1. The three-candidate change was incomplete. The dry-run failed at
stage-release-candidates.py, which carries its OWN copy of the transform
validation that the previous commit only fixed in the selector:
stage-release-candidates: candidate linux-amd64/debug-stripped has
invalid transform: 'strip-debug'
Sweeping for that assumption found it in six places, not two:
stage-release-candidates.py, select-release-candidates.py,
verify-release-selection.py, append-vt-notes.sh and two contract-test
fixtures — as hardcoded 16s, `len(TARGETS) * 2`, two-entry VARIANTS tuples
and two-key truth tables. All are now derived from len(VARIANTS).
Field prefixes needed care: the variant NAME keeps its hyphen because it is
the on-disk directory, while the evidence columns use underscores, so
`debug-stripped` reads `debug_stripped_sha256`. Every lookup now goes through
an explicit FIELD_KEY map instead of interpolating the variant directly.
The selection contract test now covers the truth table EXHAUSTIVELY: three
variants x two tolerated classifications is exactly eight combinations, and
there are exactly eight targets, so every case is exercised once.
2. Full-surface VirusTotal scanning is restored. This PR had moved scanning
upstream to the candidates and deleted the post-package pass, which silently
narrowed coverage from everything we ship to executables only. The 42 runtime
files across the 14 containers — install.sh, install.ps1, LICENSE,
THIRD_PARTY_NOTICES.md, the MCPB manifest.json and the unpacked UI assets —
were still extracted, structurally verified and strings-audited, but no
longer scanned at all. install.sh and install.ps1 are the highest-consequence
non-executable bytes we publish; users pipe them straight into a shell.
The verify job scans every extracted object again, under the same policy.
Re-submitting the selected executables alongside them is close to free
because VirusTotal is content-addressed and answers for identical bytes from
its own record — the same property that made the analysis-id equality check
untenable two commits ago.
The gate-chain contract asserted the opposite ("duplicate post-package
VirusTotal path remains"). That assertion is inverted: the pass is required,
and it is not a duplicate, since it covers a strictly larger set. README and
SECURITY.md updated from "archive containers are checksummed rather than
redundantly rescanned" to state the full covered surface.
All five release/VT contract tests pass.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Release evidence from run 31744302624 shows the tolerated Microsoft `!ml`
verdict is close to a coin flip per byte image rather than a property of the
code. Across the eight targets the stripped and unstripped candidates of the
SAME linker output disagreed on four, and in both directions:
linux-amd64 stripped microsoft-ml unstripped clean
darwin-arm64 stripped clean unstripped microsoft-ml
linux-arm64 stripped clean unstripped microsoft-ml
If the classifier were keying on something intrinsic to our code the siblings
would agree; they do not. So each variant is close to an independent draw, and
5 of 16 candidates drew the flag.
Two draws is not always enough. On that run linux-amd64-portable came back
microsoft-ml on BOTH candidates, leaving no clean binary to ship for that
target. A third independent draw at a ~31% observed per-candidate hit rate takes
the both/all-flagged case from roughly 1-in-10 per target to roughly 1-in-30.
The third candidate is `--strip-debug` (Apple: `-S`): debug information removed,
symbol table kept. Behaviourally identical to the other two — same linker
output, only metadata differs — but a distinct byte image, which is all
VirusTotal needs to scan it as its own file. Verified on the real v0.10.4
candidates: linux-amd64 gives three distinct hashes (294,634,656 /
294,623,208 / 293,746,096 bytes) and darwin-arm64 likewise, with the ad-hoc
signature verifying after strip.
Selection is unchanged in spirit and now ordered: smallest artifact first
(stripped, debug-stripped, unstripped), take the first CLEAN one, and only if
every candidate drew the tolerated verdict ship the smallest flagged one. A
hard verdict on any candidate still blocks the release before selection.
Cost is 24 objects per release instead of 16.
Derivation enforces that all three hashes differ — identical candidates would be
one draw wearing three hats, and the selector would believe it had alternatives
it does not have. Public claims in README, SECURITY.md and docs/index.html
updated from "both stripped and unstripped" to the three candidates.
All three release contract tests pass, including the native derivation test
which exercises the real strip and codesign path on this host.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The build+VT dry-run (run 31744302624) blocked all eight targets on:
BLOCKED: objects/3eb486a4... completed with only 48/75 decisive engines (< 50)
That object is the windows-arm64 STRIPPED candidate with ZERO detections. It
was classified "hard" — the same bucket as a real malware verdict — purely
because 48 engines answered instead of 50. Its own sibling scanned clean with
66.
How many engines return a decisive result is VirusTotal fleet availability on
the day, not a property of our binary. These artifacts are ~300 MB and many
engines skip or time out at that size, so the count sits near the floor and
moves run to run: on this run one object landed at 48 and the other fifteen
spread 59-68. A floor there makes shipping a lottery decided by someone else's
infrastructure, which is exactly what our CI-determinism rule forbids, and it is
not something we can influence by fixing anything on our side.
Classification now depends only on what engines FOUND:
hard - two or more engines, any non-Microsoft engine, any label that is
not `!ml`, or anything suspicious. Still blocks the release.
microsoft-ml- exactly one Microsoft `!ml`. Tolerated and disclosed.
clean - nothing flagged.
The decisive-engine count is still recorded per candidate in the results
manifest and printed as a NOTE when it is below the reference value, so the
evidence is unchanged; only its power to block is gone. The selector keeps
rejecting an incoherent response (total < completed).
Public claims updated to match in README.md, SECURITY.md and docs/index.html —
all three promised "at least 50 decisive engines" as a publication requirement,
which would no longer have been true.
Contract test updated: a clean verdict now stands regardless of engine count,
and the below-reference NOTE must still be reported. All three release contract
tests pass (vt-gate-policy, release-gate-chain, candidate-derivation).
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The dry-run for this branch (run 31729540899) failed in `select-package`, the
new job, after every build had completed and before smoke — the exact shape of
failure that would have taken a release down:
BLOCKED: VirusTotal response analysis id does not match the submitted
analysis: objects/70fbbc37...
That object is the linux-arm64 UNSTRIPPED candidate, 295,476,856 bytes.
VirusTotal is content-addressed, and for bytes it already holds it may answer
with its own canonical analysis rather than the one this upload created. A large
release binary is exactly the artifact most likely to be already on file, so
requiring the ids to be equal would have recurred on most releases rather than
being a one-off.
The equality check was guarding the right idea at the wrong level. What has to
hold is that a verdict describes THESE bytes, and parse_completed already
enforces that a few lines below against meta.file_info.sha256 and
file_info.size — a strictly stronger binding than the id. It is also what keeps
a tuple's two candidates apart: stripped and unstripped differ in hash by
construction (70fbbc37... vs 28b3890f... on linux-arm64), so neither can be read
as the other whatever ids VirusTotal hands out. The wrong-hash and wrong-size
contract cases pin that and still block.
The property deliberately given up is freshness: an older cached analysis of
identical bytes is now accepted. For identical bytes that is the same content
risk, and it buys a gate that is deterministic instead of one that fails
whenever VirusTotal recognises our own binary. The submitted id is still
recorded in the results manifest as evidence.
The selection policy is unchanged and needs no change: stripped clean ships
stripped; stripped microsoft-ml with a clean unstripped ships the clean one;
both microsoft-ml ships stripped; a hard verdict on either sibling still blocks
the release for analysis.
Contract test updated to assert the passing direction, and revert-checked: with
the fix reverted it fails with the production message, with it applied it passes.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The CLI resolves each client's config home from its own environment
variable before falling back to $HOME — cbm_codex_config_dir() honours
$CODEX_HOME and ignores the home_dir it is handed. A test that redirects
only HOME therefore does not isolate those clients: it resolves, reads
and WRITES the developer's real configuration.
On a machine with $CODEX_HOME exported (any Codex or Orca user) 27 of
269 cli tests failed for that reason alone. Every one of them asserted a
successful rc while the CLI was reporting genuine failures against the
ambient config: an unbalanced managed marker left in the real
config.toml, an AGENTS.md that is a symlink and is refused by the
O_NOFOLLOW writer, and user-modified agent profiles that are correctly
preserved. The behaviour under test passed in all of them; only the rc
was poisoned. Worse than the red: the suite mutated that real config,
installing and removing SKILL.md under the user's Codex home.
The individual tests that already neutralize CODEX_HOME are exactly the
ones that pass, and the file carries a comment admitting one suite was
"green only via env leaked from earlier suites". Rather than extend those
ad-hoc per-test lists again, neutralize the whole inventory once in the
runner, and reuse the inventory the shell fixtures are already required
to satisfy.
tests/test_smoke_fixture_contract.sh grows one check so the two cannot
drift: a client isolated in the smoke scripts but forgotten in the C
runner now fails the contract.
Verified on macOS arm64 with $CODEX_HOME and $OPENCODE_CONFIG_DIR
exported: cli went from 241 passed / 28 failed to 268 passed / 0 failed,
with no change outside tests/.
Signed-off-by: Gabriel Gonzalez Pineda <64bits@gmail.com>
A duplicated install left a Codex config.toml carrying a CLOSING
`# <<< codebase-memory-mcp SessionStart <<<` with no opener. Every later
install then failed that client with op=legacy_hook_cleanup and aborted the
whole activation, so one bad file made the installer permanently unusable for
that client. Reported by @PsyTech-Rob.
We are the only writer of these markers, which makes the imbalance our own
residue. Refusing to touch a file we cannot parse is the right default in
general — it is the wrong default for a mess we made.
Removal now strips the stray line. The bounds come from the marker scan that
already ran, so nothing is guessed and no surrounding content is touched; the
user's own keys either side survive verbatim.
A WRITE still refuses on an imbalance, deliberately: with only one marker there
is no defensible region to replace, and guessing could destroy user content.
That asymmetry is the whole design — heal where the answer is unambiguous,
refuse where it is not.
Both orientations are tested (orphan opener, orphan closer) and revert-checked:
without the fix each returns the same -1 that aborted the reporter's install.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Martin Vogel