feat(memory): carve the inert diff types out from behind git-diff - #141
Conversation
`git-diff` gated the whole `memory::diff` module, so a host that did not want libgit2 in its dependency graph could not so much as *name* a `CrossSourceDiff`. That is more than the feature needs to gate: `types.rs` and `source.rs` are `serde`/`std`-only and reach no `git2` symbol — only `ledger.rs` and `ledger_helpers.rs` do. `pub mod diff` is now always compiled. Ungated: `types`, `source`, and their re-exports. Gated on `git-diff`: `ledger` + `ledger_helpers` (the two that touch git2), `checkpoint` / `diff` / `snapshot` (whose impls are written against `Ledger`), and `DiffEngine` itself — its inherent methods live in those modules, so an ungated engine would be a handle with nothing to call. The distinction is describe-vs-compute: without the feature a host can pass a diff around, match on a `ChangeKind`, and implement `SnapshotItemSource`; it simply cannot produce one. This unblocks a `memory-git` gate in OpenHuman, whose always-on subconscious profile renders `CrossSourceDiff`/`ChangeKind` into prompts. Stubbing those types host-side instead would mean two definitions of one serde shape drifting apart silently — which is why OpenHuman's own gate guidance says to put a domain's inert types in a dependency-free submodule and gate only behaviour. Two `#[cfg(not(feature = "git-diff"))]` tests pin the carve-out, because the disabled build is the only thing that can catch it regressing: re-gating these types compiles fine with the feature on and only breaks downstream. They construct and serde-round-trip the types rather than just naming them, so a gated-away derive fails too. The pre-existing `types`/`source` unit tests now run in the disabled build as well. Verified both ways: `--features obsidian,persona,sync` (43 → the git-backed tests compile out, 14 inert ones run) and with `git-diff,wiki-git` added (43 diff tests pass, unchanged). Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
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)
📝 WalkthroughWalkthroughThe memory diff module keeps types and sources available without ChangesMemory diff feature boundary
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/memory/diff/mod.rs`:
- Around line 140-171: Move the cfg-gated carve_out_tests module from
src/memory/diff/mod.rs into the matching sibling diff_tests.rs file, preserving
both test functions and their imports. Leave only the conditional module
declaration in mod.rs, pointing to the sibling test module, and keep the tests
gated for builds without the git-diff feature.
- Around line 156-159: Update the CrossSourceDiff serialization test to
deserialize the generated json back into a CrossSourceDiff, requiring its
Deserialize implementation, then assert a restored field such as the checkpoint
value matches the original. Keep the existing serialization assertion and
ChangeKind coverage.
🪄 Autofix
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: a219b860-1676-458e-9ce4-a2623c6e3c73
📒 Files selected for processing (2)
src/memory/diff/mod.rssrc/memory/mod.rs
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0015 · 18,668 in / 4,980 out · 15,150 cached (81%) · z-ai/glm-5.2
critique: $0.0005 · 5,033 in / 1,813 out · 4,318 cached (86%) · z-ai/glm-5.2
security: $0.0002 · 4,991 in / 411 out · 3,998 cached (80%) · z-ai/glm-5.2
tests: $0.0002 · 3,576 in / 544 out · 3,021 cached (84%) · z-ai/glm-5.2
description: $0.0005 · 4,223 in / 1,914 out · 3,145 cached (74%) · z-ai/glm-5.2
What this change touches3 files, +96 -10 across 2 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/memory/diff<br/>2 files +81 -1"]:::changed
n1["src/memory<br/>1 file +15 -9"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
…ip them Review follow-ups on #141: - The tests were an inline `mod` in `mod.rs`; every other test module in this directory is a `#[path = "*_tests.rs"]` sibling. Now they match. - The serde test only serialised. These types exist to cross a boundary, so a `Deserialize` derive that got gated away would not have failed it — it now round-trips and asserts the restored fields. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Why
git-diffgated the wholememory::diffmodule, so a host that did not want libgit2 in its dependency graph could not so much as name aCrossSourceDiff.That is more than the feature needs to gate. Of the eleven files under
src/memory/diff/, exactly two touchgit2:types.rsandsource.rsareserde/std-only.What
pub mod diffis now always compiled.types,source, and their re-exportsgit-diffledger+ledger_helpers(touch git2),checkpoint/diff/snapshot(impls written againstLedger), andDiffEngineitselfDiffEngineis gated even though its own declaration is dependency-free: its inherent methods live in the gated modules, so an ungated engine would be a handle with nothing to call.The distinction is describe vs compute. Without the feature a host can pass a diff around, match on a
ChangeKind, and implementSnapshotItemSource; it simply cannot produce one.Who needs this
A
memory-gitgate in OpenHuman, whose always-on subconscious memory profile rendersCrossSourceDiffandChangeKindinto agent prompts. Stubbing those types host-side instead would mean two definitions of one serde shape, free to drift apart — which is why OpenHuman's own gate guidance says to put a domain's inert types in a dependency-free submodule and gate only behaviour. That gate shedsgit2+libgit2-sys+libz-sys, two of the five remaining native C builds in its kernel profile.Tests
Two
#[cfg(not(feature = "git-diff"))]tests pin the carve-out. The disabled build is the only thing that can catch this regressing — re-gating these types compiles fine with the feature on and only breaks downstream.They construct and serde-round-trip the types rather than just naming them, so a gated-away
derivefails too. The pre-existingtypes/sourceunit tests now also run in the disabled build.Verification
Summary by CodeRabbit