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/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/__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. 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/`. diff --git a/__roadmap__/match_units/README.md b/__roadmap__/match_units/README.md new file mode 100644 index 0000000..f1e0ed1 --- /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 +- [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] +- ⬜ PR #18 CI is green [run] + +## Status +```mermaid +graph TD + 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 + classDef amendment fill:#1e3a5f,color:#bfdbfe + classDef blocked fill:#7f1d1d,color:#fecaca +``` + +## Nodes +| Node | Type | Status | +|:-----|:-----|:-------| +| `unit_pipeline.md` | 📄 Leaf Task | ✅ Done | +| `doctor_and_docs.md` | 📄 Leaf Task | ✅ Done | +| `verify/` | 📁 Directory | ✅ Done | + +## 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..4837360 --- /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**: +- [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] +**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..071fc38 --- /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**: +- [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] +- ⬜ 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..389bd3f --- /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 +- [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] + +## Status +```mermaid +graph TD + adversarial_review[Adversarial Review]:::done + 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 | ✅ Done | + +## 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..f4291d7 --- /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**: +- [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] +**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` 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` diff --git a/server/colgrep_mcp/adapter.py b/server/colgrep_mcp/adapter.py index f74ec44..4024090 100644 --- a/server/colgrep_mcp/adapter.py +++ b/server/colgrep_mcp/adapter.py @@ -14,8 +14,9 @@ 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 .locate import split_lines from .models import IndexBuildResult, IndexInfo, IndexStatus from .textparse import parse_index_summary, parse_settings, parse_stats, parse_status @@ -73,6 +74,101 @@ 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 + + @property + 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 + 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. + + `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) + 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) + 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/colgrep_mcp/guide.md b/server/colgrep_mcp/guide.md index b2ba9d6..c767a58 100644 --- a/server/colgrep_mcp/guide.md +++ b/server/colgrep_mcp/guide.md @@ -7,7 +7,8 @@ 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. @@ -110,20 +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. +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 either colgrep or 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/server/colgrep_mcp/locate.py b/server/colgrep_mcp/locate.py index bbce5a4..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) @@ -82,13 +98,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/models.py b/server/colgrep_mcp/models.py index b6ba83f..ff95a69 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 @@ -20,9 +23,12 @@ 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 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_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/colgrep_mcp/tools_search.py b/server/colgrep_mcp/tools_search.py index a959902..bae26b7 100644 --- a/server/colgrep_mcp/tools_search.py +++ b/server/colgrep_mcp/tools_search.py @@ -12,21 +12,23 @@ import asyncio import itertools +import os 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 +from typing import Annotated, NamedTuple, TextIO from mcp.server import MCPServer from mcp.server.mcpserver import Context from mcp.types import CallToolResult, TextContent from pydantic import Field -from .adapter import RawHit, SearchRequest +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, split_lines from .locks import project_lock from .logging_utils import safe_log from .models import ExpandedUnit, ExpandResult, FileHit, FileResult, SearchHit, SearchResult @@ -57,90 +59,284 @@ 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`); `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({})`. + """ + + spans: dict[Span, list[str]] + text: str | None = None + mtime_ns: int | 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 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. 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`. 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) + code_lines = split_lines(code) - file_text = file_cache.get(file, "") + 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 - line, end_line, verified = locate_unit(file_text, code, reported_line, reported_end) - hit_id = f"{file}:{line}-{end_line}" - - code_lines = code.splitlines() - 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=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, ) -async def _read_file_off_loop(file: str) -> str: - """`Path(file).read_text(errors="replace")`, run in a worker thread. +def _sibling_hit( + leader: SearchHit, + match: UnitMatch, + body: list[str], + file_current: 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 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 = 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, + line=match.line, + end_line=match.end_line, + name=match.name, + qualified_name="", + unit_type="", + language=leader.language, + score=match.score, + 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, + 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. + + 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 (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. 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) + 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, []), file_current, 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. - 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 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. """ + 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_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. + `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} + 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()} + + +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(Path(file).read_text, errors="replace") - except OSError: - return "" + 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 + 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 (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"), mtime_ns) + except OSError: + break + return FileLines(spans, None, mtime_ns) + +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 = 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)) + 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: @@ -152,10 +348,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()) @@ -289,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( @@ -366,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) @@ -381,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, ) @@ -397,20 +601,26 @@ 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 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] = {} + 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) - name = unit.get("name") or "" + # 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: - 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(matches), top_units=names[:5]) else: - existing.hits += 1 - if len(existing.top_units) < 5: - existing.top_units.append(name) + existing.hits += len(matches) + existing.top_units.extend(names[: 5 - len(existing.top_units)]) return list(by_file.values()) @@ -434,8 +644,19 @@ 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, 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 @@ -464,9 +685,38 @@ 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_from_raw(raw, snippet_lines, include_code, file_cache, base_path=base_path) for raw in outcome.raw_hits] + 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 + # 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]] = {} + 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]], outcome.index_as_of_ns, base_path + ) + for ri, mis in indexes.items() + } + hits = [built[ri][mi] for ri, mi in keep] notes: list[str] = [] if not hits: @@ -488,7 +738,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 +795,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( @@ -666,34 +918,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'.") @@ -710,9 +934,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( @@ -725,20 +960,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_span, file, 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] @@ -749,7 +981,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/fake_colgrep.py b/server/tests/fake_colgrep.py index 8b6fddb..025bf11 100755 --- a/server/tests/fake_colgrep.py +++ b/server/tests/fake_colgrep.py @@ -1,12 +1,16 @@ #!/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: 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 @@ -86,7 +90,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/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/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_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_render.py b/server/tests/test_render.py index 81f15bb..7cdee1a 100644 --- a/server/tests/test_render.py +++ b/server/tests/test_render.py @@ -3,8 +3,10 @@ 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 ( + FileLines, _file_block, _files_header, _hit_block, @@ -38,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, 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" @@ -50,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, 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, 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) @@ -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], 2, False, FileLines({})) 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], 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, 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 @@ -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_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)) diff --git a/server/tests/test_tools_search.py b/server/tests/test_tools_search.py index c84c64b..098ae52 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 @@ -19,7 +21,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 +316,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 +359,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 +387,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 +399,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 +413,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 +424,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) - 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) + 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 == [] + + +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_from - 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 + def spy(f, spans): + spans = list(spans) + spans_read.append({(Path(f.name).name, *s) for s in spans}) + return real_read_spans(f, 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" + 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}) + + 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 +536,61 @@ 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") + + 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") @@ -564,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") @@ -598,6 +732,309 @@ 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 _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": 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(tools, "escalate", 1, 2, 'def escalate():\n return "subagent"\n', "function"), + "score": 1.0, + }, + ] + fixture_path = tmp_path / "matches_hits.json" + fixture_path.write_text(json.dumps(fixture)) + monkeypatch.setenv("FAKE_COLGREP_HITS", str(fixture_path)) + 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"]) + + +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 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}) + + 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_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") + + 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_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_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, "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": "escalation", "include_code": True}) + + 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_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, + 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"]) + + +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) ----------------------------