Ψ-W1-T4 remediation: gate silent arrange drops, pin surface totality, hedge unverified SDK claims, close comment/exception-safety minors

src/core/ui/drag_out.cpp

@@ -15,6 +15,10 @@ bool insideClient(int px, int py, const PanelClientRect& c) {
 
 } // namespace
 
+bool needsSurfaceProbe(int px, int py, const PanelClientRect& client) {
+    return !insideClient(px, py, client);
+}
+
 DropClass decideDropClass(int px, int py, const PanelClientRect& client,
                           const DropContext& ctx) {
     if (!ctx.drag.dragging || !ctx.drag.hasArmedSamples) return DropClass::None;

src/core/ui/drag_out.h

@@ -40,6 +40,9 @@ enum class ReaperSurface {
     FxEmbed,     // tcp.fxembed / mcp.fxembed — an instance already draws there
     Arrange,     // the timeline
     Other,       // ruler, transport, spacers, docker chrome, unknown future tokens
+    Count,       // sentinel, NOT a real surface — tests/test_drag_out.cpp's kAllSurfaces is
+                 // pinned against this via static_assert so a 7th surface can't silently skip
+                 // the exhaustiveness matrix
 };
 
 // The resolved target class. Every value is a DEFINED outcome the shell executes or visibly
@@ -67,6 +70,11 @@ struct DropContext {
 // so re-entry into the client always returns Internal and a surface transition always reverses.
 DropClass decideDropClass(int px, int py, const PanelClientRect& client, const DropContext& ctx);
 
+// True outside the panel client rect — the same half-open test decideDropClass gates the SDK
+// hit-test on. Exported so the shell reads this one pure predicate instead of reimplementing the
+// inside-client math inline (core/ui/CLAUDE.md forbids hit-test geometry living in shell code).
+bool needsSurfaceProbe(int px, int py, const PanelClientRect& client);
+
 // The pointer cue for a class, so "will this work" is visible BEFORE release. Internal defers
 // to card_drag's own cue; OsOwned means the OS drag loop draws the cursor and we must not fight
 // it.

src/shell/actions/CLAUDE.md

@@ -39,7 +39,7 @@ is owned by other directories and only skinned here.
 - `shell/actions` (`action_registry` / `design_view_actions` / `bank_actions` / `prune_action`) — the bindable action families, all routed via the `command_id`/`gaccel`/`hookcommand` contract. `action_registry` owns the shared registration plumbing (interned channel-qualified id strings; register and mirror-unregister present the identical pointer) **and the Q-W6 registration TABLE**: `main.cpp`'s own family (capture scopes, panel toggle, insert, batch, realtime, recapture, version) is one `ActionTableRow` array — suffix, phrase, flat function-pointer handler — that registration, hookcommand dispatch, and the unload mirror-unregister all iterate, so adding an action touches the table only (OCP). Bank mutations flow through the promptless `shell/bank_ops` verbs (`bankOp*` + `persistBankOp`, taking `ReaSamplerSession&`), which the panel menus and `bank_actions` consume as thin UX skins. **Every bank index verb wraps its mutation in a batched REAPER undo point (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) so one bank operation is one Ctrl-Z.** The prune action (`prune_action`, `BANK_PRUNE_FOLDER`) is **the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). `BANK_PRUNE_FOLDER` halts on `blockedByTracking` and prints each blocker that fired, with recovery instructions.
 - `drag_out_win` — OS drag-out shell: Windows OLE `DoDragDrop`/`CF_HDROP`, copy-only (`DROPEFFECT_MOVE` not offered); macOS/Linux via `SWELL_InitiateDragDropOfFileList`.
 - `instrument_drop_win` — instrument-drop shell: `probeDropTarget` resolves a screen point to a track + a `ReaperSurface` (via the pure `classifyReaperSurface`, which owns every token rule), and the drop half adds a ReaSampler 9000 instance and applies the dragged capture's state via a transient `.vstpreset` + `TrackFX_SetPreset` (the former `TrackFX_SetNamedConfigParm` "vst_chunk" write was silently unappliable for VST3). Exposes `loadInstrumentOntoTrack` (inner half, no own undo block) and `performInstrumentDrop` (wraps in its own undo block). **Never captures, never writes the bank, never inserts a timeline item.**
-- `arrange_drop_win` — the drag-out gesture's arrange outcome: `arrangeTimeAtScreenX` (pointer column → time via `GetSet_ArrangeView2` over a one-pixel span) and `performArrangeDrop` (snap the drop time, then one `InsertMedia` per capture on the pointer's track, laid out sequentially, in ONE undo block, with the caller's track selection and edit cursor restored). This is the one shell in this directory that DOES place timeline items — and it is legitimate because the user dragged them there, the same class of act as `RunInsertSelected`. It never captures and never writes the bank.
+- `arrange_drop_win` — the drag-out gesture's arrange outcome: `arrangeTimeAtScreenX` (pointer column → time via `GetSet_ArrangeView2`'s one-pixel-span reading — inferred, not SDK-documented) and `performArrangeDrop` (snap the drop time, then one `InsertMedia` per capture on the pointer's track — assumed, not confirmed, to land end-to-end via REAPER's own cursor advance — in ONE undo block, counting only InsertMedia's reported successes, with the caller's track selection and edit cursor restored). This is the one shell in this directory that DOES place timeline items — and it is legitimate because the user dragged them there, the same class of act as `RunInsertSelected`. It never captures and never writes the bank.
 - `ingest` — ingest-through-the-bank shell on the EXTENSION side: three surfaces — (1) arrange capture→bank→assign (bindable action), (2) Media-Explorer import→bank→instrument on the selected track, (3) file drop onto the bank panel→bank only. Only surface (1) writes the `assignment_request` ext-state wire. **ingest NEVER inserts a timeline item.**
 
 ## Gotchas

src/shell/actions/arrange_drop_win.cpp

@@ -7,13 +7,24 @@
 //   B. InsertMedia advances the edit cursor past the media it added. That advance IS the
 //      multi-file layout: the cursor is deliberately not reset between files, so N captures
 //      land end to end. If REAPER does not advance it, they stack at one position instead —
-//      visible and one Ctrl-Z away, never silent.
+//      visible and one Ctrl-Z away, never silent. insert.cpp does NOT share this assumption: it
+//      resets the cursor before every track's insert specifically to stay independent of
+//      cursor-advance behavior (insert.cpp:18-20, "this doesn't matter either way"). This call
+//      site is the first in the tree to depend on it.
 //   C. SnapToGrid honors the project's snap-enabled toggle. The header documents no
 //      snap-enabled query for the arrange, so a drop taken with snapping OFF is the test that
 //      settles it.
+//   D. GetSet_ArrangeView2's one-pixel span [screenX, screenX+1) reads the time at that column.
+//      The header documents only the all-zero span (screen_x_start==screen_x_end==0) as the
+//      "whole view" special case; the per-column reading for any other span is inferred, not
+//      documented.
+//   E. InsertMedia's int return isn't SDK-documented; treated conservatively as 0=failure,
+//      nonzero=success — performArrangeDrop counts only the latter.
 
 #include "shell/actions/arrange_drop_win.h"
 
+#include <cstddef>
+#include <cstdio>
 #include <string>
 #include <vector>
 
@@ -43,8 +54,13 @@ using capture::InsertOptions;
 namespace {
 
 // Selection snapshot/restore, so a drop leaves the user's track selection exactly as it found
-// it. Mirrors insert.cpp's pair; that TU belongs to the capture pillar and this one to the drag
-// chain, so they are kept separate rather than sharing a helper across the seam.
+// it. Mirrors insert.cpp's pair; kept separate here because insert.cpp sits in the capture
+// pillar, outside this track's surface fence — not a ruling that the two should never share a
+// helper, just not this track's call to make.
+//
+// CountSelectedTracks/GetSelectedTrack both skip the master track (SDK header), so a user with
+// the master selected loses that selection across the drop — pre-existing behavior inherited
+// from insert.cpp's identical pair, not fixed here.
 std::vector<MediaTrack*> snapshotSelectedTracks() {
     const int n = CountSelectedTracks(nullptr);  // nullptr = active project
     std::vector<MediaTrack*> tracks;
@@ -63,9 +79,11 @@ void restoreSelectedTracks(const std::vector<MediaTrack*>& tracks) {
 
 double arrangeTimeAtScreenX(int screenX) {
     double start = 0.0, end = 0.0;
-    // isSet=false reads the arrange times spanned by [screen_x_start, screen_x_end); a
-    // one-pixel span therefore reads the time at that column. (The all-zero span is the SDK's
-    // documented "whole view" form, which is why the span here must not be empty.)
+    // isSet=false with a one-pixel span [screenX, screenX+1) is assumed to read the time at
+    // that column — inferred, not documented (assumption D in the file header). The SDK's ONLY
+    // documented special form is screen_x_start==screen_x_end==0 (both zero) for "the whole
+    // arrange view's start/end time"; a zero-width span at a nonzero column (e.g. screenX,
+    // screenX) is NOT that special case, so the +1 here is precautionary rather than required.
     GetSet_ArrangeView2(nullptr, false, screenX, screenX + 1, &start, &end);
     return start < 0.0 ? 0.0 : start;
 }
@@ -87,18 +105,24 @@ int performArrangeDrop(MediaTrack* track, double time,
 
     int inserted = 0;
     for (const std::string& path : absolutePaths) {
-        // No cursor reset between files — see assumption B in the file header.
-        InsertMedia(path.c_str(), mode);
-        ++inserted;
+        // No cursor reset between files — see assumption B in the file header. InsertMedia's
+        // return isn't SDK-documented (assumption E); treated conservatively as 0=failure, so a
+        // REAPER-side refusal is reflected in the count and in the undo label, not silent.
+        if (InsertMedia(path.c_str(), mode) != 0) ++inserted;
     }
 
+    // A fixed stack buffer, not std::string concatenation: the prior shape built the label with
+    // std::to_string + `+` between the restore below and Undo_EndBlock2, so a bad_alloc there
+    // would leave an unbalanced undo block open. snprintf here removes the allocation outright.
+    char label[64];
+    std::snprintf(label, sizeof(label), "ReaSampler: drop %d %s onto arrange", inserted,
+                 inserted == 1 ? "capture" : "captures");
+
     restoreSelectedTracks(priorSelection);
     SetEditCurPos(priorCursor, /*moveview=*/false, /*seekplay=*/false);
 
     // extraflags -1 = UNDO_STATE_ALL, matching the insert action's own block.
-    const std::string label = "ReaSampler: drop " + std::to_string(inserted) +
-                              (inserted == 1 ? " capture" : " captures") + " onto arrange";
-    Undo_EndBlock2(nullptr, label.c_str(), -1);
+    Undo_EndBlock2(nullptr, label, -1);
     return inserted;
 }
 

src/shell/actions/arrange_drop_win.h

@@ -16,14 +16,18 @@ class MediaTrack;
 
 namespace reasampler {
 
-// The arrange time under a screen X, via REAPER's own pixel-column -> time mapping. Times left
-// of project start clamp to 0.
+// The arrange time under a screen X, via GetSet_ArrangeView2's one-pixel-column reading —
+// inferred behavior, not SDK-documented (see the .cpp's assumption D). Times left of project
+// start clamp to 0.
 double arrangeTimeAtScreenX(int screenX);
 
-// Places every path in `absolutePaths` on `track`, the first at `time` and each subsequent one
-// after the previous (REAPER's own multi-file drop shape), inside ONE undo block. The drop time
-// honors the project's snap setting. The caller's track selection and edit-cursor position are
-// restored before returning. Returns the number of files handed to REAPER.
+// Places every path in `absolutePaths` on `track`, inside ONE undo block. The first lands at
+// `time`; whether REAPER's own cursor advance lands the rest end-to-end, or stacks them at one
+// position instead, is assumption B in the .cpp (visible, one Ctrl-Z away, either way). The
+// drop time is assumed to honor the project's snap setting (assumption C). The caller's track
+// selection and edit-cursor position are restored before returning. Returns the number of files
+// InsertMedia reported inserting successfully — its return isn't SDK-documented; treated as
+// 0=failure (assumption E in the .cpp).
 int performArrangeDrop(MediaTrack* track, double time,
                        const std::vector<std::string>& absolutePaths);
 

src/shell/panel/panel_drag.cpp

@@ -159,7 +159,10 @@ void applyDropCue(DropCue cue) {
         case DropCue::Instrument:    idc = IDC_HAND;   break;
         case DropCue::ArrangeInsert: idc = IDC_IBEAM;  break;  // an insertion point on a timeline
         case DropCue::Refuse:        idc = IDC_NO;     break;
-        case DropCue::OsOwned:       return;  // the OS drag loop draws its own copy cursor
+        case DropCue::OsOwned:       return;  // reached once per move resolving to OsHandoff,
+                                              // right before handOffToOs is attempted; the OS
+                                              // drag loop (once it actually starts) draws its
+                                              // own copy cursor, so leave the cursor alone here
         case DropCue::Internal:      return;  // applyDragCursor owns the in-client cue
         case DropCue::None:          return;
     }
@@ -167,8 +170,8 @@ void applyDropCue(DropCue cue) {
 }
 
 // One independent evaluation of the drag's target: the resolved class plus the live facts its
-// outcome needs. Nothing is remembered between calls — that is the whole point (a transition
-// reverses, and an unresolvable evaluation cannot poison a later one).
+// outcome needs. Nothing is remembered between calls (core/ui/CLAUDE.md: "the drag-out law is
+// per-move and stateless").
 struct LiveDrop {
     DropClass cls = DropClass::None;
     MediaTrack* track = nullptr;
@@ -185,10 +188,12 @@ LiveDrop resolveLiveDrop(int x, int y) {
     ctx.singlePayload = g_panel.dragSampleIds.size() == 1;
 
     LiveDrop out;
-    // The SDK hit-test is evaluated ONLY outside the client rect, so the common internal-drag
-    // path costs nothing. Unlike before, it runs for multi payloads too — still per-mouse-move
-    // cold, and it is what gives a multi drag a defined outcome on every surface.
-    if (!(x >= cr.left && x < cr.right && y >= cr.top && y < cr.bottom)) {
+    // The SDK hit-test is evaluated ONLY outside the client rect (needsSurfaceProbe — exported
+    // by the pure law so this gate can't drift from decideDropClass's own inside-client check),
+    // so the common internal-drag path costs nothing. Unlike before, it runs for multi payloads
+    // too — still per-mouse-move cold, and it is what gives a multi drag a defined outcome on
+    // every surface.
+    if (needsSurfaceProbe(x, y, client)) {
         POINT sp{x, y};
         ClientToScreen(g_panel.hwnd, &sp);
         const DropProbe probe = probeDropTarget(sp.x, sp.y);
@@ -198,6 +203,15 @@ LiveDrop resolveLiveDrop(int x, int y) {
         out.screenX = sp.x;
     }
     out.cls = decideDropClass(x, y, client, ctx);
+
+    // ArrangeInsert has no defined outcome once every armed sample is stale/missing — the same
+    // gate handOffToOs applies via decideOsHandoff before an OS hand-off, so acceptance
+    // criterion 7 (no silent no-op release) holds on this cell too, at both a cueing move and
+    // the release itself (both call this function). Cheap: a few fs::exists checks, only
+    // reached outside the client — already a cold path.
+    if (out.cls == DropClass::ArrangeInsert && resolveDragPathsForOs().empty()) {
+        out.cls = DropClass::Refuse;
+    }
     return out;
 }
 
@@ -342,9 +356,8 @@ void onMouseMove(int x, int y) {
         }
     }
     if (g_panel.dragging) {
-        // The class is re-resolved from scratch on every move — no first-move lock, nothing
-        // latched — so a transition in either direction always reverses, and drag speed cannot
-        // change where the gesture ends up.
+        // Re-resolved from scratch every move — core/ui/CLAUDE.md, "the drag-out law is
+        // per-move and stateless."
         const LiveDrop live = resolveLiveDrop(x, y);
 
         if (live.cls != DropClass::Internal) {
@@ -353,11 +366,11 @@ void onMouseMove(int x, int y) {
             // a drop that is not going there, then show whatever cue this class carries.
             g_panel.dropKind = DropKind::None;
             g_panel.dropBankId.clear();
+            applyDropCue(cueForDropClass(live.cls));  // OsHandoff -> OsOwned, a documented no-op cue
             if (live.cls == DropClass::OsHandoff) {
                 handOffToOs();
                 return;
             }
-            applyDropCue(cueForDropClass(live.cls));
             invalidatePanel();
             return;
         }
@@ -446,15 +459,16 @@ void resetDragState() {
     g_panel.dragPrimaryId.clear();
 }
 
-// Commits (or abandons) a drag on button-up, over the class resolved AT THE RELEASE POINT.
-// The switch is deliberately exhaustive with no default: every DropClass either performs its
-// outcome or is an explicit, already-cued refusal, so a new class cannot be added without
-// answering "what does releasing here do?".
+// Commits (or abandons) a drag on button-up, over the class resolved AT THE RELEASE POINT. Every
+// DropClass either performs its outcome or is an explicit, already-cued refusal (core/ui/
+// CLAUDE.md: "no DropClass means nothing happens"). The switch below has no default so each case
+// is spelled out by hand — but this build sets no warning flags (root CLAUDE.md), so a missing
+// case is NOT a compile error here; exhaustiveness is a review discipline, not a compiler
+// guarantee.
 void onLBtnUp(int x, int y) {
     if (g_panel.dragging) {
-        // Resolve at the release point, not from anything the moves remembered: WM_MOUSEMOVE is
-        // coalesced, so the last processed move can sit well away from where the button actually
-        // came up, and the release point is the user's stated target.
+        // Resolved at the release point, not from what the (coalesced) moves last recorded —
+        // core/ui/CLAUDE.md, "the drag-out law is per-move and stateless."
         const LiveDrop live = resolveLiveDrop(x, y);
 
         switch (live.cls) {
@@ -482,9 +496,12 @@ void onLBtnUp(int x, int y) {
             case DropClass::OsHandoff:
             case DropClass::None:
                 // Nothing to perform, and nothing silent about it: the refuse cursor has been
-                // showing since the move that resolved this class. An OsHandoff reaching release
-                // means the payload never resolved — a live hand-off consumes the drag inside
-                // DoDragDrop's modal loop and never returns here.
+                // showing since the move that resolved this class. OsHandoff reaching release
+                // usually means a live hand-off consumed the drag inside DoDragDrop's modal loop
+                // and this call never ran — but WM_MOUSEMOVE coalescing can still deliver a
+                // WM_LBUTTONUP with no intervening processed move (or right after a
+                // canInitiateDragOut refusal), so this case CAN be reached with a stale
+                // OsHandoff/Refuse class; the no-op here is correct either way.
                 break;
         }
 

src/shell/panel/panel_state.h

@@ -134,6 +134,7 @@ using ui::hitTestSlot;
 using ui::hitTestTabStrip;
 using ui::menuButtonReserve;
 using ui::navigate;
+using ui::needsSurfaceProbe;
 using ui::roleColor;
 using ui::stripActionPrefix;
 using ui::tagButtonEnabled;
@@ -320,10 +321,9 @@ struct PanelState {
     CardGesture cardGesture = CardGesture::None;
     int         dragTargetSlot = -1;
 
-    // NO out-of-client drop-target state is kept here on purpose. The drag-out class and its
-    // track are re-resolved from the live pointer on every move and again at the release point,
-    // which is what makes a class transition reversible and one unresolvable evaluation
-    // harmless to the next. Do not reintroduce a remembered target or a "blocked" latch.
+    // NO out-of-client drop-target state is kept here on purpose (core/ui/CLAUDE.md: "the
+    // drag-out law is per-move and stateless"). Do not reintroduce a remembered target or a
+    // "blocked" latch.
 
     // Authoritative tail setting lives in ReaSamplerSession, not here; panel reads it for
     // drawing and mutates via footer click / scroll-wheel. bankPanelTailSetting is the

tests/test_drag_out.cpp

@@ -12,6 +12,7 @@
 
 #include "../src/core/ui/drag_out.h"
 
+#include <cstddef>
 #include <cstdio>
 #include <string>
 #include <vector>
@@ -43,6 +44,14 @@ static const ReaperSurface kAllSurfaces[] = {
     ReaperSurface::FxEmbed,   ReaperSurface::Arrange,    ReaperSurface::Other,
 };
 
+// Pins kAllSurfaces against ReaperSurface::Count so a 7th surface added to the enum without a
+// matching entry here fails the BUILD, not just a silently-incomplete matrix — the compiler
+// alone does not enforce this (no -Wswitch/-Wall or /W4 anywhere in the build; see
+// panel_drag.cpp's onLBtnUp for the same caveat on DropClass).
+static_assert(sizeof(kAllSurfaces) / sizeof(kAllSurfaces[0]) ==
+             static_cast<std::size_t>(ReaperSurface::Count),
+             "kAllSurfaces must list exactly the surfaces below ReaperSurface::Count");
+
 // A point comfortably outside the panel client rect.
 static const int kOutX = 500, kOutY = 150;