From a927dad2f46df841b2e4922cdd372afed9db1db6 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 13:21:48 -0400 Subject: [PATCH 1/2] import: a .rsbank lands as a new bank, whole or not at all Four collisions answered explicitly: ids reminted, names never overwritten, content deduped before the write, bank name auto-suffixed. Degraded ledger refuses before the picker. --- src/app/CMakeLists.txt | 8 + src/app/main.cpp | 4 + src/core/model/bank_book.cpp | 11 + src/core/model/bank_book.h | 11 + src/core/package/CLAUDE.md | 26 +- src/core/package/CMakeLists.txt | 7 + src/core/package/import_plan.cpp | 170 +++++++++ src/core/package/import_plan.h | 76 ++++ src/core/package/package_format.cpp | 7 + src/core/package/package_format.h | 6 + src/core/package/package_manifest.cpp | 14 +- src/shell/actions/CLAUDE.md | 4 +- src/shell/actions/package_import_action.cpp | 183 +++++++++ src/shell/actions/package_import_action.h | 19 + src/shell/package/CLAUDE.md | 22 +- src/shell/package/CMakeLists.txt | 8 + src/shell/package/import_bank.cpp | 94 +++++ src/shell/package/import_bank.h | 38 ++ src/shell/package/import_landing.cpp | 137 +++++++ src/shell/package/import_landing.h | 64 ++++ src/shell/panel/CLAUDE.md | 2 +- src/shell/panel/panel_bank_ops.cpp | 8 + src/shell/panel/panel_window.cpp | 24 +- src/shell/persist/session.h | 6 + tests/test_import_landing.cpp | 394 ++++++++++++++++++++ tests/test_import_plan.cpp | 370 ++++++++++++++++++ 26 files changed, 1689 insertions(+), 24 deletions(-) create mode 100644 src/core/package/import_plan.cpp create mode 100644 src/core/package/import_plan.h create mode 100644 src/shell/actions/package_import_action.cpp create mode 100644 src/shell/actions/package_import_action.h create mode 100644 src/shell/package/import_bank.cpp create mode 100644 src/shell/package/import_bank.h create mode 100644 src/shell/package/import_landing.cpp create mode 100644 src/shell/package/import_landing.h create mode 100644 tests/test_import_landing.cpp create mode 100644 tests/test_import_plan.cpp diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 044ab51..32a7ede 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -58,6 +58,14 @@ target_link_libraries(reaper_reasampler PRIVATE json wire file_bytes bank_model # free of the voice engine — a link edge to it here means the design drifted. target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) +# Bank-package import: the promptless verb plus its action skin. Kept as its own +# appended block rather than merged into the lists above, so the two package +# directions stay textually independent. +target_sources(reaper_reasampler PRIVATE + ${REASAMPLER_SRC_DIR}/shell/package/import_bank.cpp + ${REASAMPLER_SRC_DIR}/shell/actions/package_import_action.cpp) +target_link_libraries(reaper_reasampler PRIVATE import_landing package_pickers) + # OUTPUT_NAME is channel-derived; the CMake target name stays "reaper_reasampler" for both # configs, since REAPER dlopen's any reaper_* module and the two channels' artifacts load # side-by-side. LIBRARY_OUTPUT_DIRECTORY pins the module to the top of the build tree even diff --git a/src/app/main.cpp b/src/app/main.cpp index 57696ae..5592e99 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -36,6 +36,7 @@ #include "shell/panel/panel_window.h" // panel lifecycle (init/toggle/open-query/shutdown) #include "shell/persist/session.h" // ReaSamplerSession #include "shell/view/view.h" // reconcileManagedLanes / applyMode +#include "shell/actions/package_import_action.h" // bank-package import action body namespace capture = reasampler::capture; @@ -91,6 +92,7 @@ static void RunCancelRealtime(int) { capture::RunCancelRealtime(g_session); } static void RunRecaptureFromSource(int) { capture::RunRecaptureFromSource(g_session); } static void RunRenderTrackInPlace(int) { capture::RunRenderTrackInPlace(g_session); } static void RunResampleBake(int) { capture::RunResampleBake(g_session); } +static void RunImportBankPackage(int) { reasampler::doImportBankPackage(g_session); } static void RunShowVersion(int) { // On-demand only — no unconditional startup print (routine console chatter pops // the console window). @@ -148,6 +150,8 @@ static std::vector buildMainActionTable() { "land pending ReaSampler 9000 resample bake", &RunResampleBake}); rows.push_back({"SHOW_VERSION", "show version", &RunShowVersion}); + rows.push_back({"IMPORT_BANK_PACKAGE", "import bank package (.rsbank)", + &RunImportBankPackage}); return rows; } diff --git a/src/core/model/bank_book.cpp b/src/core/model/bank_book.cpp index 17b4ff5..efa5e25 100644 --- a/src/core/model/bank_book.cpp +++ b/src/core/model/bank_book.cpp @@ -117,6 +117,17 @@ bool BankBook::createBank(const std::string& id, const std::string& displayName) return true; } +// Runs behind displayNameTaken so the probe and the create/rename check can never +// disagree about what "already used" means. exceptId is deliberately "" — no bank can +// carry an empty id, so nothing is excluded from the scan. +std::string BankBook::uniqueDisplayName(const std::string& seed) const { + if (!displayNameTaken(seed, /*exceptId=*/std::string{})) return seed; + for (int n = 2;; ++n) { + std::string candidate = seed + " " + std::to_string(n); + if (!displayNameTaken(candidate, /*exceptId=*/std::string{})) return candidate; + } +} + bool BankBook::renameBank(const std::string& id, const std::string& displayName) { if (id == kPoolBankId) return false; // pool is un-renamable Bank* b = bank(id); diff --git a/src/core/model/bank_book.h b/src/core/model/bank_book.h index 284654a..3a19d65 100644 --- a/src/core/model/bank_book.h +++ b/src/core/model/bank_book.h @@ -100,6 +100,17 @@ public: // no-op success. bool renameBank(const std::string& id, const std::string& displayName); + // The first name in the sequence `seed`, "seed 2", "seed 3", … whose fold is free + // in this book — what a caller that must not be rejected (the package import) asks + // for before createBank. First-FREE-ascending, not highest-plus-one, so it fills a + // gap ("Drums" + "Drums 3" present yields "Drums 2") and is a pure function of the + // current name set. The seed is returned verbatim when free and is NEVER re-parsed: + // a bare trailing integer cannot be told from a user's own name, so "Kit 808" would + // become "Kit 2" under a stripping rule. Terminates by pigeonhole (one of the first + // N+1 candidates is free for N banks), so it needs no cap. A blank seed comes back + // blank — what a missing name should become is the caller's policy, not the model's. + std::string uniqueDisplayName(const std::string& seed) const; + // Deletes a named bank and its member entries (files untouched — a shell/prune // concern). Rejects (false, no mutation) an unknown id or the pool. Remaining // ordinals compact after; if the deleted bank was active, falls back to the pool. diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 9114f83..5d80a01 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -73,6 +73,11 @@ landing after the format. here, computed where payloads are streamed (shell). The bank's `slot_map` rides along. Unknown keys skip at every level; duplicate entry names are rejected both ways. +- `import_plan` — the pure import decision, and the reason the whole feature is + testable without a DAW: the destination bank's display name after + `BankBook`'s own fold, the reminted sample ids and remapped parents, and the + per-entry land / collapse / rename disposition. Also `importLedgerRefusal`, + the import's ledger gate. - `bank_package` — framing and arithmetic composing the two above: `encodePackage` (prefix bytes + layout + total size, stamping this build's ladder pair and `version::stampVersion()`), `decodePackage` (prefix + observed @@ -91,6 +96,11 @@ landing after the format. deliberately NOT enforced by the codec — they are `import_plan` decisions. The codec rejects only what makes the container itself incoherent (duplicate entry names, invalid names, a non-single-sample nested index). +- **`import_plan` consults no other bank's hashes, and that is the ruling, not + an omission.** An import always creates a NEW bank, so "already present in the + destination bank by content" is exactly "already landed by this same plan". + Cross-bank dedup is not enforced anywhere (`core/model/CLAUDE.md`), so a hash + the pool already holds still lands its own file here. - `requiredPrefixSize` trusts fields beyond the frozen region only when the version pair classifies `Readable`; for `TooNew` it stops at the semver — don't "fix" it to read the manifest length there, a future structural format @@ -133,15 +143,13 @@ landing after the format. collision class as the ASCII case fold, which `sameEntryName` does catch. A table-free fix does not exist, and restricting names to ASCII would be genuinely over-strict for non-English users. Left open knowingly. -- **`duplicateName` is O(n²) over `entries` on the decode path** — pre-existing - shape (the double loop is unchanged since `af35fc5`; only the comparator - changed). Under the `kMaxManifestBytes` cap (64 MB) a minimal entry is - ~100 bytes, so a hostile package can declare ~670k entries — ~2×10¹¹ pair - comparisons, a multi-minute hang on import. It signals an error rather than - UB, so the hostile-input invariant above still holds, but it sits against - this module's "a forged header cannot demand gigabytes" posture. Forward - obligation for `import_plan`: fold this into a sorted vector or hash set - when that track lands; not changed here. +- **`duplicateName` folds through a hash set, not a pairwise scan.** Under the + `kMaxManifestBytes` cap (64 MB) a minimal entry is ~100 bytes, so a hostile + package can declare ~670k entries; the former double loop was ~2×10¹¹ pair + comparisons — a multi-minute hang on the decode path an import drives. The + set is keyed on `entryNameKey`, which is `sameEntryName`'s ASCII-case fold + made explicit, so the equivalence rule still has one home (`lowerAscii`). + Do not reintroduce the pairwise scan. - **Cross-module contract with `src/shell/package`:** a genuinely zero-length entry cannot round-trip through the filesystem seam there (`appendPayload` refuses an empty payload — an empty buffer signals an upstream read failure, diff --git a/src/core/package/CMakeLists.txt b/src/core/package/CMakeLists.txt index fc7fd2d..0a04b90 100644 --- a/src/core/package/CMakeLists.txt +++ b/src/core/package/CMakeLists.txt @@ -12,3 +12,10 @@ reasampler_pure_library(bank_package LINK PUBLIC package_format package_manifest PRIVATE app_version) # app_version: the tests pin the stamped writer semver against stampVersion(). reasampler_test(bank_package LINK bank_package app_version) + +reasampler_pure_library(import_plan + SOURCES import_plan.cpp + LINK PUBLIC package_manifest bank_book origin_ledger PRIVATE package_format capture_paths) +# tracking_authority: the ledger-gate test proves the import does NOT share prune's +# composite blocker, which needs the composite to compare against. +reasampler_test(import_plan LINK import_plan tracking_authority) diff --git a/src/core/package/import_plan.cpp b/src/core/package/import_plan.cpp new file mode 100644 index 0000000..372b2c4 --- /dev/null +++ b/src/core/package/import_plan.cpp @@ -0,0 +1,170 @@ +#include "core/package/import_plan.h" + +#include +#include +#include + +#include "core/capture/capture_paths.h" +#include "core/package/package_format.h" + +namespace reasampler::package { + +namespace { + +using capture::bankRelativeForName; +using capture::deriveBankPaths; +using capture::sanitizeStem; + +bool blankName(const std::string& s) { + for (char c : s) + if (c != ' ' && c != '\t' && c != '\n' && c != '\r') return false; + return true; +} + +// "kick.wav" -> "kick"; a name with no dot is its own stem. deriveBankPaths re-adds +// the extension, so handing it the full name would file "kick.wav" as "kick.wav.wav". +std::string stemOf(const std::string& fileName) { + const std::size_t dot = fileName.rfind('.'); + if (dot == std::string::npos || dot == 0) return fileName; + return fileName.substr(0, dot); +} + +// True when `fileName` is already spelled the way this tool spells a bank file, so a +// package landing in a fresh project keeps the names it travelled with. Anything else +// is minted through deriveBankPaths, which is also the sanitizer. +bool spelledLikeABankFile(const std::string& fileName) { + const std::string stem = stemOf(fileName); + return stem != fileName && sanitizeStem(stem) == stem && fileName == stem + ".wav"; +} + +// The bank-folder names an import must not land on: what is there already, plus what +// this import has minted so far. Case-folded, because the two filesystems this tool +// ships on would treat "Kick.wav" and "kick.wav" as one file. +class NameSet { +public: + explicit NameSet(const std::vector& present) { + keys_.reserve(present.size()); + for (const std::string& n : present) keys_.insert(entryNameKey(n)); + } + bool taken(const std::string& name) const { return keys_.count(entryNameKey(name)) != 0; } + void claim(const std::string& name) { keys_.insert(entryNameKey(name)); } + +private: + std::unordered_set keys_; +}; + +// The name this entry lands under. Terminates: each attempt carries a distinct +// counter, and the taken set is finite. +std::string mintFileName(const std::string& projectDir, const std::string& packageName, + const std::string& uniqueTag, const NameSet& taken) { + if (spelledLikeABankFile(packageName) && !taken.taken(packageName)) return packageName; + + const std::string stem = stemOf(packageName); + std::string tag = uniqueTag; + for (int n = 2;; ++n) { + const std::string candidate = deriveBankPaths(projectDir, stem, tag).fileName; + if (!taken.taken(candidate)) return candidate; + tag = uniqueTag + "-" + std::to_string(n); + } +} + +} // namespace + +LedgerRefusal importLedgerRefusal(tracking::LedgerStatus status) { + switch (status) { + case tracking::LedgerStatus::Unreadable: return LedgerRefusal::Malformed; + case tracking::LedgerStatus::FutureVersion: return LedgerRefusal::FutureVersion; + case tracking::LedgerStatus::Fresh: + case tracking::LedgerStatus::Loaded: break; + } + return LedgerRefusal::None; +} + +std::string bankFolderDir(const std::string& projectDir) { + // Only the directory half of the result is wanted; the stem is a placeholder. + return deriveBankPaths(projectDir, "bank", std::string{}).absoluteDir; +} + +ImportPlan planImport(const PackageManifest& manifest, + const BankBook& destination, + const std::string& projectDir, + const std::vector& bankFolderFileNames, + const std::string& uniqueTag) { + ImportPlan plan; + + plan.seedBankName = blankName(manifest.bankDisplayName) + ? std::string(kDefaultImportBankName) + : manifest.bankDisplayName; + plan.bankDisplayName = destination.uniqueDisplayName(plan.seedBankName); + plan.bankNameAdjusted = plan.bankDisplayName != plan.seedBankName; + + NameSet taken(bankFolderFileNames); + // The destination bank is created empty by this same import, so "already in the + // destination bank by content" is exactly "already landed by this plan" — the + // hash set below IS that bank's findByHash. Cross-bank dedup is deliberately not + // enforced (core/model/CLAUDE.md), so other banks' hashes are not consulted. + std::unordered_map landedIdForHash; + // Every package id, including a collapsed one's, so a parent link that pointed at + // a duplicate still resolves to the entry that survived it. + std::unordered_map idRemap; + + plan.entries.reserve(manifest.entries.size()); + for (std::size_t i = 0; i < manifest.entries.size(); ++i) { + const PackageEntry& src = manifest.entries[i]; + + PlannedEntry e; + e.manifestIndex = i; + + const std::string& hash = src.sample.contentHash; + if (!hash.empty()) { + const auto hit = landedIdForHash.find(hash); + if (hit != landedIdForHash.end()) { + e.action = EntryAction::Collapse; + idRemap[src.sample.id] = hit->second; + ++plan.collapseCount; + plan.entries.push_back(std::move(e)); + continue; + } + } + + e.destFileName = mintFileName(projectDir, src.fileName, uniqueTag, taken); + e.renamed = e.destFileName != src.fileName; + taken.claim(e.destFileName); + + e.sample = src.sample; + e.sample.id = std::string(kImportIdPrefix) + uniqueTag + "-" + e.destFileName; + e.sample.relativePath = bankRelativeForName(e.destFileName); + + if (!hash.empty()) landedIdForHash.emplace(hash, e.sample.id); + idRemap[src.sample.id] = e.sample.id; + + ++plan.landCount; + if (e.renamed) ++plan.renameCount; + plan.entries.push_back(std::move(e)); + } + + // Second pass: the remap must be complete before a parent is resolved, since a + // sample may precede its own parent in manifest order. + for (PlannedEntry& e : plan.entries) { + if (e.action != EntryAction::Land || !e.sample.provenance) continue; + const auto hit = idRemap.find(e.sample.provenance->parentSampleId); + e.sample.provenance->parentSampleId = + hit == idRemap.end() ? std::string{} : hit->second; + } + + // The package's display order, over the ids that actually landed. SlotMap's own + // repair rules settle the rest: two package ids collapsed onto one landed id give + // one slot (first wins), and a landed sample the package never positioned is + // appended by BankBook::reconcileSlots afterwards. + std::vector> slotPairs; + for (const std::string& oldId : manifest.slots.orderedIds()) { + const auto hit = idRemap.find(oldId); + if (hit == idRemap.end()) continue; + slotPairs.emplace_back(hit->second, manifest.slots.slotOf(oldId)); + } + plan.slots = model::SlotMap::fromEntries(slotPairs); + + return plan; +} + +} // namespace reasampler::package diff --git a/src/core/package/import_plan.h b/src/core/package/import_plan.h new file mode 100644 index 0000000..a79dd46 --- /dev/null +++ b/src/core/package/import_plan.h @@ -0,0 +1,76 @@ +#pragma once +// import_plan — the pure import decision: the destination bank's display name after +// the book's own uniqueness fold, the reminted sample ids and remapped parents, and +// the per-entry land / collapse / rename disposition. Value inputs only; no +// filesystem, no session handle, no host types. + +#include +#include +#include + +#include "core/model/bank_book.h" +#include "core/model/slot_map.h" +#include "core/package/package_manifest.h" +#include "core/tracking/origin_ledger.h" + +namespace reasampler::package { + +// The bank name a package that recorded none (or a blank one) imports under. +inline constexpr const char* kDefaultImportBankName = "Imported bank"; + +// The prefix every imported sample id is reminted under, so a package's own ids — +// unique only within the project that made them — never enter this index. +inline constexpr const char* kImportIdPrefix = "pkg-"; + +// Which of the two refusal messages the import owes the user, if either. +// +// Keyed on the LEDGER STATUS ALONE, never on prune's composite blockedByTracking: +// that flag also fires on undecodable rsusage_* keys, which govern which files a +// DELETION may touch. An import deletes nothing and computes no protected set — it +// writes birth records — so an unreadable usage key must not refuse one. +enum class LedgerRefusal { None, Malformed, FutureVersion }; + +LedgerRefusal importLedgerRefusal(tracking::LedgerStatus status); + +// What one manifest entry does when the import runs. +// - Land: write the payload under destFileName and add `sample`. +// - Collapse: an equal contentHash already lands in this same import, so the payload +// is NOT written and no entry is added. Writing it and letting +// BankModel::add collapse the entry would leave the file referenced by +// nothing — an orphan manufactured by a dedup. +enum class EntryAction { Land, Collapse }; + +struct PlannedEntry { + std::size_t manifestIndex = 0; + EntryAction action = EntryAction::Land; + std::string destFileName; // Land only — a bare name in the bank folder + model::Sample sample; // Land only — id, path and parent already remapped + bool renamed = false; // the package's own name was taken, so a fresh one was minted +}; + +struct ImportPlan { + std::string bankDisplayName; + bool bankNameAdjusted = false; // the seed was taken, so the name carries a suffix + std::string seedBankName; // the seed the probe started from + std::vector entries; // one per manifest entry, in manifest order + model::SlotMap slots; // the package's slots over the reminted ids + int landCount = 0; + int collapseCount = 0; + int renameCount = 0; +}; + +// The bank folder an import lands into — the same expression capture uses, so an +// imported file is spelled exactly like a captured one. +std::string bankFolderDir(const std::string& projectDir); + +// Decides everything about an import except the bytes. `bankFolderFileNames` are the +// bare names already present in that folder (never overwritten); `uniqueTag` is the +// shell's per-import disambiguator, extended with an ascending counter where one tag +// is not enough. Total: every manifest entry yields exactly one PlannedEntry. +ImportPlan planImport(const PackageManifest& manifest, + const BankBook& destination, + const std::string& projectDir, + const std::vector& bankFolderFileNames, + const std::string& uniqueTag); + +} // namespace reasampler::package diff --git a/src/core/package/package_format.cpp b/src/core/package/package_format.cpp index 76808ac..34af822 100644 --- a/src/core/package/package_format.cpp +++ b/src/core/package/package_format.cpp @@ -103,6 +103,13 @@ bool sameEntryName(const std::string& a, const std::string& b) { return true; } +std::string entryNameKey(const std::string& name) { + std::string key; + key.reserve(name.size()); + for (unsigned char c : name) key += lowerAscii(c); + return key; +} + bool isValidNestedSamplePath(const std::string& path) { if (util::isAbsolutePath(path)) return false; // Component-wise, not a substring scan: "take..final/a.wav" is a legal diff --git a/src/core/package/package_format.h b/src/core/package/package_format.h index fd7c7a9..346f5ff 100644 --- a/src/core/package/package_format.h +++ b/src/core/package/package_format.h @@ -76,6 +76,12 @@ bool isValidEntryName(const std::string& name); // bytes compare exactly (see this directory's CLAUDE.md on NFC/NFD). bool sameEntryName(const std::string& a, const std::string& b); +// sameEntryName's fold made explicit: the ASCII-lower-cased bytes, so +// entryNameKey(a) == entryNameKey(b) exactly when sameEntryName(a, b). For a caller +// holding many names at once — folding them into a set is what turns an O(n^2) +// pairwise scan into a linear one. +std::string entryNameKey(const std::string& name); + // The one field in the format that CAN express a path: a nested Sample's // relativePath, which is bank-relative by design. Rejects every absolute form // (the shared util::isAbsolutePath test) and any ".." component — BankModel::add diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index df2f772..b634d05 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -1,5 +1,6 @@ #include "core/package/package_manifest.h" +#include #include #include "core/json/json.h" @@ -14,11 +15,16 @@ using ObjWriter = json::Writer; // Shared by serializeManifest and deserializeManifest — see this directory's // CLAUDE.md for why duplicate names are rejected both ways. Equivalence is the -// format's, not std::string's: sameEntryName folds ASCII case. +// format's, not std::string's: entryNameKey is sameEntryName's ASCII-case fold. +// +// A set, not the pairwise scan this replaced: under kMaxManifestBytes a hostile +// package can declare hundreds of thousands of minimal entries, and O(n^2) over that +// is a multi-minute hang on the decode path an import drives. bool duplicateName(const std::vector& entries) { - for (std::size_t i = 0; i < entries.size(); ++i) - for (std::size_t j = i + 1; j < entries.size(); ++j) - if (sameEntryName(entries[i].fileName, entries[j].fileName)) return true; + std::unordered_set seen; + seen.reserve(entries.size()); + for (const auto& e : entries) + if (!seen.insert(entryNameKey(e.fileName)).second) return true; return false; } diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index befbaed..9f2bb9d 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -4,7 +4,8 @@ The bindable action families routed through REAPER's `command_id`/`gaccel`/ `hookcommand` contract (Design View toggle actions, bank actions, the prune -action, and the shared registration plumbing/table), plus the three drag-out +action, the bank-package import action, and the shared registration +plumbing/table), plus the three drag-out outcome shells (OS hand-off, instrument drop, arrange drop), plus the extension-side ingest-through-the-bank shell. This is where user-facing REAPER actions and OS-level drag/drop live; the underlying @@ -42,6 +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. - `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 diff --git a/src/shell/actions/package_import_action.cpp b/src/shell/actions/package_import_action.cpp new file mode 100644 index 0000000..2531858 --- /dev/null +++ b/src/shell/actions/package_import_action.cpp @@ -0,0 +1,183 @@ +// package_import_action.cpp — see package_import_action.h for the contract. +// main.cpp owns the API pointers; this TU gets them extern. + +#include "shell/actions/package_import_action.h" + +#include + +#include "core/package/import_plan.h" +#include "core/package/package_format.h" +#include "core/version/app_version.h" +#include "shell/package/import_bank.h" +#include "shell/package/package_pickers.h" +#include "shell/persist/session.h" + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_ShowConsoleMsg +#define REAPERAPI_WANT_ShowMessageBox +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +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. +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()); +} + +// 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 std::string writer = + r.header.writerVersion.empty() ? std::string("an unidentified build") + : "ReaSampler " + r.header.writerVersion; + const 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."; + ShowMessageBox(msg.c_str(), kTitle, 0); +} + +void reportSuccess(const ImportBankResult& r) { + std::string detail = "ReaSampler import: imported " + std::to_string(r.landedCount) + + " sample(s) into a new bank: " + quoted(r.bankDisplayName); + if (r.bankNameAdjusted) + detail += " (a bank named " + quoted(r.seedBankName) + + " already exists in this project)"; + detail += ".\n"; + if (r.renamedCount > 0) { + detail += " " + std::to_string(r.renamedCount) + + " 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"; + } + if (r.collapsedCount > 0) { + detail += " " + std::to_string(r.collapsedCount) + + " sample(s) were already present by content and were not written " + "again.\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 " + "nothing, until a prune reclaims them.\n"; + ShowConsoleMsg(detail.c_str()); + + // The console carries the copyable detail; the box makes the outcome unmissable. + const std::string summary = "Imported " + std::to_string(r.landedCount) + + " sample(s) into a new bank: " + + quoted(r.bankDisplayName) + "."; + ShowMessageBox(summary.c_str(), kTitle, 0); +} + +void reportRollback(const RollbackResult& rollback, std::string& msg) { + if (rollback.failedCount > 0) { + msg += "\n" + std::to_string(rollback.failedCount) + + " partly-imported file(s) could not be removed and are still in the bank " + "folder. They are referenced by no bank; a prune will reclaim them."; + } +} + +void report(const ImportBankResult& r) { + switch (r.outcome) { + case ImportOutcome::Landed: + reportSuccess(r); + return; + case ImportOutcome::TooNew: + reportTooNew(r); + return; + case ImportOutcome::NoProject: + ShowMessageBox("Save the project before importing a bank package -- an " + "unsaved project has no bank folder to import into.", + kTitle, 0); + return; + case ImportOutcome::Unreadable: + ShowMessageBox("That file could not be opened. Nothing was imported.", + kTitle, 0); + return; + 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". + ShowMessageBox("This file is not a readable bank package (corrupt or " + "truncated). Nothing was imported.", + kTitle, 0); + return; + case ImportOutcome::IntegrityFailed: { + std::string msg = "This bank package is damaged (entry " + + quoted(r.failedEntryName) + + " failed its integrity check). Nothing was imported."; + ShowMessageBox(msg.c_str(), kTitle, 0); + return; + } + case ImportOutcome::WriteFailed: { + std::string msg = "Import failed and was rolled back. Nothing was added."; + reportRollback(r.rollback, msg); + ShowMessageBox(msg.c_str(), kTitle, 0); + return; + } + case ImportOutcome::IndexRejected: { + std::string msg = "The bank index rejected the import. Nothing was added."; + reportRollback(r.rollback, msg); + ShowMessageBox(msg.c_str(), kTitle, 0); + return; + } + } +} + +// FIRST, before the picker: making the user find and choose a file we have already +// decided to refuse is the wrong order. +bool ledgerPermits(ReaSamplerSession& session) { + const package::LedgerRefusal refusal = + package::importLedgerRefusal(session.ledgerStatus()); + if (refusal == package::LedgerRefusal::None) return true; + reportLedgerRefusal(refusal); + return false; +} + +} // namespace + +void doImportBankPackage(ReaSamplerSession& session) { + if (!ledgerPermits(session)) return; + std::string path; + if (!pickPackageForImport(path) || path.empty()) return; + report(importBankPackage(session, path)); +} + +void doImportBankPackageFile(ReaSamplerSession& session, const std::string& packageAbsPath) { + if (packageAbsPath.empty()) return; + if (!ledgerPermits(session)) return; + report(importBankPackage(session, packageAbsPath)); +} + +} // namespace reasampler diff --git a/src/shell/actions/package_import_action.h b/src/shell/actions/package_import_action.h new file mode 100644 index 0000000..719b7a5 --- /dev/null +++ b/src/shell/actions/package_import_action.h @@ -0,0 +1,19 @@ +#pragma once +// package_import_action — the bindable/menu/drop skin over importBankPackage: the +// ledger gate (which runs BEFORE the picker, so a refusal never costs the user a file +// choice), the picker itself, and every message the import produces. + +#include + +namespace reasampler { + +class ReaSamplerSession; + +// Gate, pick, import, report. The bound action and the panel's bank menu both call this. +void 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); + +} // namespace reasampler diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index b4db0c4..4d51e78 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -6,11 +6,12 @@ The filesystem and dialog acts behind bank-package export/import: streaming pack file I/O plus the file-status and exclusive-create acts (`package_io`), the UTF-8 path conversion every one of them goes through (`package_path`), the landed-file journal and its rollback delete (`package_rollback`), and the two file pickers -(`package_pickers`). This seam is bytes-only — the package format (magic, manifest, -entry layout) is `core/package`'s business, and the export/import verbs that -orchestrate both do not live here yet. No REAPER project state is touched in this -directory: no ext-state read or write, no undo block, no generation bump — those -belong to the verbs. +(`package_pickers`). Those are bytes-only — the package format (magic, manifest, entry +layout) is `core/package`'s business. Beside them sits the import verb, split so its +decisions stay testable: `import_landing` (REAPER-free) decides and writes, +`import_bank` owns the only REAPER project state this directory touches (the +ext-state persist, the undo block, the generation bump). The export verb does not +live here yet. ## Invariants @@ -65,7 +66,14 @@ belong to the verbs. landed files with no index entry and a journal that now refuses to roll them back — after which `rollback()` refuses and `writeLandedFile` refuses. (Destroying an armed journal without calling either does NOT roll it back — see - `LandedFileJournal`'s own doc comment.) + `LandedFileJournal`'s own doc comment.) `import_bank` honours it: it calls + `markIndexCommitted()` only after `persistBankOp` has returned. +- **Integrity is proven before the first byte lands, not undone after.** + `landPackage` hashes every declared payload against the manifest and only then + creates the bank folder, so a damaged package costs no rollback at all and cannot + leave debris behind a rollback that itself failed. The second read of each payload + is deliberate on a once-per-gesture path — do not fold it into one + hash-and-write pass. - **Both pickers ride `GetUserFileName`** — mode 1 for import, mode 0 for export. There is no platform split and no fallback: `main.cpp` defines `REAPERAPI_IMPLEMENT` without `REAPERAPI_MINIMAL` and aborts the extension load if any single name fails @@ -77,6 +85,8 @@ belong to the verbs. - `package_io` — every filesystem act the verbs need: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), `fileStatus` (Present/Absent/Unreadable — export's refusal message must distinguish the last two, and an empty payload cannot), `writeFileExclusive` (exclusive create + write, self-cleaning on a partial write), and `listFolderFileNames` (bare UTF-8 names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW. - `package_rollback` — `LandedFileJournal`: `writeLandedFile` (exclusive-create land, path resolved absolute, recorded on success only), `markIndexCommitted` (disarms the journal), and `rollback` (deletes exactly the recorded set, hard unlink, tolerating a vanished file; refuses once disarmed). REAPER-free; tested without a DAW. - `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`. Compile-only until the verbs land; nothing here can be exercised in a unit test. +- `import_landing` — the import's two halves that decide anything: `landPackage` (decode, plan, verify EVERY payload's digest, then land through the journal) and `applyImportedBank` (the new bank's entries plus a birth record per landed file, in one straight-line block). REAPER-free deliberately — all-or-nothing, integrity and birth-record behaviour are assertable without a DAW. +- `import_bank` — the promptless import verb over a live `ReaSamplerSession`: the project directory, the minted bank id, the `recordCreated` writer, and the one undo-batched persist. REAPER-facing, so it compiles into the extension module rather than into a library with a test target. ## Gotchas diff --git a/src/shell/package/CMakeLists.txt b/src/shell/package/CMakeLists.txt index aff4414..3003b1b 100644 --- a/src/shell/package/CMakeLists.txt +++ b/src/shell/package/CMakeLists.txt @@ -9,6 +9,14 @@ reasampler_test(package_io LINK package_io) reasampler_pure_library(package_rollback SOURCES package_rollback.cpp LINK PUBLIC package_io) reasampler_test(package_rollback LINK package_rollback) +# The import's decisions and its file half, both REAPER-free, so all-or-nothing, +# integrity and birth-record behaviour are assertable without a DAW. The REAPER-facing +# verb over them (import_bank.cpp) compiles into the extension module instead. +reasampler_pure_library(import_landing + SOURCES import_landing.cpp + LINK PUBLIC import_plan package_rollback bank_book PRIVATE bank_package wav_codec) +reasampler_test(import_landing LINK import_landing bank_package app_version wav_codec origin_ledger) + # The pickers call the REAPER API, so no test target can exercise them; declared as a # library so the TU stays compiled. reaper_plugin.h pulls SWELL in on non-Windows. add_library(package_pickers STATIC package_pickers.cpp) diff --git a/src/shell/package/import_bank.cpp b/src/shell/package/import_bank.cpp new file mode 100644 index 0000000..777b165 --- /dev/null +++ b/src/shell/package/import_bank.cpp @@ -0,0 +1,94 @@ +// import_bank.cpp — see import_bank.h for the contract. The REAPER-facing half of the +// import: the project directory, the minted bank id, the birth records, and the one +// undo-batched persist. Every decision it makes is in import_landing / import_plan. +// +// main.cpp owns the API pointers; this TU gets them extern. DAW-verified, not unit tested. + +#include "shell/package/import_bank.h" + +#include +#include +#include +#include + +#include "core/capture/capture_paths.h" // projectDirOfRpp +#include "shell/bank_ops/bank_ops.h" // persistBankOp — one bank op is one Ctrl-Z +#include "shell/persist/session.h" + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_genGuid +#define REAPERAPI_WANT_guidToString +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +std::string activeProjectDir() { + std::vector buf(4096, '\0'); + EnumProjects(-1, buf.data(), static_cast(buf.size())); + return capture::projectDirOfRpp(std::string(buf.data())); +} + +// The model mints no ids (it stays pure and deterministic), so the shell does — the +// same GUID pair bankOpCreate uses. +std::string mintBankId() { + GUID g{}; + genGuid(&g); + char buf[64] = {0}; // guidToString needs >=64 chars (SDK contract) + guidToString(&g, buf); + return std::string(buf); +} + +void fillPlanCounts(ImportBankResult& out, const package::ImportPlan& plan) { + out.bankDisplayName = plan.bankDisplayName; + out.seedBankName = plan.seedBankName; + out.bankNameAdjusted = plan.bankNameAdjusted; + out.landedCount = plan.landCount; + out.renamedCount = plan.renameCount; + out.collapsedCount = plan.collapseCount; +} + +} // namespace + +ImportBankResult importBankPackage(ReaSamplerSession& session, + const std::string& packageAbsPath) { + ImportBankResult out; + + // A per-import disambiguator, the same shape capture and ingest file under. + const std::string uniqueTag = + std::to_string(static_cast(std::time(nullptr))); + + LandedFileJournal journal; + const ImportLanding landing = landPackage(packageAbsPath, activeProjectDir(), + session.book(), uniqueTag, journal); + out.outcome = landing.outcome; + out.header = landing.header; + out.failedEntryName = landing.failedEntryName; + out.rollback = landing.rollback; + fillPlanCounts(out, landing.plan); + if (landing.outcome != ImportOutcome::Landed) return out; + + const bool applied = applyImportedBank( + session.book(), mintBankId(), landing.plan, + [&session](const model::Sample& s) { + session.recordCreated(s, tracking::OriginKind::PackageImport); + }); + if (!applied) { + out.outcome = ImportOutcome::IndexRejected; + out.rollback = journal.rollback(); + return out; + } + + // 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. + persistBankOp(session, "ReaSampler: import bank package", /*bumpGeneration=*/true); + // Only now: the files are referenced, so prune's self-cleanup carve-out no longer + // covers them (see package_rollback.h). + journal.markIndexCommitted(); + + return out; +} + +} // namespace reasampler diff --git a/src/shell/package/import_bank.h b/src/shell/package/import_bank.h new file mode 100644 index 0000000..94433cc --- /dev/null +++ b/src/shell/package/import_bank.h @@ -0,0 +1,38 @@ +#pragma once +// shell/package/import_bank — the promptless import verb: one package becomes one NEW +// bank in the live session, completely or not at all. No prompts, no message boxes, no +// picker — it reports and the action skin (shell/actions/package_import_action) speaks. +// The ledger gate is the skin's, because it must refuse BEFORE a file is even chosen. + +#include + +#include "core/package/import_plan.h" +#include "shell/package/import_landing.h" + +namespace reasampler { + +class ReaSamplerSession; + +struct ImportBankResult { + ImportOutcome outcome = ImportOutcome::Unreadable; + package::PackageHeader header; // TooNew names the writer's build from here + + 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 collapsedCount = 0; + + std::string failedEntryName; + RollbackResult rollback; +}; + +// Lands `packageAbsPath` as a new bank in `session`, in ONE undo point, bumping the +// bank generation so live instances reload. Places no timeline item. On any failure +// nothing remains on disk and the book is untouched. +ImportBankResult importBankPackage(ReaSamplerSession& session, + const std::string& packageAbsPath); + +} // namespace reasampler diff --git a/src/shell/package/import_landing.cpp b/src/shell/package/import_landing.cpp new file mode 100644 index 0000000..f51a338 --- /dev/null +++ b/src/shell/package/import_landing.cpp @@ -0,0 +1,137 @@ +// import_landing.cpp — see import_landing.h for the contract. REAPER-free: standard +// filesystem only, so every property this file decides is unit-testable. + +#include "shell/package/import_landing.h" + +#include +#include +#include +#include +#include + +#include "core/capture/wav_codec.h" // hashBytes — the digest the manifest records +#include "core/package/bank_package.h" +#include "shell/package/package_io.h" +#include "shell/package/package_path.h" + +namespace reasampler { + +namespace fs = std::filesystem; +using package::EntryAction; +using package::PackageEntrySpan; + +namespace { + +ImportLanding refusal(ImportOutcome outcome, const package::PackageHeader& header) { + ImportLanding out; + out.outcome = outcome; + out.header = header; + return out; +} + +// Reads the package head incrementally: requiredPrefixSize may grow its answer as +// fields arrive, so ask, read to the count, ask again. False means these bytes can +// never frame a package, or the file is shorter than its own header claims. +bool readPrefix(PackageFileReader& reader, std::vector& prefix) { + for (;;) { + const auto need = package::requiredPrefixSize(prefix); + if (!need) return false; + if (prefix.size() >= *need) return true; + PayloadBuffer head = reader.readRange(0, *need); + if (head.size() != *need) return false; + prefix.assign(head.data(), head.data() + head.size()); + } +} + +} // namespace + +ImportLanding landPackage(const std::string& packageAbsPath, + const std::string& projectDir, + const BankBook& destination, + const std::string& uniqueTag, + LandedFileJournal& journal) { + const package::PackageHeader noHeader; + if (projectDir.empty()) return refusal(ImportOutcome::NoProject, noHeader); + + PackageFileReader reader(packageAbsPath); + if (!reader.ok()) return refusal(ImportOutcome::Unreadable, noHeader); + + std::vector prefix; + if (!readPrefix(reader, prefix)) return refusal(ImportOutcome::Malformed, noHeader); + + const package::DecodedPackage dec = package::decodePackage(prefix, reader.fileSize()); + if (dec.status == package::PackageReadability::TooNew) + return refusal(ImportOutcome::TooNew, dec.header); + if (dec.status != package::PackageReadability::Readable) + return refusal(ImportOutcome::Malformed, dec.header); + + const std::string bankDir = package::bankFolderDir(projectDir); + + ImportLanding out; + out.header = dec.header; + out.plan = package::planImport(dec.manifest, destination, projectDir, + listFolderFileNames(bankDir), uniqueTag); + + // Integrity first, over EVERY declared entry — including one the plan collapses, + // since a package that fails its own digest is refused whole rather than partly + // trusted. Nothing is on disk yet, so a failure here needs no rollback. + for (std::size_t i = 0; i < dec.layout.size(); ++i) { + const PackageEntrySpan& span = dec.layout[i]; + // The format refuses a zero-length entry on encode; one arriving anyway cannot + // be told from a failed read at this seam, so it is not well-formed input. + if (span.length == 0) return refusal(ImportOutcome::Malformed, dec.header); + + PayloadBuffer payload = reader.readRange(span.offset, span.length); + if (payload.size() != span.length) { + out.outcome = ImportOutcome::IntegrityFailed; + out.failedEntryName = span.name; + return out; + } + if (capture::hashBytes(payload.data(), payload.size()) != + dec.manifest.entries[i].byteHash) { + out.outcome = ImportOutcome::IntegrityFailed; + out.failedEntryName = span.name; + return out; + } + } + + std::error_code ec; + fs::create_directories(utf8Path(bankDir), ec); // idempotent; the write reports failure + + for (const package::PlannedEntry& e : out.plan.entries) { + if (e.action != EntryAction::Land) continue; + const PackageEntrySpan& span = dec.layout[e.manifestIndex]; + PayloadBuffer payload = reader.readRange(span.offset, span.length); + if (payload.size() == span.length && + journal.writeLandedFile(bankDir + "/" + e.destFileName, payload)) { + continue; + } + out.outcome = ImportOutcome::WriteFailed; + out.failedEntryName = e.destFileName; + out.rollback = journal.rollback(); + return out; + } + + out.outcome = ImportOutcome::Landed; + return out; +} + +bool applyImportedBank(BankBook& book, const std::string& bankId, + const package::ImportPlan& plan, const RecordBirth& recordBirth) { + 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); + // Unconditional on the add's outcome: the file exists either way, and an + // unrecorded file is permanently unreclaimable. + recordBirth(e.sample); + } + + book.bank(bankId)->slots = plan.slots; + book.reconcileSlots(); + return true; +} + +} // namespace reasampler diff --git a/src/shell/package/import_landing.h b/src/shell/package/import_landing.h new file mode 100644 index 0000000..7dc85e3 --- /dev/null +++ b/src/shell/package/import_landing.h @@ -0,0 +1,64 @@ +#pragma once +// shell/package/import_landing — the import's filesystem half and its index half, +// both REAPER-free so the all-or-nothing, integrity and birth-record properties are +// assertable without a DAW. The verb that drives them against a live session is +// import_bank; the REAPER-facing reporting is shell/actions/package_import_action. + +#include +#include + +#include "core/model/bank_book.h" +#include "core/model/bank_model.h" +#include "core/package/import_plan.h" +#include "core/package/package_format.h" +#include "shell/package/package_rollback.h" + +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. +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 + WriteFailed, // a write failed partway; the landed files were rolled back + IndexRejected, // the book refused the bank the plan minted a free name for +}; + +struct ImportLanding { + ImportOutcome outcome = ImportOutcome::Unreadable; + // Meaningful from the moment the header parsed — a TooNew refusal names the + // writer's build, which is the only part of that message a user can act on. + package::PackageHeader header; + package::ImportPlan plan; + std::string failedEntryName; // IntegrityFailed / WriteFailed + RollbackResult rollback; // IntegrityFailed / WriteFailed +}; + +// Streams `packageAbsPath` into the project's bank folder: decode, plan, verify EVERY +// payload's digest, then land. Verification runs to completion before the first write, +// so a damaged package costs no rollback at all. Mutates no index and holds at most +// one payload at a time. `journal` is left armed on success — the caller applies the +// plan to the book and only then disarms it. +ImportLanding landPackage(const std::string& packageAbsPath, + const std::string& projectDir, + const BankBook& destination, + const std::string& uniqueTag, + LandedFileJournal& journal); + +// Called for every landed file, in the same straight-line block as its bank add — +// core/tracking/CLAUDE.md's no-silent-gaps invariant, kept structural by passing the +// writer in rather than letting a caller add first and record later. +using RecordBirth = std::function; + +// Adds the plan's landed entries to a NEW bank under `bankId`. False (no mutation) +// only if the book refuses the create — the name was minted free against this same +// book, so that means the book changed underneath the plan. +bool applyImportedBank(BankBook& book, const std::string& bankId, + const package::ImportPlan& plan, const RecordBirth& recordBirth); + +} // namespace reasampler diff --git a/src/shell/panel/CLAUDE.md b/src/shell/panel/CLAUDE.md index b8da4c2..0cc9763 100644 --- a/src/shell/panel/CLAUDE.md +++ b/src/shell/panel/CLAUDE.md @@ -48,7 +48,7 @@ live in `shell/bank_ops`, a sibling directory, not here. ## Modules - `bank_panel` (`shell/panel/`: `panel_window` / `panel_layout` / `panel_render` / `panel_input` / `panel_drag` / `panel_thumbnails` / `panel_audition` / `panel_bank_ops`, sharing state via `panel_state.h` — Q-W2 split of the former god-module into eight TUs) — docked LICE-drawn grid with three-zone layout: top toolbar (Capture → Maintenance → Placement via `action_bar`, short labels, More (⋯) overflow menu via `overflow_menu`), bottom toolbar (four opposite-mode tag buttons + Show Both), and footer (`[Arrange|Design]` toggle, Tail button, Prune via `footer_bar`). Grid renders in sparse slot order with gap cells, drop dispatch, metadata overlay, and selection via `accent/tertiary` purple border. Draws through the L1 kit by palette role; OS drag-out via `drag_out` + `drag_out_win`. `panel_window` owns the SWELL dialog lifecycle + dialog proc + drop-target opt-in; `panel_layout` the toolbar/footer/menu rects + vertical-split geometry (the one geometry source both paint and hit-test read); `panel_render` the WM_PAINT draw; `panel_input` click/wheel/keyboard routing + the new-content auto-tag timer; `panel_drag` the hover + card-drag state machine + drop dispatch; `panel_thumbnails` the PCM→envelope thumbnail cache + the bank-change fingerprint pass; `panel_audition` the preview-playback engine; `panel_bank_ops` the menu/prompt UX skin over the promptless `shell/bank_ops` verbs. `draw_kit` (shared with the VST3 editor) stays a separate TU. - - `panel_window` — SWELL dialog lifecycle + dialog proc + drop-target opt-in. + - `panel_window` — SWELL dialog lifecycle + dialog proc + drop-target opt-in. The drop splits by extension: a `.rsbank` is a whole bank and routes to the package-import action (one NEW bank each), everything else keeps the audio-ingest route. - `panel_layout` — toolbar/footer/menu rects + vertical-split geometry (the one geometry source both paint and hit-test read). - `panel_render` — the WM_PAINT draw. - `panel_input` — click/wheel/keyboard routing + the new-content auto-tag timer. diff --git a/src/shell/panel/panel_bank_ops.cpp b/src/shell/panel/panel_bank_ops.cpp index 0ce2fbb..6518ee3 100644 --- a/src/shell/panel/panel_bank_ops.cpp +++ b/src/shell/panel/panel_bank_ops.cpp @@ -16,6 +16,7 @@ #include "shell/panel/panel_state.h" #include "shell/panel/panel_bank_ops.h" +#include "shell/actions/package_import_action.h" // doImportBankPackage — the menu's import row #include "shell/bank_ops/bank_ops.h" // bankOp* promptless verbs #include "shell/persist/session.h" // ReaSamplerSession — the live session the ops mutate @@ -242,6 +243,7 @@ enum : unsigned int { kMenuEvacuate, kMenuCreate, kMenuRemove, // remove selected sample(s) from the source bank + kMenuImportPackage, // land a .rsbank as a NEW bank (never merges into this one) kMenuMoveBase = 1000, // move-to-bank: kMenuMoveBase + destination index kMenuCopyBase = 2000, // copy-to-bank: kMenuCopyBase + destination index }; @@ -267,6 +269,7 @@ void showTabMenu(int screenX, int screenY, const std::string& bankId) { menuAppend(menu, kMenuDelete, "Delete..."); menuSeparator(menu); menuAppend(menu, kMenuCreate, "New bank..."); + menuAppend(menu, kMenuImportPackage, "Import bank package..."); const int cmd = TrackPopupMenu(menu, TPM_RETURNCMD, screenX, screenY, 0, g_panel.hwnd, nullptr); @@ -278,6 +281,11 @@ void showTabMenu(int screenX, int screenY, const std::string& bankId) { case kMenuEvacuate: doEvacuateBank(bankId); break; case kMenuDelete: doDeleteBank(bankId); break; case kMenuCreate: doCreateBank(); break; + // 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); + break; default: break; } } diff --git a/src/shell/panel/panel_window.cpp b/src/shell/panel/panel_window.cpp index 9cc90f8..a953d3d 100644 --- a/src/shell/panel/panel_window.cpp +++ b/src/shell/panel/panel_window.cpp @@ -15,6 +15,7 @@ #include "shell/panel/draw_kit.h" #include "shell/actions/ingest.h" +#include "shell/actions/package_import_action.h" #ifdef _WIN32 #include // GET_X_LPARAM / GET_Y_LPARAM (SWELL supplies them on mac/linux) @@ -40,12 +41,24 @@ PanelState g_panel; namespace { +bool isPackagePath(const std::string& path) { + static const std::string kExt = ".rsbank"; + if (path.size() <= kExt.size()) return false; + std::string tail = path.substr(path.size() - kExt.size()); + for (char& c : tail) + if (c >= 'A' && c <= 'Z') c = static_cast(c - 'A' + 'a'); + return tail == kExt; +} + // DragQueryFile(hDrop, 0xFFFFFFFF, ...) returns the file count; each path is then // queried by index (length first, excludes NUL, then a sized buffer). DragFinish -// always frees the shell-allocated drop buffer. Multi-file drop imports all into -// the active bank (bank-fill only — no assignment to any live instance). +// always frees the shell-allocated drop buffer. A .rsbank is a whole bank, not audio, +// so it routes to the import verb (one new bank each); everything else keeps the +// existing ingest route — multi-file drop imports all into the active bank (bank-fill +// only, no assignment to any live instance). void handleDropFiles(HDROP hDrop) { std::vector paths; + std::vector packages; const UINT count = DragQueryFile(hDrop, 0xFFFFFFFF, nullptr, 0); paths.reserve(count); for (UINT i = 0; i < count; ++i) { @@ -54,9 +67,14 @@ void handleDropFiles(HDROP hDrop) { std::vector buf(static_cast(len) + 1, '\0'); DragQueryFile(hDrop, i, buf.data(), static_cast(buf.size())); std::string p(buf.data()); - if (!p.empty()) paths.push_back(std::move(p)); + if (p.empty()) continue; + if (isPackagePath(p)) packages.push_back(std::move(p)); + 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 (!paths.empty()) ingestDroppedFiles(paths); } diff --git a/src/shell/persist/session.h b/src/shell/persist/session.h index ffbaad0..d8a808b 100644 --- a/src/shell/persist/session.h +++ b/src/shell/persist/session.h @@ -94,6 +94,12 @@ public: // treatment. void recordCreated(const model::Sample& sample, tracking::OriginKind kind); + // Whether a birth record can be written at all this session — the status WITHOUT + // 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. + tracking::LedgerStatus ledgerStatus() const { return trackingStatus_; } + // The version that last wrote the active project: PreVersioning (no // stamp), Unknown (malformed), or Stamped. const version::WritingVersion& writingVersion() const { return writingVersion_; } diff --git a/tests/test_import_landing.cpp b/tests/test_import_landing.cpp new file mode 100644 index 0000000..e97f7f0 --- /dev/null +++ b/tests/test_import_landing.cpp @@ -0,0 +1,394 @@ +// Standalone tests for shell/package/import_landing — no REAPER, no framework, real +// package bytes on a real filesystem. Packages are FRAMED BY HAND (not by +// encodePackage) so the version-ladder suites can dial formatVersion and +// minReaderVersion independently, and so an encode-side regression cannot hide the +// import's behaviour from itself. + +#include "../src/shell/package/import_landing.h" + +#include +#include +#include +#include +#include +#include + +#include "../src/core/capture/wav_codec.h" +#include "../src/core/package/bank_package.h" +#include "../src/core/tracking/origin_ledger.h" +#include "../src/core/version/app_version.h" +#include "../src/shell/package/package_path.h" + +using namespace reasampler; +using namespace reasampler::package; +using reasampler::model::Sample; +namespace fs = std::filesystem; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +static const char* kTag = "1754000000"; +static const char* kBankId = "import-test-bank"; + +// --- scratch project --------------------------------------------------------- + +// One project directory per suite, torn down after, so no suite can observe another's +// bank folder in listFolderFileNames. +class Scratch { +public: + explicit Scratch(const std::string& name) + : dir_(pathToUtf8(fs::current_path() / utf8Path("import_scratch_" + name))) { + std::error_code ec; + fs::remove_all(utf8Path(dir_), ec); + fs::create_directories(utf8Path(dir_), ec); + } + ~Scratch() { + std::error_code ec; + fs::remove_all(utf8Path(dir_), ec); + } + const std::string& projectDir() const { return dir_; } + std::string bankDir() const { return bankFolderDir(dir_); } + std::string packagePath() const { return dir_ + "/bank.rsbank"; } + + std::vector bankFiles() const { + std::vector out; + std::error_code ec; + for (const auto& e : fs::directory_iterator(utf8Path(bankDir()), ec)) + if (e.is_regular_file(ec)) out.push_back(pathToUtf8(e.path().filename())); + return out; + } + +private: + std::string dir_; +}; + +static void writeBytes(const std::string& path, const std::vector& bytes) { + std::ofstream f(utf8Path(path), std::ios::binary | std::ios::trunc); + f.write(reinterpret_cast(bytes.data()), + static_cast(bytes.size())); +} + +static std::vector readBytes(const std::string& path) { + std::ifstream f(utf8Path(path), std::ios::binary); + return std::vector(std::istreambuf_iterator(f), + std::istreambuf_iterator()); +} + +// --- hand-rolled package framing -------------------------------------------- + +static void putU32(std::vector& out, std::uint32_t v) { + for (int b = 0; b < 4; ++b) out.push_back(static_cast((v >> (b * 8)) & 0xFFu)); +} + +static std::vector frame(std::uint32_t formatVersion, + std::uint32_t minReaderVersion, + const std::string& writerSemver, + const std::string& manifestJson, + const std::vector>& payloads) { + std::vector out(kPackageMagic, kPackageMagic + 4); + putU32(out, formatVersion); + putU32(out, minReaderVersion); + putU32(out, static_cast(writerSemver.size())); + out.insert(out.end(), writerSemver.begin(), writerSemver.end()); + putU32(out, static_cast(manifestJson.size())); + out.insert(out.end(), manifestJson.begin(), manifestJson.end()); + for (const auto& p : payloads) out.insert(out.end(), p.begin(), p.end()); + return out; +} + +static std::vector payloadOf(std::size_t n, std::uint8_t seed) { + std::vector v(n); + for (std::size_t i = 0; i < n; ++i) v[i] = static_cast(seed + i * 13u); + return v; +} + +struct Fixture { + PackageManifest manifest; + std::vector> payloads; +}; + +static void addEntry(Fixture& f, const std::string& fileName, const std::string& id, + const std::string& contentHash, std::uint8_t seed) { + std::vector payload = payloadOf(48 + seed, seed); + PackageEntry e; + e.fileName = fileName; + e.byteLength = payload.size(); + e.byteHash = capture::hashBytes(payload.data(), payload.size()); + e.sample.id = id; + e.sample.displayName = id; + e.sample.relativePath = "reasampler_bank/" + fileName; + e.sample.contentHash = contentHash; + f.manifest.entries.push_back(std::move(e)); + f.payloads.push_back(std::move(payload)); +} + +static Fixture twoEntryFixture(const std::string& bankName) { + Fixture f; + f.manifest.bankDisplayName = bankName; + addEntry(f, "kick.wav", "cap-kick", "h-kick", 1); + addEntry(f, "snare.wav", "cap-snare", "h-snare", 2); + return f; +} + +// Frames `f` with this build's own ladder pair unless overridden. +static std::vector packageBytes(const Fixture& f, + std::uint32_t formatVersion = kPackageFormatVersion, + std::uint32_t minReader = kPackageMinReaderVersion, + const std::string& manifestOverride = {}) { + const auto json = serializeManifest(f.manifest); + const std::string body = manifestOverride.empty() ? *json : manifestOverride; + return frame(formatVersion, minReader, version::stampVersion(), body, f.payloads); +} + +// --- the sequence the verb runs, minus REAPER -------------------------------- + +struct RunResult { + ImportLanding landing; + bool applied = false; + tracking::OriginLedger ledger; +}; + +static RunResult runImport(const Scratch& scratch, BankBook& book, + const std::string& bankId = kBankId) { + RunResult r; + LandedFileJournal journal; + r.landing = landPackage(scratch.packagePath(), scratch.projectDir(), book, kTag, journal); + if (r.landing.outcome != ImportOutcome::Landed) return r; + r.applied = applyImportedBank(book, bankId, r.landing.plan, [&r](const Sample& s) { + tracking::OriginRecord rec; + rec.relativePath = s.relativePath; + rec.kind = tracking::OriginKind::PackageImport; + rec.sampleId = s.id; + r.ledger.record(rec); + }); + if (r.applied) journal.markIndexCommitted(); + return r; +} + +// --- suites ------------------------------------------------------------------ + +static void testCleanImportLandsEveryPayloadByteExact() { + Scratch scratch("clean"); + const Fixture f = twoEntryFixture("Drums"); + writeBytes(scratch.packagePath(), packageBytes(f)); + + BankBook book; + const RunResult r = runImport(scratch, book); + + CHECK(r.landing.outcome == ImportOutcome::Landed); + CHECK(r.applied); + CHECK(scratch.bankFiles().size() == 2); + for (std::size_t i = 0; i < 2; ++i) { + const std::string name = r.landing.plan.entries[i].destFileName; + CHECK(readBytes(scratch.bankDir() + "/" + name) == f.payloads[i]); + } + const Bank* bank = book.bank(kBankId); + CHECK(bank != nullptr && bank->displayName == "Drums"); + CHECK(bank->index.size() == 2); + CHECK(PayloadBuffer::alive() == 0); +} + +static void testEveryLandedFileHasAPackageImportBirthRecord() { + Scratch scratch("births"); + const Fixture f = twoEntryFixture("Drums"); + writeBytes(scratch.packagePath(), packageBytes(f)); + + BankBook book; + const RunResult r = runImport(scratch, book); + CHECK(r.applied); + + // Read the ledger back rather than counting calls. + CHECK(r.ledger.size() == 2); + for (const std::string& name : scratch.bankFiles()) { + const tracking::OriginRecord* rec = + r.ledger.find("reasampler_bank/" + name); + CHECK(rec != nullptr); + if (rec) CHECK(rec->kind == tracking::OriginKind::PackageImport); + } +} + +static void testACollapsedEntryLeavesNoFileBehind() { + Scratch scratch("collapse"); + Fixture f; + f.manifest.bankDisplayName = "Drums"; + addEntry(f, "kick.wav", "cap-a", "same-hash", 1); + addEntry(f, "kick_copy.wav", "cap-b", "same-hash", 2); + writeBytes(scratch.packagePath(), packageBytes(f)); + + BankBook book; + const RunResult r = runImport(scratch, book); + CHECK(r.landing.outcome == ImportOutcome::Landed); + // One file, one entry, one birth record — the dedup never manufactured an orphan. + CHECK(scratch.bankFiles().size() == 1); + CHECK(book.bank(kBankId)->index.size() == 1); + CHECK(r.ledger.size() == 1); +} + +static void testHashMismatchLandsNothingAndMutatesNothing() { + Scratch scratch("integrity"); + const Fixture f = twoEntryFixture("Drums"); + std::vector bytes = packageBytes(f); + bytes.back() ^= 0xFFu; // corrupt the LAST entry's payload + writeBytes(scratch.packagePath(), bytes); + + BankBook book; + const BankBook before = book; + const RunResult r = runImport(scratch, book); + + CHECK(r.landing.outcome == ImportOutcome::IntegrityFailed); + CHECK(r.landing.failedEntryName == "snare.wav"); + // Refused BEFORE landing anything, not landed-then-rolled-back: the bank folder is + // created on the way into the write loop, so its absence dates the refusal. + CHECK(!fs::exists(utf8Path(scratch.bankDir()))); + CHECK(r.landing.rollback.deletedCount == 0); + CHECK(book == before); // zero index mutation + CHECK(!r.applied); +} + +static void testWriteFailureAtEntryTwoRollsBackEntryOne() { + Scratch scratch("rollback"); + const Fixture f = twoEntryFixture("Drums"); + writeBytes(scratch.packagePath(), packageBytes(f)); + + // Occupy the second entry's destination with a DIRECTORY: listFolderFileNames sees + // regular files only, so the plan does not rename around it, and the exclusive + // create then fails exactly where the injection wants it. + std::error_code ec; + fs::create_directories(utf8Path(scratch.bankDir()), ec); + fs::create_directory(utf8Path(scratch.bankDir() + "/snare.wav"), ec); + + BankBook book; + const BankBook before = book; + const RunResult r = runImport(scratch, book); + + CHECK(r.landing.outcome == ImportOutcome::WriteFailed); + CHECK(r.landing.failedEntryName == "snare.wav"); + CHECK(r.landing.rollback.deletedCount == 1); // entry one was rolled back + CHECK(scratch.bankFiles().empty()); // nothing from this import survives + CHECK(book == before); + CHECK(PayloadBuffer::alive() == 0); +} + +static void testTooNewRefusesWholeAndNamesTheWriter() { + Scratch scratch("toonew"); + const Fixture f = twoEntryFixture("Drums"); + writeBytes(scratch.packagePath(), + packageBytes(f, kPackageFormatVersion + 1, kPackageFormatVersion + 1)); + + BankBook book; + const BankBook before = book; + const RunResult r = runImport(scratch, book); + + CHECK(r.landing.outcome == ImportOutcome::TooNew); + // All three facts the refusal must name are available from the landing. + CHECK(r.landing.header.minReaderVersion == kPackageFormatVersion + 1); + CHECK(r.landing.header.writerVersion == version::stampVersion()); + // The refusal returns before the bank folder is even created. + CHECK(!fs::exists(utf8Path(scratch.bankDir()))); + CHECK(book == before); +} + +static void testNewerFormatVersionStillImportsWhenTheReaderIsReachable() { + Scratch scratch("additive"); + const Fixture f = twoEntryFixture("Drums"); + // An additive newer writer: formatVersion moved, minReaderVersion did not, and the + // manifest carries a key this build has never heard of. + std::string json = *serializeManifest(f.manifest); + CHECK(!json.empty() && json.front() == '{'); + json.insert(1, "\"futureKey\":{\"nested\":[1,2,3]},"); + writeBytes(scratch.packagePath(), + packageBytes(f, kPackageFormatVersion + 1, kPackageMinReaderVersion, json)); + + BankBook book; + const RunResult r = runImport(scratch, book); + + CHECK(r.landing.outcome == ImportOutcome::Landed); + CHECK(r.landing.header.formatVersion == kPackageFormatVersion + 1); + CHECK(book.bank(kBankId)->index.size() == 2); + CHECK(scratch.bankFiles().size() == 2); +} + +static void testTruncationAndGarbageReportMalformedNotTooNew() { + { + Scratch scratch("truncated"); + const Fixture f = twoEntryFixture("Drums"); + std::vector bytes = packageBytes(f); + bytes.pop_back(); + writeBytes(scratch.packagePath(), bytes); + + BankBook book; + const BankBook before = book; + const RunResult r = runImport(scratch, book); + CHECK(r.landing.outcome == ImportOutcome::Malformed); + CHECK(book == before); + } + { + Scratch scratch("garbage"); + writeBytes(scratch.packagePath(), payloadOf(200, 9)); + BankBook book; + CHECK(runImport(scratch, book).landing.outcome == ImportOutcome::Malformed); + } +} + +static void testMissingPackageFileIsUnreadableNotMalformed() { + Scratch scratch("absent"); + BankBook book; + CHECK(runImport(scratch, book).landing.outcome == ImportOutcome::Unreadable); +} + +static void testUnsavedProjectRefusesBeforeAnythingIsRead() { + Scratch scratch("noproject"); + writeBytes(scratch.packagePath(), packageBytes(twoEntryFixture("Drums"))); + BankBook book; + LandedFileJournal journal; + const ImportLanding landing = + landPackage(scratch.packagePath(), std::string{}, book, kTag, journal); + CHECK(landing.outcome == ImportOutcome::NoProject); +} + +static void testReimportingIntoTheSourceProjectLandsBesideAnUntouchedOriginal() { + Scratch scratch("roundtrip"); + const Fixture f = twoEntryFixture("B"); + writeBytes(scratch.packagePath(), packageBytes(f)); + + // A project that already holds bank "B" with the package's own entries. + BankBook book; + book.createBank("bank-b", "B"); + for (const PackageEntry& e : f.manifest.entries) book.index("bank-b")->add(e.sample); + const BankModel originalB = *book.index("bank-b"); + + const RunResult second = runImport(scratch, book, "bank-b2"); + CHECK(second.landing.outcome == ImportOutcome::Landed); + CHECK(book.bank("bank-b2")->displayName == "B 2"); + CHECK(book.bank("bank-b2")->index.size() == 2); + CHECK(*book.index("bank-b") == originalB); // B itself unmutated + for (const Sample& s : book.index("bank-b2")->all()) { + CHECK(s.id.rfind(kImportIdPrefix, 0) == 0); + CHECK(s.id != "cap-kick" && s.id != "cap-snare"); + } + + const RunResult third = runImport(scratch, book, "bank-b3"); + CHECK(third.landing.outcome == ImportOutcome::Landed); + CHECK(book.bank("bank-b3")->displayName == "B 3"); + CHECK(*book.index("bank-b") == originalB); + // Six distinct files: the original two plus two per re-import, never overwritten. + CHECK(scratch.bankFiles().size() == 4); +} + +int main() { + testCleanImportLandsEveryPayloadByteExact(); + testEveryLandedFileHasAPackageImportBirthRecord(); + testACollapsedEntryLeavesNoFileBehind(); + testHashMismatchLandsNothingAndMutatesNothing(); + testWriteFailureAtEntryTwoRollsBackEntryOne(); + testTooNewRefusesWholeAndNamesTheWriter(); + testNewerFormatVersionStillImportsWhenTheReaderIsReachable(); + testTruncationAndGarbageReportMalformedNotTooNew(); + testMissingPackageFileIsUnreadableNotMalformed(); + testUnsavedProjectRefusesBeforeAnythingIsRead(); + testReimportingIntoTheSourceProjectLandsBesideAnUntouchedOriginal(); + + if (g_fail == 0) std::printf("import_landing: all tests passed\n"); + return g_fail == 0 ? 0 : 1; +} diff --git a/tests/test_import_plan.cpp b/tests/test_import_plan.cpp new file mode 100644 index 0000000..760d650 --- /dev/null +++ b/tests/test_import_plan.cpp @@ -0,0 +1,370 @@ +// Standalone tests for reasampler::package::import_plan — no REAPER, no filesystem, +// no test framework. Every one of the four collision classes (sample id, bank-folder +// file name, content hash, bank display name) is exercised here, which is the point of +// the module: the whole collision rule set is decidable from strings and hashes. + +#include "../src/core/package/import_plan.h" + +#include +#include +#include + +#include "../src/core/tracking/tracking_authority.h" + +using namespace reasampler; +using namespace reasampler::package; +using reasampler::model::Sample; +using reasampler::model::SlotMap; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +static const char* kProjectDir = "/proj"; +static const char* kTag = "1754000000"; + +// --- fixtures ---------------------------------------------------------------- + +static PackageEntry entry(const std::string& fileName, const std::string& id, + const std::string& hash) { + PackageEntry e; + e.fileName = fileName; + e.byteLength = 64; + e.byteHash = "0011223344556677"; + e.sample.id = id; + e.sample.displayName = id; + e.sample.relativePath = "reasampler_bank/" + fileName; + e.sample.contentHash = hash; + return e; +} + +static PackageManifest manifestOf(std::vector entries, + const std::string& bankName) { + PackageManifest m; + m.bankDisplayName = bankName; + m.entries = std::move(entries); + return m; +} + +// A book carrying the named banks, in order, each with a caller-supplied id. +static BankBook bookWithBanks(const std::vector& names) { + BankBook book; + for (std::size_t i = 0; i < names.size(); ++i) + book.createBank("bank-" + std::to_string(i), names[i]); + return book; +} + +static const PlannedEntry& landed(const ImportPlan& plan, std::size_t manifestIndex) { + return plan.entries[manifestIndex]; +} + +// --- the bank-name probe (collision class 4) --------------------------------- + +static std::string plannedName(const std::vector& existingBanks, + const std::string& packageBankName) { + const BankBook book = bookWithBanks(existingBanks); + return planImport(manifestOf({}, packageBankName), book, kProjectDir, {}, kTag) + .bankDisplayName; +} + +static void testFreeSeedIsKeptVerbatim() { + CHECK(plannedName({"Percussion"}, "Drums") == "Drums"); + const ImportPlan plan = + planImport(manifestOf({}, "Drums"), bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(!plan.bankNameAdjusted); + CHECK(plan.seedBankName == "Drums"); +} + +static void testFoldedCollisionTakesTheFirstSuffix() { + // The book's fold is case- and whitespace-insensitive, so "drums" blocks "Drums". + CHECK(plannedName({"drums"}, "Drums") == "Drums 2"); + CHECK(plannedName({" DRUMS "}, "Drums") == "Drums 2"); + + const ImportPlan plan = + planImport(manifestOf({}, "Drums"), bookWithBanks({"drums"}), kProjectDir, {}, kTag); + CHECK(plan.bankNameAdjusted); + CHECK(plan.seedBankName == "Drums"); // the message needs what was asked for +} + +static void testProbeFillsAGap() { + // First-free-ascending, not highest-plus-one: "Drums 2" is free, so it wins. + CHECK(plannedName({"Drums", "Drums 3"}, "Drums") == "Drums 2"); +} + +static void testSeedIsNeverReparsed() { + // "Drums 2" colliding lands as "Drums 2 2", NOT "Drums 3" — a bare trailing integer + // cannot be told from a name the user wrote. + CHECK(plannedName({"Drums 2"}, "Drums 2") == "Drums 2 2"); + CHECK(plannedName({"Kit 808"}, "Kit 808") == "Kit 808 2"); +} + +static void testBlankRecordedNameFallsBackToTheDefault() { + CHECK(plannedName({}, "") == kDefaultImportBankName); + CHECK(plannedName({}, " \t ") == kDefaultImportBankName); + // And the fallback is a seed like any other, so a second one suffixes. + CHECK(plannedName({kDefaultImportBankName}, "") == + std::string(kDefaultImportBankName) + " 2"); +} + +static void testPoolExportLandsAsANamedBank() { + // The destination's pool always exists and always carries the protected name + // "Pool", so a pool export imports as a NAMED bank "Pool 2" — intended, not a glitch. + CHECK(plannedName({}, "Pool") == "Pool 2"); + const ImportPlan plan = + planImport(manifestOf({}, "Pool"), bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(plan.bankNameAdjusted); +} + +static void testRepeatedImportsWalkTheSuffixUpwards() { + CHECK(plannedName({"B"}, "B") == "B 2"); + CHECK(plannedName({"B", "B 2"}, "B") == "B 3"); +} + +// --- sample ids (collision class 1) ------------------------------------------ + +static void testEveryIdIsRemintedUnderTheImportPrefix() { + const PackageManifest m = manifestOf({entry("kick.wav", "cap-1-kick.wav", "h1"), + entry("snare.wav", "cap-2-snare.wav", "h2")}, + "Drums"); + const ImportPlan plan = planImport(m, bookWithBanks({}), kProjectDir, {}, kTag); + + CHECK(plan.landCount == 2); + for (const PlannedEntry& e : plan.entries) { + CHECK(e.sample.id.rfind(kImportIdPrefix, 0) == 0); + CHECK(e.sample.id != "cap-1-kick.wav"); + CHECK(e.sample.id != "cap-2-snare.wav"); + } + CHECK(plan.entries[0].sample.id != plan.entries[1].sample.id); +} + +static void testReimportingIntoTheSourceProjectRemintsRatherThanCollides() { + // The package came FROM this project, so its ids are the ones already in use. + BankBook book = bookWithBanks({"B"}); + Sample existing; + existing.id = "cap-1-kick.wav"; + existing.relativePath = "reasampler_bank/kick.wav"; + existing.contentHash = "h1"; + book.index("bank-0")->add(existing); + + const PackageManifest m = manifestOf({entry("kick.wav", "cap-1-kick.wav", "h1")}, "B"); + const ImportPlan plan = + planImport(m, book, kProjectDir, {"kick.wav"}, kTag); + + CHECK(plan.bankDisplayName == "B 2"); + CHECK(landed(plan, 0).sample.id != "cap-1-kick.wav"); + // The hash lives in another bank; cross-bank dedup is deliberately not enforced, + // so the entry still lands rather than collapsing onto B's copy. + CHECK(landed(plan, 0).action == EntryAction::Land); + CHECK(landed(plan, 0).renamed); +} + +static void testParentIsRemappedWhenItTravelledInThePackage() { + PackageEntry parent = entry("kick.wav", "cap-parent", "h1"); + PackageEntry child = entry("kick_r2.wav", "cap-child", "h2"); + child.sample.provenance = model::Provenance{"cap-parent", "fx-snapshot"}; + + const ImportPlan plan = planImport(manifestOf({parent, child}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + + CHECK(landed(plan, 1).sample.provenance.has_value()); + CHECK(landed(plan, 1).sample.provenance->parentSampleId == + landed(plan, 0).sample.id); + CHECK(landed(plan, 1).sample.provenance->fxChainSnapshot == "fx-snapshot"); +} + +static void testParentIsRemappedEvenWhenItFollowsTheChild() { + // Manifest order does not constrain lineage, so the remap runs after every id is minted. + PackageEntry child = entry("kick_r2.wav", "cap-child", "h2"); + child.sample.provenance = model::Provenance{"cap-parent", ""}; + PackageEntry parent = entry("kick.wav", "cap-parent", "h1"); + + const ImportPlan plan = planImport(manifestOf({child, parent}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(landed(plan, 0).sample.provenance->parentSampleId == landed(plan, 1).sample.id); +} + +static void testForeignParentIsClearedNotCarried() { + PackageEntry child = entry("kick.wav", "cap-child", "h1"); + child.sample.provenance = model::Provenance{"cap-not-in-this-package", "fx"}; + + const ImportPlan plan = planImport(manifestOf({child}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(landed(plan, 0).sample.provenance.has_value()); + CHECK(landed(plan, 0).sample.provenance->parentSampleId.empty()); + CHECK(landed(plan, 0).sample.provenance->fxChainSnapshot == "fx"); +} + +// --- bank-folder file names (collision class 2) ------------------------------ + +static void testAFreeBankLegalNameIsKept() { + const ImportPlan plan = planImport(manifestOf({entry("kick.wav", "a", "h1")}, "Drums"), + bookWithBanks({}), kProjectDir, + {"unrelated.wav"}, kTag); + CHECK(landed(plan, 0).destFileName == "kick.wav"); + CHECK(!landed(plan, 0).renamed); + CHECK(plan.renameCount == 0); + CHECK(landed(plan, 0).sample.relativePath == "reasampler_bank/kick.wav"); +} + +static void testATakenNameIsMintedFreshAndNeverOverwritten() { + const ImportPlan plan = planImport(manifestOf({entry("kick.wav", "a", "h1")}, "Drums"), + bookWithBanks({}), kProjectDir, + {"kick.wav"}, kTag); + CHECK(landed(plan, 0).destFileName != "kick.wav"); + CHECK(landed(plan, 0).renamed); + CHECK(plan.renameCount == 1); + CHECK(landed(plan, 0).sample.relativePath == + "reasampler_bank/" + landed(plan, 0).destFileName); +} + +static void testTheFolderNameCheckFoldsAsciiCase() { + // Windows and the default APFS would land "kick.wav" onto "KICK.WAV". + const ImportPlan plan = planImport(manifestOf({entry("kick.wav", "a", "h1")}, "Drums"), + bookWithBanks({}), kProjectDir, + {"KICK.WAV"}, kTag); + CHECK(landed(plan, 0).destFileName != "kick.wav"); + CHECK(landed(plan, 0).renamed); +} + +static void testTwoEntriesNeverLandOnOneName() { + // Two package names that differ only by case are one destination file. + const ImportPlan plan = + planImport(manifestOf({entry("kick.wav", "a", "h1"), entry("Kick.wav", "b", "h2")}, + "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(plan.landCount == 2); + CHECK(landed(plan, 0).destFileName != landed(plan, 1).destFileName); +} + +static void testAnUnsanitaryNameIsMintedRatherThanLandedVerbatim() { + const ImportPlan plan = + planImport(manifestOf({entry("Hit One.wav", "a", "h1")}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(landed(plan, 0).destFileName.find(' ') == std::string::npos); + CHECK(landed(plan, 0).renamed); +} + +// --- content hash (collision class 3) ---------------------------------------- + +static void testAnAlreadyLandedHashCollapsesWithoutAWrite() { + const ImportPlan plan = + planImport(manifestOf({entry("kick.wav", "a", "same"), + entry("kick_copy.wav", "b", "same")}, + "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + + CHECK(plan.landCount == 1); + CHECK(plan.collapseCount == 1); + CHECK(landed(plan, 0).action == EntryAction::Land); + CHECK(landed(plan, 1).action == EntryAction::Collapse); + // No name is claimed for it — a dedup that wrote a file would manufacture an orphan. + CHECK(landed(plan, 1).destFileName.empty()); + // Every manifest entry still yields exactly one planned entry: planImport is total. + CHECK(plan.entries.size() == 2); +} + +static void testAParentPointingAtACollapsedEntryResolvesToTheSurvivor() { + PackageEntry first = entry("kick.wav", "cap-first", "same"); + PackageEntry dupe = entry("kick_copy.wav", "cap-dupe", "same"); + PackageEntry child = entry("kick_r2.wav", "cap-child", "other"); + child.sample.provenance = model::Provenance{"cap-dupe", ""}; + + const ImportPlan plan = planImport(manifestOf({first, dupe, child}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(landed(plan, 2).sample.provenance->parentSampleId == landed(plan, 0).sample.id); +} + +static void testAnEmptyHashNeverCollapses() { + // Mirrors findByHash: an unhashable entry does not participate in dedup. + const ImportPlan plan = + planImport(manifestOf({entry("a.wav", "a", ""), entry("b.wav", "b", "")}, "Drums"), + bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(plan.landCount == 2); + CHECK(plan.collapseCount == 0); +} + +// --- slots ------------------------------------------------------------------- + +static void testSlotsRideAlongOverTheRemintedIds() { + PackageManifest m = manifestOf({entry("kick.wav", "cap-a", "h1"), + entry("snare.wav", "cap-b", "h2")}, + "Drums"); + // A gap the package carried: slot 0 empty, occupants at 1 and 3. + m.slots = SlotMap::fromEntries({{"cap-a", 1}, {"cap-b", 3}}); + + const ImportPlan plan = planImport(m, bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(plan.slots.slotOf(landed(plan, 0).sample.id) == 1); + CHECK(plan.slots.slotOf(landed(plan, 1).sample.id) == 3); + // The package's own ids are gone from the map — a foreign id never enters the index. + CHECK(plan.slots.slotOf("cap-a") == -1); +} + +static void testACollapsedEntryDoesNotDoubleOccupyASlot() { + PackageManifest m = manifestOf({entry("kick.wav", "cap-a", "same"), + entry("kick_copy.wav", "cap-b", "same")}, + "Drums"); + m.slots = SlotMap::fromEntries({{"cap-a", 0}, {"cap-b", 1}}); + + const ImportPlan plan = planImport(m, bookWithBanks({}), kProjectDir, {}, kTag); + CHECK(plan.slots.size() == 1); + CHECK(plan.slots.slotOf(landed(plan, 0).sample.id) == 0); +} + +// --- the ledger gate --------------------------------------------------------- + +static void testDegradedLedgerStatusesRefuseAndTheUsableOnesDoNot() { + CHECK(importLedgerRefusal(tracking::LedgerStatus::Unreadable) == + LedgerRefusal::Malformed); + CHECK(importLedgerRefusal(tracking::LedgerStatus::FutureVersion) == + LedgerRefusal::FutureVersion); + CHECK(importLedgerRefusal(tracking::LedgerStatus::Fresh) == LedgerRefusal::None); + CHECK(importLedgerRefusal(tracking::LedgerStatus::Loaded) == LedgerRefusal::None); +} + +static void testAnUndecodableUsageKeyBlocksPruneButNotImport() { + // The tempting reuse of PruneReport::blockedByTracking would silently refuse an + // import over a key that only ever governs what a DELETION may touch. + tracking::OriginLedger ledger; + wire::UsageFoldResult usage; + usage.abortPrune = true; + usage.offendingKeys = {"rsusage_{ABC}"}; + + const tracking::TrackingState state{tracking::LedgerStatus::Loaded, ledger, usage}; + CHECK(tracking::pruneProtection(state).blocked); + CHECK(importLedgerRefusal(tracking::LedgerStatus::Loaded) == LedgerRefusal::None); +} + +int main() { + testFreeSeedIsKeptVerbatim(); + testFoldedCollisionTakesTheFirstSuffix(); + testProbeFillsAGap(); + testSeedIsNeverReparsed(); + testBlankRecordedNameFallsBackToTheDefault(); + testPoolExportLandsAsANamedBank(); + testRepeatedImportsWalkTheSuffixUpwards(); + + testEveryIdIsRemintedUnderTheImportPrefix(); + testReimportingIntoTheSourceProjectRemintsRatherThanCollides(); + testParentIsRemappedWhenItTravelledInThePackage(); + testParentIsRemappedEvenWhenItFollowsTheChild(); + testForeignParentIsClearedNotCarried(); + + testAFreeBankLegalNameIsKept(); + testATakenNameIsMintedFreshAndNeverOverwritten(); + testTheFolderNameCheckFoldsAsciiCase(); + testTwoEntriesNeverLandOnOneName(); + testAnUnsanitaryNameIsMintedRatherThanLandedVerbatim(); + + testAnAlreadyLandedHashCollapsesWithoutAWrite(); + testAParentPointingAtACollapsedEntryResolvesToTheSurvivor(); + testAnEmptyHashNeverCollapses(); + + testSlotsRideAlongOverTheRemintedIds(); + testACollapsedEntryDoesNotDoubleOccupyASlot(); + + testDegradedLedgerStatusesRefuseAndTheUsableOnesDoNot(); + testAnUndecodableUsageKeyBlocksPruneButNotImport(); + + if (g_fail == 0) std::printf("import_plan: all tests passed\n"); + return g_fail == 0 ? 0 : 1; +} From f8dde16a7ef3f313946bfe612cde50d1fa1e1f0c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 14:02:05 -0400 Subject: [PATCH 2/2] =?UTF-8?q?import:=20remediate=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20ledger=20gate,=20docs,=20message=20split?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/PLAN.md | 15 ++-- docs/product/bank-package.md | 6 +- src/core/model/bank_book.cpp | 7 +- src/core/package/CLAUDE.md | 18 +++- src/core/package/import_plan.cpp | 67 +++++++++++++-- src/core/package/import_plan.h | 14 ++- src/core/util/CLAUDE.md | 5 +- src/core/util/ascii_ws.h | 10 +++ src/shell/actions/CLAUDE.md | 2 +- src/shell/actions/package_import_action.cpp | 95 +++++++++++---------- src/shell/actions/package_import_action.h | 11 ++- src/shell/package/import_bank.cpp | 7 +- src/shell/package/import_bank.h | 4 +- src/shell/package/import_landing.cpp | 14 ++- src/shell/package/import_landing.h | 16 ++-- src/shell/panel/panel_bank_ops.cpp | 9 +- src/shell/panel/panel_window.cpp | 22 ++++- src/shell/persist/session.h | 7 ++ tests/test_import_landing.cpp | 4 +- tests/test_import_plan.cpp | 58 ++++++++++++- 20 files changed, 299 insertions(+), 92 deletions(-) create mode 100644 src/core/util/ascii_ws.h diff --git a/docs/PLAN.md b/docs/PLAN.md index 0833a9a..b6f2e5a 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2366,7 +2366,7 @@ These bind every track in this phase, in addition to the plan-wide set above. touching it must **cite, not restate**. - **Export is read-only against the project.** No ext-state write, no `bumpBankGeneration()`, no undo point. Import does the opposite: it bumps the generation - (`src/shell/persist/session.h:108`) so live ReaSampler 9000 instances reload, and batches + (`src/shell/persist/session.h:114`) so live ReaSampler 9000 instances reload, and batches its index mutation into one Ctrl-Z through `persistBankOp`. - **All-or-nothing on both sides.** No partial export, no partial import. A truncated `.rsbank` must never exist on disk (temp file + atomic rename, the Ψ-W2-T2 precedent); a @@ -2396,10 +2396,13 @@ new directories** (`src/core/package/`, `src/shell/package/`) that no other phas not read here. The only pre-existing files any Ε track edits are named per track below — `core/tracking/origin_ledger` (W1-T3, exclusively), the root `CMakeLists.txt` `add_subdirectory` list (W1-T1 and W1-T2, one line each), `src/app/main.cpp` and the panel's -bank menu (W2-T1 and W2-T2, one registration line and one menu row each), and +bank menu (W2-T1 and W2-T2, one registration line and one menu row each), `core/model/bank_book.{h,cpp}` (W2-T2 only — **one additive public `const` member**, required -by the Ε-F2 auto-suffix rule so the name fold keeps its single home). **No Ε track -touches `core/instrument/`, `shell/instrument/`, or any capture backend.** +by the Ε-F2 auto-suffix rule so the name fold keeps its single home), and +`shell/persist/session.h` (W2-T2 only — one additive public accessor, `ledgerStatus()`, so the +import gate can key on `LedgerStatus` alone without going through `pruneDryRun()`'s +enumeration+scan). **No Ε track touches `core/instrument/`, `shell/instrument/`, or any +capture backend.** --- @@ -2764,7 +2767,7 @@ fold, it does not add a second one), `origin_ledger` (Ε-W1-T3's). `"Drums 3"` — a bare trailing integer is indistinguishable from `"Kit 808"`); the probe **fills gaps** (first-free, not highest-plus-one, so it is a pure function of the current name set); the probe **terminates** by pigeonhole within `B + 1` candidates for `B` banks, - so **no arbitrary cap**; and the fold is `BankBook`'s own (`bank_book.h:252-258`), reached + so **no arbitrary cap**; and the fold is `BankBook`'s own (`bank_book.h:263-269`), reached through the new public member, never re-implemented in `import_plan`. Sample display names are **not** suffixed, and `slot_map` positions are untouched. - **Always a new bank; never a merge (Ε-F2, ruled).** The import creates a bank — it never @@ -2787,7 +2790,7 @@ fold, it does not add a second one), `origin_ledger` (Ε-W1-T3's). remain as orphans until a prune reclaims them — the same designed window a non-empty bank delete already produces (`core/model/CLAUDE.md`'s sample-removal section). The user-facing summary says so. -- **`bumpBankGeneration()` on success** (`session.h:108`), so live instances reload. +- **`bumpBankGeneration()` on success** (`session.h:114`), so live instances reload. - **No timeline item is placed. Ever.** - **A new FOREVER-STABLE command id**, minted the same way T1's is. diff --git a/docs/product/bank-package.md b/docs/product/bank-package.md index 92c9ee4..5f4ad9b 100644 --- a/docs/product/bank-package.md +++ b/docs/product/bank-package.md @@ -381,7 +381,7 @@ trim, the seed is the literal `Imported bank`. **The probe.** Let `seed` be that string and `fold(x)` be `BankBook`'s own uniqueness key — strip leading/trailing ASCII whitespace, lower-case ASCII letters -(`bank_book.h:252-258`). Take the **first** name in this sequence whose fold is not +(`bank_book.h:263-269`). Take the **first** name in this sequence whose fold is not already carried by a bank in the destination book: seed, seed + " 2", seed + " 3", seed + " 4", … @@ -410,7 +410,7 @@ implementations diverge:** `B + 1` candidates is free by pigeonhole, so no cap is needed and none should be added. 4. **The fold has exactly one home.** `import_plan` must **not** re-implement - `nameKey` — `bank_book.h:252-258` says in as many words that a drifted second copy + `nameKey` — `bank_book.h:263-269` says in as many words that a drifted second copy would let the uniqueness invariant be violated. The probe therefore runs behind `BankBook`'s own folding, which means Ε-W2-T2 adds **one additive public `const` member** to `BankBook` (recommended: `std::string uniqueDisplayName(const @@ -690,7 +690,7 @@ constructors. freshly-generated pair. - **Bank generation.** Import mutates bank content that live ReaSampler 9000 instances may play, so it must `bumpBankGeneration()` - (`src/shell/persist/session.h:108`, whose own comment says call sites "err toward + (`src/shell/persist/session.h:114`, whose own comment says call sites "err toward bumping"). Export mutates nothing and must bump nothing, write no ext state, and open no undo point. - **Beta/stable channel isolation.** Packages are channel-**agnostic** and this is diff --git a/src/core/model/bank_book.cpp b/src/core/model/bank_book.cpp index efa5e25..3ecb817 100644 --- a/src/core/model/bank_book.cpp +++ b/src/core/model/bank_book.cpp @@ -3,6 +3,8 @@ #include #include +#include "core/util/ascii_ws.h" + // bank_book implementation — the registry RULES half: construction, pool // privileges, bank lifecycle, active bank, sample movement/removal, slot order, // and the reference queries. The JSON round-trip half lives in bank_book_json.cpp, @@ -76,9 +78,8 @@ void BankBook::normalizeOrdinals() { // one folding rule shared with bank_book_json.cpp's parse-time coalesce. std::string BankBook::nameKey(const std::string& s) { std::size_t b = 0, e = s.size(); - auto isWs = [](char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r'; }; - while (b < e && isWs(s[b])) ++b; - while (e > b && isWs(s[e - 1])) --e; + while (b < e && util::isAsciiWs(s[b])) ++b; + while (e > b && util::isAsciiWs(s[e - 1])) --e; std::string out; out.reserve(e - b); for (std::size_t i = b; i < e; ++i) { diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 5d80a01..6e5d445 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -76,8 +76,11 @@ landing after the format. - `import_plan` — the pure import decision, and the reason the whole feature is testable without a DAW: the destination bank's display name after `BankBook`'s own fold, the reminted sample ids and remapped parents, and the - per-entry land / collapse / rename disposition. Also `importLedgerRefusal`, - the import's ledger gate. + per-entry land / collapse / rename disposition. Also `importLedgerRefusal` (the + import's ledger gate, delegating entirely to `tracking::ledgerDegraded`) and + `ledgerRefusalMessage` (the gate's console-block body, a pure + `(LedgerRefusal, namespace) -> string` fold the shell only supplies the + channel-correct namespace to). - `bank_package` — framing and arithmetic composing the two above: `encodePackage` (prefix bytes + layout + total size, stamping this build's ladder pair and `version::stampVersion()`), `decodePackage` (prefix + observed @@ -156,3 +159,14 @@ landing after the format. not a real entry). `serializeManifest` refuses a zero-length `PackageEntry` at encode so this layer never produces one; decode does not enforce it (a hostile/older package declaring one is not this track's concern). +- **`import_plan`'s `spelledLikeABankFile` mints a fresh name even with NO + collision, and that third condition is a deliberate decision, not spec-derived.** + `docs/product/bank-package.md:447` ties the auto-rename mint to a *collision* + only; `spelledLikeABankFile` additionally mints whenever the package's own name + isn't spelled the way `deriveBankPaths` spells one (extension, sanitized stem). + Kept for two reasons: uniform folder spelling for every landed file regardless of + origin, and — the sharper one — a hostile entry name that isn't a legal Windows + filename or carries an unexpected extension (e.g. `evil.exe`) lands sanitized + (`evil_.wav`) rather than verbatim. `ImportPlan` counts this separately from a + genuine folder-name collision (`sanitizeRenameCount` vs `collisionRenameCount`) so + the summary line means what `bank-package.md:447` says it means. diff --git a/src/core/package/import_plan.cpp b/src/core/package/import_plan.cpp index 372b2c4..1c9c38a 100644 --- a/src/core/package/import_plan.cpp +++ b/src/core/package/import_plan.cpp @@ -6,6 +6,7 @@ #include "core/capture/capture_paths.h" #include "core/package/package_format.h" +#include "core/util/ascii_ws.h" namespace reasampler::package { @@ -14,10 +15,13 @@ namespace { using capture::bankRelativeForName; using capture::deriveBankPaths; using capture::sanitizeStem; +using util::isAsciiWs; +// Shares BankBook::nameKey's whitespace set (core/util/ascii_ws.h) so a name nameKey +// would fold to empty is never treated as recorded here. bool blankName(const std::string& s) { for (char c : s) - if (c != ' ' && c != '\t' && c != '\n' && c != '\r') return false; + if (!isAsciiWs(c)) return false; return true; } @@ -40,6 +44,14 @@ bool spelledLikeABankFile(const std::string& fileName) { // The bank-folder names an import must not land on: what is there already, plus what // this import has minted so far. Case-folded, because the two filesystems this tool // ships on would treat "Kick.wav" and "kick.wav" as one file. +// +// `bankFolderFileNames` comes from `listFolderFileNames` (shell/package/package_io), +// which skips non-regular files — so a DIRECTORY sharing a bank file's name is +// invisible here. The plan then never mints around it, and the later exclusive-create +// land fails on that one entry (WriteFailed, rolled back). Safe direction ("never +// overwrite" still holds) but worth knowing before chasing a WriteFailed report that +// traces back to a same-named folder in the bank directory; test_import_landing's +// rollback suite deliberately exploits this to exercise the rollback path. class NameSet { public: explicit NameSet(const std::vector& present) { @@ -71,13 +83,46 @@ std::string mintFileName(const std::string& projectDir, const std::string& packa } // namespace LedgerRefusal importLedgerRefusal(tracking::LedgerStatus status) { - switch (status) { - case tracking::LedgerStatus::Unreadable: return LedgerRefusal::Malformed; - case tracking::LedgerStatus::FutureVersion: return LedgerRefusal::FutureVersion; - case tracking::LedgerStatus::Fresh: - case tracking::LedgerStatus::Loaded: break; + // Delegates the refuse/proceed decision entirely to ledgerDegraded() rather than + // re-deriving it from the two named statuses, so a future degraded status added + // there is refused here too rather than silently falling through to None. + if (!tracking::ledgerDegraded(status)) return LedgerRefusal::None; + // Below this point status is known degraded; only the message variant is picked. + // Unreadable gets its own "corrupt, may be cleared" wording; every other degraded + // status (today only FutureVersion) gets the "written by a newer build" wording. + return status == tracking::LedgerStatus::Unreadable ? LedgerRefusal::Malformed + : LedgerRefusal::FutureVersion; +} + +// Mirrors prune's abort block in structure and tone (shell/actions/prune_action.cpp), +// 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. +std::string ledgerRefusalMessage(LedgerRefusal refusal, const std::string& extStateNamespace) { + if (refusal == LedgerRefusal::None) return {}; + + std::string msg = + "ReaSampler import: ABORTED -- the file-tracking ledger could not be read. " + "Nothing was imported.\n"; + if (refusal == 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, \"" + extStateNamespace + "\", \"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"; } - return LedgerRefusal::None; + 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"; + return msg; } std::string bankFolderDir(const std::string& projectDir) { @@ -139,7 +184,13 @@ ImportPlan planImport(const PackageManifest& manifest, idRemap[src.sample.id] = e.sample.id; ++plan.landCount; - if (e.renamed) ++plan.renameCount; + // A rename happens for one of two reasons: the package's own name was already + // taken (spelledLikeABankFile true but the mint's fast path lost the race to + // `taken`), or the name never qualified for that fast path at all (sanitize). + if (e.renamed) { + if (spelledLikeABankFile(src.fileName)) ++plan.collisionRenameCount; + else ++plan.sanitizeRenameCount; + } plan.entries.push_back(std::move(e)); } diff --git a/src/core/package/import_plan.h b/src/core/package/import_plan.h index a79dd46..104df91 100644 --- a/src/core/package/import_plan.h +++ b/src/core/package/import_plan.h @@ -32,6 +32,13 @@ enum class LedgerRefusal { None, Malformed, FutureVersion }; LedgerRefusal importLedgerRefusal(tracking::LedgerStatus status); +// The console-block body for a refusal — a pure (LedgerRefusal, namespace) -> string +// fold, so the wording is assertable without a DAW. `extStateNamespace` is the +// channel-correct namespace (`version::extStateNamespace()`) every recovery line must +// name, so a beta user is never handed the stable spelling. Empty string for None — +// callers only reach this once `importLedgerRefusal` has already returned a refusal. +std::string ledgerRefusalMessage(LedgerRefusal refusal, const std::string& extStateNamespace); + // What one manifest entry does when the import runs. // - Land: write the payload under destFileName and add `sample`. // - Collapse: an equal contentHash already lands in this same import, so the payload @@ -56,7 +63,12 @@ struct ImportPlan { model::SlotMap slots; // the package's slots over the reminted ids int landCount = 0; int collapseCount = 0; - int renameCount = 0; + // Two distinct triggers, counted separately (bank-package.md:447 defines the first + // as THE collision counter; conflating the second into it would misreport a mint + // that never collided as a collision). + int collisionRenameCount = 0; // the package's own name was already taken in the bank folder + int sanitizeRenameCount = 0; // the package's name was not spelled the way this tool spells + // a bank file (see spelledLikeABankFile, core/package/CLAUDE.md) }; // The bank folder an import lands into — the same expression capture uses, so an diff --git a/src/core/util/CLAUDE.md b/src/core/util/CLAUDE.md index 94ae77e..de8663f 100644 --- a/src/core/util/CLAUDE.md +++ b/src/core/util/CLAUDE.md @@ -3,8 +3,8 @@ ## Scope Tiny, dependency-free pure helpers linked by both artifacts: whole-file byte -loading, unit-interval clamping, the absolute-path rejection test, and the -per-segment envelope curve law. +loading, unit-interval clamping, the absolute-path rejection test, the +per-segment envelope curve law, and the ASCII-whitespace fold test. ## Modules @@ -19,6 +19,7 @@ per-segment envelope curve law. before curves existed play unchanged, and what the knob law's centre detent exists to keep reachable from the dial. - `relative_path` (`core/util`, header-only) — the ONE absolute-path rejection test behind the relative-paths-only invariant, shared by `bank_model` (`Sample.relativePath`) and `core/tracking/origin_ledger` (`OriginRecord.relativePath`). The two must reject identically or a path one accepts could be smuggled past the other; that is why it is one function and not two. +- `ascii_ws` (`core/util`, header-only) — the ONE ASCII-whitespace test (space/tab/CR/LF) behind `BankBook::nameKey`'s trim, shared by `core/package/import_plan`'s blank-bank-name fallback. Same rationale as `relative_path`: two independently-maintained copies could drift on what counts as blank. ## Gotchas diff --git a/src/core/util/ascii_ws.h b/src/core/util/ascii_ws.h new file mode 100644 index 0000000..a8e0c72 --- /dev/null +++ b/src/core/util/ascii_ws.h @@ -0,0 +1,10 @@ +#pragma once +// ascii_ws — the ONE ASCII-whitespace test shared by every fold that must agree with +// BankBook::nameKey's trim (space/tab/CR/LF): a drifted second copy could accept a +// package bank name nameKey would treat as blank, or vice versa. + +namespace reasampler::util { + +inline bool isAsciiWs(char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r'; } + +} // namespace reasampler::util diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index 9f2bb9d..d620190 100644 --- a/src/shell/actions/CLAUDE.md +++ b/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 diff --git a/src/shell/actions/package_import_action.cpp b/src/shell/actions/package_import_action.cpp index 2531858..c8ca753 100644 --- a/src/shell/actions/package_import_action.cpp +++ b/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 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 diff --git a/src/shell/actions/package_import_action.h b/src/shell/actions/package_import_action.h index 719b7a5..34f705e 100644 --- a/src/shell/actions/package_import_action.h +++ b/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 diff --git a/src/shell/package/import_bank.cpp b/src/shell/package/import_bank.cpp index 777b165..2261361 100644 --- a/src/shell/package/import_bank.cpp +++ b/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. diff --git a/src/shell/package/import_bank.h b/src/shell/package/import_bank.h index 94433cc..018ac9d 100644 --- a/src/shell/package/import_bank.h +++ b/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; diff --git a/src/shell/package/import_landing.cpp b/src/shell/package/import_landing.cpp index f51a338..d4c1a26 100644 --- a/src/shell/package/import_landing.cpp +++ b/src/shell/package/import_landing.cpp @@ -3,6 +3,7 @@ #include "shell/package/import_landing.h" +#include #include #include #include @@ -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); diff --git a/src/shell/package/import_landing.h b/src/shell/package/import_landing.h index 7dc85e3..0731892 100644 --- a/src/shell/package/import_landing.h +++ b/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 diff --git a/src/shell/panel/panel_bank_ops.cpp b/src/shell/panel/panel_bank_ops.cpp index 6518ee3..7cee606 100644 --- a/src/shell/panel/panel_bank_ops.cpp +++ b/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; } diff --git a/src/shell/panel/panel_window.cpp b/src/shell/panel/panel_window.cpp index a953d3d..12a188d 100644 --- a/src/shell/panel/panel_window.cpp +++ b/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 // 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); } diff --git a/src/shell/persist/session.h b/src/shell/persist/session.h index d8a808b..0bc1967 100644 --- a/src/shell/persist/session.h +++ b/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 diff --git a/tests/test_import_landing.cpp b/tests/test_import_landing.cpp index e97f7f0..441002c 100644 --- a/tests/test_import_landing.cpp +++ b/tests/test_import_landing.cpp @@ -372,7 +372,9 @@ static void testReimportingIntoTheSourceProjectLandsBesideAnUntouchedOriginal() CHECK(third.landing.outcome == ImportOutcome::Landed); CHECK(book.bank("bank-b3")->displayName == "B 3"); CHECK(*book.index("bank-b") == originalB); - // Six distinct files: the original two plus two per re-import, never overwritten. + // Four distinct files: two per re-import, never overwritten. Bank "B"'s own two + // entries were seeded index-only above (book.index("bank-b")->add), never written + // to disk, so they don't add to this count. CHECK(scratch.bankFiles().size() == 4); } diff --git a/tests/test_import_plan.cpp b/tests/test_import_plan.cpp index 760d650..2f60977 100644 --- a/tests/test_import_plan.cpp +++ b/tests/test_import_plan.cpp @@ -202,7 +202,8 @@ static void testAFreeBankLegalNameIsKept() { {"unrelated.wav"}, kTag); CHECK(landed(plan, 0).destFileName == "kick.wav"); CHECK(!landed(plan, 0).renamed); - CHECK(plan.renameCount == 0); + CHECK(plan.collisionRenameCount == 0); + CHECK(plan.sanitizeRenameCount == 0); CHECK(landed(plan, 0).sample.relativePath == "reasampler_bank/kick.wav"); } @@ -212,7 +213,9 @@ static void testATakenNameIsMintedFreshAndNeverOverwritten() { {"kick.wav"}, kTag); CHECK(landed(plan, 0).destFileName != "kick.wav"); CHECK(landed(plan, 0).renamed); - CHECK(plan.renameCount == 1); + // A genuine folder-name collision, not a spelling mint. + CHECK(plan.collisionRenameCount == 1); + CHECK(plan.sanitizeRenameCount == 0); CHECK(landed(plan, 0).sample.relativePath == "reasampler_bank/" + landed(plan, 0).destFileName); } @@ -224,6 +227,9 @@ static void testTheFolderNameCheckFoldsAsciiCase() { {"KICK.WAV"}, kTag); CHECK(landed(plan, 0).destFileName != "kick.wav"); CHECK(landed(plan, 0).renamed); + // The case-fold hit is still a collision, not a spelling mint. + CHECK(plan.collisionRenameCount == 1); + CHECK(plan.sanitizeRenameCount == 0); } static void testTwoEntriesNeverLandOnOneName() { @@ -242,6 +248,9 @@ static void testAnUnsanitaryNameIsMintedRatherThanLandedVerbatim() { bookWithBanks({}), kProjectDir, {}, kTag); CHECK(landed(plan, 0).destFileName.find(' ') == std::string::npos); CHECK(landed(plan, 0).renamed); + // No collision here (the bank folder is empty) — this is a sanitize mint. + CHECK(plan.sanitizeRenameCount == 1); + CHECK(plan.collisionRenameCount == 0); } // --- content hash (collision class 3) ---------------------------------------- @@ -334,6 +343,46 @@ static void testAnUndecodableUsageKeyBlocksPruneButNotImport() { CHECK(importLedgerRefusal(tracking::LedgerStatus::Loaded) == LedgerRefusal::None); } +// Pins the delegation itself: importLedgerRefusal must refuse EXACTLY the statuses +// ledgerDegraded() calls degraded, over every value the enum has today. A gate that +// re-derived its own notion of "degraded" could silently diverge from this the moment +// either side changes without the other. +static void testImportLedgerRefusalDelegatesToLedgerDegraded() { + const tracking::LedgerStatus all[] = { + tracking::LedgerStatus::Fresh, + tracking::LedgerStatus::Loaded, + tracking::LedgerStatus::Unreadable, + tracking::LedgerStatus::FutureVersion, + }; + for (tracking::LedgerStatus s : all) + CHECK((importLedgerRefusal(s) != LedgerRefusal::None) == tracking::ledgerDegraded(s)); +} + +// --- the ledger-refusal message (pure, so both channels are assertable without a DAW) -- + +static void testLedgerRefusalMessageIsEmptyForNone() { + CHECK(ledgerRefusalMessage(LedgerRefusal::None, "reasampler").empty()); +} + +static void testLedgerRefusalMessageNamesTheChannelCorrectNamespace() { + // The two real namespaces (app_version.h): stable "reasampler", beta "reasampler_beta". + const std::string stable = ledgerRefusalMessage(LedgerRefusal::Malformed, "reasampler"); + CHECK(stable.find("\"reasampler\"") != std::string::npos); + CHECK(stable.find("reasampler_beta") == std::string::npos); + + const std::string beta = ledgerRefusalMessage(LedgerRefusal::Malformed, "reasampler_beta"); + CHECK(beta.find("\"reasampler_beta\"") != std::string::npos); +} + +static void testLedgerRefusalMessageDistinguishesMalformedFromFutureVersion() { + const std::string malformed = ledgerRefusalMessage(LedgerRefusal::Malformed, "reasampler"); + const std::string futureVersion = + ledgerRefusalMessage(LedgerRefusal::FutureVersion, "reasampler"); + CHECK(malformed != futureVersion); + CHECK(malformed.find("malformed") != std::string::npos); + CHECK(futureVersion.find("NEWER version") != std::string::npos); +} + int main() { testFreeSeedIsKeptVerbatim(); testFoldedCollisionTakesTheFirstSuffix(); @@ -364,6 +413,11 @@ int main() { testDegradedLedgerStatusesRefuseAndTheUsableOnesDoNot(); testAnUndecodableUsageKeyBlocksPruneButNotImport(); + testImportLedgerRefusalDelegatesToLedgerDegraded(); + + testLedgerRefusalMessageIsEmptyForNone(); + testLedgerRefusalMessageNamesTheChannelCorrectNamespace(); + testLedgerRefusalMessageDistinguishesMalformedFromFutureVersion(); if (g_fail == 0) std::printf("import_plan: all tests passed\n"); return g_fail == 0 ? 0 : 1;