Fix Fan-Out Code Search Across Mirror Placements · Entire

can you take a look at https://entire.io/gh/entireio/cli/trails/794/fix-fan-out-code-search-across-mirror-placements and the open finding on the trail (you can also check out the branch) using the entire cli.

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:

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:

is 3 complicated?

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:

merged.Stats.TotalMatches = Σ deduped RepoStats.MatchCount
merged.Stats.TotalFiles   = Σ deduped RepoStats.FileCount
merged.Stats.ReposSearched = len(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 under counts, 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?

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

Trail #794 updated

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:

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