From b1ccc3f9fd00f4b785d50061ae54a7fde34c4c2d Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 25 Oct 2026 23:49:10 +0900 Subject: [PATCH 01/17] docs(reports): open the match_lines topic from the murmuration wave-8 observation A search hit on a long markdown file shows the file's first lines, not the lines that matched: colgrep's --json carries no match location, so hit_from_raw takes the unit's first snippet_lines. The observation records the probes and three fixes (A in this server, B and C upstream). The Side Jobs mission reminder sj-colgrep-match-lines cites this report as its spec, and a worktree sees only commits, so it has to be on main before an agent picks the mission up. Co-Authored-By: Claude Opus 5.5 --- .../00-observation_markdown_match_lines_v0.md | 77 +++++++++++++++++++ __reports__/match_lines/README.md | 7 ++ 2 files changed, 84 insertions(+) create mode 100644 __reports__/match_lines/00-observation_markdown_match_lines_v0.md create mode 100644 __reports__/match_lines/README.md diff --git a/__reports__/match_lines/00-observation_markdown_match_lines_v0.md b/__reports__/match_lines/00-observation_markdown_match_lines_v0.md new file mode 100644 index 0000000..d84d613 --- /dev/null +++ b/__reports__/match_lines/00-observation_markdown_match_lines_v0.md @@ -0,0 +1,77 @@ +# Observation: a search hit on a long markdown file shows the file's first lines, not the lines that matched (v0) + +date: 2026-10-24 · observed from: the Murmuration lab (wave 8, explorer-w08-b and the PI) · colgrep 1.6.2, colgrep-mcp 0.5.2 + +## In one paragraph + +colgrep indexes a markdown file as one unit (`unit_type: document`, lines 1 to the end of the +file). Its **text** renderer still shows the lines inside that unit that match the query: on a +319-line note it printed lines 137–139, where the answer is. Its **`--json`** output, which this +server reads, carries only the unit (its whole `code`) and a score, with no match location. So +`search` can only show the unit's first `snippet_lines` lines (`tools_search.py::hit_from_raw`, +`code_lines[:snippet_lines]`), and `expand` reads the unit from its first line (`max_lines` +default 200). For a long note the hit is a path and a title; the agent must open the file. In the +Murmuration lab this is why workers read notes whole: 152 of 175 whole reads of markdown in four +waves, and documents are 14.3% of what its workers pay to carry context. + +## Evidence + +| Probe | What came back | +|:--|:--| +| `colgrep -n 3 -k 3 "PI decision on w05-c1's ESCALATE" __statera__/wave-05.md` (text) | the unit header (`1 # Wave 05`), then `137 - 16:16 (the closing commit), at close:`, `138 - **PI decision on w05-c1's ESCALATE:**`, `139 …`: the matching lines | +| same query, `--json -k 1` | one hit: keys `unit`, `score`; `unit.line` 1, `unit.end_line` 319, `unit_type` `document`, `code` 25,228 characters; no field for the matching lines | +| MCP `search` on the lab's `__statera__/` (query about the same decision) | four hits, each `unit_type: document`, `hit_id` `…:1-51`, `…:1-26`, …, `snippet` = the file's first three lines (`# Hand-backs of …`); the note that holds the answer is not in the top four | +| MCP `expand` on `__statera__/wave-07.md` (603 lines; explorer-w08-b) | `truncated: true` at line 200; the answer at line 470 is out of reach without `max_lines` | + +Code: `server/colgrep_mcp/tools_search.py`, `hit_from_raw`: +`snippet = "\n".join(code_lines[:snippet_lines])`. The server already holds the whole `code`, +the query, and the optional `pattern`, so it can find the matching lines itself. + +## Two places to fix it + +```mermaid +flowchart LR + Q[query, pattern] --> CG[colgrep --json] + CG -->|unit.code, score| S[colgrep-mcp hit_from_raw] + S -->|today: first N lines| H[hit snippet] + S -.->|A: window around the match| H2[match snippet + match_hit_id] + CG -.->|B: upstream: match lines in JSON| S +``` + +**A. In this server (no upstream change).** When building a hit: +1. With `pattern`: find the first regex match in `code`; else score each line of `code` by the + query's terms (case-folded, stop words out) and take the densest window. +2. Return the window as `snippet` (keep `snippet_lines`), plus `match_line` and a narrowed + `match_hit_id` (`:-`) that `expand` reads directly. Keep `hit_id` as the unit. +3. For `document` units, let `expand` accept a hit id whose range is inside the unit (it already + reads `[line, end_line]` off disk), so an agent reads 20 lines, not 200. + +Cost: a few dozen lines in `tools_search.py`, a field in `SearchHit`, tests. Risk: the window +picks the wrong lines for a purely semantic match with no shared words; fall back to the first +lines and say so (`match_line: null`). + +**B. Upstream in colgrep.** Ask for the text renderer's match lines in `--json` (for example +`matches: [{line, text}]` per hit). Then A becomes a pass-through and agrees with the CLI. + +**C. Also upstream, separable.** A section-level unit for markdown (one unit per heading) +would make ranking itself section-aware: today a 600-line note competes as one unit, so the right +note may rank below short files that share words (the probe above). + +## What to test + +- A 300+ line markdown file with the answer past line 200: `search` snippet shows it, and + `expand(match_hit_id)` returns under 30 lines. +- `pattern` given: the window holds the first regex match. +- Code units: unchanged behaviour (the function's first lines are usually right), or the window + only when the match is past `snippet_lines`. +- A semantic-only hit: `match_line: null`, first lines as today. + +## For the skill, meanwhile + +`colgrep-search` could say: for a note or report, a hit tells you the file; then `grep -n` the +heading or term in that one file and `sed -n 'a,bp'` the lines, rather than `expand`. + +## Pointers + +Murmuration lab (local): `__reports__/wave-08/explorer-w08-b/00-findings_v0.md` (23 questions, +top-k rates, unit sizes), synthesis open item 43. diff --git a/__reports__/match_lines/README.md b/__reports__/match_lines/README.md new file mode 100644 index 0000000..00d6982 --- /dev/null +++ b/__reports__/match_lines/README.md @@ -0,0 +1,7 @@ +# Reports — match_lines + +## Round 00 +- `00-observation_markdown_match_lines_v0.md` — a hit on a long markdown file shows the file's first lines: colgrep's text renderer finds the matching lines, its `--json` does not carry them, so `hit_from_raw` takes `code_lines[:snippet_lines]`; fix A in this server (match window, `match_hit_id`), B upstream (match lines in `--json`), C upstream (section-level markdown units) (latest) + +## Status +Opened 2026-10-24 from the Murmuration lab's wave 8. Not started. From c4c291a4014a07b5652068a2725ba8a869b71b39 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sat, 31 Oct 2026 23:51:21 +0900 Subject: [PATCH 02/17] feat(search): yield one hit per unit colgrep matched in a file colgrep collapses results to one per file (collapse_by_file, colgrep/src/index/mod.rs). Since 1.8 each result also lists its matched units in `matches: [{line, end_line, name, score}]`, best first, whenever two or more matched. search showed only the best unit, so on markdown indexed by section (fork 1.8.5+cs.1) an answer in a note's second-best section was hidden and agents read the whole file. On a copy of Murmuration's __statera__, "what did the PI decide about w05-c1's escalation to switch to a subagent worker" ranks wave-05.md first with sections (162-180), (105-156), ...; the answer, line 138, is in 105-156. search now emits one SearchHit per matched unit with its own hit_id and score, ranks all units by score across files, and lets `limit` count units: colgrep's `-k limit` files always hold the top `limit` units, so the server cuts the rest and sets `truncated`. On the probe above, 105-156 is hit 15 of 25 (inside the default limit of 15), location_verified, and expand returns its 52 lines including line 138. The other units come without `code`. Their snippet and code are sliced from the file text already read for the leader, so no extra I/O; their ranges count as verified only when the leader was located exactly at matches[0]'s range. They carry no qualified_name, unit_type or signature, and the text block drops the empty separators. find_files counts each matched unit in `hits` and names them in `top_units`. Output without `matches` (colgrep 1.6, or a single unit) is unchanged. fake_colgrep.py gains FAKE_COLGREP_MATCHES=1, folding per-unit fixture hits per file the way 1.8 does. The four new tests through Client(build()) failed against the previous tools_search.py. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/guide.md | 18 ++++- server/colgrep_mcp/models.py | 3 + server/colgrep_mcp/tools_search.py | 120 +++++++++++++++++++++++++---- server/tests/fake_colgrep.py | 29 +++++++ server/tests/test_tools_search.py | 110 ++++++++++++++++++++++++++ skills/colgrep-search/SKILL.md | 4 +- 6 files changed, 264 insertions(+), 20 deletions(-) diff --git a/server/colgrep_mcp/guide.md b/server/colgrep_mcp/guide.md index b2ba9d6..37d5475 100644 --- a/server/colgrep_mcp/guide.md +++ b/server/colgrep_mcp/guide.md @@ -11,6 +11,13 @@ arbitrary line range you picked. 18+ languages plus Markdown and plain text. A unit's `code`/`snippet` is that unit's full extracted body — expect complete functions and classes, not fragments. +colgrep groups its results by file; `search` lists every unit that matched +in a file as a hit of its own, ranked by score with the hits of every other +file. One file can therefore show several hits — three sections of one note, +two functions of one module — and the answer may sit in a file's second or +third hit, not its first. Those later hits carry `name`, `score` and a +snippet, but no `unit_type` or `signature` (colgrep reports none for them). + Search is **hybrid** by default: keyword match (FTS5 full-text) and semantic match (ColBERT late-interaction embeddings) are fused into one ranked list. `semantic_only=true` disables the keyword half; `alpha` (0.0 keyword ... @@ -78,7 +85,8 @@ Example: `query="where retries are scheduled", pattern="backoff"`. ## Result size -`limit` caps the number of ranked **units** returned, not lines: +`limit` caps the number of ranked **units** returned across all files, not +lines or files: - Omit `limit` (`None`) **together with `pattern`** for an exhaustive result set — every unit matching the regex is returned, nothing is @@ -115,15 +123,17 @@ server re-locates each unit by matching its source text in the file. `true` means the lines were confirmed against the file on disk; `false` means the file changed since indexing (or the match was ambiguous) and the numbers are colgrep's unverified values — cite the file, not the line, and `expand` -to see what is really there. +to see what is really there. A file's later hits have no source text to +match; they are `true` when that file's first hit was located exactly where +colgrep put it. `score` is **relative within one query's result set only**. Do not compare scores across two different `search` calls, and do not treat a score as a confidence percentage — it ranks candidates against each other, nothing more. -A result carries `truncated=true` when either colgrep or the server's text -budget dropped hits from the compact text listing. `structured_content` +A result carries `truncated=true` when `limit` cut matched units, or when +the server's text budget dropped hits from the compact text listing. `structured_content` still carries every hit's metadata even when the text was cut; when you see `truncated=true` and need more, call `expand` on the `hit_id`s you have, or re-run with a larger or omitted `limit` rather than assuming those were the diff --git a/server/colgrep_mcp/models.py b/server/colgrep_mcp/models.py index b6ba83f..79e2389 100644 --- a/server/colgrep_mcp/models.py +++ b/server/colgrep_mcp/models.py @@ -6,6 +6,9 @@ class SearchHit(BaseModel): + """One matched unit. A file yields one hit per unit colgrep matched in it; the + units after its best carry no `qualified_name`, `unit_type` or `signature`.""" + hit_id: str = Field(description="Self-describing id: ':-'. Pass to `expand`.") file: str line: int diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index a959902..e885a6e 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -112,6 +112,75 @@ def hit_from_raw( ) +def hits_from_raw( + raw: RawHit, + snippet_lines: int, + include_code: bool, + file_cache: dict[str, str], + base_path: Path | None = None, +) -> list[SearchHit]: + """Build one `SearchHit` per unit colgrep matched in one raw result's file. + + colgrep collapses its results to one per file (`collapse_by_file`, + `colgrep/src/index/mod.rs`): `unit` is the best unit, with its range + widened over every matched unit of the file, and since 1.8 `matches` + lists each matched unit's own `{line, end_line, name, score}`, best + first, whenever there are two or more. Showing only the leader hid an + answer sitting in a file's second-best markdown section, so the agent + read the whole file instead (`__reports__/match_lines/`). The leader + goes through `hit_from_raw` unchanged; output without `matches` + (colgrep 1.6, or a single matched unit) yields that one hit. + + The other units arrive without `code`, so `locate_unit` cannot check + them one by one. Their ranges are trusted (`location_verified`) only + when the leader was located exactly at its own `matches[0]` range, + which shows this file on disk still agrees with the index's line + numbers, and the range lies inside the file. Snippet and `code` come + from `file_cache`, already read for the leader: no further I/O. Such a + hit has no `qualified_name`, `unit_type` or `signature`, since colgrep + reports none for it. + """ + leader = hit_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) + matches = raw.get("matches") or [] + if len(matches) < 2: + return [leader] + + file_lines = file_cache.get(leader.file, "").splitlines() + first = matches[0] + trusted = leader.location_verified and ( + int(first.get("line") or 0), + int(first.get("end_line") or 0), + ) == (leader.line, leader.end_line) + + hits = [leader] + seen = {leader.hit_id} + for match in matches[1:]: + line = int(match.get("line") or 1) + end_line = int(match.get("end_line") or line) + hit_id = f"{leader.file}:{line}-{end_line}" + if hit_id in seen: + continue + seen.add(hit_id) + body = file_lines[line - 1 : end_line] + hits.append( + SearchHit( + hit_id=hit_id, + file=leader.file, + line=line, + end_line=end_line, + name=match.get("name") or "", + qualified_name="", + unit_type="", + language=leader.language, + score=float(match.get("score") or 0.0), + snippet="\n".join(body[:snippet_lines]) if body else None, + code="\n".join(body) if include_code and body else None, + location_verified=trusted and 1 <= line <= end_line <= len(file_lines), + ) + ) + return hits + + async def _read_file_off_loop(file: str) -> str: """`Path(file).read_text(errors="replace")`, run in a worker thread. @@ -152,10 +221,12 @@ def _search_header(result: SearchResult) -> str: def _hit_block(hit: SearchHit) -> str: - head = ( - f"{hit.file}:{hit.line}-{hit.end_line} score={hit.score:.2f} " - f"{hit.unit_type} {hit.name} — {hit.signature or ''}" - ) + # A non-leader unit (`hits_from_raw`) has no unit_type or signature: skip + # the empty parts rather than print their separators. + kind = " ".join(part for part in (hit.unit_type, hit.name) if part) + head = f"{hit.file}:{hit.line}-{hit.end_line} score={hit.score:.2f} {kind}" + if hit.signature: + head = f"{head} — {hit.signature}" if not hit.snippet: return head indented = "\n".join(f" {line}" for line in hit.snippet.splitlines()) @@ -397,20 +468,22 @@ def _file_hits_from_raw(raw_hits: list[RawHit], base_path: Path | None) -> list[ `SearchHit`, `hit_id` string, snippet or `locate_unit` call is built. Hits arrive best-score-first (R05), so the first hit seen for a file sets `best_score`; `top_units` is capped at 5, same as the previous fold. + A raw hit carrying colgrep 1.8's `matches` counts each matched unit, as + `search` lists them (`hits_from_raw`): the leader's own name, then the + other units' names. """ by_file: dict[str, FileHit] = {} for raw in raw_hits: unit = raw.get("unit") or {} file = _resolve_hit_file(unit.get("file") or "", base_path) score = float(raw.get("score") or 0.0) - name = unit.get("name") or "" + names = [unit.get("name") or ""] + [m.get("name") or "" for m in (raw.get("matches") or [])[1:]] existing = by_file.get(file) if existing is None: - by_file[file] = FileHit(file=file, best_score=score, hits=1, top_units=[name]) + by_file[file] = FileHit(file=file, best_score=score, hits=len(names), top_units=names[:5]) else: - existing.hits += 1 - if len(existing.top_units) < 5: - existing.top_units.append(name) + existing.hits += len(names) + existing.top_units.extend(names[: 5 - len(existing.top_units)]) return list(by_file.values()) @@ -434,8 +507,15 @@ async def _do_search( snippet_lines: int, include_code: bool, ) -> SearchResult: - """`search`'s body: run the adapter, then convert every raw hit into a - `SearchHit` (locate, snippet, `hit_id`) via `hit_from_raw`. + """`search`'s body: run the adapter, then convert every raw hit into one + `SearchHit` per matched unit (locate, snippet, `hit_id`) via + `hits_from_raw`. + + Units are ranked by score across files and `limit` caps units, not + files. colgrep's `-k limit` returns the top `limit` files by their best + unit, and every unit outside them scores at most the last file's best, + so those files already hold the top `limit` units; the cut below drops + the rest and sets `truncated`. `find_files` no longer routes through here (R01 §C6): it calls `_run_adapter_search` directly and folds raw hits straight into @@ -466,7 +546,15 @@ async def _do_search( base_path = outcome.resolved[0] file_cache: dict[str, str] = {} await _fill_file_cache(outcome.raw_hits, file_cache, base_path=base_path) - hits = [hit_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) for raw in outcome.raw_hits] + hits = [ + hit + for raw in outcome.raw_hits + for hit in hits_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) + ] + hits.sort(key=lambda hit: hit.score, reverse=True) # stable: colgrep's order breaks ties + limited = limit is not None and len(hits) > limit + if limited: + hits = hits[:limit] notes: list[str] = [] if not hits: @@ -488,7 +576,7 @@ async def _do_search( paths=[str(p) for p in outcome.resolved], hits=hits, total=len(hits), - truncated=False, + truncated=limited, elapsed_ms=outcome.elapsed_ms, index_updated=outcome.index_updated, notes=notes, @@ -545,8 +633,10 @@ async def search( Prefer this over shell grep for any question about what, where or how code does something. Pass `pattern` (a regex) to narrow via hybrid keyword+semantic ranking when you know an identifier; omit `limit` - for an exhaustive listing. Each hit carries a `hit_id` — pass it to - `expand` to read the full source instead of opening the whole file. + for an exhaustive listing. Each hit is one unit, and a file yields one + hit per unit that matched in it, ranked with the rest; `limit` counts + units. Each hit carries a `hit_id` — pass it to `expand` to read the + full source instead of opening the whole file. """ settings = get_settings(ctx) result = await _do_search( diff --git a/server/tests/fake_colgrep.py b/server/tests/fake_colgrep.py index 8b6fddb..455c9ad 100755 --- a/server/tests/fake_colgrep.py +++ b/server/tests/fake_colgrep.py @@ -27,6 +27,11 @@ `status ` names `Index: /` for a project it holds and exits 1 like the real binary when the path does not exist on disk (R02) + FAKE_COLGREP_MATCHES "1" -> answer a search like colgrep 1.8.x: fixture + hits (one per unit) fold into one result per file, + `unit` the best one with its range widened over the + file's units, plus `matches` when two or more matched; + `-k` then counts files (`collapse_by_file`) """ import json @@ -59,6 +64,28 @@ def _store_entries(store): return out +def _collapse_by_file(hits): + """colgrep 1.8.x's `collapse_by_file` over score-ordered per-unit hits.""" + out = [] + by_file = {} + for h in hits: + u = h["unit"] + name = "" if u.get("unit_type") in ("document", "raw_code") else u.get("name") or "" + match = {"line": u["line"], "end_line": u["end_line"], "name": name, "score": h["score"]} + if u["file"] not in by_file: + by_file[u["file"]] = len(out) + out.append({"unit": dict(u), "score": h["score"], "matches": [match]}) + continue + leader = out[by_file[u["file"]]] + leader["matches"].append(match) + leader["unit"]["line"] = min(leader["unit"]["line"], u["line"]) + leader["unit"]["end_line"] = max(leader["unit"]["end_line"], u["end_line"]) + for r in out: + if len(r["matches"]) < 2: + del r["matches"] # serde skips an empty list; a lone match is never recorded + return out + + def main(argv): # Force UTF-8 with bare `\n` line endings regardless of platform. Python's # default text-mode stdout/stderr on Windows (a) encodes with the @@ -164,6 +191,8 @@ def main(argv): # the same literal on POSIX (no-op replace) and a drive-rooted equivalent # on Windows (genuinely absolute there too). hits = json.loads(fixture.read_text().replace("/tmp/fake-corpus", FAKE_CORPUS)) + if os.environ.get("FAKE_COLGREP_MATCHES") == "1": + hits = _collapse_by_file(hits) if "-k" in args: k = int(args[args.index("-k") + 1]) hits = hits[:k] diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index c84c64b..ce44eab 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -598,6 +598,116 @@ async def test_search_location_verified_false_when_file_lacks_the_code(settings_ assert hit["hit_id"] == f"{target}:7-9" +# --- colgrep 1.8.x `matches`: one hit per matched unit ---------------------- + +_NOTES = ( + "# Wave 05\n\n## Logistics\nRoutine scheduling.\n\n" + "## Escalation\n- **PI decision:** switch to a subagent.\n\n## Closing\nWave closed.\n" +) + + +def _unit(file: Path, name: str, line: int, end_line: int, code: str, unit_type: str = "section") -> dict: + return { + "name": name, + "qualified_name": f"{file.name}::{name}", + "file": str(file), + "line": line, + "end_line": end_line, + "language": "markdown" if unit_type == "section" else "python", + "unit_type": unit_type, + "signature": None, + "code": code, + } + + +def _matches_corpus(tmp_path: Path, monkeypatch, notes_text: str = _NOTES) -> tuple[Path, Path]: + """Three matched sections of `notes.md` and one function of `tools.py`, as + per-unit hits in score order, which `FAKE_COLGREP_MATCHES=1` folds per file + the way colgrep 1.8.x does.""" + notes = tmp_path / "notes.md" + notes.write_text(notes_text) + tools = tmp_path / "tools.py" + tools.write_text('def escalate():\n return "subagent"\n') + fixture = [ + {"unit": _unit(notes, "Logistics", 3, 4, "## Logistics\nRoutine scheduling.\n"), "score": 1.2}, + { + "unit": _unit(tools, "escalate", 1, 2, 'def escalate():\n return "subagent"\n', "function"), + "score": 1.0, + }, + { + "unit": _unit(notes, "Escalation", 6, 7, "## Escalation\n- **PI decision:** switch to a subagent.\n"), + "score": 0.9, + }, + {"unit": _unit(notes, "Closing", 9, 10, "## Closing\nWave closed.\n"), "score": 0.1}, + ] + fixture_path = tmp_path / "matches_hits.json" + fixture_path.write_text(json.dumps(fixture)) + monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) + monkeypatch.setenv("FAKE_COLGREP_MATCHES", "1") + return notes, tools + + +async def test_search_yields_every_matched_unit_ranked_across_files(settings_env, monkeypatch, tmp_path): + notes, tools = _matches_corpus(tmp_path, monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "PI decision on the escalation"}) + hits = r.structured_content["hits"] + e = await c.call_tool("expand", {"hit_ids": [hits[2]["hit_id"]]}) + + assert [h["hit_id"] for h in hits] == [f"{notes}:3-4", f"{tools}:1-2", f"{notes}:6-7", f"{notes}:9-10"] + assert [h["score"] for h in hits] == pytest.approx([1.2, 1.0, 0.9, 0.1]) + assert all(h["location_verified"] for h in hits) + assert r.structured_content["total"] == 4 + assert r.structured_content["truncated"] is False + second = hits[2] + assert (second["name"], second["unit_type"], second["language"]) == ("Escalation", "", "markdown") + assert "PI decision" in second["snippet"] + assert f"{notes}:6-7 score=0.90 Escalation\n" in r.content[0].text + assert e.structured_content["units"][0]["code"] == "## Escalation\n- **PI decision:** switch to a subagent." + + +async def test_search_limit_counts_units_not_files(settings_env, monkeypatch, tmp_path): + notes, tools = _matches_corpus(tmp_path, monkeypatch) + argv_file = tmp_path / "argv.json" + monkeypatch.setenv("FAKE_COLGREP_ARGV_FILE", str(argv_file)) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "PI decision on the escalation", "limit": 2}) + + argv = json.loads(argv_file.read_text()) + assert argv[argv.index("-k") + 1] == "2" + assert [h["hit_id"] for h in r.structured_content["hits"]] == [f"{notes}:3-4", f"{tools}:1-2"] + assert r.structured_content["truncated"] is True + + +async def test_search_other_units_unverified_when_file_moved_since_indexing(settings_env, monkeypatch, tmp_path): + # Two lines added on top since indexing: the leader is re-located at 5-6, + # so colgrep's ranges for the other sections no longer fit the file. + notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text="new\nlines\n" + _NOTES) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "PI decision on the escalation"}) + + by_id = {h["hit_id"]: h for h in r.structured_content["hits"]} + assert by_id[f"{notes}:5-6"]["location_verified"] is True + assert by_id[f"{notes}:6-7"]["location_verified"] is False + assert by_id[f"{notes}:9-10"]["location_verified"] is False + assert any(n.startswith(f"[{Code.LOCATION_UNVERIFIED}]") for n in r.structured_content["notes"]) + + +async def test_find_files_counts_each_matched_unit(settings_env, monkeypatch, tmp_path): + notes, tools = _matches_corpus(tmp_path, monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("find_files", {"query": "PI decision on the escalation"}) + + files = r.structured_content["files"] + assert [f["file"] for f in files] == [str(notes), str(tools)] + assert (files[0]["hits"], files[0]["top_units"]) == (3, ["Logistics", "Escalation", "Closing"]) + assert (files[1]["hits"], files[1]["top_units"]) == (1, ["escalate"]) + + # --- token budget (R01 §Token-budget invariant) ---------------------------- diff --git a/skills/colgrep-search/SKILL.md b/skills/colgrep-search/SKILL.md index e04d3e6..2a87c95 100644 --- a/skills/colgrep-search/SKILL.md +++ b/skills/colgrep-search/SKILL.md @@ -44,7 +44,9 @@ Knowledge-acquisition loop, not a one-shot lookup: run one broad `search` listing; if the first pass didn't land on the real implementation, run one or two narrower hybrid searches adding `pattern` for an identifier you now know; `expand` at most a handful of the hits that actually answer the -question; answer citing `file:line` from hits whose `location_verified` is +question — one file can list several hits (each matched function or +markdown section is its own), so expand the one that answers rather than +reading the file whole; answer citing `file:line` from hits whose `location_verified` is true (cite the file only when it is false). Never shell grep instead of the first broad search. From 702615e3c627aa98cd93749b5f1aa7c9c4199217 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:42:43 +0900 Subject: [PATCH 03/17] chore(roadmap): open match_units to rework the per-unit search hits Review of c4c291a: the one-hit-per-unit shape is right, but three readers of the matches wire shape, three location bugs and input-proportional work call for a rework on this branch. One implementer leaf, one coordinator leaf (doctor minimum 1.8.4, docs edited in place), one adversarial review. Co-Authored-By: Claude Opus 5.5 --- __roadmap__/match_units/README.md | 50 ++++++++ __roadmap__/match_units/doctor_and_docs.md | 40 +++++++ __roadmap__/match_units/unit_pipeline.md | 111 ++++++++++++++++++ __roadmap__/match_units/verify/README.md | 37 ++++++ .../match_units/verify/adversarial_review.md | 29 +++++ 5 files changed, 267 insertions(+) create mode 100644 __roadmap__/match_units/README.md create mode 100644 __roadmap__/match_units/doctor_and_docs.md create mode 100644 __roadmap__/match_units/unit_pipeline.md create mode 100644 __roadmap__/match_units/verify/README.md create mode 100644 __roadmap__/match_units/verify/adversarial_review.md diff --git a/__roadmap__/match_units/README.md b/__roadmap__/match_units/README.md new file mode 100644 index 0000000..bd69d79 --- /dev/null +++ b/__roadmap__/match_units/README.md @@ -0,0 +1,50 @@ +# Match Units + +## Context +PR #18 (commit c4c291a on `claude/happy-hamilton-e15dcd`) made `search` yield one hit per unit in colgrep 1.8's `matches`. A review found the shape right but the construction not idiomatic: three readers of the wire shape, three location bugs, and work proportional to colgrep's input rather than the output. This campaign reworks it on the same branch, sets colgrep 1.8.4 as the soft minimum, and edits the docs in place instead of appending to them. + +## Reference Documents +- [R01 Approved plan](~/.claude/plans/now-switching-you-to-tidy-glade.md) — verdict table (rows 1–9), target design, execution model +- [R02 Observation OBS-M](../../__reports__/match_lines/00-observation_markdown_match_lines_v0.md) — why a hidden section makes agents read whole files + +## Goal +`search` builds only the units it returns, from one normaliser of colgrep's results. Every unit keeps its own verified range and name, and the docs state that rule without accretion. + +## Pre-conditions +- [ ] `claude/happy-hamilton-e15dcd` is at c4c291a or later, with the gates green +- [ ] `colgrep --version` reports 1.8.4 or newer on this machine (for the verifier's real-binary probe) + +## Success Gates +- ⬜ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range main..HEAD` passes on the integration branch [run] +- ⬜ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] +- ⬜ `git diff main..HEAD -- skills/` is empty [run] +- ⬜ The verifier's real-binary probe passes: the wave-05 105-156 hit is verified and expands to line 138 [behavioral] +- ⬜ PR #18 CI is green [run] + +## Status +```mermaid +graph TD + unit_pipeline[Unit Pipeline]:::planned + doctor_and_docs[Doctor and Docs]:::planned + verify[Verification]:::planned + classDef done fill:#166534,color:#bbf7d0 + classDef inprogress fill:#854d0e,color:#fef08a + classDef planned fill:#374151,color:#e5e7eb + classDef amendment fill:#1e3a5f,color:#bfdbfe + classDef blocked fill:#7f1d1d,color:#fecaca +``` + +## Nodes +| Node | Type | Status | +|:-----|:-----|:-------| +| `unit_pipeline.md` | 📄 Leaf Task | ⬜ Planned | +| `doctor_and_docs.md` | 📄 Leaf Task | ⬜ Planned | +| `verify/` | 📁 Directory | ⬜ Planned | + +## Amendment Log +| ID | Date | Source | Nodes Added | Rationale | +|:---|:-----|:-------|:------------|:----------| + +## Progress +| Node | Branch | Commits | Notes | +|:-----|:-------|:--------|:------| diff --git a/__roadmap__/match_units/doctor_and_docs.md b/__roadmap__/match_units/doctor_and_docs.md new file mode 100644 index 0000000..e3b8024 --- /dev/null +++ b/__roadmap__/match_units/doctor_and_docs.md @@ -0,0 +1,40 @@ +# Doctor and Docs + +**Goal**: Declare colgrep 1.8.4 the soft minimum through `doctor`, and state the one-hit-per-unit rule by editing the guide in place rather than appending to it. +**Pre-conditions**: +- [ ] The roadmap commit is on `claude/happy-hamilton-e15dcd` +**Success Gates**: +- ⬜ `cd server && uv run pytest -q tests/test_tools_index.py tests/test_adapter.py` passes [run] +- ⬜ `git diff main..HEAD -- skills/` is empty [run] +- ⬜ `git diff --numstat main..HEAD -- server/colgrep_mcp/guide.md` adds no more than 4 lines net [run] +- ⬜ `AGENTS.md` names `OBS-M` and points it at `__reports__/match_lines/00-observation_markdown_match_lines_v0.md` [run] +**References**: [R01 §Documentation, §Minimum colgrep 1.8.4](~/.claude/plans/now-switching-you-to-tidy-glade.md) — what to revert, what to rewrite, how strict the minimum is + +## Step 1: Report a colgrep older than 1.8.4 in doctor +**Goal**: `doctor` names an old binary as a problem with a version-neutral upgrade hint. +**Implementation Logic**: +colgrep 1.8.4 (upstream 1af6b16) is the first release with `matches`, and the user set it as the minimum, softly. +- Parse the first `(\d+)\.(\d+)\.(\d+)` of `colgrep --version` and compare it as a tuple. `1.8.5+cs.1` is fine, `1.10.0` is newer than `1.8.4`, and unparsable output raises no problem. +- An older version adds a problem stating the minimum and that `search` then falls back to slower whole-file location. The hint must not say `colgrep update`: on this machine that would replace the fork. +- Move the fake's `--version` to `colgrep 1.8.5`, and update the two assertions that pin the old string (`test_adapter.py`, `test_tools_index.py`). +- Add a parametrized parse test. +- In the README requirements table, change the cell `colgrep ≥ 1.6` to `≥ 1.8.4`. +**Deliverables**: `server/colgrep_mcp/tools_index.py` — `MIN_COLGREP` and the version check in `doctor`; `server/tests/fake_colgrep.py` — `--version` line; `server/tests/test_adapter.py`, `server/tests/test_tools_index.py` — assertions and parse test; `README.md` — one table cell +**Consistency Checks**: `cd server && uv run pytest -q tests/test_tools_index.py tests/test_adapter.py` (expected: PASS) +**Commit**: `feat(index): report a colgrep older than 1.8.4 in doctor` + +## Step 2: State the one-hit-per-unit rule in place +**Goal**: The guide states exactly the rule `unit_pipeline` implements, with nothing appended, and the skill keeps the text it had on `main`. +**Implementation Logic**: +- Revert `skills/colgrep-search/SKILL.md` to `main`. The clause added in c4c291a was defensive, and the skill already defers to the guide. +- In `guide.md`: + - Drop the paragraph c4c291a appended. + - Extend the existing "A hit is one unit…" sentence by one clause: one hit per matched unit, ranked with every other file's hits. + - Rewrite the `location_verified` paragraph as one statement of the final rule. That rule covers: a leader verified against its own lines, falling back to whole-file location; a later unit trusted through its leader and its heading line; and an unverified later unit carrying no snippet. The "colgrep 1.6 itself…" history goes. + - The `truncated` sentence says only what the server can know: its own cut to `limit`, or the text budget. +- Add the `OBS-M` legend row to `AGENTS.md`. + +Write this after `unit_pipeline` reports, so the wording matches the code. +**Deliverables**: `skills/colgrep-search/SKILL.md` — reverted; `server/colgrep_mcp/guide.md` — the paragraphs above; `AGENTS.md` — one legend row +**Consistency Checks**: `git diff --quiet main -- skills/` (expected: PASS) +**Commit**: `docs(resources): state the one-hit-per-unit rule in place` diff --git a/__roadmap__/match_units/unit_pipeline.md b/__roadmap__/match_units/unit_pipeline.md new file mode 100644 index 0000000..38c02b8 --- /dev/null +++ b/__roadmap__/match_units/unit_pipeline.md @@ -0,0 +1,111 @@ +# Unit Pipeline + +**Goal**: Rebuild `search`'s hit construction as one output-proportional pipeline: adapter JSON → `unit_matches` → rank and cut to `limit` → read only the surviving spans → build hits. Every unit keeps its own verified range and name. +**Pre-conditions**: +- [ ] Task branch `task/match-units-pipeline` is cut from `claude/happy-hamilton-e15dcd` at the commit that adds this roadmap +- [ ] `cd server && uv run pytest -q` is green in the task worktree before any edit +**Success Gates**: +- ⬜ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range claude/happy-hamilton-e15dcd..HEAD` passes [run] +- ⬜ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] +- ⬜ `FAKE_COLGREP_MATCHES` no longer appears under `server/` [run] +- ⬜ Every new contract test named in steps 1–4 fails when `server/colgrep_mcp/` is checked out at c4c291a, and the report lists them [run] +- ⬜ The bodies of commits 3 and 4 carry median-of-5 before/after tables, or the commit is typed `refactor` and the report says why [run] +**References**: [R01 §Verdict on c4c291a, §Target design](~/.claude/plans/now-switching-you-to-tidy-glade.md) — the nine findings this leaf answers (rows 1–7, 9) and the design it implements + +## Step 1: Normalise colgrep results to leader-first unit matches +**Goal**: Make `adapter.unit_matches` the only reader of the `matches` wire shape, and feed the tests literal colgrep 1.8 JSON. +**Implementation Logic**: +colgrep returns one result per file (`collapse_by_file`, `colgrep/src/index/mod.rs`). Since 1.8.4, `matches: [{line, end_line, name, score}]` lists each matched unit, best first. It appears only when two or more matched, and `matches[0]` is the leader. Today `hits_from_raw` and `_file_hits_from_raw` each re-derive this, and they diverge: `search` dedupes, `find_files` does not (verdict row 1). colgrep's own idiom is `SearchResult::unit_matches()`. + +- **The type.** Add `UnitMatch(NamedTuple)` with `line`, `end_line`, `name`, `score`, next to `RawHit` in `adapter.py`. That file is the colgrep wire boundary. +- **The normaliser.** Add `unit_matches(raw) -> list[UnitMatch]`: + - It returns a leader-first list. + - It dedupes on `(line, end_line)` with max score, as colgrep's `absorb_matches` does. + - It stable-sorts the non-leaders by score descending, which enforces the prefix-cut invariant step 3 relies on. + - It drops entries that violate `1 <= line <= end_line`. + - When `matches` is absent or has one entry, it returns `[UnitMatch(unit.line, unit.end_line, unit.name, score)]`. +- **Callers.** `hits_from_raw` and `_file_hits_from_raw` go through it, with behaviour otherwise unchanged in this step. `find_files` counts the deduped units and drops empty names from `top_units`. +- **Fake and fixtures.** + - Drop the `FAKE_COLGREP_MATCHES` knob and `_collapse_by_file` from `fake_colgrep.py` (verdict row 9). Its docstring says a fixture's `matches` passes through verbatim, and that `-k` counts files because results are already per file. + - Convert `fixtures/hits_small.json` to the 1.8 shape: one result for `config.py`, with `matches` listing `parse_config` and `Settings`, then one for `README.md`. Its consumers must keep their counts: `total == 3`, and `find_files` `hits == 2` for `config.py`. + - Rewrite the four existing `matches` tests in `test_tools_search.py` on literal 1.8-shaped fixtures, ordered best first. +- **New contract test.** Duplicate ranges in `matches` yield one hit in `search` and one count in `find_files`. + +Leave the `--version` line in `fake_colgrep.py` alone. The coordinator changes it in `doctor_and_docs` on the integration branch. +**Deliverables**: `server/colgrep_mcp/adapter.py` — `UnitMatch`, `unit_matches`; `server/colgrep_mcp/tools_search.py` — `hits_from_raw`, `_file_hits_from_raw` calling `unit_matches`; `server/tests/fake_colgrep.py` — knob and `_collapse_by_file` removed; `server/tests/fixtures/hits_small.json` — 1.8 shape; `server/tests/test_tools_search.py` — literal fixtures, dedupe test +**Consistency Checks**: `cd server && uv run pytest -q tests/test_tools_search.py tests/test_render.py tests/test_smoke.py` (expected: PASS) +**Commit**: `refactor(adapter): normalise colgrep results to leader-first unit matches` + +## Step 2: Keep each unit to its own verified range and name +**Goal**: Fix the three location bugs and the naming mix in one builder (verdict rows 2, 3, 4, 6, 7). +**Implementation Logic**: +- **Row 2.** `hit_from_raw` takes the leader's `UnitMatch` and passes its range to `locate_unit` as the reported range. Today that range is the widened `unit.line..end_line`, so an unlocatable leader's `hit_id` overlaps every sibling. +- **Rows 3 and 4.** One private builder serves every non-leader, replacing the second 12-kwarg `SearchHit(...)` block. + - A non-leader is trusted only when the leader was verified exactly at `matches[0]`'s range **and** the last dotted segment of the match name (`name.rsplit(".", 1)[-1]`) occurs in the file's line `line`. + - **Nameless non-leaders fail closed.** colgrep gives `document` and `raw_code` units an empty name. + - An untrusted non-leader gets `snippet=None` and `code=None`, never text sliced at moved lines. + - The `location_verified` field description in `models.py` states this rule. +- **Row 6.** When `matches` is present, every hit of that file takes colgrep's display name, the leader included. The leader keeps `qualified_name`, `unit_type` and `signature` from `unit`. Output without `matches` is unchanged. +- **Row 7.** Pin the signature-less head in `test_render.py`: no trailing ` — `. +- **New contract tests** through `Client(build())`: + - an edit below the leader: the sibling is unverified, with no snippet; + - an unlocatable leader: its own range, not the widened span; + - a nameless sibling: unverified; + - `include_code` on a trusted non-leader; + - a one-entry `matches`; + - a heading containing a dot (`## v1.8 notes`), which stays trusted. +**Deliverables**: `server/colgrep_mcp/tools_search.py` — `hit_from_raw` taking the leader `UnitMatch`, one non-leader builder; `server/colgrep_mcp/models.py` — `location_verified` description; `server/tests/test_tools_search.py`, `server/tests/test_render.py` — the tests above +**Consistency Checks**: `cd server && uv run pytest -q tests/test_tools_search.py tests/test_render.py` (expected: PASS) +**Commit**: `fix(search): keep each unit to its own verified range and name` + +## Step 3: Build only surviving units and verify leaders by exact span +**Goal**: Make `search`'s work proportional to the hits it returns, not to the files and units colgrep sent (verdict row 5). +**Implementation Logic**: +- **Rank and cut first.** `_do_search` stable-sorts `(score, result index, match index)` over every `unit_matches` list and cuts to `limit` before building anything. A file's survivors are a prefix of its list, so its leader is always built. +- **The cache** is pinned so the builder stays pure and I/O-free. + - An async step replaces `_fill_file_cache`. For each file with survivors it reads only those spans in **one** `itertools.islice` pass, up to the largest surviving `end_line`, in `asyncio.to_thread`, with one `gather` across files. This generalises `_read_span` into `_read_spans(file, spans) -> dict[(line, end_line), list[str]]`; `expand` reuses it in step 4. + - The builder receives `file -> {span: lines}` plus an optional full text. +- **Exact-span verification.** A leader whose range has exactly as many lines as its `code` is verified by comparing that span to `code`. The comparison uses the trailing-whitespace (`rstrip`) normalisation, factored out of `locate.py` into one helper both paths call. + - Only on a mismatch, or when the lengths differ, is the whole file read once and `locate_unit` run. That happens for a file edited since indexing, or a pre-1.8.4 binary whose range is widened. + - Say in the docstring that a trailing-whitespace-only edit now verifies. +- **Tests.** + - A spy test pins that a span-verified leader never triggers a whole-file read, in the style of `test_find_files_does_not_read_hit_files`. + - Switch `test_search_reads_hit_files_off_the_event_loop` to spy on `open`, as the `expand` test does. With exact spans, the happy path never calls `read_text`, so the old spy would stop testing what it names. + - Adapt the `_fill_file_cache` tests to the new step. +- **Measurement** (`perf` rule, maintainer-policy): + - Corpus: 15 markdown files of 2,000 lines each, with 30 matched sections per file, plus a matching fixture, all in the scratchpad. + - Harness: a stub adapter like `_StderrInjectingAdapter`, timing the post-adapter part of `_do_search` at `limit=15` and `limit=None`. + - Run: median of 5, at step 2's commit versus this one. + - Record the table in the body. If the numbers don't show a gain, type the commit `refactor` and say so. +**Deliverables**: `server/colgrep_mcp/tools_search.py` — rank-and-cut in `_do_search`, `_read_spans`, span cache step replacing `_fill_file_cache`; `server/colgrep_mcp/locate.py` — the shared normalised-comparison helper; tests above; benchmark script kept in the scratchpad, not committed +**Consistency Checks**: `cd server && uv run pytest -q` (expected: PASS) +**Commit**: `perf(search): build only surviving units and verify leaders by exact span` + +## Step 4: Read each file's expanded spans in one pass +**Goal**: `expand` reads a file once however many of its `hit_id`s it is given (verdict row 5). +**Implementation Logic**: +Per-unit hits make several `hit_id`s of one file the common case, and `expand` opens and `islice`s the file once per id. + +- Group the parsed ids by file and make one `_read_spans` pass per file, off the loop as today. +- Emit units in input order. +- Malformed-id errors stay per unit. An `OSError` attaches to every id of that file. +- Overlapping and non-contiguous spans must both work. +- **Contract test:** three ids of one file, out of order and one overlapping, return the right spans in input order with one `open` (spy). +- **Measurement:** median of 5 for `expand` on 5 ids of one 2,000-line file, before and after, in the body. +**Deliverables**: `server/colgrep_mcp/tools_search.py` — `expand` grouped by file over `_read_spans`; `server/tests/test_tools_search.py` — the test above +**Consistency Checks**: `cd server && uv run pytest -q tests/test_tools_search.py` (expected: PASS) +**Commit**: `perf(search): read each file's expanded spans in one pass` + +## Step 5: Ask colgrep for limit files in find_files +**Goal**: Stop `find_files` asking colgrep to rank and serialise three times the files it returns. +**Implementation Logic**: +`find_files` passes `-k min(limit * 3, 300)`. That over-fetch dates from when hits were per unit. Every supported colgrep returns one result per file. + +- **Condition.** First confirm in colgrep's source that a search over several paths is collapsed globally, not per path. The fork's source is checked out under `~/.cargo/git/checkouts/next-plaid-e20e6e3a2c4603c9/7f1e855/colgrep/src/`; read `index/mod.rs` `collapse_by_file` and its caller in `commands/search.rs`. +- **If it is collapsed globally:** pass `-k limit`, keep the four-line cross-result fold, and pin the argv in a test. +- **If it is not:** keep `limit * 3`, skip this commit, and report what the source shows. + +This cost lies inside colgrep, beyond the in-process stub, so the commit is a `refactor`, not a measured `perf`. +**Deliverables**: `server/colgrep_mcp/tools_search.py` — `find_files` `hit_limit`; `server/tests/test_tools_search.py` — argv test +**Consistency Checks**: `cd server && uv run pytest -q tests/test_tools_search.py` (expected: PASS) +**Commit**: `refactor(search): ask colgrep for limit files in find_files` diff --git a/__roadmap__/match_units/verify/README.md b/__roadmap__/match_units/verify/README.md new file mode 100644 index 0000000..f4c4527 --- /dev/null +++ b/__roadmap__/match_units/verify/README.md @@ -0,0 +1,37 @@ +# Verification + +## Context +Runs after `unit_pipeline`. It is the named adversarial review for this campaign, because the change touches the persisted `hit_id` and `location_verified` contract, where failures are silent. Its findings go back to the implementer of `unit_pipeline`. + +## Goal +Either break the pipeline's location and trust rules or show, probe by probe, that they hold. + +## Pre-conditions +- [ ] `unit_pipeline` has reported, with its gates green + +## Success Gates +- ⬜ Every attack in `adversarial_review` is marked broken or held, and each broken one is fixed or explicitly accepted by the coordinator [behavioral] + +## Status +```mermaid +graph TD + adversarial_review[Adversarial Review]:::planned + classDef done fill:#166534,color:#bbf7d0 + classDef inprogress fill:#854d0e,color:#fef08a + classDef planned fill:#374151,color:#e5e7eb + classDef amendment fill:#1e3a5f,color:#bfdbfe + classDef blocked fill:#7f1d1d,color:#fecaca +``` + +## Nodes +| Node | Type | Status | +|:-----|:-----|:-------| +| `adversarial_review.md` | 📄 Leaf Task | ⬜ Planned | + +## Amendment Log +| ID | Date | Source | Nodes Added | Rationale | +|:---|:-----|:-------|:------------|:----------| + +## Progress +| Node | Branch | Commits | Notes | +|:-----|:-------|:--------|:------| diff --git a/__roadmap__/match_units/verify/adversarial_review.md b/__roadmap__/match_units/verify/adversarial_review.md new file mode 100644 index 0000000..97ae5e9 --- /dev/null +++ b/__roadmap__/match_units/verify/adversarial_review.md @@ -0,0 +1,29 @@ +# Adversarial Review + +**Goal**: Find what is wrong in `unit_pipeline` before it merges: silent location and trust failures, the class of bug a green suite does not show. +**Pre-conditions**: +- [ ] `unit_pipeline` has reported, with its gates green on `task/match-units-pipeline` +- [ ] `colgrep --version` reports 1.8.4 or newer, and the scratch corpus `/Users/hacker/Documents/tmp/match-units-scratch/statera` exists +**Success Gates**: +- ⬜ A findings list in which each attack below is marked broken (with a reproducer) or held (with the probe that tried to break it) [behavioral] +- ⬜ The real-binary probe: the wave-05 105-156 hit is `location_verified` and `expand` returns line 138 [run] +- ⬜ The share of leaders verified by exact span versus whole-file fallback, over 3–5 statera queries, is reported [run] +**References**: [R01 §Verdict on c4c291a, §Target design](~/.claude/plans/now-switching-you-to-tidy-glade.md) — the rules under attack + +## Step 1: Attack the pipeline and probe the real binary +**Goal**: Assume the change is wrong and try to prove it. +**Implementation Logic**: +Read-only. Write probes in the scratchpad, not in the repo. Attack: +- **Trust-rule false positives:** edits between units, renamed headings, dotted, duplicate and emphasised headings, CRLF files. +- **Exact-span false positives:** the same code at another location in the file. +- **Line counting:** `islice` versus `splitlines()` on files containing `\f`, `\x0b` or U+2028. +- **The prefix cut:** behaviour with tied scores. +- **`hit_id`s:** overlapping or inverted ranges. +- **Counts:** do `find_files` and `search` agree on how many units a file has? +- **`expand` grouping:** input order, duplicate ids, an unreadable file. +- **Step 5's `-k` evidence**, if that step landed. + +Then run this tree's server against the real binary: `COLGREP_DATA_DIR=/Users/hacker/Documents/tmp/match-units-scratch/data COLGREP_MCP_ROOT=/Users/hacker/Documents/tmp/match-units-scratch/statera`. Use 3–5 queries, including "what did the PI decide about w05-c1's escalation to switch to a subagent worker". Never point it at this repository. +**Deliverables**: the findings list in the verifier's report; probe scripts stay in the scratchpad +**Consistency Checks**: `colgrep --version` (expected: PASS) +**Commit**: `test(search): none — read-only review, findings go to the implementer` From 47cdf8733bd36428d889c18500f003f0121d13dd Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:43:48 +0900 Subject: [PATCH 04/17] chore(roadmap): name the verifier's scratch corpus by reference, not by path test_packaging's local-path guard rejects machine paths in the tree; the brief carries them instead. Co-Authored-By: Claude Opus 5.5 --- __roadmap__/match_units/verify/adversarial_review.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/__roadmap__/match_units/verify/adversarial_review.md b/__roadmap__/match_units/verify/adversarial_review.md index 97ae5e9..b1953b2 100644 --- a/__roadmap__/match_units/verify/adversarial_review.md +++ b/__roadmap__/match_units/verify/adversarial_review.md @@ -3,7 +3,7 @@ **Goal**: Find what is wrong in `unit_pipeline` before it merges: silent location and trust failures, the class of bug a green suite does not show. **Pre-conditions**: - [ ] `unit_pipeline` has reported, with its gates green on `task/match-units-pipeline` -- [ ] `colgrep --version` reports 1.8.4 or newer, and the scratch corpus `/Users/hacker/Documents/tmp/match-units-scratch/statera` exists +- [ ] `colgrep --version` reports 1.8.4 or newer, and the scratch copy of a notes corpus named in the verifier's brief exists outside this repository **Success Gates**: - ⬜ A findings list in which each attack below is marked broken (with a reproducer) or held (with the probe that tried to break it) [behavioral] - ⬜ The real-binary probe: the wave-05 105-156 hit is `location_verified` and `expand` returns line 138 [run] @@ -23,7 +23,7 @@ Read-only. Write probes in the scratchpad, not in the repo. Attack: - **`expand` grouping:** input order, duplicate ids, an unreadable file. - **Step 5's `-k` evidence**, if that step landed. -Then run this tree's server against the real binary: `COLGREP_DATA_DIR=/Users/hacker/Documents/tmp/match-units-scratch/data COLGREP_MCP_ROOT=/Users/hacker/Documents/tmp/match-units-scratch/statera`. Use 3–5 queries, including "what did the PI decide about w05-c1's escalation to switch to a subagent worker". Never point it at this repository. +Then run this tree's server against the real binary: `COLGREP_DATA_DIR` and `COLGREP_MCP_ROOT` set to the scratch data dir and corpus named in the brief. Use 3–5 queries, including "what did the PI decide about w05-c1's escalation to switch to a subagent worker". Never point it at this repository. **Deliverables**: the findings list in the verifier's report; probe scripts stay in the scratchpad **Consistency Checks**: `colgrep --version` (expected: PASS) **Commit**: `test(search): none — read-only review, findings go to the implementer` From 8c0df617871545ad1460d9e7673a0027a076a7f0 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:45:42 +0900 Subject: [PATCH 05/17] feat(index): report a colgrep older than 1.8.4 in doctor colgrep 1.8.4 (upstream 1af6b16) is the first release whose --json lists every matched unit of a file in `matches`; search now builds one hit per unit from it. An older binary still works, but search sees only each file's best unit and locates it by reading the whole file, so the minimum is soft: doctor reports it as a problem with an upgrade hint rather than search refusing to run. The hint is version-neutral and does not suggest `colgrep update`, which reinstalls the official build over a fork. The fake now reports 1.8.5 (the version its fixtures model), and the README's requirement cell moves from 1.6 to 1.8.4. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- server/colgrep_mcp/tools_index.py | 25 +++++++++++++++++++++++++ server/tests/fake_colgrep.py | 2 +- server/tests/test_adapter.py | 2 +- server/tests/test_tools_index.py | 17 ++++++++++++++++- 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5dc840a..57387ee 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ One directory installs as a **Claude Code plugin**, an **[Agent Plugins 1.0](htt | Dependency | Why | Install | |:--|:--|:--| -| `colgrep` ≥ 1.6 | does the indexing and ranking | `cargo install colgrep` or see the [next-plaid README](https://github.com/lightonai/next-plaid) | +| `colgrep` ≥ 1.8.4 | does the indexing and ranking | `cargo install colgrep` or see the [next-plaid README](https://github.com/lightonai/next-plaid) | | `uv` | `uvx` fetches the server from PyPI and caches it on first launch | [docs.astral.sh/uv](https://docs.astral.sh/uv/) | | Python ≥ 3.11 | fetched automatically by `uv` if missing | — | diff --git a/server/colgrep_mcp/tools_index.py b/server/colgrep_mcp/tools_index.py index 98381d5..09be4d7 100644 --- a/server/colgrep_mcp/tools_index.py +++ b/server/colgrep_mcp/tools_index.py @@ -7,6 +7,7 @@ import asyncio import contextlib +import re import shutil import time from pathlib import Path @@ -40,6 +41,28 @@ #: A module constant so tests can monkeypatch it down for fast heartbeat assertions. HEARTBEAT_S = 5.0 +#: The first colgrep whose `--json` lists every matched unit of a file +#: (`SearchResult.matches`, upstream 1af6b16). An older binary still works, +#: but `search` then sees one unit per file and locates it by reading the +#: whole file, so `doctor` reports it rather than `search` refusing it. +MIN_COLGREP = (1, 8, 4) +_VERSION_RE = re.compile(r"(\d+)\.(\d+)\.(\d+)") + + +def _version_problem(version: str) -> str | None: + """A `doctor` problem when `colgrep --version` is older than `MIN_COLGREP`. + + Compares the first `x.y.z` as integers, so `1.8.5+cs.1` passes and `1.10.0` + is newer than `1.8.4`; output with no `x.y.z` is not judged. + """ + found = _VERSION_RE.search(version) + if found is None or tuple(int(part) for part in found.groups()) >= MIN_COLGREP: + return None + return ( + f"colgrep {found.group(0)} is older than 1.8.4: search sees only each file's best unit " + "and locates it by reading the whole file; upgrade colgrep to 1.8.4 or newer" + ) + class Confirm(BaseModel): """Elicitation schema for the human-in-the-loop confirmation of `index_clear` and `index_prune`.""" @@ -322,6 +345,8 @@ async def doctor(*, ctx: Context) -> CallToolResult: problems.append(f"colgrep binary not found: {exc}") except ColgrepError as exc: problems.append(f"colgrep --version failed: {exc}") + if version is not None and (too_old := _version_problem(version)): + problems.append(too_old) settings_map: dict[str, str] = {} try: diff --git a/server/tests/fake_colgrep.py b/server/tests/fake_colgrep.py index 455c9ad..61ef7d5 100755 --- a/server/tests/fake_colgrep.py +++ b/server/tests/fake_colgrep.py @@ -113,7 +113,7 @@ def main(argv): args = [a for a in argv if a != "--color" and a != "never"] if "--version" in args: - print("colgrep 1.6.2") + print("colgrep 1.8.5") return 0 store = os.environ.get("FAKE_COLGREP_STORE") if "--stats" in args: diff --git a/server/tests/test_adapter.py b/server/tests/test_adapter.py index ec0d840..54e3e65 100644 --- a/server/tests/test_adapter.py +++ b/server/tests/test_adapter.py @@ -170,7 +170,7 @@ def test_argv_rejects_query_starting_with_dash(): async def test_version_via_fake_binary(fake_colgrep_bin): adapter = ColgrepAdapter(binary=fake_colgrep_bin, timeout_s=10) version = await adapter.version() - assert version == "colgrep 1.6.2" + assert version == "colgrep 1.8.5" async def test_run_prepends_color_never(fake_colgrep_bin, tmp_path, monkeypatch): diff --git a/server/tests/test_tools_index.py b/server/tests/test_tools_index.py index d7b5fc0..7b2e6f1 100644 --- a/server/tests/test_tools_index.py +++ b/server/tests/test_tools_index.py @@ -261,11 +261,26 @@ async def test_doctor_ok(settings_env): doc = result.structured_content assert doc["ok"] is True assert doc["problems"] == [] - assert doc["version"] == "colgrep 1.6.2" + assert doc["version"] == "colgrep 1.8.5" assert doc["colgrep_path"] is not None assert doc["root_source"] == "env" +@pytest.mark.parametrize( + ("version", "too_old"), + [ + ("colgrep 1.8.4", False), + ("colgrep 1.8.5+cs.1", False), + ("colgrep 1.10.0", False), + ("colgrep 1.8.3", True), + ("colgrep 1.6.2", True), + ("colgrep (unknown build)", False), + ], +) +def test_doctor_flags_a_colgrep_older_than_the_minimum(version, too_old): + assert (tools_index._version_problem(version) is not None) is too_old + + async def test_doctor_bogus_binary(monkeypatch, tmp_path): monkeypatch.setenv("COLGREP_MCP_BINARY", "/no/such/colgrep-binary-xyz") monkeypatch.setenv("COLGREP_MCP_ROOT", str(tmp_path)) From dc718a7c8b06ce73c5f2f83e519080b45ef18966 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:04:49 +0900 Subject: [PATCH 06/17] docs(resources): state the one-hit-per-unit rule in place c4c291a appended a paragraph to the guide and a clause to the skill. Both were defensive accretion: the paragraph restated the hit shape and listed fields structured_content already shows, and the skill already says to expand a handful of hits and defers to the guide for reading them, while costing tokens on every trigger. The skill goes back to main's text. In the guide, the existing "A hit is one unit" sentence gains one clause (one hit per matched unit, ranked across files); the location_verified paragraph is rewritten to state the rule the server now applies (a file's best unit checked against its source, the others once it sits at colgrep's range and the file is unchanged since this search refreshed the index; an unverified non-best unit has no snippet) and drops the colgrep 1.6 history; the truncated sentence says only what the server can know, its own cut to limit or the text budget. AGENTS.md gains the OBS-M legend row the search docstrings cite. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + server/colgrep_mcp/guide.md | 33 +++++++++++++-------------------- skills/colgrep-search/SKILL.md | 4 +--- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9e876c9..2a30d40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,6 +114,7 @@ reports, `OBS-*` observation reports from a reviewer pass. | `OBS-H` | `__reports__/repo_health/01-observation_review_v0.md` | | `OBS-C` | `__reports__/consistency/01-observation_review_v0.md` | | `OBS-D` | `__reports__/dev_plugin/02-observation_review_v0.md` | +| `OBS-M` | `__reports__/match_lines/00-observation_markdown_match_lines_v0.md` | | `PH` | `__reports__/repo_health/00-findings_launch_placeholders_v0.md` | | `CI` | `__reports__/repo_health/03-findings_ci_matrix_v0.md` | | `RI` | `__reports__/repo_health/04-findings_remote_install_v0.md` | diff --git a/server/colgrep_mcp/guide.md b/server/colgrep_mcp/guide.md index 37d5475..c767a58 100644 --- a/server/colgrep_mcp/guide.md +++ b/server/colgrep_mcp/guide.md @@ -7,17 +7,11 @@ new session; skim it again if a result looks wrong. colgrep indexes **code units**, not lines: functions, methods, classes, markdown sections, and plain-text blocks. A hit is one unit, never an -arbitrary line range you picked. 18+ languages plus Markdown and plain text. +arbitrary line range you picked; a file yields one hit per unit that +matched, ranked with every other file's. 18+ languages plus Markdown and plain text. A unit's `code`/`snippet` is that unit's full extracted body — expect complete functions and classes, not fragments. -colgrep groups its results by file; `search` lists every unit that matched -in a file as a hit of its own, ranked by score with the hits of every other -file. One file can therefore show several hits — three sections of one note, -two functions of one module — and the answer may sit in a file's second or -third hit, not its first. Those later hits carry `name`, `score` and a -snippet, but no `unit_type` or `signature` (colgrep reports none for them). - Search is **hybrid** by default: keyword match (FTS5 full-text) and semantic match (ColBERT late-interaction embeddings) are fused into one ranked list. `semantic_only=true` disables the keyword half; `alpha` (0.0 keyword ... @@ -85,8 +79,7 @@ Example: `query="where retries are scheduled", pattern="backoff"`. ## Result size -`limit` caps the number of ranked **units** returned across all files, not -lines or files: +`limit` caps the number of ranked **units** returned, not lines: - Omit `limit` (`None`) **together with `pattern`** for an exhaustive result set — every unit matching the regex is returned, nothing is @@ -118,22 +111,22 @@ what `expand(hit_ids=[...])` expects. Copy `hit_id`s straight from a result into `expand`. `location_verified` tells you whether those line numbers are trustworthy. -colgrep 1.6 itself reports wrong `line`/`end_line` for most units, so the -server re-locates each unit by matching its source text in the file. `true` -means the lines were confirmed against the file on disk; `false` means the -file changed since indexing (or the match was ambiguous) and the numbers -are colgrep's unverified values — cite the file, not the line, and `expand` -to see what is really there. A file's later hits have no source text to -match; they are `true` when that file's first hit was located exactly where -colgrep put it. +The server checks a file's best unit against its source text in the file, +and trusts the file's other units when that unit sits exactly where colgrep +put it and the file is unchanged since this search refreshed the index +(never with `skip_index_update=true`). `true` means the lines were +confirmed against the file on disk; `false` means the file changed since +indexing (or the match was ambiguous) — cite the file, not the line, and +`expand` to see what is really there. Such a unit, unless it is its file's +best, carries no snippet. `score` is **relative within one query's result set only**. Do not compare scores across two different `search` calls, and do not treat a score as a confidence percentage — it ranks candidates against each other, nothing more. -A result carries `truncated=true` when `limit` cut matched units, or when -the server's text budget dropped hits from the compact text listing. `structured_content` +A result carries `truncated=true` when the server cut units past `limit`, +or when its text budget dropped hits from the compact text listing. `structured_content` still carries every hit's metadata even when the text was cut; when you see `truncated=true` and need more, call `expand` on the `hit_id`s you have, or re-run with a larger or omitted `limit` rather than assuming those were the diff --git a/skills/colgrep-search/SKILL.md b/skills/colgrep-search/SKILL.md index 2a87c95..e04d3e6 100644 --- a/skills/colgrep-search/SKILL.md +++ b/skills/colgrep-search/SKILL.md @@ -44,9 +44,7 @@ Knowledge-acquisition loop, not a one-shot lookup: run one broad `search` listing; if the first pass didn't land on the real implementation, run one or two narrower hybrid searches adding `pattern` for an identifier you now know; `expand` at most a handful of the hits that actually answer the -question — one file can list several hits (each matched function or -markdown section is its own), so expand the one that answers rather than -reading the file whole; answer citing `file:line` from hits whose `location_verified` is +question; answer citing `file:line` from hits whose `location_verified` is true (cite the file only when it is false). Never shell grep instead of the first broad search. From c6f60dfaf0f339b480168f5d5d8fa7bc01c710fd Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:48:24 +0900 Subject: [PATCH 07/17] refactor(adapter): normalise colgrep results to leader-first unit matches colgrep returns one result per file and, since 1.8.4, lists each matched unit in `matches`, best first, when two or more matched. Two functions re-derived that shape and diverged: search deduped on range, find_files did not, so a duplicated range counted twice there. colgrep's own idiom is one normaliser, SearchResult::unit_matches() in index/mod.rs. adapter.unit_matches is now the only reader of `matches`: leader first, one entry per (line, end_line) at the best score as absorb_matches does, non-leaders stable-sorted by score so a cut is a prefix, ranges outside 1 <= line <= end_line dropped, and a fall-back to the result's own unit when `matches` is absent or lone. find_files counts those units and no longer lists empty names (documents, raw code) in top_units. The fake drops FAKE_COLGREP_MATCHES and its re-implementation of collapse_by_file: it answers a fixture as written, like its other knobs. hits_small.json and the matches tests move to literal 1.8-shaped results. The duplicate-range test through Client(build()) fails on c4c291a through find_files (3 hits counted for 2 units). Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/adapter.py | 69 ++++++++++++++++++++++++++- server/colgrep_mcp/tools_search.py | 38 +++++++-------- server/tests/fake_colgrep.py | 35 ++------------ server/tests/fixtures/hits_small.json | 62 +++++++++++------------- server/tests/test_tools_search.py | 53 +++++++++++++++----- 5 files changed, 161 insertions(+), 96 deletions(-) diff --git a/server/colgrep_mcp/adapter.py b/server/colgrep_mcp/adapter.py index f74ec44..91e1d25 100644 --- a/server/colgrep_mcp/adapter.py +++ b/server/colgrep_mcp/adapter.py @@ -14,7 +14,7 @@ from collections.abc import Awaitable, Callable from dataclasses import dataclass, field from pathlib import Path -from typing import Any +from typing import Any, NamedTuple from .models import IndexBuildResult, IndexInfo, IndexStatus from .textparse import parse_index_summary, parse_settings, parse_stats, parse_status @@ -73,6 +73,73 @@ class SearchRequest: RawHit = dict[str, Any] +class UnitMatch(NamedTuple): + """One matched unit of a file: its own range, display name and score.""" + + line: int + end_line: int + name: str + score: float + + +def _wire_match(entry: Any) -> UnitMatch | None: + """A `matches` entry as a `UnitMatch`, or `None` when it is malformed or + its range breaks `1 <= line <= end_line`.""" + if not isinstance(entry, dict): + return None + try: + line, end_line = int(entry.get("line") or 0), int(entry.get("end_line") or 0) + score = float(entry.get("score") or 0.0) + except (TypeError, ValueError): + return None + if not 1 <= line <= end_line: + return None + return UnitMatch(line, end_line, entry.get("name") or "", score) + + +def unit_matches(raw: RawHit) -> list[UnitMatch]: + """The matched units of one colgrep result, leader first. The only reader of `matches`. + + colgrep returns one result per file (`collapse_by_file`, + `colgrep/src/index/mod.rs`): `unit` is the best unit with its range + widened over every matched unit, and since 1.8.4 `matches` lists each + unit's own `{line, end_line, name, score}`, best first, whenever two or + more matched. This is `SearchResult::unit_matches()` in that file, plus + what its consumers need: entries sharing a range collapse to the best + score (`absorb_matches`), the non-leaders are stable-sorted by score so a + cut of the list is a prefix of the file's best units, and entries that + break `1 <= line <= end_line` are dropped. A malformed leader is no + leader: the result then falls back to its own `unit`, like an absent or + one-entry `matches` (colgrep 1.6, a lone matched unit). `name` is the + display name (`Class.method`), empty for documents and raw code. + """ + unit = raw.get("unit") or {} + score = float(raw.get("score") or 0.0) + line = int(unit.get("line") or 1) + own = UnitMatch(line, int(unit.get("end_line") or line), unit.get("name") or "", score) + + wire = raw.get("matches") or [] + if len(wire) < 2: + return [own] + leader = _wire_match(wire[0]) + if leader is None: + return [own] + + best = {(leader.line, leader.end_line): leader} + for entry in wire[1:]: + match = _wire_match(entry) + if match is None: + continue + key = (match.line, match.end_line) + if key not in best: + best[key] = match + elif match.score > best[key].score: + best[key] = best[key]._replace(score=match.score) + leader, *rest = best.values() + rest.sort(key=lambda m: m.score, reverse=True) + return [leader, *rest] + + def _require_absolute(path: Path, what: str) -> None: """Raise `ColgrepError` unless `path` is absolute. diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index e885a6e..6c11f2c 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -24,7 +24,7 @@ from mcp.types import CallToolResult, TextContent from pydantic import Field -from .adapter import RawHit, SearchRequest +from .adapter import RawHit, SearchRequest, unit_matches from .errors import Code, note, tool_error, translate_adapter_errors from .locate import locate_unit from .locks import project_lock @@ -141,41 +141,36 @@ def hits_from_raw( reports none for it. """ leader = hit_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) - matches = raw.get("matches") or [] + matches = unit_matches(raw) if len(matches) < 2: return [leader] file_lines = file_cache.get(leader.file, "").splitlines() first = matches[0] - trusted = leader.location_verified and ( - int(first.get("line") or 0), - int(first.get("end_line") or 0), - ) == (leader.line, leader.end_line) + trusted = leader.location_verified and (first.line, first.end_line) == (leader.line, leader.end_line) hits = [leader] seen = {leader.hit_id} for match in matches[1:]: - line = int(match.get("line") or 1) - end_line = int(match.get("end_line") or line) - hit_id = f"{leader.file}:{line}-{end_line}" + hit_id = f"{leader.file}:{match.line}-{match.end_line}" if hit_id in seen: continue seen.add(hit_id) - body = file_lines[line - 1 : end_line] + body = file_lines[match.line - 1 : match.end_line] hits.append( SearchHit( hit_id=hit_id, file=leader.file, - line=line, - end_line=end_line, - name=match.get("name") or "", + line=match.line, + end_line=match.end_line, + name=match.name, qualified_name="", unit_type="", language=leader.language, - score=float(match.get("score") or 0.0), + score=match.score, snippet="\n".join(body[:snippet_lines]) if body else None, code="\n".join(body) if include_code and body else None, - location_verified=trusted and 1 <= line <= end_line <= len(file_lines), + location_verified=trusted and match.end_line <= len(file_lines), ) ) return hits @@ -468,21 +463,22 @@ def _file_hits_from_raw(raw_hits: list[RawHit], base_path: Path | None) -> list[ `SearchHit`, `hit_id` string, snippet or `locate_unit` call is built. Hits arrive best-score-first (R05), so the first hit seen for a file sets `best_score`; `top_units` is capped at 5, same as the previous fold. - A raw hit carrying colgrep 1.8's `matches` counts each matched unit, as - `search` lists them (`hits_from_raw`): the leader's own name, then the - other units' names. + A file counts each unit colgrep matched in it, as `search` lists them, + through the one normaliser `unit_matches`; units without a name (documents, + raw code) count but are not listed in `top_units`. """ by_file: dict[str, FileHit] = {} for raw in raw_hits: unit = raw.get("unit") or {} file = _resolve_hit_file(unit.get("file") or "", base_path) score = float(raw.get("score") or 0.0) - names = [unit.get("name") or ""] + [m.get("name") or "" for m in (raw.get("matches") or [])[1:]] + matches = unit_matches(raw) + names = [m.name for m in matches if m.name] existing = by_file.get(file) if existing is None: - by_file[file] = FileHit(file=file, best_score=score, hits=len(names), top_units=names[:5]) + by_file[file] = FileHit(file=file, best_score=score, hits=len(matches), top_units=names[:5]) else: - existing.hits += len(names) + existing.hits += len(matches) existing.top_units.extend(names[: 5 - len(existing.top_units)]) return list(by_file.values()) diff --git a/server/tests/fake_colgrep.py b/server/tests/fake_colgrep.py index 61ef7d5..a563f0d 100755 --- a/server/tests/fake_colgrep.py +++ b/server/tests/fake_colgrep.py @@ -6,7 +6,11 @@ [search] [--json] [-k N] [-e PAT] [flags...] QUERY [PATH...] Environment knobs: FAKE_COLGREP_EXIT force this exit code (stderr gets "forced failure") - FAKE_COLGREP_HITS path to a JSON fixture (default: fixtures/hits_small.json) + FAKE_COLGREP_HITS path to a JSON fixture (default: fixtures/hits_small.json), + answered as written: colgrep returns one result per file + (`collapse_by_file`), so `-k N` counts files, and a + fixture lists a file's matched units in `matches` itself + (colgrep 1.8.4+ shape) FAKE_COLGREP_SLEEP seconds to sleep before answering (timeout tests) FAKE_COLGREP_INDEXED "0" -> `status` reports no index FAKE_COLGREP_ARGV_FILE path to write argv (as a JSON list) to, for adapter tests @@ -27,11 +31,6 @@ `status ` names `Index: /` for a project it holds and exits 1 like the real binary when the path does not exist on disk (R02) - FAKE_COLGREP_MATCHES "1" -> answer a search like colgrep 1.8.x: fixture - hits (one per unit) fold into one result per file, - `unit` the best one with its range widened over the - file's units, plus `matches` when two or more matched; - `-k` then counts files (`collapse_by_file`) """ import json @@ -64,28 +63,6 @@ def _store_entries(store): return out -def _collapse_by_file(hits): - """colgrep 1.8.x's `collapse_by_file` over score-ordered per-unit hits.""" - out = [] - by_file = {} - for h in hits: - u = h["unit"] - name = "" if u.get("unit_type") in ("document", "raw_code") else u.get("name") or "" - match = {"line": u["line"], "end_line": u["end_line"], "name": name, "score": h["score"]} - if u["file"] not in by_file: - by_file[u["file"]] = len(out) - out.append({"unit": dict(u), "score": h["score"], "matches": [match]}) - continue - leader = out[by_file[u["file"]]] - leader["matches"].append(match) - leader["unit"]["line"] = min(leader["unit"]["line"], u["line"]) - leader["unit"]["end_line"] = max(leader["unit"]["end_line"], u["end_line"]) - for r in out: - if len(r["matches"]) < 2: - del r["matches"] # serde skips an empty list; a lone match is never recorded - return out - - def main(argv): # Force UTF-8 with bare `\n` line endings regardless of platform. Python's # default text-mode stdout/stderr on Windows (a) encodes with the @@ -191,8 +168,6 @@ def main(argv): # the same literal on POSIX (no-op replace) and a drive-rooted equivalent # on Windows (genuinely absolute there too). hits = json.loads(fixture.read_text().replace("/tmp/fake-corpus", FAKE_CORPUS)) - if os.environ.get("FAKE_COLGREP_MATCHES") == "1": - hits = _collapse_by_file(hits) if "-k" in args: k = int(args[args.index("-k") + 1]) hits = hits[:k] diff --git a/server/tests/fixtures/hits_small.json b/server/tests/fixtures/hits_small.json index 43545ee..6af9f25 100644 --- a/server/tests/fixtures/hits_small.json +++ b/server/tests/fixtures/hits_small.json @@ -5,53 +5,49 @@ "qualified_name": "src/config.py::parse_config", "file": "/tmp/fake-corpus/src/config.py", "line": 10, - "end_line": 42, + "end_line": 60, "language": "python", "unit_type": "function", "signature": "def parse_config(path: str) -> dict", "docstring": "Parse the TOML configuration file.", - "parameters": ["path"], + "parameters": [ + "path" + ], "return_type": "dict", "extends": null, "parent_class": null, - "calls": ["open", "toml.load"], + "calls": [ + "open", + "toml.load" + ], "called_by": [], "complexity": 3, "has_loops": false, "has_branches": true, "has_error_handling": true, - "variables": ["data"], - "imports": ["toml"], + "variables": [ + "data" + ], + "imports": [ + "toml" + ], "code": "def parse_config(path: str) -> dict:\n \"\"\"Parse the TOML configuration file.\"\"\"\n with open(path) as fh:\n data = toml.load(fh)\n if 'model' not in data:\n raise KeyError('model')\n return data\n" }, - "score": 1.4321 - }, - { - "unit": { - "name": "Settings", - "qualified_name": "src/config.py::Settings", - "file": "/tmp/fake-corpus/src/config.py", - "line": 45, - "end_line": 60, - "language": "python", - "unit_type": "class", - "signature": "class Settings", - "docstring": null, - "parameters": [], - "return_type": null, - "extends": null, - "parent_class": null, - "calls": [], - "called_by": ["parse_config"], - "complexity": 1, - "has_loops": false, - "has_branches": false, - "has_error_handling": false, - "variables": [], - "imports": [], - "code": "class Settings:\n model: str\n k: int = 15\n" - }, - "score": 0.9812 + "score": 1.4321, + "matches": [ + { + "line": 10, + "end_line": 42, + "name": "parse_config", + "score": 1.4321 + }, + { + "line": 45, + "end_line": 60, + "name": "Settings", + "score": 0.9812 + } + ] }, { "unit": { diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index ce44eab..6e762c3 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -620,30 +620,40 @@ def _unit(file: Path, name: str, line: int, end_line: int, code: str, unit_type: } -def _matches_corpus(tmp_path: Path, monkeypatch, notes_text: str = _NOTES) -> tuple[Path, Path]: - """Three matched sections of `notes.md` and one function of `tools.py`, as - per-unit hits in score order, which `FAKE_COLGREP_MATCHES=1` folds per file - the way colgrep 1.8.x does.""" +def _match(line: int, end_line: int, name: str, score: float) -> dict: + return {"line": line, "end_line": end_line, "name": name, "score": score} + + +def _matches_corpus( + tmp_path: Path, monkeypatch, notes_text: str = _NOTES, notes_matches: list[dict] | None = None +) -> tuple[Path, Path]: + """Literal colgrep 1.8.4+ results (one per file, best first): `notes.md` led by + its `Logistics` section with `unit` widened over the three matched sections + and `matches` listing each, then `tools.py` with its one function.""" notes = tmp_path / "notes.md" notes.write_text(notes_text) tools = tmp_path / "tools.py" tools.write_text('def escalate():\n return "subagent"\n') + leader = _unit(notes, "Logistics", 3, 10, "## Logistics\nRoutine scheduling.\n") fixture = [ - {"unit": _unit(notes, "Logistics", 3, 4, "## Logistics\nRoutine scheduling.\n"), "score": 1.2}, { - "unit": _unit(tools, "escalate", 1, 2, 'def escalate():\n return "subagent"\n', "function"), - "score": 1.0, + "unit": leader, + "score": 1.2, + "matches": notes_matches + or [ + _match(3, 4, "Logistics", 1.2), + _match(6, 7, "Escalation", 0.9), + _match(9, 10, "Closing", 0.1), + ], }, { - "unit": _unit(notes, "Escalation", 6, 7, "## Escalation\n- **PI decision:** switch to a subagent.\n"), - "score": 0.9, + "unit": _unit(tools, "escalate", 1, 2, 'def escalate():\n return "subagent"\n', "function"), + "score": 1.0, }, - {"unit": _unit(notes, "Closing", 9, 10, "## Closing\nWave closed.\n"), "score": 0.1}, ] fixture_path = tmp_path / "matches_hits.json" fixture_path.write_text(json.dumps(fixture)) monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) - monkeypatch.setenv("FAKE_COLGREP_MATCHES", "1") return notes, tools @@ -708,6 +718,27 @@ async def test_find_files_counts_each_matched_unit(settings_env, monkeypatch, tm assert (files[1]["hits"], files[1]["top_units"]) == (1, ["escalate"]) +async def test_duplicate_ranges_in_matches_count_once(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus( + tmp_path, + monkeypatch, + notes_matches=[ + _match(3, 4, "Logistics", 1.2), + _match(6, 7, "Escalation", 0.9), + _match(6, 7, "Escalation", 0.4), + ], + ) + + async with Client(build(), raise_exceptions=True) as c: + searched = await c.call_tool("search", {"query": "escalation"}) + found = await c.call_tool("find_files", {"query": "escalation"}) + + hits = [h for h in searched.structured_content["hits"] if h["file"] == str(notes)] + assert [(h["hit_id"], h["score"]) for h in hits] == [(f"{notes}:3-4", 1.2), (f"{notes}:6-7", 0.9)] + first = found.structured_content["files"][0] + assert (first["hits"], first["top_units"]) == (2, ["Logistics", "Escalation"]) + + # --- token budget (R01 §Token-budget invariant) ---------------------------- From 3dba035b2e9491ac71ddd75a386183f54b892146 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:51:32 +0900 Subject: [PATCH 08/17] fix(search): keep each unit to its own verified range and name c4c291a built a file's best unit and its siblings in two places that disagreed, and four things went wrong. An unlocatable best unit fell back to unit.line..end_line, the range colgrep widens over every matched unit, so its hit_id overlapped all its siblings; it now falls back to its own matches[0] range. A sibling was trusted whenever the best unit sat at its own range, so an edit between the two moved the sibling while it kept snippet and code sliced at the old lines, the right name over the wrong text. A sibling with no name (colgrep names no document or raw-code unit) passed that test too. And `name` mixed unit.name with colgrep's display names. A sibling is now trusted only when the best unit was located exactly at its own range and the last dotted segment of its display name occurs in the range's first line (so `## v1.8 notes` passes). An unnamed one never is. An untrusted sibling gets no snippet and no code. One private builder serves every sibling, and every hit of a file takes colgrep's display name; the best unit keeps qualified_name, unit_type and signature. The location_verified description states this rule. A hit whose located range equals a sibling's reported one is no longer dropped: unit_matches already collapses colgrep's own duplicates, and the case left is a moved file, whose siblings are unverified anyway. Contract tests through Client(build()) pin the edit below the best unit, the unlocatable best unit, the unnamed sibling, include_code on a verified sibling, a one-entry matches and a dotted heading; test_render pins the head of a signature-less hit (no trailing separator), which c4c291a changed unpinned. The edit-below, unlocatable and unnamed tests fail on c4c291a; the other four pass there and pin what must not regress. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/models.py | 8 +- server/colgrep_mcp/tools_search.py | 150 ++++++++++++++++------------- server/tests/test_render.py | 23 +++-- server/tests/test_tools_search.py | 81 ++++++++++++++++ 4 files changed, 185 insertions(+), 77 deletions(-) diff --git a/server/colgrep_mcp/models.py b/server/colgrep_mcp/models.py index 79e2389..e1777fe 100644 --- a/server/colgrep_mcp/models.py +++ b/server/colgrep_mcp/models.py @@ -23,9 +23,11 @@ class SearchHit(BaseModel): code: str | None = Field(default=None, description="Full unit source; only when include_code=true.") location_verified: bool = Field( default=False, - description="True when `line`/`end_line` were re-derived via locate_unit and matched exactly " - "(R05 D1: colgrep's own line/end_line are frequently wrong). False means the reported " - "values are an unverified fallback.", + description="True when `line`/`end_line` were checked against the file (R05 D1: colgrep's own " + "line/end_line are frequently wrong). A file's best unit is checked by finding its code there; " + "each other unit of the file only when the best sits exactly at colgrep's range for it and this " + "unit's name occurs in the range's first line. False means colgrep's range, unchecked; such a " + "non-best unit has no snippet or code.", ) diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index 6c11f2c..490e3b3 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -24,7 +24,7 @@ from mcp.types import CallToolResult, TextContent from pydantic import Field -from .adapter import RawHit, SearchRequest, unit_matches +from .adapter import RawHit, SearchRequest, UnitMatch, unit_matches from .errors import Code, note, tool_error, translate_adapter_errors from .locate import locate_unit from .locks import project_lock @@ -59,59 +59,106 @@ def _resolve_hit_file(file: str, base_path: Path | None) -> str: def hit_from_raw( raw: RawHit, + leader: UnitMatch, snippet_lines: int, include_code: bool, file_cache: dict[str, str], base_path: Path | None = None, ) -> SearchHit: - """Build a `SearchHit` from one raw colgrep JSON hit (`{"unit": {...}, "score": ...}`). - - Re-derives `line`/`end_line` via `locate_unit` (R05 D1: colgrep's reported - values are frequently wrong) so `hit_id` is built from trustworthy - locations. Pure and I/O-free: `file_cache` must already hold every hit - file's text (or `""` for an unreadable one) — the async caller fills it - via `_fill_file_cache` (off the event loop, R05 F11) before this runs, so - an unreadable file still degrades to an empty text rather than raise, - which makes `locate_unit` fall back to the reported line/end_line with - `verified=False`. `base_path` (the first path the search ran against) - resolves a relative `unit.file` to absolute (F13) before `hit_id` is built. + """Build the `SearchHit` of one raw colgrep result's best unit (`{"unit": {...}, "score": ...}`). + + `leader` is that unit's own `UnitMatch` (`adapter.unit_matches(raw)[0]`): + `unit.line..end_line` is widened over every matched unit of the file, so + only `leader`'s range is the unit's, and the one to fall back to. Re-derives + `line`/`end_line` via `locate_unit` (R05 D1: colgrep's reported values are + frequently wrong), starting from that range, so `hit_id` is built from + trustworthy locations and an unlocatable unit keeps its own range instead + of one overlapping its siblings. Pure and I/O-free: `file_cache` must + already hold every hit file's text (or `""` for an unreadable one) — the + async caller fills it via `_fill_file_cache` (off the event loop, R05 F11) + before this runs, so an unreadable file degrades to an empty text rather + than raise, which makes `locate_unit` fall back with `verified=False`. + `base_path` (the first path the search ran against) resolves a relative + `unit.file` to absolute (F13) before `hit_id` is built. + + `name` is colgrep's display name for the unit, `Class.method` included; + `qualified_name`, `unit_type` and `signature` come from `unit`. Tolerant of missing/None optional fields (R03 §Hit JSON Schema): only `unit`/`score` are assumed present, everything else defaults to an empty string or `None`. """ unit = raw.get("unit") or {} - score = float(raw.get("score") or 0.0) file = _resolve_hit_file(unit.get("file") or "", base_path) code = unit.get("code") or "" - reported_line = int(unit.get("line") or 1) - reported_end = int(unit.get("end_line") or reported_line) - file_text = file_cache.get(file, "") - - line, end_line, verified = locate_unit(file_text, code, reported_line, reported_end) - hit_id = f"{file}:{line}-{end_line}" + line, end_line, verified = locate_unit(file_cache.get(file, ""), code, leader.line, leader.end_line) code_lines = code.splitlines() snippet = "\n".join(code_lines[:snippet_lines]) if code_lines else None return SearchHit( - hit_id=hit_id, + hit_id=f"{file}:{line}-{end_line}", file=file, line=line, end_line=end_line, - name=unit.get("name") or "", + name=leader.name, qualified_name=unit.get("qualified_name") or "", unit_type=unit.get("unit_type") or "", language=unit.get("language") or "", signature=unit.get("signature"), - score=score, + score=leader.score, snippet=snippet, code=code if include_code and code else None, location_verified=verified, ) +def _sibling_hit( + leader: SearchHit, + match: UnitMatch, + body: list[str], + leader_exact: bool, + snippet_lines: int, + include_code: bool, +) -> SearchHit: + """The `SearchHit` of a file's non-leader unit, whose `code` colgrep does not send. + + `body` is the lines the file holds at the unit's range. Nothing here can + check them against the unit's code, so the range is trusted + (`location_verified`) only on two grounds. The leader was located exactly + at its own range (`leader_exact`), so the file still agrees with the + index's line numbers there; and the last dotted segment of the unit's + display name occurs in the range's first line, so an edit between the two + units has not moved this one. A heading that holds a dot (`v1.8 notes`) + still passes, an unnamed unit (documents and raw code have an empty name, + `UnitMatch`) never does. An untrusted unit gets no snippet and no code, + for text at a moved range would sit under another unit's name. It has no + `qualified_name`, `unit_type` or `signature`, since colgrep reports none + for it. + """ + trusted = ( + leader_exact + and len(body) == match.end_line - match.line + 1 + and bool(match.name) + and match.name.rsplit(".", 1)[-1] in body[0] + ) + return SearchHit( + hit_id=f"{leader.file}:{match.line}-{match.end_line}", + file=leader.file, + line=match.line, + end_line=match.end_line, + name=match.name, + qualified_name="", + unit_type="", + language=leader.language, + score=match.score, + snippet="\n".join(body[:snippet_lines]) if trusted and body else None, + code="\n".join(body) if trusted and include_code else None, + location_verified=trusted, + ) + + def hits_from_raw( raw: RawHit, snippet_lines: int, @@ -122,58 +169,25 @@ def hits_from_raw( """Build one `SearchHit` per unit colgrep matched in one raw result's file. colgrep collapses its results to one per file (`collapse_by_file`, - `colgrep/src/index/mod.rs`): `unit` is the best unit, with its range - widened over every matched unit of the file, and since 1.8 `matches` - lists each matched unit's own `{line, end_line, name, score}`, best - first, whenever there are two or more. Showing only the leader hid an - answer sitting in a file's second-best markdown section, so the agent - read the whole file instead (`__reports__/match_lines/`). The leader - goes through `hit_from_raw` unchanged; output without `matches` - (colgrep 1.6, or a single matched unit) yields that one hit. - - The other units arrive without `code`, so `locate_unit` cannot check - them one by one. Their ranges are trusted (`location_verified`) only - when the leader was located exactly at its own `matches[0]` range, - which shows this file on disk still agrees with the index's line - numbers, and the range lies inside the file. Snippet and `code` come - from `file_cache`, already read for the leader: no further I/O. Such a - hit has no `qualified_name`, `unit_type` or `signature`, since colgrep - reports none for it. + `colgrep/src/index/mod.rs`) and since 1.8.4 `unit_matches` lists each + matched unit's own range and name. Showing only the best hid an answer + sitting in a file's second-best markdown section, so the agent read the + whole file instead (OBS-M). The best unit goes through `hit_from_raw`; the + others through `_sibling_hit`, their text read from `file_cache`, already + filled for the leader: no further I/O. Output without `matches` (colgrep + 1.6, or a single matched unit) yields that one hit. """ - leader = hit_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) matches = unit_matches(raw) + leader = hit_from_raw(raw, matches[0], snippet_lines, include_code, file_cache, base_path=base_path) if len(matches) < 2: return [leader] file_lines = file_cache.get(leader.file, "").splitlines() - first = matches[0] - trusted = leader.location_verified and (first.line, first.end_line) == (leader.line, leader.end_line) - - hits = [leader] - seen = {leader.hit_id} - for match in matches[1:]: - hit_id = f"{leader.file}:{match.line}-{match.end_line}" - if hit_id in seen: - continue - seen.add(hit_id) - body = file_lines[match.line - 1 : match.end_line] - hits.append( - SearchHit( - hit_id=hit_id, - file=leader.file, - line=match.line, - end_line=match.end_line, - name=match.name, - qualified_name="", - unit_type="", - language=leader.language, - score=match.score, - snippet="\n".join(body[:snippet_lines]) if body else None, - code="\n".join(body) if include_code and body else None, - location_verified=trusted and match.end_line <= len(file_lines), - ) - ) - return hits + leader_exact = leader.location_verified and (leader.line, leader.end_line) == (matches[0].line, matches[0].end_line) + return [leader] + [ + _sibling_hit(leader, m, file_lines[m.line - 1 : m.end_line], leader_exact, snippet_lines, include_code) + for m in matches[1:] + ] async def _read_file_off_loop(file: str) -> str: diff --git a/server/tests/test_render.py b/server/tests/test_render.py index 81f15bb..e206670 100644 --- a/server/tests/test_render.py +++ b/server/tests/test_render.py @@ -3,6 +3,7 @@ from __future__ import annotations +from colgrep_mcp.adapter import unit_matches from colgrep_mcp.models import FileHit, FileResult, SearchHit, SearchResult from colgrep_mcp.tools_search import ( _file_block, @@ -41,7 +42,7 @@ def test_hit_from_raw_locates_true_lines_when_file_readable(tmp_path): # `hit_from_raw` is pure/I/O-free (F11): the async caller (`_fill_file_cache`) # is responsible for populating `file_cache` before this runs. file_cache = {str(target): target.read_text()} - hit = hit_from_raw(raw, snippet_lines=6, include_code=False, file_cache=file_cache) + hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache=file_cache) assert (hit.line, hit.end_line, hit.location_verified) == (3, 4, True) assert hit.hit_id == f"{target}:3-4" @@ -50,7 +51,7 @@ def test_hit_from_raw_locates_true_lines_when_file_readable(tmp_path): def test_hit_from_raw_falls_back_when_file_unreadable(): raw = _raw_hit(line=7, end_line=9) - hit = hit_from_raw(raw, snippet_lines=6, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache={}) assert (hit.line, hit.end_line, hit.location_verified) == (7, 9, False) assert hit.hit_id == "/nonexistent/src/config.py:7-9" @@ -64,7 +65,7 @@ def test_hit_from_raw_never_touches_disk_itself(tmp_path): raw = _raw_hit(file=str(missing_path), line=1, end_line=1) file_cache = {str(missing_path): "def parse_config(path: str) -> dict:\n return {}\n"} - hit = hit_from_raw(raw, snippet_lines=6, include_code=False, file_cache=file_cache) + hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache=file_cache) assert (hit.line, hit.end_line, hit.location_verified) == (1, 2, True) @@ -72,18 +73,18 @@ def test_hit_from_raw_never_touches_disk_itself(tmp_path): def test_hit_from_raw_snippet_truncated_and_code_gated(): raw = _raw_hit(code="line1\nline2\nline3\nline4\n") - hit = hit_from_raw(raw, snippet_lines=2, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=2, include_code=False, file_cache={}) assert hit.snippet == "line1\nline2" assert hit.code is None - hit_full = hit_from_raw(raw, snippet_lines=2, include_code=True, file_cache={}) + hit_full = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=2, include_code=True, file_cache={}) assert hit_full.code == "line1\nline2\nline3\nline4\n" def test_hit_from_raw_tolerant_of_missing_optional_fields(): raw = {"unit": {"file": "/nonexistent/a.py"}, "score": 0.5} - hit = hit_from_raw(raw, snippet_lines=6, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache={}) assert hit.name == "" assert hit.signature is None @@ -137,6 +138,16 @@ def test_render_search_text_includes_header_and_hit_block_uncapped(): assert "more hits" not in text +def test_hit_block_head_has_no_dangling_separator_without_signature(): + """A unit with no signature (a markdown section, any non-best unit) ends its + head at the name, and one with no `unit_type` starts it at the name.""" + typed = _hit(1).model_copy(update={"signature": None}) + bare = typed.model_copy(update={"unit_type": "", "name": "Escalation", "snippet": None}) + + assert _hit_block(typed).splitlines()[0] == "/proj/f1.py:1-1 score=1.00 function unit_1" + assert _hit_block(bare) == "/proj/f1.py:1-1 score=1.00 Escalation" + + def test_render_search_text_budget_exactly_at_boundary_includes_hit(): result = _result([_hit(1)]) full_text, capped = render_search_text(result, budget=10_000) diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index 6e762c3..3536079 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -718,6 +718,87 @@ async def test_find_files_counts_each_matched_unit(settings_env, monkeypatch, tm assert (files[1]["hits"], files[1]["top_units"]) == (1, ["escalate"]) +async def test_search_unit_after_an_edit_below_the_leader_is_unverified_without_text( + settings_env, monkeypatch, tmp_path +): + # A line added under the best section: the leader still sits at its own range, + # but every later section has moved, so colgrep's ranges for them hold other lines. + text = _NOTES.replace("Routine scheduling.\n", "Routine scheduling.\nAdded.\n") + notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text=text) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "include_code": True}) + + by_id = {h["hit_id"]: h for h in r.structured_content["hits"]} + assert by_id[f"{notes}:3-4"]["location_verified"] is True + for stale in (f"{notes}:6-7", f"{notes}:9-10"): + assert by_id[stale]["location_verified"] is False + assert by_id[stale]["snippet"] is None and by_id[stale]["code"] is None + + +async def test_search_unlocatable_leader_keeps_its_own_range(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text="nothing\nof the\nindexed text\n") + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation"}) + + leader, *others = [h for h in r.structured_content["hits"] if h["file"] == str(notes)] + # colgrep's `unit` spans 3-10 over every matched section; the leader's own range is 3-4. + assert (leader["hit_id"], leader["location_verified"]) == (f"{notes}:3-4", False) + assert [h["hit_id"] for h in others] == [f"{notes}:6-7", f"{notes}:9-10"] + + +async def test_search_unit_without_a_name_is_never_verified(settings_env, monkeypatch, tmp_path): + # colgrep names no document or raw-code unit, so nothing ties its range to the file. + notes, _ = _matches_corpus( + tmp_path, monkeypatch, notes_matches=[_match(3, 4, "Logistics", 1.2), _match(6, 7, "", 0.9)] + ) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "include_code": True}) + + nameless = next(h for h in r.structured_content["hits"] if h["hit_id"] == f"{notes}:6-7") + assert nameless["location_verified"] is False + assert nameless["snippet"] is None and nameless["code"] is None + + +async def test_search_include_code_returns_a_verified_units_own_lines(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "include_code": True}) + + by_id = {h["hit_id"]: h for h in r.structured_content["hits"]} + assert by_id[f"{notes}:6-7"]["code"] == "## Escalation\n- **PI decision:** switch to a subagent." + assert by_id[f"{notes}:3-4"]["code"] == "## Logistics\nRoutine scheduling.\n" + + +async def test_search_one_entry_matches_is_a_lone_unit(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_matches=[_match(3, 4, "Logistics", 1.2)]) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation"}) + + assert [h["hit_id"] for h in r.structured_content["hits"] if h["file"] == str(notes)] == [f"{notes}:3-4"] + + +async def test_search_heading_with_a_dot_stays_verified(settings_env, monkeypatch, tmp_path): + text = "# Wave 05\n\n## Logistics\nRoutine scheduling.\n\n## v1.8 notes\nShip it.\n" + notes, _ = _matches_corpus( + tmp_path, + monkeypatch, + notes_text=text, + notes_matches=[_match(3, 4, "Logistics", 1.2), _match(6, 7, "v1.8 notes", 0.9)], + ) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "release notes"}) + + dotted = next(h for h in r.structured_content["hits"] if h["hit_id"] == f"{notes}:6-7") + assert dotted["location_verified"] is True + assert dotted["snippet"] == "## v1.8 notes\nShip it." + + async def test_duplicate_ranges_in_matches_count_once(settings_env, monkeypatch, tmp_path): notes, _ = _matches_corpus( tmp_path, From 1d896f8def8ed58a39ee3e1503aeac65b8eec001 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 00:59:24 +0900 Subject: [PATCH 09/17] perf(search): build only surviving units and verify leaders by exact span search built a SearchHit for every unit colgrep sent, then cut to limit, and read every hit file whole, split it twice (locate_unit, then the sibling slices) and scanned it for each leader's first line. The work grew with the files and units colgrep returned, not with the hits shown. _do_search now stable-sorts every unit by score (ties by colgrep's result and unit order) and cuts to limit before anything is read or built. A file's units are best first (unit_matches), so its survivors are a prefix and its best unit is among them. _plan_reads names the ranges the survivors need, _read_units reads them in one islice pass per file in asyncio.to_thread with one gather across files, and the pure builders take file -> {range: lines} (FileLines) instead of whole texts. A leader is verified by comparing the lines at colgrep's own range with its code (locate.span_holds, trailing whitespace ignored through the same rstripped helper locate_unit uses). Only when that fails (a file edited since indexing, or a pre-1.8.4 colgrep whose range is widened) is the whole file read, once, and locate_unit run. A whitespace-only edit since indexing now verifies. Spans split on \n alone, as colgrep counts lines, rather than str.splitlines (form feed, U+2028). Measured through Client(build()) with a stub adapter, timing _do_search alone, median of 5 invocations of 5 calls each, 15 markdown files of 2,000 lines with 30 matched sections each (450 units): | search | before (2afb0fa) | after | |:------------------|-----------------:|--------:| | limit=15 | 5.20 ms | 2.13 ms | | limit=None | 5.10 ms | 4.50 ms | An exhaustive listing builds every unit either way, so it gains only from the cheaper reads. A first version that stripped every span line in Python took 9.5 ms there; the spans now keep their newline and are stripped where used. Contract tests: a span-verified leader never calls Path.read_text; an edited file is read whole exactly once to relocate its leader, and the unedited file beside it not at all; the cut reads only the survivors' ranges; _read_units opens a file once for all its spans; the off-loop test now spies on open (the happy path no longer calls read_text); expand returns CRLF lines without terminators. On c4c291a the span-verified, edited-file (it read both files whole) and off-loop tests fail; the expand test passes there and pins what must not regress; the _read_units and cut tests exercise the new API. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/adapter.py | 4 + server/colgrep_mcp/locate.py | 31 ++- server/colgrep_mcp/tools_search.py | 291 ++++++++++++++++++----------- server/tests/fixture_paths.py | 2 +- server/tests/test_render.py | 32 ++-- server/tests/test_tools_search.py | 135 ++++++++----- 6 files changed, 320 insertions(+), 175 deletions(-) diff --git a/server/colgrep_mcp/adapter.py b/server/colgrep_mcp/adapter.py index 91e1d25..cb4e50c 100644 --- a/server/colgrep_mcp/adapter.py +++ b/server/colgrep_mcp/adapter.py @@ -81,6 +81,10 @@ class UnitMatch(NamedTuple): name: str score: float + @property + def span(self) -> tuple[int, int]: + return (self.line, self.end_line) + def _wire_match(entry: Any) -> UnitMatch | None: """A `matches` entry as a `UnitMatch`, or `None` when it is malformed or diff --git a/server/colgrep_mcp/locate.py b/server/colgrep_mcp/locate.py index bbce5a4..429d19c 100644 --- a/server/colgrep_mcp/locate.py +++ b/server/colgrep_mcp/locate.py @@ -82,13 +82,32 @@ def locate_unit(file_text: str, code: str, reported_line: int, reported_end: int return (reported_line, reported_end, False) +def rstripped(lines: list[str]) -> list[str]: + """`lines` with trailing whitespace dropped from each: the one normalisation + every comparison of a unit's `code` against file lines goes through (F4).""" + return [line.rstrip() for line in lines] + + +def span_holds(line: int, end_line: int, span_lines: list[str], code_lines: list[str]) -> bool: + """Whether the file's `span_lines`, read at `[line, end_line]`, are exactly `code_lines`. + + The cheap check behind `locate_unit`'s answer: colgrep's own range (since + 1.8.4, the unit's own, not the file's widened one) is right when it holds + as many lines as `code` and every one matches, trailing whitespace + ignored like `_full_matches`. A range that is shorter than the file at + that point (`span_lines` cut by EOF), or a different length from `code`, + never holds, so a stale index or a pre-1.8.4 binary falls through to + `locate_unit` over the whole text. A whitespace-only edit still holds. + """ + n = len(code_lines) + return ( + n > 0 and end_line - line + 1 == n and len(span_lines) == n and rstripped(span_lines) == rstripped(code_lines) + ) + + def _full_matches(candidates: list[int], file_lines: list[str], code_lines: list[str]) -> list[int]: """Candidates whose full block matches `code_lines`, trailing whitespace ignored per line (F4: a whitespace-only difference on a non-first line must not exclude the otherwise-correct occurrence).""" - - def _norm(lines: list[str]) -> list[str]: - return [line.rstrip() for line in lines] - - target = _norm(code_lines) - return [c for c in candidates if _norm(file_lines[c - 1 : c - 1 + len(code_lines)]) == target] + target = rstripped(code_lines) + return [c for c in candidates if rstripped(file_lines[c - 1 : c - 1 + len(code_lines)]) == target] diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index 490e3b3..311921f 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -14,8 +14,9 @@ import itertools import re import time -from collections.abc import Callable +from collections.abc import Callable, Iterable from contextlib import AsyncExitStack +from dataclasses import dataclass, field from pathlib import Path from typing import Annotated, NamedTuple @@ -26,7 +27,7 @@ from .adapter import RawHit, SearchRequest, UnitMatch, unit_matches from .errors import Code, note, tool_error, translate_adapter_errors -from .locate import locate_unit +from .locate import locate_unit, span_holds from .locks import project_lock from .logging_utils import safe_log from .models import ExpandedUnit, ExpandResult, FileHit, FileResult, SearchHit, SearchResult @@ -57,29 +58,47 @@ def _resolve_hit_file(file: str, base_path: Path | None) -> str: return str(base_path / file) +Span = tuple[int, int] + + +class FileLines(NamedTuple): + """What reading one hit file produced, for the pure builders (R05 F11). + + `spans` holds the lines at each range a hit will show or verify, read in + one pass and still ending in their newline (`_read_spans`); `text` is the + whole file, read only when a unit's own range did not hold its code + (`span_holds`), so `locate_unit` can search for it. An unreadable file is + `FileLines({})`. + """ + + spans: dict[Span, list[str]] + text: str | None = None + + def hit_from_raw( raw: RawHit, leader: UnitMatch, snippet_lines: int, include_code: bool, - file_cache: dict[str, str], + read: FileLines, base_path: Path | None = None, ) -> SearchHit: """Build the `SearchHit` of one raw colgrep result's best unit (`{"unit": {...}, "score": ...}`). `leader` is that unit's own `UnitMatch` (`adapter.unit_matches(raw)[0]`): `unit.line..end_line` is widened over every matched unit of the file, so - only `leader`'s range is the unit's, and the one to fall back to. Re-derives - `line`/`end_line` via `locate_unit` (R05 D1: colgrep's reported values are - frequently wrong), starting from that range, so `hit_id` is built from - trustworthy locations and an unlocatable unit keeps its own range instead - of one overlapping its siblings. Pure and I/O-free: `file_cache` must - already hold every hit file's text (or `""` for an unreadable one) — the - async caller fills it via `_fill_file_cache` (off the event loop, R05 F11) - before this runs, so an unreadable file degrades to an empty text rather - than raise, which makes `locate_unit` fall back with `verified=False`. - `base_path` (the first path the search ran against) resolves a relative - `unit.file` to absolute (F13) before `hit_id` is built. + only `leader`'s range is the unit's, and the one to fall back to. The + range is verified against the file (R05 D1: colgrep's reported values were + frequently wrong before 1.8.4): when the lines at it are exactly `code`, + trailing whitespace ignored, it is verified as it stands, so a + whitespace-only edit since indexing still verifies. Otherwise (an edited + file, an older colgrep) `locate_unit` searches `read.text` for the code, + and an unlocatable unit keeps its own range with `verified=False`, not one + overlapping its siblings. Pure and I/O-free: `read` was filled off the + event loop by `_read_units`, and an unreadable file degrades to the + unverified fallback rather than raise. `base_path` (the first path the + search ran against) resolves a relative `unit.file` to absolute (F13) + before `hit_id` is built. `name` is colgrep's display name for the unit, `Class.method` included; `qualified_name`, `unit_type` and `signature` come from `unit`. @@ -91,10 +110,16 @@ def hit_from_raw( unit = raw.get("unit") or {} file = _resolve_hit_file(unit.get("file") or "", base_path) code = unit.get("code") or "" + code_lines = code.splitlines() - line, end_line, verified = locate_unit(file_cache.get(file, ""), code, leader.line, leader.end_line) + held = read.spans.get(leader.span) + if held is not None and span_holds(leader.line, leader.end_line, held, code_lines): + line, end_line, verified = leader.line, leader.end_line, True + elif read.text is not None: + line, end_line, verified = locate_unit(read.text, code, leader.line, leader.end_line) + else: + line, end_line, verified = leader.line, leader.end_line, False - code_lines = code.splitlines() snippet = "\n".join(code_lines[:snippet_lines]) if code_lines else None return SearchHit( @@ -153,72 +178,136 @@ def _sibling_hit( unit_type="", language=leader.language, score=match.score, - snippet="\n".join(body[:snippet_lines]) if trusted and body else None, - code="\n".join(body) if trusted and include_code else None, + snippet=_joined(body[:snippet_lines]) if trusted and body else None, + code=_joined(body) if trusted and include_code else None, location_verified=trusted, ) def hits_from_raw( raw: RawHit, + matches: list[UnitMatch], + indexes: list[int], snippet_lines: int, include_code: bool, - file_cache: dict[str, str], + read: FileLines, base_path: Path | None = None, -) -> list[SearchHit]: - """Build one `SearchHit` per unit colgrep matched in one raw result's file. +) -> dict[int, SearchHit]: + """Build the hits of the `indexes` units of `matches` (`adapter.unit_matches(raw)`), by index. colgrep collapses its results to one per file (`collapse_by_file`, `colgrep/src/index/mod.rs`) and since 1.8.4 `unit_matches` lists each matched unit's own range and name. Showing only the best hid an answer sitting in a file's second-best markdown section, so the agent read the - whole file instead (OBS-M). The best unit goes through `hit_from_raw`; the - others through `_sibling_hit`, their text read from `file_cache`, already - filled for the leader: no further I/O. Output without `matches` (colgrep - 1.6, or a single matched unit) yields that one hit. + whole file instead (OBS-M). The best unit (index 0) goes through + `hit_from_raw`; the others through `_sibling_hit` over the lines + `_read_units` already read for their ranges: no further I/O. The best unit + is always located, whichever units `indexes` selects, since the others + are trusted only through it. Output without `matches` (colgrep 1.6, or a + single matched unit) is that one hit. + """ + leader = hit_from_raw(raw, matches[0], snippet_lines, include_code, read, base_path=base_path) + leader_exact = leader.location_verified and (leader.line, leader.end_line) == matches[0].span + return { + i: leader + if i == 0 + else _sibling_hit( + leader, matches[i], read.spans.get(matches[i].span, []), leader_exact, snippet_lines, include_code + ) + for i in indexes + } + + +def _joined(lines: list[str]) -> str: + """The text of `lines` as `_read_spans` returns them, minus the final newline. + + Text mode already folds `\\r\\n` and `\\r` into `\\n`, so this is what + `"\\n".join` of the lines stripped one by one gives, without the Python + loop. It breaks lines on `\\n` alone, like colgrep and unlike + `str.splitlines`, which also breaks on form feeds and U+2028. """ - matches = unit_matches(raw) - leader = hit_from_raw(raw, matches[0], snippet_lines, include_code, file_cache, base_path=base_path) - if len(matches) < 2: - return [leader] - - file_lines = file_cache.get(leader.file, "").splitlines() - leader_exact = leader.location_verified and (leader.line, leader.end_line) == (matches[0].line, matches[0].end_line) - return [leader] + [ - _sibling_hit(leader, m, file_lines[m.line - 1 : m.end_line], leader_exact, snippet_lines, include_code) - for m in matches[1:] - ] - - -async def _read_file_off_loop(file: str) -> str: - """`Path(file).read_text(errors="replace")`, run in a worker thread. - - A hit's file can be arbitrarily large; reading it directly on the event - loop would block every other in-flight request for as long as the read - takes (F11). An unreadable file degrades to `""` (the same fallback - `hit_from_raw` used to produce on `OSError`), not an exception. + text = "".join(lines) + return text[:-1] if text.endswith("\n") else text + + +def _read_spans(file: str, spans: Iterable[Span]) -> dict[Span, list[str]]: + """Read the lines at each `(line, end_line)` span (1-indexed, inclusive) of `file`, in one pass. + + A hit's file can be arbitrarily large, and `search` and `expand` show a few + spans of it: reading the whole file with `read_text` + `splitlines` does + I/O proportional to its size instead of to the spans asked for. Opening + the file and `itertools.islice`-ing its line iterator up to the largest + `end_line` reads (and decodes) that prefix once, however many spans share + it. The lines keep their `\\n` (`_joined` turns them back into text); + `errors="replace"` and the default text-mode encoding match + `Path.read_text(errors="replace")`; a span past EOF yields `[]`, and one + that crosses it, the lines up to EOF. """ + bounds = {span: (max(span[0] - 1, 0), max(span[1], span[0] - 1, 0)) for span in spans} + if not bounds: + return {} + with open(file, errors="replace") as f: + head = list(itertools.islice(f, max(end for _, end in bounds.values()))) + return {span: head[start:end] for span, (start, end) in bounds.items()} + + +@dataclass +class _Wanted: + """What one hit file must give back: the ranges to read, and each result's + best unit whose range has to hold its code (`FileLines`).""" + + spans: set[Span] = field(default_factory=set) + leaders: list[tuple[Span, list[str]]] = field(default_factory=list) + + +def _read_file_lines(file: str, wanted: _Wanted) -> FileLines: + """Blocking half of `_read_units` for one file: its spans in one pass, then + the whole text, once, if any leader's range did not hold its code.""" try: - return await asyncio.to_thread(Path(file).read_text, errors="replace") + spans = _read_spans(file, wanted.spans) except OSError: - return "" + return FileLines({}) + for (line, end_line), code_lines in wanted.leaders: + if not span_holds(line, end_line, spans.get((line, end_line), []), code_lines): + try: + return FileLines(spans, Path(file).read_text(errors="replace")) + except OSError: + break + return FileLines(spans) + +async def _read_units(plan: dict[str, _Wanted]) -> dict[str, FileLines]: + """Read what `plan` asks of every hit file, at most once per file, concurrently + and off the event loop (F11), so the pure builders never touch the disk.""" + files = list(plan) + reads = await asyncio.gather(*(asyncio.to_thread(_read_file_lines, file, plan[file]) for file in files)) + return dict(zip(files, reads, strict=True)) -async def _fill_file_cache(raw_hits: list[RawHit], file_cache: dict[str, str], base_path: Path | None = None) -> None: - """Read every distinct hit file referenced by `raw_hits` into `file_cache`, - at most once per file, concurrently and off the event loop (F11). - Keyed the same way `hit_from_raw` looks values up: `base_path` resolves a - relative `unit.file` to absolute first (F13), so a relative and an - equivalent already-absolute reference to the same file share one cache - entry and one read. +def _plan_reads( + raws: list[RawHit], files: list[str], matches: list[list[UnitMatch]], keep: list[tuple[int, int]] +) -> dict[str, _Wanted]: + """What `keep` (surviving `(result, unit)` pairs) needs read from each file. + + Every kept result's best unit is checked, since the rest of its file is + trusted only through it, but its range is read only when it is as long as + its code: a stale range or an older colgrep's widened one cannot hold it + and goes straight to the whole-file search, so reading it would be waste. + The other kept units each need their own range. """ - files = {_resolve_hit_file((raw.get("unit") or {}).get("file") or "", base_path) for raw in raw_hits} - missing = [f for f in files if f not in file_cache] - if not missing: - return - texts = await asyncio.gather(*(_read_file_off_loop(f) for f in missing)) - file_cache.update(zip(missing, texts, strict=True)) + plan: dict[str, _Wanted] = {} + for ri in {ri for ri, _ in keep}: + lead = matches[ri][0] + code_lines = ((raws[ri].get("unit") or {}).get("code") or "").splitlines() + wanted = plan.setdefault(files[ri], _Wanted()) + if code_lines: + wanted.leaders.append((lead.span, code_lines)) + if lead.end_line - lead.line + 1 == len(code_lines): + wanted.spans.add((lead.line, lead.end_line)) + for ri, mi in keep: + if mi: + plan[files[ri]].spans.add(matches[ri][mi].span) + return plan def _search_header(result: SearchResult) -> str: @@ -517,15 +606,19 @@ async def _do_search( snippet_lines: int, include_code: bool, ) -> SearchResult: - """`search`'s body: run the adapter, then convert every raw hit into one - `SearchHit` per matched unit (locate, snippet, `hit_id`) via - `hits_from_raw`. - - Units are ranked by score across files and `limit` caps units, not - files. colgrep's `-k limit` returns the top `limit` files by their best - unit, and every unit outside them scores at most the last file's best, - so those files already hold the top `limit` units; the cut below drops - the rest and sets `truncated`. + """`search`'s body: run the adapter, rank every matched unit, and build only the `limit` best. + + Units are ranked by score across files and `limit` caps units, not files. + colgrep's `-k limit` returns the top `limit` files by their best unit, + and every unit outside them scores at most the last file's best, so those + files already hold the top `limit` units. The stable sort over + `unit_matches` lists (score, then colgrep's result order, then unit + order) and the cut run before anything is read or built, so the work is + proportional to the units returned, not the units colgrep sent; `truncated` + is set when the cut dropped any. Each file's list is best first, so its + survivors are a prefix and its best unit is among them. `_plan_reads` then + names the lines the survivors need, `_read_units` reads them off the loop, + and `hits_from_raw` builds the hits. `find_files` no longer routes through here (R01 §C6): it calls `_run_adapter_search` directly and folds raw hits straight into @@ -554,17 +647,23 @@ async def _do_search( # (not expected from colgrep, but not guaranteed absent either) resolves # against. base_path = outcome.resolved[0] - file_cache: dict[str, str] = {} - await _fill_file_cache(outcome.raw_hits, file_cache, base_path=base_path) - hits = [ - hit - for raw in outcome.raw_hits - for hit in hits_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) - ] - hits.sort(key=lambda hit: hit.score, reverse=True) # stable: colgrep's order breaks ties - limited = limit is not None and len(hits) > limit - if limited: - hits = hits[:limit] + raws = outcome.raw_hits + matches = [unit_matches(raw) for raw in raws] + files = [_resolve_hit_file((raw.get("unit") or {}).get("file") or "", base_path) for raw in raws] + ranked = [(ri, mi) for ri, units in enumerate(matches) for mi in range(len(units))] + ranked.sort(key=lambda unit: -matches[unit[0]][unit[1]].score) # stable: colgrep's order breaks ties + limited = limit is not None and len(ranked) > limit + keep = ranked[:limit] if limited else ranked + + reads = await _read_units(_plan_reads(raws, files, matches, keep)) + indexes: dict[int, list[int]] = {} + for ri, mi in keep: + indexes.setdefault(ri, []).append(mi) + built = { + ri: hits_from_raw(raws[ri], matches[ri], mis, snippet_lines, include_code, reads[files[ri]], base_path) + for ri, mis in indexes.items() + } + hits = [built[ri][mi] for ri, mi in keep] notes: list[str] = [] if not hits: @@ -766,34 +865,6 @@ async def find_files( ) -def _strip_line_ending(line: str) -> str: - """Remove one trailing line terminator, exactly as `str.splitlines()` would per line.""" - if line.endswith("\r\n"): - return line[:-2] - if line.endswith("\n") or line.endswith("\r"): - return line[:-1] - return line - - -def _read_span(file: str, line: int, end_line: int) -> list[str]: - """Read only lines `[line, end_line]` (1-indexed, inclusive) of `file`. - - `expand` returns at most `max_lines` of a hit whose file can be - arbitrarily large; reading the whole file with `read_text` + - `splitlines` (the old approach) does I/O proportional to the file's - size instead of the span actually requested. Opening the file and - `itertools.islice`-ing its line iterator reads (and decodes) only the - requested span. `errors="replace"` and the default text-mode encoding - match `Path.read_text(errors="replace")`; a `line` past EOF yields `[]`, - same as the old `lines[start_idx:end_idx]` slicing. - """ - start_idx = max(line - 1, 0) - end_idx = max(end_line, start_idx) - with open(file, errors="replace") as f: - selected = list(itertools.islice(f, start_idx, end_idx)) - return [_strip_line_ending(raw_line) for raw_line in selected] - - async def expand( hit_ids: Annotated[ list[str], Field(description="hit_id values from a search/find_files result, e.g. '/repo/a.py:10-42'.") @@ -830,7 +901,7 @@ async def expand( try: # Off the event loop (F11): a hit's file can be arbitrarily # large, and this handler otherwise never awaits. - selected = await asyncio.to_thread(_read_span, file, line, end_line) + selected = (await asyncio.to_thread(_read_spans, file, [(line, end_line)]))[(line, end_line)] except OSError as exc: units.append( ExpandedUnit( @@ -849,7 +920,7 @@ async def expand( file=file, line=line, end_line=end_line, - code="\n".join(selected), + code=_joined(selected), truncated=was_truncated, ) ) diff --git a/server/tests/fixture_paths.py b/server/tests/fixture_paths.py index 91610a6..ec274fd 100644 --- a/server/tests/fixture_paths.py +++ b/server/tests/fixture_paths.py @@ -1,7 +1,7 @@ """Absolute path stand-ins for the suite's simulated "fake project". `fake_colgrep.py` never touches these on disk (a hit file that cannot be -read degrades to `""` in `_fill_file_cache`/`_read_file_off_loop`), but real +read degrades to an unverified hit in `_read_units`), but real code checks `pathlib.Path.is_absolute()` against them: the adapter's own absolute-path guard on `status`/`clear`/`search`, and `tools_search._resolve_hit_file`'s "already absolute, don't rebase" check. diff --git a/server/tests/test_render.py b/server/tests/test_render.py index e206670..7cdee1a 100644 --- a/server/tests/test_render.py +++ b/server/tests/test_render.py @@ -6,6 +6,7 @@ from colgrep_mcp.adapter import unit_matches from colgrep_mcp.models import FileHit, FileResult, SearchHit, SearchResult from colgrep_mcp.tools_search import ( + FileLines, _file_block, _files_header, _hit_block, @@ -39,10 +40,9 @@ def test_hit_from_raw_locates_true_lines_when_file_readable(tmp_path): target.write_text("x = 1\n\ndef parse_config(path: str) -> dict:\n return {}\n") raw = _raw_hit(file=str(target), line=1, end_line=1) # reported values are wrong (R05 D1) - # `hit_from_raw` is pure/I/O-free (F11): the async caller (`_fill_file_cache`) - # is responsible for populating `file_cache` before this runs. - file_cache = {str(target): target.read_text()} - hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache=file_cache) + # `hit_from_raw` is pure/I/O-free (F11): the async caller (`_read_units`) + # is responsible for reading the file before this runs. + hit = hit_from_raw(raw, unit_matches(raw)[0], 6, False, FileLines({}, target.read_text())) assert (hit.line, hit.end_line, hit.location_verified) == (3, 4, True) assert hit.hit_id == f"{target}:3-4" @@ -51,21 +51,21 @@ def test_hit_from_raw_locates_true_lines_when_file_readable(tmp_path): def test_hit_from_raw_falls_back_when_file_unreadable(): raw = _raw_hit(line=7, end_line=9) - hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], 6, False, FileLines({})) assert (hit.line, hit.end_line, hit.location_verified) == (7, 9, False) assert hit.hit_id == "/nonexistent/src/config.py:7-9" -def test_hit_from_raw_never_touches_disk_itself(tmp_path): - """F11: `hit_from_raw` must trust `file_cache` as given, never opening - the file itself — proven here by pointing `file` at a path that does not - exist on disk while pre-populating the cache with real text for it.""" - missing_path = tmp_path / "never-created.py" - raw = _raw_hit(file=str(missing_path), line=1, end_line=1) +def test_hit_from_raw_verifies_by_span_without_touching_disk(tmp_path): + """F11: `hit_from_raw` trusts the `FileLines` it is given, never opening the + file itself — proven by pointing `file` at a path that does not exist while + handing it the lines at colgrep's own range. A trailing-space difference + between those lines and `code` still verifies.""" + raw = _raw_hit(file=str(tmp_path / "never-created.py"), line=1, end_line=2) + read = FileLines({(1, 2): ["def parse_config(path: str) -> dict: ", " return {}"]}) - file_cache = {str(missing_path): "def parse_config(path: str) -> dict:\n return {}\n"} - hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache=file_cache) + hit = hit_from_raw(raw, unit_matches(raw)[0], 6, False, read) assert (hit.line, hit.end_line, hit.location_verified) == (1, 2, True) @@ -73,18 +73,18 @@ def test_hit_from_raw_never_touches_disk_itself(tmp_path): def test_hit_from_raw_snippet_truncated_and_code_gated(): raw = _raw_hit(code="line1\nline2\nline3\nline4\n") - hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=2, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], 2, False, FileLines({})) assert hit.snippet == "line1\nline2" assert hit.code is None - hit_full = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=2, include_code=True, file_cache={}) + hit_full = hit_from_raw(raw, unit_matches(raw)[0], 2, True, FileLines({})) assert hit_full.code == "line1\nline2\nline3\nline4\n" def test_hit_from_raw_tolerant_of_missing_optional_fields(): raw = {"unit": {"file": "/nonexistent/a.py"}, "score": 0.5} - hit = hit_from_raw(raw, unit_matches(raw)[0], snippet_lines=6, include_code=False, file_cache={}) + hit = hit_from_raw(raw, unit_matches(raw)[0], 6, False, FileLines({})) assert hit.name == "" assert hit.signature is None diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index 3536079..d32976a 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -19,7 +19,7 @@ from colgrep_mcp.locks import project_lock from colgrep_mcp.models import ExpandResult, FileResult, SearchResult from colgrep_mcp.server import build -from colgrep_mcp.tools_search import _fill_file_cache +from colgrep_mcp.tools_search import _read_units, _Wanted _FAKE_RAW_HIT = { "unit": { @@ -314,17 +314,17 @@ async def test_find_files_groups_hits_by_file_preserving_score_order(settings_en async def test_find_files_does_not_read_hit_files(settings_env, monkeypatch): """R01 §C5: `find_files` never exposes `line`/`hit_id`, so `_do_search` - must call it with `locate=False` and skip `_fill_file_cache` entirely — that - used to cost up to 300 file reads (plus a `locate_unit` pass each) per call - for output nothing downstream reads.""" + must skip `_read_units` entirely — that used to cost up to 300 file reads + (plus a `locate_unit` pass each) per call for output nothing downstream + reads.""" calls: list[None] = [] - real_fill_file_cache = tools_search._fill_file_cache + real_read_units = tools_search._read_units - async def spy(*args: object, **kwargs: object) -> None: + async def spy(*args: object, **kwargs: object) -> dict: calls.append(None) - return await real_fill_file_cache(*args, **kwargs) + return await real_read_units(*args, **kwargs) - monkeypatch.setattr(tools_search, "_fill_file_cache", spy) + monkeypatch.setattr(tools_search, "_read_units", spy) async with Client(build(), raise_exceptions=True) as c: r = await c.call_tool("find_files", {"query": "config parsing"}) @@ -357,21 +357,21 @@ def spy(*args: object, **kwargs: object): # --- off-loop file I/O (F11) -------------------------------------------- -def _spy_on_read_text(monkeypatch) -> list[int]: - """Record the thread identity `Path.read_text` actually runs on.""" - thread_ids: list[int] = [] +def _spy_on_read_text(monkeypatch) -> list[Path]: + """Record every `Path.read_text` call: a whole-file read.""" + reads: list[Path] = [] real_read_text = Path.read_text def spy(self: Path, *args: object, **kwargs: object) -> str: - thread_ids.append(threading.get_ident()) + reads.append(self) return real_read_text(self, *args, **kwargs) monkeypatch.setattr(Path, "read_text", spy) - return thread_ids + return reads def _spy_on_open(monkeypatch) -> list[int]: - """Record the thread identity the builtin `open` (what `_read_span` uses) runs on.""" + """Record the thread identity the builtin `open` (what `_read_spans` uses) runs on.""" thread_ids: list[int] = [] real_open = builtins.open @@ -385,7 +385,8 @@ def spy(*args: object, **kwargs: object): async def test_search_reads_hit_files_off_the_event_loop(settings_env, tmp_path, monkeypatch): """F11: a hit's file is read via `asyncio.to_thread`, not directly on the - event loop thread that is also running the rest of the server.""" + event loop thread that is also running the rest of the server. With colgrep's + own range exact, the one span read is the whole of it (`open`, as `expand`).""" target = tmp_path / "real_source.py" target.write_text("def real_func():\n return 42\n") @@ -396,7 +397,7 @@ async def test_search_reads_hit_files_off_the_event_loop(settings_env, tmp_path, "qualified_name": "real_source.py::real_func", "file": str(target), "line": 1, - "end_line": 1, + "end_line": 2, "language": "python", "unit_type": "function", "signature": "def real_func()", @@ -410,7 +411,7 @@ async def test_search_reads_hit_files_off_the_event_loop(settings_env, tmp_path, monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) main_thread_id = threading.get_ident() - read_thread_ids = _spy_on_read_text(monkeypatch) + read_thread_ids = _spy_on_open(monkeypatch) async with Client(build(), raise_exceptions=True) as c: r = await c.call_tool("search", {"query": "real func"}) @@ -421,41 +422,81 @@ async def test_search_reads_hit_files_off_the_event_loop(settings_env, tmp_path, assert all(tid != main_thread_id for tid in read_thread_ids) -async def test_fill_file_cache_reads_each_distinct_file_at_most_once(tmp_path, monkeypatch): - """F11: the caching behaviour that used to live inside `hit_from_raw` - (read each file at most once per call) now belongs to `_fill_file_cache`, - the async helper that populates `file_cache` before `hit_from_raw` runs.""" +async def test_read_units_reads_each_distinct_file_once_for_all_its_spans(tmp_path, monkeypatch): + """F11: `_read_units` is the one async step before the pure builders: a file + is opened once however many spans (and leaders) of it are wanted.""" target = tmp_path / "config.py" - target.write_text("def parse_config(path: str) -> dict:\n return {}\n") + target.write_text("def parse_config(path: str) -> dict:\n return {}\n\nclass Settings:\n k: int = 15\n") + other = tmp_path / "other.py" + other.write_text("x = 1\n") - read_calls: list[Path] = [] - real_read_text = Path.read_text + opened: list[str] = [] + real_open = builtins.open - def spy(self: Path, *args: object, **kwargs: object) -> str: - read_calls.append(self) - return real_read_text(self, *args, **kwargs) + def spy(file: object, *args: object, **kwargs: object): + opened.append(str(file)) + return real_open(file, *args, **kwargs) - monkeypatch.setattr(Path, "read_text", spy) + monkeypatch.setattr(builtins, "open", spy) + + wanted = _Wanted( + spans={(1, 2), (4, 5)}, leaders=[((1, 2), ["def parse_config(path: str) -> dict:", " return {}"])] + ) + reads = await _read_units({str(target): wanted, str(other): _Wanted(spans={(1, 1)})}) + + assert opened.count(str(target)) == 1 + assert reads[str(target)].spans == { + (1, 2): ["def parse_config(path: str) -> dict:\n", " return {}\n"], + (4, 5): ["class Settings:\n", " k: int = 15\n"], + } + assert reads[str(target)].text is None # the leader held, so no whole-file read + assert reads[str(other)].spans == {(1, 1): ["x = 1\n"]} + + +async def test_search_verifies_a_leader_by_its_span_without_reading_the_whole_file(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch) + whole_reads = _spy_on_read_text(monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation"}) + + assert all(h["location_verified"] for h in r.structured_content["hits"]) + assert whole_reads == [] - raw_hits = [ - {"unit": {"file": str(target)}, "score": 1.0}, - {"unit": {"file": str(target)}, "score": 0.5}, # same file, second hit - ] - file_cache: dict[str, str] = {} - await _fill_file_cache(raw_hits, file_cache) - assert file_cache[str(target)] == "def parse_config(path: str) -> dict:\n return {}\n" - assert len(read_calls) == 1 # only one actual read for two hits on the same file +async def test_search_reads_an_edited_file_whole_once_to_relocate_its_leader(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text="new\nlines\n" + _NOTES) + whole_reads = _spy_on_read_text(monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation"}) + + assert f"{notes}:5-6" in [h["hit_id"] for h in r.structured_content["hits"]] + assert whole_reads == [notes] + + +async def test_search_cut_reads_and_builds_only_the_surviving_units(settings_env, monkeypatch, tmp_path): + notes, tools = _matches_corpus(tmp_path, monkeypatch) + spans_read: list[set] = [] + real_read_spans = tools_search._read_spans - # Mutate the cache in place: a second call must trust it, not re-read. - file_cache[str(target)] = "mutated" - await _fill_file_cache(raw_hits, file_cache) - assert len(read_calls) == 1 - assert file_cache[str(target)] == "mutated" + def spy(file: str, spans): + spans = list(spans) + spans_read.append({(Path(file).name, *s) for s in spans}) + return real_read_spans(file, spans) + + monkeypatch.setattr(tools_search, "_read_spans", spy) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "limit": 2}) + + assert [h["hit_id"] for h in r.structured_content["hits"]] == [f"{notes}:3-4", f"{tools}:1-2"] + # Only the two survivors' ranges were read: the cut units' (6-7, 9-10) never were. + assert sorted(span for group in spans_read for span in group) == [("notes.md", 3, 4), ("tools.py", 1, 2)] async def test_expand_reads_files_off_the_event_loop(settings_env, tmp_path, monkeypatch): - """F11, mirrored for `expand`. `expand` reads its span via `_read_span`'s + """F11, mirrored for `expand`. `expand` reads its span via `_read_spans`'s builtin `open`, not `Path.read_text`, so the spy target follows.""" target = tmp_path / "code.py" target.write_text("x = 1\ny = 2\ndef f():\n return 1\nz = 3\n") @@ -493,6 +534,16 @@ async def test_expand_happy_path_reads_exact_span(settings_env, tmp_path): assert f"```{hit_id}" in r.content[0].text +async def test_expand_returns_lines_without_their_terminators(settings_env, tmp_path): + target = tmp_path / "crlf.txt" + target.write_bytes(b"a\r\n\r\nb\r\nlast") + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("expand", {"hit_ids": [f"{target}:1-2", f"{target}:3-9"]}) + + assert [u["code"] for u in r.structured_content["units"]] == ["a\n", "b\nlast"] + + async def test_expand_truncates_to_max_lines(settings_env, tmp_path): target = tmp_path / "code.py" target.write_text("\n".join(f"line{i}" for i in range(1, 11)) + "\n") From cc9345a516c73b06d38af51f9c7acbb576704fc8 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:01:06 +0900 Subject: [PATCH 10/17] perf(search): read each file's expanded spans in one pass search now yields one hit per matched unit, so an agent expanding the hits that matter passes several hit_ids of one file as the common case, and expand opened and islice-d the file once per id. expand groups the parsed ids by file and makes one _read_spans pass per file, in asyncio.to_thread with one gather across files, then answers in input order. Overlapping and non-contiguous spans share the pass: it reads the prefix up to the largest end_line once and slices each span from it. A malformed id still yields its own error unit, and an OSError is attached to every id of that file. Measured through Client(build()), the whole expand call, median of 5 invocations of 5 calls each, 5 ids of one 2,000-line markdown file (about 66 lines each, scratch corpus): | expand, 5 ids of one file | before (86d3254) | after | |:--------------------------|-----------------:|--------:| | whole call | 0.76 ms | 0.42 ms | The contract test gives three ids of one file, out of order and one overlapping, and checks the spans in input order and a single open; it fails on c4c291a (three opens). A second pins that malformed ids and an unreadable file keep their errors per id under grouping. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/tools_search.py | 37 +++++++++++++++++------- server/tests/test_tools_search.py | 45 ++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index 311921f..14930e2 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -251,6 +251,15 @@ def _read_spans(file: str, spans: Iterable[Span]) -> dict[Span, list[str]]: return {span: head[start:end] for span, (start, end) in bounds.items()} +async def _read_spans_or_error(file: str, spans: Iterable[Span]) -> dict[Span, list[str]] | OSError: + """`_read_spans` in a worker thread, an unreadable file returned rather than raised so + `expand` can attach it to each of that file's ids.""" + try: + return await asyncio.to_thread(_read_spans, file, spans) + except OSError as exc: + return exc + + @dataclass class _Wanted: """What one hit file must give back: the ranges to read, and each result's @@ -881,9 +890,20 @@ async def expand( """ # No adapter/lock involved: expand reads the filesystem directly, so # `ctx` (required for MCPServer's context injection) goes unused here. + parsed = [(hit_id, _HIT_ID_RE.match(hit_id)) for hit_id in hit_ids] + wanted: dict[str, set[Span]] = {} + for _, match in parsed: + if match: + wanted.setdefault(match.group(1), set()).add((int(match.group(2)), int(match.group(3)))) + # One pass per file however many of its ids were given (several units of one + # file is the common case, since `search` yields one hit per matched unit), + # off the event loop (F11): a hit's file can be arbitrarily large, and this + # handler otherwise never awaits. + files = list(wanted) + reads = dict(zip(files, await asyncio.gather(*(_read_spans_or_error(f, wanted[f]) for f in files)), strict=True)) + units: list[ExpandedUnit] = [] - for hit_id in hit_ids: - match = _HIT_ID_RE.match(hit_id) + for hit_id, match in parsed: if not match: units.append( ExpandedUnit( @@ -896,20 +916,17 @@ async def expand( ) continue - file, line_s, end_s = match.group(1), match.group(2), match.group(3) - line, end_line = int(line_s), int(end_s) - try: - # Off the event loop (F11): a hit's file can be arbitrarily - # large, and this handler otherwise never awaits. - selected = (await asyncio.to_thread(_read_spans, file, [(line, end_line)]))[(line, end_line)] - except OSError as exc: + file, line, end_line = match.group(1), int(match.group(2)), int(match.group(3)) + read = reads[file] + if isinstance(read, OSError): units.append( ExpandedUnit( - hit_id=hit_id, file=file, line=line, end_line=end_line, error=f"could not read {file}: {exc}" + hit_id=hit_id, file=file, line=line, end_line=end_line, error=f"could not read {file}: {read}" ) ) continue + selected = read[(line, end_line)] was_truncated = len(selected) > max_lines if was_truncated: selected = selected[:max_lines] diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index d32976a..e7e053c 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -534,6 +534,51 @@ async def test_expand_happy_path_reads_exact_span(settings_env, tmp_path): assert f"```{hit_id}" in r.content[0].text +async def test_expand_reads_a_file_once_for_all_its_ids_and_answers_in_input_order(settings_env, tmp_path, monkeypatch): + target = tmp_path / "ten.txt" + target.write_text("".join(f"line {n}\n" for n in range(1, 11))) + opened: list[str] = [] + real_open = builtins.open + + def spy(file: object, *args: object, **kwargs: object): + opened.append(str(file)) + return real_open(file, *args, **kwargs) + + monkeypatch.setattr(builtins, "open", spy) + + # Out of order, one span overlapping another, one not touching the rest. + ids = [f"{target}:6-8", f"{target}:2-3", f"{target}:7-9", f"{target}:10-10"] + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("expand", {"hit_ids": ids}) + + units = r.structured_content["units"] + assert [u["hit_id"] for u in units] == ids + assert [u["code"] for u in units] == [ + "line 6\nline 7\nline 8", + "line 2\nline 3", + "line 7\nline 8\nline 9", + "line 10", + ] + assert opened.count(str(target)) == 1 + + +async def test_expand_errors_stay_per_id_when_ids_share_a_file(settings_env, tmp_path): + real = tmp_path / "real.txt" + real.write_text("only\n") + missing = tmp_path / "missing.txt" + ids = [f"{missing}:1-2", "not a hit id", f"{missing}:3-4", f"{real}:1-1"] + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("expand", {"hit_ids": ids}) + + units = r.structured_content["units"] + assert [u["hit_id"] for u in units] == ids + assert units[0]["error"].startswith(f"could not read {missing}") + assert units[2]["error"].startswith(f"could not read {missing}") + assert units[1]["error"].startswith(f"[{Code.BAD_HIT_ID}]") + assert (units[3]["code"], units[3]["error"]) == ("only", None) + + async def test_expand_returns_lines_without_their_terminators(settings_env, tmp_path): target = tmp_path / "crlf.txt" target.write_bytes(b"a\r\n\r\nb\r\nlast") From 97bfd3d54e4f4cce9cd6a5e9813bd102e24c2df8 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:18:56 +0900 Subject: [PATCH 11/17] fix(search): trust a file's other units through this search's index refresh The verifier broke the wording check that decided whether a non-best unit's range could be trusted. A name ending in a dot (colgrep keeps `Title doc..Next steps.`) has an empty last dotted segment, which occurs in every line; duplicate headings and a name merely mentioned in a line passed; and the continuation chunks of a long markdown section, whose first line is body text, failed closed: on an unedited statera copy 13 of 110 hits lost their snippet and a LOCATION_UNVERIFIED note fired on every query, where c4c291a had none. The check is replaced by a guarantee. Before a search without --no-update, colgrep re-checks every file's mtime and size and re-indexes the changed ones (commands/search.rs load_index, compute_update_plan in index/mod.rs), so a file not modified since the call began was indexed as it is, and colgrep's ranges for it are exact. A non-best unit is trusted iff the best was span-verified exactly at its own range, the call did not pass skip_index_update, and the file's st_mtime_ns, read with fstat on the handle _read_spans_from already opens, is at most the time_ns taken just before colgrep was spawned. The builder stays I/O-free; the mtime travels in FileLines. A unit's name no longer matters, so an unnamed one is trusted like any other. An untrusted unit still has no snippet and no code, and the location_verified description states the rule. One premise of the intended rule is false and is left out: colgrep prints "Index is being updated by another process, searching existing index..." only when !quiet, and `--json` sets quiet (search.rs `let quiet = json || files_only`, passed to load_index). Probed on a scratch corpus with the index lock held: `search --json` exits 0 with empty stderr, the same call without --json prints the line. So a search that raced another indexer cannot be told apart, and its other units are still trusted. The window is another process holding the lock; this server serialises its own calls per project. Tests through Client(build()): a file modified after the search began and a skip_index_update search both leave the other units unverified with no snippet or code; an unedited file whose units are a mid-body chunk, a trailing-dot name and an unnamed range is verified in full with snippets and no LOCATION_UNVERIFIED note. They replace the name-check tests. The unedited-file and skip_index_update tests fail on 2bd2a8c. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/models.py | 9 ++- server/colgrep_mcp/tools_search.py | 120 ++++++++++++++++++----------- server/tests/test_tools_search.py | 83 +++++++++++++------- 3 files changed, 133 insertions(+), 79 deletions(-) diff --git a/server/colgrep_mcp/models.py b/server/colgrep_mcp/models.py index e1777fe..ff95a69 100644 --- a/server/colgrep_mcp/models.py +++ b/server/colgrep_mcp/models.py @@ -24,10 +24,11 @@ class SearchHit(BaseModel): location_verified: bool = Field( default=False, description="True when `line`/`end_line` were checked against the file (R05 D1: colgrep's own " - "line/end_line are frequently wrong). A file's best unit is checked by finding its code there; " - "each other unit of the file only when the best sits exactly at colgrep's range for it and this " - "unit's name occurs in the range's first line. False means colgrep's range, unchecked; such a " - "non-best unit has no snippet or code.", + "line/end_line were frequently wrong). A file's best unit is checked by finding its code there. " + "Each other unit of the file is trusted when the best sits exactly at colgrep's range for it, this " + "search refreshed colgrep's index (not `skip_index_update`), and the file was not modified after " + "the search began: colgrep then indexed the file as it is. False means colgrep's range, " + "unchecked; such a non-best unit has no snippet or code.", ) diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index 14930e2..81f6f60 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -12,13 +12,14 @@ import asyncio import itertools +import os import re import time from collections.abc import Callable, Iterable from contextlib import AsyncExitStack from dataclasses import dataclass, field from pathlib import Path -from typing import Annotated, NamedTuple +from typing import Annotated, NamedTuple, TextIO from mcp.server import MCPServer from mcp.server.mcpserver import Context @@ -65,7 +66,8 @@ class FileLines(NamedTuple): """What reading one hit file produced, for the pure builders (R05 F11). `spans` holds the lines at each range a hit will show or verify, read in - one pass and still ending in their newline (`_read_spans`); `text` is the + one pass and still ending in their newline (`_read_spans`); `mtime_ns` is + the file's modification time, from the handle that read them; `text` is the whole file, read only when a unit's own range did not hold its code (`span_holds`), so `locate_unit` can search for it. An unreadable file is `FileLines({})`. @@ -73,6 +75,7 @@ class FileLines(NamedTuple): spans: dict[Span, list[str]] text: str | None = None + mtime_ns: int | None = None def hit_from_raw( @@ -143,7 +146,7 @@ def _sibling_hit( leader: SearchHit, match: UnitMatch, body: list[str], - leader_exact: bool, + file_current: bool, snippet_lines: int, include_code: bool, ) -> SearchHit: @@ -151,23 +154,13 @@ def _sibling_hit( `body` is the lines the file holds at the unit's range. Nothing here can check them against the unit's code, so the range is trusted - (`location_verified`) only on two grounds. The leader was located exactly - at its own range (`leader_exact`), so the file still agrees with the - index's line numbers there; and the last dotted segment of the unit's - display name occurs in the range's first line, so an edit between the two - units has not moved this one. A heading that holds a dot (`v1.8 notes`) - still passes, an unnamed unit (documents and raw code have an empty name, - `UnitMatch`) never does. An untrusted unit gets no snippet and no code, - for text at a moved range would sit under another unit's name. It has no - `qualified_name`, `unit_type` or `signature`, since colgrep reports none - for it. + (`location_verified`) only when `file_current` says colgrep's ranges for + this file are exact (`hits_from_raw`), and the file reaches the range. + An untrusted unit gets no snippet and no code, for text at a moved range + would sit under another unit's name. It has no `qualified_name`, + `unit_type` or `signature`, since colgrep reports none for it. """ - trusted = ( - leader_exact - and len(body) == match.end_line - match.line + 1 - and bool(match.name) - and match.name.rsplit(".", 1)[-1] in body[0] - ) + trusted = file_current and len(body) == match.end_line - match.line + 1 return SearchHit( hit_id=f"{leader.file}:{match.line}-{match.end_line}", file=leader.file, @@ -191,6 +184,7 @@ def hits_from_raw( snippet_lines: int, include_code: bool, read: FileLines, + index_as_of_ns: int | None, base_path: Path | None = None, ) -> dict[int, SearchHit]: """Build the hits of the `indexes` units of `matches` (`adapter.unit_matches(raw)`), by index. @@ -201,18 +195,35 @@ def hits_from_raw( sitting in a file's second-best markdown section, so the agent read the whole file instead (OBS-M). The best unit (index 0) goes through `hit_from_raw`; the others through `_sibling_hit` over the lines - `_read_units` already read for their ranges: no further I/O. The best unit - is always located, whichever units `indexes` selects, since the others - are trusted only through it. Output without `matches` (colgrep 1.6, or a - single matched unit) is that one hit. + `_read_units` already read for their ranges: no further I/O. Output without + `matches` (colgrep 1.6, or a single matched unit) is that one hit. + + The other units arrive without `code`, so their ranges are trusted on a + guarantee, not a look at the text. Before a search without `--no-update` + colgrep re-checks every file's mtime and size and re-indexes the changed + ones (`commands/search.rs` `load_index`, `compute_update_plan` in + `index/mod.rs`), so a file not modified since the search began (`read.mtime_ns <= + index_as_of_ns`, `None` when the update was skipped) was indexed as it is + now, and the best unit located exactly at its own range shows it. A + heading's wording says nothing of this: chunks of a long section start + mid-body, headings repeat, and a name can end in a dot. The guarantee does + not hold while another process holds the index lock: colgrep then + searches the stale index and, under `--json`, says nothing of it + (`!quiet` gates the notice in `load_index`). """ leader = hit_from_raw(raw, matches[0], snippet_lines, include_code, read, base_path=base_path) - leader_exact = leader.location_verified and (leader.line, leader.end_line) == matches[0].span + file_current = ( + leader.location_verified + and (leader.line, leader.end_line) == matches[0].span + and index_as_of_ns is not None + and read.mtime_ns is not None + and read.mtime_ns <= index_as_of_ns + ) return { i: leader if i == 0 else _sibling_hit( - leader, matches[i], read.spans.get(matches[i].span, []), leader_exact, snippet_lines, include_code + leader, matches[i], read.spans.get(matches[i].span, []), file_current, snippet_lines, include_code ) for i in indexes } @@ -231,23 +242,26 @@ def _joined(lines: list[str]) -> str: def _read_spans(file: str, spans: Iterable[Span]) -> dict[Span, list[str]]: - """Read the lines at each `(line, end_line)` span (1-indexed, inclusive) of `file`, in one pass. + """`_read_spans_from` over `file`, opened here.""" + with open(file, errors="replace") as f: + return _read_spans_from(f, spans) + + +def _read_spans_from(f: TextIO, spans: Iterable[Span]) -> dict[Span, list[str]]: + """Read the lines at each `(line, end_line)` span (1-indexed, inclusive) of the open file `f`, in one pass. A hit's file can be arbitrarily large, and `search` and `expand` show a few spans of it: reading the whole file with `read_text` + `splitlines` does - I/O proportional to its size instead of to the spans asked for. Opening - the file and `itertools.islice`-ing its line iterator up to the largest - `end_line` reads (and decodes) that prefix once, however many spans share - it. The lines keep their `\\n` (`_joined` turns them back into text); - `errors="replace"` and the default text-mode encoding match - `Path.read_text(errors="replace")`; a span past EOF yields `[]`, and one - that crosses it, the lines up to EOF. + I/O proportional to its size instead of to the spans asked for. + `itertools.islice`-ing the line iterator up to the largest `end_line` + reads (and decodes) that prefix once, however many spans share it. The + lines keep their `\\n` (`_joined` turns them back into text); the handle + should be opened with `errors="replace"` and the default text-mode + encoding to match `Path.read_text(errors="replace")`. A span past EOF + yields `[]`, and one that crosses it, the lines up to EOF. """ bounds = {span: (max(span[0] - 1, 0), max(span[1], span[0] - 1, 0)) for span in spans} - if not bounds: - return {} - with open(file, errors="replace") as f: - head = list(itertools.islice(f, max(end for _, end in bounds.values()))) + head = list(itertools.islice(f, max((end for _, end in bounds.values()), default=0))) return {span: head[start:end] for span, (start, end) in bounds.items()} @@ -270,19 +284,25 @@ class _Wanted: def _read_file_lines(file: str, wanted: _Wanted) -> FileLines: - """Blocking half of `_read_units` for one file: its spans in one pass, then - the whole text, once, if any leader's range did not hold its code.""" - try: - spans = _read_spans(file, wanted.spans) - except OSError: - return FileLines({}) + """Blocking half of `_read_units` for one file: its spans in one pass (and its + mtime, from the same handle), then the whole text, once, if any leader's + range did not hold its code.""" + spans: dict[Span, list[str]] = {} + mtime_ns = None + if wanted.spans: + try: + with open(file, errors="replace") as f: + spans = _read_spans_from(f, wanted.spans) + mtime_ns = os.fstat(f.fileno()).st_mtime_ns + except OSError: + return FileLines({}) for (line, end_line), code_lines in wanted.leaders: if not span_holds(line, end_line, spans.get((line, end_line), []), code_lines): try: - return FileLines(spans, Path(file).read_text(errors="replace")) + return FileLines(spans, Path(file).read_text(errors="replace"), mtime_ns) except OSError: break - return FileLines(spans) + return FileLines(spans, None, mtime_ns) async def _read_units(plan: dict[str, _Wanted]) -> dict[str, FileLines]: @@ -467,6 +487,10 @@ class _RawSearchOutcome(NamedTuple): resolved: list[Path] elapsed_ms: int index_updated: bool # R05 D7, derived from stderr chatter + #: `time.time_ns()` taken just before colgrep was spawned, or `None` when the call + #: skipped colgrep's index refresh: a file not modified after it was indexed as it + #: is (`hits_from_raw`). + index_as_of_ns: int | None async def _run_adapter_search( @@ -544,6 +568,7 @@ async def on_stderr(line: str) -> None: for p in distinct_paths: await stack.enter_async_context(project_lock(p)) async with translate_adapter_errors(path=resolved[0]): + spawned_ns = time.time_ns() raw_hits = await adapter.search(req) elapsed_ms = int((time.monotonic() - start) * 1000) @@ -559,6 +584,7 @@ async def on_stderr(line: str) -> None: resolved=resolved, elapsed_ms=elapsed_ms, index_updated=index_updated, + index_as_of_ns=None if skip_index_update else spawned_ns, ) @@ -669,7 +695,9 @@ async def _do_search( for ri, mi in keep: indexes.setdefault(ri, []).append(mi) built = { - ri: hits_from_raw(raws[ri], matches[ri], mis, snippet_lines, include_code, reads[files[ri]], base_path) + ri: hits_from_raw( + raws[ri], matches[ri], mis, snippet_lines, include_code, reads[files[ri]], outcome.index_as_of_ns, base_path + ) for ri, mis in indexes.items() } hits = [built[ri][mi] for ri, mi in keep] diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index e7e053c..ad8ff50 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -7,7 +7,9 @@ import asyncio import builtins import json +import os import threading +import time from pathlib import Path import pytest @@ -478,14 +480,14 @@ async def test_search_reads_an_edited_file_whole_once_to_relocate_its_leader(set async def test_search_cut_reads_and_builds_only_the_surviving_units(settings_env, monkeypatch, tmp_path): notes, tools = _matches_corpus(tmp_path, monkeypatch) spans_read: list[set] = [] - real_read_spans = tools_search._read_spans + real_read_spans = tools_search._read_spans_from - def spy(file: str, spans): + def spy(f, spans): spans = list(spans) - spans_read.append({(Path(file).name, *s) for s in spans}) - return real_read_spans(file, spans) + spans_read.append({(Path(f.name).name, *s) for s in spans}) + return real_read_spans(f, spans) - monkeypatch.setattr(tools_search, "_read_spans", spy) + monkeypatch.setattr(tools_search, "_read_spans_from", spy) async with Client(build(), raise_exceptions=True) as c: r = await c.call_tool("search", {"query": "escalation", "limit": 2}) @@ -814,13 +816,21 @@ async def test_find_files_counts_each_matched_unit(settings_env, monkeypatch, tm assert (files[1]["hits"], files[1]["top_units"]) == (1, ["escalate"]) -async def test_search_unit_after_an_edit_below_the_leader_is_unverified_without_text( +def _touch_after_search_starts(path: Path) -> None: + """Give `path` a modification time no search started now can have indexed.""" + future = time.time_ns() + 3_600 * 10**9 + os.utime(path, ns=(future, future)) + + +async def test_search_units_of_a_file_modified_after_the_search_began_are_unverified_without_text( settings_env, monkeypatch, tmp_path ): - # A line added under the best section: the leader still sits at its own range, - # but every later section has moved, so colgrep's ranges for them hold other lines. + # A line added under the best section after colgrep's refresh: the best unit still sits + # at its own range, but every later section has moved, so colgrep's ranges for them + # hold other lines. text = _NOTES.replace("Routine scheduling.\n", "Routine scheduling.\nAdded.\n") notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text=text) + _touch_after_search_starts(notes) async with Client(build(), raise_exceptions=True) as c: r = await c.call_tool("search", {"query": "escalation", "include_code": True}) @@ -832,6 +842,19 @@ async def test_search_unit_after_an_edit_below_the_leader_is_unverified_without_ assert by_id[stale]["snippet"] is None and by_id[stale]["code"] is None +async def test_search_units_are_unverified_when_the_index_refresh_was_skipped(settings_env, monkeypatch, tmp_path): + notes, _ = _matches_corpus(tmp_path, monkeypatch) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "skip_index_update": True, "include_code": True}) + + by_id = {h["hit_id"]: h for h in r.structured_content["hits"] if h["file"] == str(notes)} + assert by_id[f"{notes}:3-4"]["location_verified"] is True # the best unit is checked by its code + for other in (f"{notes}:6-7", f"{notes}:9-10"): + assert by_id[other]["location_verified"] is False + assert by_id[other]["snippet"] is None and by_id[other]["code"] is None + + async def test_search_unlocatable_leader_keeps_its_own_range(settings_env, monkeypatch, tmp_path): notes, _ = _matches_corpus(tmp_path, monkeypatch, notes_text="nothing\nof the\nindexed text\n") @@ -844,20 +867,6 @@ async def test_search_unlocatable_leader_keeps_its_own_range(settings_env, monke assert [h["hit_id"] for h in others] == [f"{notes}:6-7", f"{notes}:9-10"] -async def test_search_unit_without_a_name_is_never_verified(settings_env, monkeypatch, tmp_path): - # colgrep names no document or raw-code unit, so nothing ties its range to the file. - notes, _ = _matches_corpus( - tmp_path, monkeypatch, notes_matches=[_match(3, 4, "Logistics", 1.2), _match(6, 7, "", 0.9)] - ) - - async with Client(build(), raise_exceptions=True) as c: - r = await c.call_tool("search", {"query": "escalation", "include_code": True}) - - nameless = next(h for h in r.structured_content["hits"] if h["hit_id"] == f"{notes}:6-7") - assert nameless["location_verified"] is False - assert nameless["snippet"] is None and nameless["code"] is None - - async def test_search_include_code_returns_a_verified_units_own_lines(settings_env, monkeypatch, tmp_path): notes, _ = _matches_corpus(tmp_path, monkeypatch) @@ -878,21 +887,37 @@ async def test_search_one_entry_matches_is_a_lone_unit(settings_env, monkeypatch assert [h["hit_id"] for h in r.structured_content["hits"] if h["file"] == str(notes)] == [f"{notes}:3-4"] -async def test_search_heading_with_a_dot_stays_verified(settings_env, monkeypatch, tmp_path): - text = "# Wave 05\n\n## Logistics\nRoutine scheduling.\n\n## v1.8 notes\nShip it.\n" +async def test_search_trusts_every_unit_of_an_unedited_file_whatever_its_first_line_says( + settings_env, monkeypatch, tmp_path +): + # A wording check would fail all three: a chunk of a long section starts mid-body, a name can + # end in a dot (so its last dotted segment is empty), and documents have no name at all. + text = ( + "# Wave 05\n\n## Logistics\nRoutine scheduling.\n\n" + "Escalation continued: a long section's second chunk.\n- **PI decision:** switch to a subagent.\n" + "## Title doc..Next steps.\nWave closed.\n" + ) notes, _ = _matches_corpus( tmp_path, monkeypatch, notes_text=text, - notes_matches=[_match(3, 4, "Logistics", 1.2), _match(6, 7, "v1.8 notes", 0.9)], + notes_matches=[ + _match(3, 4, "Logistics", 1.2), + _match(6, 7, "Escalation", 0.9), + _match(8, 9, "Title doc..Next steps.", 0.5), + _match(1, 2, "", 0.1), + ], ) async with Client(build(), raise_exceptions=True) as c: - r = await c.call_tool("search", {"query": "release notes"}) + r = await c.call_tool("search", {"query": "escalation", "include_code": True}) - dotted = next(h for h in r.structured_content["hits"] if h["hit_id"] == f"{notes}:6-7") - assert dotted["location_verified"] is True - assert dotted["snippet"] == "## v1.8 notes\nShip it." + by_id = {h["hit_id"]: h for h in r.structured_content["hits"] if h["file"] == str(notes)} + assert all(h["location_verified"] for h in by_id.values()) + assert by_id[f"{notes}:6-7"]["snippet"].startswith("Escalation continued") + assert by_id[f"{notes}:8-9"]["code"] == "## Title doc..Next steps.\nWave closed." + assert by_id[f"{notes}:1-2"]["snippet"] == "# Wave 05\n" + assert not any(n.startswith(f"[{Code.LOCATION_UNVERIFIED}]") for n in r.structured_content["notes"]) async def test_duplicate_ranges_in_matches_count_once(settings_env, monkeypatch, tmp_path): From e5b6fa22d66b6735dbab54da7313a1f5005723d2 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:20:54 +0900 Subject: [PATCH 12/17] fix(search): count lines on \n alone wherever ranges are derived colgrep numbers lines by \n, and expand reads a hit's range the same way, but locate_unit and search's code line counts still used str.splitlines, which also breaks on form feed, vertical tab, \x1c-\x1e, NEL and U+2028/9. A file holding one such character got a located range shifted against expand's, with location_verified=True: the verifier's form-feed case returned 6-8 for a unit expand reads at 5-7. The previous commit's claim that the exact-span path fixed this held only for that path. locate.split_lines breaks on \n and drops only the final empty element; locate_unit (file text and code) and the code line counts in tools_search use it, so every range agrees with expand. A \r stays on its line, since text mode already folds it out of file text and colgrep's code can carry it: locate_unit now compares the first line and the single-candidate block with trailing whitespace ignored, as span_holds and the multi-line disambiguation already did, and a hit's snippet drops a trailing \r from each code line as splitlines did. A CRLF unit still verifies. Contract tests through Client(build()): a stale file with a form feed above the unit, where the hit_id range must be the one expand returns as the unit (fails on 2bd2a8c), and a CRLF unit located after a shift (passes there and pins the \r handling). Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/locate.py | 30 +++++++++++++++++++------ server/colgrep_mcp/tools_search.py | 8 +++---- server/tests/test_tools_search.py | 36 ++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/server/colgrep_mcp/locate.py b/server/colgrep_mcp/locate.py index 429d19c..efaf8fc 100644 --- a/server/colgrep_mcp/locate.py +++ b/server/colgrep_mcp/locate.py @@ -11,28 +11,44 @@ from __future__ import annotations +def split_lines(text: str) -> list[str]: + """`text`'s lines, broken on `\\n` alone, with only the final empty element dropped. + + colgrep numbers lines by `\\n` (Rust `lines()`), and `expand` reads a hit's + range the same way, so every range derived here must too. `str.splitlines` + also breaks on `\\r`, form feed, vertical tab, `\\x1c`-`\\x1e`, NEL and U+2028/9, + which shifts every later line of a file holding one. A `\\r` stays on its + line; `rstripped` is how comparisons ignore it. + """ + lines = text.split("\n") + if lines[-1] == "": + lines.pop() + return lines + + def locate_unit(file_text: str, code: str, reported_line: int, reported_end: int) -> tuple[int, int, bool]: """Return `(line, end_line, verified)`, 1-indexed. - - Exact match of `code`'s first line against the file, disambiguating + - Match of `code`'s first line against the file (trailing whitespace + ignored, so a CRLF `code` still matches), disambiguating multiple candidates by matching the following lines (multi-line code, trailing whitespace ignored) or by nearest distance to `reported_line` (single-line code). - A single first-line candidate is still required to match the rest of - `code` before being trusted. + `code` before being trusted. Lines break on `\\n` alone (`split_lines`). - Falls back to `(reported_line, reported_end, False)` when no candidate first line is found, when the sole first-line candidate's block doesn't match the rest of `code`, or when a multi-line disambiguation finds no full match at all. """ - file_lines = file_text.splitlines() - code_lines = code.splitlines() + file_lines = split_lines(file_text) + code_lines = split_lines(code) if not code_lines: return (reported_line, reported_end, False) - first_line = code_lines[0] - candidates = [i + 1 for i, line in enumerate(file_lines) if line == first_line] + first_line = code_lines[0].rstrip() + candidates = [i + 1 for i, line in enumerate(file_lines) if line.rstrip() == first_line] if not candidates: return (reported_line, reported_end, False) @@ -44,7 +60,7 @@ def locate_unit(file_text: str, code: str, reported_line: int, reported_end: int # line 1) would come back confidently `verified=True` with a wrong # `end_line` (F3). line = candidates[0] - if file_lines[line - 1 : line - 1 + len(code_lines)] == code_lines: + if rstripped(file_lines[line - 1 : line - 1 + len(code_lines)]) == rstripped(code_lines): return (line, line + len(code_lines) - 1, True) return (reported_line, reported_end, False) diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index 81f6f60..e47b047 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -28,7 +28,7 @@ from .adapter import RawHit, SearchRequest, UnitMatch, unit_matches from .errors import Code, note, tool_error, translate_adapter_errors -from .locate import locate_unit, span_holds +from .locate import locate_unit, span_holds, split_lines from .locks import project_lock from .logging_utils import safe_log from .models import ExpandedUnit, ExpandResult, FileHit, FileResult, SearchHit, SearchResult @@ -113,7 +113,7 @@ def hit_from_raw( unit = raw.get("unit") or {} file = _resolve_hit_file(unit.get("file") or "", base_path) code = unit.get("code") or "" - code_lines = code.splitlines() + code_lines = split_lines(code) held = read.spans.get(leader.span) if held is not None and span_holds(leader.line, leader.end_line, held, code_lines): @@ -123,7 +123,7 @@ def hit_from_raw( else: line, end_line, verified = leader.line, leader.end_line, False - snippet = "\n".join(code_lines[:snippet_lines]) if code_lines else None + snippet = "\n".join(line.removesuffix("\r") for line in code_lines[:snippet_lines]) if code_lines else None return SearchHit( hit_id=f"{file}:{line}-{end_line}", @@ -327,7 +327,7 @@ def _plan_reads( plan: dict[str, _Wanted] = {} for ri in {ri for ri, _ in keep}: lead = matches[ri][0] - code_lines = ((raws[ri].get("unit") or {}).get("code") or "").splitlines() + code_lines = split_lines((raws[ri].get("unit") or {}).get("code") or "") wanted = plan.setdefault(files[ri], _Wanted()) if code_lines: wanted.leaders.append((lead.span, code_lines)) diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index ad8ff50..1f91cf8 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -662,6 +662,42 @@ async def test_search_location_verified_true_when_file_contains_the_code(setting assert hit["hit_id"] == f"{target}:4-5" +async def test_search_counts_lines_on_newline_alone_so_expand_agrees(settings_env, monkeypatch, tmp_path): + """A form feed breaks a line for `str.splitlines` but not for colgrep or `expand`: the file + changed since indexing, so `locate_unit` finds the unit, and the range it reports must be + the one `expand` reads back as the unit.""" + target = tmp_path / "ff.md" + target.write_text("intro\n\x0c\n\n\n## Alpha\na1\na2\n\n## Beta\nb1\n") + unit = _unit(target, "Alpha", 4, 6, "## Alpha\na1\na2\n") + fixture_path = tmp_path / "ff_hits.json" + fixture_path.write_text(json.dumps([{"unit": unit, "score": 1.0}])) + monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "alpha"}) + hit = r.structured_content["hits"][0] + e = await c.call_tool("expand", {"hit_ids": [hit["hit_id"]]}) + + assert (hit["line"], hit["end_line"], hit["location_verified"]) == (5, 7, True) + assert e.structured_content["units"][0]["code"] == "## Alpha\na1\na2" + + +async def test_search_still_verifies_a_crlf_unit(settings_env, monkeypatch, tmp_path): + target = tmp_path / "crlf.md" + target.write_bytes(b"x\r\n\r\n## Alpha\r\na1\r\n") + unit = _unit(target, "Alpha", 1, 2, "## Alpha\r\na1\r\n") + fixture_path = tmp_path / "crlf_hits.json" + fixture_path.write_text(json.dumps([{"unit": unit, "score": 1.0}])) + monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "alpha"}) + + hit = r.structured_content["hits"][0] + assert (hit["line"], hit["end_line"], hit["location_verified"]) == (3, 4, True) + assert hit["snippet"] == "## Alpha\na1" + + async def test_search_location_verified_false_when_file_lacks_the_code(settings_env, monkeypatch, tmp_path): target = tmp_path / "stale_source.py" target.write_text("# this file no longer contains the indexed unit\n") From 2d995219b0a6bca98e6e5686fa357d7f83a855f6 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:22:54 +0900 Subject: [PATCH 13/17] fix(search): drop a unit seen twice across overlapping search paths colgrep collapses its results to one per file inside one search path (collapse_by_file, called per path from search_hybrid_with_embedding); cmd_search then extends the per-path results, sorts and takes top_k with no further collapse (commands/search.rs). A search over nested paths therefore returns a file once per path, and every unit in both results came out twice: duplicate hit_ids from search, and duplicate counts in find_files. After the stable sort and before the cut, _do_search keeps the first, best-scored sighting of each (file, line, end_line, name) and stops scanning once limit is passed, so the cut stays proportional to limit. find_files' fold counts a unit once the same way, and keeps its limit*3 over-fetch, which the same overlap makes necessary. The name is in the key, beyond the ruled (file, line, end_line): before 1.8.4 colgrep's per-unit ranges were unreliable enough (R05 D1) for two units of a file to report one range, and test_errors' three hits of one file at 7-9 (vanished_0..2) is that shape; keyed on the range alone they collapsed into one hit. Benchmark unchanged (stub adapter, 450 units, median of 5): limit=15 2.17 ms, limit=None 4.53 ms, against 2.13 and 4.50 before. The contract test returns notes.md twice, the second time with another set of units sharing one, and expects each unit once from search and from find_files; it fails on 2bd2a8c. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/adapter.py | 5 +++++ server/colgrep_mcp/tools_search.py | 22 +++++++++++++++++++--- server/tests/test_tools_search.py | 23 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/server/colgrep_mcp/adapter.py b/server/colgrep_mcp/adapter.py index cb4e50c..0e42429 100644 --- a/server/colgrep_mcp/adapter.py +++ b/server/colgrep_mcp/adapter.py @@ -85,6 +85,11 @@ class UnitMatch(NamedTuple): def span(self) -> tuple[int, int]: return (self.line, self.end_line) + @property + def identity(self) -> tuple[int, int, str]: + """What names the same unit when a file is returned twice.""" + return (self.line, self.end_line, self.name) + def _wire_match(entry: Any) -> UnitMatch | None: """A `matches` entry as a `UnitMatch`, or `None` when it is malformed or diff --git a/server/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index e47b047..bae26b7 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -606,11 +606,14 @@ def _file_hits_from_raw(raw_hits: list[RawHit], base_path: Path | None) -> list[ raw code) count but are not listed in `top_units`. """ by_file: dict[str, FileHit] = {} + seen: set[tuple[str, int, int, str]] = set() for raw in raw_hits: unit = raw.get("unit") or {} file = _resolve_hit_file(unit.get("file") or "", base_path) score = float(raw.get("score") or 0.0) - matches = unit_matches(raw) + # Overlapping search paths return a file once per path (`_do_search`): count a unit once. + matches = [m for m in unit_matches(raw) if (file, *m.identity) not in seen] + seen.update((file, *m.identity) for m in matches) names = [m.name for m in matches if m.name] existing = by_file.get(file) if existing is None: @@ -687,8 +690,21 @@ async def _do_search( files = [_resolve_hit_file((raw.get("unit") or {}).get("file") or "", base_path) for raw in raws] ranked = [(ri, mi) for ri, units in enumerate(matches) for mi in range(len(units))] ranked.sort(key=lambda unit: -matches[unit[0]][unit[1]].score) # stable: colgrep's order breaks ties - limited = limit is not None and len(ranked) > limit - keep = ranked[:limit] if limited else ranked + # colgrep collapses to one result per file only within one search path, so overlapping paths + # return a file twice: keep each unit's best-scored sighting, stopping once `limit` is passed. + # The name is part of the key because before 1.8.4 colgrep's per-unit ranges were unreliable + # enough for two units of a file to report the same one (R05 D1). + seen: set[tuple[str, int, int, str]] = set() + unique: list[tuple[int, int]] = [] + for ri, mi in ranked: + unit = (files[ri], *matches[ri][mi].identity) + if unit not in seen: + seen.add(unit) + unique.append((ri, mi)) + if limit is not None and len(unique) > limit: + break + limited = limit is not None and len(unique) > limit + keep = unique[:limit] if limited else unique reads = await _read_units(_plan_reads(raws, files, matches, keep)) indexes: dict[int, list[int]] = {} diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index 1f91cf8..8245091 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -977,6 +977,29 @@ async def test_duplicate_ranges_in_matches_count_once(settings_env, monkeypatch, assert (first["hits"], first["top_units"]) == (2, ["Logistics", "Escalation"]) +async def test_a_file_returned_twice_by_overlapping_paths_counts_each_unit_once(settings_env, monkeypatch, tmp_path): + # colgrep collapses to one result per file within a search path only, so a search over nested + # paths returns the file once per path, the second time with a different set of units. + notes, _ = _matches_corpus(tmp_path, monkeypatch) + again = { + "unit": _unit(notes, "Escalation", 6, 10, "## Escalation\n- **PI decision:** switch to a subagent.\n"), + "score": 0.9, + "matches": [_match(6, 7, "Escalation", 0.9), _match(9, 10, "Closing", 0.1)], + } + fixture_path = tmp_path / "matches_hits.json" + fixture_path.write_text(json.dumps([*json.loads(fixture_path.read_text()), again])) + + async with Client(build(), raise_exceptions=True) as c: + searched = await c.call_tool("search", {"query": "escalation", "limit": 4}) + found = await c.call_tool("find_files", {"query": "escalation"}) + + ids = [h["hit_id"] for h in searched.structured_content["hits"]] + assert sorted(ids) == sorted(set(ids)) and len(ids) == 4 # 3 units of notes.md, 1 of tools.py + assert searched.structured_content["truncated"] is False + first = found.structured_content["files"][0] + assert (first["file"], first["hits"], first["top_units"]) == (str(notes), 3, ["Logistics", "Escalation", "Closing"]) + + # --- token budget (R01 §Token-budget invariant) ---------------------------- From bd10dbbc8bbabf01a2f29d16912d7954ee5b5c52 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:24:37 +0900 Subject: [PATCH 14/17] fix(adapter): pick the unit whose code colgrep sent as the leader on a score tie absorb_matches sorts a file's matched units by score and then by line ascending (index/mod.rs), so when the best two units tie, matches[0] is the lower-line one even if `unit`, whose `code` colgrep sends, is the other. unit_matches took matches[0] as the leader on faith: the leader hit then carried one unit's name over the other's text, repeated the other unit's range as its sibling's, and a cut to limit=1 kept the wrong unit (the verifier's tie case). Among the entries tied with the first, the leader is now the one whose line count equals the line count of unit.code (split on \n) and whose name equals the unit's display name, parent_class + "." + name when a parent class is set, empty for document and raw_code units, as UnitMatch::of builds it; with no such entry, or no code, it stays matches[0]. The rest keep their score order. The contract test gives a tie where matches[0] is Logistics and unit is Escalation, and expects Escalation as the verified leader with its qualified_name, every unit listed once and no duplicate hit_id; it fails on 2bd2a8c. Co-Authored-By: Claude Opus 5.5 --- server/colgrep_mcp/adapter.py | 22 ++++++++++++++++++- server/tests/test_tools_search.py | 35 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/server/colgrep_mcp/adapter.py b/server/colgrep_mcp/adapter.py index 0e42429..4024090 100644 --- a/server/colgrep_mcp/adapter.py +++ b/server/colgrep_mcp/adapter.py @@ -16,6 +16,7 @@ from pathlib import Path from typing import Any, NamedTuple +from .locate import split_lines from .models import IndexBuildResult, IndexInfo, IndexStatus from .textparse import parse_index_summary, parse_settings, parse_stats, parse_status @@ -121,6 +122,12 @@ def unit_matches(raw: RawHit) -> list[UnitMatch]: leader: the result then falls back to its own `unit`, like an absent or one-entry `matches` (colgrep 1.6, a lone matched unit). `name` is the display name (`Class.method`), empty for documents and raw code. + + `absorb_matches` breaks score ties by line, so `matches[0]` can be a unit + other than `unit`, the one whose `code` is sent. Among the entries tied + with the first, the leader is the one whose line count is `code`'s and + whose name is `unit`'s display name (`UnitMatch::of`); with none, or + without `code`, it is `matches[0]`. """ unit = raw.get("unit") or {} score = float(raw.get("score") or 0.0) @@ -144,11 +151,24 @@ def unit_matches(raw: RawHit) -> list[UnitMatch]: best[key] = match elif match.score > best[key].score: best[key] = best[key]._replace(score=match.score) - leader, *rest = best.values() + first = next(iter(best.values())) + leader = next((m for m in best.values() if m.score == first.score and _is_unit(m, unit)), first) + rest = [m for m in best.values() if m is not leader] rest.sort(key=lambda m: m.score, reverse=True) return [leader, *rest] +def _is_unit(match: UnitMatch, unit: dict[str, Any]) -> bool: + """Whether `match` is the unit `unit` describes: same line count as its + `code`, same display name (`UnitMatch::of` in colgrep's `index/mod.rs`).""" + name = unit.get("name") or "" + if unit.get("unit_type") in ("document", "raw_code"): + name = "" + elif unit.get("parent_class"): + name = f"{unit['parent_class']}.{name}" + return match.end_line - match.line + 1 == len(split_lines(unit.get("code") or "")) and match.name == name + + def _require_absolute(path: Path, what: str) -> None: """Raise `ColgrepError` unless `path` is absolute. diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index 8245091..098ae52 100644 --- a/server/tests/test_tools_search.py +++ b/server/tests/test_tools_search.py @@ -956,6 +956,41 @@ async def test_search_trusts_every_unit_of_an_unedited_file_whatever_its_first_l assert not any(n.startswith(f"[{Code.LOCATION_UNVERIFIED}]") for n in r.structured_content["notes"]) +async def test_search_leader_is_the_unit_whose_code_colgrep_sent_when_scores_tie(settings_env, monkeypatch, tmp_path): + # colgrep orders tied scores by line, so `matches[0]` is Logistics although `unit` (and its + # code) is Escalation: the leader must be Escalation, or it would carry Logistics' name over + # Escalation's text and the file's other unit would be listed twice. + notes, _ = _matches_corpus( + tmp_path, + monkeypatch, + notes_matches=[_match(3, 4, "Logistics", 0.9), _match(6, 7, "Escalation", 0.9), _match(9, 10, "Closing", 0.1)], + ) + raws = json.loads((tmp_path / "matches_hits.json").read_text()) + raws[0]["unit"] = _unit(notes, "Escalation", 3, 10, "## Escalation\n- **PI decision:** switch to a subagent.\n") + raws[0]["score"] = 0.9 + (tmp_path / "matches_hits.json").write_text(json.dumps(raws)) + + async with Client(build(), raise_exceptions=True) as c: + r = await c.call_tool("search", {"query": "escalation", "limit": 4, "include_code": True}) + + hits = [h for h in r.structured_content["hits"] if h["file"] == str(notes)] + ids = [h["hit_id"] for h in r.structured_content["hits"]] + assert len(ids) == len(set(ids)) + leader = hits[0] + assert (leader["hit_id"], leader["name"], leader["qualified_name"]) == ( + f"{notes}:6-7", + "Escalation", + f"{notes.name}::Escalation", + ) + assert leader["location_verified"] is True + assert {h["hit_id"]: h["name"] for h in hits} == { + f"{notes}:6-7": "Escalation", + f"{notes}:3-4": "Logistics", + f"{notes}:9-10": "Closing", + } + assert all(h["location_verified"] for h in hits) + + async def test_duplicate_ranges_in_matches_count_once(settings_env, monkeypatch, tmp_path): notes, _ = _matches_corpus( tmp_path, From 1f06fd53eb4f9bb78c5730c3fe6c9d30cc634ad8 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:32:33 +0900 Subject: [PATCH 15/17] test(tests): say the fake emulates colgrep 1.8, the version it reports Its --version, fixtures and matches shape are 1.8 since the match_units rework; the header still said 1.6. Co-Authored-By: Claude Opus 5.5 --- server/tests/fake_colgrep.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/tests/fake_colgrep.py b/server/tests/fake_colgrep.py index a563f0d..025bf11 100755 --- a/server/tests/fake_colgrep.py +++ b/server/tests/fake_colgrep.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 """A stand-in for the colgrep CLI used by the test-suite. -Emulates the subset of colgrep 1.6 surface the adapter drives: +Emulates the subset of colgrep 1.8 surface the adapter drives: --version | --stats | settings | status PATH | init [-y] PATH | clear PATH [search] [--json] [-k N] [-e PAT] [flags...] QUERY [PATH...] Environment knobs: From 15a610c8ddf4faf26e2fae859cdebbf5516f1ae8 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 01:32:33 +0900 Subject: [PATCH 16/17] chore(roadmap): close match_units with its gates and the step 5 skip All three nodes are done. unit_pipeline step 5 (find_files -k limit) was skipped on evidence: colgrep collapses per search path, then concatenates and takes top-k (commands/search.rs:543-613), so -k limit could return fewer than limit files. Open gates: PR #18 CI, and the literal 'every new test fails on c4c291a' gate, which holds for the behaviour tests while the regression pins pass there by design. Co-Authored-By: Claude Opus 5.5 --- __roadmap__/match_units/README.md | 24 +++++++++---------- __roadmap__/match_units/doctor_and_docs.md | 10 ++++---- __roadmap__/match_units/unit_pipeline.md | 12 +++++----- __roadmap__/match_units/verify/README.md | 8 +++---- .../match_units/verify/adversarial_review.md | 10 ++++---- 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/__roadmap__/match_units/README.md b/__roadmap__/match_units/README.md index bd69d79..f1e0ed1 100644 --- a/__roadmap__/match_units/README.md +++ b/__roadmap__/match_units/README.md @@ -11,22 +11,22 @@ PR #18 (commit c4c291a on `claude/happy-hamilton-e15dcd`) made `search` yield on `search` builds only the units it returns, from one normaliser of colgrep's results. Every unit keeps its own verified range and name, and the docs state that rule without accretion. ## Pre-conditions -- [ ] `claude/happy-hamilton-e15dcd` is at c4c291a or later, with the gates green -- [ ] `colgrep --version` reports 1.8.4 or newer on this machine (for the verifier's real-binary probe) +- [x] `claude/happy-hamilton-e15dcd` is at c4c291a or later, with the gates green +- [x] `colgrep --version` reports 1.8.4 or newer on this machine (for the verifier's real-binary probe) ## Success Gates -- ⬜ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range main..HEAD` passes on the integration branch [run] -- ⬜ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] -- ⬜ `git diff main..HEAD -- skills/` is empty [run] -- ⬜ The verifier's real-binary probe passes: the wave-05 105-156 hit is verified and expands to line 138 [behavioral] +- ✅ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range main..HEAD` passes on the integration branch [run] +- ✅ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] +- ✅ `git diff main..HEAD -- skills/` is empty [run] +- ✅ The verifier's real-binary probe passes: the wave-05 105-156 hit is verified and expands to line 138 [behavioral] - ⬜ PR #18 CI is green [run] ## Status ```mermaid graph TD - unit_pipeline[Unit Pipeline]:::planned - doctor_and_docs[Doctor and Docs]:::planned - verify[Verification]:::planned + unit_pipeline[Unit Pipeline]:::done + doctor_and_docs[Doctor and Docs]:::done + verify[Verification]:::done classDef done fill:#166534,color:#bbf7d0 classDef inprogress fill:#854d0e,color:#fef08a classDef planned fill:#374151,color:#e5e7eb @@ -37,9 +37,9 @@ graph TD ## Nodes | Node | Type | Status | |:-----|:-----|:-------| -| `unit_pipeline.md` | 📄 Leaf Task | ⬜ Planned | -| `doctor_and_docs.md` | 📄 Leaf Task | ⬜ Planned | -| `verify/` | 📁 Directory | ⬜ Planned | +| `unit_pipeline.md` | 📄 Leaf Task | ✅ Done | +| `doctor_and_docs.md` | 📄 Leaf Task | ✅ Done | +| `verify/` | 📁 Directory | ✅ Done | ## Amendment Log | ID | Date | Source | Nodes Added | Rationale | diff --git a/__roadmap__/match_units/doctor_and_docs.md b/__roadmap__/match_units/doctor_and_docs.md index e3b8024..4837360 100644 --- a/__roadmap__/match_units/doctor_and_docs.md +++ b/__roadmap__/match_units/doctor_and_docs.md @@ -2,12 +2,12 @@ **Goal**: Declare colgrep 1.8.4 the soft minimum through `doctor`, and state the one-hit-per-unit rule by editing the guide in place rather than appending to it. **Pre-conditions**: -- [ ] The roadmap commit is on `claude/happy-hamilton-e15dcd` +- [x] The roadmap commit is on `claude/happy-hamilton-e15dcd` **Success Gates**: -- ⬜ `cd server && uv run pytest -q tests/test_tools_index.py tests/test_adapter.py` passes [run] -- ⬜ `git diff main..HEAD -- skills/` is empty [run] -- ⬜ `git diff --numstat main..HEAD -- server/colgrep_mcp/guide.md` adds no more than 4 lines net [run] -- ⬜ `AGENTS.md` names `OBS-M` and points it at `__reports__/match_lines/00-observation_markdown_match_lines_v0.md` [run] +- ✅ `cd server && uv run pytest -q tests/test_tools_index.py tests/test_adapter.py` passes [run] +- ✅ `git diff main..HEAD -- skills/` is empty [run] +- ✅ `git diff --numstat main..HEAD -- server/colgrep_mcp/guide.md` adds no more than 4 lines net [run] +- ✅ `AGENTS.md` names `OBS-M` and points it at `__reports__/match_lines/00-observation_markdown_match_lines_v0.md` [run] **References**: [R01 §Documentation, §Minimum colgrep 1.8.4](~/.claude/plans/now-switching-you-to-tidy-glade.md) — what to revert, what to rewrite, how strict the minimum is ## Step 1: Report a colgrep older than 1.8.4 in doctor diff --git a/__roadmap__/match_units/unit_pipeline.md b/__roadmap__/match_units/unit_pipeline.md index 38c02b8..071fc38 100644 --- a/__roadmap__/match_units/unit_pipeline.md +++ b/__roadmap__/match_units/unit_pipeline.md @@ -2,14 +2,14 @@ **Goal**: Rebuild `search`'s hit construction as one output-proportional pipeline: adapter JSON → `unit_matches` → rank and cut to `limit` → read only the surviving spans → build hits. Every unit keeps its own verified range and name. **Pre-conditions**: -- [ ] Task branch `task/match-units-pipeline` is cut from `claude/happy-hamilton-e15dcd` at the commit that adds this roadmap -- [ ] `cd server && uv run pytest -q` is green in the task worktree before any edit +- [x] Task branch `task/match-units-pipeline` is cut from `claude/happy-hamilton-e15dcd` at the commit that adds this roadmap +- [x] `cd server && uv run pytest -q` is green in the task worktree before any edit **Success Gates**: -- ⬜ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range claude/happy-hamilton-e15dcd..HEAD` passes [run] -- ⬜ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] -- ⬜ `FAKE_COLGREP_MATCHES` no longer appears under `server/` [run] +- ✅ `cd server && uv run pytest -q && uv run ruff check && uv run ruff format --check && uv run cz check --rev-range claude/happy-hamilton-e15dcd..HEAD` passes [run] +- ✅ Only `adapter.py` reads `matches`: within `server/colgrep_mcp/`, only `adapter.py` contains the string `"matches"` [run] +- ✅ `FAKE_COLGREP_MATCHES` no longer appears under `server/` [run] - ⬜ Every new contract test named in steps 1–4 fails when `server/colgrep_mcp/` is checked out at c4c291a, and the report lists them [run] -- ⬜ The bodies of commits 3 and 4 carry median-of-5 before/after tables, or the commit is typed `refactor` and the report says why [run] +- ✅ The bodies of commits 3 and 4 carry median-of-5 before/after tables, or the commit is typed `refactor` and the report says why [run] **References**: [R01 §Verdict on c4c291a, §Target design](~/.claude/plans/now-switching-you-to-tidy-glade.md) — the nine findings this leaf answers (rows 1–7, 9) and the design it implements ## Step 1: Normalise colgrep results to leader-first unit matches diff --git a/__roadmap__/match_units/verify/README.md b/__roadmap__/match_units/verify/README.md index f4c4527..389bd3f 100644 --- a/__roadmap__/match_units/verify/README.md +++ b/__roadmap__/match_units/verify/README.md @@ -7,15 +7,15 @@ Runs after `unit_pipeline`. It is the named adversarial review for this campaign Either break the pipeline's location and trust rules or show, probe by probe, that they hold. ## Pre-conditions -- [ ] `unit_pipeline` has reported, with its gates green +- [x] `unit_pipeline` has reported, with its gates green ## Success Gates -- ⬜ Every attack in `adversarial_review` is marked broken or held, and each broken one is fixed or explicitly accepted by the coordinator [behavioral] +- ✅ Every attack in `adversarial_review` is marked broken or held, and each broken one is fixed or explicitly accepted by the coordinator [behavioral] ## Status ```mermaid graph TD - adversarial_review[Adversarial Review]:::planned + adversarial_review[Adversarial Review]:::done classDef done fill:#166534,color:#bbf7d0 classDef inprogress fill:#854d0e,color:#fef08a classDef planned fill:#374151,color:#e5e7eb @@ -26,7 +26,7 @@ graph TD ## Nodes | Node | Type | Status | |:-----|:-----|:-------| -| `adversarial_review.md` | 📄 Leaf Task | ⬜ Planned | +| `adversarial_review.md` | 📄 Leaf Task | ✅ Done | ## Amendment Log | ID | Date | Source | Nodes Added | Rationale | diff --git a/__roadmap__/match_units/verify/adversarial_review.md b/__roadmap__/match_units/verify/adversarial_review.md index b1953b2..f4291d7 100644 --- a/__roadmap__/match_units/verify/adversarial_review.md +++ b/__roadmap__/match_units/verify/adversarial_review.md @@ -2,12 +2,12 @@ **Goal**: Find what is wrong in `unit_pipeline` before it merges: silent location and trust failures, the class of bug a green suite does not show. **Pre-conditions**: -- [ ] `unit_pipeline` has reported, with its gates green on `task/match-units-pipeline` -- [ ] `colgrep --version` reports 1.8.4 or newer, and the scratch copy of a notes corpus named in the verifier's brief exists outside this repository +- [x] `unit_pipeline` has reported, with its gates green on `task/match-units-pipeline` +- [x] `colgrep --version` reports 1.8.4 or newer, and the scratch copy of a notes corpus named in the verifier's brief exists outside this repository **Success Gates**: -- ⬜ A findings list in which each attack below is marked broken (with a reproducer) or held (with the probe that tried to break it) [behavioral] -- ⬜ The real-binary probe: the wave-05 105-156 hit is `location_verified` and `expand` returns line 138 [run] -- ⬜ The share of leaders verified by exact span versus whole-file fallback, over 3–5 statera queries, is reported [run] +- ✅ A findings list in which each attack below is marked broken (with a reproducer) or held (with the probe that tried to break it) [behavioral] +- ✅ The real-binary probe: the wave-05 105-156 hit is `location_verified` and `expand` returns line 138 [run] +- ✅ The share of leaders verified by exact span versus whole-file fallback, over 3–5 statera queries, is reported [run] **References**: [R01 §Verdict on c4c291a, §Target design](~/.claude/plans/now-switching-you-to-tidy-glade.md) — the rules under attack ## Step 1: Attack the pipeline and probe the real binary From 188905ff552843ad685c70322b76ddbfec2876f2 Mon Sep 17 00:00:00 2001 From: Eliott Jacopin Date: Sun, 1 Nov 2026 11:15:50 +0900 Subject: [PATCH 17/17] docs(reports): open match_units with the upstream colgrep asks and the cycle's retrospective The rework left the server inferring 35 of 110 hits' locations, because colgrep sends no content for a file's non-best units. The findings report ranks four upstream asks (a per-unit hash in matches, the best unit kept first on ties, the index-update outcome under --json, a merge across search paths), the gaps each closes and what not to ask, so the work can be picked up through the CrackingShells/next-plaid fork. The knowledge-transfer report records why both trust-rule designs were wrong and what changes next cycle; the observation hands the dirtree-rdm add-suffix defect to its own repository. Co-Authored-By: Claude Opus 5.5 --- ...0-findings_upstream_colgrep_contract_v0.md | 108 ++++++++++++++++++ .../match_units/00-knowledge_transfer_v0.md | 61 ++++++++++ ...0-observation_dirtree_rdm_add_suffix_v0.md | 54 +++++++++ __reports__/match_units/README.md | 11 ++ 4 files changed, 234 insertions(+) create mode 100644 __reports__/match_units/00-findings_upstream_colgrep_contract_v0.md create mode 100644 __reports__/match_units/00-knowledge_transfer_v0.md create mode 100644 __reports__/match_units/00-observation_dirtree_rdm_add_suffix_v0.md create mode 100644 __reports__/match_units/README.md diff --git a/__reports__/match_units/00-findings_upstream_colgrep_contract_v0.md b/__reports__/match_units/00-findings_upstream_colgrep_contract_v0.md new file mode 100644 index 0000000..73aa113 --- /dev/null +++ b/__reports__/match_units/00-findings_upstream_colgrep_contract_v0.md @@ -0,0 +1,108 @@ +# Match Units — Findings: what colgrep should report so the server can check, not infer (v0) + +Date: 2026-11-01 + +--- +type: findings +topic: match_units +date: 2026-11-01 +version: v0 +prior-version: none +key-metric: hits whose location is inferred rather than checked: 35 of 110 (32 %) (prior: N/A, delta: N/A) +decision-required: confirm +--- + +## Headline Result + +metric: share of `search` hits whose `location_verified` rests on an inference instead of a check against the unit's own content +value: 35 / 110 (32 %) +unit: hits, colgrep 1.8.5+cs.1, 5 queries on a scratch copy of Murmuration's `__statera__` +prior: N/A (first run) +direction: new + +Every non-best unit of a file is in this class. The server has no content to check it against, because colgrep sends `code` only for the best unit. Ask #1 below would bring the number to 0. + +## Results Tables + +### The asks, most valuable first + +Source lines are at fork commit `7f1e855` (`install/markdown-sections`, upstream v1.8.5 plus one fork commit). + +| # | Ask | colgrep today | Server workaround today | Removes | +|:--|:--|:--|:--|:--| +| 1 | Add a content hash of each unit to every `matches` entry | `UnitMatch` carries `line`, `end_line`, `name` and `score` only (`colgrep/src/index/mod.rs` `UnitMatch`). `absorb_matches` already holds each unit's `code` when it merges (`index/mod.rs:3497-3510`) | Non-best units are trusted when the best unit is span-verified, the index was refreshed, and the file's mtime is no later than the spawn time | The whole mtime inference, plus all three accepted gaps below | +| 2 | Keep the best unit at `matches[0]` | `absorb_matches` sorts every entry by score, then by line ascending. On a tie, `matches[0]` may not be the unit whose `code` is in `unit` | `adapter.unit_matches` re-picks the leader by line count and display name | The tie heuristic, and the unnamed-tie gap | +| 3 | Report the pre-search update outcome under `--json` (refreshed, skipped because the index is locked, or rebuilt) | `quiet = json \|\| files_only` (`commands/search.rs:1127`) also hides "Index is being updated by another process, searching existing index..." (`search.rs:1273-1281`) | None possible: the lock-skip is silent. `index_updated` keys on "Building index", so it misses incremental re-indexing | The lock gap, and a wrong `index_updated` | +| 4 | Collapse results per file across all search paths before taking the top `-k` | `cmd_search` collapses per path, then concatenates and takes the top k (`search.rs:543-613`) | `search` dedupes on (file, line, end_line, name). `find_files` over-fetches with `limit*3` | The dedupe, and `find_files`' threefold over-fetch | + +### What not to ask + +| Candidate | Why not | +|:--|:--| +| Make `unit.line..end_line` the best unit's own range | It is widened over the file's matches on purpose (`collapse_by_file`, `index/mod.rs:4807`), and other consumers may read it as the file's hit span. `matches[0]` already gives the true range | +| Include full `code` for every `matches` entry | It multiplies the JSON payload by the number of matched units, which can be 30 per markdown file, and the server keeps only `limit` units. A hash is a few bytes | +| Move verification against disk into colgrep | colgrep cannot know what the server reads after it exits. The check belongs where the read happens; colgrep only has to state what it indexed | + +### Gaps the server accepts today + +These are measured by the adversarial review of `match_units`. Probes were in that session's scratchpad and are not committed. + +| Gap | How it shows | Window | Closed by | +|:--|:--|:--|:--| +| Another process holds the index lock | colgrep searches the old index silently. A file edited before the call keeps its later units trusted | Concurrent indexing of the same project | #1, or #3 | +| A file replaced with its mtime and size preserved (`cp -p`, `rsync -t`, or `os.utime` after a same-size edit) | colgrep's change scan skips it (ns-mtime and size equal), and so does the server's mtime rule. Later units come back off by one under the right name | Tools that preserve metadata | #1 | +| An exact score tie between two unnamed units of equal length | The wrong unit leads, giving a duplicate `hit_id` and a missing unit | Exact float ties only | #2 | + +## Observations + +| Signal | Baseline / Expected | Observed [source] | Interpretation | +|:--|:--|:--|:--| +| Best units verified by exact span | Most, if colgrep 1.8 ranges are exact | 75 / 75, with 0 whole-file reads [verifier's real-binary probe] | colgrep's own unit ranges are exact. Only the widened `unit` range was ever wrong (R05 D1) | +| Non-best units trusted by a name-on-first-line rule | Close to 100 % on unedited notes | 22 / 35. The 13 failures were all continuation chunks of long sections [first verifier pass] | Names don't identify ranges. A heuristic was the wrong tool, which is why the rule moved to index freshness | +| Non-best units trusted by the index-freshness rule | Close to 100 % on unedited notes | 35 / 35 [recheck] | Sound wherever colgrep's change detection is sound, and no sounder | +| Lock-skip signal under `--json` | Present on stderr | Absent: stderr `''`, exit 0, results returned [implementer probe with `.lock` held] | A correctness signal is being silenced together with progress chatter | +| Multi-path collapse | Global | Per path [`search.rs:543-613`] | `-k` counts results, not distinct files, so `-k limit` cannot replace the over-fetch | + +## Charts & Visualizations + +```mermaid +flowchart LR + subgraph today["colgrep 1.8.5 today"] + U["unit: best unit's code,
range widened over the file"] --> S1["server: span-check the best unit"] + M["matches[]: line, end_line, name, score"] --> S2["server: infer the others
(index refreshed + mtime ≤ spawn)"] + end + subgraph asked["with asks 1-3"] + U2["unit: unchanged"] --> T1["server: span-check the best unit"] + M2["matches[]: + hash, best first"] --> T2["server: hash-check each kept unit's span"] + E["stderr: update outcome,
even when quiet"] --> T3["server: index_updated, stale-index note"] + end +``` +*Caption: where `location_verified` comes from today, and with asks #1–#3. Ask #4 changes only how many results `-k` admits.* + +``` +hits on statera (110) checked ███████████████████████████████████████ 75 + inferred ██████████████████ 35 ← 0 with ask #1 +``` +*Caption: 110 hits over 5 queries, split by how the location was established.* + +## Contradictions & Surprises + +- colgrep's `--json` mode is the one every agent integration uses, and it is also the one mode that hides the lock-skip warning. +- The original "colgrep reports wrong line numbers" finding (R05 D1, 1.6.2) is explained by the widening in `collapse_by_file`, not by bad unit ranges. Every unit range checked here was exact. + +## Steering Questions + +- [now] Open #2 and #3 upstream first, as two separate single-concern PRs from the CrackingShells fork? Both are a few lines, and outside PRs of that shape get merged. +- [now] For #1, which hash? colgrep already hashes file contents for change detection (`compute_update_plan`, `index/mod.rs:3296+`). Reusing that algorithm avoids a new dependency. The server must hash spans the same way, including the `\n`-only line rule. +- [next run] When a colgrep release with #1 exists, should the server prefer the hash and keep the mtime rule only for older binaries? Or should the soft minimum move to that release and the mtime rule go? +- [next run] If #4 lands, switch `find_files` to `-k limit` and drop the dedupe. The step 5 test in `__roadmap__/match_units/unit_pipeline.md` already describes the argv check. +- [later] Does #3's update outcome belong on stderr, or in a JSON envelope behind a flag? The bare-array output is what existing consumers parse. + +## Pointers + +- `__roadmap__/match_units/` — the campaign, its leaves and gates +- `__reports__/match_units/00-knowledge_transfer_v0.md` — how these gaps were found +- `__reports__/match_lines/00-observation_markdown_match_lines_v0.md` (OBS-M) — why the per-unit hits exist +- `server/colgrep_mcp/adapter.py` `unit_matches`; `server/colgrep_mcp/tools_search.py` `hits_from_raw`, `_do_search` +- [CrackingShells/colgrep-mcp#18](https://github.com/CrackingShells/colgrep-mcp/pull/18) +- Upstream: [lightonai/next-plaid](https://github.com/lightonai/next-plaid); fork: CrackingShells/next-plaid, branches `feat/markdown-sections` and `install/markdown-sections` diff --git a/__reports__/match_units/00-knowledge_transfer_v0.md b/__reports__/match_units/00-knowledge_transfer_v0.md new file mode 100644 index 0000000..81630ce --- /dev/null +++ b/__reports__/match_units/00-knowledge_transfer_v0.md @@ -0,0 +1,61 @@ +# Match Units — Knowledge Transfer (v0) + +Date: 2026-11-01 + +## Executive Summary +- **What shipped:** on PR #18 (`claude/happy-hamilton-e15dcd`, head `15a610c`), `search` returns one hit per matched unit, and the construction behind it was rebuilt. + - One normaliser, `adapter.unit_matches`, is the only reader of colgrep's `matches`. + - Units are ranked and cut before any hit is built, and only the surviving spans are read. + - A file's best unit is span-verified. Its other units are trusted through this call's index refresh. + - `expand` reads each file once. + - colgrep 1.8.4 is a soft minimum, enforced in `doctor`. Docs were edited in place: the guide is +3 lines net and `skills/` is unchanged. +- **Primary outcomes:** + - `search` (limit=15) 5.20 → 2.13 ms and `expand` (5 ids) 0.76 → 0.42 ms. Stub adapter, median of 5. + - On the real binary, 0 of 110 hits are unverified. + - Five bugs in the first per-unit version (c4c291a) are fixed, and three gaps are accepted and written down. + - Four upstream asks came out of it (`00-findings_upstream_colgrep_contract_v0.md`). +- **Cost:** 6 subagent dispatches, all on Sonnet: reviewer, advisor, implementer ×2, verifier ×2. About 1.3M subagent tokens in total. + +## Wins +- **An independent review before rework.** A Sonnet reviewer given the code, not the author's conclusions, found the duplication and two of the location bugs the author had missed in c4c291a. +- **Shrinking the roadmap.** An advisor pass cut it from 5 leaves, 11 commits and 4–5 dispatches to 3 leaves, 7 planned commits and 2 dispatches. Execution confirmed the sizing: one implementer did the whole pipeline, and the coordinator wrote doctor and docs in parallel. +- **A verifier with a named attack list and a required real-binary probe.** Its first pass broke the trust rule on real data: 13 of 110 hits unverified, plus wrong text under trailing-dot and duplicate headings. That bug class does not show in a green suite. The recheck confirmed the fixes. +- **Implementers reporting spec defects instead of building on them.** Two premises in the coordinator's briefs were false, and both came back as findings with source lines: + - colgrep collapses results per path, not globally, so step 5 was skipped; + - colgrep under `--json` never prints the lock-skip warning. +- **Measuring the environment before dispatch.** A fresh worktree run of the suite caught the coordinator's own machine paths in a roadmap leaf (`test_packaging` local-path guard) before any implementer inherited them. + +## Pain Points +- **The coordinator designed the trust rule twice, and both versions were wrong.** The name-on-first-line rule failed on continuation chunks. Its replacement leaned partly on a stderr line that `--json` suppresses. +- **`dirtree-rdm add ` without the `.md` suffix** writes a Nodes row the validator rejects. It also leaves an orphan leaf file behind. It took two failed retries to find this, because the error points at the wrong table (`00-observation_dirtree_rdm_add_suffix_v0.md`). +- **An `--amend` combined with a `reset --hard` in one command was refused by the auto-mode classifier.** A new commit plus `merge --ff-only` worked. A lone `--amend` later passed. +- **`gh pr edit` fails on a token without `read:org`.** The REST route (`gh api -X PATCH …/pulls/18 -F body=@file`) worked. +- **The "every new test fails on c4c291a" gate could not hold literally.** Regression pins for behaviour that must not change pass on the old code by design. The implementer split its list into "fails there" and "passes there". + +## Root Causes +- **The trust rules were designed from the plan's model of colgrep, not from colgrep's output.** Neither the continuation-chunk shape (60-line section caps) nor `quiet = json` was looked up before the briefs pinned them. +- **The success gate conflated two test classes:** regression tests, which must fail on the old code, and contract pins, which must pass on both. The plan asked one gate to cover both. +- **`dirtree-rdm`'s `add` accepts a bare node path but writes the row in a form its own grammar rejects.** The validator reports the failure two tables later than the cause. + +## Next-cycle Changes +- **Instruction changes** + - Before pinning any rule that depends on colgrep's output, cite the colgrep source line or a captured `--json` sample in the brief. For stderr, check the `quiet`/`json` gating. + - Split such gates in two: "regression tests fail on the old code" and "contract pins hold on both". +- **Workflow changes** + - Always pass `.md` to `dirtree-rdm add` for a leaf. + - Run the full suite (`--junitxml`, parsed) in a fresh task worktree before dispatch, not a selected subset. + - Use `gh api` REST for PR edits. +- **Review process changes** + - Keep the verifier's required real-binary probe for anything touching `location_verified` or `hit_id`. It was the only check that exercised continuation chunks. + - Re-verify a fix round narrowly by `SendMessage` to the same verifier. That took 4 minutes, against 10 for its first pass. + +## Artifacts to Preserve +- `__roadmap__/match_units/`: the leaves, with their success gates and the step 5 skip +- `__reports__/match_units/00-findings_upstream_colgrep_contract_v0.md`: the upstream asks and the accepted gaps +- PR #18 commit bodies: the median-of-5 tables (`1d896f8`, `cc9345a`) and the trust-rule rationale (`97bfd3d`) +- The probe scripts stay in the session scratchpad. Reproduce from the commit bodies instead. + +## Open Questions +- Should `index_updated` (R05 D7) be kept? Under `--json` it can only detect a cold build ("Building index"). The verifier saw it miss incremental re-indexing, and ask #3 upstream would fix it. +- Would a multi-path `search` with overlapping paths ever come from an agent? If not, the (file, span, name) dedupe guards a case nobody hits. +- How does the per-unit hit list read to agents on long notes? Many hits from one file can fill the text budget sooner. The Murmuration lab's question set (OBS-M) could measure it. diff --git a/__reports__/match_units/00-observation_dirtree_rdm_add_suffix_v0.md b/__reports__/match_units/00-observation_dirtree_rdm_add_suffix_v0.md new file mode 100644 index 0000000..a75fe83 --- /dev/null +++ b/__reports__/match_units/00-observation_dirtree_rdm_add_suffix_v0.md @@ -0,0 +1,54 @@ +# Match Units — Observation: `dirtree-rdm add` without `.md` writes a row its validator rejects (v0) + +Date: 2026-11-01 + +--- +type: observation +topic: match_units +spotted-during: authoring the match_units roadmap (Phase 0) with dirtree-rdm 1.2.0 +date: 2026-11-01 +domain: code +confidence: confirmed +urgency: low +deferred-because: the tool lives in the managing-roadmaps plugin, not in this repository; passing `.md` works around it +--- + +## What Was Noticed +- `dirtree-rdm add __roadmap__// --title "…"` (no `.md`) creates `.md`. Post-write validation of the parent README then fails, so the README is not updated. +- The result is an orphan leaf file that no Nodes row lists. +- The error points at the next table, not at the row that was written: + + ``` + line 29: got: "| `unit_pipeline` | 📄 Leaf Task | ⬜ Planned |" + expected form: | ID | Date | Source | Nodes Added | Rationale | + ``` + +- The same command with `.md` succeeds, and the row reads `` `unit_pipeline.md` ``. + +## Context +- The aim was to bootstrap `__roadmap__/match_units/` with `init`, then `add` for three nodes. +- The first two attempts failed this way. Filling the README prose in between made no difference. +- Passing the suffix fixed it. + +## Location Map +- managing-roadmaps plugin 1.2.0: `skills/managing-roadmaps/scripts/dirtree-rdm.sh` (wrapper) and the compiled binary it dispatches to. +- The CLI doc's `add` examples pass `.md` (`references/dirtree-cli.md`, "add — Create a new node"). Its synopsis is `add [--type leaf|dir]`. +- Grammar productions: ``, ``, and the Nodes row production (`dirtree-rdm grammar`). + +## Evidence +- Failing row: `` | `unit_pipeline` | 📄 Leaf Task | ⬜ Planned | ``. Accepted rows in other campaigns: `` | `regenerate_manifests.md` | 📄 Leaf Task | ✅ Done | ``. +- After each failure the leaf file existed on disk and the README was unchanged (atomic commit skipped). Deleting the orphan and retrying with `.md` worked. + +## Re-observation Steps +1. `dirtree-rdm init __roadmap__/demo` in a scratch repo with an `__roadmap__/` directory. +2. `dirtree-rdm add __roadmap__/demo/task_a --title "Task A"`: validation fails, and `task_a.md` is left behind. +3. `dirtree-rdm add __roadmap__/demo/task_b.md --title "Task B"`: succeeds. + +## Hand-off Questions +Working theory: `add` derives the file name with `.md` appended for leaves, but writes the Nodes cell from the raw argument. The Nodes row grammar requires a `.md` (leaf) or `/` (directory) suffix, so a bare name breaks out of the table, and the parser reports it at the next header. +- Should `add` normalise a bare leaf path to `.md` everywhere, or refuse it before creating the file? +- Should a failed post-write validation remove the file it just created, so nothing is orphaned? +- Can the validator report "unrecognised Nodes row" at the row, instead of a header mismatch at the next table? + +## Scope Boundary +This report does not authorise changes to this repository's roadmaps or to the managing-roadmaps plugin's other commands. A fix belongs in the plugin's own repository, with its own tests. diff --git a/__reports__/match_units/README.md b/__reports__/match_units/README.md new file mode 100644 index 0000000..3c31147 --- /dev/null +++ b/__reports__/match_units/README.md @@ -0,0 +1,11 @@ +# Reports — match_units + +## Round 00 +- `00-findings_upstream_colgrep_contract_v0.md` — four upstream colgrep asks, most valuable first: a per-unit content hash in `matches`, the best unit kept first on ties, the index-update outcome reported under `--json`, and a merge across search paths. Each comes with colgrep source lines, the server workaround it would remove, the gaps accepted today, and what not to ask (latest) +- `00-knowledge_transfer_v0.md` — the rework cycle on PR #18: what shipped, what the reviewer, advisor and verifier caught, why both trust-rule designs were wrong, and the next-cycle changes (latest) +- `00-observation_dirtree_rdm_add_suffix_v0.md` — `dirtree-rdm add` without `.md` orphans a leaf file and reports the error at the wrong table; the fix belongs in the managing-roadmaps plugin (latest) + +## Status +- **Campaign:** `__roadmap__/match_units/` is closed. All three nodes are done; step 5 (`find_files -k limit`) was skipped on evidence. +- **PR:** [CrackingShells/colgrep-mcp#18](https://github.com/CrackingShells/colgrep-mcp/pull/18) is open, with nothing merged yet. +- **Next:** the upstream asks in the findings report, through the CrackingShells/next-plaid fork. The fork's markdown-sections work is tracked in `__reports__/match_lines/`.