feat(big_repo): keyhive#30
Open
dman-os wants to merge 16 commits into
Open
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (61)
📝 WalkthroughWalkthroughThe PR updates BigRepo, BigSync, and daybook to use Keyhive-derived document identities and ready-state document handles, adds Keyhive storage and transport wiring, expands subscription and membership filtering, and refreshes tests and scripts to match the new flows. ChangesKeyhive integration and sync refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/daybook_core/test_support.rs (1)
157-166: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the test repo identity in sync with BigRepo’s keyhive seed.
Line 166 seeds BigRepo from random bytes, but the
RepoCtxPartsidentity is later populated from a separateiroh_secret_key/peer_id. That makes BigRepo advertise a different local peer than the repo context. Mirror the production pattern by generating the Iroh secret before boot and using its bytes askeyhive_seed.Proposed fix
- let peer_id = crate::peer_id_from_label(&format!("test_{}", uuid::Uuid::new_v4().simple())); + let iroh_secret_key = iroh::SecretKey::generate(); + let iroh_public_key = iroh_secret_key.public(); // Initialize SharedBigRepo with memory storage let (big_sync_host, big_sync_stop) = crate::test_support::boot_part_store("sqlite::memory:").await?; let (big_repo, acx_stop) = BigRepo::boot( big_repo::Config { - keyhive_seed: rand::random::<[u8; 32]>(), + keyhive_seed: iroh_secret_key.to_bytes(), storage: big_repo::StorageConfig::Memory, }, Arc::clone(&big_sync_host.store), @@ - let iroh_secret_key = iroh::SecretKey::generate(); - let local_peer_key = daybook_types::doc::format_peer_key(peer_id.as_bytes()); + let local_peer_key = daybook_types::doc::format_peer_key(iroh_public_key.as_bytes()); @@ - iroh_public_key: peer_id.to_string(), + iroh_public_key: iroh_public_key.to_string(), iroh_secret_key,Also applies to: 299-319
🤖 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 `@src/daybook_core/test_support.rs` around lines 157 - 166, The BigRepo identity and the repo context identity are being initialized from different sources, so the test setup can advertise a mismatched local peer. Update the `boot_part_store` / `BigRepo::boot` flow to generate the Iroh secret key first and derive `keyhive_seed` from its bytes, then reuse the same identity when building `RepoCtxParts` so the test repo stays consistent with the production pattern. Apply the same change in the later setup block referenced by this test support code so `BigRepo`, `iroh_secret_key`, and `peer_id` all remain in sync.src/utils_rs/lib.rs (1)
440-446: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn an error here instead of panicking on malformed digests.
src/daybook_core/blobs.rs:820-821now sends external digest strings through this helper. A 47-character base58btc string that decodes to anything other than 34 bytes will hitassert_eq!(len, 34)and abort the process instead of returningRes.Suggested fix
pub fn decode_base58_multihash_blake3(digest: &str) -> Res<[u8; 32]> { if digest.len() != 47 { eyre::bail!("expected length 47 multibase blake3 digest"); } let mut buf = [0; 34]; let len = decode_base58_multibase_onto(digest, &mut buf)?; - assert_eq!(len, 34); + if len != 34 { + eyre::bail!("expected 34 decoded bytes for multihash blake3 digest, got {len}"); + } if buf[0] != BLAKE3 as u8 { eyre::bail!("multibase not blake3"); }🤖 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 `@src/utils_rs/lib.rs` around lines 440 - 446, The `decode_base58_multihash_blake3` helper currently panics on malformed input because it uses `assert_eq!(len, 34)` after `decode_base58_multibase_onto`, which can abort callers like `daybook_core::blobs` instead of returning `Res`. Replace that assertion with proper error handling in `decode_base58_multihash_blake3`, returning an `eyre::bail!`/error when the decoded length is not 34 and keeping the function’s `Res<[u8; 32]>` contract intact.
🧹 Nitpick comments (1)
src/daybook_types/doc.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialTurn this FIXME into a tracked verification item.
This reads like a possible metadata-integrity bug, but it is currently unactionable and has a typo (
mantaining). Please either link it to an issue or add a small regression test arounddmeta.actors. I can help draft that issue/test.🤖 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 `@src/daybook_types/doc.rs` at line 1, The FIXME in doc.rs about the drawer not maintaining dmeta.actors is currently untracked and unactionable; replace it with either a reference to a real issue or a small regression test that verifies dmeta.actors is preserved through the relevant drawer flow. Use the existing doc-related symbols around dmeta.actors to locate the affected path, and correct the typo while making the note actionable and verifiable.
🤖 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 `@Cargo.toml`:
- Around line 245-257: Remove the checked-in local patch overrides from
Cargo.toml: the keyhive_* and subduction_* path entries under the
dependency/patch sections should not rely on sibling directories outside this
repo. Update the manifest so Cargo metadata/check/test works in a normal
checkout without ../keyhive or ../subduction, keeping the existing package names
and patch behavior only if they can resolve from within this repository.
In `@src/big_repo/Cargo.toml`:
- Line 45: The rand_08 dependency is disabling default features, which drops the
getrandom support needed by rand_08::rngs::OsRng. Update the rand_08 entry in
Cargo.toml to re-enable the appropriate feature so OsRng can compile, keeping
the change localized to the rand_08 dependency specification.
In `@src/big_repo/handler.rs`:
- Around line 189-191: The boot flow in boot_keyhive should not ignore
ingest_from_storage() failures; it currently logs a warning in
keyhive_protocol.ingest_from_storage() handling and then continues booting.
Change this branch to propagate the error back through boot_keyhive’s Res result
instead of swallowing it, so BigRepo fails to start when keyhive storage
recovery cannot complete. Use the existing boot_keyhive and ingest_from_storage
symbols to locate the error path and keep the return value consistent with the
rest of the boot sequence.
- Around line 114-124: The Keyhive branch in handler logic is swallowing
failures by logging them and then still returning success, which causes failed
keyhive messages to be acknowledged as handled. Update the
BigRepoWireMessage::Keyhive path in Handler::handle so the error from
Handler::<Sendable, BigRepoIrohTransport>::handle is propagated as a composed
Keyhive error or otherwise fail-loud, and do not return Ok(()) after a Keyhive
handling failure.
In `@src/big_repo/keyhive_storage.rs`:
- Around line 75-76: Flush the temp keyhive file to stable storage before
calling the rename in the keyhive storage write path so the checkpoint is
crash-durable, not just atomically swapped. Update the logic around the
tokio::fs::write and tokio::fs::rename sequence in the keyhive storage operation
to explicitly sync the written file (and any required parent directory metadata)
before publishing the final name, preserving the existing temp-file-then-rename
flow.
- Around line 95-100: The keyhive storage scan in the code that parses
`archives/` and `ops/` entries is swallowing malformed filenames by continuing
on invalid `file_stem` or `StorageHash::from_hex` results. Change this logic so
the recovery path fails immediately on any unexpected or non-hex storage entry
instead of skipping it, using the existing parsing flow around
`path.file_stem()`, `StorageHash::from_hex`, and the surrounding archive/ops
iteration to surface the corruption as a hard error.
In `@src/big_repo/test.rs`:
- Around line 1620-1624: The stress object→document mapping is never populated,
so `snapshot_docs` in `SyncRepoNode` silently falls back through `unwrap_or` and
loses the original stress object IDs. Update the code that records snapshot
results to insert each `obj_id` to `doc_id` mapping into `obj_doc_ids`, and then
use that mapping consistently when reading payloads/parts instead of defaulting
to `obj_id`. Apply the same fix in the related `snapshot_docs` paths referenced
by the other occurrences so missing mappings are surfaced rather than hidden.
In `@src/big_sync_core/ids.rs`:
- Line 7: The exported macro alias_byte32id! currently depends on caller-scope
symbols, so update its expansion to use fully qualified paths instead of
unqualified names. In the macro definition in ids.rs, replace references like
Byte32Id and DecodeError with $crate::Byte32Id and $crate::DecodeError, and
qualify serde traits through ::serde::{Serialize, Deserialize} so downstream
crates can invoke the macro without extra imports.
In `@src/daybook_core/blobs.rs`:
- Around line 815-821: The blob digest format change in blob_id_to_digest_str
and digest_str_to_blob_id is breaking backward compatibility. Update
digest_str_to_blob_id to accept both the new Blake3 multihash encoding and the
նախկին base58-multibase form, and keep blob_id_to_digest_str as the canonical
output for new values. Add a migration/compatibility path in these helpers so
existing stored digests and cross-version references still parse correctly.
In `@src/daybook_core/crypto.rs`:
- Around line 61-64: Implement the missing store property methods before
repository loading: `KeysRepo::load` depends on `AmStore::prop()` for both
`KeysRepoStore` and the other store mentioned in the review, but the current
`prop()` implementations still `todo!()` and will panic at boot. Replace each
`prop()` with the correct static property name used by the repo/store loading
logic, and verify the `KeysRepo` load path can initialize both stores without
hitting a panic.
- Around line 326-341: The binary multikey visitor in `visit_bytes` currently
validates the 34-byte input and `PubKey::MULTIKEY_PREFIX`, but then copies the
entire 34-byte slice into a 32-byte buffer, which will panic on valid input.
Update `visit_bytes` in the `crypto` deserializer to copy only the raw key
payload after the prefix (the 32 bytes following `PubKey::MULTIKEY_PREFIX`) into
the `[u8; 32]` buffer, keeping the existing prefix and length validation intact.
- Around line 120-125: The encrypted-key store is loaded from key_doc_handle but
the store handle in daybook_core::crypto::enckey_store is created with
app_doc_handle, so encrypted-key updates will be written to the wrong document.
Update the AmStoreHandle::new call used for EncKeysStore so it is attached to
key_doc_handle instead of app_doc_handle, keeping the store and its backing
Automerge document aligned.
- Around line 240-243: Update the multikey serialization/parsing in crypto.rs:
in `to_multikey`, write `Self::MULTIKEY_PREFIX` into the first 2 bytes of the
34-byte buffer and keep the UUID bytes in the remaining slice so it no longer
panics; then fix `FromStr` to remove the multibase `z` prefix before base58
decoding so it correctly round-trips with `Display`. Use the existing
`to_multikey`, `FromStr`, and `Display` implementations as the entry points for
the change.
- Around line 353-380: The PubKey autosurgeon round-trip is inconsistent:
`reconcile` currently emits raw 32-byte key material via
`reconciler.bytes(self.0)`, while `hydrate_bytes` is written for
multikey-encoded input but only accepts length 32 before slicing the prefix.
Update `PubKey::reconcile` to serialize using the same multikey format returned
by `to_multikey()`, and change `PubKey::hydrate_bytes` to expect 34 bytes,
validate the `MULTIKEY_PREFIX`, then copy only the final 32-byte key payload
before calling `Self::new`.
In `@src/daybook_core/lib.rs`:
- Line 31: The public crypto module export is exposing unfinished store-loading
APIs, so update the crate boundary in lib.rs and the crypto module to avoid
publishing incomplete functionality. Either make AmStore::prop() and any related
store props fully implemented before keeping crypto public, or keep crypto
private / remove the public re-export until those todo!() paths are eliminated.
Use the crypto module, AmStore, and prop() symbols to locate the affected API
surface.
In `@src/xtask/keyhive_demo.rs`:
- Around line 199-260: The demo in the Bob decryption flow is swallowing
invariant failures by only printing them and still returning Ok(()). Update the
key paths in the keyhive_demo.rs block around Bob’s decrypt logic to propagate
errors with ? or bail! instead of println-only handling, especially for missing
documents, failed post-grant decrypt, unexpected pre-grant success, failed
try_open on the sealed predecessor key, and failed predecessor-chain decrypt.
Use the existing symbols get_document, try_decrypt_content_keyed,
try_decrypt_content, try_open, and the outer demo function return path so the
xtask exits non-zero when any invariant breaks.
---
Outside diff comments:
In `@src/daybook_core/test_support.rs`:
- Around line 157-166: The BigRepo identity and the repo context identity are
being initialized from different sources, so the test setup can advertise a
mismatched local peer. Update the `boot_part_store` / `BigRepo::boot` flow to
generate the Iroh secret key first and derive `keyhive_seed` from its bytes,
then reuse the same identity when building `RepoCtxParts` so the test repo stays
consistent with the production pattern. Apply the same change in the later setup
block referenced by this test support code so `BigRepo`, `iroh_secret_key`, and
`peer_id` all remain in sync.
In `@src/utils_rs/lib.rs`:
- Around line 440-446: The `decode_base58_multihash_blake3` helper currently
panics on malformed input because it uses `assert_eq!(len, 34)` after
`decode_base58_multibase_onto`, which can abort callers like
`daybook_core::blobs` instead of returning `Res`. Replace that assertion with
proper error handling in `decode_base58_multihash_blake3`, returning an
`eyre::bail!`/error when the decoded length is not 34 and keeping the function’s
`Res<[u8; 32]>` contract intact.
---
Nitpick comments:
In `@src/daybook_types/doc.rs`:
- Line 1: The FIXME in doc.rs about the drawer not maintaining dmeta.actors is
currently untracked and unactionable; replace it with either a reference to a
real issue or a small regression test that verifies dmeta.actors is preserved
through the relevant drawer flow. Use the existing doc-related symbols around
dmeta.actors to locate the affected path, and correct the typo while making the
note actionable and verifiable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7f51208-8a54-40e2-9ea2-973748d29ac5
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockand included by**/*docs/DEVDOC/log.mdis excluded by!docs/DEVDOC/**and included by**/*flake.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (32)
Cargo.tomlREADME.mdsrc/big_repo/Cargo.tomlsrc/big_repo/backend.rssrc/big_repo/changes.rssrc/big_repo/handler.rssrc/big_repo/keyhive.rssrc/big_repo/keyhive_conn.rssrc/big_repo/keyhive_storage.rssrc/big_repo/lib.rssrc/big_repo/runtime.rssrc/big_repo/test.rssrc/big_repo/wire.rssrc/big_sync_core/ids.rssrc/daybook_core/Cargo.tomlsrc/daybook_core/blobs.rssrc/daybook_core/crypto.rssrc/daybook_core/drawer/mutations.rssrc/daybook_core/keyhive.rssrc/daybook_core/lib.rssrc/daybook_core/repo.rssrc/daybook_core/rt.rssrc/daybook_core/rt/dispatch.rssrc/daybook_core/rt/triage.rssrc/daybook_core/sync/bootstrap.rssrc/daybook_core/test_support.rssrc/daybook_types/doc.rssrc/utils_rs/Cargo.tomlsrc/utils_rs/lib.rssrc/xtask/Cargo.tomlsrc/xtask/keyhive_demo.rssrc/xtask/main.rs
Comment on lines
+245
to
+257
| keyhive_crypto = { path = "../keyhive/keyhive_crypto" } | ||
| keyhive_core = { path = "../keyhive/keyhive_core" } | ||
| beekem = { path = "../keyhive/beekem" } | ||
| # sqlite-vec = { git = "https://github.com/vlasky/sqlite-vec", rev = "9facf1ab84f2a360c0f1e62db3a7f3eca5bc099a" } | ||
| # | ||
| # [patch."https://github.com/dman-os/subduction"] | ||
| # subduction_core = { path = "../subduction/subduction_core" } | ||
| # subduction_iroh = { path = "../subduction/subduction_iroh" } | ||
| # subduction_websocket = { path = "../subduction/subduction_websocket" } | ||
| # sedimentree_core = { path = "../subduction/sedimentree_core" } | ||
| # sedimentree_fs_storage = { path = "../subduction/sedimentree_fs_storage" } | ||
| # subduction_crypto = { path = "../subduction/subduction_crypto" } | ||
| # subduction_keyhive = { path = "../subduction/subduction_keyhive" } | ||
| [patch."https://github.com/dman-os/subduction"] | ||
| subduction_core = { path = "../subduction/subduction_core" } | ||
| subduction_iroh = { path = "../subduction/subduction_iroh" } | ||
| subduction_websocket = { path = "../subduction/subduction_websocket" } | ||
| sedimentree_core = { path = "../subduction/sedimentree_core" } | ||
| subduction_redb_storage = { path = "../subduction/subduction_redb_storage" } | ||
| subduction_crypto = { path = "../subduction/subduction_crypto" } | ||
| subduction_keyhive = { path = "../subduction/subduction_keyhive" } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo root: '
pwd
echo
echo 'Top-level files:'
git ls-files | sed -n '1,120p'
echo
echo 'Cargo.toml patch sections and nearby context:'
nl -ba Cargo.toml | sed -n '220,270p'
echo
echo 'Workspace-relative sibling directories mentioned by the patches:'
for p in ../keyhive ../subduction; do
if [ -e "$p" ]; then
echo "$p exists"
find "$p" -maxdepth 2 -type f | sed -n '1,40p'
else
echo "$p missing"
fi
done
echo
echo 'Search for other patch overrides in Cargo.toml:'
rg -n '^\[patch|path = "\.\./' Cargo.tomlRepository: dman-os/townframe
Length of output: 3874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Cargo.toml patch block:'
sed -n '238,262p' Cargo.toml
echo
echo 'Sibling paths from repo root:'
for p in ../keyhive ../subduction; do
if [ -e "$p" ]; then
echo "$p exists"
ls -ld "$p"
else
echo "$p missing"
fi
done
echo
echo 'References to keyhive/subduction in the repo:'
rg -n 'keyhive|subduction' README.md Cargo.toml src .github .cargo .gitmodulesRepository: dman-os/townframe
Length of output: 50373
Remove the checked-in local patch overrides.
These [patch] entries point to ../keyhive and ../subduction, which are not present in a normal checkout, so cargo metadata/check/test depends on a specific sibling directory layout outside this repo.
🤖 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 `@Cargo.toml` around lines 245 - 257, Remove the checked-in local patch
overrides from Cargo.toml: the keyhive_* and subduction_* path entries under the
dependency/patch sections should not rely on sibling directories outside this
repo. Update the manifest so Cargo metadata/check/test works in a normal
checkout without ../keyhive or ../subduction, keeping the existing package names
and patch behavior only if they can resolve from within this repository.
| serde_json = { workspace = true } | ||
| sqlx = { workspace = true, features = ["sqlite", "runtime-tokio"] } | ||
| rand.workspace = true | ||
| rand_08 = { version = "0.8", package = "rand", default-features = false } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Cargo.toml excerpt ==\n'
nl -ba src/big_repo/Cargo.toml | sed -n '35,55p'
printf '\n== keyhive.rs excerpt ==\n'
nl -ba src/big_repo/keyhive.rs | sed -n '120,190p'
printf '\n== Search rand_08 usages ==\n'
rg -n "rand_08::|package = \"rand\"|default-features = false|OsRng" src/big_repo -SRepository: dman-os/townframe
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Cargo.toml excerpt ==\n'
sed -n '35,55p' src/big_repo/Cargo.toml | cat -n
printf '\n== keyhive.rs excerpt ==\n'
sed -n '120,190p' src/big_repo/keyhive.rs | cat -n
printf '\n== rand_08 / OsRng search ==\n'
rg -n "rand_08|OsRng|default-features *= *false|features *= *\\[" src/big_repo -SRepository: dman-os/townframe
Length of output: 5154
🌐 Web query:
rand 0.8 OsRng feature gate default-features false docs.rs
💡 Result:
In the rand 0.8.x series, the OsRng struct is available only if the getrandom feature is enabled [1][2][3][4]. By default, the rand crate enables the std feature, which in turn implies the getrandom feature, making OsRng available automatically in standard environments [1][4]. If you disable default features in your Cargo.toml (e.g., default-features = false), OsRng becomes unavailable unless you explicitly re-enable the getrandom feature [1][2][4]. You can achieve this with the following dependency configuration: [dependencies] rand = { version = "0.8", default-features = false, features = ["getrandom"] } Without the getrandom feature explicitly enabled, attempting to use OsRng in a no_std environment will result in a compilation error [1][5][4].
Citations:
- 1: https://docs.rs/crate/rand/0.8.3
- 2: https://docs.rs/crate/rand/0.8.0
- 3: https://docs.rs/crate/rand/0.8.5
- 4: https://crates.io/crates/rand/0.8.6
- 5: https://docs.rs/crate/rand/0.8.2
Re-enable getrandom for rand_08 src/big_repo/Cargo.toml:45
rand_08::rngs::OsRng requires getrandom in rand 0.8; with default-features = false, this dependency won’t compile unless that feature is added back.
Suggested fix
-rand_08 = { version = "0.8", package = "rand", default-features = false }
+rand_08 = { version = "0.8", package = "rand", default-features = false, features = ["getrandom"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rand_08 = { version = "0.8", package = "rand", default-features = false } | |
| rand_08 = { version = "0.8", package = "rand", default-features = false, features = ["getrandom"] } |
🤖 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 `@src/big_repo/Cargo.toml` at line 45, The rand_08 dependency is disabling
default features, which drops the getrandom support needed by
rand_08::rngs::OsRng. Update the rand_08 entry in Cargo.toml to re-enable the
appropriate feature so OsRng can compile, keeping the change localized to the
rand_08 dependency specification.
Comment on lines
+189
to
+191
| if let Err(e) = keyhive_protocol.ingest_from_storage().await { | ||
| tracing::warn!(error = %e, "keyhive ingest_from_storage failed"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail boot when keyhive storage recovery fails.
Continuing after ingest_from_storage() fails boots BigRepo with incomplete keyhive ACL history. Since boot_keyhive already returns Res, surface this as a boot error.
Suggested fix
- if let Err(e) = keyhive_protocol.ingest_from_storage().await {
- tracing::warn!(error = %e, "keyhive ingest_from_storage failed");
- }
+ keyhive_protocol
+ .ingest_from_storage()
+ .await
+ .map_err(|e| crate::ferr!("keyhive ingest_from_storage failed: {e}"))?;As per coding guidelines, Rust code should not swallow errors with catch-and-log unless explicitly requested.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Err(e) = keyhive_protocol.ingest_from_storage().await { | |
| tracing::warn!(error = %e, "keyhive ingest_from_storage failed"); | |
| } | |
| keyhive_protocol | |
| .ingest_from_storage() | |
| .await | |
| .map_err(|e| crate::ferr!("keyhive ingest_from_storage failed: {e}"))?; |
🤖 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 `@src/big_repo/handler.rs` around lines 189 - 191, The boot flow in
boot_keyhive should not ignore ingest_from_storage() failures; it currently logs
a warning in keyhive_protocol.ingest_from_storage() handling and then continues
booting. Change this branch to propagate the error back through boot_keyhive’s
Res result instead of swallowing it, so BigRepo fails to start when keyhive
storage recovery cannot complete. Use the existing boot_keyhive and
ingest_from_storage symbols to locate the error path and keep the return value
consistent with the rest of the boot sequence.
Source: Coding guidelines
Comment on lines
+75
to
+76
| tokio::fs::write(&tmp, data).await?; | ||
| match tokio::fs::rename(&tmp, &dest).await { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Flush keyhive state before publishing the rename.
Line 75 writes the temp file and Line 76 immediately renames it. That makes the name swap atomic, but not crash-durable; a crash can leave a missing or incomplete archive/op while BigRepo later believes the keyhive checkpoint was persisted.
Suggested direction
+ use tokio::io::AsyncWriteExt;
+
- tokio::fs::write(&tmp, data).await?;
+ let mut file = tokio::fs::File::create(&tmp).await?;
+ file.write_all(&data).await?;
+ file.sync_all().await?;
+ drop(file);
match tokio::fs::rename(&tmp, &dest).await {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tokio::fs::write(&tmp, data).await?; | |
| match tokio::fs::rename(&tmp, &dest).await { | |
| use tokio::io::AsyncWriteExt; | |
| let mut file = tokio::fs::File::create(&tmp).await?; | |
| file.write_all(&data).await?; | |
| file.sync_all().await?; | |
| drop(file); | |
| match tokio::fs::rename(&tmp, &dest).await { |
🤖 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 `@src/big_repo/keyhive_storage.rs` around lines 75 - 76, Flush the temp keyhive
file to stable storage before calling the rename in the keyhive storage write
path so the checkpoint is crash-durable, not just atomically swapped. Update the
logic around the tokio::fs::write and tokio::fs::rename sequence in the keyhive
storage operation to explicitly sync the written file (and any required parent
directory metadata) before publishing the final name, preserving the existing
temp-file-then-rename flow.
Comment on lines
+240
to
+243
| pub fn to_multikey(&self) -> [u8; 34] { | ||
| let mut buf = [0; 34]; | ||
| buf.copy_from_slice(&Self::MULTIKEY_PREFIX); | ||
| buf[2..].copy_from_slice(self.0.as_bytes()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify available multibase/base58 helpers and PubKey round-trip implementation points.
rg -n "encode_base58_multibase|decode.*base58|struct PubKey|impl std::str::FromStr for PubKey|to_multikey" src/utils_rs src/daybook_core/crypto.rsRepository: dman-os/townframe
Length of output: 1446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '225,320p' src/daybook_core/crypto.rs
printf '\n--- utils ---\n'
sed -n '455,470p' src/utils_rs/lib.rsRepository: dman-os/townframe
Length of output: 3492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('src/daybook_core/crypto.rs').read_text()
for needle in ['MULTIKEY_PREFIX', 'to_multikey', 'FromStr for PubKey', 'Display for PubKey']:
print(f'-- {needle} --')
idx = text.find(needle)
print(text[max(0, idx-200):idx+900] if idx != -1 else 'not found')
PYRepository: dman-os/townframe
Length of output: 4649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '470,490p' src/utils_rs/lib.rsRepository: dman-os/townframe
Length of output: 887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('src/utils_rs/lib.rs').read_text().splitlines()
for i in range(465, 490):
if i <= len(text):
print(f"{i}:{text[i-1]}")
PYRepository: dman-os/townframe
Length of output: 1114
Fix the multikey offsets and parsing.
to_multikey()copies the 2-byte prefix into the full 34-byte buffer and panics on every call; it should write intobuf[..2].FromStrmust strip the multibasezprefix before base58 decoding. As written,Displayoutput does not round-trip.
🤖 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 `@src/daybook_core/crypto.rs` around lines 240 - 243, Update the multikey
serialization/parsing in crypto.rs: in `to_multikey`, write
`Self::MULTIKEY_PREFIX` into the first 2 bytes of the 34-byte buffer and keep
the UUID bytes in the remaining slice so it no longer panics; then fix `FromStr`
to remove the multibase `z` prefix before base58 decoding so it correctly
round-trips with `Display`. Use the existing `to_multikey`, `FromStr`, and
`Display` implementations as the entry points for the change.
Comment on lines
+326
to
+341
| fn visit_bytes<E>(self, val: &[u8]) -> Result<Self::Value, E> | ||
| where | ||
| E: serde::de::Error, | ||
| { | ||
| if val.len() != 34 { | ||
| return Err(serde::de::Error::invalid_length( | ||
| val.len(), | ||
| &"34 length byte array", | ||
| )); | ||
| } | ||
| if &val[..2] != &PubKey::MULTIKEY_PREFIX[..] { | ||
| return Err(serde::de::Error::custom("valid multikey prefix xED01_u16")); | ||
| } | ||
| let mut buf = [0u8; 32]; | ||
| buf.copy_from_slice(val); | ||
| Ok(buf) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Copy only the raw key bytes after validating the binary multikey prefix.
The visitor validates a 34-byte multikey but then copies all 34 bytes into a [u8; 32], which panics for valid binary input.
Proposed fix
if &val[..2] != &PubKey::MULTIKEY_PREFIX[..] {
return Err(serde::de::Error::custom("valid multikey prefix xED01_u16"));
}
let mut buf = [0u8; 32];
- buf.copy_from_slice(val);
+ buf.copy_from_slice(&val[2..]);
Ok(buf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn visit_bytes<E>(self, val: &[u8]) -> Result<Self::Value, E> | |
| where | |
| E: serde::de::Error, | |
| { | |
| if val.len() != 34 { | |
| return Err(serde::de::Error::invalid_length( | |
| val.len(), | |
| &"34 length byte array", | |
| )); | |
| } | |
| if &val[..2] != &PubKey::MULTIKEY_PREFIX[..] { | |
| return Err(serde::de::Error::custom("valid multikey prefix xED01_u16")); | |
| } | |
| let mut buf = [0u8; 32]; | |
| buf.copy_from_slice(val); | |
| Ok(buf) | |
| fn visit_bytes<E>(self, val: &[u8]) -> Result<Self::Value, E> | |
| where | |
| E: serde::de::Error, | |
| { | |
| if val.len() != 34 { | |
| return Err(serde::de::Error::invalid_length( | |
| val.len(), | |
| &"34 length byte array", | |
| )); | |
| } | |
| if &val[..2] != &PubKey::MULTIKEY_PREFIX[..] { | |
| return Err(serde::de::Error::custom("valid multikey prefix xED01_u16")); | |
| } | |
| let mut buf = [0u8; 32]; | |
| buf.copy_from_slice(&val[2..]); | |
| Ok(buf) |
🤖 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 `@src/daybook_core/crypto.rs` around lines 326 - 341, The binary multikey
visitor in `visit_bytes` currently validates the 34-byte input and
`PubKey::MULTIKEY_PREFIX`, but then copies the entire 34-byte slice into a
32-byte buffer, which will panic on valid input. Update `visit_bytes` in the
`crypto` deserializer to copy only the raw key payload after the prefix (the 32
bytes following `PubKey::MULTIKEY_PREFIX`) into the `[u8; 32]` buffer, keeping
the existing prefix and length validation intact.
Comment on lines
+353
to
+380
| fn reconcile<R: autosurgeon::Reconciler>(&self, mut reconciler: R) -> Result<(), R::Error> { | ||
| reconciler.bytes(self.0) | ||
| } | ||
| } | ||
|
|
||
| impl autosurgeon::Hydrate for PubKey { | ||
| fn hydrate_bytes(bytes: &[u8]) -> Result<Self, autosurgeon::HydrateError> { | ||
| if bytes.len() != 32 { | ||
| return Err(autosurgeon::HydrateError::unexpected( | ||
| "PubKey in 34 length multikey byte array", | ||
| format!("bytestring has byte length of {}", bytes.len()), | ||
| )); | ||
| } | ||
| if &bytes[..2] != &Self::MULTIKEY_PREFIX[..] { | ||
| return Err(autosurgeon::HydrateError::unexpected( | ||
| "valid multikey prefix xED01_u16", | ||
| "not the right prefix".into(), | ||
| )); | ||
| } | ||
| let mut buf = [0_u8; 32]; | ||
| buf.copy_from_slice(&bytes[2..]); | ||
| Self::new(&buf).map_err(|err| { | ||
| autosurgeon::HydrateError::unexpected( | ||
| "valid multikey encoded ed25519_dalek public key", | ||
| format!("invalid pub key: {err}"), | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect PubKey autosurgeon codec consistency.
rg -n "impl autosurgeon::Reconcile for PubKey|impl autosurgeon::Hydrate for PubKey|reconciler.bytes|hydrate_bytes" src/daybook_core/crypto.rsRepository: dman-os/townframe
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,410p' src/daybook_core/crypto.rs | cat -nRepository: dman-os/townframe
Length of output: 2854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/daybook_core/crypto.rs | cat -nRepository: dman-os/townframe
Length of output: 4603
Use the same 34-byte multikey encoding in both directions. reconciler.bytes(self.0) writes raw 32-byte keys, while hydrate_bytes expects multikey bytes but only accepts length 32 before slicing bytes[2..], so hydrating the current payload panics. Switch reconcile to to_multikey() and update hydrate to accept 34 bytes.
🤖 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 `@src/daybook_core/crypto.rs` around lines 353 - 380, The PubKey autosurgeon
round-trip is inconsistent: `reconcile` currently emits raw 32-byte key material
via `reconciler.bytes(self.0)`, while `hydrate_bytes` is written for
multikey-encoded input but only accepts length 32 before slicing the prefix.
Update `PubKey::reconcile` to serialize using the same multikey format returned
by `to_multikey()`, and change `PubKey::hydrate_bytes` to expect 34 bytes,
validate the `MULTIKEY_PREFIX`, then copy only the final 32-byte key payload
before calling `Self::new`.
Source: Learnings
|
|
||
| pub mod blobs; | ||
| pub mod config; | ||
| pub mod crypto; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid publicly exporting incomplete crypto APIs.
crypto.rs exposes AmStore::prop() implementations that still call todo!(), so consumers of this new public module can hit runtime panics through the store-loading path. Implement the store props before exporting, or keep the module private until it is complete.
🤖 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 `@src/daybook_core/lib.rs` at line 31, The public crypto module export is
exposing unfinished store-loading APIs, so update the crate boundary in lib.rs
and the crypto module to avoid publishing incomplete functionality. Either make
AmStore::prop() and any related store props fully implemented before keeping
crypto public, or keep crypto private / remove the public re-export until those
todo!() paths are eliminated. Use the crypto module, AmStore, and prop() symbols
to locate the affected API surface.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/daybook_core/sync/bootstrap.rs (1)
341-353: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid a single
Readycheck here
wait_for_full_synconly guarantees partition replication;big_repo::get_doccan still returnPendingMaterializationuntil keyhive/plaintext reconstruction catches up. Use a bounded retry loop for the app and drawer docs instead of failing immediately on the first non-Readylookup.🤖 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 `@src/daybook_core/sync/bootstrap.rs` around lines 341 - 353, The current app/drawer presence check in the bootstrap sync path fails too early by treating the first non-Ready lookup from big_repo::get_doc as missing. Update the logic in wait_for_full_sync/bootstrap flow to retry the app_doc_id and drawer_doc_id lookups in a bounded loop until they become Ready or a timeout is reached, instead of bailing immediately on PendingMaterialization. Use the existing big_repo::get_doc results and the bootstrap app_doc_id/drawer_doc_id symbols to keep the retry scoped to these two core docs.src/big_repo/backend.rs (1)
4-18: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix the malformed
BigRepoSyncBackendstruct
Line 6 has a type with no field name, so this brace struct does not parse.bootalso initializes onlyrepo, so the extra field must be removed or named and initialized.🤖 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 `@src/big_repo/backend.rs` around lines 4 - 18, The BigRepoSyncBackend definition is malformed because it contains a struct field type without a field name, so the brace struct cannot parse. Fix the BigRepoSyncBackend declaration by either removing the stray Arc<surelock::mutex::Mutex<std::collections::HashMap<PeerId, Arc<RepoRpcClient>>>> entry or giving it a proper field name, and make sure BigRepoSyncBackend::boot initializes every remaining field consistently.
🧹 Nitpick comments (5)
src/daybook_core/drawer.rs (1)
3-3: 🎯 Functional Correctness | 🔵 TrivialUnresolved FIXME: potential race in facet-reference validation.
This flags a real TOCTOU-style concern:
validate_facets/validate_facet_referenceread referenced-doc state at validation time, but the reference could be invalidated by a concurrent write before this doc's commit lands. Since the FIXME already acknowledges this is unresolved, want me to sketch an approach (e.g., re-validating at commit time or optimistic-concurrency check) or open a tracking issue?🤖 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 `@src/daybook_core/drawer.rs` at line 3, The FIXME in the facet-reference validation path highlights a TOCTOU race: `validate_facets` and `validate_facet_reference` can pass based on stale referenced-doc state before commit. Update the `drawer` write flow so facet references are rechecked at commit time or guarded with an optimistic-concurrency/version check, and keep validation coupled to the final commit state rather than only entry-time reads.src/daybook_core/drawer/tests.rs (1)
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for repeated drawer-doc creation boilerplate.
The identical 5-line "create version-0 automerge doc,
create_doc, grabdocument_id()" block is duplicated across ~20 test functions in this file. Since this PR already touches every one of these sites (switchingput_doc→create_doc), consider extracting a shared helper, e.g.:♻️ Proposed helper
async fn new_versioned_drawer_doc_id(big_repo: &BigRepo) -> Res<DocumentId> { let mut doc = automerge::Automerge::new(); let mut tx = doc.transaction(); tx.put(automerge::ROOT, "version", "0")?; tx.commit(); Ok(big_repo.create_doc(doc).await?.document_id()) }Then each call site becomes
let drawer_doc_id = new_versioned_drawer_doc_id(&big_repo).await?;As per coding guidelines, "When working on tests, take a gardening hand to it and proactively improve them... Are tests repeating too much setup code? Consider using TDD or common cleanup code."
Also applies to: 147-154, 266-273, 333-340, 434-441, 579-586, 653-660, 990-997, 1170-1177, 1409-1416, 1487-1494, 1599-1606, 1774-1781, 1981-1988, 2135-2142, 2211-2218, 2293-2300, 2369-2376, 2415-2422, 2471-2478, 2642-2649
🤖 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 `@src/daybook_core/drawer/tests.rs` around lines 52 - 59, The drawer tests repeat the same version-0 Automerge document setup and create_doc/document_id boilerplate in many places, so extract it into a shared async helper in this test module. Add a helper like new_versioned_drawer_doc_id(BigRepo) that builds the doc, sets version to 0, calls create_doc, and returns document_id(), then replace each repeated drawer_doc_id block in the affected test functions with a call to that helper.Source: Coding guidelines
src/big_repo/encrypted_blob.rs (1)
14-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the full ciphertext to encode a blob envelope.
encode_encrypted_blobclones the entireEncryptedContent(including itsVec<u8>ciphertext) just to move it into the envelope for serialization. Since this is meant for blob-sized payloads, this copy can be significant. A borrowing envelope avoids the allocation:♻️ Avoid the clone via a reference-based envelope
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -struct EncryptedBlobEnvelope { - #[serde(flatten)] - encrypted: EncryptedContent<Vec<u8>, Vec<u8>>, -} +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +struct EncryptedBlobEnvelope { + #[serde(flatten)] + encrypted: EncryptedContent<Vec<u8>, Vec<u8>>, +} + +#[derive(Serialize)] +struct EncryptedBlobEnvelopeRef<'a> { + #[serde(flatten)] + encrypted: &'a EncryptedContent<Vec<u8>, Vec<u8>>, +} pub(crate) fn encode_encrypted_blob( encrypted: &EncryptedContent<Vec<u8>, Vec<u8>>, ) -> Res<Vec<u8>> { - let env = EncryptedBlobEnvelope { - encrypted: encrypted.clone(), - }; + let env = EncryptedBlobEnvelopeRef { encrypted }; let mut encoded = vec![ENCRYPTED_BLOB_DISCRIMINATOR]; drisl::to_writer(&mut encoded, &env) .map_err(|err| ferr!("DRISL encode encrypted blob envelope: {err}"))?; Ok(encoded) }🤖 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 `@src/big_repo/encrypted_blob.rs` around lines 14 - 24, `encode_encrypted_blob` is cloning the entire `EncryptedContent` just to serialize it, which adds an unnecessary copy for large blob payloads. Update the serialization path to use a reference-based envelope instead of owning `EncryptedBlobEnvelope` with `encrypted.clone()`, so `encode_encrypted_blob` can pass a borrowed `EncryptedContent` into `drisl::to_writer` without allocating or duplicating the ciphertext.src/daybook_core/plugs.rs (1)
2107-2128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative test for wrong
exportvalue.The sibling branch (missing wflow bundle) has
test_view_provider_bundle_must_exist, but there's no test asserting that aStatelessWasmview withexport != "render-facet-view"is rejected. Worth adding to lock in this new validation path.🤖 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 `@src/daybook_core/plugs.rs` around lines 2107 - 2128, Add a negative test for the new export validation in the view validation path: extend the existing tests around `manifest::ViewProviderManifest::StatelessWasm` and `validate_manifest`/`plugs.rs` so a view with a valid bundle but an `export` other than `render-facet-view` is rejected. Mirror the style of `test_view_provider_bundle_must_exist`, and assert the error from the `render_facet_view`/view validation logic clearly mentions the invalid export and supported entrypoint.src/big_repo/backend.rs (1)
34-49: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: fold the
obj_existsprobe intopartition_doc_heads_payload.The
FIXMEat line 35 notes the extraobj_existsround-trip could be avoided by returning existence + heads from a single query. Want me to open a tracking issue?🤖 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 `@src/big_repo/backend.rs` around lines 34 - 49, The extra obj_exists probe in the local_heads handling is a separate round-trip that should be eliminated by folding existence into the partition_doc_heads_payload path. Update the logic in backend::sync_doc_with_peer flow so the query returns both local doc existence and heads together, then use that single result instead of calling repo.big_sync_store.obj_exists(doc_id) before sync_doc_with_peer. Keep the behavior around NotFound/Unauthorized/PolicyRejected mapping unchanged.
🤖 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 `@README.md`:
- Around line 28-50: The README section needs a small copy edit: fix the typo in
the list item that currently says “soforth” and hyphenate “state of the art” in
the Emacs/Vim bullet. Update the prose in the affected markdown block only,
keeping the existing meaning and style consistent with the surrounding text.
In `@src/big_repo/ephemeral.rs`:
- Around line 155-217: subscribe() is not cancel-safe:
BigEphemeralSwitchboardCmd::Register updates state before ack_rx.await, so
dropping the future before the ack can leave a listener registered with no
Unregister path. Update BigEphemeralSubscription::subscribe and the Register
handling flow so cancellation between registration and ack either cleans up the
listener immediately or performs registration atomically/cancel-safely; use the
ack_tx.send(() ) result as the signal to roll back state if the subscriber is
gone.
In `@src/big_repo/keyhive.rs`:
- Around line 389-396: The create_doc_id_bytes method in BigRepoKeyhiveStorage
is an incomplete stub: it returns an undefined doc_id and ignores all
parameters, so it will not compile. Either implement the full doc ID
construction using coparents, initial_content_heads, and storage, or remove the
unused method entirely if it is not meant to exist. Make sure the final
implementation in create_doc_id_bytes returns a real [u8; 32] value and does not
reference undefined symbols.
- Around line 302-305: The initializer in the code path around `let (id,
delegations)` is using `id.group_id()` before the `id` binding exists; change it
to read the group id from the already-bound `locked` value instead. Also fix
`create_doc_id_bytes`, where `Ok(doc_id)` is returned without any `doc_id` being
created; make sure that function binds the generated document id before
returning it so the file compiles cleanly.
In `@src/big_sync/stress_support.rs`:
- Around line 442-445: The settle-wait path in wait_for_cluster_settled always
exceeds the generic slow-operation threshold, so the log_if_slow warning is
effectively guaranteed and no longer useful. Update the wait_for_cluster_settled
success/timeout logging to use a settle-specific threshold (or skip slow logging
entirely there) while keeping the existing log_if_slow behavior for other paths,
and adjust the call sites/conditions around stable_rounds and the timeout branch
accordingly.
In `@src/daybook_core/repo.rs`:
- Around line 803-812: Update the drawer doc setup in
repo::daybook_core::repo.rs so the load error message matches what is actually
happening: the AutoCommit is being saved and reloaded from fresh version "0",
not from version_latest, so replace that misleading wrap_err text with a
drawer-doc-specific message. Also adjust the big_repo.create_doc call to
preserve the CreateDocError source chain instead of flattening it through
eyre::eyre!("{err}"), using a conversion that keeps the original error type in
the report.
---
Outside diff comments:
In `@src/big_repo/backend.rs`:
- Around line 4-18: The BigRepoSyncBackend definition is malformed because it
contains a struct field type without a field name, so the brace struct cannot
parse. Fix the BigRepoSyncBackend declaration by either removing the stray
Arc<surelock::mutex::Mutex<std::collections::HashMap<PeerId,
Arc<RepoRpcClient>>>> entry or giving it a proper field name, and make sure
BigRepoSyncBackend::boot initializes every remaining field consistently.
In `@src/daybook_core/sync/bootstrap.rs`:
- Around line 341-353: The current app/drawer presence check in the bootstrap
sync path fails too early by treating the first non-Ready lookup from
big_repo::get_doc as missing. Update the logic in wait_for_full_sync/bootstrap
flow to retry the app_doc_id and drawer_doc_id lookups in a bounded loop until
they become Ready or a timeout is reached, instead of bailing immediately on
PendingMaterialization. Use the existing big_repo::get_doc results and the
bootstrap app_doc_id/drawer_doc_id symbols to keep the retry scoped to these two
core docs.
---
Nitpick comments:
In `@src/big_repo/backend.rs`:
- Around line 34-49: The extra obj_exists probe in the local_heads handling is a
separate round-trip that should be eliminated by folding existence into the
partition_doc_heads_payload path. Update the logic in
backend::sync_doc_with_peer flow so the query returns both local doc existence
and heads together, then use that single result instead of calling
repo.big_sync_store.obj_exists(doc_id) before sync_doc_with_peer. Keep the
behavior around NotFound/Unauthorized/PolicyRejected mapping unchanged.
In `@src/big_repo/encrypted_blob.rs`:
- Around line 14-24: `encode_encrypted_blob` is cloning the entire
`EncryptedContent` just to serialize it, which adds an unnecessary copy for
large blob payloads. Update the serialization path to use a reference-based
envelope instead of owning `EncryptedBlobEnvelope` with `encrypted.clone()`, so
`encode_encrypted_blob` can pass a borrowed `EncryptedContent` into
`drisl::to_writer` without allocating or duplicating the ciphertext.
In `@src/daybook_core/drawer.rs`:
- Line 3: The FIXME in the facet-reference validation path highlights a TOCTOU
race: `validate_facets` and `validate_facet_reference` can pass based on stale
referenced-doc state before commit. Update the `drawer` write flow so facet
references are rechecked at commit time or guarded with an
optimistic-concurrency/version check, and keep validation coupled to the final
commit state rather than only entry-time reads.
In `@src/daybook_core/drawer/tests.rs`:
- Around line 52-59: The drawer tests repeat the same version-0 Automerge
document setup and create_doc/document_id boilerplate in many places, so extract
it into a shared async helper in this test module. Add a helper like
new_versioned_drawer_doc_id(BigRepo) that builds the doc, sets version to 0,
calls create_doc, and returns document_id(), then replace each repeated
drawer_doc_id block in the affected test functions with a call to that helper.
In `@src/daybook_core/plugs.rs`:
- Around line 2107-2128: Add a negative test for the new export validation in
the view validation path: extend the existing tests around
`manifest::ViewProviderManifest::StatelessWasm` and
`validate_manifest`/`plugs.rs` so a view with a valid bundle but an `export`
other than `render-facet-view` is rejected. Mirror the style of
`test_view_provider_bundle_must_exist`, and assert the error from the
`render_facet_view`/view validation logic clearly mentions the invalid export
and supported entrypoint.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fea0bf3-865d-483d-bc21-6651dac34281
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockand included by**/*docs/DEVDOC/log.mdis excluded by!docs/DEVDOC/**and included by**/*src/daybook_core/gen/mod.rsis excluded by!**/gen/**and included by**/*
📒 Files selected for processing (51)
Cargo.tomlREADME.mdsrc/big_repo/Cargo.tomlsrc/big_repo/backend.rssrc/big_repo/changes.rssrc/big_repo/encrypted_blob.rssrc/big_repo/ephemeral.rssrc/big_repo/handler.rssrc/big_repo/keyhive.rssrc/big_repo/keyhive_conn.rssrc/big_repo/keyhive_storage.rssrc/big_repo/lib.rssrc/big_repo/rpc.rssrc/big_repo/runtime.rssrc/big_repo/test.rssrc/big_repo/wire.rssrc/big_sync/part_store.rssrc/big_sync/part_store/memory.rssrc/big_sync/part_store/sqlite.rssrc/big_sync/stress_support.rssrc/big_sync_core/ids.rssrc/big_sync_core/lib.rssrc/daybook_core/Cargo.tomlsrc/daybook_core/blobs.rssrc/daybook_core/config.rssrc/daybook_core/crypto.rssrc/daybook_core/drawer.rssrc/daybook_core/drawer/mutations.rssrc/daybook_core/drawer/tests.rssrc/daybook_core/index/doc_blobs.rssrc/daybook_core/keyhive.rssrc/daybook_core/lib.rssrc/daybook_core/local_state.rssrc/daybook_core/plugs.rssrc/daybook_core/repo.rssrc/daybook_core/rt.rssrc/daybook_core/rt/dispatch.rssrc/daybook_core/rt/init.rssrc/daybook_core/rt/triage.rssrc/daybook_core/sync/bootstrap.rssrc/daybook_core/tables.rssrc/daybook_core/test_support.rssrc/daybook_types/doc.rssrc/utils_rs/Cargo.tomlsrc/utils_rs/lib.rssrc/utils_rs/testing.rssrc/xtask/Cargo.tomlsrc/xtask/keyhive_demo.rssrc/xtask/main.rsx/clean-rust.tsx/utils.ts
💤 Files with no reviewable changes (10)
- src/utils_rs/testing.rs
- x/clean-rust.ts
- src/xtask/main.rs
- src/utils_rs/Cargo.toml
- x/utils.ts
- src/big_repo/rpc.rs
- src/daybook_types/doc.rs
- src/xtask/Cargo.toml
- src/utils_rs/lib.rs
- src/xtask/keyhive_demo.rs
✅ Files skipped from review due to trivial changes (2)
- src/daybook_core/local_state.rs
- src/daybook_core/index/doc_blobs.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/daybook_core/keyhive.rs
- src/daybook_core/blobs.rs
- src/big_sync_core/ids.rs
- src/daybook_core/rt.rs
- src/daybook_core/rt/triage.rs
- src/daybook_core/Cargo.toml
- src/big_repo/keyhive_conn.rs
- src/daybook_core/drawer/mutations.rs
- Cargo.toml
- src/daybook_core/lib.rs
- src/big_repo/changes.rs
Comment on lines
+28
to
+50
| Either from scratch as is foolishly done here or by customizing an existing platform like Obsidian, Emacs, Notion and soforth. | ||
|
|
||
| Specifically, daybook design is informed by tech I find shiny, gaps I see in the current landscape for such solutions and my personal politics. | ||
| Specifically, daybook design is informed by tech I find shiny, gaps I see in the current landscape and my personal politics. | ||
|
|
||
| - Shiny tech | ||
| - Everything that is and around LLMs. | ||
| - Uses Automerge for an offline and local-first experience. | ||
| - Uses Compose Multiplatform for a reliable experience on Android and Desktop. | ||
| - Uses Compose Multiplatform for a reliable experience on Android and desktop. | ||
| - Leverage WASM for plugins. | ||
| - Gaps | ||
| - Emacs/Vim | ||
| - State of the art editing experience | ||
| - Pre-mobile tech | ||
| - Notion | ||
| - Excellent design | ||
| - Proprietary | ||
| - Questionable performance on Android | ||
| - Obsidian | ||
| - Excellent sensibilities | ||
| - Questionable performance on Android | ||
| - The plugins seem insecure | ||
| - Collaboration is secondary? | ||
| - Proprietary | ||
| - Questionable performance on Android |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the copy in this section.
soforth is a typo, and state of the art should be hyphenated here.
Suggested copy edit
- soforth
+ so forth
- State of the art
+ State-of-the-art📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Either from scratch as is foolishly done here or by customizing an existing platform like Obsidian, Emacs, Notion and soforth. | |
| Specifically, daybook design is informed by tech I find shiny, gaps I see in the current landscape for such solutions and my personal politics. | |
| Specifically, daybook design is informed by tech I find shiny, gaps I see in the current landscape and my personal politics. | |
| - Shiny tech | |
| - Everything that is and around LLMs. | |
| - Uses Automerge for an offline and local-first experience. | |
| - Uses Compose Multiplatform for a reliable experience on Android and Desktop. | |
| - Uses Compose Multiplatform for a reliable experience on Android and desktop. | |
| - Leverage WASM for plugins. | |
| - Gaps | |
| - Emacs/Vim | |
| - State of the art editing experience | |
| - Pre-mobile tech | |
| - Notion | |
| - Excellent design | |
| - Proprietary | |
| - Questionable performance on Android | |
| - Obsidian | |
| - Excellent sensibilities | |
| - Questionable performance on Android | |
| - The plugins seem insecure | |
| - Collaboration is secondary? | |
| - Proprietary | |
| - Questionable performance on Android | |
| Either from scratch as is foolishly done here or by customizing an existing platform like Obsidian, Emacs, Notion and so forth. | |
| Specifically, daybook design is informed by tech I find shiny, gaps I see in the current landscape and my personal politics. | |
| - Shiny tech | |
| - Everything that is and around LLMs. | |
| - Uses Automerge for an offline and local-first experience. | |
| - Uses Compose Multiplatform for a reliable experience on Android and desktop. | |
| - Leverage WASM for plugins. | |
| - Gaps | |
| - Emacs/Vim | |
| - State-of-the-art editing experience | |
| - Pre-mobile tech | |
| - Notion | |
| - Excellent design | |
| - Proprietary | |
| - Questionable performance on Android | |
| - Obsidian | |
| - Excellent sensibilities | |
| - The plugins seem insecure | |
| - Collaboration is secondary? | |
| - Proprietary | |
| - Questionable performance on Android |
🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: Ensure spelling is correct
Context: ...atform like Obsidian, Emacs, Notion and soforth. Specifically, daybook design is informed...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~39-~39: Use a hyphen to join words.
Context: ... - Gaps - Emacs/Vim - State of the art editing experience - Pre-mobile ...
(QB_NEW_EN_HYPHEN)
🤖 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 `@README.md` around lines 28 - 50, The README section needs a small copy edit:
fix the typo in the list item that currently says “soforth” and hyphenate “state
of the art” in the Emacs/Vim bullet. Update the prose in the affected markdown
block only, keeping the existing meaning and style consistent with the
surrounding text.
Source: Linters/SAST tools
Comment on lines
+155
to
+217
| cmd = cmd_rx.recv(), if !cmd_closed => { | ||
| match cmd { | ||
| Some(BigEphemeralSwitchboardCmd::Register { | ||
| subscription_id, | ||
| filter, | ||
| event_tx, | ||
| ack_tx, | ||
| }) => { | ||
| state.register_listener(&backend, subscription_id, filter, event_tx).await?; | ||
| ack_tx.send(()).ok(); | ||
| } | ||
| Some(BigEphemeralSwitchboardCmd::Unregister { subscription_id }) => { | ||
| state.unregister_listener(&backend, subscription_id).await?; | ||
| } | ||
| None => { | ||
| cmd_closed = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| eyre::Ok(()) | ||
| }; | ||
| runtime_tasks | ||
| .spawn({ | ||
| let stop = runtime_stop.child_token(); | ||
| async move { | ||
| if let Some(Err(err)) = stop.run_until_cancelled(fut).await { | ||
| panic!("{:?}", err); | ||
| } | ||
| } | ||
| .instrument(tracing::info_span!("BigEphemeralSwitchboard")) | ||
| }) | ||
| .expect(ERROR_TOKIO); | ||
|
|
||
| Self { | ||
| cmd_tx, | ||
| next_subscription_id, | ||
| } | ||
| } | ||
|
|
||
| pub async fn subscribe(&self, filter: BigEphemeralFilter) -> Res<BigEphemeralSubscription> { | ||
| let subscription_id = self.next_subscription_id.fetch_add(1, Ordering::Relaxed); | ||
| let (event_tx, event_rx) = mpsc::unbounded_channel(); | ||
| let (ack_tx, ack_rx) = oneshot::channel(); | ||
|
|
||
| self.cmd_tx | ||
| .send(BigEphemeralSwitchboardCmd::Register { | ||
| subscription_id, | ||
| filter, | ||
| event_tx, | ||
| ack_tx, | ||
| }) | ||
| .map_err(|_| eyre::eyre!(ERROR_ACTOR))?; | ||
|
|
||
| ack_rx.await.map_err(|_| eyre::eyre!(ERROR_CHANNEL))?; | ||
|
|
||
| Ok(BigEphemeralSubscription { | ||
| subscription_id, | ||
| cmd_tx: self.cmd_tx.clone(), | ||
| event_rx, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline src/big_repo/ephemeral.rs --view expanded || true
echo
echo "== Relevant lines around subscribe/ack =="
sed -n '130,240p' src/big_repo/ephemeral.rs
echo
echo "== Search for subscribe() call sites =="
rg -n "subscribe\(" src . --glob '!target' --glob '!node_modules' || true
echo
echo "== Search for timeout/select around BigEphemeral::subscribe or BigEphemeralSwitchboard::subscribe =="
rg -n "BigEphemeral::subscribe|BigEphemeralSwitchboard::subscribe|tokio::select!|timeout\(" src . --glob '!target' --glob '!node_modules' || true
echo
echo "== Search for Option::is_none_or usage and edition/MSRV clues =="
rg -n "is_none_or|rust-version|edition" Cargo.toml . --glob '!target' --glob '!node_modules' || trueRepository: dman-os/townframe
Length of output: 43946
subscribe() can leak a listener if the caller drops the future before the ack arrives.
Register is applied before ack_rx.await, but BigEphemeralSubscription (the only Unregister path) is only created after that await. If the future is cancelled in between, the listener and topic refcount stay alive indefinitely. The discarded ack_tx.send(()) error is the only signal that this happened; it should drive cleanup or be guarded by a cancel-safe registration flow.
🤖 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 `@src/big_repo/ephemeral.rs` around lines 155 - 217, subscribe() is not
cancel-safe: BigEphemeralSwitchboardCmd::Register updates state before
ack_rx.await, so dropping the future before the ack can leave a listener
registered with no Unregister path. Update BigEphemeralSubscription::subscribe
and the Register handling flow so cancellation between registration and ack
either cleans up the listener immediately or performs registration
atomically/cancel-safely; use the ack_tx.send(() ) result as the signal to roll
back state if the subscriber is gone.
Source: Coding guidelines
Comment on lines
442
to
445
| if stable_rounds >= 100 { | ||
| log_if_slow(label, started_at); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
log_if_slow will always warn for settle waits, defeating its purpose.
wait_for_cluster_settled only returns success after stable_rounds >= 100, and each round sleeps 50ms, so the operation takes ≥~5s by design. With SLOW_OP_LOG_THRESHOLD = 250ms, the warn at Line 443 (and 452 on timeout) always fires. Consider a separate, larger threshold for settle waits (or drop the slow-log there) so the warning stays meaningful.
🤖 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 `@src/big_sync/stress_support.rs` around lines 442 - 445, The settle-wait path
in wait_for_cluster_settled always exceeds the generic slow-operation threshold,
so the log_if_slow warning is effectively guaranteed and no longer useful.
Update the wait_for_cluster_settled success/timeout logging to use a
settle-specific threshold (or skip slow logging entirely there) while keeping
the existing log_if_slow behavior for other paths, and adjust the call
sites/conditions around stable_rounds and the timeout branch accordingly.
Comment on lines
803
to
+812
| let drawer_doc = { | ||
| let mut doc = automerge::AutoCommit::new(); | ||
| doc.put(automerge::ROOT, "version", "0")?; | ||
| let bytes = doc.save_nocompress(); | ||
| let doc = automerge::Automerge::load(&bytes) | ||
| .wrap_err("error loading version_latest for drawer doc")?; | ||
| big_repo.put_doc(DocumentId::random(), doc).await? | ||
| big_repo | ||
| .create_doc(doc) | ||
| .await | ||
| .map_err(|err| eyre::eyre!("{err}"))? |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Misleading error context on the drawer doc load.
The drawer doc is created from a fresh AutoCommit (version "0"), not from version_latest, so "error loading version_latest for drawer doc" will mislead debugging.
Also consider preserving the CreateDocError source chain instead of flattening it with eyre::eyre!("{err}") (e.g. map_err(eyre::Report::from) if it implements std::error::Error).
🔧 Proposed fix
let drawer_doc = {
let mut doc = automerge::AutoCommit::new();
doc.put(automerge::ROOT, "version", "0")?;
let bytes = doc.save_nocompress();
let doc = automerge::Automerge::load(&bytes)
- .wrap_err("error loading version_latest for drawer doc")?;
+ .wrap_err("error loading initial drawer doc")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let drawer_doc = { | |
| let mut doc = automerge::AutoCommit::new(); | |
| doc.put(automerge::ROOT, "version", "0")?; | |
| let bytes = doc.save_nocompress(); | |
| let doc = automerge::Automerge::load(&bytes) | |
| .wrap_err("error loading version_latest for drawer doc")?; | |
| big_repo.put_doc(DocumentId::random(), doc).await? | |
| big_repo | |
| .create_doc(doc) | |
| .await | |
| .map_err(|err| eyre::eyre!("{err}"))? | |
| let drawer_doc = { | |
| let mut doc = automerge::AutoCommit::new(); | |
| doc.put(automerge::ROOT, "version", "0")?; | |
| let bytes = doc.save_nocompress(); | |
| let doc = automerge::Automerge::load(&bytes) | |
| .wrap_err("error loading initial drawer doc")?; | |
| big_repo | |
| .create_doc(doc) | |
| .await | |
| .map_err(|err| eyre::eyre!("{err}"))? |
🤖 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 `@src/daybook_core/repo.rs` around lines 803 - 812, Update the drawer doc setup
in repo::daybook_core::repo.rs so the load error message matches what is
actually happening: the AutoCommit is being saved and reloaded from fresh
version "0", not from version_latest, so replace that misleading wrap_err text
with a drawer-doc-specific message. Also adjust the big_repo.create_doc call to
preserve the CreateDocError source chain instead of flattening it through
eyre::eyre!("{err}"), using a conversion that keeps the original error type in
the report.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/big_sync_core/tasks/peer_replay.rs (1)
72-82: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftUse the local
PeerIdforSubPartsRequest.peer_id.self.peer_idis the remote source (cx.rpc_clients.get(&self.peer_id)), but the server treatsSubPartsRequest.peer_idas the subscriber identity when applying the readability filter. Passing the remote peer here makes it subscribe as itself and bypasses that filter on the replay path. Thread the local node identity intoTaskCtxand use that instead.🤖 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 `@src/big_sync_core/tasks/peer_replay.rs` around lines 72 - 82, Use the local PeerId for the SubPartsRequest.peer_id in peer_replay::PeerReplayTask::run instead of self.peer_id, since self.peer_id is only the remote source used by cx.rpc_clients.get(...). Thread the local node identity through TaskCtx so this method can reference it, and update the sub_parts request construction to pass that local identity while leaving the remote lookup unchanged.src/big_sync/part_store/sqlite.rs (1)
115-123: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLoad
doc_members_cachefrombig_sync_syncableon startup
new()starts with an empty cache, and there’s no read path that repopulates it. After a restart, persisted access levels are ignored untilset_doc_membersruns again, so the table is effectively write-only.🤖 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 `@src/big_sync/part_store/sqlite.rs` around lines 115 - 123, `SqlitePartStore::new` currently initializes `doc_members_cache` empty and never reloads persisted membership data, so access levels are lost after restart. Update the startup path in `new()` (or a helper it calls) to read the existing `big_sync_syncable` records from SQLite and populate `doc_members_cache` before returning `Self`, using the existing `sql`/`scope_id`/`bucket_depth` state so the cache reflects persisted values even before `set_doc_members` runs.
🧹 Nitpick comments (3)
src/big_sync/test.rs (1)
189-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest double discards
req.peer_id, diverging from production behavior.Production
BigSyncRpcWorker::handle_rpc_messagederives the subscriber frominner.peer_idand passes it tosubscribe. Here the memory RPC hardcodesPeerId::new([0u8; 32]), so this double never exercises the readability filter with the actual requester identity. Prefer forwardingreq.peer_idto mirror production.♻️ Proposed change
- let receiver = self - .target_part_store - .subscribe(req, PeerId::new([0u8; 32])) - .await??; + let subscriber = req.peer_id; + let receiver = self + .target_part_store + .subscribe(req, subscriber) + .await??;🤖 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 `@src/big_sync/test.rs` around lines 189 - 192, The test double in `src/big_sync/test.rs` is hardcoding the subscriber identity instead of using the request’s peer identity, so it no longer matches `BigSyncRpcWorker::handle_rpc_message` behavior. Update the memory RPC path to forward `req.peer_id` into `target_part_store.subscribe` (as production does via `inner.peer_id`) so the readability filter is exercised with the actual requester identity.src/big_sync/part_store/memory.rs (1)
1186-1210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFilter tests don't assert the negative case, so they'd pass even if filtering were a no-op.
In
syncability_filter_drops_non_readable_eventsthe non-reader path justdrop(rx2)without asserting the event was withheld, andsyncability_filter_updatesnever asserts anything after revoking access. Consider asserting that a boundedrecvon the non-reader/revoked stream times out (onlyReplayComplete, noAdded/Changed), so the test actually validates the drop behavior.Also applies to: 1256-1267
🤖 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 `@src/big_sync/part_store/memory.rs` around lines 1186 - 1210, The syncability filter tests in the memory store are only checking the happy path and never prove that non-readable or revoked subscribers do not receive updates. Update the tests around `syncability_filter_drops_non_readable_events` and `syncability_filter_updates` to assert the negative case by waiting on the subscription receiver with a bounded recv/timeout and verifying that only `ReplayComplete` is seen and no `Added` or `Changed` event arrives after access is denied or revoked. Use the existing `subscribe`, `add_obj_to_parts`, and revocation flow to locate the assertions and replace the silent `drop(rx2)` style checks with explicit receive expectations.src/big_repo/changes.rs (1)
1215-1237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or use the unused filter helpers.
prekey_notification_matches_filter,cgka_notification_matches_filter, andmembership_notification_matches_filterare currently never called; drop them until the dispatch arms need them, or wire them into those paths now.🤖 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 `@src/big_repo/changes.rs` around lines 1215 - 1237, The filter helper functions prekey_notification_matches_filter, cgka_notification_matches_filter, and membership_notification_matches_filter are defined but never used in changes.rs. Either remove these unused helpers for now, or integrate them into the corresponding notification dispatch paths where BigRepoPrekeyNotification, BigRepoCgkaNotification, and BigRepoMembershipNotification are matched so the filtering logic is actually invoked.
🤖 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/big_sync_core/tasks/decide_peer_strat.rs`:
- Around line 112-128: The CursorOnly branch in DecidePeerStrategyTask is
currently unreachable because sync_modes is always initialized from default and
SetPeerEvent only passes peer_id and parts. Update the SetPeerEvent/set_peer
flow and the DecidePeerStrategyTask constructors so the per-part sync mode map
is threaded through and stored in sync_modes, then the get(&part_id) lookup can
actually return CursorOnly instead of always falling back to Bucket.
In `@src/daybook_core/rt/switch.rs`:
- Around line 346-356: The subscription in switch and the call to
part_store.subscribe are using the all-zero PeerId, which can cause
access-controlled doc events to be dropped when doc_members are present. Update
this subscription to use the node’s real principal/identity instead of
PeerId::new([0u8; 32]), or explicitly grant the zero principal read access if
that is intentional. Keep the change localized to the subscription setup near
the .subscribe call that builds SubPartsRequest.
---
Outside diff comments:
In `@src/big_sync_core/tasks/peer_replay.rs`:
- Around line 72-82: Use the local PeerId for the SubPartsRequest.peer_id in
peer_replay::PeerReplayTask::run instead of self.peer_id, since self.peer_id is
only the remote source used by cx.rpc_clients.get(...). Thread the local node
identity through TaskCtx so this method can reference it, and update the
sub_parts request construction to pass that local identity while leaving the
remote lookup unchanged.
In `@src/big_sync/part_store/sqlite.rs`:
- Around line 115-123: `SqlitePartStore::new` currently initializes
`doc_members_cache` empty and never reloads persisted membership data, so access
levels are lost after restart. Update the startup path in `new()` (or a helper
it calls) to read the existing `big_sync_syncable` records from SQLite and
populate `doc_members_cache` before returning `Self`, using the existing
`sql`/`scope_id`/`bucket_depth` state so the cache reflects persisted values
even before `set_doc_members` runs.
---
Nitpick comments:
In `@src/big_repo/changes.rs`:
- Around line 1215-1237: The filter helper functions
prekey_notification_matches_filter, cgka_notification_matches_filter, and
membership_notification_matches_filter are defined but never used in changes.rs.
Either remove these unused helpers for now, or integrate them into the
corresponding notification dispatch paths where BigRepoPrekeyNotification,
BigRepoCgkaNotification, and BigRepoMembershipNotification are matched so the
filtering logic is actually invoked.
In `@src/big_sync/part_store/memory.rs`:
- Around line 1186-1210: The syncability filter tests in the memory store are
only checking the happy path and never prove that non-readable or revoked
subscribers do not receive updates. Update the tests around
`syncability_filter_drops_non_readable_events` and `syncability_filter_updates`
to assert the negative case by waiting on the subscription receiver with a
bounded recv/timeout and verifying that only `ReplayComplete` is seen and no
`Added` or `Changed` event arrives after access is denied or revoked. Use the
existing `subscribe`, `add_obj_to_parts`, and revocation flow to locate the
assertions and replace the silent `drop(rx2)` style checks with explicit receive
expectations.
In `@src/big_sync/test.rs`:
- Around line 189-192: The test double in `src/big_sync/test.rs` is hardcoding
the subscriber identity instead of using the request’s peer identity, so it no
longer matches `BigSyncRpcWorker::handle_rpc_message` behavior. Update the
memory RPC path to forward `req.peer_id` into `target_part_store.subscribe` (as
production does via `inner.peer_id`) so the readability filter is exercised with
the actual requester identity.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83f7ed12-c760-49ee-ae5d-3838e83a98d7
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockand included by**/*docs/DEVDOC/log.mdis excluded by!docs/DEVDOC/**and included by**/*
📒 Files selected for processing (24)
AGENTS.mdsrc/big_repo/backend.rssrc/big_repo/changes.rssrc/big_repo/encrypted_blob.rssrc/big_repo/ephemeral.rssrc/big_repo/handler.rssrc/big_repo/keyhive.rssrc/big_repo/keyhive_listener.rssrc/big_repo/lib.rssrc/big_repo/runtime.rssrc/big_repo/test.rssrc/big_sync/Cargo.tomlsrc/big_sync/part_store.rssrc/big_sync/part_store/memory.rssrc/big_sync/part_store/sqlite.rssrc/big_sync/rpc.rssrc/big_sync/stress_support.rssrc/big_sync/test.rssrc/big_sync/test/stress.rssrc/big_sync_core/lib.rssrc/big_sync_core/rpc.rssrc/big_sync_core/tasks/decide_peer_strat.rssrc/big_sync_core/tasks/peer_replay.rssrc/daybook_core/rt/switch.rs
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- src/big_sync/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/big_repo/encrypted_blob.rs
- src/big_repo/handler.rs
- src/big_repo/ephemeral.rs
- src/big_repo/lib.rs
Comment on lines
+112
to
+128
| // Per-part sync mode: look up from peer_state or default to Bucket. | ||
| // The mode is supplied by the embedder via set_peer's parts map. | ||
| let sync_mode = self | ||
| .sync_modes | ||
| .get(&part_id) | ||
| .copied() | ||
| .unwrap_or(SyncMode::Bucket); | ||
| if sync_mode == SyncMode::CursorOnly { | ||
| part_strats.insert( | ||
| part_id, | ||
| PeerPartStratDecision::Cursor(CursorStrat { | ||
| latest_cursor: part_summary.latest_cursor, | ||
| last_cursor: last_peer_cursor, | ||
| }), | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all constructors of DecidePeerStrategyTask and inspect sync_modes wiring.
rg -nP -C2 'DecidePeerStrategyTask\s*\{' src
# Confirm whether any path populates a non-empty sync_modes map.
rg -nP 'sync_modes\s*:' srcRepository: dman-os/townframe
Length of output: 2346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the task definition and the three constructor sites with surrounding context.
sed -n '1,220p' src/big_sync_core/tasks/decide_peer_strat.rs
printf '\n--- lib.rs around constructor 1 ---\n'
sed -n '690,720p' src/big_sync_core/lib.rs
printf '\n--- lib.rs around constructor 2 ---\n'
sed -n '878,905p' src/big_sync_core/lib.rs
printf '\n--- lib.rs around constructor 3 ---\n'
sed -n '958,980p' src/big_sync_core/lib.rs
printf '\n--- search for sync_modes population outside constructors ---\n'
rg -n 'sync_modes|SyncMode::CursorOnly|SetPeerEvent|set_peer' src/big_sync_coreRepository: dman-os/townframe
Length of output: 12912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'struct SetPeerEvent|enum SetPeerEvent|SetPeerEvent' src/big_sync_core src
printf '\n--- sync_modes references ---\n'
rg -n -C3 'sync_modes' srcRepository: dman-os/townframe
Length of output: 7458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the SetPeerEvent definition and any fields related to per-part sync mode.
rg -n -C4 'struct SetPeerEvent|enum SetPeerEvent|pub struct SetPeerEvent|parts:.*SyncMode|SyncMode' src/big_sync_coreRepository: dman-os/townframe
Length of output: 3375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'sync_modes\.(insert|extend|clear|remove)|\.sync_modes' src/big_sync_core srcRepository: dman-os/townframe
Length of output: 309
CursorOnly can’t be selected yet. The three DecidePeerStrategyTask constructors still pass sync_modes: default(), and SetPeerEvent only carries peer_id and parts, so this branch always falls back to Bucket. Thread the per-part mode map through set_peer/SetPeerEvent if CursorOnly is meant to be usable.
🤖 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 `@src/big_sync_core/tasks/decide_peer_strat.rs` around lines 112 - 128, The
CursorOnly branch in DecidePeerStrategyTask is currently unreachable because
sync_modes is always initialized from default and SetPeerEvent only passes
peer_id and parts. Update the SetPeerEvent/set_peer flow and the
DecidePeerStrategyTask constructors so the per-part sync mode map is threaded
through and stored in sync_modes, then the get(&part_id) lookup can actually
return CursorOnly instead of always falling back to Bucket.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit