diff --git a/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/design.md b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/design.md new file mode 100644 index 000000000..bd32082dd --- /dev/null +++ b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/design.md @@ -0,0 +1,45 @@ +## Context + +See proposal.md. Verified state: + +- `export_after_publish` (src/pipeline/pipeline.c:2492) runs AFTER the DB publish/rename (pipeline.c:2602-2613): the graph DB is already live at `final_path` when the artifact export runs. On failure it logs `pipeline.err` (phase `artifact_export`, err = `cbm_artifact_export_last_error()`) and returns the non-zero rc, which `cbm_pipeline_run` propagates (pipeline.c:2615-2618). +- `handle_index_repository` (src/mcp/mcp.c:8335-8351): `rc != 0` ⇒ `status: "error"` + the fixed hint "Pipeline failed. Check repo_path exists and contains source files. Try mode='fast'..." — written regardless of where in the pipeline the failure occurred. +- The fail-hard policy is an in-tree, cited decision ("A failed persistence export intentionally fails the run; this used to be ignored.", pipeline.c; commit 44d51745 "fix: address review feedback on artifact export error surfacing"). This change does NOT touch that policy. +- `cbm_artifact_export_last_error()` (src/pipeline/artifact.c:67) is global state — it can carry a PREVIOUS run's error, so the response must not consult it directly; the pipeline must snapshot the error of the run it just executed. + +## Goals / Non-Goals + +**Goals:** +- When a run fails post-publish specifically at the artifact export, the `index_repository` error hint names the real cause (artifact_export, the directory, the remedy: writable checkout or `--persistence false`), and notes the index was published. +- All other failures keep the existing hint byte-for-byte. +- The error surfaced is from the current run (snapshot on the pipeline), never stale global state. + +**Non-Goals:** +- Changing the fail-hard semantics (non-zero rc on export failure stays). +- Changing the success path, `artifact_present`, or any non-error surface. +- Moving the artifact target elsewhere (a cache-dir fallback would be a maintainer-level design decision; not taken). + +## Decisions + +**D1 — Snapshot on the pipeline struct.** +`cbm_pipeline_t` (defined in pipeline_internal.h) gains `char export_error[CBM_SZ_1K]`, zeroed at `cbm_pipeline_run` start; `export_after_publish` writes `cbm_artifact_export_last_error()` (or "unknown") into it on failure. Accessor `cbm_pipeline_export_error()` declared in pipeline.h returns NULL-safe `export_error`. Rationale: the MCP handler already holds `p`; no global-state races (pipeline runs happen in workers or in-process). + +**D2 — MCP hint selection by rc + snapshot only.** +In `handle_index_repository`'s error branch: if `cbm_pipeline_export_error(p)` is non-empty AND non-zero rc, emit the truthful artifact hint via the pipeline's `pipeline.err` phase knowledge (the snapshot is only ever set by `export_after_publish`, so its presence identifies the phase exactly). Otherwise the existing generic hint is unchanged, preserving the spec's "genuine pipeline failure" scenario. + +**D3 — Test at the pipeline+store level, not full MCP.** +A full read-only-repo `index_repository` run in tests is heavy and platform-dependent (root can write anywhere; EROFS needs a real read-only mount). Instead the regression test drives `cbm_pipeline_run` with `persistence` enabled against a fixture repo whose artifact directory is replaced by a read-only directory (`chmod 0555` on the `.codebase-memory` parent — works for non-root; skipped politely when running as root, where it is not meaningful). Asserts: `rc != 0`, `cbm_pipeline_export_error(p)` non-empty and mentioning the artifact directory, and the pipeline error log contains `pipeline.err`. + +## Risks / Trade-offs + +- [chmod-based read-only fixture flaky under root/CI] → Test skips when `geteuid() == 0` (root bypasses permission checks); CI runs the suite as non-root in this repo's matrix. +- [Hint wording churn] → The truthful-hint string is new; existing tests asserting the exact generic hint on non-export failures keep passing because the snapshot is empty there (spec scenario 2 pins this). +- [Adding a field to `cbm_pipeline_t`] → Internal struct, single definition; accessor keeps the public header minimal. + +## Migration Plan + +No data/config migration. Behavior: only the error message of an already-failing run becomes accurate. Rollback: revert. + +## Open Questions + +None. diff --git a/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/proposal.md b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/proposal.md new file mode 100644 index 000000000..cec9cc76d --- /dev/null +++ b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/proposal.md @@ -0,0 +1,24 @@ +## Why + +`index_repository` on a read-only repository with `--persistence true` fails after a successful publish. The graph database was published and is usable; only the team-sharing artifact export (`.codebase-memory/graph.db.zst` written inside the repo) fails with EROFS. The run then reports `status: "error"` with a generic hint — "Pipeline failed. Check repo_path exists and contains source files" — which blames `repo_path` for a cause (read-only repo) that the hint never names, and hides the fact that the index itself succeeded. + +## What Changes + +- The artifact export failure is captured on the pipeline and surfaced in the `index_repository` error hint, naming `artifact_export`, the artifact path, and the remedy (writable checkout or `--persistence false`), instead of the generic repo_path blame. +- Fail-hard semantics are **preserved**: a failed persistence export still returns a non-zero run result, exactly per the in-tree decision ("A failed persistence export intentionally fails the run; this used to be ignored", pipeline.c). Only the error message becomes truthful and actionable. +- The pipeline records its own artifact-export error snapshot so a hint is never built from a stale global error left over from a previous run. + +## Capabilities + +### New Capabilities +- `index-artifact-export-error-surfacing`: the `index_repository` error response truthfully attributes a post-publish persistence failure to the artifact export. + +### Modified Capabilities +- None (no existing specs). + +## Impact + +- `src/pipeline/pipeline.c` — capture the artifact export error in `export_after_publish`; expose a query accessor. +- `src/pipeline/pipeline.h` — declare the accessor. +- `src/mcp/mcp.c` — error branch of `handle_index_repository` builds a truthful hint when the failure is the artifact export. +- `tests/test_pipeline.c` or `tests/test_artifact.c` — regression test: persistence export failure into an unwritable artifact directory returns non-zero, names the artifact export, and does not blame repo_path. diff --git a/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/specs/index-artifact-export-error-surfacing/spec.md b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/specs/index-artifact-export-error-surfacing/spec.md new file mode 100644 index 000000000..9198435ae --- /dev/null +++ b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/specs/index-artifact-export-error-surfacing/spec.md @@ -0,0 +1,22 @@ +## Purpose +Defines how `index_repository` reports a post-publish persistence artifact export failure: the run result stays failed (the in-tree policy is deliberate), but the error response names the actual cause — the artifact export into the repo — instead of blaming `repo_path`. + +## ADDED Requirements + +### Requirement: Failure attribution names the artifact export +When `index_repository` returns a non-zero result and the failure was the post-publish artifact export (`export_after_publish`), the response SHALL carry a hint that states the persistence artifact export failed, names `.codebase-memory/` (and the export error detail when available), and states that the index database itself was published — the remedy being a writable checkout or running without `--persistence`. + +#### Scenario: persistence export fails on a read-only repo +- **WHEN** `index_repository` runs with `--persistence true` on a read-only repository and the graph database publishes successfully while the artifact export fails +- **THEN** the returned result has `status: "error"` and its hint names the artifact export failure (with the artifact directory and the export error), does not claim the repository path is missing or source-less, and does not recommend a mode change as the remedy + +#### Scenario: a genuine pipeline failure keeps the existing hint +- **WHEN** `index_repository` fails before publish (e.g. the repository path does not exist or is empty) +- **THEN** the hint is the existing generic message ("Pipeline failed. Check repo_path exists and contains source files"), unchanged, and no artifact-export wording is shown + +### Requirement: The surfaced error is from the current run only +The pipeline SHALL capture the artifact export error of the run it just executed and expose it to the response builder; the response SHALL NOT be built from obsolete global error state belonging to a previous run. + +#### Scenario: previous run failed, current run fails earlier +- **WHEN** a run fails after publish due to artifact export, and a later run fails before export (or never requests one) +- **THEN** the later response does not mention the earlier run's artifact export error in its hint diff --git a/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/tasks.md b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/tasks.md new file mode 100644 index 000000000..d4f2eb6be --- /dev/null +++ b/openspec/changes/archive/2026-08-21-oss-codebase-memory-mcp-issue-1665/tasks.md @@ -0,0 +1,20 @@ +## 1. Pipeline snapshot of the artifact export error + +- [ ] 1.1 Add `char export_error[CBM_SZ_1K]` to `cbm_pipeline_t` (pipeline_internal.h), zero it at the start of `cbm_pipeline_run`, and in `export_after_publish` copy `cbm_artifact_export_last_error()` (fallback "unknown") into it on failure +- [ ] 1.2 Add `const char *cbm_pipeline_export_error(const cbm_pipeline_t *p);` to pipeline.h and implement it (NULL-safe) in pipeline.c +- [ ] 1.3 Verify: `make -f Makefile.cbm test-focused TEST_SUITES=pipeline` compiles and passes + +## 2. Truthful MCP error hint + +- [ ] 2.1 In `handle_index_repository`'s error branch (src/mcp/mcp.c), when `rc != 0` and `cbm_pipeline_export_error(p)` is non-empty, emit a hint naming the artifact export failure (`.codebase-memory/` directory, the export error detail when available), state the index database was published, and name the remedies (writable checkout or `--persistence false`); keep the generic hint verbatim in all other failure cases + +## 3. Regression test + +- [ ] 3.1 Add a test in tests/test_pipeline.c: build a pipeline with `persistence` enabled over a fixture repo, replace the artifact directory with a chmod 0555 directory (skip when `geteuid() == 0`), run `cbm_pipeline_run`, assert `rc != 0`, `cbm_pipeline_export_error(p)` non-empty, and the exported error mentions the artifact path +- [ ] 3.2 Verify: `make -f Makefile.cbm test-focused TEST_SUITES=pipeline` passes with the new test + +## 4. Final validation + +- [ ] 4.1 Run `make -f Makefile.cbm test` (full suite) and `make -f Makefile.cbm lint-ci` — pass +- [ ] 4.2 `OPENSPEC_NO_UPDATE_CHECK=1 openspec validate oss-codebase-memory-mcp-issue-1665 --json` — clean +- [ ] 4.3 Archive: `OPENSPEC_NO_UPDATE_CHECK=1 openspec archive oss-codebase-memory-mcp-issue-1665 --yes` diff --git a/openspec/specs/index-artifact-export-error-surfacing/spec.md b/openspec/specs/index-artifact-export-error-surfacing/spec.md new file mode 100644 index 000000000..d4ab2bbaa --- /dev/null +++ b/openspec/specs/index-artifact-export-error-surfacing/spec.md @@ -0,0 +1,24 @@ +# index-artifact-export-error-surfacing Specification + +## Purpose +Defines how `index_repository` reports a post-publish persistence artifact export failure: the run result stays failed (the in-tree policy is deliberate), but the error response names the actual cause — the artifact export into the repo — instead of blaming `repo_path`. + +## Requirements + +### Requirement: Failure attribution names the artifact export +When `index_repository` returns a non-zero result and the failure was the post-publish artifact export (`export_after_publish`), the response SHALL carry a hint that states the persistence artifact export failed, names `.codebase-memory/` (and the export error detail when available), and states that the index database itself was published — the remedy being a writable checkout or running without `--persistence`. + +#### Scenario: persistence export fails on a read-only repo +- **WHEN** `index_repository` runs with `--persistence true` on a read-only repository and the graph database publishes successfully while the artifact export fails +- **THEN** the returned result has `status: "error"` and its hint names the artifact export failure (with the artifact directory and the export error), does not claim the repository path is missing or source-less, and does not recommend a mode change as the remedy + +#### Scenario: a genuine pipeline failure keeps the existing hint +- **WHEN** `index_repository` fails before publish (e.g. the repository path does not exist or is empty) +- **THEN** the hint is the existing generic message ("Pipeline failed. Check repo_path exists and contains source files"), unchanged, and no artifact-export wording is shown + +### Requirement: The surfaced error is from the current run only +The pipeline SHALL capture the artifact export error of the run it just executed and expose it to the response builder; the response SHALL NOT be built from obsolete global error state belonging to a previous run. + +#### Scenario: previous run failed, current run fails earlier +- **WHEN** a run fails after publish due to artifact export, and a later run fails before export (or never requests one) +- **THEN** the later response does not mention the earlier run's artifact export error in its hint diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 04316627b..14b57cfc9 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -8345,9 +8345,25 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { yyjson_mut_obj_add_str(doc, root, "status", degraded ? "degraded" : "indexed"); } 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 65ac75183..ea42deab9 100644 --- a/src/pipeline/pipeline.c +++ b/src/pipeline/pipeline.c @@ -157,6 +157,13 @@ struct cbm_pipeline { atomic_int *cancelled; bool persistence; /* write .codebase-memory/graph.db.zst after indexing */ + /* 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; @@ -304,6 +311,10 @@ void cbm_pipeline_set_persistence(cbm_pipeline_t *p, bool enabled) { } } +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; @@ -2497,6 +2508,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; } @@ -2510,6 +2527,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 df684ede5..44f623b81 100644 --- a/src/pipeline/pipeline.h +++ b/src/pipeline/pipeline.h @@ -53,6 +53,13 @@ cbm_pipeline_t *cbm_pipeline_new(const char *repo_path, const char *db_path, cbm * When enabled, the pipeline writes a compressed artifact after indexing. */ void cbm_pipeline_set_persistence(cbm_pipeline_t *p, bool enabled); +/* 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 fb33bcc72..c00131749 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -288,6 +288,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"); @@ -12094,6 +12159,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);