Repository navigation
feat(mcp): report truthful index freshness from checkout evidence - #1561
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
Thank you @tmonestudio. The underlying issue is confirmed on current This is a broad correctness change despite the focused product claim: 11 files and about 1,300 added lines across git subprocess handling, MCP output, pipeline publication, store migration, and tests. It therefore needs a full storage/schema and fail-closed review rather than a quick UI pass. The current head is mergeable, but Thank you for the reproduce-first coverage and legacy-database checks. The queue is full, so detailed feedback may take some time, but this is labeled and queued. |
|
Checking in — this has been quiet since 12 August and it is still open and still wanted, so here is where it stands with the friction removed. The lint blocker is six clang-format violations in two files:
And an honest note on the rest, so the delay does not read as all yours. Even with lint green, this is not a fast merge: 11 files and ~1,300 lines spanning git subprocess handling, MCP output, pipeline publication, store migration and tests. It needs a full storage/schema and fail-closed review, and that review is ours to do and has not happened yet. Clearing lint is what lets the main test matrix actually run — it was skipped entirely on the last CI pass, so nobody has seen this change tested. The underlying problem you identified is confirmed on current If you have moved on, say so and we will take it from here rather than leaving it to age. If not, clear the lint and the matrix will finally have something to say. |
167783b to
9e9fb2f
Compare
|
An update, and a decision that went in your favour. #1727 independently added a top-level Your object stands. The reasoning is that a caller which learns why the graph is not current, and what to do about it, is better served than one that gets a single word — your One idea from that PR worth folding into yours if you touch the docs: "a dirty tree is never treated as evidence in either direction". Your implementation already behaves that way — a dirty tree cannot reach Where this stands otherwise: the storage/schema and fail-closed review you were owed is done and it passes — the three-DB-state migration (fresh, legacy-writable, legacy read-only) and the verdict ladder that never reaches Thanks for your patience — this has been open a long time, and it is the design that is being kept. |
|
Clearance is done and I've now verified this end to end on today's main rather than on the August base: trial-merged onto Two things are needed before it merges, both mechanical, and both caused by main moving under you:
Non-blocking, take or leave: the Keep the sign-offs on the rebased commits and this merges on green. Thanks for the fast turnaround on the lint fix in August and for sticking with it. |
ca7442b to
b939a2d
Compare
|
Thanks for the CI report. I traced both failures to the same underlying issue: The failure was scheduler-sensitive, not a freshness regression: the shared-package scaling test observed Fix pushed in commit Local verification on the updated source:
Please rerun the Unix shard/CI checks for |
|
Thank you for tracking down the failing shard and recording both the parallel and isolated counts. That evidence is useful. Please separate the serial-suite workaround in 6875483 from this freshness PR. The reported change from 484 to 842 nodes is a difference in graph contents, not just elapsed time; moving the suite to the quiet tail may avoid the symptom, but does not yet establish whether the cause is shared test state, extraction nondeterminism, or another defect. Please preserve the failing run and fixture details so we can investigate that discrepancy separately rather than treating a serial green run as a correctness fix. This does not withdraw our agreement on the freshness design. We also owe you the final integration review after our earlier merge commitment. Thank you for the repeated updates and your patience. |
a7941a4 to
e0b9642
Compare
Signed-off-by: tmonestudio <tmonestudio@users.noreply.github.com>
e0b9642 to
789c60c
Compare
|
Thank you for splitting the complexity-suite workaround out as we asked. The branch is now the freshness change alone, which is the version we want to merge.
One heads-up so it doesn't surprise you: a rebase produces a new head, so we re-run our security clearance and verification on it before merging. That is routine and nothing you need to do. The design agreement from 2026-09-01 (your |
#2167) The background watcher only watches the project an open MCP session is rooted in, and drops the watch when the last session for that project closes. A repository indexed with `cli index_repository` (or from a session rooted elsewhere) is never watched, but README promised "Auto-sync keeps it fresh after that" for every index_repository target and the MCP instructions said "Indexes auto-refresh". Nothing in the status output said which projects were actually watched, so a stale CLI-indexed repo looked like a watcher bug. What gets watched is unchanged. Instead: - index_status gains a `watch` object: `watched`, plus `strategy`, `poll_interval_ms` and `last_scan_at` when watched, or a `reason` (not_session_project, cli_session, auto_watch_off, watcher_disabled, not_registered, no_watcher) and a re-index hint when not. It does not touch the #1561 `freshness` key. - The watcher publishes per-project strategy, cadence and last completed scan time through atomics and exposes them via cbm_watcher_project_info(); the daemon answers index_status through a session-scoped provider. - README, docs/CONFIGURATION.md, docs/index.html, the index_status tool description and the MCP instructions now state the real scope. `watcher_enabled` already shows in `config list` since v0.11.0 (680bc72). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…a#2144) index_repository blocks until the whole index completes. MCP clients with a per-call deadline (Copilot for IntelliJ) give up on a large repository; their cancel drops the daemon job's last subscriber, the daemon cancels the worker, and every retry starts over, so the index never finishes. Add an opt-in async mode and a status query to the same tool; the default synchronous behaviour is unchanged: - async: true starts the project's index job in the daemon, or joins the one already running for it, and returns at once with its state. The application holds one subscriber reference for an async job until its terminal publish, so the request returning, the client cancelling or the starting session closing no longer cancels it. Only daemon shutdown (or the final session of a non-permanent generation) ends it. Async requests never queue behind the physical job limit; a full daemon answers busy. async/status are stripped from the worker args, so an async request coalesces with an identical synchronous one. - status: true reports the running or last job for the project (queued/running/cancelling/succeeded/failed/cancelled, started_at, finished_at, error summary) from a small per-project record kept in the daemon's job registry after the job itself is reaped. An unknown project is an error; a project indexed before this daemon generation reports idle. No freshness key is added: index_status keeps the DeusData#1561 freshness object. - async + status together, non-boolean values, and async with cross-repo-intelligence are refused. In-process servers (index worker, embedders) refuse both modes: nothing there outlives the call. - On a temporary (non-permanent) daemon, async is refused when the request arrives on the one-shot `cli` tool channel and that session is the daemon's only live session: the daemon would stop and cancel the job the moment the command exits. The error points at `daemon start` (or an open MCP session). A long-lived MCP session stays a valid host even when it is the only one, which is the IDE case this exists for; status stays allowed everywhere. The daemon lifecycle is unchanged. - New allocations go through the memory core (cbm_alloc/cbm_calloc/ cbm_mem_strdup/cbm_free); blocks returned by non-core APIs are released with safe_free, so no file's raw allocator count grows. - A cut-short synchronous call now offers the async alternative: in the cancellation text the daemon or frontend delivers (including the -32800 reply to a cancelled index_repository), and, because a timed-out client never reads that reply, as a notice on the next index_repository or status call for the project. - The tool description and schema, the CLI help and the README explain the flags, the deadline problem and the polling pattern. No server-side timeout or budget is introduced. Tests (deterministic, held fake worker, bounded observation waits): async returns before completion and status observes running then succeeded; an async job survives its session closing while a sync job in the same shape is still cancelled; validation errors; the cancellation reply and the next-call notice carry the advice; a lone one-shot client of a temporary daemon is refused async while status and an MCP session on the same daemon are allowed; in-process refusal; schema; notice helper. Refs DeusData#2031 Fixes DeusData#2144 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
A project indexed from a directory that is not a git repository was never refreshed after its first index: init_baseline classified it is_git=false and poll_project/check_changes returned early for it forever, so the graph silently went stale. The comment in init_baseline even claimed such folders were "polled as a plain directory", which was never true. Add an opt-in config key, watch_non_git (default false, read once at daemon startup like watcher_enabled). When on, a non-git root is polled on the existing adaptive cadence (cbm_watcher_poll_interval_ms) by a tree signature: the indexer's own cbm_discover walk (same skip lists, .gitignore stack and .cbmignore), sorted by path and folded over (relative path, size, mtime). A changed signature takes the existing reindex path with the same commit-after-success discipline as the #937 dirty signature. The baseline leaves the signature "unknown", so the first poll reindexes once (at-least-once, covering edits made while the daemon was down). Runaway guard: only files discovery would index enter the signature, so cbm's own .codebase-memory output (built-in skip) and the cache directory (pruned by absolute path) can never make a finished reindex look like a new change; a scratch folder under an unrelated dirty repository is polled on its own files and never inherits the ancestor's dirty state. cbm_file_info_t gains mtime_ns, filled from the stat discovery already performs, so the signature needs no second stat per file and stays UTF-8-correct on Windows (discovery's wide stat). Docs: README and docs/CONFIGURATION.md now say non-git roots are not watched by default and document watch_non_git. The staleness signal in index_status is left to #1561's freshness object. Refs #1948. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
#2167) The background watcher only watches the project an open MCP session is rooted in, and drops the watch when the last session for that project closes. A repository indexed with `cli index_repository` (or from a session rooted elsewhere) is never watched, but README promised "Auto-sync keeps it fresh after that" for every index_repository target and the MCP instructions said "Indexes auto-refresh". Nothing in the status output said which projects were actually watched, so a stale CLI-indexed repo looked like a watcher bug. What gets watched is unchanged. Instead: - index_status gains a `watch` object: `watched`, plus `strategy`, `poll_interval_ms` and `last_scan_at` when watched, or a `reason` (not_session_project, cli_session, auto_watch_off, watcher_disabled, not_registered, no_watcher) and a re-index hint when not. It does not touch the #1561 `freshness` key. - The watcher publishes per-project strategy, cadence and last completed scan time through atomics and exposes them via cbm_watcher_project_info(); the daemon answers index_status through a session-scoped provider. - README, docs/CONFIGURATION.md, docs/index.html, the index_status tool description and the MCP instructions now state the real scope. `watcher_enabled` already shows in `config list` since v0.11.0 (680bc72). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
#2167) The background watcher only watches the project an open MCP session is rooted in, and drops the watch when the last session for that project closes. A repository indexed with `cli index_repository` (or from a session rooted elsewhere) is never watched, but README promised "Auto-sync keeps it fresh after that" for every index_repository target and the MCP instructions said "Indexes auto-refresh". Nothing in the status output said which projects were actually watched, so a stale CLI-indexed repo looked like a watcher bug. What gets watched is unchanged. Instead: - index_status gains a `watch` object: `watched`, plus `strategy`, `poll_interval_ms` and `last_scan_at` when watched, or a `reason` (not_session_project, cli_session, auto_watch_off, watcher_disabled, not_registered, no_watcher) and a re-index hint when not. It does not touch the #1561 `freshness` key. - The watcher publishes per-project strategy, cadence and last completed scan time through atomics and exposes them via cbm_watcher_project_info(); the daemon answers index_status through a session-scoped provider. - README, docs/CONFIGURATION.md, docs/index.html, the index_status tool description and the MCP instructions now state the real scope. `watcher_enabled` already shows in `config list` since v0.11.0 (680bc72). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Resolve five conflicts where main and this branch added code at the same place; both sides are kept: - src/mcp/mcp.c: main's index_status watch object (DeusData#2167) next to the freshness helpers - src/pipeline/pipeline.c, pipeline_internal.h: main's resource-policy accessors next to cbm_pipeline_indexed_checkout_sha - src/store/store.c: main's edges schema probe next to the indexed_checkout_sha column probe - tests/test_git_context.c: main's inherited-git-env tests (DeusData#2003) next to the bounded worktree-status tests Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The freshness verdict reads cbm_git_worktree_status(), which spawned git with plain cbm_popen. A GIT_DIR/GIT_WORK_TREE inherited from a git hook or an editor overrides `git -C <root>`, so a clean checkout was reported stale with the other repository's changes. Every other git child already goes through cbm_popen_git since DeusData#2003; this probe now does too. worktree_status_ignores_inherited_repo_env pins it: a dirty decoy in the hook environment must not leak into a clean fixture's status. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The status samples and the stored indexed_checkout_sha were the only new raw malloc/realloc/free sites, which grew the memory-core ratchet (scripts/lint-memory-core.py) for git_context.c and store.c. They now use cbm_alloc/cbm_realloc/cbm_free and cbm_mem_strdup, with one class per pointer (OTHER for the samples, STORE for the column value). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
.clang-format sets IndentCaseLabels: false; the status_code_ok switch and a few wrapped calls in the new parser did not match it. Formatting only; no behaviour change. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @tmonestudio -- and thank you for splitting the complexity workaround out when we asked; the branch you left is exactly the freshness change we agreed to ship on 1 September. Since main had moved a long way, we finished the last mile here rather than ask you for another rebase. Your commit is untouched; we added four commits on top:
We re-ran it end to end on a real repository: clean -> current, tracked edit -> stale, new commit -> stale with the checkout mismatch, untracked-only -> unknown, re-index -> current with the new SHA. The fail-closed ladder you designed did all the work there. Thank you for sticking with this one for so long. |
|
Thanks for your patience and for the freshness design. The October 20 update records that we took over the integration work, so there is no outstanding rebase request for you. Sorry for the delay on the remaining maintainer follow-up—I’ll follow up soon. |
index_coverage_meta is one of the tables the graph digest pins column by column, so the new indexed_checkout_sha column made the digest reject every live store with a schema error: the live-schema compatibility test failed, and test-impact, which digests the graph it builds for HEAD, fell back to running every test for every change. Record the checkout in a table of its own instead: index_checkout (project primary key referencing projects ON DELETE CASCADE, indexed_checkout_sha NOT NULL). A checkout is source identity, which the graph digest deliberately does not bind, and the store already keeps other per-project data (doc_link_*, observed_*) outside the digested tables. index_coverage_meta keeps exactly the shape the digest expects, so the digest needs no change and no version bump, and a new table needs no migration of existing databases. The row is written and cleared in the same coverage transaction, so a failed replace keeps the previous identity, and it is removed with its project. The store API is unchanged: cbm_coverage_meta_t carries indexed_checkout_sha as before. Read-only opens of databases without the table report no identity, as before. store_graph_digest_indexed_checkout_identity_is_not_bound writes a store through the real coverage API with and without a checkout SHA and asserts that it still digests and that the SHA leaves the digest unchanged. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Second maintainer refresh of DeusData#1561 (truthful index_status freshness), onto main 9c19b5d, after the fix-up that keeps the indexed checkout in its own index_checkout table (a0d0521). The first refresh is b44a22c. Five files conflicted; each is resolved as main plus the branch change: - src/pipeline/pipeline.c and src/pipeline/pipeline_incremental.c (two sites): main added .unresolved_calls_complete to the coverage-meta initializers where the branch added .indexed_checkout_sha. Both fields are kept, main first. - src/store/store.h cbm_coverage_meta_t: main added unresolved_calls_complete where the branch added indexed_checkout_sha. Both fields are kept, main first. - src/store/store.c init_schema: main added the unresolved_calls_complete column where the branch closed index_coverage_meta and created index_checkout. index_coverage_meta keeps main DDL exactly, and the index_checkout table follows it. - src/store/store.c coverage replace: main moved the coverage-meta write out of cbm_store_coverage_replace_ex into coverage_write_metadata, whose caller owns the transaction and rolls it back on failure. The branch wrote or cleared the index_checkout row right after that write, so its two calls move into the helper: the upsert path returns index_checkout_write(s, project, meta->indexed_checkout_sha), the delete path returns index_checkout_write(s, project, NULL). index_checkout_write stays where the branch defined it, ahead of the new helpers. - tests/test_store_nodes.c: main added store_coverage_shadow_excludes_unresolved_calls and its helper where the branch added store_coverage_meta_indexed_checkout_sha_roundtrip_and_rollback. Both tests are kept and registered, main first. src/pipeline/pipeline_internal.h merged without a conflict: main raised CBM_SEMANTIC_INDEX_VERSION to 13 for Next.js App Router routes, in a separate hunk from the branch's cbm_pipeline_indexed_checkout_sha declaration. No semantic conflicts: - The branch bumps no version. index_checkout is created by init_schema with IF NOT EXISTS and read only when present, so main's semantic index version 13 applies unchanged. - The branch changes no existing function signature, and main changes none that the branch calls. - Every main caller of cbm_store_coverage_meta_get added since the merge base, including the new Next.js upgrade test in test_pipeline.c, releases the result with cbm_store_coverage_meta_clear exactly once, which frees the checkout string. That test passes a shallow copy of the result to cbm_store_coverage_replace_ex and never frees the copy on its own. No test call site needed adapting. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @tmonestudio, for this change and for your patience while main kept moving under it. Your freshness object and the fail-closed verdict ladder are exactly what we agreed to ship, and your tests (the store round-trip, the rollback on a failed replace, and the git-backed freshness cases) carry over unchanged. One thing first, because it changes something we said in public: on 5 September we wrote that the nullable We pushed these commits on top of your branch:
Please pull before pushing again, since the branch has new commits from us. Thank you again for the careful work and for sticking with this one for so long. |
|
Merged - thank you very much, @tmonestudio! |
What does this PR do?
Makes verbose
index_statusdistinguish the live checkout from the generation that was actually indexed.indexed_checkout_shaonly at the staged-generation publication boundarycurrent,stale, or fail-closedunknownThis is intentionally separate from #1181 and #1065.
Local verification
git_context mcp store_nodesexited 0unknown/indexed_checkout_unavailableChecklist
git commit -s)No runtime binary, cache, ACL, or corpus was modified.