Undo-block review fixes: exception-safe depth, corrected docs, real test teeth

Fixes ctor exception safety, a ValidatePtr2 guard, false SDK-citation and
enforcement claims, the span-depth invariant, and vacuous fx_reclaim tests.

src/shell/bank_ops/CLAUDE.md

@@ -33,7 +33,13 @@ calls into that model, it does not reimplement its rules.
   a Design View mode switch's FX reclaim span is open: without it the verb's own
   close landed at depth 2->1, minted nothing, and the rename or delete was absorbed
   into the mode switch, un-undoable on its own. Reason and mechanism:
-  `src/shell/undo/CLAUDE.md`.
+  `src/shell/undo/CLAUDE.md`. ACCEPTED, same cost `view_lanes`' `mintManagedLanes`
+  already carries (`src/shell/view/view_lanes.cpp`'s note at its own `UndoBlock`):
+  opening `persistBankOp`'s block ends any pending reclaim span unconditionally,
+  including on the unsaved-project path above, which then records no point of its
+  own — the switch's point still mints early, and its remaining parking mints again
+  under `kReclaimUndoLabel`. Bounded at one extra undo point; the alternative
+  (nesting over the span) destroys the switch's point outright.
 
 ## Modules
 

src/shell/bank_ops/bank_ops.cpp

@@ -44,15 +44,14 @@ std::string mintBankId() {
 // ext-state is the block's "after" image.
 //
 // UNSAVED-PROJECT GUARDRAIL: on an unsaved/no-active project saveToActiveProject()
-// no-ops; we still CLOSE the block, but with an empty label + zero flag so REAPER
+// no-ops, so recordPoint below is skipped — the block closes on its own default
-// discards the point instead of recording a no-effect undo entry. Quiet persist by
+// discard form (empty label, zero flag) instead of recording a no-effect undo entry.
-// design (mirrors capture, not Design-View) — deliberately no Save-As prompt.
+// Quiet persist by design (mirrors capture, not Design-View) — deliberately no
+// Save-As prompt.
 void persistBankOp(ReaSamplerSession& session, const char* label,
                    bool bumpGeneration) {
     UndoBlock undo(nullptr);
     if (bumpGeneration) session.bumpBankGeneration();
-    // No ext-state write -> record no point; the block's default discard form is what
-    // keeps an unsaved project from collecting no-effect undo entries.
     if (session.saveToActiveProject()) undo.recordPoint(label, UNDO_STATE_MISCCFG);
 }
 

src/shell/undo/CLAUDE.md

@@ -11,13 +11,19 @@ nests over it.
 
 ## Invariants
 
-- **`Undo_BeginBlock2` is not callable outside this directory, and that is enforced
+- **`Undo_BeginBlock2` is not callable outside this directory in every TU but one, and
-  by the compiler, not by review.** Only `undo_block.cpp` and `shell/view`'s
+  that is enforced by the compiler, not by review.** Only `undo_block.cpp` and
-  `fx_reclaim.cpp` (which owns the span's own begin/end) define
+  `shell/view`'s `fx_reclaim.cpp` (which owns the span's own begin/end) define
-  `REAPERAPI_WANT_Undo_BeginBlock2` / `..._Undo_EndBlock2`. Every other TU compiles
+  `REAPERAPI_WANT_Undo_BeginBlock2` / `..._Undo_EndBlock2`; every other TU compiles
-  under `REAPERAPI_MINIMAL` with no header in its include graph pulling the SDK in
+  under `REAPERAPI_MINIMAL`, so a raw `Undo_BeginBlock2` there is an undeclared
-  unguarded, so a raw `Undo_BeginBlock2` there is an undeclared identifier. Adding
+  identifier. **`app/main.cpp` is the one exception** — as the sole
-  the WANT define back to a third TU is the one way to defeat this — do not.
+  `REAPERAPI_IMPLEMENT` TU it never sets `REAPERAPI_MINIMAL` (it must allocate storage
+  for whatever ANY TU wants), so the whole API, `Undo_*` included, compiles clean
+  there too. That TU stays out of the undo business by discipline
+  (`src/app/CLAUDE.md`: pointers + entry + dispatch only), not because the compiler
+  stops it — do not add business logic there that would make this matter. Adding the
+  WANT define back to a third TU is a second, easier way to defeat the enforcement
+  that DOES exist — do not.
 - **Ending the reclaim span is part of opening, and ONLY the outermost block does
   it.** At depth 0 the span is the only block that can be open, so ending it mints
   the pending mode switch's point whole before this block exists. At depth > 0 an
@@ -26,6 +32,12 @@ nests over it.
   is the same criterion `applyMode`'s reapply is gated on; it lives in the
   constructor so no call site has to make the judgment. Full reasoning for the rule
   itself: `src/shell/view/CLAUDE.md`.
+- **The constructor's body is one `try`.** `fxReclaimEndUndoSpan` copies a
+  `std::string` and can throw; a throwing constructor runs no destructor, so an
+  uncompensated depth increment would stick forever — `undoBlockOpen()` reads true
+  for the rest of the session, `fxReclaimTick` refuses every tick, and no later
+  `UndoBlock` is ever outermost. Any throw between the increment and the end of the
+  constructor rolls the depth back before propagating.
 - **Closing is a destructor.** No throw between the open and the label decision can
   leave a block open in the user's project, which is why several call sites can
   allocate freely between the two.
@@ -46,6 +58,15 @@ nests over it.
 
 ## Gotchas
 
+- **`Undo_EndBlock2` is guarded by `ValidatePtr2` only when `project_` is a real,
+  non-null pointer.** `nullptr` means "the active project" — the same convention
+  `Undo_BeginBlock2` was handed — and `ValidatePtr2` does not accept `nullptr` as a
+  valid object of any type, so guarding it unconditionally would silently stop every
+  `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real pointer, by
+  contrast, can go stale between open and close (several call sites allocate freely
+  in between), so only that case is validated — the same asymmetry `fx_reclaim.cpp`'s
+  `closeSpan` already has, for the same reason: its span always holds a real pinned
+  pointer, never `nullptr` for "active".
 - The depth counter deliberately does NOT count the reclaim span. The span is the
   thing this stands off, not a peer.
 - The counter drops AFTER `Undo_EndBlock2`, not before: the close that reaches depth

src/shell/undo/undo_block.cpp

@@ -10,6 +10,7 @@
 #define REAPERAPI_MINIMAL
 #define REAPERAPI_WANT_Undo_BeginBlock2
 #define REAPERAPI_WANT_Undo_EndBlock2
+#define REAPERAPI_WANT_ValidatePtr2
 #include "reaper_plugin_functions.h"
 
 namespace reasampler {
@@ -31,14 +32,35 @@ UndoBlock::UndoBlock(ReaProject* proj) : project_(proj) {
     // enclosing UndoBlock already did it, and a close from here would land at depth
     // 2->1 — minting nothing and hijacking the enclosing block's label instead. That is
     // the same criterion applyMode's reapply is gated on.
+    //
+    // The whole body is one try: fxReclaimEndUndoSpan's closeSpan copies a std::string
+    // and can throw. A throwing constructor runs no destructor, so an uncompensated
+    // ++g_depth would stick forever — undoBlockOpen() reads true for the rest of the
+    // session, fxReclaimTick refuses every tick, and no later UndoBlock is ever
+    // outermost. Roll the depth back on any throw so the type degrades to "this one
+    // block failed to open," not "every block after it is now poisoned."
+    try {
         if (g_depth++ == 0) fxReclaimEndUndoSpan();
         Undo_BeginBlock2(proj);
+    } catch (...) {
+        --g_depth;
+        throw;
+    }
 }
 
 UndoBlock::~UndoBlock() {
     // Depth drops AFTER the SDK call, not before: the close that reaches depth 0 mints a
     // point, and anything re-entered through that must still read a block as open rather
     // than let the drain open a span underneath it.
+    //
+    // Guarded the same way fx_reclaim.cpp's closeSpan guards its own Undo_EndBlock2, but
+    // only for a REAL pinned pointer: `project_ == nullptr` means "the active project",
+    // the same convention Undo_BeginBlock2 was handed, and ValidatePtr2 does not accept
+    // nullptr as a valid object of any type — guarding it unconditionally would silently
+    // stop every `UndoBlock undo(nullptr)` site (most of them) from ever closing. A real
+    // pointer, by contrast, can go stale: several call sites allocate freely between open
+    // and close, and the project it named may have closed underneath by now.
+    if (!project_ || ValidatePtr2(nullptr, project_, "ReaProject*"))
         Undo_EndBlock2(project_, label_.c_str(), mask_);
     --g_depth;
 }

src/shell/undo/undo_block.h

@@ -3,9 +3,12 @@
 // opening, so no block can ever nest over it, and the close is a destructor, so no throw
 // can leave one open in the user's project.
 //
-// Enforced by the SDK's own opt-in: only this TU and fx_reclaim.cpp (which owns the span
+// The SDK's WANT opt-in stops a raw Undo_BeginBlock2/_EndBlock2 call in every OTHER TU —
-// itself) define REAPERAPI_WANT_Undo_BeginBlock2, so `Undo_BeginBlock2` does not even
+// only this file and fx_reclaim.cpp (the span's own owner) request them, so the symbol is
-// name a declared symbol anywhere else in the tree.
+// undeclared elsewhere. app/main.cpp is the one exception: the sole REAPERAPI_IMPLEMENT TU
+// cannot set REAPERAPI_MINIMAL (it must allocate storage for whatever ANY TU wants), so a
+// raw call compiles clean there too — kept out by discipline (src/app/CLAUDE.md), not by
+// the compiler.
 
 #include <string>
 
@@ -16,8 +19,10 @@ namespace reasampler {
 
 class UndoBlock {
 public:
-    // `proj` is nullptr for the active project, matching Undo_BeginBlock2's own contract
+    // `proj` is nullptr for the active project — the general REAPER convention for a
-    // (reaper_plugin_functions.h:7753).
+    // ReaProject* parameter. reaper_plugin_functions.h documents no proj semantics at all
+    // for Undo_BeginBlock2/_EndBlock2 (the line once cited here was the #if WANT guard,
+    // not a doc comment). `[verify — DAW]`
     explicit UndoBlock(ReaProject* proj);
     ~UndoBlock();
 

src/shell/view/CLAUDE.md

@@ -159,6 +159,15 @@ decide membership or mode rules.
   The mirror half is still `applyMode`'s: a REAPPLY never closes a span, precisely
   because it is reachable from inside one of those blocks — only a real switch
   supersedes, and a real switch is always fired at depth 0.
+  **Consequently, a span is never open at depth > 0.** The only two openers are a real
+  switch (always fired straight from an action handler, never from inside another
+  block) and the reclaimer's own lazy `writeOffline`, which the residual above makes
+  unreachable at depth > 0. This is what lets `fxReclaimAbandonForStateLoad` and
+  `fxReclaimFlushRelease` close the span unconditionally with no depth check of their
+  own, even when the hook that calls them fires from inside an extension `UndoBlock` —
+  a capture's own save can (`capture_orchestrator`'s `RunCaptureItemAssign` wraps
+  `RunCapture`, which reaches `Main_SaveProject`). `closeSpan()` there just finds
+  nothing open.
   ACCEPTED, and not fixed: `mintManagedLanes` opens its block once its plan is
   non-empty, but a non-empty plan whose writes are all already satisfied still mints
   nothing — so a background detect tick can end a switch's span having competed with it

src/shell/view/fx_reclaim.cpp

@@ -221,6 +221,11 @@ FxReclaimActivity fxReclaimActivity() {
     // The pin is only re-tested at beginTick; a poll landing between two ticks can find the
     // project already gone, and GetPlayStateEx is about to be handed this pointer.
     if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return FxReclaimActivity::Idle;
+    // A FOURTH stand-off, alongside no-project/invalid-project/transport: fxReclaimTick
+    // refuses the tick outright while undoBlockOpen(), so nothing may move for as long as
+    // one is open — the same "Idle whenever nothing may move" promise this reports for the
+    // debounce hold and the transport gate.
+    if (undoBlockOpen()) return FxReclaimActivity::Idle;
     return g_queue.activity(monotonicMs(), transportBlocksModeSwitch(proj));
 }
 
@@ -247,7 +252,10 @@ void fxReclaimTick(ReaSamplerSession& session, ReaProject* proj) {
     // Refused whole rather than written unspanned, which would instead absorb the FX
     // toggles into the user's act. Starves nothing: the wakeup, the derived queue and the
     // project pin are all untouched (this returns before beginTick), so the first tick
-    // after the block closes drains exactly as if this one had never been due.
+    // after the block closes drains exactly as if this one had never been due. That last
+    // claim is this function's own control-flow order — this TU is REAPER-linked, off the
+    // pure fx_reclaim_tests target, so unlike the queue/span state machine above it, the
+    // ordering is DAW-verified, not unit-tested.
     if (undoBlockOpen()) return;
     DrainLatch latch;
 
@@ -293,7 +301,11 @@ void fxReclaimEndUndoSpan() { closeSpan(); }
 void fxReclaimAbandonForStateLoad() {
     // Close FIRST and unconditionally. The abandon below stops the drain and puts the loop
     // to sleep; the block is what a Ctrl-Z would otherwise close over the wrong state, and
-    // it is open whether or not a drain is on the stack at all.
+    // it is open whether or not a drain is on the stack at all. Safe even if this hook were
+    // reachable from inside an extension UndoBlock (see fxReclaimFlushRelease below for a
+    // confirmed case of that shape on the save side) — the span can never be open at depth
+    // > 0 in the first place: src/shell/view/CLAUDE.md's "NOTHING MAY OPEN AN UNDO BLOCK
+    // WHILE THE SPAN IS OPEN" invariant. closeSpan() just finds nothing to close there.
     closeSpan();
     g_queue.abandon();
 }
@@ -307,7 +319,9 @@ void fxReclaimFlushRelease(ReaSamplerSession& session) {
     // different project saved and write into that wrong project's ext state.
     ReaProject* saving = GetCurrentProjectInLoadSave();
     // BEFORE the arm test, and unconditionally: a save is a user act the span must not
-    // absorb, and the arm is not what decides whether one is open.
+    // absorb, and the arm is not what decides whether one is open. Same "depth > 0 never
+    // holds an open span" invariant as fxReclaimAbandonForStateLoad above makes this safe
+    // even when the save itself is issued from inside an extension UndoBlock.
     closeSpan();
     if (!g_releaseFlush.pendingFor(saving)) return;
     // Promptless, unlike design_view_actions' persistViewState, which can open a modal

tests/test_fx_reclaim.cpp

@@ -1478,7 +1478,19 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() {
 
     // 2. The state-load terminator. `abandon` is the one move that clears the wakeup with
     //    work still owed, which is what makes the close-FIRST ordering load-bearing rather
-    //    than tidy — reverse the two and the loop is asleep under an open block.
+    //    than tidy. Proved both ways, not asserted by construction: the WRONG order below
+    //    (abandon, then close) is caught mid-sequence by the same predicate the RIGHT order
+    //    is checked against — swapping the two lines here would make the first CHECK below
+    //    fail, which is the teeth `fxReclaimAbandonForStateLoad` closing FIRST relies on.
+    {
+        FxReclaimQueue q;
+        UndoSpan       span;
+        q.invalidate();
+        span.begin(kProjA, "ReaSampler: activate Design view", 9);
+        q.abandon();  // WRONG order: wakeup cleared before the span closes
+        CHECK(asleepWithTheSpanOpen(q, span));  // the exact leak the real order avoids
+        span.end();
+    }
     {
         FxReclaimQueue q;
         UndoSpan       span;
@@ -1492,8 +1504,13 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() {
 
     // 3. The project moving out from under the span. No active project sleeps the loop
     //    (beginTick's nullptr arm), a tab switch does not — but both close, because the
-    //    span is keyed on its OWN project and neither one is it.
+    //    span is keyed on its OWN project and neither one is it. kProjA is the control: a
-    for (ReaProject* arrived : {static_cast<ReaProject*>(nullptr), kProjB}) {
+    //    tick landing back on the SAME project must NOT close the span, or a live switch's
+    //    block would drop the flags-and-items mask it opened with mid-drain. Sweeping all
+    //    three, with a branch-dependent CHECK, is what gives the guard real teeth — the
+    //    original two-value sweep always took the close branch, so the no-close branch was
+    //    never exercised.
+    for (ReaProject* arrived : {kProjA, static_cast<ReaProject*>(nullptr), kProjB}) {
         FxReclaimQueue q;
         UndoSpan       span;
         q.invalidate();
@@ -1503,21 +1520,16 @@ static void testNoTerminatorLeavesTheSpanOpenWithTheLoopAsleep() {
         if (span.project() != arrived) span.end();
         q.beginTick(arrived);
 
-        CHECK(!span.open());
+        if (arrived == kProjA) CHECK(span.open());   // same project: nothing to close
+        else                   CHECK(!span.open());
         CHECK(!asleepWithTheSpanOpen(q, span));
     }
 
     // 4. The terminators with no queue move at all — a project save, an extension unload,
-    //    the next real switch superseding. Each is the idempotent close alone, so the
+    //    the next real switch superseding. Each is the idempotent close alone, with no
-    //    wakeup is whatever it already was; the implication has to hold either way.
+    //    guard and nothing else to sweep, so it reduces to end()'s own idempotency
-    for (bool woken : {false, true}) {
+    //    (testTheSpanOpensOnceAndClosesOnceHoweverManyTimesItIsAsked) rather than adding a
-        FxReclaimQueue q;
+    //    separate claim here.
-        UndoSpan       span;
-        if (woken) q.invalidate();
-        span.begin(kProjA, "ReaSampler: activate Design view", 9);
-        span.end();
-        CHECK(!asleepWithTheSpanOpen(q, span));
-    }
 }
 
 int main() {