diff --git a/docs/adr/ADR-056-reindex-reports-freshness-against-its-source.md b/docs/adr/ADR-056-reindex-reports-freshness-against-its-source.md new file mode 100644 index 0000000..72174e8 --- /dev/null +++ b/docs/adr/ADR-056-reindex-reports-freshness-against-its-source.md @@ -0,0 +1,104 @@ +# ADR-056: `reindex()` Reports Freshness Against the Index's Source + +**Status:** accepted +**Date:** 2026-11-02 +**Branch:** `feature/adr-056-reindex-freshness` +**Reviewer:** @edb +**Backlog:** [B-070](../backlog.md#b-070) +**Depends on:** none +**Depended on by:** none yet + +## Context + +On 2026-11-02, InventoryApp-V2 and GanttWebApp ran their first passes on the post-ADR-055 code. Each +`reindex()` result opened with a false alarm: + +``` +⚠️ INDEX STALENESS DETECTED + Last indexed at commit: e8fd91e6 + Current HEAD: 39737053 + Files changed since the index was built: [~330 paths] + Consider reindex(changed_files_only=False) for a clean rebuild. +``` + +`_reindex()` builds this block **before** the run. It compares `last_indexed_commit` with +`git rev-parse HEAD` and lists `git diff --name-only HEAD`. Both live repos index in git mode +(ADR-042): the index follows a ref (`origin/main`, `refs/code-index/target`), not the folder, and +both folders sit on feature branches. So the block compares the wrong two commits. V2's listed about +600 files. Gantt's listed about 330, and two lines further down the same result said "Nothing +changed — index is up to date", which was true. + +Three things are wrong with the block even once it compares the right commits: + +1. **It describes the state before the run.** The incremental run that follows catches up to the + target, so a list of what differed before the run is stale by the time the caller reads it. +2. **Its advice is wrong in both modes.** Since ADR-049, the incremental pass re-checks every file's + content, so `changed_files_only=False` (a wipe and full re-embed) fixes nothing the block + describes. An agent that trusts it pays for a full rebuild every time it reindexes from a branch. +3. **`index_status` already answers the question correctly.** It compares against `git_ref() or + "HEAD"` (ADR-042 §5). The same check lives in two places, and one copy is wrong. + +The server instructions (`_server_instructions()`, ADR-038) have the same root cause. They quote +`commit …, last verified …` as it was when the client connected, and Claude Code doesn't re-read +instructions during a session. Gantt's session read 86d34d5 there while `index_status` said +e8fd91e. In git mode, the "files your branch changed" hint also used that frozen commit: +`git diff --name-only 86d34d5...HEAD`. + +## Decision + +1. **Drop `_reindex()`'s pre-run staleness block.** It's replaced by one line written **after** the + run, saying whether the index now matches its target. That's the only freshness fact the caller + can act on: + - `index: e8fd91e6 (== refs/code-index/target (→ origin/integration/staging-2026-10-01); current)` + - `index: e8fd91e6 refs/code-index/target: a871f049 ⚠️ STALE (the watching server follows it)` + - Folder mode reads the same, with `HEAD` as the target. + + No line suggests `changed_files_only=False`. +2. **One definition of "the target".** `_freshness_target(cwd)` returns `(ref, target)`, where + `target` is `ref or "HEAD"`. `index_status` and `_reindex()` both use it, so the two can't + disagree again. +3. **The instructions quote nothing that goes stale.** They name the source (the folder, or the + ref with its symbolic target) and send the caller to `index_status` for the indexed commit and + freshness. In git mode, the branch hint becomes `git diff --name-only ...HEAD`. That stays + right as the ref moves, and it's what the caller means: the files their branch changed since it + left the base. The instructions no longer read the database, so the "no finished build yet" + clause goes too. It was frozen at connect time like the commit, and `index_status` reports a + missing build. + +## Consequences + +**Better:** +- A git-mode `reindex()` from a feature-branch folder no longer prints hundreds of "changed" files + or advises a full rebuild. +- The reindex result and `index_status` share one target, so they can't contradict each other. +- An agent's instructions stay true for the whole session. + +**Worse:** +- A folder-mode `reindex()` no longer lists the files that changed since the last indexed commit + before the run. The run's own output already names what it processed, so nothing is lost. +- The instructions no longer show the indexed commit at a glance. An agent that wants it calls + `index_status`, which is one cheap call that is always current. + +**Neutral:** `index_meta`'s `last_indexed_commit` (ADR-025, read by segmem) doesn't change. + +## Alternatives Considered + +| Option | Why rejected | +|--------|-------------| +| Keep the pre-run block and only switch it to `ref or "HEAD"` (B-070's first sketch) | Still reports a state the run is about to change, and still lists every file a moved ref touched. The fact worth reporting is the state after the run. | +| Delete the block and print nothing | The caller then has to call `index_status` to learn whether the run caught up. One line costs nothing. | +| Refresh the instructions when the index moves | MCP has no way to push new instructions to a connected client, and Claude Code reads them once. | +| `git diff ...HEAD` in folder mode too | Folder mode has no ref. The indexed commit is the right base there, and it's in `index_status`. | + +## Implementation Log + +- [x] `_freshness_target()`; `index_status` switched to it with no change in output +- [x] `_reindex()`: pre-run block removed; post-run freshness line +- [x] `_server_instructions()`: no commit, no database read, `...HEAD` hint, still under 1,000 characters +- [x] Tests: a git-mode reindex from a folder on another branch prints no staleness and no rebuild advice; a moved ref shows STALE; folder mode reports `HEAD`; the instructions quote no commit +- [x] Full suite and flake8 (CI's flags); MCP Inspector `tools/list --strict` on a scratch folder + +**Notes:** + + + diff --git a/src/MCPServer.py b/src/MCPServer.py index 2d2b2b8..6a4a363 100644 --- a/src/MCPServer.py +++ b/src/MCPServer.py @@ -34,36 +34,17 @@ def _server_instructions(repo_path: str | None = None) -> str: """What every connected agent is told about this index (ADR-038, B-053). - Names the folder and the commit the index was built from, read from index_meta, - so an agent working on another branch or worktree knows which hits to distrust. + Names the folder or the ref the index is built from, so an agent working on another + branch or worktree knows which hits to distrust. Quotes no commit: a client reads + this once, at connect, so a commit here goes stale while the session runs (ADR-056). Kept short: every session pays for it. """ - from index_location import git_ref, index_dir + from index_location import git_ref repo_path = repo_path or os.getcwd() - commit = verified = None try: ref = git_ref(repo_path) - db_path = os.path.join(repo_path, index_dir(), "graph.db") except ValueError: - ref, db_path = None, os.path.join(repo_path, ".code-index", "graph.db") - if os.path.exists(db_path): - try: - con = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True, timeout=2) - try: - meta = dict(con.execute( - "SELECT key, value FROM index_meta " - "WHERE key IN ('last_indexed_commit', 'last_verified_at')").fetchall()) - finally: - con.close() - commit = meta.get("last_indexed_commit") - verified = meta.get("last_verified_at") - except sqlite3.Error as exc: - # The instructions then say "no finished build yet"; say why (#54). - import sys - print(f"[MCP] could not read index_meta from {db_path}: {exc}", - file=sys.stderr, flush=True) - built = (f"commit {commit[:10]}, last verified {verified}" if commit - else "no finished build yet") + ref = None # ADR-049 (B-021 point 3): when to reach for the index instead of Grep. routing = ("For how the rest of the codebase works, where a symbol is used, what writes a " "field, or blast radius, call `semantic_code_search`, `trace_data_flow`, " @@ -76,23 +57,24 @@ def _server_instructions(repo_path: str | None = None) -> str: # ADR-042 §5: the index is a commit, read from git, not any folder. from index_location import ref_display return ( - f"This code index reflects {ref_display(repo_path, ref)} ({built}), read straight from git: not this " + f"This code index reflects {ref_display(repo_path, ref)}, read straight from git: not this " "folder's working tree, and not your branch. For any file your branch changed " - f"(git diff --name-only {commit[:10] if commit else ''}...HEAD, plus " + f"(git diff --name-only {ref}...HEAD, plus " "uncommitted edits), Read the file itself: index hits for it show the indexed " f"version, and line numbers may be off. {routing}" f"It follows {ref} on its own when that ref moves; do not call " - "`reindex` to pick up branch work. `index_status` reports freshness." + "`reindex` to pick up branch work. `index_status` reports the indexed commit " + "and freshness." ) return ( - f"This code index reflects ONE checkout: {repo_path} ({built}). " + f"This code index reflects ONE checkout: {repo_path}. " "It does not see edits in other branches or worktrees. For any file your branch " - "changed (git diff --name-only ...HEAD, plus uncommitted edits), Read " + "changed (git diff --name-only ...HEAD, plus uncommitted edits), Read " "the file itself: index hits for it show the indexed version, and line numbers may " f"be off. {routing}" "Never call `reindex` from a git worktree made for separate or parallel work; it " "refuses there, and one server per index keeps it current on its own. " - "`index_status` reports freshness." + "`index_status` reports the indexed commit and freshness." ) @@ -1739,57 +1721,57 @@ def reindex(changed_files_only: bool = True) -> str: ) from exc +def _freshness_target(cwd: str) -> tuple[str | None, str]: + """``(ref, target)``: the ref a git-mode index follows (None in folder mode), and the + revision the index is current against (``ref or "HEAD"``, ADR-042 §5). One definition + for ``index_status`` and ``reindex``, which used to disagree (ADR-056).""" + from index_location import git_ref + try: + ref = git_ref(cwd) + except ValueError: + ref = None + return ref, ref or "HEAD" + + +def _freshness_line(db_path: str) -> str: + """One line saying whether the index matches its target, or "" when git can't say.""" + import subprocess + from db import CodeDB + ref, target = _freshness_target(os.getcwd()) + try: + with CodeDB(db_path) as db: + last = db.meta_get("last_indexed_commit") + curr = subprocess.check_output( + ["git", "rev-parse", target], text=True, stderr=subprocess.DEVNULL, stdin=subprocess.DEVNULL + ).strip() + except (sqlite3.Error, FileNotFoundError, subprocess.CalledProcessError): + return "" # no index, git not installed, not a repo, or the ref is gone + if not last: + return "" + if ref is not None: + from index_location import ref_display + target = ref_display(os.getcwd(), ref) + if last == curr: + return f"index: {last[:8]} (== {target}; current)\n" + return (f"index: {last[:8]} {target}: {curr[:8]} ⚠️ STALE" + + (" (the watching server follows it)" if ref else "") + "\n") + + def _reindex(changed_files_only: bool) -> str: """The body of `reindex`. Call it holding `_reindex_lock`.""" import sys import io import os import sqlite3 - import subprocess from incremental_indexer import run_incremental, INDEX_DIR, TIER_CONFIGS from db import CodeDB print(f"\n[MCP] reindex: changed_files_only={changed_files_only}") - # Git-aware staleness report for incremental mode (ADR-025 §5). - # The last-indexed commit is now read from the index_meta table in graph.db — - # not the retired last_indexed_commit.txt, which segmem's DB-only connector could - # never see. Exceptions are handled narrowly: "git absent" (FileNotFoundError) is - # distinguished from "git ran and failed" (CalledProcessError), instead of a blanket - # swallow that hid non-git-repo, no-commits, and git-missing alike. - _stale_warning = "" + # ADR-056: freshness is reported after the run, against the index's own target. The + # pre-run "INDEX STALENESS DETECTED" block compared the folder's HEAD in git mode too, + # and advised a full rebuild that the incremental run makes pointless. db_path = os.path.join(INDEX_DIR, "graph.db") - if changed_files_only: - _last_hash = None - if os.path.exists(db_path): - try: - with CodeDB(db_path) as _meta_db: - _last_hash = _meta_db.meta_get("last_indexed_commit") - except sqlite3.Error: - _last_hash = None - if _last_hash: - try: - _curr_hash = subprocess.check_output( - ["git", "rev-parse", "HEAD"], text=True, stderr=subprocess.DEVNULL, stdin=subprocess.DEVNULL - ).strip() - if _last_hash != _curr_hash: - _changed = subprocess.check_output( - ["git", "diff", "--name-only", _last_hash, "HEAD"], - text=True, stderr=subprocess.DEVNULL, stdin=subprocess.DEVNULL - ).strip() - if _changed: - _stale_warning = ( - f"⚠️ INDEX STALENESS DETECTED\n" - f" Last indexed at commit: {_last_hash[:8]}\n" - f" Current HEAD: {_curr_hash[:8]}\n" - f" Files changed since the index was built:\n" - + "\n".join(f" - {f}" for f in _changed.splitlines() if f) - + "\n Consider reindex(changed_files_only=False) for a clean rebuild.\n\n" - ) - except FileNotFoundError: - pass # git binary not installed — no staleness signal available - except subprocess.CalledProcessError: - pass # not a git repo, or HEAD/ref invalid — nothing to compare _preserved: dict[str, tuple] = {} _backup_dir = None @@ -1874,8 +1856,8 @@ def _reindex(changed_files_only: bool) -> str: output = _captured.getvalue().strip() return ( f"--- REINDEX ({mode}) COMPLETE ---\n\n" - f"{_stale_warning}" f"{output}\n\n" + f"{_freshness_line(db_path)}" "In-memory indexes reloaded. All MCP tools now reflect the updated index." ) @@ -1964,12 +1946,7 @@ def _parse_since(s: str) -> datetime: ] # ADR-042 §5: a git-mode index is current when it matches its ref, not HEAD. - from index_location import git_ref - try: - ref = git_ref(os.getcwd()) - except ValueError: - ref = None - target = ref or "HEAD" + ref, target = _freshness_target(os.getcwd()) if last_commit: try: curr = subprocess.check_output( diff --git a/tests/test_index_a_commit.py b/tests/test_index_a_commit.py index 580d4e8..f56d136 100644 --- a/tests/test_index_a_commit.py +++ b/tests/test_index_a_commit.py @@ -225,16 +225,60 @@ def test_folder_source_reads_what_it_always_read(tmp_path): # ── the server in git mode ─────────────────────────────────────────────────── -def test_instructions_name_the_ref_and_the_commit(repo, build): +def test_instructions_name_the_ref_but_no_commit(repo, build): + # ADR-056: the diff hint names the ref, so it stays right as the ref moves. import MCPServer build() text = MCPServer._server_instructions(str(repo)) commit = git(repo, "rev-parse", "main") - assert "main" in text and commit[:10] in text - assert f"git diff --name-only {commit[:10]}...HEAD" in text + assert "main" in text and commit[:10] not in text + assert "git diff --name-only main...HEAD" in text and "index_status" in text assert "working tree" in text and len(text) < 1000 +# ── reindex reports freshness against the ref (ADR-056, B-070) ─────────────── + +@pytest.fixture +def reindex(build, monkeypatch): + """MCPServer._reindex over the fixture's index, with the run itself stubbed out.""" + import MCPServer + build() + monkeypatch.setattr(ii, "run_incremental", + lambda *a, **k: print("Nothing changed — index is up to date.")) + monkeypatch.setattr(MCPServer, "_reload_indexes", lambda: None) + return lambda: MCPServer._reindex(changed_files_only=True) + + +def test_a_reindex_from_a_folder_on_another_branch_reports_no_staleness(repo, reindex): + """The 2026-11-02 incident: the folder sat on a feature branch, the index on main.""" + main = git(repo, "rev-parse", "main") + git(repo, "checkout", "-q", "-b", "feature") + (repo / "src" / "a.ts").write_text("export const edited = 1;\n", encoding="utf-8") + git(repo, "commit", "-q", "-am", "feature work") + out = reindex() + assert "STALENESS" not in out and "changed_files_only=False" not in out + assert "src/a.ts" not in out + assert f"index: {main[:8]} (== main; current)" in out + + +def test_a_reindex_that_did_not_catch_up_says_stale_without_rebuild_advice(repo, reindex): + old = git(repo, "rev-parse", "main") + (repo / "src" / "a.ts").write_text("export const moved = 1;\n", encoding="utf-8") + git(repo, "commit", "-q", "-am", "main moves") + out = reindex() # the stubbed run doesn't move the index + new = git(repo, "rev-parse", "main") + assert f"index: {old[:8]} main: {new[:8]} ⚠️ STALE (the watching server follows it)" in out + assert "changed_files_only=False" not in out + + +def test_index_status_and_reindex_share_one_target(repo): + import MCPServer + assert MCPServer._freshness_target(str(repo)) == ("main", "main") + (repo / "indexer.toml").write_text("[indexer]\n", encoding="utf-8") + config.reset_config_cache() + assert MCPServer._freshness_target(str(repo)) == (None, "HEAD") + + @pytest.mark.skipif(not __import__("MCPServer")._WATCHDOG_AVAILABLE, reason="watchdog not installed") def test_the_watchdog_follows_the_ref_instead_of_files(repo, build, monkeypatch): import MCPServer diff --git a/tests/test_index_writer_lock.py b/tests/test_index_writer_lock.py index 20a8aed..a85c4ad 100644 --- a/tests/test_index_writer_lock.py +++ b/tests/test_index_writer_lock.py @@ -226,16 +226,20 @@ def test_linked_worktrees_are_refused_unless_they_opt_in(tmp_path): # ── server instructions ────────────────────────────────────────────────────── -def test_instructions_name_the_checkout_and_its_commit(index_dir): +def test_instructions_name_the_checkout_but_no_commit(index_dir): + # ADR-056: a client reads these once, at connect; a quoted commit goes stale mid-session. _built_index(index_dir) text = MCPServer._server_instructions(os.getcwd()) - assert os.getcwd() in text and "abcdef0123" in text - assert "reindex" in text and "Read" in text + assert os.getcwd() in text and "abcdef0123" not in text + assert "reindex" in text and "Read" in text and "index_status" in text assert len(text) < 1000 # every connected session pays for this -def test_instructions_without_a_build(index_dir): - assert "no finished build yet" in MCPServer._server_instructions(os.getcwd()) +def test_instructions_do_not_change_when_a_build_lands(index_dir): + before = MCPServer._server_instructions(os.getcwd()) + _built_index(index_dir) + assert MCPServer._server_instructions(os.getcwd()) == before + assert "no finished build yet" not in before # ── a server that did not write reloads what another process saved ───────────