Repository navigation
perf: rewriting a transcript no longer re-reads the whole history (#99) - #128
Conversation
|
🤖 Automated first-pass review TL;DR Analysis
Suggested next step(s)
🤖 Generated by DeepSeek as an automated first pass — not a maintainer decision. |
|
Addressing the first-pass review, all four points in one push:
|
Carl723000
left a comment
There was a problem hiding this comment.
Thanks @rsyuzyov for the order/ownership probes and the updates to the earlier feedback. A fresh review against released main d6d69aa found a mixed rewrite/append case those probes do not cover.
Rewritten owner + appended surviving claimant: A owns UUID X, and B's existing prefix contains X's suppressed duplicate. Rewrite A to drop X while keeping its first timestamp unchanged, and append unrelated content to B. The bounded-rewrite guard change exposes the ownership-restoration pass's plannedPaths.has(claimant) skip (head line 2731). B gets only a tail read. X disappears from content analysis and stays missing on the next unchanged refresh.
Order is unchanged, and X is absent from B's parsed tail, so neither probe restores the missing contribution. The released baseline matches the full loader. The case was independently rerun with synthetic files; token totals and prior snapshots remained correct. Combining #127 and #128 still reproduces the content-analysis loss.
Please promote append plans that require ownership replay and add this mixed-event regression with full-loader equivalence, an unchanged follow-up refresh and snapshot immutability. Existing index tests pass, so those tests alone do not cover the missing path. Please also reconcile this original branch with current main: GitHub reports policy-document merge conflicts. Preserve the current canonical/bilingual policy and move the new release note under Unreleased, not released 2.5.0.
Reviewed head: a998b040b1cc596b5b71fe46f370419de06c8e18. This remains a valuable optimization to refine through your original PR; it has not been included in 2.5.0.
🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.
…audeCodeUsage#99) A mid-file rewrite or truncation produced a 'source' rebuild plan with a prior, and unsafeSourceMutation escalated it to replaceWithFullAnalysisPlans: every surviving file was re-read. Live logs show ~31 such full rebuilds in four days on one workstation, 20–40 s each. The bounded path was already in place: the rewritten file is re-read in full, its previously owned UUIDs feed the ownership restoration pass, and the materialized analysis is rebuilt in memory from retained contributions. Two post-parse probes guard what a rewrite can actually break — a moved first event (canonical order) and a UUID claimed ahead of its later owner (first-owner dedup); either retries as the full ordered rebuild, reusing the existing append-probe fallback. Measured on a 976-file, 1.58 GB corpus, one mid-history transcript truncated: 19.3 s CPU, 976 body reads, 149k aggregate mutations before; 0.8 s CPU, one body read, 11 mutations after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ClaudeCodeUsage#99) First-pass review follow-ups: the AGENTS.md invariant (and the zh-CN companion) now names the one permitted full-corpus retry — probe-guarded, when a changed file's ownership order is unproven; the two fallback tests assert the escalation through changed.rebuild instead of an exact bodyReads total; the orderShiftedRebuild seed documents its short-circuit; the stale 650-file figures in the unsafeSourceMutation comment match the 976-file bench. Co-Authored-By: Claude Code <noreply@anthropic.com>
…CodeUsage#99) The ownership-restoration pass skipped a claimant already planned as a tail append, so a prefix duplicate vanished and stayed missing. That append is promoted to a full replay. Rebased onto the published 2.5.0 policy wording; the release note stays under Unreleased. Co-Authored-By: Claude Code <noreply@anthropic.com>
a998b04 to
05d6816
Compare
|
Addressing the review of
|
Carl723000
left a comment
There was a problem hiding this comment.
The appended-claimant restoration fix is verified, but a separate ordering regression remains at 05d6816cee7c16d8b719d0afb620e8bc3288be75.
Blocking: ownership promotion can bypass the first-timestamp/order-change guard
The promotion around src/claudeIncrementalIndex.ts:2897–2903 changes an append plan into an ownership rebuild. The order-shift check around :3027–3029 only considers source rebuilds, so it misses an order change in that promoted file.
Synthetic reproduction (no real logs required):
- A and B have no dated events. A owns UUID X. B's existing prefix contains a suppressed duplicate X and UUID Y owned by B.
- C has its first event at 09:00 and contains a duplicate Y with different content.
- In one refresh, rewrite A to remove X (still undated), and append B's first dated event at 11:00.
- Restoring X promotes B's append to an ownership rebuild. B now sorts after C, but the source-only guard does not replay canonical content ownership.
Expected: Y's content-analysis owner is C, matching a full load. Actual: the incremental snapshot keeps Y from B; an unchanged follow-up with zero body reads retains the mismatch. The baseline and #127 individually pass this case; #128 fails it. Token totals and previously returned snapshots stayed correct in this fixture, so this finding is about content-analysis ownership, not demonstrated token-total corruption.
Please retain order-change detection for promoted rebuilds regardless of their overwritten reason (or an equivalent correctness-preserving guard), with a regression comparing the initial, changed and unchanged follow-up snapshots against the full loader, and checking prior-snapshot immutability. Please also exercise the deletion variant when integrating with #127. Holding this PR until that is addressed; no need to replace the original contribution.
🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.
Retain the contributor's bounded transcript rewrite/truncation path alongside the existing deletion fast path and concurrent manifest discovery. Validate first timestamp changes for all prior-file rebuild reasons so ownership restoration cannot leave stale source-order deduplication. Add rewrite/truncation/deletion regressions with canonical-loader equivalence, unchanged-refresh zero body reads and immutable prior-snapshot checks. Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>
Carl723000
left a comment
There was a problem hiding this comment.
Correctness finding
The current a864bfe head can retain stale content-analysis UUID ownership when discovery ties reorder existing files while a new file is added. canonicalOrderChanged requires the projected old-file count to equal the complete current manifest count, so the new file suppresses that check.
Synthetic case: B/C share a UUID at the same first timestamp; rewrite an unrelated A without changing its timestamp, add unique D, and change the manifest from [A,B,C] to [A,C,B,D]. The canonical loader assigns the shared UUID to C; the incremental snapshot retains B. The content-analysis estimate is 16 rather than 106 and remains stale on an unchanged refresh. This is not an actual usage/billing discrepancy in the fixture.
Compare the relative canonical order of surviving existing files even when the manifest also has insertions/deletions, and retain a regression against the canonical loader before merging this head.
中文:新增文件不应屏蔽存活旧文件之间的相对归属顺序检查。当前组合场景仍会产生持久的内容分析归属偏差,需补齐守卫和回归后再合入。
🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.
Compare the relative canonical order of surviving old files even when a refresh also inserts or deletes files. Detect discovery-rank changes on changed bodies, without treating deletions or distinct timestamp order as unsafe by themselves. Add mixed rewrite/insertion/deletion/append regressions and safe controls with canonical-loader equality, immutable snapshots and zero-read unchanged followups. Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>
Co-authored-by: OpenAI Codex <215057067+openai-codex[bot]@users.noreply.github.com>
…sage into codex/pr128-rewrite-integration
Carl723000
left a comment
There was a problem hiding this comment.
Reviewed final integration commit 5dd3da06f9b1604f5647b5b348617400e5f14b2a against current main fcdb858f1eb4b8d9dcd7ae5a8988435cddfd510a.
The ownership-restoration and surviving-file order findings are addressed. Regression cases cover rewrite/insertion/deletion combinations, tied versus distinct timestamps, concurrent appends and deterministic reused identities. Synthetic comparisons agree with the canonical full loader, preserve prior snapshots, and read zero bodies on an unchanged follow-up.
Local integrated Node suite: 2699 passed, 2 existing skips, no failures. The latest hosted workflow is still running; merge remains conditional on all required checks succeeding at this exact head. No release is part of this change.
中文:前两轮归属问题已修复;新增文件不再屏蔽旧文件顺序变化,独立文件与身份复用均有确定性用例。与最新 main 联合后的本地全量测试通过,仍须等待本提交完整云端门禁全绿。
🤖 Generated by OpenAI Codex as an automated first pass — not a maintainer decision.
Problem
Any mid-file mutation of one transcript — a
/rewindtruncation, session repair, an mtime-only touch that fails append verification — produced a'source'rebuild plan with a prior, andunsafeSourceMutationescalated it toreplaceWithFullAnalysisPlans(): every surviving file was re-read from disk. Live logs from one workstation show ~31 such full rebuilds in four days, 20–40 s each — the dominant remaining refresh cost after #119–#123. (Same class as the deletion rebuild in #127, through a different clause.)Change
The
sourcePlansclause is removed fromunsafeSourceMutation; only deletions (until #127 lands) and moves still escalate pre-parse. Everything the bounded path needs already existed:'ownership'plan and is re-read, nothing else);Two post-parse probes guard what a rewrite can actually break, reusing the existing append-probe fallback:
contribution.firstTimestampMs !== prior.firstTimestampMs), so its place in the canonical order is unproven.Either one retries once as the full ordered rebuild.
Measured
976-file, 1.58 GB corpus, one mid-history transcript truncated:
Tests
Five new tests, each comparing records and
contentAnalysisagainst the full loader:a truncated transcript re-reads only that file—bodyReads === 1(fails on main: full rebuild).a mid-file rewrite re-reads only the rewritten file— same.a rewrite that drops an owned uuid re-reads the twin that also carries it— the rewritten file plus the surviving twin, nothing else.a rewrite that moves the first event falls back to a full ordered rebuild.a rewrite claiming a uuid owned by a later file falls back to a full rebuild.🤖 Generated with Claude Code