import: remediate review findings — ledger gate, docs, message split

Delegates the refuse-gate to ledgerDegraded(), lifts its console message into a
pure testable fold, fixes stale doc line citations and an inaccurate outcome-enum
comment, and splits the rename counter into collision-vs-sanitize.

src/shell/actions/CLAUDE.md

@@ -43,7 +43,7 @@ is owned by other directories and only skinned here.
 - `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 `wire::classifyReaperSurface`, whose token rules `core/wire/CLAUDE.md` owns), 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`'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). The one timeline-placing shell here, per the invariant above; it never captures and never writes the bank.
-- `package_import_action` — the bindable / bank-menu / file-drop skin over `shell/package`'s `importBankPackage`. Owns the **ledger gate**, which runs BEFORE the file picker (a refusal must not cost the user a file choice) and is keyed on the session's `LedgerStatus` alone — never on `PruneReport::blockedByTracking`, whose undecodable-`rsusage_*` arm governs deletion-time protection and would refuse an import that only writes birth records. Owns every message the import produces; the verb itself is promptless.
+- `package_import_action` — the bindable / bank-menu / file-drop skin over `shell/package`'s `importBankPackage`. Owns the **ledger gate**, which runs BEFORE the file picker (a refusal must not cost the user a file choice) and is keyed on the session's `LedgerStatus` alone — never on `PruneReport::blockedByTracking`, whose undecodable-`rsusage_*` arm governs deletion-time protection and would refuse an import that only writes birth records. Builds and shows every message the import produces, but the ledger-refusal body itself is `core/package::ledgerRefusalMessage` — a pure fold this TU only supplies the channel-correct namespace to — so the wording is assertable without a DAW. `doImportBankPackage`/`doImportBankPackageFile` return the minted bank id on a landed import (empty otherwise) so a caller can focus it; the verb itself is promptless.
 - `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.** Surface (2)'s action is the one in this directory published into a NON-main action section (Media Explorer) as well as Main — two ids, one handler, two dispatch hooks; see root `CLAUDE.md` §"REAPER extension contract" for the mechanism.
 
 ## Gotchas

src/shell/actions/package_import_action.cpp

@@ -25,48 +25,31 @@ constexpr const char* kTitle = "ReaSampler: import bank package";
 
 std::string quoted(const std::string& s) { return "\"" + s + "\""; }
 
-// Mirrors prune's abort block in structure and tone, because a user who has hit that
-// one should recognise this one. Every recovery line names THIS build's namespace: a
-// beta user handed the stable spelling clears the wrong key and is still blocked.
+// The message body itself is core/package::ledgerRefusalMessage — a pure
+// (LedgerRefusal, namespace) -> string fold, testable without a DAW. This TU only
+// supplies the channel-correct namespace and the console call.
 void reportLedgerRefusal(package::LedgerRefusal refusal) {
-    const std::string& ns = version::extStateNamespace();
-    std::string msg =
-        "ReaSampler import: ABORTED -- the file-tracking ledger could not be read. "
-        "Nothing was imported.\n";
-    if (refusal == package::LedgerRefusal::Malformed) {
-        msg += "The stored file-tracking ledger is malformed. It has been left intact "
-               "rather than overwritten, so it can be repaired or cleared:\n"
-               "  reaper.SetProjExtState(0, \"" + ns + "\", \"owned_files\", \"\")\n"
-               "Clearing it makes every existing bank file un-reclaimable (they stop "
-               "being attributable to ReaSampler); no file is lost. Reopen the project "
-               "afterwards -- the block is held for the rest of this session.\n";
-    } else {
-        msg += "The stored file-tracking ledger was written by a NEWER version of "
-               "ReaSampler than this one, so its records cannot be read safely. It has "
-               "been left intact and will NOT be overwritten. Reopen the project with "
-               "that newer version -- do NOT clear this key from here, that would "
-               "discard tracking records this build cannot see. The block is held for "
-               "the rest of this session.\n";
-    }
-    msg += "An import can land hundreds of files in one gesture. With no readable "
-           "ledger, none of them could be given a birth record, and every one would be "
-           "permanently unreclaimable.\n";
-    ShowConsoleMsg(msg.c_str());
+    ShowConsoleMsg(
+        package::ledgerRefusalMessage(refusal, version::extStateNamespace()).c_str());
 }
 
 // The refusal a user can act on names all three: what the package needs, what this
 // build reads, and which build wrote it. Any two of them leave them stuck.
 void reportTooNew(const ImportBankResult& r) {
+    const bool knownWriter = !r.header.writerVersion.empty();
     const std::string writer =
-        r.header.writerVersion.empty() ? std::string("an unidentified build")
-                                       : "ReaSampler " + r.header.writerVersion;
-    const std::string msg =
+        knownWriter ? "ReaSampler " + r.header.writerVersion : std::string("an unidentified build");
+    std::string msg =
         "Cannot import this bank package.\n"
         "It was written by " + writer + " and needs package format " +
         std::to_string(r.header.minReaderVersion) + " or newer.\n"
         "This build (" + version::appVersion() + ") reads package format " +
         std::to_string(package::kPackageFormatVersion) + ".\n"
-        "Nothing was imported. Install " + writer + " or newer and try again.";
+        "Nothing was imported. ";
+    // "Install <writer> or newer" reads fine when writer is a real semver; it does not
+    // when writer is the "unidentified build" filler, so that case gets its own sentence.
+    msg += knownWriter ? "Install " + writer + " or newer and try again."
+                       : "Install a newer version of ReaSampler and try again.";
     ShowMessageBox(msg.c_str(), kTitle, 0);
 }
 
@@ -77,17 +60,27 @@ void reportSuccess(const ImportBankResult& r) {
         detail += " (a bank named " + quoted(r.seedBankName) +
                   " already exists in this project)";
     detail += ".\n";
-    if (r.renamedCount > 0) {
-        detail += "  " + std::to_string(r.renamedCount) +
+    // Two distinct triggers (core/package::ImportPlan), reported as two counts rather
+    // than folded into one ambiguous "already taken, or not spelled right" line.
+    if (r.collisionRenameCount > 0) {
+        detail += "  " + std::to_string(r.collisionRenameCount) +
                   " file(s) landed under a freshly minted name (the package's own name "
-                  "was already taken in the bank folder, or was not spelled the way "
-                  "this bank spells a file). An existing bank file is never "
-                  "overwritten.\n";
+                  "was already taken in the bank folder). An existing bank file is "
+                  "never overwritten.\n";
+    }
+    if (r.sanitizeRenameCount > 0) {
+        detail += "  " + std::to_string(r.sanitizeRenameCount) +
+                  " file(s) landed under a freshly minted name (not spelled the way "
+                  "this bank spells a file).\n";
     }
     if (r.collapsedCount > 0) {
+        // "Already present" here can only mean a duplicate BY CONTENT inside this same
+        // package (Ε-F2: import never consults another bank's hashes) — deliberately
+        // reworded from bank-package.md:448's "already present" phrasing, which reads
+        // as "already in your project" and is misleading in this direction.
         detail += "  " + std::to_string(r.collapsedCount) +
-                  " sample(s) were already present by content and were not written "
-                  "again.\n";
+                  " sample(s) duplicated another entry in this same package by content "
+                  "and were written once.\n";
     }
     detail += "One undo removes the imported bank and its entries. It does NOT delete "
               "the imported files -- they stay in the bank folder, referenced by "
@@ -129,6 +122,14 @@ void report(const ImportBankResult& r) {
         case ImportOutcome::Malformed:
             // Distinct from TooNew on purpose: the recoveries are opposite -- one is
             // "install a newer build", this one is "get an intact copy".
+            //
+            // bank-package.md:443 asks for a separate "This package is not well-formed"
+            // message when an entry name carries a separator / ".." / an absolute form.
+            // Not implemented: deserializeManifest returns one indistinguishable nullopt
+            // for that and for ordinary corruption, so it folds into this generic box.
+            // The binding spec (PLAN.md:2678) only requires Malformed != TooNew, which
+            // this still satisfies -- that product-doc row is knowingly left open, not
+            // silently missed.
             ShowMessageBox("This file is not a readable bank package (corrupt or "
                            "truncated). Nothing was imported.",
                            kTitle, 0);
@@ -167,17 +168,21 @@ bool ledgerPermits(ReaSamplerSession& session) {
 
 }  // namespace
 
-void doImportBankPackage(ReaSamplerSession& session) {
-    if (!ledgerPermits(session)) return;
+std::string doImportBankPackage(ReaSamplerSession& session) {
+    if (!ledgerPermits(session)) return {};
     std::string path;
-    if (!pickPackageForImport(path) || path.empty()) return;
-    report(importBankPackage(session, path));
+    if (!pickPackageForImport(path) || path.empty()) return {};
+    const ImportBankResult r = importBankPackage(session, path);
+    report(r);
+    return r.outcome == ImportOutcome::Landed ? r.bankId : std::string{};
 }
 
-void doImportBankPackageFile(ReaSamplerSession& session, const std::string& packageAbsPath) {
-    if (packageAbsPath.empty()) return;
-    if (!ledgerPermits(session)) return;
-    report(importBankPackage(session, packageAbsPath));
+std::string doImportBankPackageFile(ReaSamplerSession& session, const std::string& packageAbsPath) {
+    if (packageAbsPath.empty()) return {};
+    if (!ledgerPermits(session)) return {};
+    const ImportBankResult r = importBankPackage(session, packageAbsPath);
+    report(r);
+    return r.outcome == ImportOutcome::Landed ? r.bankId : std::string{};
 }
 
 }  // namespace reasampler

src/shell/actions/package_import_action.h

@@ -9,11 +9,14 @@ namespace reasampler {
 
 class ReaSamplerSession;
 
-// Gate, pick, import, report. The bound action and the panel's bank menu both call this.
-void doImportBankPackage(ReaSamplerSession& session);
+// Gate, pick, import, report. The bound action and the panel's bank menu both call
+// this. Returns the minted bank id on a landed import, "" otherwise (cancelled,
+// refused, or failed) — a caller that wants to focus the new bank (mirroring
+// doCreateBank) checks the return rather than reaching back into ImportBankResult.
+std::string doImportBankPackage(ReaSamplerSession& session);
 
 // Same, for a .rsbank already named by the user — the panel's file-drop route. The gate
-// still runs first; only the picker is skipped.
-void doImportBankPackageFile(ReaSamplerSession& session, const std::string& packageAbsPath);
+// still runs first; only the picker is skipped. Same return contract as doImportBankPackage.
+std::string doImportBankPackageFile(ReaSamplerSession& session, const std::string& packageAbsPath);
 
 }  // namespace reasampler

src/shell/package/import_bank.cpp

@@ -46,7 +46,8 @@ void fillPlanCounts(ImportBankResult& out, const package::ImportPlan& plan) {
     out.seedBankName = plan.seedBankName;
     out.bankNameAdjusted = plan.bankNameAdjusted;
     out.landedCount = plan.landCount;
-    out.renamedCount = plan.renameCount;
+    out.collisionRenameCount = plan.collisionRenameCount;
+    out.sanitizeRenameCount = plan.sanitizeRenameCount;
     out.collapsedCount = plan.collapseCount;
 }
 
@@ -70,8 +71,9 @@ ImportBankResult importBankPackage(ReaSamplerSession& session,
     fillPlanCounts(out, landing.plan);
     if (landing.outcome != ImportOutcome::Landed) return out;
 
+    const std::string bankId = mintBankId();
     const bool applied = applyImportedBank(
-        session.book(), mintBankId(), landing.plan,
+        session.book(), bankId, landing.plan,
         [&session](const model::Sample& s) {
             session.recordCreated(s, tracking::OriginKind::PackageImport);
         });
@@ -80,6 +82,7 @@ ImportBankResult importBankPackage(ReaSamplerSession& session,
         out.rollback = journal.rollback();
         return out;
     }
+    out.bankId = bankId;
 
     // Generation bump + persist ride inside one undo block, so a Ctrl-Z takes the whole
     // import back out of the index. It does NOT un-write the files — the summary says so.

src/shell/package/import_bank.h

@@ -17,12 +17,14 @@ struct ImportBankResult {
     ImportOutcome outcome = ImportOutcome::Unreadable;
     package::PackageHeader header;      // TooNew names the writer's build from here
 
+    std::string bankId;                 // the minted id — meaningful only when outcome == Landed
     std::string bankDisplayName;        // the bank actually created
     std::string seedBankName;           // what the package asked to be called
     bool bankNameAdjusted = false;
 
     int landedCount = 0;
-    int renamedCount = 0;
+    int collisionRenameCount = 0;  // renamed: the package's own name was already taken
+    int sanitizeRenameCount = 0;   // renamed: not spelled the way this tool spells a bank file
     int collapsedCount = 0;
 
     std::string    failedEntryName;

src/shell/package/import_landing.cpp

@@ -3,6 +3,7 @@
 
 #include "shell/package/import_landing.h"
 
+#include <cassert>
 #include <cstdint>
 #include <filesystem>
 #include <system_error>
@@ -118,12 +119,23 @@ ImportLanding landPackage(const std::string& packageAbsPath,
 
 bool applyImportedBank(BankBook& book, const std::string& bankId,
                        const package::ImportPlan& plan, const RecordBirth& recordBirth) {
+    // An empty std::function throws std::bad_function_call on invoke; every real caller
+    // supplies one, so an empty one here is a caller bug, not a runtime condition to
+    // recover from — enforce the contract rather than let it surface as an uncaught
+    // exception out of an extension action.
+    assert(recordBirth && "applyImportedBank: RecordBirth must not be empty");
     if (!book.createBank(bankId, plan.bankDisplayName)) return false;
 
     BankModel* index = book.index(bankId);
     for (const package::PlannedEntry& e : plan.entries) {
         if (e.action != EntryAction::Land) continue;
-        index->add(e.sample);
+        const AddResult added = index->add(e.sample);
+        // planImport already deduped Land entries by hash against an empty destination
+        // bank (this same freshly-created one), so a Collapsed add here would mean the
+        // plan and the book disagree — that would silently undercount reportSuccess's
+        // landedCount rather than fail loudly.
+        assert(added == AddResult::Added && "planImport's Land entries must not collapse");
+        (void)added;
         // Unconditional on the add's outcome: the file exists either way, and an
         // unrecorded file is permanently unreclaimable.
         recordBirth(e.sample);

src/shell/package/import_landing.h

@@ -15,18 +15,22 @@
 
 namespace reasampler {
 
-// How a landing ended. Every value but Landed means NOTHING is on disk and NO index
-// was touched — the two refuse-whole failures (TooNew, Malformed) before a byte is
-// written, the other two after a rollback.
+// How a landing ended. Every value but Landed means NOTHING is on disk and NO index was
+// touched. NoProject/Unreadable/Malformed/TooNew refuse before a byte is written.
+// IntegrityFailed also refuses before any write — the full-package digest verification
+// runs to completion first (landPackage) — so it needs no rollback either. WriteFailed
+// is the only outcome that actually wrote and then rolled back. IndexRejected is never
+// returned by landPackage/this struct — it is import_bank's own outcome, minted after a
+// successful landing when the book itself refuses the create.
 enum class ImportOutcome {
     Landed,
     NoProject,        // unsaved project: there is no bank folder to land into
     Unreadable,       // the package file could not be opened
     Malformed,        // not a well-formed RSBK: corrupt, truncated, or trailing garbage
     TooNew,           // minReaderVersion above this build's ladder
-    IntegrityFailed,  // an entry's payload did not match its recorded digest
+    IntegrityFailed,  // an entry's payload did not match its recorded digest; pre-write refusal
     WriteFailed,      // a write failed partway; the landed files were rolled back
-    IndexRejected,    // the book refused the bank the plan minted a free name for
+    IndexRejected,    // never set here — see the comment above; import_bank's outcome only
 };
 
 struct ImportLanding {
@@ -36,7 +40,7 @@ struct ImportLanding {
     package::PackageHeader header;
     package::ImportPlan    plan;
     std::string            failedEntryName;  // IntegrityFailed / WriteFailed
-    RollbackResult         rollback;         // IntegrityFailed / WriteFailed
+    RollbackResult         rollback;         // WriteFailed only — IntegrityFailed leaves it default
 };
 
 // Streams `packageAbsPath` into the project's bank folder: decode, plan, verify EVERY

src/shell/panel/panel_bank_ops.cpp

@@ -284,7 +284,14 @@ void showTabMenu(int screenX, int screenY, const std::string& bankId) {
         // Always a NEW bank, never a merge into the right-clicked one — the row sits
         // here because this is the panel's bank menu, not because it targets this bank.
         case kMenuImportPackage:
-            if (g_panel.session) doImportBankPackage(*g_panel.session);
+            if (g_panel.session) {
+                const std::string id = doImportBankPackage(*g_panel.session);
+                if (!id.empty()) {  // landed — show the freshly-imported bank
+                    g_panel.shownBankId = id;
+                    g_panel.focusedRegion = Region::Banks;
+                    invalidatePanel();
+                }
+            }
             break;
         default: break;
     }

src/shell/panel/panel_window.cpp

@@ -16,6 +16,9 @@
 #include "shell/panel/draw_kit.h"
 #include "shell/actions/ingest.h"
 #include "shell/actions/package_import_action.h"
+#include "core/package/import_plan.h"
+#include "core/version/app_version.h"
+#include "shell/persist/session.h"  // ReaSamplerSession::ledgerStatus() — panel_state.h only forward-declares it
 
 #ifdef _WIN32
 #include <windowsx.h>  // GET_X_LPARAM / GET_Y_LPARAM (SWELL supplies them on mac/linux)
@@ -29,6 +32,7 @@
 #define REAPERAPI_WANT_DockWindowActivate
 #define REAPERAPI_WANT_DockWindowRemove
 #define REAPERAPI_WANT_GetMainHwnd
+#define REAPERAPI_WANT_ShowConsoleMsg
 #include "reaper_plugin_functions.h"
 
 // main.cpp owns the module instance handle.
@@ -72,9 +76,21 @@ void handleDropFiles(HDROP hDrop) {
         else paths.push_back(std::move(p));
     }
     DragFinish(hDrop);
-    if (g_panel.session)
-        for (const std::string& pkg : packages)
-            doImportBankPackageFile(*g_panel.session, pkg);
+    if (g_panel.session && !packages.empty()) {
+        // One refusal block for the whole drop, not one per dropped .rsbank: the gate
+        // decision is the same for all N (session state does not change mid-drop), so
+        // checking it here first avoids doImportBankPackageFile's own per-file gate
+        // check printing the identical console block N times.
+        const package::LedgerRefusal refusal =
+            package::importLedgerRefusal(g_panel.session->ledgerStatus());
+        if (refusal != package::LedgerRefusal::None) {
+            ShowConsoleMsg(
+                package::ledgerRefusalMessage(refusal, version::extStateNamespace()).c_str());
+        } else {
+            for (const std::string& pkg : packages)
+                doImportBankPackageFile(*g_panel.session, pkg);
+        }
+    }
     if (!paths.empty()) ingestDroppedFiles(paths);
 }
 

src/shell/persist/session.h

@@ -98,6 +98,13 @@ public:
     // the records. That is not a hole in the pairing rule above: the rule exists so an
     // absent record is never read as a definite answer, and this exposes strictly less
     // than the pair. The package import gates on it before it opens a file picker.
+    //
+    // A tradeoff, not the only route: `pruneDryRun()` already exposes the same degraded
+    // pair via `PruneReport::ledgerUnreadable`/`ledgerFutureVersion`, with no new
+    // accessor needed. Rejected because that route is genuinely worse for a gate: it
+    // drags a full bank-folder enumeration and every live instance's FX scan onto a
+    // check that only needs to know "can I write a record", and it shapes an import
+    // decision as an answer borrowed from prune's report rather than the session's own.
     tracking::LedgerStatus ledgerStatus() const { return trackingStatus_; }
 
     // The version that last wrote the active project: PreVersioning (no