perf: deleting a transcript no longer re-reads the whole history (#99) - #127

🤖 Automated first-pass review

TL;DR

The change is well-reasoned and the incremental machinery it relies on is described as already existing, but the diff as supplied only shows the guard-clause edit plus tests — it does not show removeFile, the ownership-restoration pass, or the append fast-path condition. I can review the intent and the test strategy, but I cannot verify the three load-bearing claims (records dropped, removedOwnedUuids collected from deletions, append path requires deletions.length === 0) from the diff. Merge-readiness hinges on those being true.

Analysis

• Correctness framing. Removing deletions.length > 0 from unsafeSourceMutation is only safe if every deletion-dependent invariant is handled elsewhere. The PR body enumerates them; the diff does not demonstrate them. The new tests assert the outcomes (bodyReads === 0, analysis/records match the full loader), which is the right acceptance shape, but they pass only because the described internals hold — if any of the three claims is inaccurate, the tests may be green while a subtler stale-aggregate case regresses.
• Residual risk class. The dangerous deletion path is not "ordinary delete" but deletion where the deleted file contributed to cross-file response identity (removedOwnedUuids), or where deletion interacts with canonicalOrderChanged/metadataRetags. The two new tests plus the reworked clone test cover the ownership-transfer case at bodyReads === 1; that is a reasonable boundary. What is not evidenced: deletion combined with a move in the same refresh (moves.length > 0 still forces full rebuild — good), and deletion that changes canonical order without a move. The body asserts perf: a new transcript no longer re-tags every file discovered after it (#99) #123 makes metadataRetags empty, but there is no test here pinning canonicalOrderChanged === false for a delete.
• Test hygiene. Pinning Date.now inside the analysis window is a real improvement and correctly makes the old code fail for the right reason. The manual Date.now save/restore + try/finally is repeated across three tests; that is acceptable but a shared helper would reduce the chance a future test forgets the finally. No mock framework issue apparent.
• CHANGELOG. An entry is present under [Unreleased]/Fixed and follows Keep a Changelog. Good. Note the PR body and CHANGELOG disagree on the corpus figures: body says "976-file, 1.58 GB"; CHANGELOG says "650-file, 1.4 GB". One of these is stale — pick one and make body and CHANGELOG match.
• Docs / i18n. This is a pure internal performance fix with no user-facing string and no new setting, so the eight-locale requirement and the nine-README update do not apply. The AGENTS.md invariant "runtime refreshes must not fall back to a full-corpus body read" is exactly what this PR advances, so no doc contradiction.
• Privacy invariants. No new diagnostics, no path/session leakage introduced by the diff; the CHANGELOG text contains only counts and bytes. Fine.
• Attribution. The PR footer uses the Claude Code banner; per CONTRIBUTING that is the tool actually used, so truthful. The commit trailer requirement in AGENTS.md applies to maintainer-authored commits, not this external PR — no action here.
• Merge readiness. Scope is one logical change, on-branch, tests included. The only blocker is verifiability of the three internal claims plus the corpus-number discrepancy.

Suggested next step(s)

1. Reconcile the corpus figures between PR body (976-file / 1.58 GB) and CHANGELOG (650-file / 1.4 GB); update whichever is stale.
2. Add one focused assertion that a plain delete does not set canonicalOrderChanged / metadataRetags, pinning the perf: a new transcript no longer re-tags every file discovered after it (#99) #123 dependency this PR leans on.
3. If verifiable from the base repo, confirm in the PR description (or a comment) the exact lines of removeFile, the ownership-restoration removedOwnedUuids collection, and the deletions.length === 0 condition in the append fast path — so a reviewer can check the three claims without reconstructing them from the full module.

🤖 Generated by DeepSeek as an automated first pass — not a maintainer decision.

Addressing the first-pass review:

1. Corpus figures reconciled — the stale 650-file/1.4 GB numbers came from the 30.09 live observations; the comment and CHANGELOG now carry the bench measurement (976 files, 1.58 GB, 30.0 s CPU / 975 body reads → 1.0 s / 0 body reads).
2. No-retag contract pinned — the plain-delete test now asserts changed deep-equals { append: 0, rebuild: 0, move: 0, delete: 1 }. Since canonicalOrderChanged escalates through replaceWithFullAnalysisPlans() (which sets changed.rebuild to the full corpus), rebuild === 0 proves no canonical-order escalation happened.
3. The three load-bearing lines (this branch, src/claudeIncrementalIndex.ts): records and the stored contribution are dropped by removeFile at line 2968, applied to deletions at line 3008; removedOwnedUuids is collected from deletions at line 2753; the append fast path requires deletions.length === 0 at line 2800, which is what routes a delete refresh to the in-memory buildAnalysisRuntimeState rebuild instead of the fast path.

Addressing the review of b8b63ac, rebased onto current main:

1. Deleted owner + appended claimant. The ownership-restoration pass skipped any file already in plannedPaths, so a claimant planned as a tail append never replayed the prefix duplicate. An append whose retained analysisAllUuids intersect the removed owner's UUIDs is now promoted to a full replay; a plan that already re-reads its body is left alone. Test: deleting an owner while appending its claimant replays that claimant — one body read (a bystander is not read), full-loader content-analysis equivalence, an unchanged follow-up with zero body reads, and the prior index/analysis frozen before the refresh.

2. Unrelated deletion + earlier append preempting a later owner. Deletion kept the refresh off the append fast path, which was the only place the later-owner probe ran. Mixed deletion refreshes now run that probe and still fall back to one ordered rebuild on collision. Test: an unrelated deletion does not hide an earlier append preempting a later owner, with the same equivalence, follow-up and snapshot checks.

3. Changelog. The note is under Unreleased, not the published 2.5.0 section.