From f8dde16a7ef3f313946bfe612cde50d1fa1e1f0c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 14:02:05 -0400 Subject: [PATCH] =?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;