Group lifecycle: participation taxonomy + eviction detection, recovery probe, removal notices, quarantine (#727)#769
Merged
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:
WalkthroughThe PR adds participation state and membership interval tracking to group records, threads that state through engine ingest, lifecycle, and convergence paths, and extends transport handling with removal notices and group backfill. App routing, probing, storage, and FFI layers are updated to surface the new participation state. ChangesParticipation, engine state, and specs
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 13 individual chapters for you: 
Chapters generated by Stage for commit aefab2e on Jul 2, 2026 6:53pm UTC. |
erskingardner
left a comment
erskingardner
left a comment
Member
There was a problem hiding this comment.
Adversarial pass found two contract issues in the new lifecycle taxonomy. I left them inline where the taxonomy is introduced.
|
|
||
| - `Member`: the local identity is present in the group's canonical roster. This is the only participation state in | ||
| which a client MAY prepare local group-state commits or emit delivered app payloads for the group. | ||
| - `Evicted`: the local identity is authoritatively no longer a member of the group. The group is inactive for this |
Member
There was a problem hiding this comment.
[P2] This taxonomy collapses voluntary departure into eviction/removal semantics. Existing protocol and event surfaces distinguish SelfRemove/MemberLeft from admin removal/MemberRemoved, but the only non-member participation state here is Evicted, and the public-surface rule below requires callers to distinguish only Member/Evicted/Quarantined/unknown. That leaves no way for APIs or UI to represent "I left voluntarily" without labeling it as eviction/removal. Please either make this state neutral (for example NonMember/Inactive with a reason) or add a separate Left/Departed terminal state.
| - commits that fork from outside the rollback horizon: these are ineligible for branch selection (see | ||
| [convergence.md](./convergence.md), "Eligibility") and, when their source epoch is also older than the retained | ||
| anchor, are reported as `BeyondAnchor`. | ||
| - messages that predate the local identity's membership and can never be decrypted by this client |
Member
There was a problem hiding this comment.
[P2] PreMembership needs to be defined against membership intervals, not a single membership boundary. The same PR allows rejoin/reinstatement after eviction; after leave/evict/rejoin, messages from a prior valid membership interval may still be recoverable from retained state, while messages between eviction and rejoin are outside membership but not simply "before membership". Please specify this as outside any valid local membership interval, and state what interval data implementations must retain/classify against.
Member
|
Reviewed the taxonomy against what OpenMLS actually surfaces. The types + vocabulary read clean and the "no behavior change" claim checks out. One thing to reconcile before the follow-up slices build on it: The spec's path-2 eviction ("removal commit never applied … MLS reports that the local identity has been evicted") has no OpenMLS realization. OpenMLS has exactly one eviction mechanism, and it's entirely gated on merging the removal commit:
So if the removal commit is never applied (the relay-reordering case the spec calls "a required fallback, not an edge case"), the group stays Two concrete asks:
(Substrate for representing the state already exists — we keep a tombstoned group after removal per |
Member
|
Overall the idea of this participation state makes sense to me but we need to be really careful that it's actually using mechanics that exist and are actually usable by us at the point in which we need to change them. |
Contributor
Author
|
@erskingardner Thanks — all three land, and the OpenMLS archaeology is the important one. You're right: I claimed an MLS mechanism that doesn't exist. Verified against our own engine ( A — path 2 has no MLS realization (conceded). Reworded Your seam question, answered in-spec: #722's B — voluntary leave vs eviction. Added a distinct C — membership intervals. No behavior change still holds (additive types; spec + |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/foundation/errors.md`:
- Around line 24-26: Clarify the `pre_membership` error definition in the errors
spec so it only applies when retained membership history exists for the identity
and the input falls outside those retained membership intervals. Update the
wording around `PreMembership` in the documented error enum/section to exclude
the no-state case, which should remain covered by `unknown_group`, and ensure
any repeated description in the later spec section stays consistent with this
scope.
In `@spec/protocol-core/group-state.md`:
- Around line 138-177: The path-2 fallback in the participation state rules
loses the distinction between `Left` and `Evicted`, which the later contract
requires preserving. Update the fallback wording near the non-member transition
logic to say that authenticated roster/relay evidence may establish
non-membership, but the client MUST still carry forward the best-supported
reason (`Left` vs `Evicted`) when available; reference the `Left`/`Evicted`
definitions and the “Removal commit not applied” section so the preserved reason
stays consistent with the public-surface requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a05c87-3620-48d8-b525-112d3f2b4d41
⛔ Files ignored due to path filters (4)
crates/traits/tests/snapshots/snapshots__participation_evicted.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__participation_left.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__participation_member.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__participation_quarantined.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
crates/traits/src/group.rscrates/traits/tests/snapshots.rsspec/foundation/errors.mdspec/protocol-core/group-state.mdspec/protocol-core/inbound-processing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/protocol-core/inbound-processing.md
- crates/traits/src/group.rs
Member
|
Pushed cc08bd7, which restructures the design around the conclusion from the thread above: the fragility was treating discovery failures as an epistemology problem. The commit makes the removal commit the sole membership authority and demotes everything else to delivery mechanisms for it. Why this closes the remaining hole. The removal commit is protected under the removed member's last epoch, so it stays readable to them no matter how late it arrives. That means the imperfect-information problem was never authentication — it's delivery of one specific, always-consumable artifact. So instead of a second "derive non-membership above MLS" path (which had no realizable oracle),
The spec also now states the irreducible limit honestly: a removed client that receives nothing is indistinguishable from a member of a quiet group, so the guarantee is eventual detection, with the mechanisms above making it fast in practice. Transport agnosticism enforced, not assumed. Protocol-core names only capability-neutral obligations (missed-input recovery, recipient inbox). The Nostr realization. Kind Types. Verified: |
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
…emoval commits Package A of the #769 follow-ups: participation transitions now have exactly one authority — the applied removal commit — realized on BOTH apply paths. - traits: Group gains a serde-defaulted `participation` field (legacy JSON records decode as Member; regression-tested). Living on the durable record means it survives live-OpenMLS teardown and rolls back with fork-recovery snapshots (a rolled-back removal also rolls back the transition). GroupEvent::ParticipationChanged surfaces transitions; CgkaEngine::participation() exposes Member/Left/Evicted/Quarantined vs None ("no such group") per the public-surface rule in group-state.md. - engine direct seam (ingest.rs): self in the post-merge removed set → Left when the commit consumed our own SelfRemove, else Evicted. - engine convergence seam: CommitStaged replay observations now carry self_remove_senders (resolved pre-merge, like the direct seam), so the replay path attributes MemberLeft to the leaver AND transitions self participation identically — the Left/Evicted distinction is no longer path-dependent (fixes the asymmetry documented in invite_leave.rs). - rejoin: do_join_welcome resets participation to Member (fresh membership grant is the reinstatement path) and emits the transition when the prior record was non-member. - inactive-group ingest arms (is_active()==false, UseAfterEviction): now invariant repairs — a record still claiming Member on an MLS-inactive group moves to Quarantined(IntegrityHold) instead of the signal being swallowed (the #722 arm); never fabricates a Left/Evicted reason. - uniffi: ParticipationChanged FFI variant with stable low-cardinality participation tags. Tests: invite_leave (Evicted via convergence apply; Left from consumed SelfRemove; Member restored + event on re-add welcome; remaining member unaffected), traits legacy-record decode + snapshots. Engine 18/18 test binaries green; workspace tests green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
Package B of the #769 follow-ups: stop collapsing eviction-adjacent inbound into the generic PeelFailed bucket. - traits: StaleReason::Evicted — inbound for a group the local identity is no longer a member of; maps to the `evicted` category / `stale` disposition in spec/foundation/errors.md. Snapshot-locked. Classification never *drives* the participation transition (that stays with the applied removal commit, package A). - engine: three arms now classify Evicted instead of PeelFailed/ UnknownGroup: 1. live MLS state inactive (merged removal) at ingest; 2. the UseAfterEviction guard (path-1 aftermath); 3. no live MLS state but the durable record says Left/Evicted — the tombstoned-group case after teardown, previously misreported as UnknownGroup (errors.md scopes unknown_group to "no state at all"); persisted Failed (terminal) instead of Retryable. - audit: stable "evicted" stale-reason string. Test: readd_after_remove now asserts an evicted member ingesting the re-add commit gets Stale(Evicted) — an evicted client cannot apply a later commit and must not try — while the fresh Welcome still reinstates. PreMembership classification is deferred to the membership-intervals package where it can actually fire. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
Package A2 of the #769 follow-ups — closes the relay-plane audit finding that a removed group's subscriptions could persist forever. Before: post-removal cleanup was gated on `state.groups.len() != before` (sync.rs), which a convergence-applied removal never trips (the group record is retained by design), and `refresh_group_routes` unconditionally re-added every retained group's route — so an evicted group kept its relay subscriptions and could even be silently re-added after a transient drop. The adapter's sync_account_groups diff was already correct; the app simply never handed it a reduced set. Now, driven by the authoritative GroupEvent::ParticipationChanged (pkg A): - Left/Evicted → self_membership flag (chat-list row stays renderable per the left-vs-removed distinction), route removed, transport groups synced immediately → adapter unsubscribes; - Quarantined → route removed + synced (withheld from live processing; no membership flag write — quarantine asserts nothing about membership); - Member (verified rejoin) → flag cleared, route re-added, synced. - refresh_group_routes now filters groups whose durable self_membership is 'removed', so no later refresh resurrects a dead group's subscription (survives restart via the existing account_groups projection). - AppTransportRouting::remove_group + storage account_group_ids_with_removed_membership() query. Tests: routing remove_group unit (targeted + idempotent), storage removed-membership query roundtrip incl. rejoin flip-back. marmot-app + storage-sqlite suites green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
…overy probe Package C of the #769 follow-ups — the #722 root-cause fix. The persisted transport cursor previously advanced after EVERY ingest, including deliveries that failed outer decryption. Since the cursor becomes the relay `since` filter, one post-removal (undecryptable) message pushed the window past the missed removal commit's timestamp and no future subscription ever fetched it: deterministic, permanent silent eviction. Cursor fix (marmot-app): `outcome_advances_transport_cursor` — an undecryptable delivery (Stale/PeelFailed) freezes the cursor at the last input successfully consumed (the spec's recovery-window anchor); every durably-accounted outcome (processed, buffered-for-convergence, terminal-stale incl. Evicted) still advances it. Consumed input for a group also resets that group's probe budget. Membership recovery probe (spec group-state.md "Reaching a non-member state", realized per transports/nostr.md "Membership backfill probe"): - traits: TransportGroupBackfill + TransportAdapter::backfill_account_group. - nostr adapter: re-issues the group's STORED route (caller endpoints ignored — a stale caller cannot point the probe at rogue relays) with the widened `since`, reusing the live subscription id so the relay replaces and replays instead of stacking duplicates; typed rejection for unknown groups. - relay-plane wrapper: endpoint safety sanitization like group sync. - marmot-account: backfill_transport_group resolves the routed subscription; routeless (withheld) groups are a no-op. - marmot-app driver: undecryptable delivery for a known group triggers the probe with exponential backoff (60s doubling, 1h cap) anchored at the newest consumed app event minus a 300s slack; no consumed input → full re-fetch (dedup collapses overlap). Tests: cursor-gating matrix (PeelFailed freezes; all else advances), probe backoff schedule (base/double/cap, no overflow), runtime forwards the routed subscription + since and no-ops without a route, adapter reissues the stored route with probe window + rejects unknown groups + reuses the subscription id. Workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
…ceive Package D of the #769 follow-ups: the removal notice mechanism from spec/transports/nostr.md ("Removal notice delivery") and spec/protocol-core/member-departure.md ("Removal notices"). Wire shape (transport-nostr-peeler): NIP-59 gift wrap (1059 → 13 seal → unsigned kind-451 rumor) addressed to the removed member. The rumor embeds the exact published kind-445 removal-commit event as stringified JSON (NIP-18 convention) with h/e cross-check tags — no fetch round-trip, no relay-retention dependency, and the gift wrap leaks no group id to relays. Peel side verifies the embedded event's id + signature and full kind-445 envelope validation before returning PeeledContent::RemovalNotice; a tampered embed is rejected, so a forged notice can never inject bytes the committer did not publish. Authority stays with the commit: the engine's inbox seam re-injects the embedded, transport-validated group message into ordinary do_ingest — dedup, validation, and (when it really is our removal commit) the package-A participation transition all apply as if it arrived on the group stream. The notice itself changes nothing. Committer send seam (marmot-account runtime): after this device's published + confirmed commit removes members (correlated by origin_commit_id against the commits published in the same drain — only the committer holds the published wire event, matching the spec's SHOULD), wrap per removed member and publish to their cached inbox route. Every notice failure (no inbox route, binding without notice support, publish error) is logged and swallowed — a delivery aid must never fail the commit flow. Plumbing: TransportPeeler::wrap_removal_notice and CgkaEngine::wrap_removal_notice both default to Ok(None) ("this binding does not carry removal notices") so mocks and future bindings stay compiling and opt in explicitly; session passthrough; kind 451 constant. Tests: peeler wrap/peel roundtrip (byte-identical embed, no relay-visible group id) + non-445-embed rejection + tampered-embed rejection; runtime committer test (commit publish then inbox-targeted notice carrying the published commit); engine re-injection test (embedded message classifies through ordinary ingest; non-group embeds rejected). Workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/cgka-engine/src/message_processor/ingest.rs (1)
67-89: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftOrdinary Welcomes now get peeled twice.
When
peel_welcomesucceeds and returnsPeeledContent::Welcome(notRemovalNotice), thisif letchain doesn't match and execution falls through unchanged to the existingself.do_join_welcome(msg.clone())call below, which re-runsself.peeler.peel_welcome(&welcome_msg).awaitinternally (group_lifecycle.rs). Every successful real-Welcome ingest now performs the NIP-59 gift-wrap+seal unwrap/decrypt twice instead of once.Consider threading the already-peeled content into
do_join_welcome(e.g. anOption<PeeledMessage>parameter it uses instead of re-peeling when present) to avoid the duplicate crypto work. Please also confirmpeel_welcomehas no single-use/replay-sensitive state that could make calling it twice on the same message unsafe (vs. just wasteful).♻️ Illustrative direction (needs a matching change in group_lifecycle.rs's do_join_welcome to accept pre-peeled content)
- if let Ok(peeled) = self.peeler.peel_welcome(msg).await - && let PeeledContent::RemovalNotice { embedded } = peeled.content + let peeled = self.peeler.peel_welcome(msg).await; + if let Ok(peeled_ok) = &peeled + && let PeeledContent::RemovalNotice { embedded } = &peeled_ok.content { if !matches!( embedded.envelope, cgka_traits::transport::TransportEnvelope::GroupMessage { .. } ) { return Ok(IngestOutcome::Stale { reason: StaleReason::PeelFailed, }); } - return Box::pin(self.do_ingest(embedded)).await; + return Box::pin(self.do_ingest(embedded.clone())).await; } - // Reuse the existing join_welcome machinery. Map its error shapes - // to typed stale reasons where applicable. - match self.do_join_welcome(msg.clone()).await { + // Reuse the existing join_welcome machinery, passing the already- + // peeled content so a real Welcome is not unwrapped twice. + match self.do_join_welcome_from_peeled(msg.clone(), peeled).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 `@crates/cgka-engine/src/message_processor/ingest.rs` around lines 67 - 89, The welcome ingest path is peeling the same message twice when `peel_welcome` returns `PeeledContent::Welcome`, because `ingest` falls through to `do_join_welcome` which peels again internally. Update `ingest` and `group_lifecycle::do_join_welcome` to thread through the already-peeled welcome content (for example via an optional pre-peeled parameter) and reuse it instead of calling `self.peeler.peel_welcome` a second time. Keep the existing removal-notice handling in `PeeledContent::RemovalNotice`, and verify `peel_welcome` does not depend on single-use state before reusing the peeled result.crates/cgka-engine/src/engine.rs (1)
922-955: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving participation-transition behavior out of
engine.rs, and pair it with an audit record.Two related points on
set_group_participation:
- It implements real behavior (load/compare/persist/emit) directly in
engine.rs, whereas the file's role here is meant to be construction and trait dispatch, with behavior living in sibling modules such asgroup_lifecycle.rs(which already owns related group-record mutation helpers). As per path instructions, "Inengine.rs, own construction and trait dispatch while keeping behavior in focused sibling modules."- Sibling state transitions in this file (
quarantine_stored_group_on_hydrate, thePendingCommitRecoveredpath) emit an explicit audit record alongside theirevents_bufpush; this method only pushes theGroupEvent. Given the PR spec calls out authoritative eviction and quarantine consistency as security-relevant, a matching audit record would keep the forensic trail consistent with how other lifecycle transitions in this file are recorded.♻️ Illustrative sketch (exact placement/audit-event shape TBD by a maintainer)
- pub(crate) fn set_group_participation( - &mut self, - group_id: &GroupId, - participation: cgka_traits::GroupParticipation, - ) -> Result<(), EngineError> { - let mut group = match self.storage.get_group(group_id) { - Ok(group) => group, - Err(cgka_traits::StorageError::NotFound) => return Ok(()), - Err(e) => return Err(EngineError::Backend(format!("get_group: {e:?}"))), - }; - if group.participation == participation { - return Ok(()); - } - group.participation = participation; - self.storage - .put_group(&group) - .map_err(|e| EngineError::Backend(format!("put_group: {e:?}")))?; - self.events_buf - .push_back(cgka_traits::engine::GroupEvent::ParticipationChanged { - group_id: group_id.clone(), - participation, - }); - Ok(()) - } + pub(crate) fn set_group_participation( + &mut self, + group_id: &GroupId, + participation: cgka_traits::GroupParticipation, + ) -> Result<(), EngineError> { + // Delegate the load/compare/persist behavior to group_lifecycle.rs. + let Some(event) = crate::group_lifecycle::transition_group_participation( + &self.storage, + group_id, + participation, + )? else { + return Ok(()); + }; + self.audit_group(group_id, /* forensic event mirroring `event` */ todo!()); + self.events_buf.push_back(event); + Ok(()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/engine.rs` around lines 922 - 955, Move the participation transition logic out of engine.rs and into the sibling module that owns group record mutations, such as group_lifecycle, while keeping set_group_participation as a thin dispatch point. Preserve the current load/compare/persist/emit flow and update the helper that handles GroupId/GroupParticipation transitions so engine.rs only delegates. Also add a matching audit record alongside the existing events_buf push, following the pattern used by quarantine_stored_group_on_hydrate and PendingCommitRecovered so participation changes have the same forensic trail.Source: Path instructions
🤖 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 `@crates/marmot-account/src/runtime.rs`:
- Around line 471-517: The removal-notice logging in send_removal_notices
currently emits raw errors, which may expose GroupId, MemberId, or backend text
from EngineError and TransportAdapterError. Update the tracing::warn!/debug!
calls in the wrap_removal_notice, publish_target, and adapter.publish paths to
keep the existing error_code but replace %e with a redacted, generic summary
that does not include the underlying error payload. Use the same method name and
flow in runtime.rs to locate the three log sites and make the messages safe for
production logs.
In `@crates/marmot-app/src/client/sync.rs`:
- Around line 640-653: Clamp the backfill anchor before computing `since` in
`sync.rs` so a sender-controlled `recorded_at` cannot push recovery into the
future. Update the `anchor`/`since` flow in `messages_with_query` handling to
bound the timestamp against a safe upper limit (for example, current time or
another trusted clock source) before applying `MEMBERSHIP_BACKFILL_SLACK_SECS`.
Keep the change localized around the `anchor` mapping and `since` derivation so
the cursor-freeze recovery path stays valid.
---
Nitpick comments:
In `@crates/cgka-engine/src/engine.rs`:
- Around line 922-955: Move the participation transition logic out of engine.rs
and into the sibling module that owns group record mutations, such as
group_lifecycle, while keeping set_group_participation as a thin dispatch point.
Preserve the current load/compare/persist/emit flow and update the helper that
handles GroupId/GroupParticipation transitions so engine.rs only delegates. Also
add a matching audit record alongside the existing events_buf push, following
the pattern used by quarantine_stored_group_on_hydrate and
PendingCommitRecovered so participation changes have the same forensic trail.
In `@crates/cgka-engine/src/message_processor/ingest.rs`:
- Around line 67-89: The welcome ingest path is peeling the same message twice
when `peel_welcome` returns `PeeledContent::Welcome`, because `ingest` falls
through to `do_join_welcome` which peels again internally. Update `ingest` and
`group_lifecycle::do_join_welcome` to thread through the already-peeled welcome
content (for example via an optional pre-peeled parameter) and reuse it instead
of calling `self.peeler.peel_welcome` a second time. Keep the existing
removal-notice handling in `PeeledContent::RemovalNotice`, and verify
`peel_welcome` does not depend on single-use state before reusing the peeled
result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b7cfabf-6890-4795-9428-202777c10d28
⛔ Files ignored due to path filters (2)
crates/traits/tests/snapshots/snapshots__group.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__stale_evicted.snapis excluded by!**/*.snap
📒 Files selected for processing (37)
crates/cgka-conformance-simulator/src/client.rscrates/cgka-conformance-simulator/tests/openmls_replay_probe.rscrates/cgka-engine/src/audit_helpers.rscrates/cgka-engine/src/distributed_convergence.rscrates/cgka-engine/src/engine.rscrates/cgka-engine/src/group_lifecycle.rscrates/cgka-engine/src/message_processor/ingest.rscrates/cgka-engine/src/openmls_projection.rscrates/cgka-engine/tests/hydration_quarantine.rscrates/cgka-engine/tests/ingest.rscrates/cgka-engine/tests/invite_leave.rscrates/cgka-session/src/lib.rscrates/marmot-account/src/runtime.rscrates/marmot-account/tests/runtime.rscrates/marmot-app/src/client/mod.rscrates/marmot-app/src/client/sync.rscrates/marmot-app/src/groups.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/relay_plane/mod.rscrates/marmot-app/src/relay_plane/safety.rscrates/marmot-app/src/runtime/event_routing.rscrates/marmot-app/src/tests.rscrates/marmot-uniffi/src/conversions/event.rscrates/storage-sqlite/src/account_projection.rscrates/storage-sqlite/src/account_projection/tests.rscrates/storage-sqlite/src/storage/test_support.rscrates/traits/src/engine.rscrates/traits/src/group.rscrates/traits/src/ingest.rscrates/traits/src/lib.rscrates/traits/src/peeler.rscrates/traits/src/transport_adapter.rscrates/traits/tests/snapshots.rscrates/transport-nostr-adapter/src/lib.rscrates/transport-nostr-adapter/tests/inbound_routing.rscrates/transport-nostr-peeler/src/lib.rscrates/transport-nostr-peeler/src/peeler.rs
✅ Files skipped from review due to trivial changes (3)
- crates/storage-sqlite/src/storage/test_support.rs
- crates/cgka-engine/tests/hydration_quarantine.rs
- crates/cgka-conformance-simulator/tests/openmls_replay_probe.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/traits/src/lib.rs
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
…8/#689) Package E of the #769 follow-ups: Quarantined{reason} becomes enforced behavior, not just vocabulary. - Ingest withhold guard: inbound for a quarantined group is retained (PeelDeferred, at the group's held epoch so it stays inside the convergence window) and classified Stale(Quarantined) — withheld, not discarded, and never processed live. New StaleReason::Quarantined, snapshot-locked, audit tag "quarantined". - Live convergence exclusion: advance_convergence over a quarantined group is a no-op. Ordinary inbound and scheduled convergence can never silently re-activate a hold (spec group-state.md, "Quarantine"). - CgkaEngine::quarantine_group(reason): places the hold; an authoritative Left/Evicted is never downgraded to "withheld". - CgkaEngine::resolve_group_quarantine: THE explicit recovery transition — the withhold guard stands down for exactly this group for exactly one deliberate convergence pass over the retained input (run decisively past the settlement quiescence window: resolution decides over a closed input set, it does not wait for stragglers). The ordinary apply seams decide the outcome: a retained removal commit resolves to Left/Evicted through the same code path as live traffic; Member is restored only when the live MLS state is active and the canonical roster still contains the local identity; anything else keeps the hold. - Wiring: session + runtime passthroughs; the membership recovery probe now holds a group as Quarantined(PendingMembership) after 8 dry attempts (hours of backoff) instead of probing forever — the kind-451 inbox notice remains a live discovery path (it needs no group route), and resolution replays everything retained meanwhile. Tests: quarantine withholds a live commit (epoch frozen, convergence no-op) then resolve consumes it and restores Member at the advanced epoch; quarantine never masks Evicted and resolving a non-quarantined group reports the authoritative state. Full engine/app/account/session suites green; workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
…(#622) Package F of the #769 follow-ups: membership becomes a set of retained epoch intervals (a group may be left/removed and rejoined), and input from outside every interval classifies as terminal PreMembership. - traits: MembershipInterval { joined_at, ended_at } on the durable Group record (serde-defaulted; snapshot/rollback-coherent like participation); membership_intervals_contain fails OPEN on empty history — errors.md scopes PreMembership to groups with retained membership history, so legacy records never classify it. StaleReason::PreMembership, snapshot-locked, audit tag "pre_membership". - Maintenance rides the participation transitions (one authority, one bookkeeper): create/join open an interval at the current/welcome epoch; a rejoin appends to the retained history instead of overwriting it; ending membership closes the open interval at the removing commit's SOURCE epoch — the last epoch this identity held keys for; post-removal epochs were never readable and stay outside. Quarantine holds touch no interval. - Classification fires ONLY on the too-distant-in-the-past process arm — content this client can never read whose source epoch is readable. Deliberately NOT on the AlreadyAtEpoch arm: a commit older than our join is already represented by the joined state (welcome-before-commit) and keeps its established classification. Terminal: persisted Failed, never retried (unlike deferred MissingRetainedAnchor). Tests: interval-contains unit matrix (empty fails open; join/gap/rejoin; open interval extends forward); the remove→rejoin flow now asserts a message sent inside the removed gap classifies Stale(PreMembership) while welcome-before-commit still classifies AlreadyAtEpoch (fork_detection suite kept green). Full engine + workspace crates green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
added a commit
that referenced
this pull request
Jul 2, 2026
Package G of the #769 follow-ups: clients can now gate the composer and group list on the local identity's participation. - AppGroupMlsState gains a stable low-cardinality `participation` tag ("member" / "left" / "evicted" / "quarantined_pending_membership" / "quarantined_integrity_hold"), populated from the durable engine record on both the live read and the worker's command-ready snapshot (one builder covers both). Serde default keeps pre-field snapshots decoding as live members. Only "member" allows sending, per the public-surface rule in group-state.md — combined with participation() (pkg A), Ok(None) for "no such group", the API distinguishes every state the spec requires. - group_participation_tag is now the single shared vocabulary in marmot-app (uniffi's event conversion re-exports it instead of duplicating the match), and AppGroupMlsStateFfi mirrors the field. Test: tag stability matrix + legacy-snapshot default. marmot-app + marmot-uniffi suites green; workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
changed the title
Member
|
Pushed the complete follow-up implementation as 8 bisectable commits ( |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cgka-engine/src/message_processor/ingest.rs (1)
118-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate storage failures during participation checks.
Both checks currently fail open on
get_grouperrors. A backend error can skip quarantine withholding or leave aMemberrecord unrepaired while returning a stale outcome. OnlyNotFoundshould be ignored; other storage failures should propagate.Proposed fix
- let Ok(group) = self.storage.get_group(group_id) else { - return Ok(()); - }; + let group = match self.storage.get_group(group_id) { + Ok(group) => group, + Err(StorageError::NotFound) => return Ok(()), + Err(err) => return Err(EngineError::Storage(err)), + };- if !self.quarantine_resolutions.contains(&group_id) - && let Ok(group) = self.storage.get_group(&group_id) - && matches!( - group.participation, - cgka_traits::GroupParticipation::Quarantined { .. } - ) - { + if !self.quarantine_resolutions.contains(&group_id) { + match self.storage.get_group(&group_id) { + Ok(group) + if matches!( + group.participation, + cgka_traits::GroupParticipation::Quarantined { .. } + ) => + { // PeelDeferred: the resolution pass replays exactly this state // through `retry_deferred_peels`, re-entering ordinary ingest // with the guard stood down. self.persist_transport_message_for_existing_group( msg, &group_id, group.epoch, MessageState::PeelDeferred, )?; return Ok(IngestOutcome::Stale { reason: StaleReason::Quarantined, }); + } + Ok(_) | Err(StorageError::NotFound) => {} + Err(err) => return Err(EngineError::Storage(err)), + } }Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/message_processor/ingest.rs` around lines 118 - 120, In the participation-check paths that call self.storage.get_group in ingest.rs, stop treating every failure as a benign miss: only ignore the NotFound case and propagate any other storage error instead of returning Ok(()). Update both affected checks (including the later repeated get_group usage) so backend/storage failures cannot incorrectly bypass quarantine withholding or member repair while still preserving the existing behavior for genuinely missing groups.crates/cgka-engine/src/engine.rs (1)
949-984: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the empty-history sentinel on legacy records.
membership_intervals_contain([])intentionally fails open, but theMemberbranch can turn a legacy quarantined record into[current_epoch..]; older retained inputs can then be misclassified asPreMembership. Only mutate intervals when retained history already exists, or synthesize a conservative legacy interval first.One localized guard option
if group.participation == participation { return Ok(()); } + let had_retained_history = !group.membership_intervals.is_empty(); group.participation = participation; @@ cgka_traits::GroupParticipation::Member => { - if !group + if had_retained_history + && !group .membership_intervals .iter() .any(|interval| interval.ended_at.is_none()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cgka-engine/src/engine.rs` around lines 949 - 984, The GroupParticipation handling in engine.rs is overwriting the empty-history sentinel for legacy records by creating a new open MembershipInterval in the Member branch, which can misclassify old quarantined inputs. Update the participation transition logic around the membership_intervals update so it only mutates intervals when retained history already exists, or first synthesizes a conservative legacy interval before opening a new one. Keep the change localized to the GroupParticipation::Member path and preserve the []-means-fail-open behavior for membership_intervals_contain.
🤖 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 `@crates/cgka-engine/src/engine.rs`:
- Around line 1523-1528: The quarantined-group check in engine.rs currently uses
matches! on self.storage.get_group(...) and silently falls through on backend
errors, so update this control flow to propagate storage failures instead of
treating them like a non-quarantined group. In the code around the group
participation check in the engine logic, only let
cgka_traits::StorageError::NotFound continue to the normal unknown-group path,
and return/propagate any other storage error before live convergence proceeds.
- Around line 1609-1622: The recovery probe in engine.rs is masking OpenMLS and
storage read failures by using `.ok().flatten()` and `.unwrap_or(false)`, which
turns backend errors into a false “still quarantined” result. Update the probe
logic around `MlsGroup::load` and `storage.get_group` to preserve and return
those errors instead of collapsing them, so explicit recovery callers can
distinguish unresolved quarantine from operational failure. Keep the membership
and `is_active()` checks in the same `live_and_member` flow, but make the
overall result error-aware rather than defaulting to false on read failures.
In `@crates/traits/src/group.rs`:
- Around line 49-54: The `Membership` docs in `group.rs` describe `ended_at`
using the removing commit’s reached epoch, which conflicts with the inclusive
interval semantics used by the engine. Update the doc comment on `ended_at` to
state that it is the last epoch in the membership interval, i.e. the final
readable source epoch before removal (`group.epoch - 1`), and keep the wording
aligned with `PreMembership` and the `Membership`/`ended_at` semantics.
---
Outside diff comments:
In `@crates/cgka-engine/src/engine.rs`:
- Around line 949-984: The GroupParticipation handling in engine.rs is
overwriting the empty-history sentinel for legacy records by creating a new open
MembershipInterval in the Member branch, which can misclassify old quarantined
inputs. Update the participation transition logic around the
membership_intervals update so it only mutates intervals when retained history
already exists, or first synthesizes a conservative legacy interval before
opening a new one. Keep the change localized to the GroupParticipation::Member
path and preserve the []-means-fail-open behavior for
membership_intervals_contain.
In `@crates/cgka-engine/src/message_processor/ingest.rs`:
- Around line 118-120: In the participation-check paths that call
self.storage.get_group in ingest.rs, stop treating every failure as a benign
miss: only ignore the NotFound case and propagate any other storage error
instead of returning Ok(()). Update both affected checks (including the later
repeated get_group usage) so backend/storage failures cannot incorrectly bypass
quarantine withholding or member repair while still preserving the existing
behavior for genuinely missing groups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1cb6243-9e6f-408c-bd33-9a5fe2a41a38
⛔ Files ignored due to path filters (3)
crates/traits/tests/snapshots/snapshots__group.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__stale_pre_membership.snapis excluded by!**/*.snapcrates/traits/tests/snapshots/snapshots__stale_quarantined.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
crates/cgka-conformance-simulator/tests/openmls_replay_probe.rscrates/cgka-engine/src/audit_helpers.rscrates/cgka-engine/src/engine.rscrates/cgka-engine/src/group_lifecycle.rscrates/cgka-engine/src/message_processor/ingest.rscrates/cgka-engine/tests/hydration_quarantine.rscrates/cgka-engine/tests/invite_leave.rscrates/cgka-session/src/lib.rscrates/marmot-account/src/runtime.rscrates/marmot-account/tests/runtime.rscrates/marmot-app/src/client/mod.rscrates/marmot-app/src/client/sync.rscrates/marmot-app/src/groups.rscrates/marmot-app/src/lib.rscrates/marmot-uniffi/src/conversions/event.rscrates/marmot-uniffi/src/conversions/group.rscrates/storage-sqlite/src/storage/test_support.rscrates/traits/src/engine.rscrates/traits/src/group.rscrates/traits/src/ingest.rscrates/traits/src/lib.rscrates/traits/tests/snapshots.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/cgka-conformance-simulator/tests/openmls_replay_probe.rs
- crates/cgka-engine/tests/hydration_quarantine.rs
- crates/cgka-engine/src/audit_helpers.rs
- crates/marmot-app/src/client/mod.rs
- crates/marmot-uniffi/src/conversions/event.rs
- crates/traits/src/ingest.rs
- crates/marmot-app/src/lib.rs
- crates/storage-sqlite/src/storage/test_support.rs
- crates/marmot-account/src/runtime.rs
- crates/traits/src/engine.rs
- crates/marmot-account/tests/runtime.rs
- crates/cgka-session/src/lib.rs
- crates/cgka-engine/src/group_lifecycle.rs
- crates/marmot-app/src/client/sync.rs
Comment on lines
+1523
to
+1528
| if matches!( | ||
| self.storage.get_group(group_id).map(|g| g.participation), | ||
| Ok(cgka_traits::GroupParticipation::Quarantined { .. }) | ||
| ) { | ||
| return Ok(Vec::new()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate storage errors before live convergence.
matches!(self.storage.get_group(...), Ok(Quarantined { .. })) falls through on backend errors, so convergence can continue without proving the group is not withheld. Treat backend failures as errors; only NotFound should fall through to the normal unknown-group path.
Suggested control flow
- if matches!(
- self.storage.get_group(group_id).map(|g| g.participation),
- Ok(cgka_traits::GroupParticipation::Quarantined { .. })
- ) {
- return Ok(Vec::new());
- }
+ match self.storage.get_group(group_id).map(|g| g.participation) {
+ Ok(cgka_traits::GroupParticipation::Quarantined { .. }) => {
+ return Ok(Vec::new());
+ }
+ Ok(_) | Err(cgka_traits::StorageError::NotFound) => {}
+ Err(e) => return Err(EngineError::Backend(format!("get_group: {e:?}"))),
+ }📝 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 matches!( | |
| self.storage.get_group(group_id).map(|g| g.participation), | |
| Ok(cgka_traits::GroupParticipation::Quarantined { .. }) | |
| ) { | |
| return Ok(Vec::new()); | |
| } | |
| match self.storage.get_group(group_id).map(|g| g.participation) { | |
| Ok(cgka_traits::GroupParticipation::Quarantined { .. }) => { | |
| return Ok(Vec::new()); | |
| } | |
| Ok(_) | Err(cgka_traits::StorageError::NotFound) => {} | |
| Err(e) => return Err(EngineError::Backend(format!("get_group: {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 `@crates/cgka-engine/src/engine.rs` around lines 1523 - 1528, The
quarantined-group check in engine.rs currently uses matches! on
self.storage.get_group(...) and silently falls through on backend errors, so
update this control flow to propagate storage failures instead of treating them
like a non-quarantined group. In the code around the group participation check
in the engine logic, only let cgka_traits::StorageError::NotFound continue to
the normal unknown-group path, and return/propagate any other storage error
before live convergence proceeds.
Contributor
|
One remaining concern: the membership backfill anchor uses the last consumed message’s One thing: using Nostr Small related naming nit: |
…re-membership outcomes
…roup commit processing
…state, interval-based pre-membership
…icted reason from the removal commit in path 2
…tices + recovery probe
Restructures the non-member-state design so participation transitions have
exactly one authority — the commit that removes the identity — and everything
else is a discovery mechanism for delivering that commit:
- spec(group-state): rewrite "Reaching a non-member state" around the single
authority. The removed member can always process the removal commit (it is
protected under its last member epoch), so the imperfect-information problem
is delivery, not authentication. Discovery: (1) undecryptable traffic MUST
trigger the active transport's missed-input recovery, bounded around the
last consumed input, (2) removal notices carry/reference the commit with no
authority of their own, (3) a bounded quarantine hold when probes stay dry.
States the irreducible limit explicitly: detection is eventual, not
immediate.
- spec(group-state): quarantine now carries a reason — pending_membership
(exit: resolution via the commit) vs integrity_hold (exit: repair) — since
the two holds have opposite recovery directions.
- spec(member-departure): new "Removal notices" section — committer SHOULD
notify each removed member via its account inbox on bindings that have one;
covers Left (SelfRemove confirmation) and Evicted symmetrically.
- Transport agnosticism enforced, not assumed: protocol-core names only the
capability-neutral obligations (missed-input recovery, recipient inbox);
transports/README.md checklist now requires every binding to state its
recovery mechanism (or delivery guarantees making it unnecessary) and its
removal-notice envelope (or that it carries none); quic.md explicitly
answers both (carries no group evolution).
- spec(nostr): removal notice delivery as NIP-59 gift wrap -> kind 13 seal ->
unsigned kind 451 rumor embedding the kind 445 removal-commit event
(NIP-18 stringified-event convention), so no fetch round-trip or relay
retention dependency; validation + metadata-privacy rules. Membership
backfill probe (since = last consumed 445 minus slack) named as this
binding's missed-input recovery.
- spec(errors): non-membership reached only by applying the removal commit;
never asserted from an undecryptable message or unverified notice.
- spec(registries): allocate kind 451 (Marmot removal notice rumor).
- traits: GroupParticipation::Quarantined now carries QuarantineReason
{ PendingMembership, IntegrityHold }; snapshots updated (no external
consumers yet, so still additive).
Verified: just fast-ci green; cargo test -p cgka-traits (90 tests) green;
protocol-core transport-leak grep clean (no kinds, relays, gift wraps, or
backfill mechanics outside the Nostr binding).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…emoval commits Package A of the #769 follow-ups: participation transitions now have exactly one authority — the applied removal commit — realized on BOTH apply paths. - traits: Group gains a serde-defaulted `participation` field (legacy JSON records decode as Member; regression-tested). Living on the durable record means it survives live-OpenMLS teardown and rolls back with fork-recovery snapshots (a rolled-back removal also rolls back the transition). GroupEvent::ParticipationChanged surfaces transitions; CgkaEngine::participation() exposes Member/Left/Evicted/Quarantined vs None ("no such group") per the public-surface rule in group-state.md. - engine direct seam (ingest.rs): self in the post-merge removed set → Left when the commit consumed our own SelfRemove, else Evicted. - engine convergence seam: CommitStaged replay observations now carry self_remove_senders (resolved pre-merge, like the direct seam), so the replay path attributes MemberLeft to the leaver AND transitions self participation identically — the Left/Evicted distinction is no longer path-dependent (fixes the asymmetry documented in invite_leave.rs). - rejoin: do_join_welcome resets participation to Member (fresh membership grant is the reinstatement path) and emits the transition when the prior record was non-member. - inactive-group ingest arms (is_active()==false, UseAfterEviction): now invariant repairs — a record still claiming Member on an MLS-inactive group moves to Quarantined(IntegrityHold) instead of the signal being swallowed (the #722 arm); never fabricates a Left/Evicted reason. - uniffi: ParticipationChanged FFI variant with stable low-cardinality participation tags. Tests: invite_leave (Evicted via convergence apply; Left from consumed SelfRemove; Member restored + event on re-add welcome; remaining member unaffected), traits legacy-record decode + snapshots. Engine 18/18 test binaries green; workspace tests green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Package B of the #769 follow-ups: stop collapsing eviction-adjacent inbound into the generic PeelFailed bucket. - traits: StaleReason::Evicted — inbound for a group the local identity is no longer a member of; maps to the `evicted` category / `stale` disposition in spec/foundation/errors.md. Snapshot-locked. Classification never *drives* the participation transition (that stays with the applied removal commit, package A). - engine: three arms now classify Evicted instead of PeelFailed/ UnknownGroup: 1. live MLS state inactive (merged removal) at ingest; 2. the UseAfterEviction guard (path-1 aftermath); 3. no live MLS state but the durable record says Left/Evicted — the tombstoned-group case after teardown, previously misreported as UnknownGroup (errors.md scopes unknown_group to "no state at all"); persisted Failed (terminal) instead of Retryable. - audit: stable "evicted" stale-reason string. Test: readd_after_remove now asserts an evicted member ingesting the re-add commit gets Stale(Evicted) — an evicted client cannot apply a later commit and must not try — while the fresh Welcome still reinstates. PreMembership classification is deferred to the membership-intervals package where it can actually fire. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Package A2 of the #769 follow-ups — closes the relay-plane audit finding that a removed group's subscriptions could persist forever. Before: post-removal cleanup was gated on `state.groups.len() != before` (sync.rs), which a convergence-applied removal never trips (the group record is retained by design), and `refresh_group_routes` unconditionally re-added every retained group's route — so an evicted group kept its relay subscriptions and could even be silently re-added after a transient drop. The adapter's sync_account_groups diff was already correct; the app simply never handed it a reduced set. Now, driven by the authoritative GroupEvent::ParticipationChanged (pkg A): - Left/Evicted → self_membership flag (chat-list row stays renderable per the left-vs-removed distinction), route removed, transport groups synced immediately → adapter unsubscribes; - Quarantined → route removed + synced (withheld from live processing; no membership flag write — quarantine asserts nothing about membership); - Member (verified rejoin) → flag cleared, route re-added, synced. - refresh_group_routes now filters groups whose durable self_membership is 'removed', so no later refresh resurrects a dead group's subscription (survives restart via the existing account_groups projection). - AppTransportRouting::remove_group + storage account_group_ids_with_removed_membership() query. Tests: routing remove_group unit (targeted + idempotent), storage removed-membership query roundtrip incl. rejoin flip-back. marmot-app + storage-sqlite suites green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…overy probe Package C of the #769 follow-ups — the #722 root-cause fix. The persisted transport cursor previously advanced after EVERY ingest, including deliveries that failed outer decryption. Since the cursor becomes the relay `since` filter, one post-removal (undecryptable) message pushed the window past the missed removal commit's timestamp and no future subscription ever fetched it: deterministic, permanent silent eviction. Cursor fix (marmot-app): `outcome_advances_transport_cursor` — an undecryptable delivery (Stale/PeelFailed) freezes the cursor at the last input successfully consumed (the spec's recovery-window anchor); every durably-accounted outcome (processed, buffered-for-convergence, terminal-stale incl. Evicted) still advances it. Consumed input for a group also resets that group's probe budget. Membership recovery probe (spec group-state.md "Reaching a non-member state", realized per transports/nostr.md "Membership backfill probe"): - traits: TransportGroupBackfill + TransportAdapter::backfill_account_group. - nostr adapter: re-issues the group's STORED route (caller endpoints ignored — a stale caller cannot point the probe at rogue relays) with the widened `since`, reusing the live subscription id so the relay replaces and replays instead of stacking duplicates; typed rejection for unknown groups. - relay-plane wrapper: endpoint safety sanitization like group sync. - marmot-account: backfill_transport_group resolves the routed subscription; routeless (withheld) groups are a no-op. - marmot-app driver: undecryptable delivery for a known group triggers the probe with exponential backoff (60s doubling, 1h cap) anchored at the newest consumed app event minus a 300s slack; no consumed input → full re-fetch (dedup collapses overlap). Tests: cursor-gating matrix (PeelFailed freezes; all else advances), probe backoff schedule (base/double/cap, no overflow), runtime forwards the routed subscription + since and no-ops without a route, adapter reissues the stored route with probe window + rejects unknown groups + reuses the subscription id. Workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ceive Package D of the #769 follow-ups: the removal notice mechanism from spec/transports/nostr.md ("Removal notice delivery") and spec/protocol-core/member-departure.md ("Removal notices"). Wire shape (transport-nostr-peeler): NIP-59 gift wrap (1059 → 13 seal → unsigned kind-451 rumor) addressed to the removed member. The rumor embeds the exact published kind-445 removal-commit event as stringified JSON (NIP-18 convention) with h/e cross-check tags — no fetch round-trip, no relay-retention dependency, and the gift wrap leaks no group id to relays. Peel side verifies the embedded event's id + signature and full kind-445 envelope validation before returning PeeledContent::RemovalNotice; a tampered embed is rejected, so a forged notice can never inject bytes the committer did not publish. Authority stays with the commit: the engine's inbox seam re-injects the embedded, transport-validated group message into ordinary do_ingest — dedup, validation, and (when it really is our removal commit) the package-A participation transition all apply as if it arrived on the group stream. The notice itself changes nothing. Committer send seam (marmot-account runtime): after this device's published + confirmed commit removes members (correlated by origin_commit_id against the commits published in the same drain — only the committer holds the published wire event, matching the spec's SHOULD), wrap per removed member and publish to their cached inbox route. Every notice failure (no inbox route, binding without notice support, publish error) is logged and swallowed — a delivery aid must never fail the commit flow. Plumbing: TransportPeeler::wrap_removal_notice and CgkaEngine::wrap_removal_notice both default to Ok(None) ("this binding does not carry removal notices") so mocks and future bindings stay compiling and opt in explicitly; session passthrough; kind 451 constant. Tests: peeler wrap/peel roundtrip (byte-identical embed, no relay-visible group id) + non-445-embed rejection + tampered-embed rejection; runtime committer test (commit publish then inbox-targeted notice carrying the published commit); engine re-injection test (embedded message classifies through ordinary ingest; non-group embeds rejected). Workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…8/#689) Package E of the #769 follow-ups: Quarantined{reason} becomes enforced behavior, not just vocabulary. - Ingest withhold guard: inbound for a quarantined group is retained (PeelDeferred, at the group's held epoch so it stays inside the convergence window) and classified Stale(Quarantined) — withheld, not discarded, and never processed live. New StaleReason::Quarantined, snapshot-locked, audit tag "quarantined". - Live convergence exclusion: advance_convergence over a quarantined group is a no-op. Ordinary inbound and scheduled convergence can never silently re-activate a hold (spec group-state.md, "Quarantine"). - CgkaEngine::quarantine_group(reason): places the hold; an authoritative Left/Evicted is never downgraded to "withheld". - CgkaEngine::resolve_group_quarantine: THE explicit recovery transition — the withhold guard stands down for exactly this group for exactly one deliberate convergence pass over the retained input (run decisively past the settlement quiescence window: resolution decides over a closed input set, it does not wait for stragglers). The ordinary apply seams decide the outcome: a retained removal commit resolves to Left/Evicted through the same code path as live traffic; Member is restored only when the live MLS state is active and the canonical roster still contains the local identity; anything else keeps the hold. - Wiring: session + runtime passthroughs; the membership recovery probe now holds a group as Quarantined(PendingMembership) after 8 dry attempts (hours of backoff) instead of probing forever — the kind-451 inbox notice remains a live discovery path (it needs no group route), and resolution replays everything retained meanwhile. Tests: quarantine withholds a live commit (epoch frozen, convergence no-op) then resolve consumes it and restores Member at the advanced epoch; quarantine never masks Evicted and resolving a non-quarantined group reports the authoritative state. Full engine/app/account/session suites green; workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(#622) Package F of the #769 follow-ups: membership becomes a set of retained epoch intervals (a group may be left/removed and rejoined), and input from outside every interval classifies as terminal PreMembership. - traits: MembershipInterval { joined_at, ended_at } on the durable Group record (serde-defaulted; snapshot/rollback-coherent like participation); membership_intervals_contain fails OPEN on empty history — errors.md scopes PreMembership to groups with retained membership history, so legacy records never classify it. StaleReason::PreMembership, snapshot-locked, audit tag "pre_membership". - Maintenance rides the participation transitions (one authority, one bookkeeper): create/join open an interval at the current/welcome epoch; a rejoin appends to the retained history instead of overwriting it; ending membership closes the open interval at the removing commit's SOURCE epoch — the last epoch this identity held keys for; post-removal epochs were never readable and stay outside. Quarantine holds touch no interval. - Classification fires ONLY on the too-distant-in-the-past process arm — content this client can never read whose source epoch is readable. Deliberately NOT on the AlreadyAtEpoch arm: a commit older than our join is already represented by the joined state (welcome-before-commit) and keeps its established classification. Terminal: persisted Failed, never retried (unlike deferred MissingRetainedAnchor). Tests: interval-contains unit matrix (empty fails open; join/gap/rejoin; open interval extends forward); the remove→rejoin flow now asserts a message sent inside the removed gap classifies Stale(PreMembership) while welcome-before-commit still classifies AlreadyAtEpoch (fork_detection suite kept green). Full engine + workspace crates green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Package G of the #769 follow-ups: clients can now gate the composer and group list on the local identity's participation. - AppGroupMlsState gains a stable low-cardinality `participation` tag ("member" / "left" / "evicted" / "quarantined_pending_membership" / "quarantined_integrity_hold"), populated from the durable engine record on both the live read and the worker's command-ready snapshot (one builder covers both). Serde default keeps pre-field snapshots decoding as live members. Only "member" allows sending, per the public-surface rule in group-state.md — combined with participation() (pkg A), Ok(None) for "no such group", the API distinguishes every state the spec requires. - group_participation_tag is now the single shared vocabulary in marmot-app (uniffi's event conversion re-exports it instead of duplicating the match), and AppGroupMlsStateFfi mirrors the field. Test: tag stability matrix + legacy-snapshot default. marmot-app + marmot-uniffi suites green; workspace clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
erskingardner
force-pushed
the
feat/group-lifecycle-taxonomy
branch
from
July 2, 2026 18:52
2dbb83a to
aefab2e
Compare
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
3 participants
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.
Part of marmot-protocol/marmot#73 — grew from the group-lifecycle-taxonomy spec PR into the full implementation. No longer "no behavior change": after the spec redesign (removal commit as sole membership authority, discussion in the thread below), all follow-up slices landed here as individually-green, bisectable commits so the repo can reach zero open PRs before the repo move.
Spec (
spec/, commits7e4b3ddc..cc08bd77)Member/Left/Evicted/Quarantined{reason}), orthogonal to the convergence lifecycle.transports/README.mdchecklist (missed-input recovery + notice envelope per binding); kind451registered; membership-intervalPreMembershipinerrors.md.Implementation (one commit per package)
e592debbMemberLeft/MemberRemovedattribution asymmetry), rejoin reinstatement, invariant repair at theUseAfterEviction/is_activearms,GroupEvent::ParticipationChanged,CgkaEngine::participation()b3814effStale(Evicted)classification replacing thePeelFailedcollapse; tombstoned groups no longer misreportUnknownGroup1b18f99a47c3780eTransportAdapter::backfill_account_groupprobe (stored-route pinned, live-subscription-id reuse) driven with exponential backoff from the app2184e5e58818f476Stale(Quarantined)), live convergence excluded,quarantine_group/resolve_group_quarantineas the explicit recovery transition; probe dry-bound →Quarantined(PendingMembership)1adc7162PreMembershipterminal classification on the too-distant-in-the-past arm (welcome-before-commit deliberately keepsAlreadyAtEpoch); empty history fails open2dbb83aeAppGroupMlsState+ FFI as stable tags; composer gating unblocked (whitenoise)marmot-protocol/mdk#376 end-to-end
Cursor can't skip the removal window (C) → probe recovers a missed commit (C) → notice delivers it even across relay partitions (D) → engine transitions participation on either apply path (A) → app tears down subscriptions (A2) and surfaces the state (G). Bounded failure honestly parks in quarantine (E) instead of lying about membership.
Verification
just fast-cigreen; fullcargo test --workspacegreen (103 test binaries), including new coverage at every layer: engine (eviction/left/rejoin/gap-PreMembership/quarantine-withhold-resolve), peeler (notice wrap/peel/tamper), runtime (committer notice publish, backfill routing), adapter (probe reissues stored route), app (cursor gating matrix, probe backoff, teardown). Simulator-tier scenario vectors for the reorder flows are sensible post-move hardening; the flows are currently pinned by the engine-tier integration tests listed above.Summary by CodeRabbit