Reworks the ADR-loss-across-reindex fix to Alternative 1 (per the PR review):
remove the race rather than narrow it. ADRs live in a per-project sidecar SQLite
DB ("<graph_db>.adr.db"), separate from the graph DB a reindex rebuilds or
deletes, so there is no capture, re-read, re-apply, or lock — and no sleeps,
polling, or timeout deciding an outcome.
store.c:
- ADR writes (store/delete/update_sections) use a cached read-write handle opened
SQLITE_OPEN_READWRITE|CREATE and configured like the graph store (PRAGMA
busy_timeout=10000 + journal_mode=WAL) so cross-process access waits rather
than failing SQLITE_BUSY. A :memory: store keeps ADRs in the graph DB.
- Reads never write: cbm_store_adr_get uses the cached RW handle if open, else
opens the sidecar SQLITE_OPEN_READONLY (with the same busy timeout) when the
file exists, else reads the legacy graph row without creating anything. When
the sidecar exists but is NOT marked migrated (half-created), it falls back to
the legacy graph row so a partial sidecar never hides an ADR.
- One-time migration is atomic: adr_copy_legacy_once wraps the marker check,
legacy copy, and marker insert in a single BEGIN IMMEDIATE...COMMIT
(re-checking the marker under the lock; ROLLBACK->ERR on failure). New public
cbm_store_adr_migrate_once. Sidecar path via cbm_path_for_file_api (Windows
long-path). Dropped the write-only adr_db_path field.
pipeline.c: migrate a legacy ADR into the sidecar BEFORE the format-change and
forced-full routes unlink the old generation; preserve-and-abort on failure, and
do NOT fail open — a query-open returning NULL preserves unless the final DB
genuinely does not exist. Migration targets the FINAL path (staging suffix
stripped). The early-capture/re-apply machinery and its test hook are gone.
artifact.c: the exported artifact carries the ADR — snapshot_inject_adr always
DELETEs project_summaries first, then injects only when an ADR is present, so a
deleted ADR never resurrects through a lingering legacy row. Import restores the
ADR into the destination sidecar via cbm_store_adr_restore_from_graph_if_absent:
it copies the artifact's ADR (INSERT OR IGNORE — never clobbers a local one)
regardless of the migrated marker, so a teammate who already indexed the project
(marked sidecar, no ADR) still receives the shared decision record. store.c
factors the row copy into adr_copy_rows, shared by the one-time legacy migration
and the ungated restore.
mcp.c: delete_project removes "<db>.adr.db" (+ -wal/-shm/-journal) only after the
graph unlink succeeds; is_project_db_file skips *.adr.db. cli.c: exclude
*.adr.db from index listing/counting. README + memory-core baselines updated.
Tests (file-based): ADR written mid-reindex survives on BOTH the full and delta
routes (route-pinned FORCED_FULL / CLOSURE_REPAIR); a legacy row migrates before
a forced-full/format reindex with no prior read; migration failure (a directory
at <db>.adr.db) preserves-and-aborts with the old generation intact;
delete_project via the MCP handler removes the sidecar + journals; the artifact
export->import round trip keeps the ADR; a deleted ADR does not resurrect on
import; an import does not clobber a local ADR; an import into an already-indexed
destination (marked sidecar, no ADR) restores the artifact's ADR; a half-created
sidecar falls back to the legacy row. The import tests remove the destination
graph DB (keeping the sidecar + marker) before importing, matching production
(try_artifact_bootstrap imports only when the graph DB is missing) so the rename
succeeds on Windows, not only on POSIX.
Closes#516
Signed-off-by: 0xb007ab1e <134006168+0xb007ab1e@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the #1171 fix. Exporting an artifact from a repository under
a CJK path on Windows wrote graph.db.zst and artifact.json, but the
.codebase-memory/.gitattributes file carrying the merge=ours protection
was silently missing.
Root cause: ensure_gitattributes() created the file with the raw POSIX
open(O_WRONLY | O_CREAT | O_EXCL). On Windows that is the ANSI CRT, so a
repo path with characters outside the active code page fails with ENOENT.
The failure was only logged, and the warning itself was malformed: it
passed printf-style arguments to the key/value logger, which printed the
literal "msg=artifact.gitattributes.open_path=%s_err=%s <path>=<error>".
Fix: create the file with cbm_fopen(path, "wbx") (_wfopen on Windows).
The "x" mode keeps the O_CREAT | O_EXCL create-only-if-absent semantics,
so an existing (possibly user-edited) .gitattributes is never rewritten.
Binary mode writes the same LF bytes on every platform. The warning now
uses the logger's key/value form (path=..., err=...).
Test: artifact::artifact_roundtrip_non_ascii_paths now reads
.gitattributes back from the CJK repo through cbm_fopen and asserts the
merge=ours line. It also replaces the file with user content, exports
again, and asserts the content is unchanged. On the Windows VM the
assertion failed 3 of 3 without the fix and passes 3 of 3 with it. The
no-overwrite check fails if the mode loses its "x".
Refs #1171
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
With persistence=true, indexing a repository under a CJK path on Windows
made the worker exit nonzero and no .codebase-memory/graph.db.zst was
written, while an ASCII junction to the same directory worked.
Root cause: write_file_atomic() -- used for artifact.json, graph.db.zst
and the import temp db -- opened its temp file with the raw CRT fopen()
and published it with MoveFileExA(). Both go through the ANSI code page,
so a repo path containing characters outside it cannot be opened. The
same file had two more ANSI calls on the same flow: cbm_artifact_exists()
used stat() on the repo path, and the import published the cache db with
CRT rename(), which is ANSI and also fails when the destination exists.
Fix: use the repo's UTF-8 helpers -- cbm_fopen() for the temp file,
cbm_rename_replace() (MoveFileExW, write-through, replace-existing) for
both publishes, and a cbm_fopen() probe for the non-empty existence
check. rename_temp now reports errno on every platform (cbm_rename_replace
translates the Win32 error).
Test: artifact::artifact_roundtrip_non_ascii_paths exports from a CJK
repo directory, checks cbm_artifact_exists(), and imports twice into a
CJK cache directory (the second import replaces the existing db).
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Review of the previous head found the global rule unsound for one caller:
cbm_mkdir_p also creates <repo_path>/.codebase-memory inside the checkout
being indexed, and git creates symlinks owned by whoever cloned, so a
repository shipping `.codebase-memory -> ~/.ssh` would have been followed
by the artifact writer. "User-owned" says nothing about "user-intended"
once the path root is a checkout.
cbm_mkdir_p keeps the strict walk (root-owned links only). The new
cbm_mkdir_p_ex(path, mode, policy) with CBM_MKDIR_FOLLOW_OWNED also
follows a symlink owned by the invoking user, and only callers whose
paths are rooted in the user's own configuration opt in: the installer's
agent roots, skills, instruction files and hook directories, the
JSON-like config editor's parent creation, the cache root, <cache>/logs,
the UI config directory and the DB parent. The artifact directory inside
the checkout and the ~/.local/bin creations stay strict; the activation
transaction re-walks the latter with its own no-follow policy anyway.
What a follow lands on is now checked too: the opened target must be a
directory owned by root or the invoking user and not world-writable
unless sticky, so a link swapped between the fstatat and the openat
cannot steer the walk into a directory an attacker controls or one where
anyone can pre-plant entries. This applies to root-owned links as well;
macOS /private/tmp is root-owned and sticky and still passes. Refusal
restores the first openat's errno.
Tests: the plain walk refuses an own link that the opted-in walk
follows; targets that are world-writable without the sticky bit are
refused; as root, a foreign-owned target and a user-owned link are both
refused, each followed by restoring ownership and walking again; the
identical `.codebase-memory -> <user-owned dir>` link is refused by the
plain walk and followed once opted in; cbm_artifact_export refuses a
checked-in artifact-dir link and leaves its target empty; the installer
test gains the euid-0 leg with home, link and target owned by a non-root
uid. Under euid 0 the tests demote their links to a non-root owner, since
root-owned links are trusted by both walks.
Refs #1722
Signed-off-by: TheStranjer <791672+TheStranjer@users.noreply.github.com>
The reconcile_basis marker was never written on Windows, so all six
build_trusted_artifact_repo() tests plus artifact_export_marks_clean_basis
failed there (16 passed, 7 failed) while every Linux and macOS leg was green.
Root cause is a stamp-encoding mismatch, not shell quoting. file_hashes rows
are stamped by the semantic manifest via cbm_path_info_utf8(), which on Windows
derives mtime_ns from ftLastWriteTime -- 100-nanosecond FILETIME ticks.
db_hashes_match_disk() re-stat()ed those same files through _wstat64, whose
st_mtime carries WHOLE SECONDS, and compared the two. The values can only agree
when a file's write time lands exactly on a second boundary, so the trust check
rejected the very first row on every export, on Windows and only on Windows.
On POSIX both paths read st_mtim nanoseconds and agree, which is why the split
was clean along platform lines.
The disk side now reads through cbm_path_info_utf8 -- the same function that
wrote the rows -- so there is no encoding left to keep in sync. Its
is_regular/is_symlink guard preserves exactly the symlink refusal
reconcile_stat_no_symlink provided.
reconcile_restamp_rows deliberately keeps art_stat_mtime_ns: a restamped row is
read by the incremental classifier and by check_index_coverage, both of which
stat() and encode whole seconds on Windows. The asymmetry is now pinned with a
comment at both sites so neither is "unified" into breaking the other.
Diagnostics, which is why this took two wrong diagnoses to find:
- Export's four clean-basis preconditions were one short-circuiting && chain
collapsing to a single bool. They are now evaluated by reconcile_basis_trusted,
which records the first failing gate (head_unresolved / head_not_hex_oid /
tree_not_clean / db_hashes_differ_from_disk, the last carrying the offending
path and BOTH stamps) and logs it as artifact.reconcile_basis_omitted. The
reason is recorded rather than recomputed on demand: export's own
ensure_gitattributes leaves an untracked .gitattributes behind, so any later
evaluation answers tree_not_clean whatever the real cause was.
- build_trusted_artifact_repo() had six `return NULL` exits and reported none of
them; CI could not tell a failed `git commit` from a missing marker. Every exit
now names its step and carries the number that decided it -- git exit status,
pipeline rc, or export's recorded blocker -- surfaced through
BUILD_TRUSTED_REPO_OR_FAIL at the call site so file:line still points at the
failing test.
Verified locally on macOS by emulating the Windows encoding: truncating the
disk-side mtime to whole seconds reproduces the CI signature exactly (16 passed,
7 failed, same seven sites), and the new diagnostic names it as
`db_hashes_differ_from_disk path=a.rs reason=mtime_differs stored_mtime_ns=...
disk_mtime_ns=...`. Restored, the artifact/pipeline/incremental/mcp/index_format/
daemon_bootstrap suites are green and make lint-ci passes.
Co-authored-by: moofone <35233167+moofone@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
clone_to_b() cloned the source repo but never configured user.email or
user.name on the result. A clone does not inherit the source repo's LOCAL
config, and CI runners have no global identity, so every commit made in the
clone exited 128 -- surfacing as:
FAIL tests/test_artifact.c:734:
runf("git -C ... add -A && git -C ... commit -qm edits") == 32768, expected 0
32768 is 128 << 8. It passed locally only because developer machines carry a
global git identity; the Linux CI shards do not.
git_init() already configures identity for the source repo (:574-575); the
clone needs the same treatment. This is the defect originally diagnosed on
#868 as "git identity in clone_to_b" -- correct for the POSIX legs, and
separate from the cmd.exe quoting bug that caused the Windows failure.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
An imported artifact carries the EXPORTER's mtime/size in every file_hashes
row, so the first incremental run after a bootstrap sees every file as
changed and re-parses ~the whole repository — most of the time the artifact
was supposed to save.
Reconcile the rows instead. After a successful import, ask the local git
which files are unchanged between the artifact's commit and the working
tree, and re-stamp exactly those rows with local stat() values. Everything
git cannot vouch for stays foreign and is re-parsed.
Export writes an optional "reconcile_basis":"git-clean-head" marker into the
existing artifact.json, and only when the tree is provably clean outside
.codebase-memory/ AND every hash row still matches disk. Unknown fields are
ignored by older binaries, so no schema_version bump.
Eligibility is deliberately narrow, and the polarity is skip-on-doubt:
a row is restamped only if it is tracked at the artifact commit (git
diff/ls-files are blind to ignored files — a .cbmignore negation can index
a gitignored file, #500) and absent from the changed set. Any uncertainty —
no git, missing/non-hex commit, shallow clone, truncated -z output,
allocation failure — skips reconciliation entirely and falls back to
today's behavior. Reconciliation can never fail the import.
Windows correctness: every git argument uses DOUBLE quotes, per the rule
already stated in artifact.c. cbm_popen runs cmd.exe /c on Windows, which
does not honor single quotes, so a single-quoted ":(exclude)" pathspec made
the clean-tree probe report dirty and the marker was never written there.
The commit-existence probe uses `cat-file -t <oid>` rather than
`cat-file -e <oid>^{commit}` because ^ is cmd.exe's escape character.
Fail closed on allocation failure: both cbm_ht_create() results are checked,
and every insertion is verified to have landed (cbm_ht_set discards the
underlying insert's status, so OOM drops entries silently). A dropped entry
in the changed set would make a genuinely modified file read as unchanged
and be restamped — a silently stale graph. This is the one place where the
membership-set primitive has unsafe polarity; classify_files uses it where a
dropped entry only means "re-parse".
Symlinks are refused rather than followed when stamping: git tracks a
symlink's link text, not its target's content, so "unchanged" from git says
nothing about the bytes the indexer parses. Uses the established
lstat + S_ISLNK / reparse-point idiom (discover.c, pass_pkgmap.c,
pass_envscan.c), which on Windows also keeps non-ASCII paths off the ANSI
CRT.
The trust trade-off is documented at both the import site and in
artifact.h: foreign mtimes currently force a re-parse that auto-scrubs a
poisoned artifact at an unpredictable clone time, so today's exposure is
transient and self-healing. Restamping makes it durable and gates it on a
producer-written string — acceptable only because the marker alone never
suffices, as every restamped row must additionally be proven unchanged by
the local git against a commit present in this clone.
Also skip synthetic file_hashes rows. The pipeline stores semantic-input
digests under .codebase-memory/.semantic-input/ that have no file on disk;
stat()ing them made the export-side check fail unconditionally, so the
marker was never written on ANY platform and the whole feature was inert.
find_deleted_files skips the same class. Both halves of the trust check now
ignore the .codebase-memory/ subtree, matching the clean-tree pathspec.
Distilled from #868 by moofone; #869 and #870 are not included.
Co-authored-by: moofone <35233167+moofone@users.noreply.github.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The FAST export path (the watcher/incremental auto-update) read the
raw main-file bytes of a live WAL-mode store: committed rows still in
the -wal were missing entirely and mid-checkpoint reads produced torn
snapshots (field artifact: four short indexes + btreeInitPage error 11
across ~12 nodes-table pages). Import then installed the torn file
byte-for-byte — the integrity check only sanity-checked the projects
table.
- Export: both quality levels now snapshot via VACUUM INTO (the
consistent copy); only the index-stripping remains BEST-only, FAST
keeps zstd-3. The fast path pays a full-copy VACUUM INTO now —
correctness over speed; a torn team-shared artifact costs far more.
- Import: new cbm_store_check_integrity_deep (shallow check + PRAGMA
quick_check) — used only on this rare path, hot-path callers keep
the cheap shallow check.
Reproduce-first: artifact_fast_export_snapshots_live_wal_store
exports FAST while the writer connection is open — RED on the old
code with 0 of 60 nodes surviving the round-trip (all rows lived in
the WAL); GREEN now. store_deep_integrity_detects_page_corruption
pins the import guard both directions (healthy passes, zero-banded
pages refused).
Closes#895
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The import path allocated the destination from the plaintext original_size
field in artifact.json and passed that value to the decoder truncated
through int. A large real database already wraps that cast (a full-kernel
index is ~14 GB), and a doctored field could additionally desync the decoder
capacity from the actual buffer. Read the decompressed size from the zstd
frame's own content-size header, require the metadata field to match it, cap
the total, and run the decoder with a size_t capacity equal to the
allocation. cbm_zstd_decompress now takes size_t lengths and returns int64_t.
Adds a regression test that a mismatched original_size is rejected.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
gitattributes apply left to right and the binary macro expands to
-diff -merge -text, so the generated line
graph.db.zst merge=ours binary
leaves the merge attribute UNSET (git check-attr merge reports
"unset"), and concurrent artifact refreshes produce a binary merge
conflict instead of auto-resolving -- the exact failure this file is
generated to prevent. Reordering to
graph.db.zst binary merge=ours
makes check-attr report merge: ours while keeping -diff/-text intact.
Verified on a real repository: before the reorder git check-attr merge
-- .codebase-memory/graph.db.zst printed "unset"; after, "ours".
Extends artifact_gitattributes_created to pin the ordering so the
regression cannot return.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Alex <plosceac.alexandre@gmail.com>
artifact.c shells out to git twice via cbm_popen with the repo path interpolated
straight into the command:
git -C '%s' rev-parse HEAD 2>/dev/null (git_head_hash)
git -C '%s' config merge.ours.driver true 2>/dev/null (ensure_gitattributes)
Both used SINGLE quotes with NO validation of repo_path. cmd.exe does not honor
single quotes, so on Windows a repo path containing a space broke argument grouping
(the artifact commit hash and merge-driver config silently failed for any spaced repo
path), and an embedded quote or shell metacharacter could break out of the intended
argument. This was the only unvalidated git shell interpolation left in the tree.
Different bug class from the CreateProcess argv-quoting fix (which re-parses a single
command STRING back into argv); this is popen/shell command-string quoting — hence a
separate, atomic PR per review.
Fix: validate repo_path with cbm_artifact_repo_path_is_shell_safe() and switch to
double quotes + a per-platform null device, matching git_context.c (the best-hardened
git shell-out): cbm_validate_shell_arg rejects quote / backslash / substitution
metacharacters, and on Windows we also reject the cmd.exe expansion metacharacters
% ! ^. Double quotes are honored by both POSIX sh and cmd.exe, so a legitimate spaced
repo path now works — the concrete regression the single-quote form caused.
The validator is exposed (artifact.h) and guarded by a new test_artifact.c suite:
plain + spaced paths accepted; quote / ; / $() / backtick / pipe injection rejected;
the cmd.exe metacharacters % ! ^ rejected on Windows (allowed on POSIX, where they are
literal inside double quotes).
Refs: #881
Signed-off-by: Flipper <jacobphilipp@ymail.com>
- Declare cbm_artifact_export_last_error() in artifact.h and drop the ad-hoc
forward declarations in pipeline.c and test_artifact.c so there is a single
shared declaration.
- Document that dump_and_persist_hashes now intentionally propagates a failed
artifact export as a pipeline error (previously silently ignored), so the
behavior change is recorded in-tree.
- Use MoveFileExA(MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) for the
Windows replace in write_file_atomic instead of unlink+rename; it replaces
the destination atomically (credit @Ayush7Ranjan, from #492).
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Add .codebase-memory/graph.db.zst — a zstd-compressed knowledge graph
artifact that can be committed to the repo. Teammates bootstrap from
the artifact instead of running a full reindex from scratch.
- Vendor zstd 1.5.7 (amalgamated build) for 8-13:1 compression
- Two-tier export: zstd -9 + index stripping for explicit index,
zstd -3 for watcher/incremental auto-updates
- Import: decompress → integrity check → auto-recreate indexes
- Bootstrap in handle_index_repository: when no local DB exists but
artifact is present, import first then run incremental
- Auto-create .gitattributes with merge=ours to prevent conflicts
- Fix: add missing idx_edges_url_path to create_user_indexes and
url_path_gen generated column to init_schema
- 13 new tests (5 zstd wrapper + 8 artifact round-trip/edge cases)
0xb007ab1eandClaude Opus 4.8
Martin Vogel
TheStranjer
AlexandClaude Fable 5
Flipper
Matt Van Horn
mvanhorn
test