GH #81: Surface b2id collisions and identity restamps, never auto-fix - #84
Conversation
…y restamps (GH #81) Reindex-time diagnostics for the two external-edit pathologies #79 left open, surfacing only — no auto-fix, no writes beyond W1's missing-id stamp (W4). - Duplicate-file collision (a Finder copy): the projection pass pre-scans claims and resolves each contested b2id BEFORE any row is written — incumbent-wins (the path the index already attributes the id to, when that file still claims it), falling back to first-in-sorted-walk *flagged as a tie-break* on a memory-less rebuild. Walk-order last-wins is gone (note: "a copy.md" sorts BEFORE "a.md", so any vault-pure order rule would hand the identity to the copy). Shadowed claimants stay on disk but get no row, and the collision is re-reported on every pass until the human resolves it; all three resolutions (delete the copy / strip its b2id line / delete the original) heal on the next pass. - Identity restamp (a b2id line blanked or removed outside b2): the fresh stamp is reported (path, old id -> new id) instead of silently churning identity. Never auto-restored: removing the line is also the documented gesture for requesting a fresh identity, so restoring would guess intent. Changes: - b2-core: ProjectOutcome/IngestOutcome and ReindexReport/ProjectReport carry `collisions` + `restamped`; plan_reindex previews everything read-only (stamp_paths — the "notes without a b2id" view — would_restamp, collisions); ingest_file refuses an outright identity steal (Error::B2idCollision) while the ordinary external-move repointing still works; project_file stays unguarded (the in-app save path — the whole-vault pass owns that state). - b2-cli: reindex prints the notices (stderr, like `skipped`); --dry-run lists id-less notes, pending restamps, and collisions; --json carries them all. - desktop/ui: ProjectReport flows through IPC unchanged; the reindex flash and the fs-watch pulse (reconcile onReport) surface anomalies the moment they land; new pure anomalyNotice formatter with node tests. - docs: S3 carve-out in invariants.md (a collision has no well-defined projection; incumbent-wins is anomaly containment, surfaced every pass, equivalence resumes on resolution); index-engine.md §8 documents the mechanics; CLAUDE.md Flow ① summarizes. - tests: crates/b2-core/tests/collision.rs pins incumbent-beats-walk-order, the flagged tie-break, all three resolution flows, restamp reporting, the dry-run preview, and the single-note guard (refuse steal / allow move). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXYN4zb8iFtLPP1mHm1KuQ
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds b2id collision and identity-restamp detection to projection, ingest, and dry-run planning. Reports now carry anomaly details through core, CLI, desktop, and UI layers, with integration tests and documentation covering resolution and repeated reporting. ChangesIdentity anomaly handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
crates/b2-core/src/ingest.rs (1)
736-754: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftEvery projection pass now reads every note's file twice.
scan_b2id_claims(736-754) does a full read of every note in the vault to build the claim map; then the Phase 1 loop inproject_vault(857-885) callsproject_note_and_chunks, which reads the same file again from disk. Previously each note was read once per pass; now it's read twice, and — per theProjectReportdoc comment — this whole-vault pass runs on every desktop fs-watch pulse, not just an explicitreindex. For vaults with many notes this doubles I/O on a hot, frequently-triggered path.The two-pass structure (scan claims fully before writing any row) is a reasonable way to avoid walk-order dependence, but the raw content read in the scan could be captured and threaded into Phase 1 instead of re-reading, trading some transient memory for the avoided I/O.
💡 Sketch: thread scanned content into Phase 1
-fn scan_b2id_claims(vault_root: &Path, rel_paths: &[String]) -> HashMap<String, Vec<String>> { - let mut claims: HashMap<String, Vec<String>> = HashMap::new(); - for rel in rel_paths { - let Ok(raw) = fs::read_to_string(vault_root.join(rel)) else { - continue; - }; - if let Some(id) = note::parse(&raw).fields().b2id.clone() { - claims.entry(id).or_default().push(rel.clone()); - } - } - claims -} +fn scan_b2id_claims( + vault_root: &Path, + rel_paths: &[String], +) -> (HashMap<String, Vec<String>>, HashMap<String, String>) { + let mut claims: HashMap<String, Vec<String>> = HashMap::new(); + let mut contents: HashMap<String, String> = HashMap::new(); + for rel in rel_paths { + let Ok(raw) = fs::read_to_string(vault_root.join(rel)) else { + continue; + }; + if let Some(id) = note::parse(&raw).fields().b2id.clone() { + claims.entry(id).or_default().push(rel.clone()); + } + contents.insert(rel.clone(), raw); + } + (claims, contents) +}Then pass the cached
contentsmap intoproject_note_and_chunksso it skips its ownfs::read_to_stringwhen a cached entry exists.Also applies to: 857-885
🤖 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 `@crates/b2-core/src/ingest.rs` around lines 736 - 754, Update scan_b2id_claims and the Phase 1 flow in project_vault to retain each successfully read note’s raw contents in a cache, then pass that cache into project_note_and_chunks so cached files are parsed without a second fs::read_to_string. Preserve unreadable-file handling and the existing full claim-map-before-writing behavior.
🤖 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 `@docs/design/index-engine.md`:
- Around line 404-406: Update the collision-guard wording near
Error::B2idCollision to describe the full single-note ingest scope as “add” and
“mv,” replacing the narrower “add” reference while leaving “write” excluded
because it does not use ingest_file.
---
Nitpick comments:
In `@crates/b2-core/src/ingest.rs`:
- Around line 736-754: Update scan_b2id_claims and the Phase 1 flow in
project_vault to retain each successfully read note’s raw contents in a cache,
then pass that cache into project_note_and_chunks so cached files are parsed
without a second fs::read_to_string. Preserve unreadable-file handling and the
existing full claim-map-before-writing 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: 01747efa-79f6-431d-860d-bff4ad0c3f42
📒 Files selected for processing (13)
CLAUDE.mdcrates/b2-cli/src/main.rscrates/b2-core/src/error.rscrates/b2-core/src/ingest.rscrates/b2-core/src/vault.rscrates/b2-core/tests/collision.rscrates/b2-desktop/src/error.rsdocs/design/index-engine.mddocs/design/invariants.mdui/src/main.tsui/src/reconcile.test.tsui/src/reconcile.tsui/src/types.ts
…bbit) The single-note guard lives in `ingest_file`, so its scope is the `add`/`mv`/`link` path — not `add` alone (and not `write`, which goes through the deliberately-unguarded `project_file`). Also document why the projection pass's claims pre-scan deliberately re-reads files in Phase 1 instead of caching bytes: the re-read is page-cache-warm, and stamping from pre-scan-cached bytes would widen the window in which a stamp write-back could clobber a mid-pass external edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXYN4zb8iFtLPP1mHm1KuQ
2 participants
Implements the GH #81 specification for detecting and surfacing cross-note
b2idcollisions and identity restamps during projection passes, with incumbent-wins collision resolution and no automatic fixes (W4 — the human decides).Summary
When two or more files present the same
b2id(e.g., a note duplicated in Finder), or when a file'sb2idline is externally removed/blanked causing an identity change, B2 now:b2id:line, or delete the original)add/link/mvrefuse to project a file claiming an id whose incumbent still exists and claims it, preventing silent shadowingKey Changes
Core engine (
crates/b2-core/src/ingest.rs):ProjectedNotefrom a 5-tuple to a struct carryingrestamped_from: Option<String>to track identity churnB2idCollision,CollisionPrecedence, andRestampedNotetypes to represent the anomaliesscan_b2id_claims()to pre-scan all files' claimed ids before projectionresolve_collisions()to decide contested ids: incumbent-wins when the index remembers, tie-break on first-in-walk otherwiseguard_single_note_collision()to refuse identity steals in single-note ingestproject_note_and_chunks()to detect when a stamp changes an identity (external blank/removal)ProjectOutcomeandIngestOutcometo carrycollisionsandrestampedvectorsVault façade (
crates/b2-core/src/vault.rs):B2idCollision,CollisionPrecedence,RestampedNotefor the public contractcollisionsandrestampedfields toReindexReportandProjectReportPlannedRestampand updatedReindexPlanto preview restamps in dry-runError handling (
crates/b2-core/src/error.rs):B2idCollisionerror variant for single-note ingest refusalsCLI (
crates/b2-cli/src/main.rs):Desktop UI (
ui/src/):reconcile.tsto pass projection reports toonReportcallback (for pulse notices)anomalyNotice()to generate compact, actionable flash messages per anomalymain.tsto display anomaly notices in reindex flashestypes.tswith TypeScript interfaces for collisions and restampsTests (
crates/b2-core/tests/collision.rs):Notable Details
https://claude.ai/code/session_01TXYN4zb8iFtLPP1mHm1KuQ
Summary by CodeRabbit
New Features
Bug Fixes
Documentation