From 99c3bb4cbe4c0f9542bf722d29cbb5dd01397455 Mon Sep 17 00:00:00 2001 From: kavish-19 <63698788+kavish-19@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:54:51 +0530 Subject: [PATCH 01/12] fix(store): report a failed COUNT read instead of returning zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cbm_store_count_nodes and cbm_store_count_edges treat every sqlite3_step result other than SQLITE_ROW as a count of zero. A read that failed — SQLITE_CORRUPT, SQLITE_BUSY, SQLITE_IOERR — is therefore indistinguishable from a project that genuinely holds no rows, and index_status renders it as the positive assertion status "empty". A user or agent reading that concludes the repository was never indexed and starts a multi-minute re-index, while the corruption itself is never surfaced. Both functions already have an error channel: each returns CBM_STORE_ERR when prepare_cached fails. Only the step result was not reported through it. index_status is already written for that value — it sets degraded on a negative node count and clamps a negative edge count — so the guard existed and simply never fired. Initialise count to CBM_STORE_ERR so a non-row step is reported as a failed read. A successful step still overwrites it with the real count, so the healthy path is unchanged. Closes #2012 Signed-off-by: kavish-19 <63698788+kavish-19@users.noreply.github.com> --- src/store/store.c | 11 +++++++++-- tests/test_store_nodes.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/store/store.c b/src/store/store.c index c36f00b444..d2369c1f9f 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -3069,7 +3069,11 @@ int cbm_store_count_nodes(cbm_store_t *s, const char *project) { } bind_text(stmt, SKIP_ONE, project); - int count = 0; + /* A step that does not yield a row is a failed read (SQLITE_CORRUPT, + * SQLITE_BUSY, SQLITE_IOERR), not a count of zero. Report it through the + * error channel this function already uses for a failed prepare, so a + * caller cannot mistake an unreadable table for an empty project. */ + int count = CBM_STORE_ERR; if (sqlite3_step(stmt) == SQLITE_ROW) { count = sqlite3_column_int(stmt, 0); } @@ -3389,7 +3393,10 @@ int cbm_store_count_edges(cbm_store_t *s, const char *project) { } bind_text(stmt, SKIP_ONE, project); - int count = 0; + /* See cbm_store_count_nodes: a non-row step is a failed read, not zero. + * index_status reads this alongside the node count and already treats a + * negative value as degraded. */ + int count = CBM_STORE_ERR; if (sqlite3_step(stmt) == SQLITE_ROW) { count = sqlite3_column_int(stmt, 0); } diff --git a/tests/test_store_nodes.c b/tests/test_store_nodes.c index cf7b8bc8fd..c08beadada 100644 --- a/tests/test_store_nodes.c +++ b/tests/test_store_nodes.c @@ -2030,6 +2030,37 @@ TEST(store_count_nodes_unknown_project) { PASS(); } +/* A COUNT(*) that cannot be read must not be reported as a count of zero: + * index_status renders that as the positive assertion status "empty", so a + * corrupt project looks like one that was never indexed. Dropping the table + * after the statement is cached makes the step fail deterministically. */ +TEST(store_count_failed_read_is_not_zero) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + cbm_node_t n = {.project = "test", + .label = "File", + .name = "main.c", + .qualified_name = "test.main.c", + .file_path = "main.c"}; + cbm_store_upsert_node(s, &n); + + /* Sanity: a readable table still counts normally. */ + ASSERT_EQ(cbm_store_count_nodes(s, "test"), 1); + ASSERT_TRUE(cbm_store_count_edges(s, "test") >= 0); + + /* Make the read fail. The statements are cached by the calls above, so the + * step (not the prepare) is what fails once the tables are gone. */ + ASSERT_EQ(cbm_store_exec(s, "DROP TABLE nodes;"), 0); + ASSERT_EQ(cbm_store_exec(s, "DROP TABLE edges;"), 0); + + ASSERT_TRUE(cbm_store_count_nodes(s, "test") < 0); + ASSERT_TRUE(cbm_store_count_edges(s, "test") < 0); + + cbm_store_close(s); + PASS(); +} + /* ── Index coverage (#963) ──────────────────────────────────────── */ /* Round-trip + deleted-file prune + shadow miss-graph materialization + @@ -2432,4 +2463,5 @@ SUITE(store_nodes) { RUN_TEST(store_node_properties_special_chars); RUN_TEST(store_delete_nodes_nonexistent); RUN_TEST(store_count_nodes_unknown_project); + RUN_TEST(store_count_failed_read_is_not_zero); } From 73c509f4246b0f97ca772b6d9a3159181a7fa6b6 Mon Sep 17 00:00:00 2001 From: kavish-19 <63698788+kavish-19@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:54:51 +0530 Subject: [PATCH 02/12] fix(mcp): report an unreadable count as an error, not an empty project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The store change alone does not close #2012. handle_index_status reads the counts directly and renders status as `nodes > 0 ? "ready" : "empty"`, so a negative count from a failed read still answered "empty" — now with a bare -1 printed as the count, and with no hint at all, because the "Project is empty" branch is guarded on `nodes == 0`. The guard cited earlier lives in build_index_success_response, which is index_repository's response builder, not this path. Treat a negative node or edge count as a failed read: report status "error", suppress the negative numbers rather than emitting them as counts, and replace the re-index hint with one that names the table that could not be read. Closes #2012 Signed-off-by: kavish-19 <63698788+kavish-19@users.noreply.github.com> --- src/mcp/mcp.c | 28 +++++++++++++++++++---- tests/test_mcp.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 3259221c47..959a076845 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -6668,10 +6668,17 @@ static char *handle_index_status(cbm_mcp_server_t *srv, const char *args) { if (project) { int nodes = cbm_store_count_nodes(store, project); int edges = cbm_store_count_edges(store, project); + /* A negative count is a failed read (CBM_STORE_ERR), not a small + * project. Reporting it as a count would put a bare -1 in the output, + * and `nodes > 0 ? "ready" : "empty"` would answer "empty" for an + * unreadable table — the false all-clear #2012 is about. Say the read + * failed, and name the table that could not be read. */ + const bool counts_unreadable = nodes < 0 || edges < 0; yyjson_mut_obj_add_str(doc, root, "project", project); - yyjson_mut_obj_add_int(doc, root, "nodes", nodes); - yyjson_mut_obj_add_int(doc, root, "edges", edges); - yyjson_mut_obj_add_str(doc, root, "status", nodes > 0 ? "ready" : "empty"); + yyjson_mut_obj_add_int(doc, root, "nodes", counts_unreadable ? 0 : nodes); + yyjson_mut_obj_add_int(doc, root, "edges", counts_unreadable ? 0 : edges); + yyjson_mut_obj_add_str(doc, root, "status", + counts_unreadable ? "error" : (nodes > 0 ? "ready" : "empty")); cbm_project_t proj_info = {0}; bool have_proj_info = cbm_store_get_project(store, project, &proj_info) == CBM_STORE_OK; if (have_proj_info) { @@ -6688,7 +6695,20 @@ static char *handle_index_status(cbm_mcp_server_t *srv, const char *args) { safe_str_free(&proj_info.name); safe_str_free(&proj_info.indexed_at); safe_str_free(&proj_info.root_path); - if (nodes == 0) { + if (counts_unreadable) { + yyjson_mut_obj_add_str( + doc, root, "hint", + nodes < 0 && edges < 0 + ? "The nodes and edges tables could not be read; the database may be " + "corrupt. Re-run index_repository(repo_path=...) or remove the project " + "cache and re-index." + : (nodes < 0 ? "The nodes table could not be read; the database may be " + "corrupt. Re-run index_repository(repo_path=...) or remove " + "the project cache and re-index." + : "The edges table could not be read; the database may be " + "corrupt. Re-run index_repository(repo_path=...) or remove " + "the project cache and re-index.")); + } else if (nodes == 0) { yyjson_mut_obj_add_str( doc, root, "hint", "Project is empty. Re-run index_repository(repo_path=...) to populate."); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index f97eda09e8..905213cfc2 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -3384,6 +3384,63 @@ TEST(tool_index_status_keeps_authoritative_ignored_total_when_rows_are_sampled) PASS(); } +/* #2012: a COUNT(*) that cannot be read must not be reported as an empty + * project. The store now returns CBM_STORE_ERR for a failed step, and + * index_status has to say the read failed rather than answering "empty" with + * a bare -1 as the count. Dropping the tables after the first call makes the + * step (not the prepare) fail deterministically, because the statements are + * cached by then. */ +TEST(tool_index_status_reports_an_unreadable_count_as_an_error) { + char tmp[256]; + cbm_mcp_server_t *srv = setup_snippet_server(tmp, sizeof(tmp)); + ASSERT_NOT_NULL(srv); + cbm_store_t *store = cbm_mcp_server_store(srv); + ASSERT_NOT_NULL(store); + + /* First call: a healthy project, and it caches the count statements. */ + char *response = cbm_mcp_handle_tool(srv, "index_status", + "{\"project\":\"test-project\",\"format\":\"json\"}"); + char *inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + yyjson_doc *doc = yyjson_read(inner, strlen(inner), 0); + ASSERT_NOT_NULL(doc); + ASSERT_STR_EQ(yyjson_get_str(yyjson_obj_get(yyjson_doc_get_root(doc), "status")), "ready"); + yyjson_doc_free(doc); + free(inner); + free(response); + + ASSERT_EQ(cbm_store_exec(store, "DROP TABLE nodes;"), 0); + ASSERT_EQ(cbm_store_exec(store, "DROP TABLE edges;"), 0); + + response = cbm_mcp_handle_tool(srv, "index_status", + "{\"project\":\"test-project\",\"format\":\"json\"}"); + inner = extract_text_content(response); + ASSERT_NOT_NULL(inner); + doc = yyjson_read(inner, strlen(inner), 0); + ASSERT_NOT_NULL(doc); + yyjson_val *root = yyjson_doc_get_root(doc); + + /* Not "empty": that is the false all-clear the issue is about. */ + ASSERT_STR_EQ(yyjson_get_str(yyjson_obj_get(root, "status")), "error"); + + /* A failed read is not a count, so no negative number reaches the caller. */ + ASSERT_TRUE(yyjson_get_int(yyjson_obj_get(root, "nodes")) >= 0); + ASSERT_TRUE(yyjson_get_int(yyjson_obj_get(root, "edges")) >= 0); + + /* The hint names the unreadable table instead of telling the user to + * re-index an "empty" project. */ + const char *hint = yyjson_get_str(yyjson_obj_get(root, "hint")); + ASSERT_NOT_NULL(hint); + ASSERT_TRUE(strstr(hint, "could not be read") != NULL); + ASSERT_TRUE(strstr(hint, "Project is empty") == NULL); + + yyjson_doc_free(doc); + free(inner); + free(response); + cbm_mcp_server_free(srv); + PASS(); +} + TEST(tool_output_byte_budgets) { /* GUARD: absolute byte ceilings on default tool outputs. Re-bloat (e.g. * a property blob sneaking back into row emission — the fp field alone @@ -19827,6 +19884,7 @@ SUITE(mcp) { RUN_TEST(tool_search_graph_toon_never_leaks_internal_fields); RUN_TEST(tool_lean_defaults_schema_and_status); RUN_TEST(tool_index_status_keeps_authoritative_ignored_total_when_rows_are_sampled); + RUN_TEST(tool_index_status_reports_an_unreadable_count_as_an_error); RUN_TEST(tool_output_regression_gate); RUN_TEST(tool_output_byte_budgets); RUN_TEST(tool_search_graph_query_honors_file_pattern_issue552); From 60449f895dbaedb4e74c4f155a8e214d78486e12 Mon Sep 17 00:00:00 2001 From: kavish-19 <63698788+kavish-19@users.noreply.github.com> Date: Sun, 6 Sep 2026 10:00:57 +0530 Subject: [PATCH 03/12] style(mcp): pick the unreadable-count hint with an if/else chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cppcheck 2.20 flags the nested ternary in handle_index_status: src/mcp/mcp.c:6705:30: style: Condition 'nodes<0' is always false [knownConditionTrueFalse] After the `nodes < 0 && edges < 0` arm, cppcheck's value flow takes `nodes < 0` to be the false half of that conjunction and rejects the second test. Select the hint with a plain if / else-if / else chain into a `const char *hint` and add it once, rather than suppressing the check. The three messages are byte-identical to before — only the line wrapping of the adjacent literals moved — so status, counts and hint text are unchanged and the tests added in the previous commit still describe the behaviour. Verified with cppcheck 2.21 locally using the flags from Makefile.cbm's lint-cppcheck target: the finding is reproduced on the parent commit and gone here, with no new ones. clang-format reports no drift over the changed lines. Signed-off-by: kavish-19 <63698788+kavish-19@users.noreply.github.com> --- src/mcp/mcp.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 959a076845..005e25b2c4 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -6696,18 +6696,19 @@ static char *handle_index_status(cbm_mcp_server_t *srv, const char *args) { safe_str_free(&proj_info.indexed_at); safe_str_free(&proj_info.root_path); if (counts_unreadable) { - yyjson_mut_obj_add_str( - doc, root, "hint", - nodes < 0 && edges < 0 - ? "The nodes and edges tables could not be read; the database may be " - "corrupt. Re-run index_repository(repo_path=...) or remove the project " - "cache and re-index." - : (nodes < 0 ? "The nodes table could not be read; the database may be " - "corrupt. Re-run index_repository(repo_path=...) or remove " - "the project cache and re-index." - : "The edges table could not be read; the database may be " - "corrupt. Re-run index_repository(repo_path=...) or remove " - "the project cache and re-index.")); + const char *hint; + if (nodes < 0 && edges < 0) { + hint = "The nodes and edges tables could not be read; the database may be " + "corrupt. Re-run index_repository(repo_path=...) or remove the project " + "cache and re-index."; + } else if (nodes < 0) { + hint = "The nodes table could not be read; the database may be corrupt. Re-run " + "index_repository(repo_path=...) or remove the project cache and re-index."; + } else { + hint = "The edges table could not be read; the database may be corrupt. Re-run " + "index_repository(repo_path=...) or remove the project cache and re-index."; + } + yyjson_mut_obj_add_str(doc, root, "hint", hint); } else if (nodes == 0) { yyjson_mut_obj_add_str( doc, root, "hint", From 8f3c15044b8200a15ee1895c5127f4bf117da493 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Mon, 7 Sep 2026 11:49:28 +0000 Subject: [PATCH 04/12] fix(cli): emit the OpenCode plugin as a V2 default export OpenCode's V2 loader validates only the module's default export, expecting {id, setup()|effect()}. cbm_client_adapter_opencode still emitted the V1 named export (export const CodebaseMemory = ...), so every server start failed with SchemaError: Missing key at ["default"]. Wrap the existing hook logic in a default export with an id and a setup(ctx) function; the returned hooks object is unchanged. Fixes #2077 Signed-off-by: Amir Fathi --- src/cli/client_adapter.c | 105 +++++++++++++++++++------------------ tests/test_agent_clients.c | 14 +++++ 2 files changed, 68 insertions(+), 51 deletions(-) diff --git a/src/cli/client_adapter.c b/src/cli/client_adapter.c index dbd9134268..ed77bd67aa 100644 --- a/src/cli/client_adapter.c +++ b/src/cli/client_adapter.c @@ -300,57 +300,60 @@ char *cbm_client_adapter_opencode(const char *binary_path) { " });\n" "}\n\n"); - sb_append(&sb, - "export const CodebaseMemory = async (ctx) => {\n" - " const dir = ctx?.directory;\n" - " const seen = new Set();\n" - " const lifecycle = () =>\n" - " augment({ hook_event_name: 'SessionStart', cwd: dir });\n" - " return {\n" - " 'tool.execute.after': async (input, output) => {\n" - " if (typeof output?.output !== 'string') return;\n" - " const pieces = [];\n" - " const sid = input?.sessionID;\n" - " if (typeof sid === 'string' && !seen.has(sid)) {\n" - " seen.add(sid);\n" - " pieces.push(await lifecycle());\n" - " }\n" - " const args = input?.args ?? {};\n" - " const search =\n" - " input?.tool === 'grep' ? 'Grep' : input?.tool === 'glob' ? 'Glob' : null;\n" - " if (search) {\n" - " pieces.push(await augment({\n" - " hook_event_name: 'PreToolUse',\n" - " tool_name: search,\n" - " tool_input: args,\n" - " cwd: dir,\n" - " }));\n" - " } else if (input?.tool === 'read') {\n" - " const filePath = args.filePath ?? args.file_path ?? args.path;\n" - " if (typeof filePath === 'string' && filePath) {\n" - " pieces.push(await augment({\n" - " hook_event_name: 'PostToolUse',\n" - " tool_name: 'Read',\n" - " tool_input: { file_path: filePath },\n" - " cwd: dir,\n" - " }));\n" - " }\n" - " }\n" - " const extra = pieces.filter(Boolean).join('\\n');\n" - " if (extra) {\n" - " output.output += '\\n' + extra;\n" - " }\n" - " },\n" - " // Documented (experimental) compaction surface: output.context is the\n" - " // mutable array of context strings for the rebuilt session.\n" - " 'experimental.session.compacting': async (_input, output) => {\n" - " const note = await lifecycle();\n" - " if (note && Array.isArray(output?.context)) {\n" - " output.context.push(note);\n" - " }\n" - " },\n" - " };\n" - "};\n"); + sb_append( + &sb, "export default {\n" + " id: 'codebase-memory-augment',\n" + " async setup(ctx) {\n" + " const dir = ctx?.directory;\n" + " const seen = new Set();\n" + " const lifecycle = () =>\n" + " augment({ hook_event_name: 'SessionStart', cwd: dir });\n" + " return {\n" + " 'tool.execute.after': async (input, output) => {\n" + " if (typeof output?.output !== 'string') return;\n" + " const pieces = [];\n" + " const sid = input?.sessionID;\n" + " if (typeof sid === 'string' && !seen.has(sid)) {\n" + " seen.add(sid);\n" + " pieces.push(await lifecycle());\n" + " }\n" + " const args = input?.args ?? {};\n" + " const search =\n" + " input?.tool === 'grep' ? 'Grep' : input?.tool === 'glob' ? 'Glob' : null;\n" + " if (search) {\n" + " pieces.push(await augment({\n" + " hook_event_name: 'PreToolUse',\n" + " tool_name: search,\n" + " tool_input: args,\n" + " cwd: dir,\n" + " }));\n" + " } else if (input?.tool === 'read') {\n" + " const filePath = args.filePath ?? args.file_path ?? args.path;\n" + " if (typeof filePath === 'string' && filePath) {\n" + " pieces.push(await augment({\n" + " hook_event_name: 'PostToolUse',\n" + " tool_name: 'Read',\n" + " tool_input: { file_path: filePath },\n" + " cwd: dir,\n" + " }));\n" + " }\n" + " }\n" + " const extra = pieces.filter(Boolean).join('\\n');\n" + " if (extra) {\n" + " output.output += '\\n' + extra;\n" + " }\n" + " },\n" + " // Documented (experimental) compaction surface: output.context is the\n" + " // mutable array of context strings for the rebuilt session.\n" + " 'experimental.session.compacting': async (_input, output) => {\n" + " const note = await lifecycle();\n" + " if (note && Array.isArray(output?.context)) {\n" + " output.context.push(note);\n" + " }\n" + " },\n" + " };\n" + " },\n" + "};\n"); if (sb.failed) { free(sb.buf); diff --git a/tests/test_agent_clients.c b/tests/test_agent_clients.c index 59d48b3206..a1e4c40204 100644 --- a/tests/test_agent_clients.c +++ b/tests/test_agent_clients.c @@ -1316,6 +1316,19 @@ TEST(client_adapter_opencode_covers_lifecycle_read_and_compaction) { PASS(); } +/* #2077: OpenCode's V2 loader only reads the default export and needs an + * id plus a setup()/effect() function; the old named export had neither. */ +TEST(client_adapter_opencode_exports_the_v2_default_definition_issue2077) { + char *js = cbm_client_adapter_opencode("/usr/local/bin/codebase-memory-mcp"); + ASSERT_NOT_NULL(js); + ASSERT_NOT_NULL(strstr(js, "export default {")); + ASSERT_NOT_NULL(strstr(js, "id: 'codebase-memory-augment'")); + ASSERT_NOT_NULL(strstr(js, "async setup(ctx) {")); + ASSERT_NULL(strstr(js, "export const CodebaseMemory")); + free(js); + PASS(); +} + /* Empty/NULL inputs must not produce a module at all. */ TEST(client_adapter_rejects_missing_binary_path) { ASSERT_NULL(cbm_client_adapter_pi(NULL)); @@ -1362,5 +1375,6 @@ SUITE(agent_clients) { RUN_TEST(client_adapter_escapes_windows_paths_and_quotes); RUN_TEST(client_adapter_opencode_sends_the_required_hook_event); RUN_TEST(client_adapter_opencode_covers_lifecycle_read_and_compaction); + RUN_TEST(client_adapter_opencode_exports_the_v2_default_definition_issue2077); RUN_TEST(client_adapter_rejects_missing_binary_path); } From c8184ed94887a6b5e130726885b44a07c63a47b7 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Fri, 11 Sep 2026 21:49:52 +0000 Subject: [PATCH 05/12] fix(discover): honor an enclosing repo's .gitignore for a git-less subfolder resolve_git_common_dir() only stats repo_path/.git directly, so indexing a subfolder with no .git of its own never consulted any ancestor repo's .gitignore. Add resolve_enclosing_git_root() to walk up parent directories the way git itself does, and merge that ancestor's .gitignore (and info/exclude) ahead of the subfolder's own so the more specific file still wins on conflict. Fixes #510 Signed-off-by: Amir Fathi --- src/discover/discover.c | 64 ++++++++++++++++++++++++++-- tests/test_discover.c | 92 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 4 deletions(-) diff --git a/src/discover/discover.c b/src/discover/discover.c index 5cadaed75e..b90edc023e 100644 --- a/src/discover/discover.c +++ b/src/discover/discover.c @@ -1168,6 +1168,37 @@ static bool resolve_git_common_dir(const char *repo_path, char *common_dir, size return true; } +/* When repo_path itself carries no .git (resolve_git_common_dir already + * returned false for it), walk upward looking for an enclosing repository, + * exactly as `git` itself would when run from a subfolder. Bounded by the + * filesystem/drive root: dir is truncated at each iteration, so the loop + * cannot run more times than repo_path is characters long. Returns true and + * fills ancestor_root (for the enclosing repo's own .gitignore) plus + * common_dir (for its info/exclude + config, via the same resolution + * resolve_git_common_dir already applies to an ordinary repo root). Fixes + * the remaining half of issue #510: only the indexed directory's own + * .gitignore was ever consulted, never an enclosing repo's. */ +static bool resolve_enclosing_git_root(const char *repo_path, char *ancestor_root, size_t ar_sz, + char *common_dir, size_t cd_sz) { + char dir[CBM_SZ_4K]; + snprintf(dir, sizeof(dir), "%s", repo_path); + cbm_normalize_path_sep(dir); + + for (;;) { + char *slash = strrchr(dir, '/'); + /* No separator left, or only the root separator (POSIX "/") or a + * bare drive prefix (Windows "C:/"): nothing above this to check. */ + if (!slash || slash == dir || (slash > dir && *(slash - 1) == ':')) { + return false; + } + *slash = '\0'; + if (resolve_git_common_dir(dir, common_dir, cd_sz)) { + snprintf(ancestor_root, ar_sz, "%s", dir); + return true; + } + } +} + int cbm_discover(const char *repo_path, const cbm_discover_opts_t *opts, cbm_file_info_t **out, int *count) { return cbm_discover_ex(repo_path, opts, out, count, NULL, NULL); @@ -1215,11 +1246,13 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc /* Load gitignore sources for ordinary repos AND linked worktrees. * Sources merged in order (later patterns win on conflict): - * 1. /.gitignore — committed exclusions - * 2. /info/exclude — per-clone exclusions, not committed + * 1. /.gitignore: enclosing repo's root exclusions, only when + * repo_path itself has no .git of its own (see below) + * 2. /.gitignore: committed exclusions + * 3. /info/exclude: per-clone exclusions, not committed * is the git common dir, resolved via resolve_git_common_dir() so a * worktree (where .git is a gitlink file) reads the shared info/exclude/config - * just like a normal checkout. Both are folded into a single matcher so all + * just like a normal checkout. All are folded into a single matcher so all * downstream call paths remain unchanged. Fixes issue #489: OOM on repos whose * worktrees are excluded only via .git/info/exclude (e.g. Sandcastle). */ cbm_gitignore_t *gitignore = NULL; @@ -1231,11 +1264,34 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc char git_common_dir[CBM_SZ_4K]; bool is_git_repo = resolve_git_common_dir(repo_path, git_common_dir, sizeof(git_common_dir)); bool has_git_config = false; + /* repo_path itself is not a git repo root: walk upward for an enclosing one, + * exactly as `git` does when run from a subfolder. Its root .gitignore is + * loaded FIRST below (least specific), so the indexed directory's own + * .gitignore and info/exclude (both more specific) still override it on + * conflict. Fixes the remaining half of issue #510: only the indexed + * directory's own .gitignore was ever consulted, never an enclosing repo's. */ + char enclosing_root[CBM_SZ_4K]; + if (!is_git_repo && + resolve_enclosing_git_root(repo_path, enclosing_root, sizeof(enclosing_root), + git_common_dir, sizeof(git_common_dir))) { + is_git_repo = true; + char enclosing_gi_path[CBM_SZ_4K]; + path_join(enclosing_gi_path, sizeof(enclosing_gi_path), enclosing_root, ".gitignore"); + gitignore = cbm_gitignore_load(enclosing_gi_path); + } /* Always honour the .gitignore at the indexed-directory root, even when the * directory is not a git repo root (e.g. indexing a sub-package directly). * Fixes issue #510: a root .gitignore was silently ignored without .git/. */ snprintf(gi_path, sizeof(gi_path), "%s/.gitignore", repo_path); - gitignore = cbm_gitignore_load(gi_path); + cbm_gitignore_t *local_gitignore = cbm_gitignore_load(gi_path); + if (local_gitignore) { + if (!gitignore) { + gitignore = local_gitignore; + } else { + (void)cbm_gitignore_merge(gitignore, local_gitignore); + cbm_gitignore_free(local_gitignore); + } + } if (is_git_repo) { path_join(gi_path, sizeof(gi_path), git_common_dir, "config"); has_git_config = wide_stat(gi_path, &gi_stat) == 0 && S_ISREG(gi_stat.st_mode); diff --git a/tests/test_discover.c b/tests/test_discover.c index 0f3dac7d2d..ae4d9585d7 100644 --- a/tests/test_discover.c +++ b/tests/test_discover.c @@ -1355,6 +1355,93 @@ TEST(discover_worktree_committed_gitignore) { PASS(); } +/* ── Enclosing-repo .gitignore tests (issue #510, second half) ──── */ + +/* repo_path itself has no .git (indexing a git-less subfolder of a larger + * repo). The enclosing repo's root .gitignore must still be honored, exactly + * as `git status`/`git check-ignore` run from that subfolder would. Before + * this fix, resolve_git_common_dir() only ever stat'd repo_path/.git + * directly and gave up, so the enclosing repo's rules were silently never + * consulted. */ +TEST(discover_enclosing_repo_gitignore_issue510) { + char *base = th_mktempdir("cbm_disc_enc_gi"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git")); + th_write_file(TH_PATH(base, ".gitignore"), "secret.py\n"); + th_write_file(TH_PATH(base, "pkg/secret.py"), "TOKEN = 1\n"); + th_write_file(TH_PATH(base, "pkg/keep.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "pkg"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + ASSERT_EQ(count, 1); + ASSERT_TRUE(strstr(files[0].rel_path, "keep.py") != NULL); + ASSERT_FALSE(discover_has_rel_path(files, count, "secret.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + +/* The enclosing repo's /info/exclude (per-clone, uncommitted) must + * be honored the same way once the enclosing root is found, exactly as it + * already is for a repo_path that carries its own .git (issue #489). */ +TEST(discover_enclosing_repo_info_exclude) { + char *base = th_mktempdir("cbm_disc_enc_exc"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git/info")); + th_write_file(TH_PATH(base, ".git/info/exclude"), "scratch/\n"); + th_write_file(TH_PATH(base, "pkg/main.py"), "pass\n"); + th_write_file(TH_PATH(base, "pkg/scratch/tmp.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "pkg"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + ASSERT_EQ(count, 1); + ASSERT_TRUE(strstr(files[0].rel_path, "main.py") != NULL); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + +/* Precedence: the indexed directory's own .gitignore is more specific than + * the enclosing repo's root .gitignore and must still win on conflict, + * matching git's shallow-to-deep rule (a later, deeper pattern overrides an + * earlier, shallower one). Without this, folding the enclosing root in + * ahead of repo_path's own .gitignore in the wrong order would let a root + * pattern silently re-ignore a file the subfolder's own .gitignore + * un-ignores. */ +TEST(discover_enclosing_repo_gitignore_local_overrides) { + char *base = th_mktempdir("cbm_disc_enc_gi_ovr"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git")); + th_write_file(TH_PATH(base, ".gitignore"), "*.py\n"); + th_write_file(TH_PATH(base, "pkg/.gitignore"), "!keep.py\n"); + th_write_file(TH_PATH(base, "pkg/keep.py"), "pass\n"); + th_write_file(TH_PATH(base, "pkg/drop.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "pkg"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + ASSERT_EQ(count, 1); + ASSERT_TRUE(strstr(files[0].rel_path, "keep.py") != NULL); + ASSERT_FALSE(discover_has_rel_path(files, count, "drop.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + /* ── Nested .gitignore tests (issue #178) ──────────────────────── */ TEST(discover_nested_gitignore) { @@ -1971,6 +2058,11 @@ SUITE(discover) { RUN_TEST(discover_worktree_info_exclude); RUN_TEST(discover_worktree_committed_gitignore); + /* Enclosing-repo .gitignore resolution (issue #510, second half) */ + RUN_TEST(discover_enclosing_repo_gitignore_issue510); + RUN_TEST(discover_enclosing_repo_info_exclude); + RUN_TEST(discover_enclosing_repo_gitignore_local_overrides); + /* Nested .gitignore tests (issue #178) */ RUN_TEST(discover_nested_gitignore); RUN_TEST(discover_nested_gitignore_stacks_with_root); From e171bd48b5e5845ec6f00db65ff300caac4f7304 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Sun, 20 Sep 2026 09:15:32 +0000 Subject: [PATCH 06/12] fix(cli): move opencode plugin hooks under server(), leave setup() empty DeusData asked (2026-09-19T20:25:34Z on PR #2089) to place the hooks in the server entry rather than returning them from setup(), since the server runtime reads default.server and dispatches the hooks it returns, while the V2 config loader only reads id + setup()/effect() and has no tool domain yet to register against. Moved the existing hook body under server: async (ctx) => {...} and added an empty setup() {}. Updated the emission test to check for both and to assert the old async setup(ctx) { shape is gone. Signed-off-by: Amir Fathi --- src/cli/client_adapter.c | 6 +++++- tests/test_agent_clients.c | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/cli/client_adapter.c b/src/cli/client_adapter.c index ed77bd67aa..16f2b3ba0e 100644 --- a/src/cli/client_adapter.c +++ b/src/cli/client_adapter.c @@ -303,7 +303,11 @@ char *cbm_client_adapter_opencode(const char *binary_path) { sb_append( &sb, "export default {\n" " id: 'codebase-memory-augment',\n" - " async setup(ctx) {\n" + " // V2 config loader: requires id + setup|effect; no tool domain yet\n" + " // at this point, so there is nothing to register here.\n" + " setup() {},\n" + " // Server runtime: reads default.server and dispatches the hooks it returns.\n" + " server: async (ctx) => {\n" " const dir = ctx?.directory;\n" " const seen = new Set();\n" " const lifecycle = () =>\n" diff --git a/tests/test_agent_clients.c b/tests/test_agent_clients.c index a1e4c40204..1cf5cab020 100644 --- a/tests/test_agent_clients.c +++ b/tests/test_agent_clients.c @@ -1323,7 +1323,11 @@ TEST(client_adapter_opencode_exports_the_v2_default_definition_issue2077) { ASSERT_NOT_NULL(js); ASSERT_NOT_NULL(strstr(js, "export default {")); ASSERT_NOT_NULL(strstr(js, "id: 'codebase-memory-augment'")); - ASSERT_NOT_NULL(strstr(js, "async setup(ctx) {")); + /* Hooks live under server(), which the server runtime reads; setup() + * stays empty since the V2 config loader has no tool domain yet. */ + ASSERT_NOT_NULL(strstr(js, "setup() {}")); + ASSERT_NOT_NULL(strstr(js, "server: async (ctx) => {")); + ASSERT_NULL(strstr(js, "async setup(ctx) {")); ASSERT_NULL(strstr(js, "export const CodebaseMemory")); free(js); PASS(); From 282fbfcdabfec79c19c36f5bec5f75c22eb36df5 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 23 Sep 2026 22:41:07 +0200 Subject: [PATCH 07/12] fix(discover): match an enclosing repository's ignore rules relative to their own directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit found the enclosing repository when a git-less subfolder is indexed, but merged its patterns into the matcher whose rel_path is relative to repo_path. gitignore.c matches a rooted pattern with `glob_match_bounded(p->pattern, rel_path)`, so every anchored pattern of the enclosing repository was silently re-anchored onto the indexed subfolder. That is wrong in both directions, and both were checked against real `git check-ignore` run inside the subfolder: * enclosing root .gitignore `/secret.py`, indexing pkg/ git INDEXES pkg/secret.py — we HID it. A rooted pattern means "in the repository root", and re-anchored it became "in pkg". Silent index loss: the file is gone from the graph with no diagnostic, the worst failure mode discovery has. * enclosing .git/info/exclude `pkg/scratch/`, indexing pkg/ git IGNORES pkg/scratch/tmp.py — we INDEXED it. The pattern is anchored THROUGH pkg, so once rel_path lost the "pkg/" prefix it stopped matching and an excluded directory got walked. Only the enclosing root's .gitignore was consulted, too; git reads the .gitignore of every directory between the repository root and the indexed one, deeper overriding shallower. Mechanism. discover.c already had the chain that encodes "the deepest file with an opinion wins": gitignore_link_t carries a `prefix`, the walk-relative directory a nested matcher came from, which local_rel_path() strips off. An ancestor is the mirror image of that, so the chain is extended rather than duplicated: each link now also carries a `base`, the walk root's path relative to the directory the matcher came from, which gitignore_chain_result PREPENDS instead of stripping (discover.c:569, 610). Exactly one of the two offsets is ever non-empty, and both live in the link's flexible tail, so no allocation site is added. * ancestor_ignores_build() (discover.c:1282) loads the .gitignore of every directory from the enclosing repository root down to — but not including — the indexed one, shallow to deep, and chains them so the deepest wins, negations included. The enclosing repository's info/exclude is merged into the ROOT ancestor's matcher, which is where git anchors it, keeping the precedence info/exclude already has over .gitignore at the same level. * the indexed directory's own .gitignore stays its own matcher at the leaf of that chain (discover.c:1480), so it still overrides every ancestor. Nothing is merged into it any more, which also removes the earlier merge's failure mode of dropping the local patterns and keeping the ancestor's. * repo_path is canonicalized with cbm_canonical_path() before the lexical parent walk (discover.c:1414), so `cbm index pkg` and `cbm index /abs/path/pkg` discover the same files; pipeline.c strdups the caller's path unchanged. * walk_owned_gitignore_free() (discover.c:1019) is factored out of walk_dir and shared with the ancestor chain, which owns its matchers the same way. Proof on real input, before the suites. Indexing scripts/ of this checkout — a git-less subfolder whose root .gitignore carries the rooted patterns `/memlab-*` and `/soak*/`: before 87 files memlab-drive.py MISSING memlab-report.py MISSING after 89 files memlab-drive.py present memlab-report.py present `git check-ignore` inside scripts/ indexes both, so the two recovered files are exactly the ones git keeps. No file is lost in the other direction. RED on the previous commit, four new tests, each direction pinned to a `git check-ignore` verdict on an identical fixture: test_discover.c:1471: count == 1, expected 2 == 2 rooted_pattern_not_reanchored test_discover.c:1498: count == 2, expected 1 == 1 info_exclude_rooted_subpath test_discover.c:1526: count == 2, expected 1 == 1 intermediate_gitignore test_discover.c:1555: count == 0, expected 1 == 1 deeper_ancestor_negation_wins discover: 120 passed, 4 failed The three tests already on the branch use unanchored patterns, which match at any depth and so cannot see this bug; they pass before and after. GREEN: discover 124 passed, 0 failed; discover + gitignore + pipeline + git_context 425 passed, 0 failed. Revert-check: reverting only the discover.c hunks and rebuilding puts the four new tests RED again on the same four lines with the same counts (120 passed, 4 failed); restoring returns 425 passed. Memory-core linter: none grew (31 raw sites in discover.c before and after). clang-format clean. Co-authored-by: Amir Fathi Signed-off-by: Martin Vogel --- src/discover/discover.c | 229 ++++++++++++++++++++++++++++++++-------- tests/test_discover.c | 123 +++++++++++++++++++++ 2 files changed, 309 insertions(+), 43 deletions(-) diff --git a/src/discover/discover.c b/src/discover/discover.c index b90edc023e..c10d345f12 100644 --- a/src/discover/discover.c +++ b/src/discover/discover.c @@ -554,26 +554,40 @@ static const char *local_rel_path(const char *rel_path, const char *local_prefix * matcher borrows a pointer to the deepest link governing it, so a directory * without a .gitignore of its own simply shares its parent's link. Links live * on the heap (the frame stack is realloc'd and popped) and walk_dir owns them - * through the `owned_next` list. `prefix` is the walk-relative directory the - * matcher was loaded from ("" for the root). */ + * through the `owned_next` list. + * + * Every pattern is anchored to the directory its file came from, so each link + * carries the offset between that directory and the walk-relative rel_path it + * is asked about. The two offsets are mirror images and exactly one is ever + * non-empty: + * `prefix` — the matcher's directory is at or BELOW the walk root + * ("" for the root itself): strip it from rel_path. + * `base` — the matcher's directory is ABOVE the walk root (an enclosing + * repository's ignore files, when a git-less subfolder is + * indexed): prepend the walk root's path relative to it. + * Both strings live in the flexible tail, `base` right after `prefix`. */ typedef struct gitignore_link { const cbm_gitignore_t *gi; const struct gitignore_link *parent; struct gitignore_link *owned_next; + const char *base; char prefix[]; } gitignore_link_t; static gitignore_link_t *gitignore_link_new(const cbm_gitignore_t *gi, const char *prefix, - const gitignore_link_t *parent, + const char *base, const gitignore_link_t *parent, gitignore_link_t **owned_links) { size_t prefix_size = strlen(prefix) + SKIP_ONE; - gitignore_link_t *link = malloc(sizeof(*link) + prefix_size); + size_t base_size = strlen(base) + SKIP_ONE; + gitignore_link_t *link = malloc(sizeof(*link) + prefix_size + base_size); if (!link) { return NULL; } link->gi = gi; link->parent = parent; memcpy(link->prefix, prefix, prefix_size); + memcpy(link->prefix + prefix_size, base, base_size); + link->base = link->prefix + prefix_size; link->owned_next = *owned_links; *owned_links = link; return link; @@ -594,9 +608,20 @@ static void gitignore_links_free(gitignore_link_t *link) { * re-included by a negation, 0 when no file mentions the path. Cost is one * match per .gitignore on the path — O(depth), never a rescan. */ static int gitignore_chain_result(const gitignore_link_t *link, const char *rel_path, bool is_dir) { + char based[CBM_SZ_4K]; for (; link; link = link->parent) { - int verdict = - cbm_gitignore_match_result(link->gi, local_rel_path(rel_path, link->prefix), is_dir); + const char *probe = local_rel_path(rel_path, link->prefix); + if (link->base[0]) { + /* Matcher from a directory above the walk root: ask it about the + * path it would see, i.e. the one its own rooted patterns are + * anchored against. */ + int written = snprintf(based, sizeof(based), "%s/%s", link->base, probe); + if (written < 0 || (size_t)written >= sizeof(based)) { + continue; /* longer than this matcher can address: no opinion */ + } + probe = based; + } + int verdict = cbm_gitignore_match_result(link->gi, probe, is_dir); if (verdict != 0) { return verdict; } @@ -988,9 +1013,20 @@ static bool walk_owned_gitignore_append(cbm_gitignore_t ***owned, size_t *count, return true; } +/* Release an owner array built by walk_owned_gitignore_append(). Shared by the + * walk and by the enclosing-repository chain, which own their matchers the + * same way. */ +static void walk_owned_gitignore_free(cbm_gitignore_t **owned, size_t count) { + for (size_t i = 0; i < count; i++) { + cbm_gitignore_free(owned[i]); + } + free(owned); +} + static void walk_dir(const char *dir_path, const char *rel_prefix, const cbm_discover_opts_t *opts, - const cbm_gitignore_t *gitignore, const cbm_gitignore_t *global_gi, - const cbm_gitignore_t *cbmignore, file_list_t *out) { + const cbm_gitignore_t *gitignore, const gitignore_link_t *ancestors, + const cbm_gitignore_t *global_gi, const cbm_gitignore_t *cbmignore, + file_list_t *out) { walk_stack_t ws = { .frames = calloc(WALK_STACK_CAP, sizeof(walk_frame_t)), .top = 0, .cap = WALK_STACK_CAP}; if (!ws.frames) { @@ -1012,8 +1048,12 @@ static void walk_dir(const char *dir_path, const char *rel_prefix, const cbm_dis free(ws.frames); return; } + /* `ancestors` is the deepest link of the enclosing repository's chain (or + * NULL); the walk root's own matcher sits below it so it still wins. */ + ws.frames[0].ignore_chain = ancestors; if (gitignore) { - ws.frames[0].ignore_chain = gitignore_link_new(gitignore, rel_prefix, NULL, &owned_links); + ws.frames[0].ignore_chain = + gitignore_link_new(gitignore, rel_prefix, "", ancestors, &owned_links); if (!ws.frames[0].ignore_chain) { out->failed = true; free(ws.frames); @@ -1034,7 +1074,7 @@ static void walk_dir(const char *dir_path, const char *rel_prefix, const cbm_dis } /* owned_gis owns `loaded` from here on, even if the link fails. */ const gitignore_link_t *link = - gitignore_link_new(loaded, frame.prefix, frame.ignore_chain, &owned_links); + gitignore_link_new(loaded, frame.prefix, "", frame.ignore_chain, &owned_links); if (!link) { out->failed = true; break; @@ -1056,10 +1096,7 @@ static void walk_dir(const char *dir_path, const char *rel_prefix, const cbm_dis } cbm_closedir(d); } - for (size_t i = 0; i < owned_count; i++) { - cbm_gitignore_free(owned_gis[i]); - } - free(owned_gis); + walk_owned_gitignore_free(owned_gis, owned_count); gitignore_links_free(owned_links); free(ws.frames); } @@ -1199,6 +1236,102 @@ static bool resolve_enclosing_git_root(const char *repo_path, char *ancestor_roo } } +/* The enclosing repository's ignore files, kept OUT of the indexed directory's + * own matcher. Merging them in would re-anchor every rooted pattern onto the + * indexed subfolder: "/secret.py" at the enclosing root would start hiding + * /secret.py (silent index loss), and "pkg/scratch/" would stop + * hiding pkg/scratch (a directory git excludes would be walked). Each matcher + * instead keeps its own directory as its anchor, via the link's `base`. */ +typedef struct { + cbm_gitignore_t **owned; + size_t count; + size_t capacity; + const gitignore_link_t *chain; /* deepest ancestor link, or NULL */ + gitignore_link_t *links; /* owner list backing `chain` */ +} ancestor_ignores_t; + +static void ancestor_ignores_free(ancestor_ignores_t *anc) { + walk_owned_gitignore_free(anc->owned, anc->count); + gitignore_links_free(anc->links); +} + +/* Add one ancestor directory's matcher to the chain, deepest last so that the + * deepest file with an opinion wins (git's shallow-to-deep rule, negations + * included). Takes ownership of `gi`. */ +static bool ancestor_ignores_push(ancestor_ignores_t *anc, cbm_gitignore_t *gi, const char *base) { + if (!walk_owned_gitignore_append(&anc->owned, &anc->count, &anc->capacity, gi)) { + cbm_gitignore_free(gi); + return false; + } + /* owned[] owns `gi` from here on, even if the link fails. */ + const gitignore_link_t *link = gitignore_link_new(gi, "", base, anc->chain, &anc->links); + if (!link) { + return false; + } + anc->chain = link; + return true; +} + +/* Load the .gitignore of EVERY directory from the enclosing repository root + * (inclusive) down to the indexed directory (exclusive) — git consults all of + * them, not just the root's. `root_extra` is that repository's + * /info/exclude, which git anchors at its root like the root + * .gitignore; ownership is taken. `leaf` must be a canonical path under + * `root`, both with '/' separators. Returns false only on allocation + * failure. */ +static bool ancestor_ignores_build(ancestor_ignores_t *anc, const char *root, const char *leaf, + cbm_gitignore_t *root_extra) { + size_t root_len = strlen(root); + if (strncmp(leaf, root, root_len) != 0) { + cbm_gitignore_free(root_extra); + return true; + } + const char *base = leaf + root_len; + while (*base == '/') { + base++; + } + + char dir[CBM_SZ_4K]; + int dir_len = snprintf(dir, sizeof(dir), "%s", root); + if (dir_len < 0 || (size_t)dir_len >= sizeof(dir) || !*base) { + cbm_gitignore_free(root_extra); + return dir_len >= 0 && (size_t)dir_len < sizeof(dir); + } + + for (;;) { + char gi_path[CBM_SZ_4K]; + path_join(gi_path, sizeof(gi_path), dir, ".gitignore"); + cbm_gitignore_t *gi = cbm_gitignore_load(gi_path); + if (root_extra) { + /* First iteration: `dir` is the enclosing repository root, the one + * directory info/exclude shares an anchor with. Merged after the + * .gitignore patterns so it overrides them, the same precedence + * the two already have for an ordinary repository root. */ + if (!gi) { + gi = root_extra; + } else { + (void)cbm_gitignore_merge(gi, root_extra); + cbm_gitignore_free(root_extra); + } + root_extra = NULL; + } + if (gi && !ancestor_ignores_push(anc, gi, base)) { + return false; + } + const char *slash = strchr(base, '/'); + if (!slash) { + return true; /* the next directory down IS the indexed one */ + } + int written = snprintf(dir + dir_len, sizeof(dir) - (size_t)dir_len, "/%.*s", + (int)(slash - base), base); + if (written < 0 || (size_t)written >= sizeof(dir) - (size_t)dir_len) { + return true; /* deeper directories are unaddressable: stop here */ + } + dir_len += written; + base = slash + SKIP_ONE; + } +} + int cbm_discover(const char *repo_path, const cbm_discover_opts_t *opts, cbm_file_info_t **out, int *count) { return cbm_discover_ex(repo_path, opts, out, count, NULL, NULL); @@ -1246,15 +1379,18 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc /* Load gitignore sources for ordinary repos AND linked worktrees. * Sources merged in order (later patterns win on conflict): - * 1. /.gitignore: enclosing repo's root exclusions, only when - * repo_path itself has no .git of its own (see below) - * 2. /.gitignore: committed exclusions - * 3. /info/exclude: per-clone exclusions, not committed + * 1. /.gitignore — committed exclusions + * 2. /info/exclude — per-clone exclusions, not committed * is the git common dir, resolved via resolve_git_common_dir() so a * worktree (where .git is a gitlink file) reads the shared info/exclude/config - * just like a normal checkout. All are folded into a single matcher so all + * just like a normal checkout. Both are folded into a single matcher so all * downstream call paths remain unchanged. Fixes issue #489: OOM on repos whose - * worktrees are excluded only via .git/info/exclude (e.g. Sandcastle). */ + * worktrees are excluded only via .git/info/exclude (e.g. Sandcastle). + * + * An ENCLOSING repository's ignore files (indexing a git-less subfolder, + * issue #510) are deliberately NOT folded in here: their patterns are + * anchored to their own directories, so they stay separate matchers on the + * chain with a `base` offset. See ancestor_ignores_build(). */ cbm_gitignore_t *gitignore = NULL; char gi_path[CBM_SZ_4K]; struct stat gi_stat; @@ -1265,33 +1401,30 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc bool is_git_repo = resolve_git_common_dir(repo_path, git_common_dir, sizeof(git_common_dir)); bool has_git_config = false; /* repo_path itself is not a git repo root: walk upward for an enclosing one, - * exactly as `git` does when run from a subfolder. Its root .gitignore is - * loaded FIRST below (least specific), so the indexed directory's own - * .gitignore and info/exclude (both more specific) still override it on - * conflict. Fixes the remaining half of issue #510: only the indexed - * directory's own .gitignore was ever consulted, never an enclosing repo's. */ + * exactly as `git` does when run from a subfolder. Fixes the remaining half + * of issue #510: only the indexed directory's own .gitignore was ever + * consulted, never an enclosing repo's. The walk is lexical, so repo_path is + * canonicalized first — otherwise `cbm index pkg` and `cbm index /abs/pkg` + * would discover different files from the same directory. */ + ancestor_ignores_t ancestors = {0}; char enclosing_root[CBM_SZ_4K]; - if (!is_git_repo && - resolve_enclosing_git_root(repo_path, enclosing_root, sizeof(enclosing_root), - git_common_dir, sizeof(git_common_dir))) { - is_git_repo = true; - char enclosing_gi_path[CBM_SZ_4K]; - path_join(enclosing_gi_path, sizeof(enclosing_gi_path), enclosing_root, ".gitignore"); - gitignore = cbm_gitignore_load(enclosing_gi_path); + char canonical_repo[CBM_SZ_4K]; + bool has_enclosing_repo = false; + if (!is_git_repo) { + if (!cbm_canonical_path(repo_path, canonical_repo, sizeof(canonical_repo))) { + snprintf(canonical_repo, sizeof(canonical_repo), "%s", repo_path); + } + cbm_normalize_path_sep(canonical_repo); + has_enclosing_repo = + resolve_enclosing_git_root(canonical_repo, enclosing_root, sizeof(enclosing_root), + git_common_dir, sizeof(git_common_dir)); + is_git_repo = has_enclosing_repo; } /* Always honour the .gitignore at the indexed-directory root, even when the * directory is not a git repo root (e.g. indexing a sub-package directly). * Fixes issue #510: a root .gitignore was silently ignored without .git/. */ snprintf(gi_path, sizeof(gi_path), "%s/.gitignore", repo_path); - cbm_gitignore_t *local_gitignore = cbm_gitignore_load(gi_path); - if (local_gitignore) { - if (!gitignore) { - gitignore = local_gitignore; - } else { - (void)cbm_gitignore_merge(gitignore, local_gitignore); - cbm_gitignore_free(local_gitignore); - } - } + gitignore = cbm_gitignore_load(gi_path); if (is_git_repo) { path_join(gi_path, sizeof(gi_path), git_common_dir, "config"); has_git_config = wide_stat(gi_path, &gi_stat) == 0 && S_ISREG(gi_stat.st_mode); @@ -1299,7 +1432,16 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc char exc_path[CBM_SZ_4K]; path_join(exc_path, sizeof(exc_path), git_common_dir, "info/exclude"); cbm_gitignore_t *git_exclude = cbm_gitignore_load(exc_path); - if (git_exclude) { + if (has_enclosing_repo) { + /* git_common_dir belongs to the ENCLOSING repository, so its + * info/exclude is anchored at that repository's root, not at the + * indexed subfolder. Ownership passes to the ancestor chain. */ + if (!ancestor_ignores_build(&ancestors, enclosing_root, canonical_repo, git_exclude)) { + ancestor_ignores_free(&ancestors); + cbm_gitignore_free(gitignore); + return CBM_DISCOVER_ERROR; + } + } else if (git_exclude) { if (!gitignore) { gitignore = git_exclude; } else { @@ -1335,9 +1477,10 @@ static cbm_discover_status_t discover_impl(const char *repo_path, const cbm_disc .collect_ignored = !count_only && ignored_out != NULL, }; walk_cache_dir_snapshot(); - walk_dir(repo_path, "", opts, gitignore, global_gi, cbmignore, &fl); + walk_dir(repo_path, "", opts, gitignore, ancestors.chain, global_gi, cbmignore, &fl); /* Cleanup */ + ancestor_ignores_free(&ancestors); cbm_gitignore_free(gitignore); cbm_gitignore_free(global_gi); cbm_gitignore_free(cbmignore); diff --git a/tests/test_discover.c b/tests/test_discover.c index ae4d9585d7..d0ab9a0205 100644 --- a/tests/test_discover.c +++ b/tests/test_discover.c @@ -1442,6 +1442,125 @@ TEST(discover_enclosing_repo_gitignore_local_overrides) { PASS(); } +/* An enclosing repo's patterns are ANCHORED to the enclosing repo's own root, + * never to the indexed subfolder. Folding them into the subfolder's matcher + * re-anchors every rooted pattern one or more levels too deep, in both + * directions. The four tests below pin the four cases; each expectation was + * taken from `git check-ignore` run inside the subfolder on an identical + * fixture, so they encode git's behaviour, not ours. + * + * Direction 1 — silent index loss, the worst failure mode for discovery: + * `/secret.py` at the enclosing root means "secret.py in the ROOT", so git + * indexes pkg/secret.py. Re-anchored onto pkg it becomes "secret.py in pkg" + * and the file vanishes from the index with no diagnostic. */ +TEST(discover_enclosing_rooted_pattern_not_reanchored) { + char *base = th_mktempdir("cbm_disc_enc_anchor"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git")); + th_write_file(TH_PATH(base, ".gitignore"), "/secret.py\n"); + th_write_file(TH_PATH(base, "pkg/secret.py"), "TOKEN = 1\n"); + th_write_file(TH_PATH(base, "pkg/keep.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "pkg"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + /* git check-ignore inside pkg: both files INDEXED. */ + ASSERT_EQ(count, 2); + ASSERT_TRUE(discover_has_rel_path(files, count, "secret.py")); + ASSERT_TRUE(discover_has_rel_path(files, count, "keep.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + +/* Direction 2 — the mirror: a pattern the enclosing repo anchors THROUGH the + * indexed subfolder ("pkg/scratch/") no longer matches once rel_path is + * relative to pkg, so a directory git excludes gets walked and indexed. */ +TEST(discover_enclosing_info_exclude_rooted_subpath) { + char *base = th_mktempdir("cbm_disc_enc_exc_sub"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git/info")); + th_write_file(TH_PATH(base, ".git/info/exclude"), "pkg/scratch/\n"); + th_write_file(TH_PATH(base, "pkg/main.py"), "pass\n"); + th_write_file(TH_PATH(base, "pkg/scratch/tmp.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "pkg"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + /* git check-ignore inside pkg: scratch/tmp.py IGNORED, main.py indexed. */ + ASSERT_EQ(count, 1); + ASSERT_TRUE(discover_has_rel_path(files, count, "main.py")); + ASSERT_FALSE(discover_has_rel_path(files, count, "scratch/tmp.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + +/* git consults the .gitignore of EVERY directory between the enclosing root + * and the indexed one, not just the root's. Here only /a/.gitignore has + * an opinion, and it is rooted at a/ — so it needs both the intermediate file + * to be loaded at all and its patterns to be matched relative to a/. */ +TEST(discover_enclosing_intermediate_gitignore) { + char *base = th_mktempdir("cbm_disc_enc_mid"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git")); + th_write_file(TH_PATH(base, "a/.gitignore"), "/b/drop.py\n"); + th_write_file(TH_PATH(base, "a/b/drop.py"), "pass\n"); + th_write_file(TH_PATH(base, "a/b/keep.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "a/b"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + /* git check-ignore inside a/b: drop.py IGNORED, keep.py indexed. */ + ASSERT_EQ(count, 1); + ASSERT_TRUE(discover_has_rel_path(files, count, "keep.py")); + ASSERT_FALSE(discover_has_rel_path(files, count, "drop.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + +/* Precedence among ancestors: the deeper .gitignore wins over the shallower + * one, negations included. /.gitignore ignores every *.py; + * /a/.gitignore re-includes b/keep.py. Merging both into one matcher + * would decide this by file order instead of by depth. */ +TEST(discover_enclosing_deeper_ancestor_negation_wins) { + char *base = th_mktempdir("cbm_disc_enc_neg"); + ASSERT(base != NULL); + + th_mkdir_p(TH_PATH(base, ".git")); + th_write_file(TH_PATH(base, ".gitignore"), "*.py\n"); + th_write_file(TH_PATH(base, "a/.gitignore"), "!/b/keep.py\n"); + th_write_file(TH_PATH(base, "a/b/drop.py"), "pass\n"); + th_write_file(TH_PATH(base, "a/b/keep.py"), "pass\n"); + + cbm_discover_opts_t opts = {0}; + cbm_file_info_t *files = NULL; + int count = 0; + int rc = cbm_discover(TH_PATH(base, "a/b"), &opts, &files, &count); + ASSERT_EQ(rc, 0); + /* git check-ignore inside a/b: drop.py IGNORED, keep.py indexed. */ + ASSERT_EQ(count, 1); + ASSERT_TRUE(discover_has_rel_path(files, count, "keep.py")); + ASSERT_FALSE(discover_has_rel_path(files, count, "drop.py")); + + cbm_discover_free(files, count); + th_cleanup(base); + PASS(); +} + /* ── Nested .gitignore tests (issue #178) ──────────────────────── */ TEST(discover_nested_gitignore) { @@ -2062,6 +2181,10 @@ SUITE(discover) { RUN_TEST(discover_enclosing_repo_gitignore_issue510); RUN_TEST(discover_enclosing_repo_info_exclude); RUN_TEST(discover_enclosing_repo_gitignore_local_overrides); + RUN_TEST(discover_enclosing_rooted_pattern_not_reanchored); + RUN_TEST(discover_enclosing_info_exclude_rooted_subpath); + RUN_TEST(discover_enclosing_intermediate_gitignore); + RUN_TEST(discover_enclosing_deeper_ancestor_negation_wins); /* Nested .gitignore tests (issue #178) */ RUN_TEST(discover_nested_gitignore); From 629e9c2c1f87e2ac8b5d644d38ff4e935928752a Mon Sep 17 00:00:00 2001 From: Sarlette <282189207+SarletteD@users.noreply.github.com> Date: Sun, 27 Sep 2026 01:40:23 +0200 Subject: [PATCH 08/12] style(mcp): compare nullable strings through one helper cppcheck 2.20 (the version the lint job pins) reports knownConditionTrueFalse on two comparators in src/mcp/mcp.c whose tie-breakers compare one strcmp result after another, each call with inline "x ? x : \"\"" fallbacks. Neither function differs between these trees; which of them is flagged depends on the rest of the lint source list. With scripts/lint.sh --ci and cppcheck 2.20.0 built from source: - main itself fails on search_result_cmp ("file_order != 0 is always false"); - adding one more language grammar and its sources moves the finding to project_record_compare ("by_root != 0 is always false"); - carrying each comparator's order in one variable instead still fails, now as redundantAssignment on project_record_compare. Both comparators now call nullable_strcmp(), which orders NULL like "" exactly as the inline fallbacks did. lint-cppcheck passes again on main and with the additional sources. Comparison order and results are unchanged for every input. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Sarlette <282189207+SarletteD@users.noreply.github.com> --- src/mcp/mcp.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index d6c7bd4686..20d570ba69 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -2723,18 +2723,23 @@ static void project_record_clear(mcp_project_record_t *record) { memset(record, 0, sizeof(*record)); } +/* strcmp that orders NULL like "". */ +static int nullable_strcmp(const char *a, const char *b) { + return strcmp(a ? a : "", b ? b : ""); +} + static int project_record_compare(const void *left, const void *right) { const mcp_project_record_t *a = left; const mcp_project_record_t *b = right; - int by_name = strcmp(a->name ? a->name : "", b->name ? b->name : ""); + int by_name = nullable_strcmp(a->name, b->name); if (by_name != 0) { return by_name; } - int by_root = strcmp(a->root_path ? a->root_path : "", b->root_path ? b->root_path : ""); + int by_root = nullable_strcmp(a->root_path, b->root_path); if (by_root != 0) { return by_root; } - return strcmp(a->db_file ? a->db_file : "", b->db_file ? b->db_file : ""); + return nullable_strcmp(a->db_file, b->db_file); } typedef enum { @@ -12605,12 +12610,11 @@ static int search_result_cmp(const void *a, const void *b) { if (score_order != 0) { return score_order; } - int qn_order = strcmp(ra->qualified_name ? ra->qualified_name : "", - rb->qualified_name ? rb->qualified_name : ""); + int qn_order = nullable_strcmp(ra->qualified_name, rb->qualified_name); if (qn_order != 0) { return qn_order; } - int file_order = strcmp(ra->file ? ra->file : "", rb->file ? rb->file : ""); + int file_order = nullable_strcmp(ra->file, rb->file); if (file_order != 0) { return file_order; } From fa2292111802b5f3b66db3e4dd168280b53ac17e Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 13:32:22 +0200 Subject: [PATCH 09/12] 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); From 7707c9d7918263cd446a4b54d4eb48c3fea91048 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 26 Sep 2026 15:00:15 +0200 Subject: [PATCH 10/12] fix(extract): AST walker stacks grow instead of silently capping content Three iterative AST walkers kept their pending nodes in fixed-size stacks and silently stopped pushing children once full: - cbm_compute_complexity and count_branching_iter (helpers.c, BRANCHING_STACK_CAP 4096) -> cyclomatic, cognitive, loop and access metrics - extract_body_ident_tokens (extract_defs.c, BT_STACK 512) -> the body-token sample that feeds semantic search / SEMANTICALLY_RELATED Children are pushed last-to-first, so a full stack dropped a wide body's FIRST children. The metrics undercounted, and the token sampler read a mid-body window instead of the first identifiers. That let a work cap decide graph content. Real input: tree-sitter-llvm's generated ts_lex (a 4654-case switch) reported cyclomatic 11133 / cognitive 29303. It really has 8776 if + 4654 case + 1 default + 1 switch = 13432 branches, which is what it reports now (cognitive 35639). On the Linux kernel no function reaches the old 4096 cap, so complexity is byte-identical for every function. Three functions with bodies over 512 pending nodes now sample their tokens from the start of the body (for example rimob_panel_on), which moves 11 SEMANTICALLY_RELATED edges. Index time is unchanged within noise (main 214.4/207.9 s, fix 197.6/192.2 s). Fix: cbm_walk_stack_reserve()/cbm_walk_stack_release() in helpers.c. Each walker keeps a small on-stack first chunk (512 frames) and spills to the heap through the memory core (cbm_alloc/cbm_realloc/cbm_free, class EXTRACT) only for deep or wide trees. Traversal order is unchanged. If the stack cannot grow, the failure is logged ("extract.walk_stack_alloc_failed") and the walker returns a documented sentinel instead of a truncated result: CBM_WALK_METRIC_UNAVAILABLE (-1) for the metrics, or no token sample. The body-token sampler's caps of 128 identifiers and a 2048-byte buffer are its sampling contract and stay. The 512-entry stack was never part of that contract, because it broke the "first identifiers in source order" property. Tests (extraction suite): a C function with 5000 top-level if/while statements must report exact cyclomatic/cognitive/loop counts through both the extractor and cbm_count_branching, and a 600-call body must sample from f0 on. All three are RED on main (4095 / 4095 / sample starting at f91) and GREEN with the fix. Signed-off-by: Martin Vogel --- internal/cbm/extract_defs.c | 20 ++++++-- internal/cbm/helpers.c | 81 +++++++++++++++++++++++++---- internal/cbm/helpers.h | 23 +++++++++ tests/test_extraction.c | 100 ++++++++++++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 12 deletions(-) diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 49543e59a5..1e63d26fc1 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -87,10 +87,18 @@ static bool try_append_ident(const char *source, uint32_t s, int len, uint32_t * } /* Walk AST body, collect unique identifier text as space-separated string. - * Returns arena-allocated string or NULL. */ + * Contract: the first BT_MAX_IDENTS unique identifiers in pre-order (source + * order), bounded by BT_BUF bytes. The ident/byte caps ARE the sampling + * contract; the pending stack is not — BT_STACK is only the on-stack first + * chunk and spills to the heap, because a full fixed stack dropped a wide + * body's FIRST children and sampled a mid-body window instead. + * Returns arena-allocated string, or NULL when there are no identifiers or the + * stack could not be grown (logged; no truncated sample is ever returned). */ static char *extract_body_ident_tokens(CBMExtractCtx *ctx, TSNode body) { enum { BT_STACK = 512, BT_BUF = 2048, BT_MAX_IDENTS = 128, BT_SEEN = 256, BT_SEEN_MASK = 255 }; - TSNode bt_stack[BT_STACK]; + TSNode bt_inline[BT_STACK]; + TSNode *bt_stack = bt_inline; + int bt_cap = BT_STACK; int bt_top = 0; bt_stack[bt_top++] = body; char bt_buf[BT_BUF]; @@ -120,11 +128,17 @@ static char *extract_body_ident_tokens(CBMExtractCtx *ctx, TSNode body) { } } } else { - for (int i = (int)nc - SKIP_ONE; i >= 0 && bt_top < BT_STACK; i--) { + if (!cbm_walk_stack_reserve((void **)&bt_stack, &bt_cap, bt_top + (int)nc, + sizeof(TSNode), bt_inline, "body_ident_tokens")) { + bt_pos = 0; /* sentinel: no sample rather than a truncated one */ + break; + } + for (int i = (int)nc - SKIP_ONE; i >= 0; i--) { bt_stack[bt_top++] = ts_node_child(nd, (uint32_t)i); } } } + cbm_walk_stack_release(bt_stack, bt_inline); if (bt_pos > 0) { bt_buf[bt_pos] = '\0'; return cbm_arena_strdup(ctx->arena, bt_buf); diff --git a/internal/cbm/helpers.c b/internal/cbm/helpers.c index 50c75bdf2e..4c4d340f0b 100644 --- a/internal/cbm/helpers.c +++ b/internal/cbm/helpers.c @@ -4,8 +4,10 @@ #include "lang_specs.h" #include "tree_sitter/api.h" // TSNode, ts_node_* #include "foundation/constants.h" -#include "foundation/compat.h" // CBM_TLS -#include // calloc/free for the symbol-set cache +#include "foundation/compat.h" // CBM_TLS +#include "foundation/log.h" // cbm_log_error -- walker stack allocation failure +#include "foundation/mem_core.h" // cbm_alloc/cbm_realloc/cbm_free -- walker stacks +#include // calloc/free for the symbol-set cache enum { MIN_ROUTE_LEN = 3, @@ -25,7 +27,8 @@ enum { /* Prefix length helper for strncmp with string literals. */ #define SLEN(s) (sizeof(s) - SKIP_ONE) -#include // uint32_t +#include // uint32_t, SIZE_MAX +#include // INT_MAX #include #include #include @@ -630,10 +633,52 @@ bool cbm_has_ancestor_kind(TSNode node, const char *kind, int max_depth) { return false; } -// Recursive branching count -#define BRANCHING_STACK_CAP 4096 +bool cbm_walk_stack_reserve(void **items, int *cap, int need, size_t elem_size, + const void *inline_buf, const char *walker) { + if (need <= *cap) { + return true; + } + size_t new_cap = (size_t)*cap; + while (new_cap < (size_t)need) { + new_cap *= 2; + } + void *grown = NULL; + if (new_cap <= (size_t)INT_MAX && new_cap <= SIZE_MAX / elem_size) { + if (*items == inline_buf) { + grown = cbm_alloc(CBM_MEM_CLASS_EXTRACT, new_cap * elem_size); + if (grown) { + memcpy(grown, inline_buf, (size_t)*cap * elem_size); + } + } else { + grown = cbm_realloc(CBM_MEM_CLASS_EXTRACT, *items, new_cap * elem_size); + } + } + if (!grown) { + char pending[24]; + snprintf(pending, sizeof(pending), "%d", need); + cbm_log_error("extract.walk_stack_alloc_failed", "walker", walker, "pending", pending); + return false; + } + *items = grown; + *cap = (int)new_cap; + return true; +} + +void cbm_walk_stack_release(void *items, const void *inline_buf) { + if (items != inline_buf) { + cbm_free(CBM_MEM_CLASS_EXTRACT, items); + } +} + +// On-stack first chunk of the walker stacks. Normal functions never leave it; +// only a pending set this large (a very wide or deep body) spills to the heap. +enum { WALK_STACK_INLINE = 512 }; + +// Iterative branching count (pre-order; no cap on the pending set). static int count_branching_iter(TSNode root, const char **types) { - TSNode stack[BRANCHING_STACK_CAP]; + TSNode inline_stack[WALK_STACK_INLINE]; + TSNode *stack = inline_stack; + int cap = WALK_STACK_INLINE; int top = 0; int count = 0; stack[top++] = root; @@ -647,10 +692,16 @@ static int count_branching_iter(TSNode root, const char **types) { } } uint32_t n = ts_node_child_count(node); - for (int i = (int)n - SKIP_ONE; i >= 0 && top < BRANCHING_STACK_CAP; i--) { + if (!cbm_walk_stack_reserve((void **)&stack, &cap, top + (int)n, sizeof(TSNode), + inline_stack, "count_branching")) { + count = CBM_WALK_METRIC_UNAVAILABLE; + break; + } + for (int i = (int)n - SKIP_ONE; i >= 0; i--) { stack[top++] = ts_node_child(node, (uint32_t)i); } } + cbm_walk_stack_release(stack, inline_stack); return count; } @@ -723,7 +774,9 @@ void cbm_compute_complexity(TSNode node, const char **branching_types, cbm_compl int ldepth; int adepth; }; - struct cx_frame stack[BRANCHING_STACK_CAP]; + struct cx_frame inline_stack[WALK_STACK_INLINE]; + struct cx_frame *stack = inline_stack; + int cap = WALK_STACK_INLINE; int top = 0; stack[top].node = node; stack[top].bdepth = 0; @@ -770,7 +823,16 @@ void cbm_compute_complexity(TSNode node, const char **branching_types, cbm_compl child_l = d; } uint32_t n = ts_node_child_count(f.node); - for (int i = (int)n - SKIP_ONE; i >= 0 && top < BRANCHING_STACK_CAP; i--) { + if (!cbm_walk_stack_reserve((void **)&stack, &cap, top + (int)n, sizeof(struct cx_frame), + inline_stack, "compute_complexity")) { + out->cyclomatic = CBM_WALK_METRIC_UNAVAILABLE; + out->cognitive = CBM_WALK_METRIC_UNAVAILABLE; + out->loop_count = CBM_WALK_METRIC_UNAVAILABLE; + out->loop_depth = CBM_WALK_METRIC_UNAVAILABLE; + out->max_access_depth = CBM_WALK_METRIC_UNAVAILABLE; + break; + } + for (int i = (int)n - SKIP_ONE; i >= 0; i--) { stack[top].node = ts_node_child(f.node, (uint32_t)i); stack[top].bdepth = child_b; stack[top].ldepth = child_l; @@ -778,6 +840,7 @@ void cbm_compute_complexity(TSNode node, const char **branching_types, cbm_compl top++; } } + cbm_walk_stack_release(stack, inline_stack); } // --- Enclosing function detection --- diff --git a/internal/cbm/helpers.h b/internal/cbm/helpers.h index 905de04a8d..00e3d30dcc 100644 --- a/internal/cbm/helpers.h +++ b/internal/cbm/helpers.h @@ -174,7 +174,29 @@ void cbm_kind_in_set_free_cache(void); // Check if node has an ancestor of the given kind, within max_depth levels. bool cbm_has_ancestor_kind(TSNode node, const char *kind, int max_depth); +// Growable pending-node stack for the iterative AST walkers. +// +// No work cap may decide graph content, so walker stacks never truncate: a +// walker starts on a small caller-owned on-stack chunk (`inline_buf`) and spills +// to the heap (through the memory core, class EXTRACT) only for trees whose +// pending set outgrows it. Traversal order is unaffected by where the frames +// live. Ensures room for `need` frames in total; on success `*items`/`*cap` +// describe the (possibly moved) stack and true is returned. On allocation +// failure the stack is left intact, the failure is logged at error level under +// "extract.walk_stack_alloc_failed" (walker=), and false is returned — +// the caller then reports its documented failure sentinel instead of a +// silently truncated result. +bool cbm_walk_stack_reserve(void **items, int *cap, int need, size_t elem_size, + const void *inline_buf, const char *walker); +// Release a stack grown by cbm_walk_stack_reserve (no-op for the inline chunk). +void cbm_walk_stack_release(void *items, const void *inline_buf); + +// Sentinel reported by the walkers below when their pending stack could not be +// grown (allocation failure): the metric is unavailable, never an undercount. +#define CBM_WALK_METRIC_UNAVAILABLE (-1) + // Count nodes of given kinds in subtree (for complexity metric). +// Returns CBM_WALK_METRIC_UNAVAILABLE on allocation failure. int cbm_count_branching(TSNode node, const char **branching_types); // Per-function structural complexity, computed in a single AST walk. @@ -188,6 +210,7 @@ typedef struct { // Compute the metrics above in one traversal of `node`'s subtree. // `branching_types` is the language's branching node-type set. +// On allocation failure every field is CBM_WALK_METRIC_UNAVAILABLE. void cbm_compute_complexity(TSNode node, const char **branching_types, cbm_complexity_t *out); // Is `kind` a loop construct node type? Language-agnostic curated set (for/while/ diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 6370c090e4..6a1dcbdcd8 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -16,6 +16,8 @@ #include "result_spill.h" #include "pipeline/pass_lsp_cross.h" #include "iris_export_xml.h" +#include "helpers.h" /* cbm_count_branching (walker stack cap) */ +#include "lang_specs.h" /* cbm_lang_spec, cbm_ts_language */ /* ── Helpers ───────────────────────────────────────────────────── */ @@ -5512,6 +5514,101 @@ TEST(walk_defs_wide_file_extracts_every_definition) { PASS(); } +/* ── Walker stacks must never cap graph content ────────────────────── + * + * The complexity walkers and the body-token sampler used fixed-size pending + * stacks (4096 / 512 nodes) and silently stopped pushing children once full. + * Children are pushed last-to-first, so a wide body lost its FIRST statements: + * a function with 5000 top-level branches under-reported its cyclomatic count, + * and the token sampler started mid-body. These build bodies wider than either + * old cap and assert the exact metrics. */ +enum { WIDE_STMTS = 5000, WIDE_CALLS = 600, BODY_TOKEN_PREFIX = 100 }; + +/* C function `wide` whose body holds `n` top-level statements alternating + * `if (x) g();` and `while (x) g();`. Caller frees. */ +static char *build_wide_branch_fn(int n) { + size_t cap = (size_t)n * 24 + 64; + char *src = malloc(cap); + if (!src) + return NULL; + size_t pos = (size_t)snprintf(src, cap, "void g(void);\nvoid wide(int x) {\n"); + for (int i = 0; i < n; i++) { + pos += (size_t)snprintf(src + pos, cap - pos, "%s", + (i % 2 == 0) ? "if (x) g();\n" : "while (x) g();\n"); + } + snprintf(src + pos, cap - pos, "}\n"); + return src; +} + +TEST(complexity_wide_body_exceeds_old_stack_cap) { + char *src = build_wide_branch_fn(WIDE_STMTS); + ASSERT_NOT_NULL(src); + CBMFileResult *r = extract(src, CBM_LANG_C, "t", "wide.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const CBMDefinition *d = find_def(r, "wide"); + ASSERT_NOT_NULL(d); + /* every if/while is one branching node at nesting depth 0 */ + ASSERT_EQ(d->complexity, WIDE_STMTS); + ASSERT_EQ(d->cognitive, WIDE_STMTS); + ASSERT_EQ(d->loop_count, WIDE_STMTS / 2); + ASSERT_EQ(d->loop_depth, 1); + cbm_free_result(r); + free(src); + PASS(); +} + +TEST(count_branching_wide_body_exceeds_old_stack_cap) { + char *src = build_wide_branch_fn(WIDE_STMTS); + ASSERT_NOT_NULL(src); + TSParser *parser = ts_parser_new(); + ASSERT_NOT_NULL(parser); + ASSERT_TRUE(ts_parser_set_language(parser, cbm_ts_language(CBM_LANG_C))); + TSTree *tree = ts_parser_parse_string(parser, NULL, src, (uint32_t)strlen(src)); + ASSERT_NOT_NULL(tree); + int n = cbm_count_branching(ts_tree_root_node(tree), + cbm_lang_spec(CBM_LANG_C)->branching_node_types); + ASSERT_EQ(n, WIDE_STMTS); + ts_tree_delete(tree); + ts_parser_delete(parser); + free(src); + PASS(); +} + +/* The body-token sampler's contract is "the first BODY_TOKEN_CAP unique + * identifiers in source order". A body with 600 distinct calls must sample + * from f0 on, not a window starting mid-body (the capped stack began at f91). + * Only the source-order prefix f0..f99 is asserted: the sampler's dedupe key + * (hash | 1) also merges identifiers whose hashes differ only in bit 0, which + * drops a few later names (f122, f124, ...) -- a separate dedupe property this + * test deliberately does not pin. */ +TEST(body_tokens_wide_body_samples_from_the_start) { + size_t cap = (size_t)WIDE_CALLS * 16 + 64; + char *src = malloc(cap); + ASSERT_NOT_NULL(src); + size_t pos = (size_t)snprintf(src, cap, "void wide_calls(void) {\n"); + for (int i = 0; i < WIDE_CALLS; i++) { + pos += (size_t)snprintf(src + pos, cap - pos, "f%d();\n", i); + } + snprintf(src + pos, cap - pos, "}\n"); + char expect[BODY_TOKEN_PREFIX * 8]; + size_t ep = 0; + for (int i = 0; i < BODY_TOKEN_PREFIX; i++) { + ep += (size_t)snprintf(expect + ep, sizeof(expect) - ep, "f%d ", i); + } + CBMFileResult *r = extract(src, CBM_LANG_C, "t", "wide_calls.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const CBMDefinition *d = find_def(r, "wide_calls"); + ASSERT_NOT_NULL(d); + ASSERT_NOT_NULL(d->body_tokens); + ASSERT_GTE(strlen(d->body_tokens), ep); + ASSERT_MEM_EQ(d->body_tokens, expect, ep); + cbm_free_result(r); + free(src); + PASS(); +} + /* ═══════════════════════════════════════════════════════════════════ * Perl call-graph noise (#459 follow-up) * ═══════════════════════════════════════════════════════════════════ */ @@ -8771,6 +8868,9 @@ SUITE(extraction) { RUN_TEST(complexity_delegation_receivers_not_recursive_issue876); RUN_TEST(complexity_access_depth_and_params); RUN_TEST(walk_defs_wide_file_extracts_every_definition); + RUN_TEST(complexity_wide_body_exceeds_old_stack_cap); + RUN_TEST(count_branching_wide_body_exceeds_old_stack_cap); + RUN_TEST(body_tokens_wide_body_samples_from_the_start); RUN_TEST(extract_c_ifdef_split_brace_fn_recovered_issue961); RUN_TEST(extract_cpp_preproc_signature_gap_issue946); RUN_TEST(extract_cpp_preproc_macro_generated_callable_skipped_issue949); From f339aa625ffd71b936fe386250858e281e5b87ff Mon Sep 17 00:00:00 2001 From: Andrew Hundt Date: Thu, 13 Aug 2026 01:44:44 -0400 Subject: [PATCH 11/12] perf(lsp): collapse repeated Python scope-chain lookups Add cbm_scope_lookup_binding so callers can read lexical presence, type, and callable identity in one traversal. Route Python expression, call, and callable-reference resolution through that complete binding and use the existing checked bind APIs for allocation-failure detection. The neutral 11-pair production matrix kept exact output parity. Python scale latency was 4.33% below merge parent 07255cfe and 61.01% below upstream parent 0c64de93; incremental instructions were 3.09% and 54.07% lower. Focused ASan/UBSan scope and Python suites passed 33 and 125 tests. Signed-off-by: Andrew Hundt (cherry picked from commit 4d3a10304737b3a70c543ff19563be2dff214fdf) Signed-off-by: Martin Vogel --- internal/cbm/lsp/py_lsp.c | 52 +++++++++++++++++---------------------- internal/cbm/lsp/scope.c | 50 ++++++++++++------------------------- internal/cbm/lsp/scope.h | 4 +++ tests/test_scope.c | 20 +++++++++++++++ 4 files changed, 62 insertions(+), 64 deletions(-) diff --git a/internal/cbm/lsp/py_lsp.c b/internal/cbm/lsp/py_lsp.c index 8a03961152..2cf66ad012 100644 --- a/internal/cbm/lsp/py_lsp.c +++ b/internal/cbm/lsp/py_lsp.c @@ -191,20 +191,15 @@ static void py_disable_callable_value_proof(PyLSPContext *ctx) { static void py_scope_bind(PyLSPContext *ctx, const char *name, const CBMType *type) { ctx->type_cache_gen++; - cbm_scope_bind(ctx->current_scope, name, type); - if (name && !cbm_scope_contains(ctx->current_scope, name)) + if (!cbm_scope_bind_checked(ctx->current_scope, name, type)) py_disable_callable_value_proof(ctx); } static void py_scope_bind_callable(PyLSPContext *ctx, const char *name, const CBMType *type, const char *callable_qn) { ctx->type_cache_gen++; - cbm_scope_bind_callable(ctx->current_scope, name, type, callable_qn); - const char *bound = name ? cbm_scope_lookup_callable(ctx->current_scope, name) : NULL; - if (name && (!cbm_scope_contains(ctx->current_scope, name) || - (callable_qn && (!bound || strcmp(bound, callable_qn) != 0)))) { + if (!cbm_scope_bind_callable_checked(ctx->current_scope, name, type, callable_qn)) py_disable_callable_value_proof(ctx); - } } static CBMScope *py_scope_push_checked(PyLSPContext *ctx) { @@ -938,8 +933,9 @@ static const char *py_exact_callable_target_ex(PyLSPContext *ctx, TSNode node, char *name = py_node_text(ctx, node); if (!name) return NULL; - if (cbm_scope_contains(ctx->current_scope, name)) { - const char *bound = cbm_scope_lookup_callable(ctx->current_scope, name); + const CBMVarBinding *binding = cbm_scope_lookup_binding(ctx->current_scope, name); + if (binding) { + const char *bound = binding->callable_qn; if (bound && lexical_alias_out) { /* A lexical binding that does NOT simply name the module symbol * of the same spelling is an alias introduced in this body. Only @@ -1018,11 +1014,12 @@ static void py_resolve_value_references_at(PyLSPContext *ctx, TSNode call) { const char *candidate = strcmp(kind, "identifier") == 0 ? py_exact_imported_reference_candidate(ctx, source_name) : NULL; - if (candidate && cbm_scope_contains(ctx->current_scope, source_name)) { - const CBMType *binding = - cbm_type_resolve_alias(cbm_scope_lookup(ctx->current_scope, source_name)); - if (binding && binding->kind == CBM_TYPE_NAMED && binding->data.named.qualified_name && - strcmp(binding->data.named.qualified_name, candidate) == 0) { + const CBMVarBinding *scope_binding = + candidate ? cbm_scope_lookup_binding(ctx->current_scope, source_name) : NULL; + if (scope_binding) { + const CBMType *type = cbm_type_resolve_alias(scope_binding->type); + if (type && type->kind == CBM_TYPE_NAMED && type->data.named.qualified_name && + strcmp(type->data.named.qualified_name, candidate) == 0) { py_emit_unresolved_reference(ctx, candidate, arg); } } @@ -1498,9 +1495,9 @@ static const CBMType *py_eval_expr_type_uncached(PyLSPContext *ctx, TSNode node) char *name = py_node_text(ctx, node); if (!name) return cbm_type_unknown(); - const CBMType *t = cbm_scope_lookup(ctx->current_scope, name); - if (cbm_scope_contains(ctx->current_scope, name)) - return t ? t : cbm_type_unknown(); + const CBMVarBinding *binding = cbm_scope_lookup_binding(ctx->current_scope, name); + if (binding) + return binding->type ? binding->type : cbm_type_unknown(); // Builtin globals: True / False / None at top level. if (strcmp(name, "True") == 0 || strcmp(name, "False") == 0) return cbm_type_builtin(ctx->arena, "bool"); @@ -1767,8 +1764,9 @@ static const CBMType *py_eval_expr_type_uncached(PyLSPContext *ctx, TSNode node) } } // Constructor call: ClassName() returns NAMED(ClassName). - const CBMType *in_scope = cbm_scope_lookup(ctx->current_scope, fname); - const char *callable_qn = cbm_scope_lookup_callable(ctx->current_scope, fname); + const CBMVarBinding *binding = cbm_scope_lookup_binding(ctx->current_scope, fname); + const CBMType *in_scope = binding && binding->type ? binding->type : cbm_type_unknown(); + const char *callable_qn = binding ? binding->callable_qn : NULL; if (callable_qn) { return py_func_return_type(ctx, callable_qn); } @@ -1796,7 +1794,7 @@ static const CBMType *py_eval_expr_type_uncached(PyLSPContext *ctx, TSNode node) } /* Even an UNKNOWN local/parameter is a real lexical shadow. Do * not borrow the return type of a same-named module function. */ - if (cbm_scope_contains(ctx->current_scope, fname)) + if (binding) return cbm_type_unknown(); // Module-local function call. const CBMRegisteredFunc *f = @@ -2566,14 +2564,15 @@ static void py_emit_call_for(PyLSPContext *ctx, TSNode call_node) { * An ordinary local binding is also a hard shadow: if it is not a * proven callable alias, do not fall through and fabricate a direct * call to a module-level function with the same spelling. */ - if (cbm_scope_contains(ctx->current_scope, fname)) { - const char *alias_target = cbm_scope_lookup_callable(ctx->current_scope, fname); + const CBMVarBinding *binding = cbm_scope_lookup_binding(ctx->current_scope, fname); + if (binding) { + const char *alias_target = binding->callable_qn; if (alias_target) { py_emit_resolved_call_reason(ctx, alias_target, "lsp_callable_alias", 0.97f, fname, call_node); return; } - const CBMType *in_scope = cbm_scope_lookup(ctx->current_scope, fname); + const CBMType *in_scope = binding->type ? binding->type : cbm_type_unknown(); if (!cbm_type_is_unknown(in_scope) && in_scope->kind == CBM_TYPE_NAMED) { const char *qn = in_scope->data.named.qualified_name; const char *tail = qn ? strrchr(qn, '.') : NULL; @@ -2604,13 +2603,6 @@ static void py_emit_call_for(PyLSPContext *ctx, TSNode call_node) { } return; } - // Constructor call (ClassName()) - const CBMType *in_scope = cbm_scope_lookup(ctx->current_scope, fname); - if (!cbm_type_is_unknown(in_scope) && in_scope->kind == CBM_TYPE_NAMED) { - const char *qn = in_scope->data.named.qualified_name; - py_emit_resolved_call(ctx, qn, "lsp_constructor", 0.85f, call_node); - return; - } // Module-local function const CBMRegisteredFunc *f = cbm_registry_lookup_symbol(ctx->registry, ctx->module_qn, fname); diff --git a/internal/cbm/lsp/scope.c b/internal/cbm/lsp/scope.c index 5b842cc1ba..c44a0b544a 100644 --- a/internal/cbm/lsp/scope.c +++ b/internal/cbm/lsp/scope.c @@ -88,52 +88,34 @@ bool cbm_scope_bind_callable_checked(CBMScope *scope, const char *name, const CB return cbm_scope_bind_value(scope, name, type, callable_qn); } -const CBMType* cbm_scope_lookup(const CBMScope* scope, const char* name) { - if (!name) { - return cbm_type_unknown(); - } +const CBMVarBinding *cbm_scope_lookup_binding(const CBMScope *scope, const char *name) { + if (!name) + return NULL; for (const CBMScope* s = scope; s != NULL; s = s->parent) { - for (CBMScopeChunk* c = s->chunks; c != NULL; c = c->next) { + for (const CBMScopeChunk* c = s->chunks; c != NULL; c = c->next) { for (int i = 0; i < c->used; i++) { - if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) { - return c->bindings[i].type; - } + if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) + return &c->bindings[i]; } } } + return NULL; +} + +const CBMType* cbm_scope_lookup(const CBMScope* scope, const char* name) { + const CBMVarBinding *binding = cbm_scope_lookup_binding(scope, name); + if (binding) + return binding->type; return cbm_type_unknown(); } bool cbm_scope_contains(const CBMScope *scope, const char *name) { - if (!name) { - return false; - } - for (const CBMScope *s = scope; s != NULL; s = s->parent) { - for (const CBMScopeChunk *c = s->chunks; c != NULL; c = c->next) { - for (int i = 0; i < c->used; i++) { - if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) { - return true; - } - } - } - } - return false; + return cbm_scope_lookup_binding(scope, name) != NULL; } const char *cbm_scope_lookup_callable(const CBMScope *scope, const char *name) { - if (!name) { - return NULL; - } - for (const CBMScope *s = scope; s != NULL; s = s->parent) { - for (const CBMScopeChunk *c = s->chunks; c != NULL; c = c->next) { - for (int i = 0; i < c->used; i++) { - if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) { - return c->bindings[i].callable_qn; - } - } - } - } - return NULL; + const CBMVarBinding *binding = cbm_scope_lookup_binding(scope, name); + return binding ? binding->callable_qn : NULL; } bool cbm_scope_update_callable(CBMScope *scope, const char *name, const char *callable_qn) { diff --git a/internal/cbm/lsp/scope.h b/internal/cbm/lsp/scope.h index f4ec782a7e..3ec438fe9a 100644 --- a/internal/cbm/lsp/scope.h +++ b/internal/cbm/lsp/scope.h @@ -82,6 +82,10 @@ bool cbm_scope_bind_callable_checked(CBMScope *scope, const char *name, const CB void cbm_scope_bind_callable(CBMScope *scope, const char *name, const CBMType *type, const char *callable_qn); const CBMType* cbm_scope_lookup(const CBMScope* scope, const char* name); +/* Return the complete nearest binding in one scope-chain walk, or NULL when + * unbound. Callers that need presence, type, and callable identity together + * should use this instead of repeating three linear scans. */ +const CBMVarBinding *cbm_scope_lookup_binding(const CBMScope *scope, const char *name); /* True when any lexical frame contains name, even when its type is UNKNOWN. */ bool cbm_scope_contains(const CBMScope *scope, const char *name); /* Return the exact callable QN from the nearest binding. A nearer ordinary diff --git a/tests/test_scope.c b/tests/test_scope.c index 9effb7ca9b..9ecd497bc2 100644 --- a/tests/test_scope.c +++ b/tests/test_scope.c @@ -125,6 +125,25 @@ TEST(scope_callable_identity_follows_nearest_binding) { PASS(); } +TEST(scope_lookup_binding_returns_nearest_complete_record) { + CBMArena a; + cbm_arena_init(&a); + CBMScope *root = cbm_scope_push(&a, NULL); + CBMScope *child = cbm_scope_push(&a, root); + cbm_scope_bind_callable(root, "callback", named_t(&a, "Callback"), "pkg.actual"); + cbm_scope_bind(child, "callback", named_t(&a, "Shadow")); + + const CBMVarBinding *binding = cbm_scope_lookup_binding(child, "callback"); + ASSERT_NOT_NULL(binding); + ASSERT_STR_EQ(binding->name, "callback"); + ASSERT_STR_EQ(binding->type->data.named.qualified_name, "Shadow"); + ASSERT_NULL(binding->callable_qn); + ASSERT_NULL(cbm_scope_lookup_binding(child, "missing")); + + cbm_arena_destroy(&a); + PASS(); +} + TEST(scope_assignment_updates_or_clears_nearest_callable_only) { CBMArena a; cbm_arena_init(&a); @@ -260,6 +279,7 @@ SUITE(scope) { RUN_TEST(scope_lookup_walks_parent_chain); RUN_TEST(scope_child_shadows_parent); RUN_TEST(scope_callable_identity_follows_nearest_binding); + RUN_TEST(scope_lookup_binding_returns_nearest_complete_record); RUN_TEST(scope_assignment_updates_or_clears_nearest_callable_only); RUN_TEST(scope_checked_bind_reports_child_oom_despite_parent_name); RUN_TEST(scope_dynamic_growth_300_bindings); From df8dd1011bd1987d0ca5a45f324775aad2411f54 Mon Sep 17 00:00:00 2001 From: Andrew Hundt Date: Thu, 13 Aug 2026 02:58:52 -0400 Subject: [PATCH 12/12] internal/cbm/lsp/scope.*: inline complete binding lookup Previous behavior: cbm_scope_lookup(), cbm_scope_contains(), and cbm_scope_lookup_callable() called an out-of-line complete-binding scan after the Python lookup consolidation. What changed: - internal/cbm/lsp/scope.h defines cbm_scope_lookup_binding() as the one static-inline scope-chain scan. - internal/cbm/lsp/scope.c keeps the type, presence, and callable wrappers on that shared implementation without another function boundary. Why: Python still reads type and callable identity in one traversal, while existing language resolvers retain the direct hot path used before the consolidation. Verification: - CBM_ONLY_SUITE=scope build/c/test-runner: 33 passed - CBM_ONLY_SUITE=cs_lsp build/c/test-runner: 62 passed - CBM_ONLY_SUITE=py_lsp build/c/test-runner: 125 passed - neutral 11-pair extraction matrix: output parity; C# latency -2.97% vs first parent and -1.64% vs upstream; retired instructions +0.29% vs first parent and -1.01% vs upstream; Python scale latency -4.25% and instructions -2.81% vs first parent Signed-off-by: Andrew Hundt (cherry picked from commit 150c88e1c85a3f0a5ee4f97cde02729d896d7fc5) Signed-off-by: Martin Vogel --- internal/cbm/lsp/scope.c | 14 -------------- internal/cbm/lsp/scope.h | 24 ++++++++++++++++++++---- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/internal/cbm/lsp/scope.c b/internal/cbm/lsp/scope.c index c44a0b544a..2d9740db0c 100644 --- a/internal/cbm/lsp/scope.c +++ b/internal/cbm/lsp/scope.c @@ -88,20 +88,6 @@ bool cbm_scope_bind_callable_checked(CBMScope *scope, const char *name, const CB return cbm_scope_bind_value(scope, name, type, callable_qn); } -const CBMVarBinding *cbm_scope_lookup_binding(const CBMScope *scope, const char *name) { - if (!name) - return NULL; - for (const CBMScope* s = scope; s != NULL; s = s->parent) { - for (const CBMScopeChunk* c = s->chunks; c != NULL; c = c->next) { - for (int i = 0; i < c->used; i++) { - if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) - return &c->bindings[i]; - } - } - } - return NULL; -} - const CBMType* cbm_scope_lookup(const CBMScope* scope, const char* name) { const CBMVarBinding *binding = cbm_scope_lookup_binding(scope, name); if (binding) diff --git a/internal/cbm/lsp/scope.h b/internal/cbm/lsp/scope.h index 3ec438fe9a..5ba7265a36 100644 --- a/internal/cbm/lsp/scope.h +++ b/internal/cbm/lsp/scope.h @@ -5,6 +5,7 @@ #include "../arena.h" #include /* relaxed cache for cbm_lsp_max_walk_depth */ #include /* getenv, atoi (cbm_lsp_max_walk_depth) */ +#include /* strcmp (cbm_scope_lookup_binding) */ typedef struct { const char* name; @@ -30,6 +31,25 @@ typedef struct CBMScope { CBMArena* arena; // owning arena, propagated to children at push time } CBMScope; +/* Return the complete nearest binding in one scope-chain walk, or NULL when + * unbound. Keep this internal hot-path primitive inline: all language + * resolvers use cbm_scope_lookup(), while Python also consumes the complete + * record to avoid repeating the same linear scan. */ +static inline const CBMVarBinding *cbm_scope_lookup_binding(const CBMScope *scope, + const char *name) { + if (!name) + return NULL; + for (const CBMScope *s = scope; s != NULL; s = s->parent) { + for (const CBMScopeChunk *c = s->chunks; c != NULL; c = c->next) { + for (int i = 0; i < c->used; i++) { + if (c->bindings[i].name && strcmp(c->bindings[i].name, name) == 0) + return &c->bindings[i]; + } + } + } + return NULL; +} + // Bail-to-UNKNOWN depth for type-lookup chains: alias resolution, MRO walks, // embedded-field/struct-traversal. Exceeding this collapses to cbm_type_unknown // rather than recursing — guards against pathological hierarchies. @@ -82,10 +102,6 @@ bool cbm_scope_bind_callable_checked(CBMScope *scope, const char *name, const CB void cbm_scope_bind_callable(CBMScope *scope, const char *name, const CBMType *type, const char *callable_qn); const CBMType* cbm_scope_lookup(const CBMScope* scope, const char* name); -/* Return the complete nearest binding in one scope-chain walk, or NULL when - * unbound. Callers that need presence, type, and callable identity together - * should use this instead of repeating three linear scans. */ -const CBMVarBinding *cbm_scope_lookup_binding(const CBMScope *scope, const char *name); /* True when any lexical frame contains name, even when its type is UNKNOWN. */ bool cbm_scope_contains(const CBMScope *scope, const char *name); /* Return the exact callable QN from the nearest binding. A nearer ordinary