fix(undo): preserve partial /redo progress, bound _states, public redo_count bump, clamp /redo

cli.py

@@ -7443,6 +7443,10 @@ def process_command(self, command: str) -> bool:
                 except ValueError:
                     print(f"(._.) Invalid count {_redo_parts[1]!r} — use /redo or /redo N.")
                     return
+                if _redo_n < 1:
+                    # Match /undo: a non-positive count clamps to 1 rather than
+                    # falling through to a misleading "nothing to redo".
+                    _redo_n = 1
             self.redo_last(_redo_n)
         elif canonical == "branch":
             self._handle_branch_command(cmd_original)

hermes_state.py

@@ -3285,6 +3285,23 @@ def _do(conn):
 
         return self._execute_write(_do)
 
+    def bump_redo_count(self, session_id: str) -> None:
+        """Increment ``sessions.redo_count`` by one (once per /redo command).
+
+        Public helper so the shared undo/redo core doesn't reach into the
+        private ``_execute_write``. Counter asymmetry is intentional:
+        ``rewind_count`` bumps per low-level ``rewind_to_message`` call;
+        ``redo_count`` bumps once per /redo command, regardless of M.
+        """
+        def _do(conn):
+            conn.execute(
+                "UPDATE sessions SET redo_count = COALESCE(redo_count, 0) + 1 "
+                "WHERE id = ?",
+                (session_id,),
+            )
+
+        self._execute_write(_do)
+
     def list_recent_user_messages(
         self,
         session_id: str,

hermes_undo.py

@@ -13,6 +13,7 @@
 
 from __future__ import annotations
 
+from collections import OrderedDict
 from dataclasses import dataclass, field
 from typing import Any, Dict, List, Optional
 
@@ -31,7 +32,13 @@ class UndoRedoState:
     redo_stack: List[UndoOp] = field(default_factory=list)
 
 
-_states: Dict[str, UndoRedoState] = {}
+# Bound the in-memory undo/redo holder so a long-running gateway that touches
+# many distinct session_ids doesn't grow _states without limit. The state is
+# purely an in-memory redo branch that already does not survive a restart, so
+# evicting the least-recently-used session is graceful: a later /redo on an
+# evicted session behaves exactly like /redo after a restart (handled in redo()).
+_STATE_CAP = 2048
+_states: "OrderedDict[str, UndoRedoState]" = OrderedDict()
 _session_db: Optional[SessionDB] = None
 
 
@@ -43,7 +50,17 @@ def _get_db() -> SessionDB:
 
 
 def get_state(session_id: str) -> UndoRedoState:
-    return _states.setdefault(session_id, UndoRedoState())
+    state = _states.get(session_id)
+    if state is None:
+        state = UndoRedoState()
+        _states[session_id] = state
+        # Evict the least-recently-used entries beyond the cap.
+        while len(_states) > _STATE_CAP:
+            _states.popitem(last=False)
+    else:
+        # Refresh recency on access (LRU).
+        _states.move_to_end(session_id)
+    return state
 
 
 def clear_state(session_id: Optional[str] = None) -> None:
@@ -182,52 +199,72 @@ def redo(session_id: str, m: int) -> Dict[str, Any]:
         }
 
     reactivated_total = 0
+    ops_redone = 0
+    transcript_changed = False
     for _ in range(k):
         op = state.undo_stack.pop()
         reactivated = db.restore_ids(session_id, op.rewound_ids)
         if reactivated == 0 and op.rewound_ids:
             # NONE of this op's rows could be restored — the transcript was
             # rewritten out from under the stack (/compress, /retry, and any
             # other replace_messages flow hard-delete + renumber rows). The redo
-            # branch is meaningless now, so discard the whole stack rather than
-            # raising: redo across a transcript rewrite is impossible, same as
-            # redo after a restart.
-            state.undo_stack.clear()
-            state.redo_stack.clear()
-            return {
-                "reactivated_count": 0,
-                "new_tail_id": None,
-                "prefill_text": None,
-                "message": "nothing to redo (transcript changed since undo)",
-            }
+            # branch is dead from here on, so stop and discard the rest of the
+            # stack rather than raising: redo across a transcript rewrite is
+            # impossible, same as redo after a restart.
+            #
+            # Crucially, do NOT throw away progress from EARLIER ops in this same
+            # /redo. If we already reactivated rows (reactivated_total > 0), those
+            # rows are live in the DB now; returning reactivated_count:0 here
+            # would make the caller skip its history reload (screen/DB desync)
+            # and leave redo_count un-bumped despite real work. Break and report
+            # the partial result honestly instead.
+            transcript_changed = True
+            break
         if reactivated != len(op.rewound_ids):
-            # PARTIAL restore — some rows came back, some didn't. This is a
-            # genuine desync (not a clean transcript rewrite), so fail loud to
-            # surface latent corruption rather than silently half-redoing.
+            # PARTIAL restore of a SINGLE op — some rows came back, some didn't.
+            # This is a genuine desync (not a clean transcript rewrite), so fail
+            # loud to surface latent corruption rather than silently half-redoing.
             raise RuntimeError(
                 "redo invariant violated: restored "
                 f"{reactivated} of {len(op.rewound_ids)} rewound rows"
             )
         reactivated_total += reactivated
+        ops_redone += 1
         state.redo_stack.append(op)
 
+    if transcript_changed:
+        # The remaining (older) ops can't be redone across the rewrite — drop them.
+        state.undo_stack.clear()
+        if reactivated_total == 0:
+            # No work done at all → pure no-op; leave redo_count untouched and
+            # clear the now-meaningless redo history too.
+            state.redo_stack.clear()
+            return {
+                "reactivated_count": 0,
+                "new_tail_id": None,
+                "prefill_text": None,
+                "message": "nothing to redo (transcript changed since undo)",
+            }
+        # else: earlier ops did real work — fall through to commit + report it.
+
     # Counter asymmetry is intentional: rewind_count increments per low-level
-    # rewind_to_message call; redo_count increments once per /redo command.
-    db._execute_write(
-        lambda conn: conn.execute(
-            "UPDATE sessions SET redo_count = COALESCE(redo_count, 0) + 1 "
-            "WHERE id = ?",
-            (session_id,),
-        )
-    )
+    # rewind_to_message call; redo_count increments once per /redo command that
+    # did real work (regardless of M).
+    db.bump_redo_count(session_id)
 
     active_after = db.get_messages(session_id, include_inactive=False)
     tail = _new_tail(active_after)
-    return {
+    result: Dict[str, Any] = {
         "reactivated_count": reactivated_total,
         "new_tail_id": tail["id"] if tail else None,
         "prefill_text": None,
     }
+    if transcript_changed:
+        result["message"] = (
+            f"redid {ops_redone} operation(s); the rest can't be redone "
+            "(transcript changed since undo)"
+        )
+    return result
 
 
 def on_user_message_appended(session_id: str) -> None:

tests/cli/test_undo_redo_half_turn.py

@@ -106,3 +106,36 @@ def test_clear_redo_on_send_leaves_undo_stack_available(db):
 
     assert state.redo_stack == []
     assert state.undo_stack == []
+
+
+@pytest.mark.parametrize(
+    "command, expected_n",
+    [
+        ("/redo", 1),
+        ("/redo 2", 2),
+        ("/redo 0", 1),    # non-positive clamps to 1 (parity with /undo)
+        ("/redo -3", 1),   # negative clamps to 1
+    ],
+)
+def test_redo_command_clamps_non_positive_count_to_one(command, expected_n):
+    """/redo 0 and /redo -N must clamp to 1 like /undo, not fall through to a
+    misleading 'nothing to redo' (Greptile PR #49 finding)."""
+    captured = {}
+    cli_obj = SimpleNamespace(
+        _pending_resume_sessions=None,
+        redo_last=lambda n=1: captured.__setitem__("n", n),
+    )
+    cont = HermesCLI.process_command(cli_obj, command)
+    assert cont is True
+    assert captured.get("n") == expected_n
+
+
+def test_redo_command_rejects_non_numeric_count():
+    """A non-numeric /redo argument is rejected, not silently treated as 1."""
+    captured = {}
+    cli_obj = SimpleNamespace(
+        _pending_resume_sessions=None,
+        redo_last=lambda n=1: captured.__setitem__("n", n),
+    )
+    HermesCLI.process_command(cli_obj, "/redo abc")
+    assert "n" not in captured  # redo_last never called on a parse error

tests/test_undo_redo_stack.py

@@ -348,3 +348,82 @@ def partial_restore(_sid, rewound_ids):
     with pytest.raises(RuntimeError, match="restored 1 of 2 rewound rows"):
         hermes_undo.redo(sid, 1)
     assert calls == [[ids[2], ids[3]]]
+
+
+def test_multi_redo_preserves_earlier_progress_when_later_op_hits_rewrite(monkeypatch, db):
+    """A /redo N must not throw away rows an EARLIER op already reactivated.
+
+    Bug (Greptile, merged PR #49): in a multi-op /redo, the first op pops and
+    restores rows (real DB work, redo_stack grows), then a later op hits the
+    transcript-rewrite path (restore_ids -> 0). The old code discarded the whole
+    stack and returned reactivated_count:0, so the caller printed "Nothing to
+    redo" and SKIPPED its history reload — screen/DB desync — and redo_count was
+    never bumped despite the committed first op. The fix reports the partial
+    progress honestly and still commits it.
+    """
+    sid = _make_session(db)
+    ids = _seed_three_half_turns(db, sid)  # u1,a1,u2,a2
+    # Two single-half-turn undos -> two ops on the stack (LIFO: a2 then u2 group).
+    hermes_undo.undo(sid, 1)
+    hermes_undo.undo(sid, 1)
+    state = hermes_undo.get_state(sid)
+    assert len(state.undo_stack) == 2
+    first_op_ids = list(state.undo_stack[-1].rewound_ids)  # the op redo pops FIRST
+
+    real_restore = db.restore_ids
+    seen = {"n": 0}
+
+    def restore_first_then_rewrite(_sid, rewound_ids):
+        # First op: genuinely restore. Second op: simulate transcript rewrite.
+        seen["n"] += 1
+        if seen["n"] == 1:
+            return real_restore(_sid, rewound_ids)
+        return 0
+
+    monkeypatch.setattr(db, "restore_ids", restore_first_then_rewrite)
+
+    r = hermes_undo.redo(sid, 2)
+
+    # Earlier op's work is preserved and reported, not silently dropped.
+    assert r["reactivated_count"] == len(first_op_ids)
+    assert r["reactivated_count"] > 0
+    assert "transcript changed" in r["message"]
+    # The op that succeeded moved to redo_stack; the dead remainder is dropped.
+    assert len(state.redo_stack) == 1
+    assert state.undo_stack == []
+    # redo_count WAS bumped because real work committed (not left at None).
+    assert db.get_session(sid)["redo_count"] == 1
+
+
+def test_redo_uses_public_bump_redo_count_not_private_execute_write():
+    """The shared core must call the public SessionDB.bump_redo_count helper,
+    not reach into the private _execute_write (Greptile PR #49 finding)."""
+    source = inspect.getsource(hermes_undo)
+    assert "bump_redo_count" in source
+    assert "_execute_write" not in source, (
+        "hermes_undo must not access the private SessionDB._execute_write"
+    )
+    # And the helper actually exists on SessionDB.
+    assert callable(getattr(SessionDB, "bump_redo_count", None))
+
+
+def test_states_holder_is_lru_bounded(monkeypatch):
+    """_states must evict least-recently-used sessions instead of growing forever
+    (Greptile PR #49: unbounded module-global dict = gateway memory leak)."""
+    hermes_undo.clear_state()
+    monkeypatch.setattr(hermes_undo, "_STATE_CAP", 3)
+    try:
+        for i in range(5):
+            hermes_undo.get_state(f"s{i}")
+        assert len(hermes_undo._states) == 3
+        # The three most-recent survive; the two oldest were evicted.
+        assert set(hermes_undo._states.keys()) == {"s2", "s3", "s4"}
+
+        # Accessing an existing session refreshes its recency (LRU, not FIFO).
+        hermes_undo.get_state("s2")          # s2 -> most recent
+        hermes_undo.get_state("s5")          # inserts s5, evicts the now-oldest (s3)
+        assert "s2" in hermes_undo._states
+        assert "s3" not in hermes_undo._states
+        assert len(hermes_undo._states) == 3
+    finally:
+        hermes_undo.clear_state()