From 2f8341c1c6bd11acc03d2fa1dccb11cf8b429ad8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 18:25:17 +0000 Subject: [PATCH 1/2] fix: make the non-merge path in Execute() exception-safe [patch] Execute() cleared forward history and cleaned up save boundaries before calling command.Execute(). When Execute() threw, the exception propagated but the redo history and any save boundaries inside it had already been discarded, even though the new command was never applied. Move command.Execute() ahead of ClearForward() and CleanupInvalidBoundaries(), mirroring the auto-merge path above and CompositeCommand.Execute(). Behaviour on success is unchanged: CurrentPosition is untouched by Execute(), so CleanupInvalidBoundaries() still receives the same position, and AddCommand() already clears forward history itself when branching. Adds two tests, each failing before the change: - Execute_CommandThrowsException_PreservesRedoHistory - Execute_CommandThrowsException_PreservesSaveBoundariesInForwardHistory Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T1ntzPwg732TrH7D5vWpSV --- UndoRedo.Test/UndoRedoStackTests.cs | 56 ++++++++++++++++++++++++++++ UndoRedo/Services/UndoRedoService.cs | 8 ++-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index 6ec924e..042f75b 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -443,6 +443,62 @@ public void Execute_CommandThrowsException_DoesNotCorruptStack() Assert.IsTrue(stack.CanUndo, "CanUndo should remain true after failed command execution"); } + [TestMethod] + public void Execute_CommandThrowsException_PreservesRedoHistory() + { + // Arrange: three commands, then undo twice so B and C are available to redo + UndoRedoService stack = CreateService(); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); + stack.Execute(new DelegateCommand("C", () => value = 3, () => value = 2)); + + stack.Undo(); + stack.Undo(); + Assert.AreEqual(1, value); + Assert.IsTrue(stack.CanRedo, "B and C should be available to redo before the failing command"); + + // Act: a command that fails to apply must not branch the stack + Assert.ThrowsExactly(() => + stack.Execute(new DelegateCommand("Bad Command", () => throw new InvalidOperationException(), () => { }))); + + // Assert: nothing was applied, so nothing may have been discarded + Assert.AreEqual(3, stack.CommandCount, "A command that failed to apply must not discard the forward history"); + Assert.IsTrue(stack.CanRedo, "CanRedo should remain true after a failed command execution"); + Assert.AreEqual(1, value, "The failed command must not have changed the application state"); + + // And the preserved history must still be replayable + stack.Redo(); + Assert.AreEqual(2, value, "Redo after a failed command must reapply B"); + stack.Redo(); + Assert.AreEqual(3, value, "Redo after a failed command must reapply C"); + Assert.IsFalse(stack.CanRedo, "The stack should be fully redone after replaying both preserved commands"); + } + + [TestMethod] + public void Execute_CommandThrowsException_PreservesSaveBoundariesInForwardHistory() + { + // Arrange: a save boundary that lives inside the forward history + UndoRedoService stack = CreateService(); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); + stack.MarkAsSaved("saved at B"); + stack.Undo(); + + Assert.AreEqual(1, value); + Assert.AreEqual(1, stack.SaveBoundaries.Count, "The save boundary should exist before the failing command"); + + // Act + Assert.ThrowsExactly(() => + stack.Execute(new DelegateCommand("Bad Command", () => throw new InvalidOperationException(), () => { }))); + + // Assert: the boundary only becomes invalid once the branch actually happens + Assert.AreEqual(1, stack.SaveBoundaries.Count, "A command that failed to apply must not invalidate save boundaries"); + } + [TestMethod] public void Execute_MergedCommandThrowsException_DoesNotCorruptStack() { diff --git a/UndoRedo/Services/UndoRedoService.cs b/UndoRedo/Services/UndoRedoService.cs index d3cad05..0eb0f14 100644 --- a/UndoRedo/Services/UndoRedoService.cs +++ b/UndoRedo/Services/UndoRedoService.cs @@ -121,13 +121,15 @@ public void Execute(ICommand command) } } + // Apply the real state change before discarding anything, so a failure here can never throw + // away redo history for a command that was not actually applied. This mirrors the merge path + // above and CompositeCommand.Execute(). + command.Execute(); + // Clear any commands after the current position and cleanup save boundaries _stackManager.ClearForward(); _saveBoundaryManager.CleanupInvalidBoundaries(_stackManager.CurrentPosition); - // Execute the command - command.Execute(); - // Add to stack _stackManager.AddCommand(command); From 6e1e26a7fa4472226e9ad4e59f42a0a870049cc0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 18:43:50 +0000 Subject: [PATCH 2/2] test: assert save boundary count with Assert.HasCount [patch] MSTEST0037, reported by SonarQube Cloud on this PR: use Assert.HasCount rather than Assert.AreEqual on a collection's Count. The rest of the suite already asserts save boundaries this way. Assertion strength is unchanged: both new tests still fail against the pre-fix UndoRedoService with the same messages. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T1ntzPwg732TrH7D5vWpSV --- UndoRedo.Test/UndoRedoStackTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index 042f75b..0cdee0d 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -489,14 +489,14 @@ public void Execute_CommandThrowsException_PreservesSaveBoundariesInForwardHisto stack.Undo(); Assert.AreEqual(1, value); - Assert.AreEqual(1, stack.SaveBoundaries.Count, "The save boundary should exist before the failing command"); + Assert.HasCount(1, stack.SaveBoundaries, "The save boundary should exist before the failing command"); // Act Assert.ThrowsExactly(() => stack.Execute(new DelegateCommand("Bad Command", () => throw new InvalidOperationException(), () => { }))); // Assert: the boundary only becomes invalid once the branch actually happens - Assert.AreEqual(1, stack.SaveBoundaries.Count, "A command that failed to apply must not invalidate save boundaries"); + Assert.HasCount(1, stack.SaveBoundaries, "A command that failed to apply must not invalidate save boundaries"); } [TestMethod]