diff --git a/Makefile.cbm b/Makefile.cbm index 44ccb7473..b5f194542 100644 --- a/Makefile.cbm +++ b/Makefile.cbm @@ -450,6 +450,7 @@ TEST_REPRO_SRCS = \ tests/repro/repro_issue581.c \ tests/repro/repro_issue787.c \ tests/repro/repro_issue842.c \ + tests/repro/repro_issue964.c \ tests/repro/repro_invariant_calls.c \ tests/repro/repro_invariant_graph.c \ tests/repro/repro_invariant_breadth.c \ diff --git a/internal/cbm/cbm.c b/internal/cbm/cbm.c index 8de94b941..af454426f 100644 --- a/internal/cbm/cbm.c +++ b/internal/cbm/cbm.c @@ -817,6 +817,38 @@ static bool cbm_region_is_recovered(uint32_t rs, uint32_t re, const CBMDefArray return covered_to >= re; } +/* #961: true when 1-based `line` of `src` contains `name` (used to verify a + * def recovered from EXPANDED source really lives on that ORIGINAL line — + * rejects header-inlined defs whose physical expanded lines alias unrelated + * raw lines when compile_commands include paths are present). */ +static bool cbm_line_contains(const char *src, int src_len, uint32_t line, const char *name) { + if (!src || !name || !name[0] || line == 0) { + return false; + } + uint32_t cur = 1; + int i = 0; + while (i < src_len && cur < line) { + if (src[i] == '\n') { + cur++; + } + i++; + } + if (cur != line) { + return false; + } + int end = i; + while (end < src_len && src[end] != '\n') { + end++; + } + size_t nlen = strlen(name); + for (int j = i; j + (int)nlen <= end; j++) { + if (strncmp(src + j, name, nlen) == 0) { + return true; + } + } + return false; +} + static void cbm_subtract_recovered_regions(cbm_error_regions_t *regs, const CBMDefArray *defs) { int kept = 0; for (int i = 0; i < regs->count; i++) { @@ -1103,6 +1135,52 @@ static CBMFileResult *cbm_extract_file_impl(const char *source, int source_len, cbm_run_c_lsp(a, result, expanded, expanded_len, pp_root, language != CBM_LANG_C); + /* #961: a def whose body braces are split across + * #ifdef/#else branches parses as an ERROR region on the + * RAW source (both branches present at once -> unbalanced + * braces), so the raw defs walk silently dropped it. The + * expanded tree parses clean (simplecpp picked one + * branch) and same-file token lines stay aligned, so + * recover defs from it — adopting ONLY those that + * intersect a raw ERROR region, whose name is visible on + * the raw source line, and whose QN the raw pass did not + * already extract. */ + if (ts_node_has_error(root)) { + cbm_error_regions_t raw_regs = {{0}, {0}, 0}; + cbm_collect_error_regions(root, &raw_regs); + if (raw_regs.count > 0) { + int defs_before = result->defs.count; + cbm_extract_definitions(&pp_ctx); + int w = defs_before; + for (int i = defs_before; i < result->defs.count; i++) { + CBMDefinition *d = &result->defs.items[i]; + bool adopt = false; + for (int rj = 0; rj < raw_regs.count && !adopt; rj++) { + if (d->start_line <= raw_regs.ends[rj] && + d->end_line >= raw_regs.starts[rj]) { + adopt = true; + } + } + if (adopt && + (!d->name || !cbm_line_contains(source, source_len, + d->start_line, d->name))) { + adopt = false; + } + for (int j = 0; j < defs_before && adopt; j++) { + const char *q = result->defs.items[j].qualified_name; + if (q && d->qualified_name && + strcmp(q, d->qualified_name) == 0) { + adopt = false; + } + } + if (adopt) { + result->defs.items[w++] = *d; + } + } + result->defs.count = w; + } + } + ts_tree_delete(pp_tree); } } diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 2de63ffbb..9b7acc42a 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -4646,6 +4646,37 @@ static void extract_destructured_vars(CBMExtractCtx *ctx, TSNode pattern, TSNode } } +/* True for `require("...")` call_expressions with a string-literal path — the + * CommonJS import form that extract_imports records with local_name = the + * enclosing declarator's identifier. Same string-argument kinds as + * process_commonjs_require so the two stay in lockstep. */ +static bool is_require_import_call(TSNode value, const char *source, CBMArena *a) { + if (strcmp(ts_node_type(value), "call_expression") != 0) { + return false; + } + TSNode fn = ts_node_child_by_field_name(value, TS_FIELD("function")); + if (ts_node_is_null(fn) || strcmp(ts_node_type(fn), "identifier") != 0) { + return false; + } + char *fn_name = cbm_node_text(a, fn, source); + if (!fn_name || strcmp(fn_name, "require") != 0) { + return false; + } + TSNode args = ts_node_child_by_field_name(value, TS_FIELD("arguments")); + if (ts_node_is_null(args)) { + return false; + } + uint32_t argc = ts_node_named_child_count(args); + for (uint32_t i = 0; i < argc; i++) { + const char *ak = ts_node_type(ts_node_named_child(args, i)); + if (strcmp(ak, "string") == 0 || strcmp(ak, "string_literal") == 0 || + strcmp(ak, "template_string") == 0) { + return true; + } + } + return false; +} + // JS/TS variable extraction: skip function-assigned declarators. static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) { uint32_t n = ts_node_named_child_count(node); @@ -4654,6 +4685,7 @@ static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) { if (strcmp(ts_node_type(child), "variable_declarator") != 0) { continue; } + bool is_require = false; TSNode value = ts_node_child_by_field_name(child, TS_FIELD("value")); if (!ts_node_is_null(value)) { const char *vk = ts_node_type(value); @@ -4661,6 +4693,7 @@ static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) { strcmp(vk, "generator_function") == 0) { continue; } + is_require = is_require_import_call(value, ctx->source, a); } TSNode vname = ts_node_child_by_field_name(child, TS_FIELD("name")); if (!ts_node_is_null(vname)) { @@ -4670,6 +4703,15 @@ static void extract_js_vars(CBMExtractCtx *ctx, TSNode node, CBMArena *a) { if (strcmp(nk, "object_pattern") == 0 || strcmp(nk, "array_pattern") == 0) { extract_destructured_vars(ctx, vname, child, a); } else { + /* `const foo = require('./foo')` is an import binding, not a + * definition — extract_imports records local_name="foo". A + * Variable node here shadows call resolution onto the alias + * and orphans the imported callee (#871); ESM `import` + * bindings emit no Variable either. Destructured requires + * keep theirs: the import row only records the module. */ + if (is_require) { + continue; + } push_var_def(ctx, cbm_node_text(a, vname, ctx->source), child); } } diff --git a/src/main.c b/src/main.c index 80a9c0d48..b21d99d03 100644 --- a/src/main.c +++ b/src/main.c @@ -159,16 +159,18 @@ static void *http_thread(void *arg) { static int watcher_index_fn(const char *project_name, const char *root_path, void *user_data) { (void)user_data; - /* Skip indexing if shutdown is in progress */ + /* Skip indexing if shutdown is in progress (skipped, not indexed) */ if (atomic_load(&g_shutdown)) { - return 0; + return 1; } - /* Non-blocking: skip if another pipeline is already running. - * Watcher will retry on next poll cycle (5-60s). */ + /* Non-blocking: skip if another pipeline is already running. The + * positive return keeps the watcher's baselines uncommitted so the + * change is retried on the next poll cycle (5-60s) instead of being + * recorded as seen and lost (#937). */ if (!cbm_pipeline_try_lock()) { cbm_log_info("watcher.skip", "project", project_name, "reason", "pipeline_busy"); - return 0; + return 1; } cbm_log_info("watcher.reindex", "project", project_name, "path", root_path); diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 2cf81d468..e05ffdc6c 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -853,6 +853,67 @@ static char *normalize_project_arg(char *project) { return project; } +/* Forward decls — defined below alongside store resolution. */ +static const char *cache_dir(char *buf, size_t bufsz); +static bool is_project_db_file(const char *name, size_t len); +bool cbm_validate_project_name(const char *project); + +/* #1025: agents naturally pass the repo FOLDER name ("codebase-memory-mcp"), + * but indexed project names derive from the full path + * (E:\project\graph\x -> "E-project-graph-x"), so the exact lookup fails + * while list_projects clearly shows the project. When no .db exists, + * scan cache-dir FILENAMES for a segment-aligned tail match ("-.db"): + * exactly one match adopts the full name; zero or several keep the original so + * the existing not-found error (which lists all candidates) fires. Filename- + * level only — internal-name drift stays #704's fallback in resolve_store. */ +static char *resolve_project_tail(char *project) { + if (!project || !cbm_validate_project_name(project)) { + return project; + } + char dir[CBM_SZ_1K]; + cache_dir(dir, sizeof(dir)); + char exact[CBM_SZ_2K]; + snprintf(exact, sizeof(exact), "%s/%s.db", dir, project); + if (cbm_file_exists(exact)) { + return project; /* exact name — untouched fast path */ + } + size_t plen = strlen(project); + char match[CBM_SZ_1K] = ""; + int matches = 0; + cbm_dir_t *d = cbm_opendir(dir); + if (!d) { + return project; + } + cbm_dirent_t *entry; + while ((entry = cbm_readdir(d)) != NULL) { + const char *n = entry->name; + size_t len = strlen(n); + if (!is_project_db_file(n, len)) { + continue; + } + size_t stem_len = len - MCP_DB_EXT; /* strip ".db" */ + if (stem_len <= plen + 1 || stem_len >= sizeof(match)) { + continue; + } + if (n[stem_len - plen - 1] != '-' || strncmp(n + stem_len - plen, project, plen) != 0) { + continue; + } + matches++; + if (matches > 1) { + break; /* ambiguous — keep the original name */ + } + memcpy(match, n, stem_len); + match[stem_len] = '\0'; + } + cbm_closedir(d); + if (matches == 1) { + cbm_log_info("mcp.project_tail_resolved", "passed", project, "resolved", match); + free(project); + return heap_strdup(match); + } + return project; +} + /* Resolve the project argument, accepting the canonical "project" key plus the * aliases a caller naturally reaches for (#640): list_projects surfaces the * field as "name" and the not-found hint says "pass the project name", so @@ -870,7 +931,7 @@ static char *get_project_arg(const char *args_json) { if (!p) { p = cbm_mcp_get_string_arg(args_json, "projectName"); } - return normalize_project_arg(p); + return resolve_project_tail(normalize_project_arg(p)); } int cbm_mcp_get_int_arg(const char *args_json, const char *key, int default_val) { diff --git a/src/watcher/watcher.c b/src/watcher/watcher.c index 396964208..5459503a7 100644 --- a/src/watcher/watcher.c +++ b/src/watcher/watcher.c @@ -7,9 +7,15 @@ * * Per-project state tracks: * - Last git HEAD hash (detects commits, checkout, pull) + * - Dirty-state signature (#937): porcelain entries + per-file size/mtime, + * so a persistently dirty tree reindexes once per distinct state instead + * of on every poll (write amplification) * - Last poll time + adaptive interval * - Whether the project is a git repo * + * Baselines are committed only after a successful reindex; busy-skips and + * failed runs leave them untouched so the change is retried, never lost. + * * Adaptive interval: 5s base + 1s per 500 files, capped at 60s. * Matches the Go watcher's `pollInterval()` logic. */ @@ -38,7 +44,7 @@ typedef struct { char *project_name; char *root_path; - char last_head[CBM_SZ_64]; /* git HEAD hash */ + char last_head[CBM_SZ_64]; /* git HEAD hash (committed baseline) */ bool is_git; /* false → skip polling */ bool baseline_done; /* true after first poll */ int missing_root_count; /* consecutive polls where root was missing (ENOENT/ENOTDIR) */ @@ -46,6 +52,14 @@ typedef struct { int file_count; /* approximate, for interval calc */ int interval_ms; /* adaptive poll interval */ int64_t next_poll_ns; /* next poll time (monotonic ns) */ + /* Dirty-state signature (#937): a persistently dirty worktree must + * reindex once per DISTINCT dirty state, not on every poll. Baselines + * are committed only after a SUCCESSFUL reindex (busy-skips and failed + * runs retry); check_changes stages its observations in the pending_* + * fields. 0 = clean tree. */ + uint64_t last_dirty_sig; /* committed dirty-state signature */ + uint64_t pending_dirty_sig; /* observed at check time */ + char pending_head[CBM_SZ_64]; /* HEAD observed at check time */ } project_state_t; /* ── Watcher struct ─────────────────────────────────────────────── */ @@ -150,64 +164,144 @@ static int git_head(const char *root_path, char *out, size_t out_size) { return CBM_NOT_FOUND; } -/* Returns true if working tree has changes (modified, untracked, etc.). - * Also checks submodules via `git submodule foreach` to detect uncommitted - * changes inside submodules that `git status` alone would not report. */ -static bool git_is_dirty(const char *root_path) { +/* ── Dirty-state signature (#937) ───────────────────────────────── */ + +#define SIG_FNV_OFFSET 1469598103934665603ULL +#define SIG_FNV_PRIME 1099511628211ULL + +static uint64_t sig_fold(uint64_t h, const void *data, size_t len) { + const unsigned char *p = data; + for (size_t i = 0; i < len; i++) { + h ^= p[i]; + h *= SIG_FNV_PRIME; + } + return h; +} + +/* Platform-portable mtime_ns (mirrors pipeline_incremental.c). */ +static int64_t sig_stat_mtime_ns(const struct stat *st) { +#ifdef __APPLE__ + return ((int64_t)st->st_mtimespec.tv_sec * NS_PER_SEC) + (int64_t)st->st_mtimespec.tv_nsec; +#elif defined(_WIN32) + return (int64_t)st->st_mtime * NS_PER_SEC; +#else + return ((int64_t)st->st_mtim.tv_sec * NS_PER_SEC) + (int64_t)st->st_mtim.tv_nsec; +#endif +} + +/* Fold a listed path's (size, mtime) into the signature so an in-place edit + * of an already-dirty file still produces a new signature. A failed stat + * (deleted file, quoting artifact) degrades to the entry text alone — the + * deletion itself is represented by the porcelain status. */ +static uint64_t sig_fold_path_stat(uint64_t h, const char *root_path, const char *rel) { + char abs[CBM_SZ_4K]; + snprintf(abs, sizeof(abs), "%s/%s", root_path, rel); + struct stat st; + if (stat(abs, &st) == 0) { + int64_t mt = sig_stat_mtime_ns(&st); + int64_t sz = (int64_t)st.st_size; + h = sig_fold(h, &mt, sizeof(mt)); + h = sig_fold(h, &sz, sizeof(sz)); + } + return h; +} + +/* Signature of the current dirty state: FNV-1a over the entries of + * `git status --porcelain -uall -z` plus each listed path's (size, mtime). + * Returns 0 for a clean tree. Two polls over an untouched dirty tree yield + * the same value; editing a dirty file, adding/removing one, or reverting + * the tree to clean each yield a new one. -uall lists files inside + * untracked directories individually (a nested addition under `?? dir/` + * would otherwise be invisible); -z gives unquoted NUL-separated paths that + * hash identically across polls and stat cleanly. */ +static uint64_t git_dirty_signature(const char *root_path) { char cmd[CBM_SZ_1K]; - snprintf(cmd, sizeof(cmd), - "git --no-optional-locks -C \"%s\" status --porcelain " - "--untracked-files=normal 2>%s", + snprintf(cmd, sizeof(cmd), "git --no-optional-locks -C \"%s\" status --porcelain -uall -z 2>%s", root_path, WATCHER_NULDEV); FILE *fp = cbm_popen(cmd, "r"); if (!fp) { - return false; + return 0; } - char line[CBM_SZ_256]; - bool dirty = false; - if (fgets(line, sizeof(line), fp)) { - size_t len = strlen(line); - while (len > 0 && (line[len - SKIP_ONE] == '\n' || line[len - SKIP_ONE] == '\r')) { - line[--len] = '\0'; + uint64_t h = SIG_FNV_OFFSET; + bool any = false; + char entry[CBM_SZ_4K]; + size_t elen = 0; + bool overflow = false; + /* Rename/copy entries are followed by a second NUL token (the origin + * path) that carries no XY prefix — fold it as text, don't stat it. */ + bool origin_token = false; + int c; + for (;;) { + c = fgetc(fp); + if (c != EOF && c != '\0') { + if (elen + 1 < sizeof(entry)) { + entry[elen] = (char)c; + } else { + overflow = true; /* keep hashing prefix; skip stat for entry */ + } + elen++; + continue; } - if (len > 0) { - dirty = true; + size_t stored = elen < sizeof(entry) ? elen : sizeof(entry) - 1; + entry[stored] = '\0'; + if (stored > 0) { + any = true; + h = sig_fold(h, entry, stored); + h = sig_fold(h, "", 1); /* token separator */ + if (origin_token) { + origin_token = false; + } else if (!overflow && stored > 3 && entry[2] == ' ') { + if (entry[0] == 'R' || entry[0] == 'C') { + origin_token = true; + } + h = sig_fold_path_stat(h, root_path, entry + 3); + } + } + elen = 0; + overflow = false; + if (c == EOF) { + break; } } cbm_pclose(fp); - if (dirty) { - return true; - } - #if !defined(_WIN32) - /* Check submodules: uncommitted changes inside a submodule are invisible - * to the parent's git status. Use `git submodule foreach` as a portable - * fallback (Apple Git lacks --recurse-submodules). POSIX-only: foreach takes - * an inner shell command that cmd.exe cannot pass intact; the parent-repo - * status check above already covers the common (non-submodule) case. */ + /* Submodules (POSIX-only, mirroring git_is_dirty): fold each submodule's + * porcelain output with paths re-rooted at the superproject so they stat + * correctly. Line mode here (foreach output is line-oriented); rename + * lines fail the stat and degrade to text-only. */ snprintf(cmd, sizeof(cmd), "git --no-optional-locks -C '%s' submodule foreach --quiet --recursive " - "'git status --porcelain --untracked-files=normal 2>/dev/null' " - "2>/dev/null", + "'git status --porcelain -uall 2>/dev/null | " + "sed -e \"s@^\\(..\\) @\\1 $displaypath/@\"' 2>/dev/null", root_path); fp = cbm_popen(cmd, "r"); - if (!fp) { - return false; - } - if (fgets(line, sizeof(line), fp)) { - size_t len = strlen(line); - while (len > 0 && (line[len - SKIP_ONE] == '\n' || line[len - SKIP_ONE] == '\r')) { - line[--len] = '\0'; - } - if (len > 0) { - dirty = true; + if (fp) { + char line[CBM_SZ_4K]; + while (fgets(line, sizeof(line), fp)) { + size_t len = strlen(line); + while (len > 0 && (line[len - SKIP_ONE] == '\n' || line[len - SKIP_ONE] == '\r')) { + line[--len] = '\0'; + } + if (len == 0) { + continue; + } + any = true; + h = sig_fold(h, line, len); + h = sig_fold(h, "", 1); + if (len > 3 && line[2] == ' ') { + h = sig_fold_path_stat(h, root_path, line + 3); + } } + cbm_pclose(fp); } - cbm_pclose(fp); #endif - return dirty; + + if (!any) { + return 0; + } + return h ? h : 1; /* reserve 0 for "clean" */ } /* Count tracked files via git ls-files */ @@ -497,6 +591,11 @@ static void init_baseline(project_state_t *s) { if (s->is_git) { git_head(s->root_path, s->last_head, sizeof(s->last_head)); + /* last_dirty_sig stays 0 ("clean known"): a tree that is ALREADY + * dirty at baseline reindexes once on the first poll — the watcher + * cannot know whether that state made it into the DB (e.g. server + * restart with a stale artifact). At-least-once, then the signature + * gates further polls (#937). */ s->file_count = git_file_count(s->root_path); s->interval_ms = cbm_watcher_poll_interval_ms(s->file_count); cbm_log_info("watcher.baseline", "project", s->project_name, "strategy", "git", "files", @@ -508,25 +607,41 @@ static void init_baseline(project_state_t *s) { s->next_poll_ns = now_ns() + ((int64_t)s->interval_ms * US_PER_MS); } -/* Check if a project has changes. Returns true if reindex needed. */ +/* Check if a project has changes. Returns true if reindex needed. + * Must NOT mutate the committed baselines (last_head, last_dirty_sig): + * poll_project commits them only after a SUCCESSFUL reindex so that + * busy-skips and failed runs retry instead of silently losing the change + * (#937). Observations are staged in the pending_* fields. */ static bool check_changes(project_state_t *s) { if (!s->is_git) { return false; } - /* Check HEAD movement */ + bool changed = false; + + /* Check HEAD movement (commit, checkout, pull) */ + s->pending_head[0] = '\0'; char head[CBM_SZ_64] = {0}; if (git_head(s->root_path, head, sizeof(head)) == 0) { - if (s->last_head[0] != '\0' && strcmp(head, s->last_head) != 0) { - /* HEAD moved — commit, checkout, pull */ - strncpy(s->last_head, head, sizeof(s->last_head) - 1); - return true; + if (s->last_head[0] == '\0') { + /* First observed HEAD: adopt as baseline, not a change. */ + snprintf(s->last_head, sizeof(s->last_head), "%s", head); + } else if (strcmp(head, s->last_head) != 0) { + changed = true; } - strncpy(s->last_head, head, sizeof(s->last_head) - 1); + snprintf(s->pending_head, sizeof(s->pending_head), "%s", head); } - /* Check working tree */ - return git_is_dirty(s->root_path); + /* Working tree: reindex only when the DIRTY STATE ITSELF changed — + * a persistently dirty tree polled while idle must not re-trigger + * full reindex/write cycles (#937 write amplification). */ + uint64_t sig = git_dirty_signature(s->root_path); + if (sig != s->last_dirty_sig) { + changed = true; + } + s->pending_dirty_sig = sig; + + return changed; } /* Context for poll_once foreach callback */ @@ -637,11 +752,20 @@ static void poll_project(const char *key, void *val, void *ud) { int rc = ctx->w->index_fn(s->project_name, s->root_path, ctx->w->user_data); if (rc == 0) { ctx->reindexed++; - /* Update HEAD after successful reindex */ - git_head(s->root_path, s->last_head, sizeof(s->last_head)); + /* Commit the baselines OBSERVED AT CHECK TIME — the state whose + * reindex just succeeded. A commit/edit landing during the + * reindex is deliberately not absorbed: the next poll sees it + * as a new delta (at-least-once, never lost). */ + if (s->pending_head[0] != '\0') { + snprintf(s->last_head, sizeof(s->last_head), "%s", s->pending_head); + } + s->last_dirty_sig = s->pending_dirty_sig; /* Refresh file count for interval */ s->file_count = git_file_count(s->root_path); s->interval_ms = cbm_watcher_poll_interval_ms(s->file_count); + } else if (rc > 0) { + /* Busy-skip: baseline stays uncommitted, next poll retries. */ + cbm_log_info("watcher.index.retry", "project", s->project_name); } else { cbm_log_warn("watcher.index.err", "project", s->project_name); } diff --git a/src/watcher/watcher.h b/src/watcher/watcher.h index 8b57466ac..096d4e4a2 100644 --- a/src/watcher/watcher.h +++ b/src/watcher/watcher.h @@ -22,7 +22,11 @@ typedef struct cbm_watcher cbm_watcher_t; /* ── Index callback ─────────────────────────────────────────────── */ -/* Called when file changes are detected. Return 0 on success, -1 on error. +/* Called when file changes are detected. Return 0 on success, a POSITIVE + * value when the reindex was skipped and should be retried on the next poll + * (e.g. another pipeline holds the lock), negative on error. Only a 0 return + * commits the watcher's change baselines — a skipped or failed reindex keeps + * the change pending so it is retried, never silently lost (#937). * project_name: project identifier * root_path: absolute path to the repository root */ typedef int (*cbm_index_fn)(const char *project_name, const char *root_path, void *user_data); diff --git a/tests/repro/repro_issue964.c b/tests/repro/repro_issue964.c new file mode 100644 index 000000000..f0b2692a7 --- /dev/null +++ b/tests/repro/repro_issue964.c @@ -0,0 +1,121 @@ +/* + * repro_issue964.c — Reproduce-first case for OPEN bug #964. + * + * Issue: #964 — "C/C++ #include not mapped as IMPORTS edges — expected or a + * bug?" (triaged as a bug: header files look disconnected; the reporter's + * zero-inbound query returns 194 of 214 files on a PlatformIO project). + * + * Root cause (deeper than the report guesses): + * cbm_pipeline_fqn_compute (src/pipeline/fqn.c) strips the file extension, + * so a header and its same-stem source collide on BOTH derived QNs: + * NodeController.h -> module "proj.NodeController", + * file "proj.NodeController.__file__" + * NodeController.cpp -> module "proj.NodeController", + * file "proj.NodeController.__file__" + * Node upserts match by QN, so the header's File node is merged into the + * source's — the header has NO node of its own in the graph at all. The + * `#include "NodeController.h"` import then resolves to the shared module + * (named after the .cpp), and the header can never receive an inbound + * IMPORTS edge. Headers without a same-stem sibling keep a node but the + * include-edge targeting still lands on the module, not the header file. + * + * NOTE: the module-QN unification itself is (at least partly) load-bearing + * for C/C++ — header declarations and source definitions sharing a module + * QN is what lets cross-file call resolution join them. The fix must give + * headers their own FILE identity + point include edges at it WITHOUT + * splitting the shared module QN. Both File-node creation sites (pipeline.c + * full build; pipeline_incremental.c changed-file re-creation) must agree, + * and the incremental site is concurrently being fixed by PR #995 (#994) — + * implement this fix on top of that landing, never in parallel with it. + * + * Expected (correct) behaviour: + * 1. NodeController.h has its own File node (distinct from the .cpp's). + * 2. main.cpp's `#include "NodeController.h"` produces an IMPORTS edge + * whose target is the HEADER's node, so the header is not disconnected. + * + * Why RED on current code: the header's File node does not exist (merged by + * QN collision), so both assertions fail. + */ + +#include +#include "test_framework.h" +#include "repro_harness.h" + +#include +#include +#include + +/* True if a File-labelled node with exactly `name` exists. */ +static int r964_file_node_exists(cbm_store_t *store, const char *project, const char *name) { + cbm_node_t *nodes = NULL; + int count = 0; + if (cbm_store_find_nodes_by_label(store, project, "File", &nodes, &count) != CBM_STORE_OK) + return 0; + int found = 0; + for (int i = 0; i < count; i++) { + if (nodes[i].name && strcmp(nodes[i].name, name) == 0) + found = 1; + } + cbm_store_free_nodes(nodes, count); + return found; +} + +/* Count inbound IMPORTS edges whose TARGET node name is exactly `name`. */ +static int r964_inbound_imports(cbm_store_t *store, const char *project, const char *name) { + cbm_edge_t *edges = NULL; + int n = 0; + if (cbm_store_find_edges_by_type(store, project, "IMPORTS", &edges, &n) != CBM_STORE_OK) + return -1; + int hits = 0; + for (int i = 0; i < n; i++) { + cbm_node_t tgt; + if (cbm_store_find_node_by_id(store, edges[i].target_id, &tgt) != CBM_STORE_OK) + continue; + if (tgt.name && strcmp(tgt.name, name) == 0) + hits++; + cbm_node_free_fields(&tgt); + } + cbm_store_free_edges(edges, n); + return hits; +} + +TEST(repro_issue964_header_has_node_and_inbound_import) { + static const RFile files[] = { + {"NodeController.h", "#pragma once\n" + "class NodeController {\n" + "public:\n" + " void run();\n" + "};\n"}, + {"NodeController.cpp", "#include \"NodeController.h\"\n" + "void NodeController::run() {}\n"}, + {"main.cpp", "#include \"NodeController.h\"\n" + "#include \n" + "\n" + "int main() {\n" + " NodeController c;\n" + " c.run();\n" + " return 0;\n" + "}\n"}}; + + RProj lp; + cbm_store_t *store = rh_index_files(&lp, files, 3); + ASSERT_NOT_NULL(store); + + int header_node = r964_file_node_exists(store, lp.project, "NodeController.h"); + int header_inbound = r964_inbound_imports(store, lp.project, "NodeController.h"); + if (!header_node || header_inbound < 1) { + fprintf(stderr, + " [964] FAIL header_file_node=%d inbound_imports=%d (header merged into " + "same-stem .cpp by extension-stripped QN collision)\n", + header_node, header_inbound); + } + ASSERT_TRUE(header_node); /* header must keep its own File node */ + ASSERT_TRUE(header_inbound >= 1); /* #include must land an edge on it */ + + rh_cleanup(&lp, store); + PASS(); +} + +SUITE(repro_issue964) { + RUN_TEST(repro_issue964_header_has_node_and_inbound_import); +} diff --git a/tests/repro/repro_main.c b/tests/repro/repro_main.c index 41330d8e3..fe0ac29f4 100644 --- a/tests/repro/repro_main.c +++ b/tests/repro/repro_main.c @@ -82,6 +82,7 @@ extern void suite_repro_issue363(void); extern void suite_repro_issue581(void); extern void suite_repro_issue787(void); extern void suite_repro_issue842(void); +extern void suite_repro_issue964(void); /* NEW bugs found by the discovery sweep */ extern void suite_repro_new_ts_class_field_arrow(void); extern void suite_repro_new_py_tuple_unpack(void); @@ -168,6 +169,7 @@ int main(void) { RUN_SUITE(repro_issue581); RUN_SUITE(repro_issue787); RUN_SUITE(repro_issue842); + RUN_SUITE(repro_issue964); RUN_SUITE(repro_invariant_calls); RUN_SUITE(repro_invariant_graph); RUN_SUITE(repro_invariant_breadth); diff --git a/tests/test_extraction.c b/tests/test_extraction.c index f8228e46a..686691974 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -3115,6 +3115,74 @@ TEST(complexity_go_method_receiver_self_recursion) { PASS(); } +/* #876: the delegation/store bucket left open after #599 — same-named calls + * whose receiver is a CALL RESULT (_get_store().get), a module singleton + * (_default.check), or a local cursor inside a loop (cur.execute) are not + * self-recursion. All three are the reporter's exact v0.8.1 false positives; + * fixed by the receiver-aware narrowing (87091ed), guarded here so the + * bucket never regresses. */ +TEST(complexity_delegation_receivers_not_recursive_issue876) { + /* Store wrapper: _get_store().get() inside get — receiver is the call + * result, a different object. */ + CBMFileResult *r = extract("def _get_store():\n" + " return object()\n" + "\n" + "def get(collection, record_id):\n" + " rec = _get_store().get(collection, record_id)\n" + " return rec\n", + CBM_LANG_PYTHON, "t", "soil.py"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + const CBMDefinition *d = find_def(r, "get"); + ASSERT_NOT_NULL(d); + ASSERT_FALSE(d->is_recursive); /* _get_store() result is not this fn */ + ASSERT_FALSE(d->unguarded_recursion); + cbm_free_result(r); + + /* Module-singleton delegation: _default.check() inside check. */ + r = extract("class _Checker:\n" + " def check(self, app_id, tool_name):\n" + " return (True, \"ok\")\n" + "\n" + "_default = _Checker()\n" + "\n" + "def check(app_id, tool_name):\n" + " return _default.check(app_id, tool_name)\n", + CBM_LANG_PYTHON, "t", "gleipnir.py"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + /* The module-level wrapper (not the method) must stay clean. */ + d = NULL; + for (int i = 0; i < r->defs.count; i++) { + if (r->defs.items[i].name && strcmp(r->defs.items[i].name, "check") == 0 && + strcmp(r->defs.items[i].label, "Function") == 0) { + d = &r->defs.items[i]; + } + } + ASSERT_NOT_NULL(d); + ASSERT_FALSE(d->is_recursive); /* _default is not this fn */ + ASSERT_FALSE(d->unguarded_recursion); + cbm_free_result(r); + + /* Same-named method on a local, called inside a loop (willow.nuke case): + * neither recursive nor recursion_in_loop. */ + r = extract("def execute(conn, paths):\n" + " for p in paths:\n" + " cur = conn.cursor()\n" + " cur.execute(\"DELETE FROM t WHERE p = %s\", (p,))\n" + " return True\n", + CBM_LANG_PYTHON, "t", "nuke.py"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + d = find_def(r, "execute"); + ASSERT_NOT_NULL(d); + ASSERT_FALSE(d->is_recursive); /* cur is not this fn */ + ASSERT_FALSE(d->unguarded_recursion); + ASSERT_FALSE(d->recursion_in_loop); + cbm_free_result(r); + PASS(); +} + /* Deep chained member access + parameter count structure smells. */ TEST(complexity_access_depth_and_params) { CBMFileResult *r = extract("package p\n" @@ -3321,6 +3389,64 @@ TEST(extract_js_member_call_flags_is_method) { PASS(); } +/* #961: a C function whose body braces are split across #ifdef/#else + * branches (one open brace per branch, a single shared close) parses with + * an ERROR region on the raw source — both branches are present at once — + * and the defs walk silently dropped the function while its callers stayed + * (cbm_path_within_root, handle_process_kill). The preprocessed second + * pass (simplecpp picks one branch, same-file token lines stay aligned) + * must recover the definition with its original line. */ +TEST(extract_c_ifdef_split_brace_fn_recovered_issue961) { + CBMFileResult *r = extract("static int a(void) { return 1; }\n" + "static int b(void) { return 2; }\n" + "int split_brace_fn(int x) {\n" + "#ifdef _WIN32\n" + " if (a() && b()) {\n" + "#else\n" + " if (a() || b()) {\n" + "#endif\n" + " x += 1;\n" + " }\n" + " return x;\n" + "}\n" + "int after_fn(int x) { return x; }\n", + CBM_LANG_C, "t", "split.c"); + ASSERT_NOT_NULL(r); + const CBMDefinition *d = find_def(r, "split_brace_fn"); + if (!d) { + fprintf(stderr, " [961] FAIL split_brace_fn dropped (defs walk lost the " + "#ifdef-split function)\n"); + } + ASSERT_NOT_NULL(d); + ASSERT_EQ((int)d->start_line, 3); + /* Error recovery must stay localized: neighbours extract either way. */ + ASSERT_NOT_NULL(find_def(r, "a")); + ASSERT_NOT_NULL(find_def(r, "after_fn")); + cbm_free_result(r); + PASS(); +} + +/* #961 inverse guard: a clean C file must not gain duplicate or phantom + * defs from the recovery path (it only engages on raw-parse ERROR regions). */ +TEST(extract_c_clean_file_no_recovery_duplicates_issue961) { + CBMFileResult *r = extract("#ifdef _WIN32\n" + "static int w(void) { return 1; }\n" + "#else\n" + "static int u(void) { return 2; }\n" + "#endif\n" + "int use(int x) { return x; }\n", + CBM_LANG_C, "t", "clean.c"); + ASSERT_NOT_NULL(r); + int use_count = 0; + for (int i = 0; i < r->defs.count; i++) { + if (r->defs.items[i].name && strcmp(r->defs.items[i].name, "use") == 0) + use_count++; + } + ASSERT_EQ(use_count, 1); + cbm_free_result(r); + PASS(); +} + /* #668: walk_defs used a fixed `walk_defs_frame_t stack[4096]` — a ~160 KB * C-stack frame that overflowed small thread stacks (the reporter's crash was in * the "definitions pass" on a large SQL file), and whose `top < 4096` push guards @@ -3736,7 +3862,10 @@ SUITE(extraction) { RUN_TEST(complexity_self_receiver_still_recursive); RUN_TEST(complexity_chained_receiver_not_self); RUN_TEST(complexity_go_method_receiver_self_recursion); + RUN_TEST(complexity_delegation_receivers_not_recursive_issue876); RUN_TEST(complexity_access_depth_and_params); + RUN_TEST(extract_c_ifdef_split_brace_fn_recovered_issue961); + RUN_TEST(extract_c_clean_file_no_recovery_duplicates_issue961); RUN_TEST(walk_defs_no_truncation_over_4096_issue668); RUN_TEST(extract_rust_test_attr_marks_is_test_issue855); diff --git a/tests/test_lang_contract.c b/tests/test_lang_contract.c index f9f91066f..b71a39279 100644 --- a/tests/test_lang_contract.c +++ b/tests/test_lang_contract.c @@ -1202,6 +1202,68 @@ TEST(contract_edge_imports_alias_resolves_real_file_issue767) { PASS(); } +/* True if some CALLS edge's TARGET node carries `label` and a QN ending with + * `qn_suffix` — i.e. the call resolved to that specific definition. */ +static int calls_edge_targets(cbm_store_t *store, const char *project, const char *label, + const char *qn_suffix) { + cbm_edge_t *edges = NULL; + int n = 0; + if (cbm_store_find_edges_by_type(store, project, "CALLS", &edges, &n) != CBM_STORE_OK) + return 0; + int found = 0; + size_t sl = strlen(qn_suffix); + for (int i = 0; i < n && !found; i++) { + cbm_node_t tgt; + if (cbm_store_find_node_by_id(store, edges[i].target_id, &tgt) != CBM_STORE_OK) + continue; + const char *qn = tgt.qualified_name; + if (qn && tgt.label && strcmp(tgt.label, label) == 0) { + size_t ql = strlen(qn); + if (ql >= sl && strcmp(qn + ql - sl, qn_suffix) == 0) + found = 1; + } + cbm_node_free_fields(&tgt); + } + cbm_store_free_edges(edges, n); + return found; +} + +/* #871: a CommonJS require() binding shadowed call resolution. `const doThing + * = require("../bs/doThing")` emitted a Variable def for `doThing` in the + * importing module, so the call `doThing(...)` resolved to that same-module + * Variable (an import ALIAS, not a definition) instead of following the + * recorded import to the exported function — which stayed at zero inbound + * CALLS (the reporter's disconnected-node symptom). ESM `import` bindings + * never materialize as Variables; the identifier-bound require form must + * behave identically: no shadowing Variable, CALLS edge lands on the real + * Function. Fixture = the reporter's exact two-file repro. */ +TEST(contract_edge_commonjs_require_call_resolves_issue871) { + LangProj lp; + static const LangFile f[] = { + {"src/bs/doThing.js", + "module.exports = async function doThing({ id }) {\n return id;\n};\n"}, + {"src/mutations/doThing.js", + "const doThing = require(\"../bs/doThing\");\n\n" + "module.exports = async (parent, args) => {\n return doThing({ id: args.id });\n};\n"}}; + cbm_store_t *store = lang_index_files(&lp, f, 2); + ASSERT_TRUE(store != NULL); + /* The call resolves THROUGH the require to the exported function. */ + int resolved = calls_edge_targets(store, lp.project, "Function", ".bs.doThing.doThing"); + /* The alias must not swallow the call: no CALLS edge may terminate on a + * Variable in the importing module (ESM parity: the binding is an import, + * not a definition). */ + int shadowed = calls_edge_targets(store, lp.project, "Variable", ".mutations.doThing.doThing"); + if (!resolved || shadowed) { + fprintf(stderr, " [871] FAIL resolved=%d shadowed=%d (require binding must resolve to " + "the exported Function, not the local alias Variable)\n", + resolved, shadowed); + } + ASSERT_TRUE(resolved); + ASSERT_TRUE(!shadowed); + lang_cleanup(&lp, store); + PASS(); +} + /* DEPENDS_ON — Helm Chart.yaml `dependencies:` -> per-dependency Chart node. * Basename must be exactly "Chart.yaml"; pass_k8s runs in both pipeline paths. */ TEST(contract_edge_depends_on) { @@ -1411,6 +1473,7 @@ SUITE(lang_contract) { RUN_TEST(contract_edge_workspaces_imports_issue408); RUN_TEST(contract_edge_imports_alias_no_phantom_folder_edge_issue767); RUN_TEST(contract_edge_imports_alias_resolves_real_file_issue767); + RUN_TEST(contract_edge_commonjs_require_call_resolves_issue871); RUN_TEST(contract_edge_depends_on); RUN_TEST(contract_edge_parallel_service_edges); RUN_TEST(contract_edge_file_changes_with); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index ccbb93ad4..0f88f68c4 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -1636,6 +1636,105 @@ TEST(tool_search_graph_accepts_project_name_alias_issue640) { PASS(); } +/* #1025: agents pass the repo FOLDER name ("codebase-memory-mcp"), but + * indexed project names derive from the full path + * (E:\project\graph\x -> "E-project-graph-x"), so exact lookup fails with + * "project not found" while list_projects clearly shows the project. A + * passed name that matches exactly ONE indexed project as a segment-aligned + * tail ("-" suffix) must resolve to it; zero or several matches keep + * the existing error. Runs against real cache-dir .db files (the resolution + * scans filenames), so this test indexes real fixtures under an overridden + * CBM_CACHE_DIR. */ +static void i1025_write_repo(const char *dir, const char *fn_name) { + char path[CBM_SZ_512]; + snprintf(path, sizeof(path), "%s/mod.py", dir); + FILE *f = fopen(path, "w"); + if (!f) + return; + fprintf(f, "def %s(x):\n return x + 1\n", fn_name); + fclose(f); +} + +TEST(tool_project_arg_resolves_unique_tail_issue1025) { + char repo_a[CBM_SZ_256]; + char repo_b[CBM_SZ_256]; + char repo_c[CBM_SZ_256]; + char cache[CBM_SZ_256]; + snprintf(repo_a, sizeof(repo_a), "/tmp/cbm-i1025a-XXXXXX"); + snprintf(repo_b, sizeof(repo_b), "/tmp/cbm-i1025b-XXXXXX"); + snprintf(repo_c, sizeof(repo_c), "/tmp/cbm-i1025c-XXXXXX"); + snprintf(cache, sizeof(cache), "/tmp/cbm-i1025d-XXXXXX"); + if (!cbm_mkdtemp(repo_a) || !cbm_mkdtemp(repo_b) || !cbm_mkdtemp(repo_c) || + !cbm_mkdtemp(cache)) { + FAIL("mkdtemp failed"); + } + const char *saved_cache = getenv("CBM_CACHE_DIR"); + char *saved_cache_copy = saved_cache ? cbm_strdup(saved_cache) : NULL; + cbm_setenv("CBM_CACHE_DIR", cache, 1); + cbm_setenv("CBM_INDEX_SUPERVISOR", "0", 1); + + i1025_write_repo(repo_a, "unique_tail_target"); + i1025_write_repo(repo_b, "amb_one"); + i1025_write_repo(repo_c, "amb_two"); + + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + ASSERT_NOT_NULL(srv); + + char args[CBM_SZ_1K]; + snprintf(args, sizeof(args), + "{\"repo_path\":\"%s\",\"name\":\"E-project-graph-suffix1025\"}", repo_a); + char *r = cbm_mcp_handle_tool(srv, "index_repository", args); + ASSERT_NOT_NULL(r); + free(r); + snprintf(args, sizeof(args), "{\"repo_path\":\"%s\",\"name\":\"F-alpha-amb1025\"}", repo_b); + r = cbm_mcp_handle_tool(srv, "index_repository", args); + ASSERT_NOT_NULL(r); + free(r); + snprintf(args, sizeof(args), "{\"repo_path\":\"%s\",\"name\":\"G-beta-amb1025\"}", repo_c); + r = cbm_mcp_handle_tool(srv, "index_repository", args); + ASSERT_NOT_NULL(r); + free(r); + + /* 1. Unique tail resolves (RED today: "project not found"). */ + r = cbm_mcp_handle_tool(srv, "search_graph", + "{\"project\":\"suffix1025\",\"name_pattern\":\".*target.*\"}"); + ASSERT_NOT_NULL(r); + if (strstr(r, "project not found")) { + fprintf(stderr, " [1025] FAIL unique tail did not resolve: %.200s\n", r); + } + ASSERT_NULL(strstr(r, "project not found")); + ASSERT_NOT_NULL(strstr(r, "unique_tail_target")); + free(r); + + /* 2. Ambiguous tail stays an error (never guess between projects). */ + r = cbm_mcp_handle_tool(srv, "search_graph", + "{\"project\":\"amb1025\",\"name_pattern\":\".*\"}"); + ASSERT_NOT_NULL(r); + ASSERT_NOT_NULL(strstr(r, "project not found")); + free(r); + + /* 3. Exact full name keeps working unchanged. */ + r = cbm_mcp_handle_tool(srv, "search_graph", + "{\"project\":\"E-project-graph-suffix1025\"," + "\"name_pattern\":\".*target.*\"}"); + ASSERT_NOT_NULL(r); + ASSERT_NULL(strstr(r, "project not found")); + free(r); + + cbm_mcp_server_free(srv); + if (saved_cache_copy) { + cbm_setenv("CBM_CACHE_DIR", saved_cache_copy, 1); + free(saved_cache_copy); + } else { + cbm_unsetenv("CBM_CACHE_DIR"); + } + th_rmtree(repo_a); + th_rmtree(repo_b); + th_rmtree(repo_c); + th_rmtree(cache); + PASS(); +} + /* Regression for #604: path scopes architecture totals and content. */ TEST(tool_get_architecture_path_scoping) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); @@ -5681,6 +5780,7 @@ SUITE(mcp) { RUN_TEST(tool_get_architecture_rejects_unknown_aspect_pr560); RUN_TEST(tool_get_architecture_accepts_project_name_alias_issue640); RUN_TEST(tool_search_graph_accepts_project_name_alias_issue640); + RUN_TEST(tool_project_arg_resolves_unique_tail_issue1025); RUN_TEST(tool_get_architecture_path_scoping); RUN_TEST(tool_query_graph_missing_query); diff --git a/tests/test_watcher.c b/tests/test_watcher.c index ffa817495..6e3c32f7b 100644 --- a/tests/test_watcher.c +++ b/tests/test_watcher.c @@ -642,6 +642,168 @@ TEST(watcher_no_change_no_reindex) { PASS(); } +/* #937: a PERSISTENTLY dirty worktree must reindex ONCE per distinct dirty + * state, not on every poll. The watcher used to treat "tree is dirty" as + * "tree changed", so an idle repo with one uncommitted file re-triggered a + * full reindex (and its DB/artifact rewrite) every poll cycle — the reported + * 1 TB/day write amplification. A dirty-state signature (porcelain entries + + * per-file size/mtime) must gate the trigger: same signature → no reindex; + * editing a dirty file again, or reverting the tree to clean, are NEW states + * that must each trigger exactly one reindex. */ +TEST(watcher_dirty_state_reindexes_once_issue937) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_amp_XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) + FAIL("cbm_mkdtemp failed"); + + if (wt_git(tmpdir, "init -q") != 0) { + th_rmtree(tmpdir); + FAIL("git init failed"); + } + { + char p[300]; + th_write_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "hello\n"); + } + wt_git(tmpdir, "add file.txt"); + wt_git(tmpdir, "commit -q -m init"); + + cbm_store_t *store = cbm_store_open_memory(); + cbm_watcher_t *w = cbm_watcher_new(store, index_callback, NULL); + + cbm_watcher_watch(w, "amp-repo", tmpdir); + index_call_count = 0; + + /* Baseline (clean tree) */ + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 0); + + /* Dirty the tree once (uncommitted modification). */ + { + char _p[1024]; + snprintf(_p, sizeof(_p), "%s/file.txt", tmpdir); + th_append_file(_p, "modified\n"); + } + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 1); /* new dirty state → one reindex */ + + /* Idle polls on the SAME dirty state must not re-trigger. */ + for (int i = 0; i < 3; i++) { + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + } + ASSERT_EQ(index_call_count, 1); /* was 4 before the fix: one per poll */ + + /* Editing the dirty file AGAIN is a new state (size changes). */ + { + char _p[1024]; + snprintf(_p, sizeof(_p), "%s/file.txt", tmpdir); + th_append_file(_p, "modified again\n"); + } + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 2); + + /* Same-state polls stay quiet again. */ + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 2); + + /* Reverting to a clean tree changes on-disk content (back to HEAD) — + * that is a new state and must reindex exactly once. */ + wt_git(tmpdir, "checkout -- file.txt"); + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 3); + + /* Stable clean tree: quiet. */ + cbm_watcher_touch(w, "amp-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 3); + + cbm_watcher_free(w); + cbm_store_close(store); + th_rmtree(tmpdir); + PASS(); +} + +/* #937 companion: a change whose reindex FAILS (or is skipped busy) must be + * retried on the next poll. The watcher used to commit the new HEAD at CHECK + * time, so a commit observed while the pipeline was busy/failing was recorded + * as seen and never indexed — a silent lost update. Baselines (HEAD and dirty + * signature) may only be committed after a SUCCESSFUL reindex. */ +static int failing_index_calls = 0; +static int failing_index_fail_first_n = 0; +static int failing_index_callback(const char *name, const char *path, void *ud) { + (void)name; + (void)path; + (void)ud; + failing_index_calls++; + if (failing_index_calls <= failing_index_fail_first_n) { + return -1; /* simulated pipeline failure */ + } + return 0; +} + +TEST(watcher_failed_reindex_retries_issue937) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_rty_XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) + FAIL("cbm_mkdtemp failed"); + + if (wt_git(tmpdir, "init -q") != 0) { + th_rmtree(tmpdir); + FAIL("git init failed"); + } + { + char p[300]; + th_write_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "hello\n"); + } + wt_git(tmpdir, "add file.txt"); + wt_git(tmpdir, "commit -q -m init"); + + cbm_store_t *store = cbm_store_open_memory(); + cbm_watcher_t *w = cbm_watcher_new(store, failing_index_callback, NULL); + + cbm_watcher_watch(w, "rty-repo", tmpdir); + failing_index_calls = 0; + failing_index_fail_first_n = 1; /* first reindex attempt fails */ + + /* Baseline (clean tree) */ + cbm_watcher_poll_once(w); + ASSERT_EQ(failing_index_calls, 0); + + /* HEAD moves (new commit). */ + { + char p[300]; + th_append_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "world\n"); + } + wt_git(tmpdir, "add file.txt"); + wt_git(tmpdir, "commit -q -m add-world"); + + /* First poll: change detected, reindex attempt FAILS. */ + cbm_watcher_touch(w, "rty-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(failing_index_calls, 1); + + /* The failed change must NOT have been recorded as seen: the next poll + * retries and succeeds. Before the fix, the new HEAD was stored at check + * time and the commit was silently lost (calls stayed at 1). */ + cbm_watcher_touch(w, "rty-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(failing_index_calls, 2); + + /* Successful reindex commits the baseline: no further triggers. */ + cbm_watcher_touch(w, "rty-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(failing_index_calls, 2); + + cbm_watcher_free(w); + cbm_store_close(store); + th_rmtree(tmpdir); + PASS(); +} + TEST(watcher_multiple_projects) { /* Create two temporary git repos */ char tmpdirA[256]; @@ -893,8 +1055,11 @@ TEST(watcher_git_removed_no_crash) { } TEST(watcher_continued_dirty) { - /* If working tree stays dirty, each poll should re-trigger reindex. - * Port of repeated git sentinel detection behavior. */ + /* A tree that STAYS dirty re-triggers only when the dirty state itself + * changes (#937): repeat polls over the untouched state are quiet, a + * further edit re-triggers, and the cleaning commit triggers once more + * (HEAD move + tree back to clean). Historically this test asserted one + * reindex per poll while dirty — that WAS the #937 write amplification. */ char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_cont_XXXXXX"); if (!cbm_mkdtemp(tmpdir)) @@ -932,7 +1097,17 @@ TEST(watcher_continued_dirty) { cbm_watcher_poll_once(w); ASSERT_EQ(index_call_count, 1); - /* Still dirty — should detect again */ + /* Still dirty but UNCHANGED — must stay quiet (#937) */ + cbm_watcher_touch(w, "cont-repo"); + cbm_watcher_poll_once(w); + ASSERT_EQ(index_call_count, 1); + + /* A further edit is a NEW dirty state — detect again */ + { + char _p[1024]; + snprintf(_p, sizeof(_p), "%s/file.txt", tmpdir); + th_append_file(_p, "dirtier\n"); + } cbm_watcher_touch(w, "cont-repo"); cbm_watcher_poll_once(w); ASSERT_EQ(index_call_count, 2); @@ -1914,6 +2089,8 @@ SUITE(watcher) { RUN_TEST(watcher_detects_dirty_worktree); RUN_TEST(watcher_detects_new_file); RUN_TEST(watcher_no_change_no_reindex); + RUN_TEST(watcher_dirty_state_reindexes_once_issue937); + RUN_TEST(watcher_failed_reindex_retries_issue937); RUN_TEST(watcher_multiple_projects); /* Non-git project */