Navigation is pinned to the retired ktsu.UndoRedo.Core 1.1.4, and moving to ktsu.UndoRedo 3.x makes a failed undo re-run an unrelated action (tests stay green) #82

What's wrong

SimpleUndoRedoProvider (Navigation/Services/SimpleUndoRedoProvider.cs:66-115) recovers from a throwing Undo/Redo on the assumption that UndoRedoService moves its position before it runs the command:

catch
{
	// The service has already moved past the action. Put it back without running it again.
	current.SkipNextExecute();
	_service.Redo();
	throw;
}

The Redo path (lines 105-111) mirrors it with SkipNextUndo() + _service.Undo().

That assumption holds only for ktsu.UndoRedo.Core 1.1.4, the version pinned at Directory.Packages.props:12.

• The package was retired. 1.1.4 was its last release, and the library now ships as ktsu.UndoRedo (2.0.0 onward, currently 3.0.1). Dependabot will never offer the rename, so Navigation stays on 1.1.4 and misses every UndoRedo fix until someone moves it by hand.
• The behaviour changed. ktsu-dev/UndoRedo commit 88ff1d1 ("move the stack position only after Undo/Redo succeeds") ships in 2.x/3.x. A throwing command now leaves the position where it was.

On 2.x/3.x, then, the compensating _service.Redo() acts on a history that never moved:

1. If a redo entry exists, it re-executes the next action, which has nothing to do with the failure, and advances past it.
2. If there is no redo entry, it does nothing, but the SkipNextExecute flag stays set on the failed action. The next real Redo of that action then moves the history without running Execute.

PR #68 said: "If UndoRedoService later restores its position on a throw, the compensation can go and the tests will still hold." The tests do still hold, and all 45 pass after swapping in ktsu.UndoRedo 3.0.1. But the compensation doesn't merely become redundant. It corrupts the history, and no test catches it.

Repro

These probes use the existing IUndoableAction contract:

// Probe 1: failed undo while a redo entry exists
SimpleUndoRedoProvider p = new();
p.RegisterAction(new Rec("a", log), "a");
p.RegisterAction(new Rec("b", log) { FailUndo = true }, "b");
p.RegisterAction(new Rec("c", log), "c");
p.Undo();                       // undo c
try { p.Undo(); } catch { }     // undo b throws

// Probe 2: failed undo, retry, then redo
SimpleUndoRedoProvider p = new();
Rec a = new("a", log) { FailUndo = true };
p.RegisterAction(a, "a");
try { p.Undo(); } catch { }
a.FailUndo = false;
p.Undo();
p.Redo();

Why it matters

Navigation can't take any further UndoRedo fix without changing this adapter. The obvious migration is to swap the package ID and watch the tests stay green, and that would ship these regressions silently.

For a navigation stack, case 1 means a failed undo (for example a throwing NavigationChanged handler, as in #75) sends the user forward to a page they had just undone away from. Case 2 leaves the history saying the user is on a page the stack never returned to.

Suggested fix / acceptance criteria

• Move to ktsu.UndoRedo 3.x and delete both catch compensation blocks along with SkipNextExecute/SkipNextUndo. Keep the skip-first-Execute used during registration.
• If the move has to wait, make the compensation independent of the package version: record _service.CurrentPosition before the call and compensate only if it changed.
• Add tests that pin each of these, so they fail on whichever package behaviour the adapter mishandles:
  • A failed undo while a redo entry exists leaves the redo stack unchanged and runs no other action.
  • After a failed undo and a successful retry, Redo() runs Execute.
  • The mirror cases for a failed redo while an undo entry exists.

Related: #75 (a throwing NavigationChanged handler) is a separate defect, and this adapter issue would make it worse after an upgrade.