Skip to content

Commit 4cbc9e1

Browse files
DeusDatamoofone
andcommitted
fix(artifact): stamp the clean-basis trust check the way the rows were written
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>
1 parent a9d635f commit 4cbc9e1

3 files changed

Lines changed: 212 additions & 25 deletions

File tree

‎src/pipeline/artifact.c‎

Lines changed: 112 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,18 @@ static const char *itoa_buf(int v) {
6969
return bufs[i];
7070
}
7171

72+
/* Same rotating-buffer trick for 64-bit values. mtime_ns does not fit an int,
73+
* and a diagnostic that truncates the number it is there to explain is worse
74+
* than no diagnostic. */
75+
static const char *i64_buf(int64_t v) {
76+
static _Thread_local char bufs[ART_RING][CBM_SZ_32];
77+
static _Thread_local int idx = 0;
78+
int i = idx;
79+
idx = (idx + ART_NUL) & ART_RING_MASK;
80+
snprintf(bufs[i], sizeof(bufs[i]), "%lld", (long long)v);
81+
return bufs[i];
82+
}
83+
7284
const char *cbm_artifact_export_last_error(void) {
7385
return g_export_error[0] ? g_export_error : NULL;
7486
}
@@ -517,8 +529,27 @@ static bool reconcile_is_synthetic_row(const char *rel_path) {
517529
* mtime_ns + size matches the stored stamp. Any stat failure, path overflow,
518530
* or mismatch makes the artifact untrusted for reconciliation — this is the
519531
* belt-and-suspenders that catches a stale/swapped DB even when the tree looks
520-
* clean. */
521-
static bool db_hashes_match_disk(const char *repo_path, const char *db_path, const char *project) {
532+
* clean.
533+
*
534+
* The disk side MUST be read with cbm_path_info_utf8, because that is the
535+
* function that WROTE the rows being compared: the semantic manifest stamps
536+
* every row via pipeline_incremental.c's semantic_manifest_hash_file, which
537+
* calls cbm_path_info_utf8. On POSIX any lstat-based helper agrees with it,
538+
* but on Windows cbm_path_info_utf8 derives mtime_ns from ftLastWriteTime
539+
* (100-ns FILETIME ticks) while a _wstat64 st_mtime carries WHOLE SECONDS —
540+
* so a seconds-truncated re-stat can only ever equal the stored value when a
541+
* file's write time lands exactly on a second boundary. It essentially never
542+
* does, which made this check fail for every row on Windows and only on
543+
* Windows, suppressing the reconcile_basis marker there. Compare like with
544+
* like: same function on both sides, no encoding to keep in sync.
545+
*
546+
* cbm_path_info_utf8 reports symlinks/reparse points rather than following
547+
* them, so the is_symlink/is_regular guard below preserves exactly the
548+
* refusal reconcile_stat_no_symlink provides (git tracks a symlink's link
549+
* text, not its target's bytes). */
550+
static bool db_hashes_match_disk(const char *repo_path, const char *db_path, const char *project,
551+
char *detail, size_t detail_sz) {
552+
snprintf(detail, detail_sz, "store_unreadable");
522553
cbm_store_t *s = cbm_store_open_path(db_path);
523554
if (!s) {
524555
return false;
@@ -527,9 +558,11 @@ static bool db_hashes_match_disk(const char *repo_path, const char *db_path, con
527558
int count = 0;
528559
bool match = true;
529560
if (cbm_store_get_file_hashes(s, project, &hashes, &count) != CBM_STORE_OK) {
561+
snprintf(detail, detail_sz, "file_hashes_unreadable project=%s", project);
530562
cbm_store_close(s);
531563
return false;
532564
}
565+
detail[0] = '\0';
533566
for (int i = 0; i < count; i++) {
534567
if (reconcile_is_synthetic_row(hashes[i].rel_path)) {
535568
continue;
@@ -538,12 +571,29 @@ static bool db_hashes_match_disk(const char *repo_path, const char *db_path, con
538571
int n = snprintf(abs, sizeof(abs), "%s/%s", repo_path, hashes[i].rel_path);
539572
if (n < 0 || n >= (int)sizeof(abs)) {
540573
match = false;
574+
snprintf(detail, detail_sz, "path=%s reason=path_too_long", hashes[i].rel_path);
541575
break;
542576
}
543-
struct stat st;
544-
if (reconcile_stat_no_symlink(abs, &st) != 0 ||
545-
art_stat_mtime_ns(&st) != hashes[i].mtime_ns || (int64_t)st.st_size != hashes[i].size) {
577+
cbm_path_info_t info;
578+
if (cbm_path_info_utf8(abs, &info) != 0 || !info.is_regular || info.is_symlink) {
546579
match = false;
580+
snprintf(detail, detail_sz, "path=%s reason=not_a_readable_regular_file",
581+
hashes[i].rel_path);
582+
break;
583+
}
584+
if (info.mtime_ns != hashes[i].mtime_ns || info.size != hashes[i].size) {
585+
match = false;
586+
/* BOTH stamps are reported: a stale/swapped DB and a stamp-encoding
587+
* mismatch look identical at the boolean, and the numbers are the
588+
* only thing that separates them on a platform we cannot attach a
589+
* debugger to. */
590+
snprintf(detail, detail_sz,
591+
"path=%s reason=%s stored_mtime_ns=%s disk_mtime_ns=%s stored_size=%s "
592+
"disk_size=%s",
593+
hashes[i].rel_path,
594+
info.size != hashes[i].size ? "size_differs" : "mtime_differs",
595+
i64_buf(hashes[i].mtime_ns), i64_buf(info.mtime_ns), i64_buf(hashes[i].size),
596+
i64_buf(info.size));
547597
break;
548598
}
549599
}
@@ -580,6 +630,49 @@ static bool read_metadata_reconcile_trusted(const char *repo_path) {
580630
return trusted;
581631
}
582632

633+
/* Description of why the last export on THIS thread withheld the clean-basis
634+
* marker; empty when it wrote one. See cbm_artifact_reconcile_basis_last_blocker. */
635+
static _Thread_local char g_basis_blocker[CBM_SZ_512];
636+
637+
const char *cbm_artifact_reconcile_basis_last_blocker(void) {
638+
return g_basis_blocker[0] ? g_basis_blocker : NULL;
639+
}
640+
641+
/* Evaluate export's four clean-basis preconditions, recording the first one
642+
* that fails in g_basis_blocker. Returns true iff all four hold.
643+
*
644+
* Split out of cbm_artifact_export deliberately: as one short-circuiting &&
645+
* chain the four gates collapsed into a single bool, and a marker that went
646+
* missing on one platform gave no way to tell "HEAD unresolved" from "tree
647+
* dirty" from "DB does not match disk" — which is exactly how a Windows-only
648+
* stamp-encoding bug hid behind a suspected quoting bug for two diagnoses.
649+
* The reason is recorded rather than merely logged so a caller can quote it
650+
* verbatim: recomputing it after the fact reports a DIFFERENT gate, because
651+
* export's own ensure_gitattributes leaves an untracked .gitattributes behind
652+
* and every later evaluation then answers tree_not_clean. */
653+
static bool reconcile_basis_trusted(const char *repo_path, const char *db_path,
654+
const char *project_name, const char *commit, bool has_commit) {
655+
g_basis_blocker[0] = '\0';
656+
if (!has_commit) {
657+
snprintf(g_basis_blocker, sizeof(g_basis_blocker), "head_unresolved");
658+
return false;
659+
}
660+
if (!is_hex_oid(commit)) {
661+
snprintf(g_basis_blocker, sizeof(g_basis_blocker), "head_not_hex_oid");
662+
return false;
663+
}
664+
if (!tree_clean_for_reconcile(repo_path)) {
665+
snprintf(g_basis_blocker, sizeof(g_basis_blocker), "tree_not_clean");
666+
return false;
667+
}
668+
char detail[CBM_SZ_256] = "";
669+
if (!db_hashes_match_disk(repo_path, db_path, project_name, detail, sizeof(detail))) {
670+
snprintf(g_basis_blocker, sizeof(g_basis_blocker), "db_hashes_differ_from_disk %s", detail);
671+
return false;
672+
}
673+
return true;
674+
}
675+
583676
/* ── Metadata read/write ─────────────────────────────────────────── */
584677

585678
/* Read schema_version from artifact.json. Returns -1 if missing/invalid. */
@@ -951,9 +1044,11 @@ int cbm_artifact_export(const char *db_path, const char *repo_path, const char *
9511044
* bootstrap falls back to today's slow-safe full re-parse. */
9521045
char commit[CBM_SZ_64] = "";
9531046
bool has_commit = git_head_hash(repo_path, commit, sizeof(commit));
954-
bool reconcile_trusted = has_commit && is_hex_oid(commit) &&
955-
tree_clean_for_reconcile(repo_path) &&
956-
db_hashes_match_disk(repo_path, db_path, project_name);
1047+
bool reconcile_trusted =
1048+
reconcile_basis_trusted(repo_path, db_path, project_name, commit, has_commit);
1049+
if (!reconcile_trusted) {
1050+
cbm_log_info("artifact.reconcile_basis_omitted", "reason", g_basis_blocker);
1051+
}
9571052

9581053
/* Write metadata */
9591054
if (write_metadata(repo_path, project_name, commit, nodes, edges, db_size, (size_t)clen,
@@ -1352,6 +1447,15 @@ static int reconcile_restamp_rows(const char *repo_path, const char *cache_db_pa
13521447
batch[batch_n].project = project_name;
13531448
batch[batch_n].rel_path = stored[i].rel_path;
13541449
batch[batch_n].sha256 = stored[i].sha256;
1450+
/* art_stat_mtime_ns, NOT cbm_path_info_utf8 (which db_hashes_match_disk
1451+
* uses): the two disagree on Windows and each side must match ITS OWN
1452+
* consumer. A restamped row exists to be read by the incremental
1453+
* classifier (pipeline_incremental.c classify_files) and by
1454+
* check_index_coverage (mcp.c coverage_path_freshness) — both stat()
1455+
* the file and encode whole seconds on Windows. Stamping a 100-ns
1456+
* FILETIME value here would make every restamped row read as changed
1457+
* there and reconcile would buy nothing. Do not unify these two call
1458+
* sites without changing those consumers first. */
13551459
batch[batch_n].mtime_ns = art_stat_mtime_ns(&st);
13561460
batch[batch_n].size = (int64_t)st.st_size;
13571461
batch_n++;

‎src/pipeline/artifact.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,18 @@ int cbm_artifact_export(const char *db_path, const char *repo_path, const char *
3939
* Returns NULL if no export error is recorded. */
4040
const char *cbm_artifact_export_last_error(void);
4141

42+
/* Why the most recent cbm_artifact_export on THIS thread did NOT write the
43+
* optional "reconcile_basis" marker, or NULL when it did write one. The string
44+
* names the failing precondition (head_unresolved / head_not_hex_oid /
45+
* tree_not_clean / db_hashes_differ_from_disk) and, for the last of those, the
46+
* offending row with both stamps.
47+
*
48+
* Must be read from the thread that called export and BEFORE anything else
49+
* exports on it. It cannot be reconstructed by re-running export: export's own
50+
* ensure_gitattributes leaves an untracked .gitattributes behind, so a second
51+
* evaluation always answers tree_not_clean regardless of the real cause. */
52+
const char *cbm_artifact_reconcile_basis_last_blocker(void);
53+
4254
/* Import artifact from .codebase-memory/graph.db.zst to cache_db_path.
4355
* Decompresses, runs integrity check, recreates indexes.
4456
* Returns 0 on success, -1 on error. */

‎tests/test_artifact.c‎

Lines changed: 88 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -634,10 +634,48 @@ static bool stamp_row_mtime(const char *db, const char *proj, const char *rel, i
634634
return rc == CBM_STORE_OK;
635635
}
636636

637+
/* Why the last build_trusted_artifact_repo() call returned NULL. Read by the
638+
* BUILD_TRUSTED_REPO_OR_FAIL() call sites.
639+
*
640+
* A setup helper whose only failure signal is "NULL" is untestable on a
641+
* platform nobody can attach to: six distinct exits (git commit, pipeline
642+
* construction, pipeline run, marker absence, artifact commit, project-name
643+
* derivation) all reported the same thing, and CI could not tell a failed
644+
* `git commit` from a missing reconcile_basis marker. Every exit now names
645+
* itself and carries the number that decided it. */
646+
static char g_build_repo_err[4096];
647+
648+
static void build_repo_errf(const char *fmt, ...) {
649+
va_list ap;
650+
va_start(ap, fmt);
651+
vsnprintf(g_build_repo_err, sizeof(g_build_repo_err), fmt, ap);
652+
va_end(ap);
653+
}
654+
655+
/* Report which of export's four clean-basis preconditions denied the marker.
656+
* Read straight from export's recorded blocker rather than recomputed: export's
657+
* own ensure_gitattributes leaves an untracked .gitattributes behind, so any
658+
* later evaluation answers tree_not_clean whatever the real cause was. Must run
659+
* on the thread that ran the pipeline (cbm_pipeline_run exports inline). */
660+
static void describe_missing_marker(const char *repo) {
661+
const char *blocker = cbm_artifact_reconcile_basis_last_blocker();
662+
char meta[1152];
663+
snprintf(meta, sizeof(meta), "%s/.codebase-memory/artifact.json", repo);
664+
struct stat st;
665+
build_repo_errf(
666+
"step=reconcile_basis: marker absent from %s (artifact.json %s); export blocked by: %s",
667+
meta, stat(meta, &st) == 0 ? "present" : "MISSING",
668+
blocker ? blocker
669+
: "<nothing — export believed it wrote the marker, so the file read here is not "
670+
"the file export wrote>");
671+
}
672+
637673
/* Build a git repo at g_repo with 5 .rs files, full-index + export (clean tree
638674
* -> reconcile_basis marker set), and commit .codebase-memory so a clone receives
639-
* the artifact. Returns the derived project name (caller frees) or NULL. */
675+
* the artifact. Returns the derived project name (caller frees) or NULL, with
676+
* g_build_repo_err naming the failing step. */
640677
static char *build_trusted_artifact_repo(void) {
678+
g_build_repo_err[0] = '\0';
641679
git_init(g_repo);
642680
const char *names[] = {"a.rs", "b.rs", "c.rs", "d.rs", "e.rs"};
643681
for (int i = 0; i < 5; i++) {
@@ -647,29 +685,58 @@ static char *build_trusted_artifact_repo(void) {
647685
snprintf(body, sizeof(body), "pub fn f%d() {}\n", i);
648686
write_text_file(p, body);
649687
}
650-
if (runf("git -C \"%s\" add -A && git -C \"%s\" commit -qm init", g_repo, g_repo) != 0) {
688+
int git_rc = runf("git -C \"%s\" add -A && git -C \"%s\" commit -qm init", g_repo, g_repo);
689+
if (git_rc != 0) {
690+
build_repo_errf("step=commit_sources: `git add -A && git commit -qm init` in %s "
691+
"exited status=%d",
692+
g_repo, git_rc);
651693
return NULL;
652694
}
653695
cbm_pipeline_t *p = cbm_pipeline_new(g_repo, g_db, CBM_MODE_FAST);
654696
if (!p) {
697+
build_repo_errf("step=pipeline_new: cbm_pipeline_new(repo=%s, db=%s) returned NULL", g_repo,
698+
g_db);
655699
return NULL;
656700
}
657701
cbm_pipeline_set_persistence(p, true);
658702
int rc = cbm_pipeline_run(p);
659703
cbm_pipeline_free(p);
660704
if (rc != 0) {
705+
build_repo_errf("step=pipeline_run: cbm_pipeline_run(repo=%s) rc=%d", g_repo, rc);
706+
return NULL;
707+
}
708+
char *project = cbm_project_name_from_path(g_repo);
709+
if (!project) {
710+
build_repo_errf("step=project_name: cbm_project_name_from_path(%s) returned NULL", g_repo);
661711
return NULL;
662712
}
663713
/* Clean source tree at export -> marker must be present. */
664714
if (!meta_contains(g_repo, "\"reconcile_basis\"")) {
715+
describe_missing_marker(g_repo);
716+
free(project);
665717
return NULL;
666718
}
667-
if (runf("git -C \"%s\" add -A && git -C \"%s\" commit -qm artifact", g_repo, g_repo) != 0) {
719+
git_rc = runf("git -C \"%s\" add -A && git -C \"%s\" commit -qm artifact", g_repo, g_repo);
720+
if (git_rc != 0) {
721+
build_repo_errf("step=commit_artifact: `git add -A && git commit -qm artifact` in %s "
722+
"exited status=%d",
723+
g_repo, git_rc);
724+
free(project);
668725
return NULL;
669726
}
670-
return cbm_project_name_from_path(g_repo);
727+
return project;
671728
}
672729

730+
/* Call-site form: keeps file:line on the failing TEST while printing the step
731+
* that actually failed inside the helper. */
732+
#define BUILD_TRUSTED_REPO_OR_FAIL(var) \
733+
do { \
734+
(var) = build_trusted_artifact_repo(); \
735+
if (!(var)) { \
736+
FAIL(g_build_repo_err); \
737+
} \
738+
} while (0)
739+
673740
/* Clones g_repo (A) into <tmp>/work/repo (B) so both share the basename "repo"
674741
* and thus the derived project name — required for artifact bootstrap. */
675742
static bool clone_to_b(char *repoB_out, size_t repoB_sz) {
@@ -708,7 +775,11 @@ TEST(artifact_export_marks_clean_basis) {
708775
cbm_pipeline_set_persistence(p, true);
709776
ASSERT_EQ(cbm_pipeline_run(p), 0);
710777
cbm_pipeline_free(p);
711-
ASSERT(meta_contains(g_repo, "\"reconcile_basis\""));
778+
if (!meta_contains(g_repo, "\"reconcile_basis\"")) {
779+
describe_missing_marker(g_repo);
780+
free(proj);
781+
FAIL(g_build_repo_err);
782+
}
712783

713784
/* Dirty the source tree (uncommitted edit). Re-export must omit the marker:
714785
* the tree is non-clean outside .codebase-memory (and the a.rs hash row no
@@ -724,8 +795,8 @@ TEST(artifact_export_marks_clean_basis) {
724795

725796
TEST(artifact_reconcile_restamps_unchanged) {
726797
setup_artifact_test();
727-
char *proj = build_trusted_artifact_repo();
728-
ASSERT_NOT_NULL(proj);
798+
char *proj = NULL;
799+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
729800

730801
char repoB[1024];
731802
ASSERT(clone_to_b(repoB, sizeof(repoB)));
@@ -775,8 +846,8 @@ TEST(artifact_reconcile_restamps_unchanged) {
775846

776847
TEST(artifact_reconcile_skips_untracked_rows) {
777848
setup_artifact_test();
778-
char *proj = build_trusted_artifact_repo();
779-
ASSERT_NOT_NULL(proj);
849+
char *proj = NULL;
850+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
780851
char repoB[1024];
781852
ASSERT(clone_to_b(repoB, sizeof(repoB)));
782853

@@ -822,8 +893,8 @@ TEST(artifact_reconcile_skips_untracked_rows) {
822893

823894
TEST(artifact_reconcile_skips_untrusted_metadata) {
824895
setup_artifact_test();
825-
char *proj = build_trusted_artifact_repo();
826-
ASSERT_NOT_NULL(proj);
896+
char *proj = NULL;
897+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
827898
char repoB[1024];
828899
ASSERT(clone_to_b(repoB, sizeof(repoB)));
829900

@@ -867,8 +938,8 @@ TEST(artifact_reconcile_skips_untrusted_metadata) {
867938

868939
TEST(artifact_reconcile_skips_unknown_commit) {
869940
setup_artifact_test();
870-
char *proj = build_trusted_artifact_repo();
871-
ASSERT_NOT_NULL(proj);
941+
char *proj = NULL;
942+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
872943
char repoB[1024];
873944
ASSERT(clone_to_b(repoB, sizeof(repoB)));
874945

@@ -897,8 +968,8 @@ TEST(artifact_reconcile_skips_unknown_commit) {
897968

898969
TEST(artifact_reconcile_skips_non_hex_commit) {
899970
setup_artifact_test();
900-
char *proj = build_trusted_artifact_repo();
901-
ASSERT_NOT_NULL(proj);
971+
char *proj = NULL;
972+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
902973
char repoB[1024];
903974
ASSERT(clone_to_b(repoB, sizeof(repoB)));
904975

@@ -930,8 +1001,8 @@ TEST(artifact_reconcile_skips_non_hex_commit) {
9301001

9311002
TEST(artifact_reconcile_skips_without_git) {
9321003
setup_artifact_test();
933-
char *proj = build_trusted_artifact_repo();
934-
ASSERT_NOT_NULL(proj);
1004+
char *proj = NULL;
1005+
BUILD_TRUSTED_REPO_OR_FAIL(proj);
9351006
char repoB[1024];
9361007
ASSERT(clone_to_b(repoB, sizeof(repoB)));
9371008

0 commit comments

Comments
 (0)