Fix Fan-Out Code Search Across Mirror Placements

I've verified all four findings against the actual code on the branch. Here's my assessment.

Trail #794 — "fix: fan out code search across mirror placements"

The PR changes groupReposByCell so code search fans out to every placement of a repo (home + mirror cells), not just its home cell. The diff touches only cmd/entire/cli/cell_fanout.go (+ its test). I'm now on the branch investigate-cli-cross-region-code-search-fan-out-i.

There are 4 open findings (the dashboard shows 3 by default; a 4th is filtered out as stale). Verdicts after reading the code:

🟠 #3 — Medium · search_cmd.go · Real, and the one worth acting on

Merged stats double-count matches for mirrored repos.

Confirmed. In mergeSearchResults, the result list is deduped by repo\x00path\x00line:col (search_cmd.go:698-708), but the stats are not:

• Stats.TotalMatches/TotalFiles/ReposSearched are summed across cells (:649-651)
• RepoStats dedupes by repo name but sums MatchCount/FileCount (:715-716)

A US repo with an EU mirror indexes the same content, so both cells return the same 3 matches. Post-merge: 3 unique results shown, but "6 matches across N files in 2 repos", and ReposSearched counts the repo twice. Worse, the TotalMatches(6) > shown(3) branch (:799) prints "Showing 3 of 6 matches" — falsely implying truncation.

The comment at :731 says summing is deliberate, but it predates this PR and its stated justification (zero-match repos, per-cell truncation) doesn't cover two cells indexing the same content — which this PR turns into the common case. The diff added zero tests to search_cmd_test.go; the existing TestMergeSearchResults only covers two cells with distinct repos, so the double-count path is untested.

🔵 #2 — Low · cell_fanout.go:103 · Valid robustness nit

Home placement identified by p.Cell == r.Cell string-match instead of the RepoPlacement.Mirror bool.

Confirmed — RepoPlacement.Mirror exists (oas_schemas_gen.go:6911) and directly marks home(false)/mirror(true). The current code assigns the home cluster slug only when p.Cell == r.Cell; if the server ever returns placements with an empty top-level r.Cell, homeCell == "" matches nothing → the precise slug→catalog join is silently skipped for every group and they all drop to the fuzzy fallback. p.Mirror == false is the contract-guaranteed primitive. Marked PLAUSIBLE because it only bites if the server omits top-level Cell alongside placements.

🔵 #1 — Low · cell_fanout.go:170 · Valid narrow corner

Jurisdiction-default fallback can route a placement to the wrong cell in its jurisdiction.

Confirmed but genuinely rare. byJurisdiction prefers the default cluster (:148-154). If a placement's cell name appears in no catalog URL and it has no slug, the last-resort fallback (:170) routes to the jurisdiction's default cluster — which may not host that repo ID → silent miss (the exact bug class this PR fixes). It sits behind two more-precise paths (slug match, then matchClusterByCellInURL), so it should rarely trigger. The suggestion is just a debug log so a miss is diagnosable.

⚪ #4 — Medium · Stale / superseded — no action needed

This is the earlier-code-version ancestor of #1. It flagged the wrong-cell routing at the old :158; the author then fixed exactly what it suggested by adding matchClusterByCellInURL (commit 43ae6061f). That's why it went stale, and #1 is its narrowed residual.

---

Recommendation

The one substantive issue is #3 — it's a correctness/UX regression this PR introduces, and it's untested. #1 and #2 are minor hardening. My suggested path:

1. Fix #3: reconcile stats against the deduped result set (recompute TotalMatches/TotalFiles/ReposSearched from the deduped Results, or dedupe RepoStats by taking max instead of summing for the mirror case), and add a search_cmd_test.go case with two cells returning identical (repo,path,line) tuples.
2. Optionally address #2 (swap to p.Mirror) and #1 (add the debug log) as cheap follow-ups.

I haven't changed any code or touched the findings' status — this trail belongs to evisdren. Want me to:

• implement the #3 fix (+ test) on this branch, or
• resolve/dismiss any of these via entire trail finding (e.g. dismiss the stale #4, or resolve once #3 is fixed)?

Not really — it's contained to one function (mergeSearchResults) and the core fix is ~15–20 lines plus a test. There's exactly one wrinkle that keeps it from being fully trivial.

Why it's mostly easy: the response already gives you a per-repo breakdown (RepoStats{Repo, MatchCount, FileCount}, keyed by repo name which is stable across cells). The results list is already deduped correctly. So the fix is to make the stats agree with the deduped data instead of blind-summing across cells:

• RepoStats dedup (:715): change existing.MatchCount += rs.MatchCount → max(...). Mirror cells report the same repo's same count, so you take one, not the sum.
• Aggregate stats (:649-651): stop summing raw per-cell Stats; instead derive them from the deduped RepoStats:

That kills all three visible symptoms: "6 matches" → 3, the false "Showing 3 of 6" truncation line, and the doubled repo count. DurationMs (max) stays as-is.

The one wrinkle — and the reason I'd guard rather than blindly swap:

1. ReposSearched counts zero-match repos (the :731 comment calls this out). A repo with 0 matches isn't in RepoStats, so len(RepoStats) slightly undercounts, and a zero-match mirrored repo can't be de-duplicated from the response at all (no per-repo identity exists for it — mirror placements have different repo IDs, so even our own grouping can't collapse them). This is unrecoverable from peregrine's response alone.
2. Assumes RepoStats is always populated when TotalMatches > 0. If peregrine ever returns matches with an empty RepoStats, deriving totals from it would zero them out — a regression.

Both are handled by a small guard: only recompute from RepoStats when it's non-empty; otherwise fall back to today's summed behavior. That keeps the fix safe and still strictly better than the current double-count (over-counting a zero-match mirrored repo in one denominator is far milder than inflating every matched repo's count + the false truncation message).

So: low complexity, one real edge case, ~20 lines + a merge-test with two cells returning identical (repo,path,line) tuples. The existing TestMergeSearchResults uses two distinct repos, so it doesn't exercise this — I'd add a sibling test.

Want me to implement it on this branch?

Done. Finding #3 is fixed on the branch.

What I changed

cmd/entire/cli/search_cmd.go — mergeSearchResults The RepoStats dedup now keeps one representative entry per repo (max of each count) instead of summing across cells, and records the duplicated portion. The aggregate stats then subtract that mirror double-count from the summed totals:

Key properties, as discussed:

• Truncation-aware — the base is peregrine's own per-cell totals; I only subtract the provable duplicate portion (sum − max per repo), never more. A single cell that legitimately truncated keeps its true total.
• Zero-match repos preserved — they contribute 0 to the subtraction, so ReposSearched still counts them (I subtract the mirror overcount rather than recomputing len(RepoStats), which would have dropped them).
• Graceful degradation — a matched repo with no RepoStats row, or a zero-match mirrored repo, can't be de-duplicated from the response, so it keeps its summed contribution (a mild over-count, far milder than the current "count every mirrored match twice"). I documented this in the comment.
• The stale :731 comment (which justified the old summing) is replaced.

cmd/entire/cli/search_cmd_test.go

• New TestMergeSearchResults_MirrorPlacementsDoNotDoubleCount — the finding's exact scenario (US home + EU mirror, same 3 matches from both cells): asserts 3 unique results, TotalMatches=3/TotalFiles=2/ReposSearched=1, DurationMs still max, and RepoStats deduped to a representative {3,2} rather than summed {6,4}.
• Strengthened TestMergeSearchResults_DeduplicatesOverlappingCells — it already modeled overlapping cells but only checked result count (so it passed despite the bug); it now asserts stats aren't double-counted too.

Verification: mise run fmt + mise run lint (0 issues), and go test ./cmd/entire/cli/ ./cmd/entire/cli/codesearch/ all pass. The existing TestMergeSearchResults (distinct repos) still asserts summed 4/3/2 — unaffected, since with no duplicate repo names all overcounts are 0.

Notes / what I did not touch

• I left the two Low findings (#1 debug-log-for-jurisdiction-fallback, #2 use p.Mirror) alone — cheap follow-ups if you want them, but out of scope for "fix #3."
• I haven't committed or changed any finding's status on the trail (it's evisdren's trail).

Want me to commit this (branch it's already on), knock out the two Low findings while I'm here, or resolve #3 on the trail via entire trail finding resolve?

Confirmed. The two files are modified but uncommitted in the working tree — nothing is staged, committed, or pushed. The branch has no upstream tracking set locally.

I don't commit or push unless you ask me to (per the repo conventions). To get this onto the PR I'd:

1. git commit the two files (with an Entire trailer, on this branch)
2. git push to origin (which will update the existing PR since the branch already exists on origin)

Before I push, the repo's rule is to run mise run lint on the final tree — already clean — and ideally the full mise run test:ci.

Want me to commit and push to the PR? If so, I can also resolve finding #3 on the trail in the same go, referencing the commit sha.

All done. Summary of what shipped:

Committed & pushed to PR

• Commit b72a6e621 on investigate-cli-cross-region-code-search-fan-out-i — pushed to origin, so the PR is updated (43ae6061f..b72a6e621). Checkpoint ref went up too.
• Fix: mergeSearchResults now dedupes RepoStats by repo (representative max, not sum) and subtracts the provable mirror duplicate from TotalMatches/TotalFiles/ReposSearched. Truncation-awareness and zero-match repos preserved.
• Tests: new TestMergeSearchResults_MirrorPlacementsDoNotDoubleCount + stats assertions added to the previously-silent overlapping-cells test.
• Full mise run fmt / lint (0 issues) / test:ci (unit + integration + e2e canary — all passed) before the push.

Trail #794 updated

• Finding #3 (Medium, double-count) → resolved, with a note referencing the commit sha and what changed. The trail now shows 1 resolved, 3 open.

Still open (I didn't touch these — your call)

The trail correctly re-marked #3's location as stale now that search_cmd.go changed. Remaining open:

• #2 (Low, cell_fanout.go:103) — use p.Mirror instead of p.Cell == r.Cell string-match. Cheap hardening.
• #1 (Low, cell_fanout.go:170) — add a debug log when the jurisdiction-default fallback fires, so a silent mis-route is diagnosable.
• #4 (Medium, stale) — superseded ancestor of #1; already effectively addressed by the earlier matchClusterByCellInURL commit. Reasonable to dismiss as superseded if you want it off the board.

Want me to knock out #1 and #2 (both small) in a follow-up commit, and/or dismiss the stale #4?

All three findings I fixed are now resolved. The board is down to 1 open — and it's the stale #4 (019f3fb2-78a), the superseded ancestor of the debug-log finding I just resolved.

Summary

Two commits pushed to the PR branch (investigate-cli-cross-region-code-search-fan-out-i):

Commit	Finding	Fix
b72a6e621	#3 Medium (search_cmd.go)	Dedupe merged code-search stats across mirror cells
eaa6b3232	#2 Low (cell_fanout.go:103)	Home placement keyed off RepoPlacement.Mirror, not p.Cell == r.Cell
eaa6b3232	#1 Low (cell_fanout.go:170)	Debug log when the jurisdiction-default fallback fires

• Each commit passed fmt + lint (0 issues) + full test:ci (unit + integration + e2e canary).
• New/strengthened tests: TestMergeSearchResults_MirrorPlacementsDoNotDoubleCount, stats assertions on the overlapping-cells test, and TestGroupReposByCell_PlacementSlugFromMirrorFlag.
• All three findings resolved on the trail with notes referencing the commit shas.

Remaining: #4 (Medium, stale) — recommend dismiss

019f3fb2-78a is the earlier-code-version predecessor of #1: it flagged the wrong-cell jurisdiction routing at the old :158, which the PR author already addressed by adding matchClusterByCellInURL, and which I've now further hardened with the diagnostic log. It's stale (points at code that no longer exists in that form) and effectively superseded.

Want me to dismiss #4 as superseded (with a note pointing at eaa6b3232 / finding #1)? That'd clear the board to zero open.