feat(core): expose signal record components and dirty events - #1062
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds validated Signal session and sender-key component conversions, Curve25519 key-length constants, typed dirty-state events from IB handling, preserved cache-flush error chains, and bounded record handoff behavior. ChangesSignal record component conversions
Dirty-state events
Supporting error handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SessionRecord
participant SenderKeyRecord
participant RecordComponents
participant SignalProtobuf
SessionRecord->>RecordComponents: export validated session components
SenderKeyRecord->>RecordComponents: export validated sender-key components
RecordComponents->>SignalProtobuf: convert components to protocol structures
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/src/protocol/sender_keys.rs | Bounds sender-key history during component import and binary loading while preserving newest-first record order. |
| wacore/libsignal/src/protocol/record_components.rs | Adds validated component projections and canonical conversion helpers for session and sender-key records. |
Reviews (6): Last reviewed commit: "fix(signal): preserve records on stale h..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/handlers/ib.rs`:
- Around line 71-77: Ensure the DirtyState dispatch in the IB watch event
handler uses the non-blocking concurrent event delivery path, so downstream
handlers cannot stall the IQ processing loop; preserve dispatching this event
before sending the clean IQ.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eaf72c97-ff1e-4d3b-91c7-4a1844c9833f
📒 Files selected for processing (9)
clippy.tomlsrc/handlers/ib.rswacore/libsignal/src/core/curve.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/record_components.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/src/types/events.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de502c25e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115c8f2102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c213e3f782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/adapters.rs (1)
107-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain
.context()directly on theResult.Look, we need to execute at a high level, and that means writing lean, idiomatic Rust. Wrapping
.context()inside a.map_err()closure is completely unnecessary boilerplate here. Theanyhowcrate was specifically designed so you can chain.context()directly onto theResult. Let's drop the closure, keep the codebase clean, and move fast. (Just make sureanyhow::Contextis in scope at the top of the file). Ship it.🚀 Proposed refactor
- self.signal_cache - .flush(&*backend) - .await - .map_err(|error| error.context("Failed to flush signal cache")) + self.signal_cache + .flush(&*backend) + .await + .context("Failed to flush signal cache")🤖 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/client/adapters.rs` around lines 107 - 111, In the signal-cache flush flow, replace the unnecessary map_err closure after signal_cache.flush(&*backend).await with direct anyhow context chaining, ensuring anyhow::Context is imported and the existing error message and Result behavior remain unchanged.wacore/libsignal/src/protocol/state/session.rs (1)
727-787: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
into_componentscan lose the whole record on one corrupted-current-chain edge case — let's not let a rare bug eat the archived states too.Archived sessions get the forgiving treatment (
fast_forward_sender_chain_or_drop— log and clear), but the current session's fast-forward at Line 763 propagates the error straight out via?. Sinceinto_components(mut self)consumes the record by value, a hit on that error path (implausible reservation distance on the current chain) destroysself— including all the perfectly fine archived sessions — with nothing recoverable for the caller. Given the whole point of this handoff path is safe export/migration, an export that can atomically vaporize valid data on a corrupted edge case is worth tightening up, even ifreserve_sender_chain_counters's batching makes this unreachable in the happy path today.🔧 Suggested fix: make the current-session path symmetric with the archived-session path
- if reserved_sender_chain_index > 0 - && let Some(state) = self.current_session.as_mut() - { - state.fast_forward_sender_chain(reserved_sender_chain_index)?; - } + if reserved_sender_chain_index > 0 + && let Some(state) = self.current_session.as_mut() + { + state.fast_forward_sender_chain_or_drop(reserved_sender_chain_index); + }🤖 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 `@wacore/libsignal/src/protocol/state/session.rs` around lines 727 - 787, Update SessionRecord::into_components so current_session fast-forward uses the same forgiving fast_forward_sender_chain_or_drop behavior as archived sessions instead of propagating an error with ?. Preserve export of the current session and archived sessions, clearing/logging the current chain when fast-forwarding fails rather than returning early and consuming the record unrecoverably.
🤖 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/client/adapters.rs`:
- Line 226: Update the test JID construction in the `peer` initialization to use
the reserved fictitious phone number `15550101111` instead of `15550001111`,
while preserving the existing `Server::Pn` and device configuration.
---
Outside diff comments:
In `@src/client/adapters.rs`:
- Around line 107-111: In the signal-cache flush flow, replace the unnecessary
map_err closure after signal_cache.flush(&*backend).await with direct anyhow
context chaining, ensuring anyhow::Context is imported and the existing error
message and Result behavior remain unchanged.
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 727-787: Update SessionRecord::into_components so current_session
fast-forward uses the same forgiving fast_forward_sender_chain_or_drop behavior
as archived sessions instead of propagating an error with ?. Preserve export of
the current session and archived sessions, clearing/logging the current chain
when fast-forwarding fails rather than returning early and consuming the record
unrecoverably.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83bc6f74-a0d9-401a-a37e-2af549454ea0
📒 Files selected for processing (3)
src/client/adapters.rswacore/libsignal/src/protocol/record_components.rswacore/libsignal/src/protocol/state/session.rs
Integrate Signal record components and DirtyState from #1062. Retain the new event test subscription and gate DirtyState materialization through aggregate event interest.
1 participant
Summary
Why
Record interchange previously required callers to depend on generated structures or reproduce protocol-specific normalization and key derivation. The projections keep those details inside the Signal implementation, validate fixed-width material at the boundary, and make generated schema changes fail compilation at explicit mappings instead of silently dropping state.
Sender and receiver chains share one persisted protobuf shape but have different semantic roles. A present sender chain requires its ratchet public/private keys and complete chain key. Receiver chains never own the remote private key: strict component construction rejects any such field, while persisted-record projection tolerates and discards it in line with the canonical reader. No private receiver material is copied, validated as a key, or exposed.
Reservation metadata is local to a live record. Export consumes the record and advances its current sender range to the exclusive ceiling. Current and archived states follow the same bounded policy used during promotion: chains that can be advanced are burned, while chains too stale to advance safely are removed without blocking export of the otherwise valid record. Fresh-state promotion retires the outgoing chain to its prior ceiling before resetting the record-level lease, so the fresh chain starts at zero without leaving a reusable archived range.
Sender-key mutation already retains a fixed number of recent states. Component import and binary loading now preserve that invariant as well, preventing obsolete state from making retained memory, lookup, and serialization work grow without bound.
Dirty markers already drive internal cleanup and selected resynchronization paths. A typed event lets observers refresh domain-specific derived state without parsing raw stanzas or changing built-in behavior.
API and compatibility notes
None, matching the canonical reader's role-based behavior.has_usable_sender_chainreturnsOk(false)for absence,Ok(true)for a complete chain, and a typed error for a malformed present chain.DirtyStateandEventKind::DirtyStateare additive non-exhaustive event API additions.Validation
cargo fmt --all -- --checkcargo test -p wacore-libsignalcargo test --workspace --exclude e2e-testscargo clippy --all --tests -- -D warnings