fix(artifact): reconcile imported hashes against git on bootstrap (1/3)

[moofone]

Tracking issue: #867 (Refs #867). This is part 1 of 3 stacked PRs addressing incremental-reindex cost. Review only commit fix(artifact): reconcile imported hashes against git on bootstrap. Parts 2 (#869) and 3 (#870) stack on top of this one.

What & why

After artifact bootstrap, try_artifact_bootstrap byte-copies a teammate's DB into the local cache, but the imported file_hashes rows carry the exporter's mtime_ns. A fresh clone stamps every file with checkout time, so classify_files (mtime+size only) marks ~every file changed and the first incremental run re-parses the whole repo — the exact run artifact bootstrap exists to make cheap.

Change

Add cbm_artifact_reconcile_hashes() in src/pipeline/artifact.c (+ artifact.h), called from try_artifact_bootstrap immediately after a successful cbm_artifact_import. It re-stamps the file_hashes rows of files git reports unchanged between the artifact's commit and the local working tree, using local stat() values. The existing, untouched classify_files logic then classifies correctly. Zero changes to the incremental pipeline itself.

Reuses two pieces of existing dead code: cbm_artifact_commit() (never called in production) and cbm_store_upsert_file_hash_batch() (existed, never called in production — reconciliation persists all restamped rows in one transaction).

Trust gate (the design point flagged in #867)

cbm_artifact_export now writes an optional "reconcile_basis":"git-clean-head" field to the existing artifact.json, only when export can prove the DB matches a clean checked-out tree at commit:

1. commit is a valid hex OID (40/64-char) from git rev-parse HEAD;
2. the working tree has no tracked/staged/untracked changes outside .codebase-memory/ (:(exclude) pathspec);
3. every file_hashes row's on-disk mtime_ns + size matches the stored stamp (belt-and-suspenders that catches a stale/swapped DB even when the tree looks clean).

Reconciliation requires the marker and the commit to be present + hex-valid + resolvable locally, and skips (returns -1) on any doubt: no git, untrusted/missing marker, non-hex/unknown commit, shallow clone, or any popen/parse uncertainty. A skip leaves rows foreign → today's slow-safe full incremental. No schema_version bump (older binaries ignore the unknown field).

Security

commit is hex-validated before any command construction; repo_path is shell-validated via cbm_validate_shell_arg (same pattern as git_context.c / watcher.c); git output is parsed as NUL-delimited (-z) directly (never line-oriented), so paths with newlines/quotes are handled. A non-empty -z buffer not ending in NUL is treated as corrupt → skip. New cbm_popen call site added to scripts/security-allowlist.txt with justification.

Tests (tests/test_artifact.c, following the existing suite)

• artifact_export_marks_clean_basis — marker set on a clean export, dropped when the tree is dirty.
• artifact_reconcile_restamps_unchanged — full-index → export → clone → modify 2 + add 1 → import → reconcile restamps unchanged rows to local mtime, leaves changed rows foreign (restamped == 3).
• artifact_reconcile_skips_untrusted_metadata — strip the marker → -1, rows untouched (regression guard for old/manual/dirty/stale artifacts).
• artifact_reconcile_skips_unknown_commit — valid-hex-but-absent commit → -1.
• artifact_reconcile_skips_without_git — no .git → -1.
• null-arg safety on cbm_artifact_reconcile_hashes.

All fixtures generated at runtime in tmpdirs; no committed fixtures. scripts/test.sh (ASan+UBSan) green locally — full suite passes.

Benchmark (S2 = bootstrap + first incremental, K = 7 changed)

N	baseline wall / changed	candidate wall / changed
500	635 ms / 503	308 ms / 7
2000	4561 ms / 2003	735 ms / 7
8000	53691 ms / 8003	2563 ms / 7

Candidate S2 wall is 4.8 % of baseline at N=8000 (~21× faster). After S2, candidate node/edge counts exactly equal a from-scratch full index of the same tree (500/2000/8000). Full matrix + raw CSV kept outside the repo.

Checklist

• C only; conventional commit; DCO signed on every commit.
• scripts/test.sh (ASan/UBSan) green.
• New popen/cbm_popen site justified + added to scripts/security-allowlist.txt.
• scripts/lint.sh (clang-tidy/cppcheck/clang-format) — not installed locally; verified ≤100-col + style by hand, CI is the gate.

Seeking maintainer sign-off on the artifact.json trust-marker reuse (point (a) in #867). Happy to rework if a different convention is preferred.

[DeusData]

Thanks for splitting this out from #867. Triage: performance/correctness bug, review first in the #868 -> #869 -> #870 stack.

Because this touches artifact import and the security allowlist file scripts/security-allowlist.txt, review will be a bit cautious: git trust boundaries, path handling, unchanged-file detection, and the necessity/scope of the allowlist update all need to be checked. The focused test coverage in this PR is the right shape.

[moofone]

Thanks for the triage focus areas — here's a map of each to the code, plus one hardening commit pushed in response.

New commit 2e49906 — while re-auditing "unchanged-file detection" against your list, I found one hole and closed it: a file that is gitignored yet indexed (possible via a .cbmignore negation un-skipping a gitignored dir — the #500 feature) appears in file_hashes but is invisible to both git diff and ls-files --others --exclude-standard, so it would have been restamped as "unchanged" even though git can't vouch for its content. Reconciliation now also captures git ls-tree -r -z --name-only <commit> and only restamps rows tracked at the artifact commit and not in the changed set; everything git can't vouch for stays foreign → re-parsed. New regression test artifact_reconcile_skips_untracked_rows covers exactly this shape. The allowlist line was extended to mention ls-tree (same call path, same validation).

Concern → where it's handled:

Git trust boundaries

• Repo-controlled metadata never reaches a git command unless: the reconcile_basis marker equals the sole trusted value AND commit passes is_hex_oid (40/64-char hex) — both hard gates at the top of cbm_artifact_reconcile_hashes.
• repo_path is the only other interpolated component; it goes through cbm_validate_shell_arg (rejects '" ;|&$<> backticks, newlines, and \ on POSIX) in build_git_cmd, mirroring the existing git_context.c/watcher.c pattern, plus a %!^ rejection on Windows.
• Shallow-clone guard: cat-file -e <commit>^{commit} must succeed before any diff is trusted.
• Every uncertainty path (no git, popen failure, non-zero exit, OOM, truncated output) returns -1 → rows stay foreign → today's slow-safe full incremental. Tests: artifact_reconcile_skips_untrusted_metadata, _skips_unknown_commit, _skips_without_git, null-arg safety.

Path handling

• All three git listings are parsed as NUL-delimited (-z) buffers directly — no line-oriented parsing, so paths with newlines/quotes are handled.
• Parse-invariant guard: a non-empty -z buffer not ending in NUL is treated as truncated/corrupt → skip (-1), never a partial parse.
• Per-row snprintf overflow of repo_path + rel_path skips that row (stays foreign), counted in the logged skipped field.

Unchanged-file detection

• Changed set = diff -z --name-only --no-renames <commit> (tracked changes, staged or not, renames shown as add+delete) ∪ ls-files -z --others --exclude-standard (untracked). Rows in the changed set stay foreign; rows missing locally stay foreign and are purged by find_deleted_files; and per the new commit, rows not tracked at the commit stay foreign.
• The exporter side only emits the trust marker when the tree is clean outside .codebase-memory/ AND every hash row's on-disk mtime+size matches the DB (db_hashes_match_disk) — so a stale/swapped DB can't be marked reconcilable even from a clean tree. Test: artifact_export_marks_clean_basis.

Allowlist necessity/scope

• One line, covering the three (now four) read-only git subcommands on the single new call path, all funneled through build_git_cmd's validation. No new process-spawn mechanism — same cbm_popen wrapper as the two existing artifact.c entries.

Also filed #885 for the cross-path bootstrap limitation noted in #867's deferred list (pre-existing, caps this PR's team-sharing benefit until fixed).

Full ASan/UBSan suite green with the new commit. Happy to adjust the trust-marker convention if you'd prefer a different shape for point (a).

[DeusData]

Thanks — this is an excellent writeup and the design (trust-gated reconcile, skip-on-doubt, -z parsing, shell-validated paths) reads exactly right. Two things block the merge:

1. CI: the new tests fail on all three unix/windows legs — git commit exits 128 inside the test fixtures (artifact_reconcile_restamps_unchanged, artifact_reconcile_skips_untracked_rows). The bare CI runners have no git identity, so any git commit in a tmpdir fixture needs it set explicitly, e.g. git -C "$repo" -c user.email=test@test -c user.name=test commit ... (or a one-time git config after init). Locally this passes because of your global identity.
2. Branch staleness — the branch predates several large merges on main (kernel-perf batch, coverage signal). Please update/rebase onto current main so CI revalidates against reality.

One more nit while you're in there: runf(...) == 32768 in the assertion output suggests the helper reports raw system() status — if it's cheap, decoding with WEXITSTATUS would make future failures readable.

Happy to merge once green — parts 2/3 (#869, #870) will follow after this one lands.

[DeusData]

Thanks @moofone — reviewed the whole P1→P2→P3 stack (#868/#869/#870) together. The security posture is clean across all three (careful git-shell hygiene: hex-validated OIDs, cbm_validate_shell_arg, NUL-delimited parsing, and the security-allowlist entry), and the design is sound and clearly #867-scoped. Three things before any of it can land:

1. CI is red on Linux + Windows (all three PRs), and it's a test-harness bug — not your production code. artifact_reconcile_restamps_unchanged and artifact_reconcile_skips_untracked_rows fail with git exit 128 ("committer identity unknown"): the clone_to_b helper in tests/test_artifact.c clones repo B but never sets user.email/user.name in it (only repo A gets git_init). Your laptop has a global git identity so it passes locally; CI runners don't. The LeakSanitizer failure is a knock-on (the proj string leaks on the early ASSERT_EQ abort). Please set a git identity in repo B in the fixture (and free proj on the early-return paths) so the P1 guards are honestly green.

2. All three are CONFLICTING against current main (the kernel-perf work rewrote pipeline.c/pipeline_incremental.c since you branched). They'll need a rebase, and they must land as an ordered stack (fix(artifact): reconcile imported hashes against git on bootstrap (1/3) #868 → refactor(incremental): batch + single-stat persist_hashes (2/3) #869 → feat(incremental): debounce artifact auto re-export (3/3) #870) since the branches are chained by merge commits.

3. Two direction decisions are the maintainer's call and I'm raising them now so there are no surprises after the rebase: (a) fix(artifact): reconcile imported hashes against git on bootstrap (1/3) #868 persists a reconcile_basis:"git-clean-head" trust marker into artifact.json and adds a git-reconcile step on every bootstrap — you explicitly asked for sign-off on that convention, and we'll confirm it; (b) feat(incremental): debounce artifact auto re-export (3/3) #870 changes the default so incremental runs skip artifact re-export below thresholds, adding two permanent env-var knobs (CBM_ARTIFACT_EXPORT_MIN_CHANGES, CBM_ARTIFACT_EXPORT_MAX_AGE_S) plus a drift heuristic — a default-behavior + surface change we'll weigh. refactor(incremental): batch + single-stat persist_hashes (2/3) #869 (P2, the batched single-stat persist) is clean and in-scope on its own.

Really appreciate the depth here — let's get the harness green + rebased and we'll take it from there.

[DeusData]

Reviewed all three PRs in this stack together. First, an apology that matters more than the review: you asked for design sign-off in #867 on 5 July and have been waiting roughly four weeks. You did exactly the right thing — raised the trust-model question before building on it — and we left you without an answer while main moved underneath you. That is our failure, not yours.

This is one of the strongest external stacks we have received. Concretely: a real, measured problem (bootstrap re-parsing nearly everything it just imported), a tracking issue asking for sign-off before landing, and security hygiene on every git shell-out that is better than a lot of our own code — cbm_validate_shell_arg on repo_path, a hard is_hex_oid() gate on the commit value from artifact.json before any command construction, NUL-delimited -z parsing with an explicit truncation guard so filenames containing newlines or quotes are handled. You also reused two existing dead APIs instead of inventing new ones, caught a #500-shaped hole yourself, and fixed it with its own regression test. The ~21× bootstrap win with exact graph-equivalence is the kind of evidence that makes a reviewer's job easy.

The stack structure, confirmed by commit-OID comparison: these literally stack. #869 contains all of #868's commits, and #870 contains all of both. Since all three target main, merging #870 would merge the entire stack at once. So the order is forced — 868, then 869, then 870 — with a re-diff after each.

---

Two decisions are the maintainer's, and both are genuinely yours to have asked about.

1. The reconcile_basis trust marker (#868). This is the one you raised in #867, and the review agrees it is a real trust-model question rather than a code-quality one. The marker is self-attested by the artifact producer and unverifiable by the importer. Today a tampered artifact gets accidentally scrubbed on first run, because every file re-parses on the mtime mismatch. With reconcile, files git reports as unchanged are not re-parsed — so poisoned graph rows in a crafted artifact would survive in exactly those files. It stays within the existing artifact trust model (importing already byte-copies a foreign DB from the same trust domain as the repo), and your tracked-set and local-git-diff gates are sound. But it removes an accidental safety property, and that trade — scrubbing versus the bootstrap win — is the maintainer's call.

2. The debounce policy and env surface (#870). CBM_ARTIFACT_EXPORT_MIN_CHANGES and CBM_ARTIFACT_EXPORT_MAX_AGE_S would be permanent public configuration, and the 10 / 24 h defaults are themselves a policy choice about acceptable staleness in a shared artifact.

I have put both in front of the maintainer explicitly. Please do not spend time rebasing until those are answered — the stack's fate hangs on the first one, and it would be unfair to have you resolve four weeks of drift for a design that might not land.

---

Meanwhile, three things worth knowing.

Your CI red is not your production code. clone_to_b() does not set a git identity in the clone, so git commit exits 128 on identity-less runners, failing two reconcile tests — and the assert-fail path then leaks proj (68 bytes), which reddens the ASan leg a second way. Both are two-line test fixes.

Main moved under you, and it changes #869. Commit b79c6da made hash persistence all-or-nothing: persist_hashes now returns int, and any failed upsert rejects the whole staging generation, because atomic publication makes preserving the complete previous generation safer than installing partial metadata. Your row-at-a-time fallback was written against the old "partial preservation beats total loss" contract and now points backwards. The irony is that your single-transaction batch fits the new model better than what you wrote — the right rework keeps the classify-time stamps and the batch upsert, returns the batch return code, and drops the fallback loop entirely. I verified the transactionality concern I most cared about, incidentally: a crash mid-batch rolls back, old stamps survive, and the next run re-parses. That is the safe direction and it converges.

#870 needs re-targeting: the incremental re-export moved to export_after_publish in pipeline.c.

One test note for #869 when it does get reworked: incremental_persist_batch_roundtrip passes unmodified on main, because without a mid-run edit the post-run re-stat produces the same mtime. It is a fine guard for the batch path, but the headline claim — that the old code masks a mid-run edit — currently ships untested. A seam that mutates the file between classify and persist would bind it.

And an offer. Given all three conflict with a heavily-reworked main, if you would rather not carry that rebase, we can distill the approved design points onto main ourselves with Co-Authored-By credit to you. Entirely your preference — the work is yours either way, and I would rather ask than assume.

Answers on #867 coming.