fix(store): ADR sidecar follow-ups from #2315 review

src/pipeline/artifact.c

@@ -1130,6 +1130,28 @@ int cbm_artifact_export(const char *db_path, const char *repo_path, const char *
 
 /* ── Import ──────────────────────────────────────────────────────── */
 
+/* Restore the imported ADR (carried in the graph DB's project_summaries) into
+ * the per-project sidecar so it arrives with the shared artifact — even for a
+ * teammate who already indexed the project (a marked sidecar with no ADR),
+ * which the marker-gated migrate-once would skip. INSERT OR IGNORE means a
+ * local ADR is never overwritten. Best-effort: the graph DB is already
+ * installed when this runs, so a failed restore never fails the import, but it
+ * is logged with the store's reason so a team ADR that did not arrive is
+ * diagnosable instead of silently missing. */
+static void artifact_import_restore_adr(const char *cache_db_path) {
+    cbm_store_t *restore = cbm_store_open_path_existing(cache_db_path);
+    if (!restore) {
+        cbm_log_warn("artifact.import", "err", "adr_restore", "detail", "reopen_imported_db",
+                     "path", cache_db_path);
+        return;
+    }
+    if (cbm_store_adr_restore_from_graph_if_absent(restore) != CBM_STORE_OK) {
+        cbm_log_warn("artifact.import", "err", "adr_restore", "detail", cbm_store_error(restore),
+                     "path", cache_db_path);
+    }
+    cbm_store_close(restore);
+}
+
 int cbm_artifact_import(const char *repo_path, const char *cache_db_path) {
     if (!repo_path || !cache_db_path) {
         return CBM_NOT_FOUND;
@@ -1257,16 +1279,7 @@ int cbm_artifact_import(const char *repo_path, const char *cache_db_path) {
     cbm_unlink(wal);
     cbm_unlink(shm);
 
-    /* Restore the imported ADR (carried in the graph DB's project_summaries) into
-     * the per-project sidecar so it arrives with the shared artifact — even for a
-     * teammate who already indexed the project (a marked sidecar with no ADR),
-     * which the marker-gated migrate-once would skip. INSERT OR IGNORE means a
-     * local ADR is never overwritten. Best-effort. */
-    cbm_store_t *restore = cbm_store_open_path_existing(cache_db_path);
-    if (restore) {
-        (void)cbm_store_adr_restore_from_graph_if_absent(restore);
-        cbm_store_close(restore);
-    }
+    artifact_import_restore_adr(cache_db_path);
 
     cbm_log_info("artifact.import", "db", cache_db_path, "size_mb",
                  itoa_buf((int)((size_t)dlen / ART_BYTES_PER_MB)));

src/store/store.c

@@ -12010,13 +12010,6 @@ static bool adr_migrated(sqlite3 *adb) {
     return done;
 }
 
-/* Copy any legacy ADR rows from the graph DB's project_summaries into the
- * sidecar (INSERT OR IGNORE, so a local row is never clobbered), then set the
- * one-time marker. The marker check, copy and marker insert run in ONE
- * BEGIN IMMEDIATE transaction on the sidecar, so two processes migrating at
- * once cannot re-copy a row the other just deleted. Returns CBM_STORE_ERR on
- * any failure (transaction rolled back) so callers can fail closed and preserve
- * the old generation. No-op once the marker is set. */
 /* Copy every ADR row from the graph DB's project_summaries into the sidecar with
  * INSERT OR IGNORE (so a row already present locally is never overwritten). Must
  * run inside a transaction on the sidecar connection. Returns OK/ERR; does not
@@ -12070,6 +12063,13 @@ static int adr_copy_rows(cbm_store_t *s, sqlite3 *adb) {
     return (rc == SQLITE_DONE) ? CBM_STORE_OK : CBM_STORE_ERR;
 }
 
+/* Copy any legacy ADR rows from the graph DB's project_summaries into the
+ * sidecar (INSERT OR IGNORE, so a local row is never clobbered), then set the
+ * one-time marker. The marker check, copy and marker insert run in ONE
+ * BEGIN IMMEDIATE transaction on the sidecar, so two processes migrating at
+ * once cannot re-copy a row the other just deleted. Returns CBM_STORE_ERR on
+ * any failure (transaction rolled back) so callers can fail closed and preserve
+ * the old generation. No-op once the marker is set. */
 static int adr_copy_legacy_once(cbm_store_t *s, sqlite3 *adb) {
     if (adr_migrated(adb)) {
         return CBM_STORE_OK; /* fast path: already migrated, no lock needed */
@@ -12106,19 +12106,22 @@ static int adr_copy_legacy_once(cbm_store_t *s, sqlite3 *adb) {
  * DB's project_summaries, and a teammate who already indexed the project (so the
  * sidecar is marked, but has no ADR) must still receive it. INSERT OR IGNORE
  * guarantees a local ADR is never clobbered; running ungated by the marker is
- * what distinguishes this from the one-time legacy migration. */
+ * what distinguishes this from the one-time legacy migration. It never writes
+ * the marker: `adb` comes from adr_conn_write, which hands out a sidecar handle
+ * only after adr_copy_legacy_once committed it, and nothing deletes it. On
+ * failure the store error names the step (the import logs it). */
 static int adr_restore_if_absent(cbm_store_t *s, sqlite3 *adb) {
     if (sqlite3_exec(adb, "BEGIN IMMEDIATE;", NULL, NULL, NULL) != SQLITE_OK) {
+        store_set_error_sqlite_on(s, adb, "adr restore begin");
         return CBM_STORE_ERR;
     }
     if (adr_copy_rows(s, adb) != CBM_STORE_OK) {
+        store_set_error(s, "adr restore copy failed");
         (void)sqlite3_exec(adb, "ROLLBACK;", NULL, NULL, NULL);
         return CBM_STORE_ERR;
     }
-    /* Mark migrated too, so a later reindex's before-delete migration is a no-op. */
-    (void)sqlite3_exec(adb, "INSERT OR IGNORE INTO adr_meta (key, value) VALUES ('migrated','1');",
-                       NULL, NULL, NULL);
     if (sqlite3_exec(adb, "COMMIT;", NULL, NULL, NULL) != SQLITE_OK) {
+        store_set_error_sqlite_on(s, adb, "adr restore commit");
         (void)sqlite3_exec(adb, "ROLLBACK;", NULL, NULL, NULL);
         return CBM_STORE_ERR;
     }

tests/test_artifact.c

@@ -252,6 +252,21 @@ TEST(artifact_export_roundtrip_keeps_adr) {
     cbm_store_adr_free(&adr);
     cbm_store_close(dst);
 
+    /* The fresh destination sidecar is marked migrated after the import, so a
+     * later reindex's migrate-before-delete is a no-op. Opening the sidecar for
+     * the restore already commits the marker; the restore itself adds none. */
+    char side[1100];
+    snprintf(side, sizeof(side), "%s.adr.db", import_db);
+    sqlite3 *raw = NULL;
+    ASSERT_EQ(sqlite3_open_v2(side, &raw, SQLITE_OPEN_READONLY, NULL), SQLITE_OK);
+    sqlite3_stmt *st = NULL;
+    ASSERT_EQ(
+        sqlite3_prepare_v2(raw, "SELECT value FROM adr_meta WHERE key='migrated';", -1, &st, NULL),
+        SQLITE_OK);
+    ASSERT_EQ(sqlite3_step(st), SQLITE_ROW);
+    sqlite3_finalize(st);
+    sqlite3_close(raw);
+
     cleanup_dir(g_tmpdir);
     PASS();
 }
@@ -391,6 +406,42 @@ TEST(artifact_import_restores_adr_when_local_absent) {
     PASS();
 }
 
+/* A failed ADR restore must not be silent (#2315 follow-up). A DIRECTORY at
+ * "<db>.adr.db" makes the sidecar open fail deterministically (no hook). The
+ * graph DB is already installed when the restore runs, so the import still
+ * succeeds (best-effort, unchanged), but the team ADR that did not arrive is
+ * logged with the store's reason instead of being dropped without a trace. */
+TEST(artifact_import_logs_failed_adr_restore) {
+    setup_artifact_test();
+    create_test_db(g_db);
+
+    cbm_store_t *src = cbm_store_open_path(g_db);
+    ASSERT_NOT_NULL(src);
+    ASSERT_EQ(cbm_store_adr_store(src, "test-proj", "# Decision\nshared A"), CBM_STORE_OK);
+    cbm_store_close(src);
+    ASSERT_EQ(cbm_artifact_export(g_db, g_repo, "test-proj", CBM_ARTIFACT_FAST), 0);
+
+    char import_db[1024];
+    snprintf(import_db, sizeof(import_db), "%s/imported.db", g_tmpdir);
+    char adr_dir[1100];
+    snprintf(adr_dir, sizeof(adr_dir), "%s.adr.db", import_db);
+    ASSERT_EQ(cbm_mkdir(adr_dir), 0);
+
+    capture_logs_start();
+    int rc = cbm_artifact_import(g_repo, import_db);
+    const char *logs = capture_logs_end();
+
+    ASSERT_EQ(rc, 0); /* the graph DB is installed; the restore stays best-effort */
+    ASSERT_TRUE(cbm_file_exists(import_db));
+    const char *line = strstr(logs, "level=warn msg=artifact.import err=adr_restore");
+    ASSERT_NOT_NULL(line);
+    /* The store's reason is carried (the text log format maps spaces to '_'). */
+    ASSERT(strstr(line, "detail=adr_sidecar_open") != NULL);
+
+    cleanup_dir(g_tmpdir);
+    PASS();
+}
+
 TEST(artifact_export_best_roundtrip) {
     setup_artifact_test();
     create_test_db(g_db);
@@ -1361,6 +1412,7 @@ SUITE(artifact) {
     RUN_TEST(artifact_export_without_adr_does_not_resurrect_legacy_row);
     RUN_TEST(artifact_import_does_not_clobber_local_adr);
     RUN_TEST(artifact_import_restores_adr_when_local_absent);
+    RUN_TEST(artifact_import_logs_failed_adr_restore);
     RUN_TEST(artifact_export_best_roundtrip);
     RUN_TEST(artifact_exists_check);
     RUN_TEST(artifact_commit_hash);