fix(index): seed nested-worktree indexes from a sibling instead of rebuilding - #177
fix(index): seed nested-worktree indexes from a sibling instead of rebuilding#177ntotten wants to merge 2 commits into
Conversation
…-embedding Working in a fresh git worktree of an already-indexed repo re-embedded every file from scratch (~minutes on a local embedder) instead of reusing a sibling worktree's embeddings. Two independent bugs defeated the existing donor-seeding path, and both bite Claude Code's default repo/.claude/worktrees/<name> layout. Bug 1 — donor discovery picked the wrong worktree (internal/config/seed.go). FindDonorIndexBase selected the FIRST `git worktree list` entry containing the project. git lists the main checkout first, so for a worktree nested inside the repo it identified the main checkout as "self", searched for donors at nonexistent <sibling>/.claude/worktrees/<name> paths, and skipped the one real donor (the parent repo's index). Fix: pick the deepest (most specific) containing worktree — the longest matching path, since every match is an ancestor of the project and they form a prefix chain. Bug 2 — the CLI indexer never seeded (cmd/index.go, cmd/seed.go). Seeding only ran in the MCP search handler, but the SessionStart hook spawns `lumen index`, which created the DB first; SeedFromDonor then no-ops because the DB exists, so the hook permanently won the race and forced a full rebuild. Fix: seed from a donor in runIndexer, under the index lock, before the DB is created. Because both the CLI indexer and the MCP handler can now seed the same fresh worktree concurrently, harden SeedFromDonor (internal/index/seed.go) to copy to a unique temp file (os.CreateTemp) and publish via a create-if-absent hard link (os.Link fails on EEXIST). The loser of the race no-ops instead of renaming a fresh copy over a database the winner already opened for writing. Adds unit tests for the nested-worktree layout, concurrent seeding, and the runIndexer seed helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIndexer startup now seeds new project databases from sibling worktree indexes. Donor selection handles nested worktrees, and database publication uses temporary copies with create-if-absent hard links for concurrent seeders. Tests cover discovery, fallback, nested paths, and concurrent publication. ChangesIndex seeding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant runIndexer
participant seedFromDonorIfNew
participant FindDonorIndexBase
participant SeedFromDonor
participant destinationDB
runIndexer->>seedFromDonorIfNew: seed new database
seedFromDonorIfNew->>FindDonorIndexBase: find donor worktree index
FindDonorIndexBase-->>seedFromDonorIfNew: donor path
seedFromDonorIfNew->>SeedFromDonor: copy donor to destination
SeedFromDonor->>destinationDB: publish with create-if-absent hard link
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/config/seed_test.go (1)
68-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a table-driven test for multiple layout cases.
As per coding guidelines, use table-driven tests for multiple test cases in Go. Consider refactoring this new nested worktree test, along with the adjacent
TestFindDonorIndex_WithSiblingandTestFindDonorIndex_WrongModel, into a single table-driven test to cleanly group related worktree discovery scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/seed_test.go` around lines 68 - 105, Refactor TestFindDonorIndex_NestedWorktree, TestFindDonorIndex_WithSibling, and TestFindDonorIndex_WrongModel into one table-driven test covering each worktree discovery scenario. Define per-case setup and expected donor results, reuse shared Git and database setup where possible, and preserve each test’s existing assertions and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/seed_test.go`:
- Around line 39-93: Consolidate the four seedFromDonorIfNew test functions into
one table-driven test with cases for an existing database, successful seeding,
no donor, and seed error. Keep each case’s stub behavior and assertions intact,
using per-case fields or callbacks to express expected calls and outcomes; run
cases with t.Run and preserve the existing temporary database setup and
discardLogger usage.
In `@internal/index/seed.go`:
- Around line 76-86: Update the temporary-file handling in the seed flow around
tmpFile and copyFile: defer tmpFile.Close(), change the copy operation to use
the already-open file descriptor instead of reopening tmp by path, and handle
any Close error before linking the completed seed. Preserve cleanup of the
temporary path and existing error wrapping.
---
Nitpick comments:
In `@internal/config/seed_test.go`:
- Around line 68-105: Refactor TestFindDonorIndex_NestedWorktree,
TestFindDonorIndex_WithSibling, and TestFindDonorIndex_WrongModel into one
table-driven test covering each worktree discovery scenario. Define per-case
setup and expected donor results, reuse shared Git and database setup where
possible, and preserve each test’s existing assertions and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59f20e4e-cc9b-451f-a686-cf8cdbac5644
📒 Files selected for processing (7)
cmd/index.gocmd/seed.gocmd/seed_test.gointernal/config/seed.gointernal/config/seed_test.gointernal/index/seed.gointernal/index/seed_test.go
ntotten
changed the title
- SeedFromDonor: copy into the open temp descriptor instead of closing and re-opening it by name (avoids a Windows sharing violation), defer the descriptor's close, and check the Close error before publishing via os.Link so a short write can't be linked into place. Removes the now-unused copyFile. - cmd/seed_test.go: consolidate the four seedFromDonorIfNew cases into a single table-driven test, per the repo's Go testing guideline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI statusOnly the E2E job is red; Build (macOS + Ubuntu), Lint, Test, Script tests (all three OSes), CLA, and CodeRabbit are green. The E2E failures are all in the The same suite fails on the most recent Happy to dig in if you think it's related, but I don't believe this change can affect the language suite. |
|
E2E failing indicates an issue in the PR of this code, since it's passing on master |
3 participants
Problem
Working in a fresh git worktree of an already-indexed repo re-embedded every file from scratch (minutes on a local embedder) instead of reusing a sibling worktree's embeddings. Two independent bugs defeated the existing donor-seeding path, and both bite Claude Code's default
repo/.claude/worktrees/<name>layout, where worktrees are nested inside the repo.Fixes
Bug 1 — donor discovery picked the wrong worktree (
internal/config/seed.go)FindDonorIndexBaseselected the firstgit worktree listentry containing the project. git lists the main checkout first, so for a worktree nested inside the repo it identified the main checkout as "self", searched for donors at nonexistent<sibling>/.claude/worktrees/<name>paths, and skipped the one real donor — the parent repo's index. Fix: pick the deepest (most specific) containing worktree. That's the longest matching path, since every match is an ancestor of the project and they form a prefix chain.Bug 2 — the CLI indexer never seeded (
cmd/index.go, newcmd/seed.go)Seeding only ran in the MCP search handler, but the SessionStart hook spawns
lumen index, which created the DB first;SeedFromDonorthen no-ops because the DB already exists, so the hook permanently won the race and forced a full rebuild. Fix: seed from a donor inrunIndexer, under the index lock, before the DB is created.Hardening — safe concurrent seeding (
internal/index/seed.go)Because both the CLI indexer and the MCP handler can now seed the same fresh worktree concurrently,
SeedFromDonorcopies to a unique temp file (os.CreateTemp) and publishes it via a create-if-absent hard link (os.Linkfails withEEXIST). The loser of the race no-ops instead of renaming a fresh copy over a database the winner has already opened for writing.Tests
Adds unit tests for the nested-worktree layout, concurrent seeding, and the
runIndexerseed helper.go testandgo vetpass for theconfig,index, andcmdpackages.Relationship to #170
#170 also makes the CLI indexer seed under the lock (Bug 2), by adding lock coordination to
getOrCreatein the MCP handler. This PR addresses Bug 2 with a different approach — hardeningSeedFromDonorfor concurrent callers rather than refactoring the handler's locking — and additionally fixes donor discovery for nested worktrees (Bug 1), which #170 does not touch. Happy to rebase, split, or defer to #170 for the overlapping part if that's easier to land.Summary by CodeRabbit
New Features
Bug Fixes
Tests