Skip to content

Fix multi user undo model - #4639

Open
TrueDoctor wants to merge 1 commit into
graph-storage-4-settled-marksfrom
graph-storage-5-exact-undo
Open

TrueDoctor wants to merge 1 commit into
graph-storage-4-settled-marksfrom
graph-storage-5-exact-undo

Conversation

@TrueDoctor

Copy link
Copy Markdown
Member

No description provided.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 9 files

Confidence score: 2/5

  • crdt.rs changes the serialized shape of every Delta, but format v1 still reads history frames as the old shape. Existing documents may not reopen reliably; preserve compatibility when loading v1 history.
  • document.rs removes a peer’s RegisterPeer mapping when undoing its first interaction, so later edits lose that peer’s user identity and same-user undo grouping breaks. Keep the mapping available for subsequent edits.
  • session.rs attributes historical deltas using the peer’s current identity, so reassignment can make the new user appear to own the old user’s edits and undo the wrong work. Use the identity recorded for each historical interaction.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="document/graph-storage/src/crdt.rs">

<violation number="1" location="document/graph-storage/src/crdt.rs:20">
P1: Changing `reverse` changes the serialized shape of every `Delta`, but format v1 still loads history frames directly as `Delta`. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.</violation>
</file>

<file name="document/graph-storage/src/document.rs">

<violation number="1" location="document/graph-storage/src/document.rs:82">
P2: Undoing a peer’s first interaction also removes its `RegisterPeer` mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.</violation>
</file>

<file name="document/graph-storage/src/session.rs">

<violation number="1" location="document/graph-storage/src/session.rs:401">
P2: This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

pub kind: RegistryDelta,
pub reverse: RegistryDelta,
/// What every slot `kind` wrote held before it, stamps included, in write order. See [`Prior`].
pub reverse: Vec<Prior>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Changing reverse changes the serialized shape of every Delta, but format v1 still loads history frames directly as Delta. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/crdt.rs, line 20:

<comment>Changing `reverse` changes the serialized shape of every `Delta`, but format v1 still loads history frames directly as `Delta`. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.</comment>

<file context>
@@ -16,7 +16,8 @@ pub struct Delta {
 	pub kind: RegistryDelta,
-	pub reverse: RegistryDelta,
+	/// What every slot `kind` wrote held before it, stamps included, in write order. See [`Prior`].
+	pub reverse: Vec<Prior>,
 	/// Local, mutable annotations on this commit (interaction-end marker, future commit messages / labels).
 	/// Deliberately excluded from `compute_rev`: relabeling a commit must not change its content-addressed
</file context>

}
/// Undo `delta` by putting back what it overwrote: in the snapshot, and in the working registry when nothing is hot.
pub(crate) fn revert_delta(&mut self, delta: &Delta) {
crate::prior::restore(&mut self.retired_snapshot, &delta.reverse);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Undoing a peer’s first interaction also removes its RegisterPeer mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/document.rs, line 82:

<comment>Undoing a peer’s first interaction also removes its `RegisterPeer` mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.</comment>

<file context>
@@ -78,17 +77,12 @@ impl Document {
-			}
+	/// Undo `delta` by putting back what it overwrote: in the snapshot, and in the working registry when nothing is hot.
+	pub(crate) fn revert_delta(&mut self, delta: &Delta) {
+		crate::prior::restore(&mut self.retired_snapshot, &delta.reverse);
+		if self.hot_log.is_empty() {
+			crate::prior::restore(&mut self.working_registry, &delta.reverse);
</file context>

fn interaction_start_parent(&self, end: Rev, silent: bool) -> Option<Rev> {
let history = &self.document.history;
let mut current = history.get(end)?;
let user = self.user_of(current.author);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/session.rs, line 401:

<comment>This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.</comment>

<file context>
@@ -387,33 +387,29 @@ impl Session {
+	fn interaction_start_parent(&self, end: Rev, silent: bool) -> Option<Rev> {
+		let history = &self.document.history;
+		let mut current = history.get(end)?;
+		let user = self.user_of(current.author);
+		if matches!(current.kind, RegistryDelta::Merge { .. }) || (silent && user != Some(self.document.user)) {
+			return None;
</file context>

This branch was successfully deployed

1 active deployment
graphite-dev (Preview) — 208c86a5 Deployed Oct 5, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant