#133 + #134: the semantic flag stops loading the model; one context per posture - #138
Conversation
Closes #133. vault_info documented itself as a model-free read while semantic_available called LocalEmbedder::load — parsing config.json, building the tokenizer, and mmapping model.safetensors — on every call, and vault_info sits on the first-paint path and on every vault switch, exactly the path the project/embed split exists to keep model-free. semantic_available now asks EmbedConfig::is_model_provisioned, the repo's one "installed" check (files_present) and literally load's own fail-fast precondition, so the flag answers the same question without the load. What that trades is stated where the trade is made: a present-but-corrupt model reads as semantic: true here and fails at the first search/reindex instead — already fail-fast and actionable, and the flag's contract is "is a model installed", which is what a file check answers. The two comments that said otherwise now say what happens. A new b2-embed test pins the property the probe rests on: over an empty cache the probe and load agree (NotProvisioned), and with the three required files present-but-corrupt the probe says installed while load fails with Load — so the file gate demonstrably passed and only the deeper parse failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVJSUVP6FHdhYoAPfqrcTU
…er) bundle Closes #134. Behavior-preserving. Fifteen b2-core functions threaded the same four-or-five values in three incompatible parameter orders, with four #[allow(clippy::too_many_arguments)] holding the line; rm::reproject_dangled used a fourth order inside its own file. They now take one of two view structs, split along the model-free vs. embedding posture the module docs already stated: ProjectionCtx (conn, root, idgen, cfg) and EmbedCtx (that plus the embedder). The posture becomes the type system's to keep — rm / create_note / write hold no embedder and so cannot embed — and the ordering hazard and all four clippy allows are gone. Vault::ctx / Vault::embed_ctx build them, so a façade call is now e.g. mv::move_note(self.embed_ctx(), &b2id, &old_rel, to). Both are Copy, borrow-only, never stored: the sanctioned exception to "prefer owned fields" (CLAUDE.md; the NoteRow precedent), said so in their doc comments. ingest_vault keeps its loose arguments deliberately — it exists to supply the default ChunkConfig, so it builds the context itself. The riders from the same survey of mv.rs, all in this diff because they are the same files and the same review: - the move preflight (same-path refusal, occupied-target refusal with the case-only-rename carve-out, create_dir_all + rename), written three times, is now refuse_same_path / refuse_occupied / rename_with_parents. Composed rather than folded into one preflight on purpose: a folder move refuses an inside-itself destination and a missing source *before* it looks at the target, and that error precedence is part of each op's contract. - the "rewrite the inbound files" loop, written three times, is now rewrite_inbound — which is also the one home of move_dir's `let empty = BTreeMap::new()` sentinel. - the wikilink .md-convention replacement and the fragment-preserving resource replacement, each computed in two places, are now wiki_replacement and resource_replacement. As pure functions they get direct unit tests, including the #fragment rule, which had none — its only cover was a whole-vault move. The ~12 integration-test call sites build their contexts with the new constructors; the ~15 ingest_vault sites are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVJSUVP6FHdhYoAPfqrcTU
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
📝 WalkthroughWalkthroughThe PR introduces ChangesCore context and vault operation refactor
Model availability probe
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
crates/b2-core/src/mv.rs (1)
261-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
BTreeSet<&str>for the touched-file key set.
touchedcollects&Stringvalues.&strkeys work for bothBTreeMaplookups andvault_root.join(...), and match the project preference for&strover&String. This is a local readability change only; behavior is identical.♻️ Optional simplification
- let touched: BTreeSet<&String> = wiki.keys().chain(md.keys()).collect(); + let touched: BTreeSet<&str> = wiki.keys().chain(md.keys()).map(String::as_str).collect(); for src_path in touched { let abs = vault_root.join(src_path); let raw = fs::read_to_string(&abs)?; let (pass1, n1) = rewrite_links(&raw, wiki.get(src_path).unwrap_or(&none)); let (pass2, n2) = rewrite_md_targets(&pass1, md.get(src_path).unwrap_or(&none)); if n1 + n2 > 0 { fs::write(&abs, pass2)?; - rewrote.push(src_path.clone()); + rewrote.push(src_path.to_string()); links_rewritten += n1 + n2; } }As per coding guidelines: "Prefer
&strover&String,&[T]over&Vec<T>".🤖 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/mv.rs` around lines 261 - 278, Update the touched-file key set in rewrite_inbound to use BTreeSet<&str> instead of BTreeSet<&String>, converting the chained wiki and md keys to string slices while preserving the existing lookups, path joining, and rewrite 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.
Nitpick comments:
In `@crates/b2-core/src/mv.rs`:
- Around line 261-278: Update the touched-file key set in rewrite_inbound to use
BTreeSet<&str> instead of BTreeSet<&String>, converting the chained wiki and md
keys to string slices while preserving the existing lookups, path joining, and
rewrite behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a7b0dce-3a09-4978-99a9-f9e1a097b77f
📒 Files selected for processing (13)
crates/b2-core/src/add.rscrates/b2-core/src/ingest.rscrates/b2-core/src/mv.rscrates/b2-core/src/rm.rscrates/b2-core/src/vault.rscrates/b2-core/tests/cancel.rscrates/b2-core/tests/collision.rscrates/b2-core/tests/embed.rscrates/b2-core/tests/graph.rscrates/b2-core/tests/project_embed.rscrates/b2-desktop/src/commands.rscrates/b2-desktop/src/main.rscrates/b2-embed/src/model.rs
Review feedback on PR #138. The set of files to rewrite collected &String, where the repo's own rule is &str over &String — and nothing here wanted the owned type: BTreeMap::get takes a Borrow key and Path::join takes an AsRef, so both call sites accept the slice unchanged. Behavior identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVJSUVP6FHdhYoAPfqrcTU
2 participants
Both improvements from the PR #135 survey, agreed with and implemented. Two commits, one per issue.
#133 —
vault_info's "model-free read" becomes truesemantic_availablecalledLocalEmbedder::load— parseconfig.json, build the tokenizer, mmapmodel.safetensors— on everyvault_info, which is the first-paint path and every vault switch. The comment claiming otherwise was load-bearing and wrong.Took option 1 from the issue: probe, don't load.
EmbedConfig::is_model_provisionedis the repo's one "installed" check (files_present) and is literallyload's own fail-fast precondition, so the flag answers the same question far cheaper. The accepted trade is stated where it is made: a present-but-corrupt model reads assemantic: trueand fails at the firstsearch/reindexinstead — already fail-fast and actionable, andsemantic's contract is "is a model installed", which is exactly what a file check answers. Options 2 (cache the load) and 3 (derive fromembed_status) both buy less than they cost: the first adds staleness rules for a guarantee the flag doesn't promise, the second conflates "a model exists" with "this vault is embedded", which the UI already distinguishes vianotes_embedded/notes_total.Both stale comments now describe what happens, and a new
b2-embedtest pins the property the probe rests on: over an empty cache the probe andloadagree (NotProvisioned); with the three required files present-but-corrupt the probe says installed whileloadfails withLoad— so the file gate demonstrably passed and only the deeper parse failed.#134 — two context structs, split by posture
Fifteen functions threaded the same values in three incompatible orders, with four
#[allow(clippy::too_many_arguments)]holding the line. They now takeProjectionCtx(conn, root, idgen, cfg) orEmbedCtx(that plus the embedder), built byVault::ctx/Vault::embed_ctx:The model-free rule the module docs state for
rm/create_note/writeis now the compiler's to keep — those ops hold no embedder and cannot embed. All four clippy allows and the ordering hazard are gone. Both structs areCopy, borrow-only, never stored — the sanctioned exception to "prefer owned fields" (CLAUDE.md, theNoteRowprecedent), said so in their doc comments.ingest_vaultdeliberately keeps its loose arguments: it exists to supply the defaultChunkConfig, so it builds the context itself and its ~15 test call sites are untouched.Riders, same files and same review:
create_dir_all+rename) →refuse_same_path/refuse_occupied/rename_with_parents. Composed rather than folded into onepreflighton purpose: a folder move refuses an inside-itself destination and a missing source before it looks at the target, and that error precedence is part of each op's contract.rewrite_inbound, which is also the single home ofmove_dir'slet empty = BTreeMap::new()sentinel..md-convention replacement and the fragment-preserving resource replacement →wiki_replacement/resource_replacement, with direct unit tests including the#fragmentrule, which had none.Verification
cargo fmt --check,cargo clippy --workspace --exclude b2-desktop --all-targets(clean),cargo test --workspace --exclude b2-desktop(all green, 31 binaries), and theui/suite (27 tests).b2-desktopwas not compiled here — this container is Linux and the crate's WebView deps needgdk-3.0/webkit, socargo check -p b2-desktopdies ingdk-sys's build script (CI runs macOS, wherejust cicovers it). The #133 diff there is three lines plus comments; its changed expression was type-checked against the realb2-embedAPI in a scratch crate, and the identicalis_model_provisioned(&config.model)call runs in the newb2-embedtest. #134 does not touchb2-desktop— the host only ever callsVault.One observation, deliberately not fixed
Extracting the replacement rules made it visible that
move_noterepairs only the[[…]]form, so an inbound Markdown-form link to a note ([text](concepts/memory.md), whichresolve_targetdoes resolve to a note) is left stale by a note move. That is pre-existing behavior, not something #134 asked to change, and fixing it properly needs the note move to learn the relative-vs-root convention logic the resource move has — worth its own issue if you want it.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation