From fa2292111802b5f3b66db3e4dd168280b53ac17e Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 13:32:22 +0200 Subject: [PATCH] fix(mcp): name the artifact phase when a post-publish export fails Distilled from #1789 by Ulises Millan Guerrero, unchanged except for the five openspec/ scaffolding files that could not land. The indexing pipeline already logged its own phase error when artifact export failed after publication, but the MCP failure path returned the generic "Pipeline failed. Check repo_path" regardless. A caller whose export failed was told to check a repository path that was fine, while the one log line naming the real phase sat somewhere they were not looking. The fix carries the failing phase out of the pipeline and into the MCP error, so the message names artifact export when that is what failed. The existing fail-hard contract is preserved -- this changes what the caller is told, not whether the run fails. Why this is a distill rather than the original: five of the nine files in on main, no other contributor uses it, and adopting a spec framework is a project-level decision rather than something that rides in on a bug fix. That was explained on 2026-09-02 and the code half was approved on merit the same day; the scaffolding is the only reason it did not land then. Co-authored-by: Ulises Millan Guerrero Signed-off-by: Martin Vogel --- src/mcp/mcp.c | 22 ++++++++++++-- src/pipeline/pipeline.c | 18 +++++++++++ src/pipeline/pipeline.h | 7 +++++ tests/test_pipeline.c | 66 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index d6c7bd4686..f524564344 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -11414,9 +11414,25 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { yyjson_mut_obj_add_strcpy(doc, root, "message", message); } else { yyjson_mut_obj_add_str(doc, root, "status", "error"); - yyjson_mut_obj_add_str(doc, root, "hint", - "Pipeline failed. Check repo_path exists and contains source files. " - "Try mode='fast' for a quicker diagnostic run."); + /* #1665: a post-publish artifact export failure (read-only repo, etc.) + * must not be blamed on the repository path. The pipeline snapshots the + * export error of THIS run, so its presence names the phase exactly — + * the graph database was already published when the export ran. */ + const char *export_error = cbm_pipeline_export_error(p); + if (export_error && export_error[0]) { + char hint[CBM_SZ_1K]; + (void)snprintf( + hint, sizeof(hint), + "Index database was published, but the persistence artifact export failed for " + "repo_path/.codebase-memory (%s). Use a writable checkout, or re-run with " + "--persistence false if the shared artifact is not needed.", + export_error); + yyjson_mut_obj_add_strcpy(doc, root, "hint", hint); + } else { + yyjson_mut_obj_add_str(doc, root, "hint", + "Pipeline failed. Check repo_path exists and contains source " + "files. Try mode='fast' for a quicker diagnostic run."); + } } char *json = yy_doc_to_str(doc); diff --git a/src/pipeline/pipeline.c b/src/pipeline/pipeline.c index fa62441a11..c808cb1b47 100644 --- a/src/pipeline/pipeline.c +++ b/src/pipeline/pipeline.c @@ -193,6 +193,13 @@ struct cbm_pipeline { cbm_index_resource_policy_t resource_policy; cbm_index_resource_violation_t resource_violation; + /* Snapshot of the artifact export failure of THIS run (set only by + * export_after_publish failure, zeroed at run start, cleared on success). + * The MCP layer reads it to attribute a failed run to artifact export + * without consulting cbm_artifact_export_last_error() directly — that + * global can still hold a PREVIOUS run's error. */ + char export_error[CBM_SZ_1K]; + /* Indexing state (set during run) */ cbm_gbuf_t *gbuf; cbm_registry_t *registry; @@ -389,6 +396,10 @@ void cbm_pipeline_get_resource_violation(const cbm_pipeline_t *p, } } +const char *cbm_pipeline_export_error(const cbm_pipeline_t *p) { + return p ? p->export_error : ""; +} + bool cbm_pipeline_set_project_name(cbm_pipeline_t *p, const char *name) { if (!p || !name || !name[0]) { return false; @@ -3162,6 +3173,12 @@ static int export_after_publish(cbm_pipeline_t *p, const char *final_path) { if (rc != 0) { const char *err = cbm_artifact_export_last_error(); cbm_log_error("pipeline.err", "phase", "artifact_export", "err", err ? err : "unknown"); + /* #1665: snapshot the error of THIS run so the MCP layer can + * attribute the failure truthfully, instead of re-reading the + * process-global export error (which may describe a previous run) + * and instead of the generic "Pipeline failed" hint that blames + * repo_path for a write-permission failure. */ + (void)snprintf(p->export_error, sizeof(p->export_error), "%s", err ? err : "unknown"); } return rc; } @@ -3397,6 +3414,7 @@ int cbm_pipeline_run(cbm_pipeline_t *p) { if (!p) { return CBM_NOT_FOUND; } + p->export_error[0] = '\0'; char *final_path = resolve_db_path(p); if (!final_path || !ensure_db_parent(final_path)) { free(final_path); diff --git a/src/pipeline/pipeline.h b/src/pipeline/pipeline.h index e5a672cb0b..c0fddc7e77 100644 --- a/src/pipeline/pipeline.h +++ b/src/pipeline/pipeline.h @@ -60,6 +60,13 @@ void cbm_pipeline_set_resource_policy(cbm_pipeline_t *p, const cbm_index_resourc void cbm_pipeline_get_resource_violation(const cbm_pipeline_t *p, cbm_index_resource_violation_t *violation); +/* Snapshot of the artifact export failure of the last cbm_pipeline_run, or "" + * when the run succeeded / did not reach post-publish export. Used to + * truthfully attribute a failed run to the persistence export (#1665) instead + * of the generic pipeline-error hint. Valid until the next cbm_pipeline_run or + * cbm_pipeline_free(). Returns "" for NULL p. */ +const char *cbm_pipeline_export_error(const cbm_pipeline_t *p); + /* Free a pipeline and all its internal state. NULL-safe. */ void cbm_pipeline_free(cbm_pipeline_t *p); diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 798c6556ad..50840af3a4 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -477,6 +477,71 @@ TEST(pipeline_structure_nodes) { * ADR before the delete and restores it after the rebuild. Reproduce-first: * index, store an ADR, force a full re-index by adding files, assert the ADR * is still present and unchanged. */ +/* #1665: a persistence artifact export failure must not be reported as a + * repo_path problem. Create the artifact directory as a regular FILE so + * cbm_mkdir_p(.codebase-memory) fails deterministically (works as root too), + * run with persistence enabled, and assert: the run fails, the DB was + * published (publish happens before export), the pipeline snapshots the export + * error, and a clean re-run leaves the snapshot empty. */ +TEST(pipeline_export_error_snapshot_on_artifact_failure) { + char tmp[256]; + snprintf(tmp, sizeof(tmp), "/tmp/cbm_export1665_XXXXXX"); + if (!cbm_mkdtemp(tmp)) { + FAIL("failed to create temp dir"); + } + + char path[512]; + snprintf(path, sizeof(path), "%s/main.py", tmp); + FILE *f = fopen(path, "w"); + ASSERT_NOT_NULL(f); + fprintf(f, "def foo():\n pass\n"); + fclose(f); + + char db_path[512]; + snprintf(db_path, sizeof(db_path), "%s/test.db", tmp); + + /* Block the artifact directory: a FILE at .codebase-memory makes the + * export's mkdir_p fail with a deterministic error on every platform. */ + char art_block[512]; + snprintf(art_block, sizeof(art_block), "%s/.codebase-memory", tmp); + f = fopen(art_block, "w"); + ASSERT_NOT_NULL(f); + fprintf(f, "block\n"); + fclose(f); + + cbm_pipeline_t *p = cbm_pipeline_new(tmp, db_path, CBM_MODE_FULL); + ASSERT_NOT_NULL(p); + cbm_pipeline_set_persistence(p, true); + + int rc = cbm_pipeline_run(p); + ASSERT_NEQ(rc, 0); + + /* The export runs after publish, so the DB must exist despite the failure. */ + struct stat db_st; + ASSERT_EQ(stat(db_path, &db_st), 0); + + const char *export_error = cbm_pipeline_export_error(p); + ASSERT_NOT_NULL(export_error); + ASSERT_TRUE(export_error[0] != '\0'); + ASSERT_NOT_NULL(strstr(export_error, "prepare_artifact_dir")); + cbm_pipeline_free(p); + + /* Control: remove the blocker; a fresh run succeeds and leaves no snapshot. */ + (void)cbm_unlink(art_block); + cbm_pipeline_t *p2 = cbm_pipeline_new(tmp, db_path, CBM_MODE_FULL); + ASSERT_NOT_NULL(p2); + cbm_pipeline_set_persistence(p2, true); + rc = cbm_pipeline_run(p2); + ASSERT_EQ(rc, 0); + const char *export_error2 = cbm_pipeline_export_error(p2); + ASSERT_NOT_NULL(export_error2); + ASSERT_TRUE(export_error2[0] == '\0'); + cbm_pipeline_free(p2); + + rm_rf(tmp); + PASS(); +} + TEST(pipeline_adr_survives_full_reindex) { char tmp[256]; snprintf(tmp, sizeof(tmp), "/tmp/cbm_adr_XXXXXX"); @@ -15151,6 +15216,7 @@ SUITE(pipeline) { RUN_TEST(pipeline_structure_nodes); RUN_TEST(pipeline_committed_counts_match_persisted); RUN_TEST(pipeline_adr_survives_full_reindex); + RUN_TEST(pipeline_export_error_snapshot_on_artifact_failure); RUN_TEST(pipeline_structure_edges); RUN_TEST(pipeline_branch_root_structure); RUN_TEST(pipeline_project_name_derived);