From 081b6f10281c93cdb07e2ffd58a95c924e89add7 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 13:21:43 -0400 Subject: [PATCH 1/2] package: one bank leaves the project as one .rsbank, or the export refuses and says why Pure planner classifies missing/unreadable/unrepresentable and repairs transport names; the verb digests, streams and commits atomically over a const session. --- src/app/CMakeLists.txt | 3 +- src/app/main.cpp | 6 + src/core/package/CLAUDE.md | 14 + src/core/package/CMakeLists.txt | 5 + src/core/package/export_plan.cpp | 165 +++++++++ src/core/package/export_plan.h | 89 +++++ src/shell/actions/CLAUDE.md | 1 + src/shell/actions/package_export_action.cpp | 190 ++++++++++ src/shell/actions/package_export_action.h | 18 + src/shell/package/CLAUDE.md | 6 +- src/shell/package/CMakeLists.txt | 9 + src/shell/package/export_bank.cpp | 184 ++++++++++ src/shell/package/export_bank.h | 91 +++++ src/shell/panel/panel_bank_ops.cpp | 4 + tests/test_export_bank.cpp | 375 ++++++++++++++++++++ tests/test_export_plan.cpp | 268 ++++++++++++++ 16 files changed, 1426 insertions(+), 2 deletions(-) create mode 100644 src/core/package/export_plan.cpp create mode 100644 src/core/package/export_plan.h create mode 100644 src/shell/actions/package_export_action.cpp create mode 100644 src/shell/actions/package_export_action.h create mode 100644 src/shell/package/export_bank.cpp create mode 100644 src/shell/package/export_bank.h create mode 100644 tests/test_export_bank.cpp create mode 100644 tests/test_export_plan.cpp diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 044ab51..fa863d4 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -46,13 +46,14 @@ add_library(reaper_reasampler MODULE ${REASAMPLER_SRC_DIR}/shell/actions/design_view_actions.cpp ${REASAMPLER_SRC_DIR}/shell/actions/bank_actions.cpp ${REASAMPLER_SRC_DIR}/shell/actions/prune_action.cpp + ${REASAMPLER_SRC_DIR}/shell/actions/package_export_action.cpp ${REASAMPLER_SRC_DIR}/shell/actions/ingest.cpp ${REASAMPLER_SRC_DIR}/shell/actions/arrange_drop_win.cpp ${REASAMPLER_SRC_DIR}/shell/actions/drag_out_win.cpp ${REASAMPLER_SRC_DIR}/shell/actions/instrument_drop_win.cpp ${REASAMPLER_SRC_DIR}/shell/persist/usage_scan.cpp ) -target_link_libraries(reaper_reasampler PRIVATE json wire file_bytes bank_model capture_paths capture_name peaks bank_grid mode_switch tab_strip view_mode_model view_tree guid_diff lane_keys solo_cache insert_plan render_settings render_window track_topology batch_capture tail_control capture_realtime bank_book wav_codec origin_ledger tracking_authority prune_reconcile prune_button app_version provenance drag_out instrument_drop theme component_geometry action_bar footer_bar overflow_menu mode_enable tooltip card_meta card_drag assignment_request bank_sync sample_usage bake_wire resample_name) +target_link_libraries(reaper_reasampler PRIVATE json wire file_bytes bank_model capture_paths capture_name peaks bank_grid mode_switch tab_strip view_mode_model view_tree guid_diff lane_keys solo_cache insert_plan render_settings render_window track_topology batch_capture tail_control capture_realtime bank_book wav_codec origin_ledger tracking_authority prune_reconcile prune_button app_version provenance drag_out instrument_drop theme component_geometry action_bar footer_bar overflow_menu mode_enable tooltip card_meta card_drag assignment_request bank_sync sample_usage bake_wire resample_name export_bank package_pickers) # NOT linked here, deliberately: sampler_core / pitch_shift / the filter. The instrument # renders its own bake in its own process, which is what keeps the extension's link graph # free of the voice engine — a link edge to it here means the design drifted. diff --git a/src/app/main.cpp b/src/app/main.cpp index 57696ae..599003c 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -26,6 +26,7 @@ #include "shell/actions/action_registry.h" // the registration table #include "shell/actions/bank_actions.h" // multi-bank action family #include "shell/actions/design_view_actions.h" // Design View action family +#include "shell/actions/package_export_action.h" // bank-package export action body #include "core/wire/bake_wire.h" // kBakeActionSuffix (the shared action id) #include "shell/capture/bake_land.h" // resample-bake landing action body #include "shell/capture/capture_batch.h" // batch + recapture action bodies @@ -91,6 +92,9 @@ 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 RunExportBankPackage(int) { + reasampler::doBankPackageExport(g_session, g_session.book().activeBankId()); +} static void RunShowVersion(int) { // On-demand only — no unconditional startup print (routine console chatter pops // the console window). @@ -147,6 +151,8 @@ static std::vector buildMainActionTable() { rows.push_back({reasampler::wire::kBakeActionSuffix, "land pending ReaSampler 9000 resample bake", &RunResampleBake}); + rows.push_back({"EXPORT_BANK_PACKAGE", "export active bank as package", + &RunExportBankPackage}); rows.push_back({"SHOW_VERSION", "show version", &RunShowVersion}); return rows; diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 9114f83..80f475f 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -73,6 +73,12 @@ 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. +- `export_plan` — the pure export decision over value inputs (the bank's members + plus the shell's per-file probe result): the verdict (`Ready` / `Incomplete` / + `Refused`), the transport name per shipping entry, and what is excluded and why + (missing / unreadable / an index record the format cannot represent). Owns the + name repair the codec's refusal backstops, and normalizes each shipping record's + `relativePath` to the bare package name — see the transport-name gotcha below. - `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 @@ -115,6 +121,14 @@ landing after the format. `minReaderVersion` bump, not additive: an old reader would otherwise compare a stored digest against bytes hashed the new way and silently misjudge corruption. +- **A written package carries no path in ANY field.** `isValidNestedSamplePath` + permits a relative `relativePath` because a *record* may hold one, but + `export_plan` writes each shipping entry's `relativePath` as its bare package + name, so an emitted manifest has no separator anywhere and the entry name is the + single naming authority on both sides. The directory component it drops carries + no information — the bank subfolder is a fixed `capture_paths` constant the + importer re-spells. The nested-path rule stays as the decode-side backstop for a + package this build did not write. - **Obligation on the export track: sanitize, don't relay the refusal.** `serializeManifest` returns one indistinguishable `nullopt` for every rejection — an unrepresentable name, a case-folded collision, a traversing nested path, a diff --git a/src/core/package/CMakeLists.txt b/src/core/package/CMakeLists.txt index fc7fd2d..32ae08d 100644 --- a/src/core/package/CMakeLists.txt +++ b/src/core/package/CMakeLists.txt @@ -6,6 +6,11 @@ reasampler_pure_library(package_manifest LINK PUBLIC bank_model slot_map PRIVATE package_format json) reasampler_test(package_manifest LINK package_manifest) +reasampler_pure_library(export_plan + SOURCES export_plan.cpp + LINK PUBLIC package_manifest PRIVATE package_format) +reasampler_test(export_plan LINK export_plan package_format) + # bytes.h is header-only (see src/core/wire/CLAUDE.md) — no wire link edge needed. reasampler_pure_library(bank_package SOURCES bank_package.cpp diff --git a/src/core/package/export_plan.cpp b/src/core/package/export_plan.cpp new file mode 100644 index 0000000..aa4e6a0 --- /dev/null +++ b/src/core/package/export_plan.cpp @@ -0,0 +1,165 @@ +// export_plan.cpp — see export_plan.h for the contract. + +#include "core/package/export_plan.h" + +#include + +#include "core/package/package_format.h" + +namespace reasampler::package { + +namespace { + +// The bare file name a bank-relative path ends in. Separators are matched in both +// spellings: a persisted index may hold either on Windows. +std::string baseNameOf(const std::string& path) { + const std::size_t sep = path.find_last_of("/\\"); + return sep == std::string::npos ? path : path.substr(sep + 1); +} + +// Trailing dots and spaces are stripped at file creation on Windows, so a name +// carrying them would collide with its stripped twin (isValidEntryName refuses them +// for that reason). +std::string stripTrailingDotsAndSpaces(std::string s) { + while (!s.empty() && (s.back() == '.' || s.back() == ' ')) s.pop_back(); + return s; +} + +// Truncates to at most `max` bytes without splitting a UTF-8 sequence — a split one +// would leave the name ill-formed, which isValidEntryName refuses outright. +std::string truncateUtf8(std::string s, std::size_t max) { + if (s.size() <= max) return s; + s.resize(max); + while (!s.empty() && (static_cast(s.back()) & 0xC0) == 0x80) s.pop_back(); + if (!s.empty() && static_cast(s.back()) >= 0xC0) s.pop_back(); + return s; +} + +// `name` with `suffix` inserted before its extension, trimmed so the result still +// fits the entry-name cap. +std::string insertSuffix(const std::string& name, const std::string& suffix) { + const std::size_t dot = name.find_last_of('.'); + const bool hasExt = dot != std::string::npos && dot > 0; + std::string stem = hasExt ? name.substr(0, dot) : name; + const std::string ext = hasExt ? name.substr(dot) : std::string(); + const std::size_t room = kMaxEntryNameBytes - suffix.size() - ext.size(); + stem = truncateUtf8(std::move(stem), room); + return stem + suffix + ext; +} + +bool nameTaken(const std::string& candidate, const std::vector& taken) { + for (const std::string& t : taken) + if (sameEntryName(candidate, t)) return true; + return false; +} + +// A transport name distinct from every name already claimed, under the format's own +// case-folding equivalence (two names differing only by ASCII case would extract onto +// one file on Windows and default APFS). +std::string uniqueEntryName(const std::string& base, const std::vector& taken) { + if (!nameTaken(base, taken)) return base; + // Bounded by construction: each iteration either returns or collides with a + // distinct member of `taken`, and the suffixed names are pairwise distinct. + std::string candidate = base; + for (std::size_t n = 2; n <= taken.size() + 2; ++n) { + candidate = insertSuffix(base, "_" + std::to_string(n)); + if (!nameTaken(candidate, taken) && isValidEntryName(candidate)) return candidate; + } + return candidate; +} + +// What BankModel::add and the manifest's nested-path rule together accept — the pair +// package_manifest::serializeManifest checks per entry. The codec's refusal is the +// backstop; classifying here is what lets the export name the offending entry. +bool recordRepresentable(const model::Sample& s) { + return !s.id.empty() && isValidNestedSamplePath(s.relativePath); +} + +ExcludedEntry excludedFrom(const model::Sample& s, ExclusionReason reason) { + ExcludedEntry e; + e.sampleId = s.id; + e.displayName = s.displayName; + e.relativePath = s.relativePath; + e.reason = reason; + return e; +} + +} // namespace + +std::string sanitizeEntryName(const std::string& rawFileName) { + std::string n = rawFileName; + for (char& c : n) { + const unsigned char u = static_cast(c); + if (u < 0x20 || u == 0x7F || u == '/' || u == '\\' || u == ':' || u == '*' || + u == '?' || u == '|' || u == '<' || u == '>' || u == '"') + c = '_'; + } + // One byte of headroom so the prefix repair below still fits the cap. + n = stripTrailingDotsAndSpaces(truncateUtf8(std::move(n), kMaxEntryNameBytes - 1)); + if (isValidEntryName(n)) return n; + + // One prefix answers every remaining reserved form at once: "." / "..", a DOS + // device name, and a name the strips emptied. + std::string prefixed = stripTrailingDotsAndSpaces("_" + n); + if (isValidEntryName(prefixed)) return prefixed; + + // Ill-formed UTF-8 is what is left, and isValidEntryName is the only authority on + // it here, so fold the whole non-ASCII range rather than re-deriving the scanner. + for (char& c : prefixed) + if (static_cast(c) >= 0x80) c = '_'; + prefixed = stripTrailingDotsAndSpaces(prefixed); + return isValidEntryName(prefixed) ? prefixed : std::string("entry"); +} + +ExportPlan planExport(const ExportInputs& in) { + ExportPlan plan; + plan.manifest.bankDisplayName = in.bankDisplayName; + + bool anyAbsent = false; + bool anyUnrepresentable = false; + std::vector takenNames; + std::vector shippedIds; + + for (const ExportCandidate& c : in.candidates) { + if (!recordRepresentable(c.sample)) { + plan.excluded.push_back( + excludedFrom(c.sample, ExclusionReason::RecordUnrepresentable)); + anyUnrepresentable = true; + continue; + } + if (c.fileState != SourceFileState::Present) { + plan.excluded.push_back(excludedFrom( + c.sample, c.fileState == SourceFileState::Unreadable + ? ExclusionReason::FileUnreadable + : ExclusionReason::FileMissing)); + anyAbsent = true; + continue; + } + + PackageEntry e; + e.sample = c.sample; + e.fileName = uniqueEntryName(sanitizeEntryName(baseNameOf(c.sample.relativePath)), + takenNames); + // The transport record names its payload by the package name and nothing + // else, so the manifest carries no path at all — the bank subfolder is a + // fixed constant the importer re-spells through capture_paths. + e.sample.relativePath = e.fileName; + + takenNames.push_back(e.fileName); + shippedIds.push_back(c.sample.id); + plan.sourceRelativePaths.push_back(c.sample.relativePath); + plan.manifest.entries.push_back(std::move(e)); + } + + // Display positions follow membership: an excluded entry's slot marker would name + // a sample the package does not carry. + plan.manifest.slots = in.slots; + plan.manifest.slots.reconcile(shippedIds); + + plan.verdict = anyUnrepresentable ? ExportVerdict::Refused + : anyAbsent ? ExportVerdict::Incomplete + : ExportVerdict::Ready; + return plan; +} + +} // namespace reasampler::package diff --git a/src/core/package/export_plan.h b/src/core/package/export_plan.h new file mode 100644 index 0000000..77df29d --- /dev/null +++ b/src/core/package/export_plan.h @@ -0,0 +1,89 @@ +#pragma once +// export_plan — the pure export decision: which bank entries ship, what each one is +// named inside the package, what is absent, and therefore whether the export may +// proceed at all. Values in, verdict out — the shell probes the filesystem and hands +// the results here. Pure: no filesystem, no host types. + +#include +#include + +#include "core/model/bank_model.h" +#include "core/model/slot_map.h" +#include "core/package/package_manifest.h" + +namespace reasampler::package { + +// What the shell's filesystem probe found for one indexed entry. Missing and +// Unreadable stay distinct all the way to the refusal message: the file is gone vs. +// the file is there and will not open, which have opposite recoveries. +enum class SourceFileState { + Present, + Missing, + Unreadable, +}; + +struct ExportCandidate { + model::Sample sample; + SourceFileState fileState = SourceFileState::Missing; +}; + +// One bank as the planner sees it: the display name that rides in the manifest +// envelope, the members in bank insertion order, and the bank's display positions. +struct ExportInputs { + std::string bankDisplayName; + std::vector candidates; + model::SlotMap slots; +}; + +// Why an indexed entry cannot ship. +enum class ExclusionReason { + FileMissing, + FileUnreadable, + // The index record itself cannot be written: an empty id, or a relativePath the + // format's nested-path rule refuses. Not something a confirm can proceed past. + RecordUnrepresentable, +}; + +struct ExcludedEntry { + std::string sampleId; + std::string displayName; + std::string relativePath; + ExclusionReason reason = ExclusionReason::FileMissing; +}; + +enum class ExportVerdict { + Ready, // every candidate ships + Incomplete, // a file is absent or unreadable; the rest may ship behind an explicit confirm + Refused, // an index record the format cannot represent — no confirm path +}; + +struct ExportPlan { + ExportVerdict verdict = ExportVerdict::Ready; + + // Entries in bank order, each carrying its transport name and its record. The + // shell measures `byteLength`/`byteHash` from the payload, so they are 0/"" here; + // `exportTimestamp` is the shell's clock read and is 0 here too. + PackageManifest manifest; + + // Where each shipping entry's bytes are read from, parallel to + // `manifest.entries` — the record's own relativePath is normalized to the bare + // package name (see the transport-name note in this directory's CLAUDE.md), so + // the source spelling has to survive separately. + std::vector sourceRelativePaths; + + std::vector excluded; +}; + +ExportPlan planExport(const ExportInputs& in); + +// The smallest repair of one bare file name that satisfies isValidEntryName — +// separators, reserved characters and control bytes to '_', an over-long name +// truncated on a UTF-8 boundary, and an underscore prefix for the reserved forms +// ("." / ".." / a DOS device name). Never returns a name isValidEntryName refuses. +// +// A bank ingested on macOS/Linux legitimately holds names Windows cannot spell, and +// relaying the codec's one indistinguishable refusal would make a single such file +// an unactionable total failure of the whole export. +std::string sanitizeEntryName(const std::string& rawFileName); + +} // namespace reasampler::package diff --git a/src/shell/actions/CLAUDE.md b/src/shell/actions/CLAUDE.md index befbaed..6b4b412 100644 --- a/src/shell/actions/CLAUDE.md +++ b/src/shell/actions/CLAUDE.md @@ -39,6 +39,7 @@ is owned by other directories and only skinned here. ## Modules - `shell/actions` (`action_registry` / `design_view_actions` / `bank_actions` / `prune_action`) — the bindable action families, all routed via the `command_id`/`gaccel`/`hookcommand` contract. `action_registry` owns the shared registration plumbing (interned channel-qualified id strings; register and mirror-unregister present the identical pointer) **and the Q-W6 registration TABLE**: `main.cpp`'s own family (capture scopes, panel toggle, insert, batch, realtime, recapture, version) is one `ActionTableRow` array — suffix, phrase, flat function-pointer handler — that registration, hookcommand dispatch, and the unload mirror-unregister all iterate, so adding an action touches the table only (OCP). Bank mutations flow through the promptless `shell/bank_ops` verbs (`bankOp*` + `persistBankOp`, taking `ReaSamplerSession&`), which the panel menus and `bank_actions` consume as thin UX skins. **Every bank index verb wraps its mutation in a batched REAPER undo point (`Undo_BeginBlock2`/`EndBlock2`, `UNDO_STATE_MISCCFG`) so one bank operation is one Ctrl-Z.** The prune action (`prune_action`, `BANK_PRUNE_FOLDER`) is **the ONLY file-deletion action in the system**; it opens no undo point (file deletion is not REAPER-undoable). `BANK_PRUNE_FOLDER` halts on `blockedByTracking` and prints each blocker that fired, with recovery instructions. +- `package_export_action` — the "export bank as package" skin: survey and report first, confirm what is absent (and, separately, a destination being replaced), pick a destination, write. Every prompt in the flow lives here so `shell/package/export_bank` stays promptless. Read-only against the project — it holds the session by `const&`, so no ext-state write, generation bump or undo point is reachable. Registration rides `main.cpp`'s action table (`EXPORT_BANK_PACKAGE`); the panel's tab menu is the second skin over the same body. - `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. diff --git a/src/shell/actions/package_export_action.cpp b/src/shell/actions/package_export_action.cpp new file mode 100644 index 0000000..9b83c6c --- /dev/null +++ b/src/shell/actions/package_export_action.cpp @@ -0,0 +1,190 @@ +// package_export_action.cpp — see package_export_action.h for the contract this TU +// preserves. main.cpp owns the API pointers; this TU gets them extern. + +#include "shell/actions/package_export_action.h" + +#include +#include +#include +#include +#include + +#include "core/capture/capture_paths.h" // projectDirOfRpp, sanitizeStem +#include "shell/package/export_bank.h" +#include "shell/package/package_pickers.h" +#include "shell/persist/session.h" + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_ShowConsoleMsg +#define REAPERAPI_WANT_ShowMessageBox +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +constexpr const char* kUnsavedProjectMsg = + "ReaSampler export: save the project first -- an unsaved project has no bank folder " + "to read from.\n"; + +std::string currentProjectDir() { + std::vector buf(4096, '\0'); + EnumProjects(-1, buf.data(), static_cast(buf.size())); + return capture::projectDirOfRpp(std::string(buf.data())); +} + +// 6 == YES; anything else cancels (SDK ~6544). +bool confirmed(const std::string& msg, const char* title) { + return ShowMessageBox(msg.c_str(), title, 4) == 6; +} + +std::string entryLine(const package::ExcludedEntry& e) { + const char* why = e.reason == package::ExclusionReason::FileMissing ? "missing" + : e.reason == package::ExclusionReason::FileUnreadable ? "unreadable" + : "unusable index record"; + return " " + (e.displayName.empty() ? e.sampleId : e.displayName) + " [" + why + + "] " + e.relativePath + "\n"; +} + +// `maxLines` == 0 lists everything (the console record); a positive cap keeps a +// confirm dialog readable on a bank with hundreds of absent files, prune's own +// truncate-the-confirm-not-the-report discipline. +std::string excludedManifest(const std::vector& excluded, + std::size_t maxLines) { + std::string msg; + std::size_t shown = 0; + for (const package::ExcludedEntry& e : excluded) { + if (maxLines != 0 && shown == maxLines) { + msg += " ... (" + std::to_string(excluded.size() - shown) + + " more, listed in the console)\n"; + break; + } + msg += entryLine(e); + ++shown; + } + return msg; +} + +void reportOutcome(const ExportOutcome& out, const std::string& destPath) { + switch (out.status) { + case ExportStatus::Written: + ShowConsoleMsg(("ReaSampler export: wrote " + std::to_string(out.entriesWritten) + + " entry/entries (" + std::to_string(out.bytesWritten) + + " bytes) to " + destPath + "\n") + .c_str()); + return; + case ExportStatus::SourceReadFailed: + ShowConsoleMsg(("ReaSampler export: ABORTED -- \"" + out.offendingName + + "\" could not be read. Nothing was written.\n") + .c_str()); + return; + case ExportStatus::SourceChanged: + ShowConsoleMsg(("ReaSampler export: ABORTED -- \"" + out.offendingName + + "\" changed on disk while the package was being written. " + "Nothing was written; run the export again.\n") + .c_str()); + return; + case ExportStatus::EncodeFailed: + ShowConsoleMsg("ReaSampler export: ABORTED -- this bank could not be encoded " + "as a package. Nothing was written.\n"); + return; + // Both refusals are re-derived from a FRESH plan, so reaching them after the + // survey means the bank changed under the export, not that the user declined. + case ExportStatus::RefusedIncomplete: + case ExportStatus::RefusedUnrepresentable: + ShowConsoleMsg("ReaSampler export: ABORTED -- the bank changed between the " + "report and the write. Nothing was written; run the export " + "again.\n"); + return; + case ExportStatus::RefusedDestinationExists: + ShowConsoleMsg("ReaSampler export: cancelled -- nothing was written.\n"); + return; + case ExportStatus::NoSuchBank: + ShowConsoleMsg("ReaSampler export: that bank no longer exists.\n"); + return; + case ExportStatus::NoProjectDir: + ShowConsoleMsg(kUnsavedProjectMsg); + return; + case ExportStatus::WriteFailed: + ShowConsoleMsg(("ReaSampler export: FAILED writing " + destPath + + ". No package was left behind; any file already at that path is " + "untouched.\n") + .c_str()); + return; + } +} + +} // namespace + +void doBankPackageExport(const ReaSamplerSession& session, const std::string& bankId) { + const std::string projectDir = currentProjectDir(); + if (projectDir.empty()) { + ShowConsoleMsg(kUnsavedProjectMsg); + return; + } + + // Report before acting, and before the picker opens: a refusal the user cannot + // act on should not cost them a trip through a save dialog first. + const ExportSurvey survey = surveyBankExport(session, projectDir, bankId); + if (!survey.bankFound) { + ShowConsoleMsg("ReaSampler export: no such bank.\n"); + return; + } + const std::string bankName = survey.plan.manifest.bankDisplayName; + + bool allowIncomplete = false; + if (survey.plan.verdict == package::ExportVerdict::Refused) { + ShowConsoleMsg(("ReaSampler export: ABORTED -- \"" + bankName + + "\" holds index record(s) a package cannot carry. Nothing was " + "written.\n" + + excludedManifest(survey.plan.excluded, 0)) + .c_str()); + return; + } + if (survey.plan.verdict == package::ExportVerdict::Incomplete) { + const std::string headline = + "ReaSampler export: \"" + bankName + "\" has " + + std::to_string(survey.plan.excluded.size()) + + " entry/entries whose file is missing or unreadable:\n"; + ShowConsoleMsg((headline + excludedManifest(survey.plan.excluded, 0)).c_str()); + if (!confirmed(headline + excludedManifest(survey.plan.excluded, 10) + + "\nExport the " + + std::to_string(survey.plan.manifest.entries.size()) + + " present entry/entries anyway?", + "ReaSampler: incomplete bank")) { + ShowConsoleMsg("ReaSampler export: cancelled -- nothing was written.\n"); + return; + } + allowIncomplete = true; + } + + // The bank's own name, not the project's: the artifact is a bank, and a user + // exporting three banks from one project needs three distinguishable files. + const std::string suggested = + projectDir + "/" + capture::sanitizeStem(bankName) + ".rsbank"; + std::string dest; + if (!pickPackageSavePath(suggested, dest)) return; // user cancelled the picker + + ExportRequest req; + req.projectDir = projectDir; + req.bankId = bankId; + req.destAbsPath = dest; + req.exportTimestamp = static_cast(std::time(nullptr)); + req.allowIncomplete = allowIncomplete; + + ExportOutcome out = exportBank(session, req); + if (out.status == ExportStatus::RefusedDestinationExists) { + if (!confirmed("A file already exists at:\n\n " + dest + + "\n\nReplace it with this bank package?", + "ReaSampler: replace package")) { + ShowConsoleMsg("ReaSampler export: cancelled -- nothing was written.\n"); + return; + } + req.allowOverwrite = true; + out = exportBank(session, req); + } + reportOutcome(out, dest); +} + +} // namespace reasampler diff --git a/src/shell/actions/package_export_action.h b/src/shell/actions/package_export_action.h new file mode 100644 index 0000000..695d5a3 --- /dev/null +++ b/src/shell/actions/package_export_action.h @@ -0,0 +1,18 @@ +#pragma once +// package_export_action — the "export bank as package" action body: survey and +// report first, confirm what is absent, pick a destination, write. Every prompt in +// the flow lives here; shell/package/export_bank stays promptless. Registration and +// dispatch for its FOREVER-STABLE id ride main.cpp's action table. + +#include + +namespace reasampler { + +class ReaSamplerSession; + +// Exports one bank (the pool included — it is structurally a bank) to a .rsbank the +// user picks. Read-only against the project: the session is const, so no ext-state +// write, generation bump or undo point is reachable from here. +void doBankPackageExport(const ReaSamplerSession& session, const std::string& bankId); + +} // namespace reasampler diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index b4db0c4..0ab8adc 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -54,7 +54,10 @@ belong to the verbs. `PackageFileWriter` — after any extension append — and get its own consent if that re-checked path is `Present`; the dialog's confirm only ever covered the pre-append path. Not fixed at this seam: prompting is verb-level UX, and `pickPackageSavePath` - has no caller yet, so the gap is latent, not live. + has no caller yet, so the gap is latent, not live. **Closed on the export side:** + `exportBank` re-checks `fileStatus()` on the post-append path and refuses + `RefusedDestinationExists` until the caller sets `allowOverwrite`, which + `package_export_action` does only after its own confirm naming that exact path. - **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.** The citation and the full discriminator live at `package_rollback.cpp`'s header. "Did this call create it" is structural: only exclusively-created paths are @@ -77,6 +80,7 @@ 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. +- `export_bank` — the promptless export verb, in three composable public steps: `surveyBankExport` (the read-only plan, report-before-acting), `digestSources` (measures each entry's length + `hashBytes` digest, one payload at a time), and `writePackageFile` (prefix, then each payload re-read and re-verified against that digest before it is appended, then commit). `exportBank` composes the three and gates on the plan verdict, the incomplete confirm and the destination confirm. The session arrives **const** — every mutator on it is non-const, so "an export writes no ext state, opens no undo point and never bumps the generation" is enforced by the type rather than remembered. Reads the session through inline accessors only, which is why its tests link and run without a DAW. ## Gotchas diff --git a/src/shell/package/CMakeLists.txt b/src/shell/package/CMakeLists.txt index aff4414..caa87da 100644 --- a/src/shell/package/CMakeLists.txt +++ b/src/shell/package/CMakeLists.txt @@ -9,6 +9,15 @@ 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) +# export_bank reads the live session through ReaSamplerSession's INLINE accessors only, +# so it pulls in no REAPER-facing TU and its tests link (and run) without a DAW. +reasampler_pure_library(export_bank + SOURCES export_bank.cpp + LINK PUBLIC export_plan bank_package package_io PRIVATE capture_paths wav_codec) +reasampler_test(export_bank + LINK export_bank bank_book slot_map view_mode_model tail_control origin_ledger + tracking_authority prune_reconcile app_version capture_paths) + # 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/export_bank.cpp b/src/shell/package/export_bank.cpp new file mode 100644 index 0000000..d2aee61 --- /dev/null +++ b/src/shell/package/export_bank.cpp @@ -0,0 +1,184 @@ +// export_bank.cpp — see export_bank.h for the contract. +// +// wav_codec is called for hashBytes ONLY. Payload bytes are copied and hashed, never +// rebuilt, trimmed, normalized or collapsed — the capture path's mono collapse must +// not reach an export. + +#include "shell/package/export_bank.h" + +#include +#include +#include + +#include "core/capture/capture_paths.h" // resolveBankFile — the index's relative -> absolute +#include "core/capture/wav_codec.h" // hashBytes +#include "core/model/bank_book.h" +#include "shell/package/package_io.h" +#include "shell/persist/session.h" // ReaSamplerSession — read through its inline book() only + +namespace reasampler { + +namespace { + +package::SourceFileState stateOf(const std::string& absPath) { + switch (fileStatus(absPath)) { + case FileStatus::Present: return package::SourceFileState::Present; + case FileStatus::Unreadable: return package::SourceFileState::Unreadable; + case FileStatus::Absent: break; + } + return package::SourceFileState::Missing; +} + +std::vector absoluteSources(const std::string& projectDir, + const std::vector& relativePaths) { + std::vector out; + out.reserve(relativePaths.size()); + for (const std::string& rel : relativePaths) + out.push_back(capture::resolveBankFile(projectDir, rel)); + return out; +} + +} // namespace + +ExportSurvey surveyBankExport(const ReaSamplerSession& session, + const std::string& projectDir, + const std::string& bankId) { + ExportSurvey survey; + const Bank* bank = session.book().bank(bankId); + if (!bank) return survey; + survey.bankFound = true; + + package::ExportInputs inputs; + inputs.bankDisplayName = bank->displayName; + inputs.slots = bank->slots; + for (const model::Sample& s : bank->index.all()) { + package::ExportCandidate c; + c.sample = s; + c.fileState = stateOf(capture::resolveBankFile(projectDir, s.relativePath)); + inputs.candidates.push_back(std::move(c)); + } + survey.plan = package::planExport(inputs); + return survey; +} + +bool digestSources(package::PackageManifest& manifest, + const std::vector& sourceAbsPaths, + std::string& outFailedName) { + outFailedName.clear(); + if (sourceAbsPaths.size() != manifest.entries.size()) return false; + for (std::size_t i = 0; i < manifest.entries.size(); ++i) { + const PayloadBuffer payload = readFilePayload(sourceAbsPaths[i]); + if (payload.empty()) { + outFailedName = manifest.entries[i].fileName; + return false; + } + manifest.entries[i].byteLength = payload.size(); + manifest.entries[i].byteHash = capture::hashBytes(payload.data(), payload.size()); + } + return true; +} + +ExportOutcome writePackageFile(const package::EncodedPackage& encoded, + const package::PackageManifest& manifest, + const std::vector& sourceAbsPaths, + const std::string& destAbsPath) { + ExportOutcome out; + if (sourceAbsPaths.size() != manifest.entries.size() || + encoded.layout.size() != manifest.entries.size()) { + out.status = ExportStatus::EncodeFailed; + return out; + } + + // Every early return below abandons the writer through its destructor, which + // removes the temp and leaves the destination untouched. + PackageFileWriter writer(destAbsPath); + if (!writer.ok() || !writer.appendRaw(encoded.prefix.data(), encoded.prefix.size())) { + out.status = ExportStatus::WriteFailed; + return out; + } + std::uint64_t written = encoded.prefix.size(); + + for (std::size_t i = 0; i < manifest.entries.size(); ++i) { + const package::PackageEntry& entry = manifest.entries[i]; + const PayloadBuffer payload = readFilePayload(sourceAbsPaths[i]); + if (payload.empty()) { + out.status = ExportStatus::SourceReadFailed; + out.offendingName = entry.fileName; + return out; + } + if (payload.size() != entry.byteLength || + capture::hashBytes(payload.data(), payload.size()) != entry.byteHash) { + out.status = ExportStatus::SourceChanged; + out.offendingName = entry.fileName; + return out; + } + if (!writer.appendPayload(payload)) { + out.status = ExportStatus::WriteFailed; + return out; + } + written += payload.size(); + } + + if (written != encoded.totalSize || !writer.commit()) { + out.status = ExportStatus::WriteFailed; + return out; + } + out.status = ExportStatus::Written; + out.entriesWritten = manifest.entries.size(); + out.bytesWritten = written; + return out; +} + +ExportOutcome exportBank(const ReaSamplerSession& session, const ExportRequest& req) { + ExportOutcome out; + if (req.projectDir.empty()) { + out.status = ExportStatus::NoProjectDir; + return out; + } + + const ExportSurvey survey = surveyBankExport(session, req.projectDir, req.bankId); + if (!survey.bankFound) { + out.status = ExportStatus::NoSuchBank; + return out; + } + out.bankDisplayName = survey.plan.manifest.bankDisplayName; + out.excluded = survey.plan.excluded; + + if (survey.plan.verdict == package::ExportVerdict::Refused) { + out.status = ExportStatus::RefusedUnrepresentable; + return out; + } + if (survey.plan.verdict == package::ExportVerdict::Incomplete && !req.allowIncomplete) { + out.status = ExportStatus::RefusedIncomplete; + return out; + } + // The save dialog's own overwrite confirm covered the path the USER chose, which + // is not necessarily the path handed here (the picker re-appends `.rsbank`), so + // consent for the real target is re-taken by the skin. + if (!req.allowOverwrite && fileStatus(req.destAbsPath) == FileStatus::Present) { + out.status = ExportStatus::RefusedDestinationExists; + return out; + } + + package::PackageManifest manifest = survey.plan.manifest; + manifest.exportTimestamp = req.exportTimestamp; + const std::vector sources = + absoluteSources(req.projectDir, survey.plan.sourceRelativePaths); + + if (!digestSources(manifest, sources, out.offendingName)) { + out.status = ExportStatus::SourceReadFailed; + return out; + } + const std::optional encoded = package::encodePackage(manifest); + if (!encoded) { + out.status = ExportStatus::EncodeFailed; + return out; + } + + ExportOutcome written = writePackageFile(*encoded, manifest, sources, req.destAbsPath); + written.bankDisplayName = out.bankDisplayName; + written.excluded = std::move(out.excluded); + return written; +} + +} // namespace reasampler diff --git a/src/shell/package/export_bank.h b/src/shell/package/export_bank.h new file mode 100644 index 0000000..a6deae8 --- /dev/null +++ b/src/shell/package/export_bank.h @@ -0,0 +1,91 @@ +// shell/package/export_bank — the promptless bank-export verb: survey, digest, +// stream, commit. No prompts and no message boxes (shell/actions/ +// package_export_action is the skin). The session arrives CONST, which is how "an +// export writes no ext state, opens no undo point and never bumps the bank +// generation" is enforced rather than remembered — every mutator on the session is +// non-const. Blocking I/O: UI-thread actions only. + +#pragma once + +#include +#include +#include +#include + +#include "core/package/bank_package.h" +#include "core/package/export_plan.h" + +namespace reasampler { + +class ReaSamplerSession; + +struct ExportRequest { + std::string projectDir; // absolute; the root the index's relative paths hang off + std::string bankId; + std::string destAbsPath; // the .rsbank to write + std::int64_t exportTimestamp = 0; // manifest envelope; the caller's clock read + // Both default false and are set ONLY after the skin's explicit confirm: one + // lists what is absent, the other names the destination being replaced. + bool allowIncomplete = false; + bool allowOverwrite = false; +}; + +enum class ExportStatus { + Written, + NoSuchBank, + NoProjectDir, + RefusedIncomplete, + RefusedUnrepresentable, + RefusedDestinationExists, + SourceReadFailed, // a file the plan classified Present would not read, or is empty + SourceChanged, // a payload's bytes moved between the digest pass and the stream pass + EncodeFailed, + WriteFailed, +}; + +struct ExportOutcome { + ExportStatus status = ExportStatus::WriteFailed; + std::size_t entriesWritten = 0; + std::uint64_t bytesWritten = 0; + std::string bankDisplayName; + std::vector excluded; + std::string offendingName; // the entry a SourceReadFailed / SourceChanged names +}; + +struct ExportSurvey { + bool bankFound = false; + package::ExportPlan plan; +}; + +// Report-before-acting: the same plan exportBank recomputes, with nothing written. +// Read-only against both the project and the filesystem. +ExportSurvey surveyBankExport(const ReaSamplerSession& session, + const std::string& projectDir, + const std::string& bankId); + +// Fills each manifest entry's byteLength and byteHash from its source file — the +// digest pass, one payload in memory at a time. False with `outFailedName` set when a +// source will not read or is empty; a zero-length entry cannot round-trip the +// format's own seam, so it is a failure here rather than an entry. +bool digestSources(package::PackageManifest& manifest, + const std::vector& sourceAbsPaths, + std::string& outFailedName); + +// Streams one package to `destAbsPath`: the encoded prefix, then each payload re-read +// from `sourceAbsPaths` (parallel to `manifest.entries`) and re-checked against the +// length and digest recorded for it before it is appended — so the digest the +// manifest claims describes the bytes actually written, not the bytes a concurrent +// edit replaced. Any failure abandons the writer, leaving the destination absent or +// holding its prior contents. +// +// Public because that atomicity is this function's property: proving it needs a +// failure injected mid-stream, which is a call to this seam, not to exportBank. +ExportOutcome writePackageFile(const package::EncodedPackage& encoded, + const package::PackageManifest& manifest, + const std::vector& sourceAbsPaths, + const std::string& destAbsPath); + +// The verb: plan, gate on the verdict and the destination, digest, encode, stream. +ExportOutcome exportBank(const ReaSamplerSession& session, const ExportRequest& req); + +} // namespace reasampler diff --git a/src/shell/panel/panel_bank_ops.cpp b/src/shell/panel/panel_bank_ops.cpp index 0ce2fbb..f946c4f 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_export_action.h" // doBankPackageExport — the export skin #include "shell/bank_ops/bank_ops.h" // bankOp* promptless verbs #include "shell/persist/session.h" // ReaSamplerSession — the live session the ops mutate @@ -241,6 +242,7 @@ enum : unsigned int { kMenuDelete, kMenuEvacuate, kMenuCreate, + kMenuExport, // export this bank as a .rsbank package kMenuRemove, // remove selected sample(s) from the source bank kMenuMoveBase = 1000, // move-to-bank: kMenuMoveBase + destination index kMenuCopyBase = 2000, // copy-to-bank: kMenuCopyBase + destination index @@ -265,6 +267,7 @@ void showTabMenu(int screenX, int screenY, const std::string& bankId) { menuAppend(menu, kMenuRename, "Rename..."); menuAppend(menu, kMenuEvacuate, "Evacuate to pool", /*grayed=*/!nonEmpty); menuAppend(menu, kMenuDelete, "Delete..."); + menuAppend(menu, kMenuExport, "Export as package..."); menuSeparator(menu); menuAppend(menu, kMenuCreate, "New bank..."); @@ -277,6 +280,7 @@ void showTabMenu(int screenX, int screenY, const std::string& bankId) { case kMenuRename: doRenameBank(bankId); break; case kMenuEvacuate: doEvacuateBank(bankId); break; case kMenuDelete: doDeleteBank(bankId); break; + case kMenuExport: doBankPackageExport(*g_panel.session, bankId); break; case kMenuCreate: doCreateBank(); break; default: break; } diff --git a/tests/test_export_bank.cpp b/tests/test_export_bank.cpp new file mode 100644 index 0000000..25e1b23 --- /dev/null +++ b/tests/test_export_bank.cpp @@ -0,0 +1,375 @@ +// Standalone tests for shell/package/export_bank — no REAPER, no framework. The +// export reads the session through inline accessors only, so a real ReaSamplerSession +// and a real bank folder on disk are both constructible here. +// +// Mid-stream failure is INJECTED rather than simulated: the digest pass and the +// stream pass are separate public calls, so a source file removed or rewritten +// between them is exactly the concurrent-edit case the stream pass re-checks for. + +#include "../src/shell/package/export_bank.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../src/core/capture/wav_codec.h" +#include "../src/core/model/bank_book.h" +#include "../src/core/package/bank_package.h" +#include "../src/shell/package/package_io.h" +#include "../src/shell/package/package_path.h" +#include "../src/shell/persist/session.h" + +using namespace reasampler; +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) + +// --- scratch filesystem ------------------------------------------------------- + +static std::string g_root; + +static std::string scratchRoot() { + if (g_root.empty()) { + std::error_code ec; + const fs::path p = fs::temp_directory_path(ec) / "reasampler_export_tests"; + fs::remove_all(p, ec); + fs::create_directories(p, ec); + g_root = p.generic_string(); + } + return g_root; +} + +static std::vector patternBytes(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 * 7u); + return v; +} + +static void writeFile(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 readFile(const std::string& path) { + std::ifstream f(utf8Path(path), std::ios::binary); + return std::vector(std::istreambuf_iterator(f), + std::istreambuf_iterator()); +} + +static bool exists(const std::string& path) { return fs::exists(utf8Path(path)); } + +// --- fixture project ---------------------------------------------------------- + +// One scratch project directory with a bank folder, plus the session whose book +// names its contents. `fileNames` are written into the bank folder with distinct +// byte patterns; a name in `omit` gets an index entry but NO file on disk. +struct Fixture { + std::string projectDir; + ReaSamplerSession session; + std::string bankId = "bank-1"; + + explicit Fixture(const std::string& tag) { + projectDir = scratchRoot() + "/" + tag; + std::error_code ec; + fs::create_directories(utf8Path(projectDir + "/reasampler_bank"), ec); + session.book().createBank(bankId, "Drums " + tag); + } + + void addSample(const std::string& id, const std::string& fileName, + std::size_t bytes, std::uint8_t seed, bool writeToDisk = true) { + model::Sample s; + s.id = id; + s.displayName = id; + s.relativePath = std::string("reasampler_bank/") + fileName; + s.sampleRate = 48000; + s.channelCount = 2; + s.contentHash = "hash-" + id; + CHECK(session.book().index(bankId)->add(s) == model::AddResult::Added); + session.book().reconcileSlots(); + if (writeToDisk) writeFile(absPathOf(fileName), patternBytes(bytes, seed)); + } + + std::string absPathOf(const std::string& fileName) const { + return projectDir + "/reasampler_bank/" + fileName; + } + std::string destPath() const { return projectDir + "/out.rsbank"; } + + ExportRequest request() const { + ExportRequest req; + req.projectDir = projectDir; + req.bankId = bankId; + req.destAbsPath = destPath(); + req.exportTimestamp = 1234567890; + return req; + } +}; + +// --- package readback --------------------------------------------------------- + +// Decodes an emitted package straight off disk, growing the prefix read the way the +// format's own requiredPrefixSize seam asks callers to. +static package::DecodedPackage decodeFromDisk(const std::string& path) { + PackageFileReader reader(path); + const std::uint64_t size = reader.fileSize(); + std::vector prefix; + for (int guard = 0; guard < 8; ++guard) { + const std::optional need = package::requiredPrefixSize(prefix); + if (!need) break; + if (*need <= prefix.size()) break; + PayloadBuffer buf = reader.readRange(0, *need); + if (buf.empty()) break; + prefix.assign(buf.data(), buf.data() + buf.size()); + } + return package::decodePackage(prefix, size); +} + +// --- tests -------------------------------------------------------------------- + +static void testHealthyExportCarriesEveryPayloadByteExact() { + Fixture fx("healthy"); + fx.addSample("s1", "kick.wav", 800, 1); + fx.addSample("s2", "snare.wav", 1300, 60); + fx.addSample("s3", "hat.wav", 97, 200); + + const ExportOutcome out = exportBank(fx.session, fx.request()); + CHECK(out.status == ExportStatus::Written); + CHECK(out.entriesWritten == 3); + CHECK(out.excluded.empty()); + CHECK(PayloadBuffer::alive() == 0); + + const package::DecodedPackage decoded = decodeFromDisk(fx.destPath()); + CHECK(decoded.status == package::PackageReadability::Readable); + CHECK(decoded.manifest.entries.size() == 3); + CHECK(decoded.layout.size() == 3); + CHECK(decoded.manifest.bankDisplayName == "Drums healthy"); + CHECK(decoded.manifest.exportTimestamp == 1234567890); + + // PER ENTRY, not in aggregate: the digest the package records, the digest of the + // payload actually stored at that entry's span, and the digest of the source file + // on disk must all be the same string. + PackageFileReader reader(fx.destPath()); + const std::vector sourceNames = {"kick.wav", "snare.wav", "hat.wav"}; + CHECK(decoded.manifest.entries.size() == sourceNames.size()); + for (std::size_t i = 0; i < decoded.manifest.entries.size(); ++i) { + const package::PackageEntry& entry = decoded.manifest.entries[i]; + const PayloadBuffer stored = reader.readRange(decoded.layout[i].offset, + decoded.layout[i].length); + CHECK(!stored.empty()); + const std::vector source = readFile(fx.absPathOf(sourceNames[i])); + const std::string sourceDigest = capture::hashBytes(source.data(), source.size()); + CHECK(entry.byteLength == source.size()); + CHECK(entry.byteHash == sourceDigest); + CHECK(capture::hashBytes(stored.data(), stored.size()) == sourceDigest); + CHECK(stored.size() == source.size()); + CHECK(std::equal(source.begin(), source.end(), stored.data())); + } + CHECK(PayloadBuffer::alive() == 0); +} + +static void testEmittedManifestBytesCarryNoPath() { + Fixture fx("nopath"); + // Both source records carry a directory component; neither may reach the file. + fx.addSample("s1", "kick.wav", 200, 3); + fx.addSample("s2", "snare take 2.wav", 200, 9); + CHECK(exportBank(fx.session, fx.request()).status == ExportStatus::Written); + + // Scan the MANIFEST REGION of the emitted file, located by walking the frozen + // header the way a reader does: magic | fv | minReader | len+semver | len+JSON. + // The binary length fields are deliberately excluded — a length whose byte + // happens to be 0x2F is not a separator. + const std::vector file = readFile(fx.destPath()); + CHECK(file.size() > 20); + auto le32 = [&](std::size_t at) { + return static_cast(file[at]) | + (static_cast(file[at + 1]) << 8) | + (static_cast(file[at + 2]) << 16) | + (static_cast(file[at + 3]) << 24); + }; + const std::uint32_t semverLen = le32(12); + const std::size_t manifestLenAt = 16 + semverLen; + const std::uint32_t manifestLen = le32(manifestLenAt); + const std::size_t manifestAt = manifestLenAt + 4; + CHECK(manifestAt + manifestLen <= file.size()); + const std::string manifest(reinterpret_cast(file.data() + manifestAt), + manifestLen); + + CHECK(manifest.find("kick.wav") != std::string::npos); // the scan is looking at the manifest + CHECK(manifest.find('/') == std::string::npos); + CHECK(manifest.find('\\') == std::string::npos); + CHECK(manifest.find("..") == std::string::npos); + CHECK(manifest.find("reasampler_bank") == std::string::npos); + // ':' cannot be banned outright — it is JSON's own key separator — so the check + // is for the drive form specifically: a string value opening with ':'. + // With '/' and '\\' already absent, that covers the drive-relative spelling too. + bool driveForm = false; + for (std::size_t i = 0; i + 2 < manifest.size(); ++i) + if (manifest[i] == '"' && + std::isalpha(static_cast(manifest[i + 1])) && + manifest[i + 2] == ':') + driveForm = true; + CHECK(!driveForm); +} + +static void testFailureInjectedMidWriteLeavesNoPackageAndSparesThePriorFile() { + Fixture fx("midwrite"); + fx.addSample("s1", "kick.wav", 500, 1); + fx.addSample("s2", "snare.wav", 500, 2); + + const std::vector prior = patternBytes(64, 99); + writeFile(fx.destPath(), prior); + + ExportSurvey survey = surveyBankExport(fx.session, fx.projectDir, fx.bankId); + CHECK(survey.plan.verdict == package::ExportVerdict::Ready); + package::PackageManifest manifest = survey.plan.manifest; + const std::vector sources = {fx.absPathOf("kick.wav"), + fx.absPathOf("snare.wav")}; + std::string failed; + CHECK(digestSources(manifest, sources, failed)); + const std::optional encoded = package::encodePackage(manifest); + CHECK(encoded.has_value()); + + // Injection: the second payload vanishes after the framing that claims it was + // already encoded, so the failure lands with the prefix and one payload written. + std::error_code ec; + fs::remove(utf8Path(fx.absPathOf("snare.wav")), ec); + + const ExportOutcome out = + writePackageFile(*encoded, manifest, sources, fx.destPath()); + CHECK(out.status == ExportStatus::SourceReadFailed); + CHECK(out.offendingName == "snare.wav"); + CHECK(readFile(fx.destPath()) == prior); // the prior file is untouched + CHECK(!exists(fx.destPath() + ".rsbanktmp")); // and no debris is left behind + CHECK(PayloadBuffer::alive() == 0); +} + +static void testPayloadChangedBetweenDigestAndStreamAborts() { + Fixture fx("changed"); + fx.addSample("s1", "kick.wav", 500, 1); + CHECK(!exists(fx.destPath())); + + ExportSurvey survey = surveyBankExport(fx.session, fx.projectDir, fx.bankId); + package::PackageManifest manifest = survey.plan.manifest; + const std::vector sources = {fx.absPathOf("kick.wav")}; + std::string failed; + CHECK(digestSources(manifest, sources, failed)); + const std::optional encoded = package::encodePackage(manifest); + CHECK(encoded.has_value()); + + // Same length, different bytes — only the digest re-check can catch this. + writeFile(fx.absPathOf("kick.wav"), patternBytes(500, 77)); + + const ExportOutcome out = + writePackageFile(*encoded, manifest, sources, fx.destPath()); + CHECK(out.status == ExportStatus::SourceChanged); + CHECK(out.offendingName == "kick.wav"); + CHECK(!exists(fx.destPath())); +} + +static void testExportTouchesNoProjectState() { + Fixture fx("readonly"); + fx.addSample("s1", "kick.wav", 400, 5); + fx.addSample("s2", "snare.wav", 400, 6); + + // The ext-state blob IS the serialized book (shell/persist/ext_state_io), so + // byte-identity of that string is byte-identity of what a persist would write. + const std::string extStateBefore = fx.session.book().serialize(); + const std::int64_t generationBefore = fx.session.bankGeneration(); + + CHECK(exportBank(fx.session, fx.request()).status == ExportStatus::Written); + + CHECK(fx.session.book().serialize() == extStateBefore); + CHECK(fx.session.bankGeneration() == generationBefore); +} + +static void testEmptyBankExportsAsAValidZeroEntryPackage() { + Fixture fx("empty"); + const ExportOutcome out = exportBank(fx.session, fx.request()); + CHECK(out.status == ExportStatus::Written); + CHECK(out.entriesWritten == 0); + + const package::DecodedPackage decoded = decodeFromDisk(fx.destPath()); + CHECK(decoded.status == package::PackageReadability::Readable); + CHECK(decoded.manifest.entries.empty()); + CHECK(decoded.layout.empty()); + CHECK(decoded.manifest.bankDisplayName == "Drums empty"); + // The size proof is decodePackage's, and it ran against the real on-disk size. + CHECK(decoded.prefixSize == readFile(fx.destPath()).size()); +} + +static void testIncompleteBankRefusesUntilConfirmed() { + Fixture fx("incomplete"); + fx.addSample("s1", "kick.wav", 300, 1); + fx.addSample("s2", "gone.wav", 300, 2, /*writeToDisk=*/false); + + ExportRequest req = fx.request(); + const ExportOutcome refused = exportBank(fx.session, req); + CHECK(refused.status == ExportStatus::RefusedIncomplete); + CHECK(refused.excluded.size() == 1); + CHECK(refused.excluded[0].sampleId == "s2"); + CHECK(refused.excluded[0].reason == package::ExclusionReason::FileMissing); + CHECK(!exists(fx.destPath())); + + req.allowIncomplete = true; + const ExportOutcome allowed = exportBank(fx.session, req); + CHECK(allowed.status == ExportStatus::Written); + CHECK(allowed.entriesWritten == 1); + CHECK(allowed.excluded.size() == 1); // the report survives into the summary + CHECK(decodeFromDisk(fx.destPath()).manifest.entries.size() == 1); +} + +static void testExistingDestinationRefusesUntilConfirmed() { + Fixture fx("overwrite"); + fx.addSample("s1", "kick.wav", 300, 1); + const std::vector prior = patternBytes(32, 11); + writeFile(fx.destPath(), prior); + + ExportRequest req = fx.request(); + const ExportOutcome refused = exportBank(fx.session, req); + CHECK(refused.status == ExportStatus::RefusedDestinationExists); + CHECK(readFile(fx.destPath()) == prior); + + req.allowOverwrite = true; + CHECK(exportBank(fx.session, req).status == ExportStatus::Written); + CHECK(readFile(fx.destPath()) != prior); +} + +static void testUnknownBankAndUnsavedProjectAreNamedSeparately() { + Fixture fx("guards"); + ExportRequest req = fx.request(); + req.bankId = "no-such-bank"; + CHECK(exportBank(fx.session, req).status == ExportStatus::NoSuchBank); + + ExportRequest unsaved = fx.request(); + unsaved.projectDir.clear(); + CHECK(exportBank(fx.session, unsaved).status == ExportStatus::NoProjectDir); + CHECK(!exists(fx.destPath())); +} + +int main() { + testHealthyExportCarriesEveryPayloadByteExact(); + testEmittedManifestBytesCarryNoPath(); + testFailureInjectedMidWriteLeavesNoPackageAndSparesThePriorFile(); + testPayloadChangedBetweenDigestAndStreamAborts(); + testExportTouchesNoProjectState(); + testEmptyBankExportsAsAValidZeroEntryPackage(); + testIncompleteBankRefusesUntilConfirmed(); + testExistingDestinationRefusesUntilConfirmed(); + testUnknownBankAndUnsavedProjectAreNamedSeparately(); + + if (g_fail == 0) { + std::printf("export_bank_tests: all passed\n"); + return 0; + } + std::printf("export_bank_tests: %d failure(s)\n", g_fail); + return 1; +} diff --git a/tests/test_export_plan.cpp b/tests/test_export_plan.cpp new file mode 100644 index 0000000..f29c356 --- /dev/null +++ b/tests/test_export_plan.cpp @@ -0,0 +1,268 @@ +// Standalone tests for reasampler::package::export_plan — no REAPER, no filesystem, +// no test framework. The planner's totality claim is the point: every input class +// (missing / unreadable / unrepresentable / zero / one) classifies here, and the +// names it produces are asserted against the codec's OWN predicates rather than +// against a hand-copied rule. + +#include "../src/core/package/export_plan.h" + +#include +#include +#include +#include +#include + +#include "../src/core/package/package_format.h" +#include "../src/core/package/package_manifest.h" + +using namespace reasampler::package; +using namespace reasampler::model; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// --- fixtures ---------------------------------------------------------------- + +static Sample sampleAt(const std::string& id, const std::string& relativePath) { + Sample s; + s.id = id; + s.displayName = id + " display"; + s.relativePath = relativePath; + s.sampleRate = 48000; + s.channelCount = 2; + s.contentHash = "0123456789abcdef"; + return s; +} + +static ExportCandidate present(const std::string& id, const std::string& rel) { + return ExportCandidate{sampleAt(id, rel), SourceFileState::Present}; +} + +static ExportCandidate withState(const std::string& id, const std::string& rel, + SourceFileState state) { + return ExportCandidate{sampleAt(id, rel), state}; +} + +static ExportInputs bankOf(std::vector candidates) { + ExportInputs in; + in.bankDisplayName = "Drums"; + in.candidates = std::move(candidates); + std::vector ids; + for (const ExportCandidate& c : in.candidates) ids.push_back(c.sample.id); + in.slots.resetDense(ids); + return in; +} + +static bool hasExclusion(const ExportPlan& p, const std::string& id, ExclusionReason why) { + for (const ExcludedEntry& e : p.excluded) + if (e.sampleId == id && e.reason == why) return true; + return false; +} + +// --- the four classification inputs the plan names --------------------------- + +static void testZeroSamplesIsAReadyEmptyPlan() { + const ExportPlan p = planExport(bankOf({})); + CHECK(p.verdict == ExportVerdict::Ready); + CHECK(p.manifest.entries.empty()); + CHECK(p.sourceRelativePaths.empty()); + CHECK(p.excluded.empty()); + CHECK(p.manifest.bankDisplayName == "Drums"); + CHECK(p.manifest.slots.empty()); +} + +static void testOneSamplePresentShips() { + const ExportPlan p = planExport(bankOf({present("s1", "reasampler_bank/kick.wav")})); + CHECK(p.verdict == ExportVerdict::Ready); + CHECK(p.manifest.entries.size() == 1); + CHECK(p.excluded.empty()); + CHECK(p.manifest.entries[0].fileName == "kick.wav"); + CHECK(p.manifest.entries[0].sample.id == "s1"); + // The source spelling survives only on the side channel; the transport record + // names the payload by its bare package name. + CHECK(p.sourceRelativePaths.size() == 1); + CHECK(p.sourceRelativePaths[0] == "reasampler_bank/kick.wav"); + CHECK(p.manifest.entries[0].sample.relativePath == "kick.wav"); +} + +static void testMissingFileIsIncompleteNotRefused() { + const ExportPlan p = planExport(bankOf({ + present("s1", "reasampler_bank/kick.wav"), + withState("s2", "reasampler_bank/gone.wav", SourceFileState::Missing), + })); + CHECK(p.verdict == ExportVerdict::Incomplete); + CHECK(p.manifest.entries.size() == 1); + CHECK(p.manifest.entries[0].sample.id == "s1"); + CHECK(p.excluded.size() == 1); + CHECK(hasExclusion(p, "s2", ExclusionReason::FileMissing)); + CHECK(p.excluded[0].relativePath == "reasampler_bank/gone.wav"); +} + +static void testUnreadableFileStaysDistinctFromMissing() { + const ExportPlan p = planExport(bankOf({ + withState("s1", "reasampler_bank/locked.wav", SourceFileState::Unreadable), + withState("s2", "reasampler_bank/gone.wav", SourceFileState::Missing), + })); + CHECK(p.verdict == ExportVerdict::Incomplete); + CHECK(p.manifest.entries.empty()); + CHECK(p.excluded.size() == 2); + CHECK(hasExclusion(p, "s1", ExclusionReason::FileUnreadable)); + CHECK(hasExclusion(p, "s2", ExclusionReason::FileMissing)); + CHECK(!hasExclusion(p, "s1", ExclusionReason::FileMissing)); +} + +static void testUnrepresentableRecordRefusesWholeExport() { + // A traversing nested path — the one thing an index record can carry that + // BankModel::add does not itself refuse. + const ExportPlan traversal = + planExport(bankOf({present("s1", "reasampler_bank/kick.wav"), + present("s2", "reasampler_bank/../evil.wav")})); + CHECK(traversal.verdict == ExportVerdict::Refused); + CHECK(hasExclusion(traversal, "s2", ExclusionReason::RecordUnrepresentable)); + CHECK(traversal.manifest.entries.size() == 1); // still reports what WOULD ship + + const ExportPlan emptyId = planExport(bankOf({present("", "reasampler_bank/kick.wav")})); + CHECK(emptyId.verdict == ExportVerdict::Refused); + + const ExportPlan absolute = + planExport(bankOf({present("s1", "C:/elsewhere/kick.wav")})); + CHECK(absolute.verdict == ExportVerdict::Refused); + CHECK(hasExclusion(absolute, "s1", ExclusionReason::RecordUnrepresentable)); + + // Refused outranks Incomplete: a corrupt record is not something the + // "export the present N" confirm can proceed past. + const ExportPlan both = planExport(bankOf({ + withState("s1", "reasampler_bank/gone.wav", SourceFileState::Missing), + present("s2", "reasampler_bank/../evil.wav"), + })); + CHECK(both.verdict == ExportVerdict::Refused); +} + +// --- transport names ---------------------------------------------------------- + +static void testHostileNamesAreRepairedNotRelayed() { + // Every one of these is a name the codec refuses and a filesystem somewhere + // produces honestly. + const std::vector hostile = { + "reasampler_bank/ki:ck?.wav", "reasampler_bank/a|bd\"e*f.wav", + "reasampler_bank/CON.wav", "reasampler_bank/nul", + "reasampler_bank/trailing .wav ", "reasampler_bank/dots...", + // A literal ".." COMPONENT is not a name to repair — it is a traversing + // record, and the Refused test above owns it. + "reasampler_bank/.....", "reasampler_bank/.", + std::string("reasampler_bank/bad\xC3.wav"), // truncated UTF-8 sequence + std::string("reasampler_bank/") + std::string(400, 'x') + ".wav", + }; + std::vector candidates; + for (std::size_t i = 0; i < hostile.size(); ++i) + candidates.push_back(present("s" + std::to_string(i), hostile[i])); + + const ExportPlan p = planExport(bankOf(candidates)); + CHECK(p.verdict == ExportVerdict::Ready); + CHECK(p.manifest.entries.size() == hostile.size()); + for (const PackageEntry& e : p.manifest.entries) { + CHECK(isValidEntryName(e.fileName)); + CHECK(isValidNestedSamplePath(e.sample.relativePath)); + } +} + +static void testCaseFoldedCollisionsAreDisambiguated() { + const ExportPlan p = planExport(bankOf({ + present("s1", "reasampler_bank/Kick.wav"), + present("s2", "reasampler_bank/kick.wav"), + present("s3", "reasampler_bank/KICK.wav"), + })); + CHECK(p.verdict == ExportVerdict::Ready); + CHECK(p.manifest.entries.size() == 3); + for (std::size_t i = 0; i < p.manifest.entries.size(); ++i) + for (std::size_t j = i + 1; j < p.manifest.entries.size(); ++j) + CHECK(!sameEntryName(p.manifest.entries[i].fileName, + p.manifest.entries[j].fileName)); + CHECK(p.manifest.entries[0].fileName == "Kick.wav"); + // The suffix goes before the extension, so the payload keeps its type. + CHECK(p.manifest.entries[1].fileName == "kick_2.wav"); +} + +static void testSanitizeNeverReturnsANameTheCodecRefuses() { + const std::vector raws = { + "", ".", "..", "...", " ", "com1", "LPT9.WAV", "a/b", "a\\b", "C:evil", + std::string("\x01\x02\x03"), std::string(300, 'y'), + std::string("caf\xC3\xA9.wav"), // well-formed UTF-8 must survive intact + }; + for (const std::string& raw : raws) CHECK(isValidEntryName(sanitizeEntryName(raw))); + CHECK(sanitizeEntryName("caf\xC3\xA9.wav") == "caf\xC3\xA9.wav"); + CHECK(sanitizeEntryName("kick.wav") == "kick.wav"); +} + +// --- what the plan hands the codec ------------------------------------------- + +static void testPlannedManifestSatisfiesTheCodec() { + ExportPlan p = planExport(bankOf({ + present("s1", "reasampler_bank/Kick.wav"), + present("s2", "reasampler_bank/kick.wav"), + present("s3", "reasampler_bank/CON.wav"), + present("s4", std::string("reasampler_bank/caf\xC3\xA9 mix.wav")), + })); + // byteLength/byteHash are the shell's to measure; stand them in so the encode + // path under test is the naming, not the digest. + for (PackageEntry& e : p.manifest.entries) { + e.byteLength = 44; + e.byteHash = "aaaaaaaabbbbbbbb"; + } + const std::optional json = serializeManifest(p.manifest); + CHECK(json.has_value()); + if (json) { + const std::optional back = deserializeManifest(*json); + CHECK(back.has_value()); + if (back) CHECK(*back == p.manifest); + } +} + +static void testSlotsFollowMembership() { + ExportInputs in = bankOf({ + present("s1", "reasampler_bank/a.wav"), + withState("s2", "reasampler_bank/gone.wav", SourceFileState::Missing), + present("s3", "reasampler_bank/c.wav"), + }); + const ExportPlan p = planExport(in); + CHECK(p.manifest.slots.slotOf("s2") == -1); // an excluded id keeps no display position + CHECK(p.manifest.slots.slotOf("s1") >= 0); + CHECK(p.manifest.slots.slotOf("s3") >= 0); + CHECK(p.manifest.slots.size() == 2); +} + +static void testPlanIsDeterministic() { + const ExportInputs in = bankOf({ + present("s1", "reasampler_bank/Kick.wav"), + present("s2", "reasampler_bank/kick.wav"), + withState("s3", "reasampler_bank/gone.wav", SourceFileState::Missing), + }); + const ExportPlan a = planExport(in); + const ExportPlan b = planExport(in); + CHECK(a.verdict == b.verdict); + CHECK(a.manifest == b.manifest); + CHECK(a.sourceRelativePaths == b.sourceRelativePaths); + CHECK(a.excluded.size() == b.excluded.size()); +} + +int main() { + testZeroSamplesIsAReadyEmptyPlan(); + testOneSamplePresentShips(); + testMissingFileIsIncompleteNotRefused(); + testUnreadableFileStaysDistinctFromMissing(); + testUnrepresentableRecordRefusesWholeExport(); + testHostileNamesAreRepairedNotRelayed(); + testCaseFoldedCollisionsAreDisambiguated(); + testSanitizeNeverReturnsANameTheCodecRefuses(); + testPlannedManifestSatisfiesTheCodec(); + testSlotsFollowMembership(); + testPlanIsDeterministic(); + + if (g_fail == 0) { + std::printf("export_plan_tests: all passed\n"); + return 0; + } + std::printf("export_plan_tests: %d failure(s)\n", g_fail); + return 1; +} From 454f67b3bc6e3b6421ea15d9234bfe8414e5bed3 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 14:02:01 -0400 Subject: [PATCH 2/2] Close bank-export review findings: name-cap underflow, double overwrite prompt, test scope Clamps insertSuffix's underflow, floors uniqueEntryName's validity guard, suppresses the redundant overwrite confirm via a picker out-param, adds a PayloadBuffer high-water mark, and corrects stale CLAUDE.md/CMake claims. --- docs/PLAN.md | 10 +- docs/product/bank-package.md | 24 ++++ src/app/main.cpp | 4 +- src/core/package/CLAUDE.md | 24 ++-- src/core/package/export_plan.cpp | 40 ++++-- src/core/package/export_plan.h | 6 +- src/shell/actions/package_export_action.cpp | 8 +- src/shell/package/CLAUDE.md | 38 ++--- src/shell/package/CMakeLists.txt | 13 +- src/shell/package/export_bank.h | 8 +- src/shell/package/package_io.cpp | 10 +- src/shell/package/package_io.h | 5 + src/shell/package/package_pickers.cpp | 7 +- src/shell/package/package_pickers.h | 9 +- src/shell/panel/panel_bank_ops.cpp | 2 +- tests/test_export_bank.cpp | 148 ++++++++++++++++---- tests/test_export_plan.cpp | 22 +++ tests/test_package_io.cpp | 4 +- 18 files changed, 291 insertions(+), 91 deletions(-) diff --git a/docs/PLAN.md b/docs/PLAN.md index 0833a9a..b2ccbbe 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2704,10 +2704,12 @@ on the import side, `bank_ops`, or `persist`. - Exporting an empty bank produces a valid, importable package with zero entries rather than refusing. An empty bank is a legitimate thing to carry. -**Open questions.** **[propose at review]** whether the export affordance is action-only, -panel-only, or both at ship. **[propose at review]** whether the default file name is derived -from the bank's display name (recommended, sanitized through -`capture_paths::sanitizeStem`) or from the project name. +**Open questions — both answered at implementation review, recorded at +`docs/product/bank-package.md` §"Implementation decisions — Ε-W2-T1".** Affordance: +**both** the action and the panel row (the action is the only spelling that can reach +the pool; the panel row is the direct gesture on a named bank). Default file name: +the bank's **display name**, sanitized through `capture_paths::sanitizeStem`, as +recommended. #### Ε-W2-T2 — `bank-import` diff --git a/docs/product/bank-package.md b/docs/product/bank-package.md index 92c9ee4..dd51dea 100644 --- a/docs/product/bank-package.md +++ b/docs/product/bank-package.md @@ -725,6 +725,30 @@ contemplates one untracked capture, an import strands hundreds). --- +## Implementation decisions — Ε-W2-T1 + +Not [Daniel]-class forks — both were `[propose at review]` calls in `docs/PLAN.md`'s +Ε-W2-T1 track, answered at implementation review rather than by Daniel, and recorded +here per this phase's own convention for keeping such answers where the design lives +rather than only in the track's own now-stale open-questions line. + +- **Affordance: both the bindable action and the panel row.** The action targets the + **active** bank and is the only spelling that can reach the **pool** (the panel's + `showTabMenu` returns early on `isPool()` — a named-bank-tab context menu has no tab + to right-click for the pool), while the exported unit's own definition above includes + the pool. The panel row is the direct gesture on a specific named bank. Neither + subsumes the other. +- **Default file name: the bank's display name**, sanitized through + `capture_paths::sanitizeStem`, seeded into `/.rsbank`. A + project-derived name was the rejected alternative: three banks exported from one + project must produce three distinguishable files, and a project-derived name + collides on the second export. Known wart, worth recording rather than hiding: + `sanitizeStem` collapses an all-non-ASCII display name to the literal `capture`, so + two such banks still collide — the existing rename verb is the recovery, same as the + import-side auto-suffix collisions above. + +--- + ## Non-goals and guardrails - **No auto-insertion of imported audio into the arrange.** Same rule as capture. diff --git a/src/app/main.cpp b/src/app/main.cpp index 599003c..b898cd6 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -92,9 +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 RunExportBankPackage(int) { - reasampler::doBankPackageExport(g_session, g_session.book().activeBankId()); -} +static void RunExportBankPackage(int) { reasampler::doBankPackageExport(g_session, g_session.book().activeBankId()); } static void RunShowVersion(int) { // On-demand only — no unconditional startup print (routine console chatter pops // the console window). diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 80f475f..fb9391a 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -6,8 +6,8 @@ The hand-rolled `RSBK` bank-package container, entirely pure (REAPER-free, unit-tested outside the DAW): the format contract and version ladder, the JSON manifest, and the framing/layout codec. No filesystem — the shell (`src/shell/package`) streams bytes against the layouts produced here. The -export/import *decisions* (`export_plan` / `import_plan`) are separate modules -landing after the format. +export/import *decisions* (`export_plan` / `import_plan`) are separate modules; +`export_plan` has landed, `import_plan` has not. ## Invariants @@ -123,12 +123,20 @@ landing after the format. corruption. - **A written package carries no path in ANY field.** `isValidNestedSamplePath` permits a relative `relativePath` because a *record* may hold one, but - `export_plan` writes each shipping entry's `relativePath` as its bare package - name, so an emitted manifest has no separator anywhere and the entry name is the - single naming authority on both sides. The directory component it drops carries - no information — the bank subfolder is a fixed `capture_paths` constant the - importer re-spells. The nested-path rule stays as the decode-side backstop for a - package this build did not write. + `export_plan` writes each shipping entry's `relativePath` as its bare, sanitized + and disambiguated transport name (`export_plan.cpp`'s `e.fileName`), so an + emitted manifest has no separator anywhere and the entry name is the single + naming authority on both sides. The directory component it drops carries no + information — the bank subfolder is a fixed `capture_paths` constant + (`capture_paths.cpp`'s `deriveBankPaths`) the importer re-spells. **The + basename spelling is dropped too, not just the directory**: `e.fileName` is + `uniqueEntryName(sanitizeEntryName(...))`, not the source basename, so a + macOS-authored `Hit?.wav` survives only in `displayName` — the transport name + itself may differ. Accepted for the same reason the directory drop is: the + transport name exists to be a valid, collision-free package entry, not a + faithful copy of the source spelling, and `displayName` is the field that + carries the original for display. The nested-path rule stays as the decode-side + backstop for a package this build did not write. - **Obligation on the export track: sanitize, don't relay the refusal.** `serializeManifest` returns one indistinguishable `nullopt` for every rejection — an unrepresentable name, a case-folded collision, a traversing nested path, a diff --git a/src/core/package/export_plan.cpp b/src/core/package/export_plan.cpp index aa4e6a0..4803cf0 100644 --- a/src/core/package/export_plan.cpp +++ b/src/core/package/export_plan.cpp @@ -17,16 +17,18 @@ std::string baseNameOf(const std::string& path) { return sep == std::string::npos ? path : path.substr(sep + 1); } -// Trailing dots and spaces are stripped at file creation on Windows, so a name -// carrying them would collide with its stripped twin (isValidEntryName refuses them -// for that reason). +// Mirrors package_format.cpp's trailing-dot/space rule so a truncated stem never +// reintroduces the collision isValidEntryName exists to prevent. std::string stripTrailingDotsAndSpaces(std::string s) { while (!s.empty() && (s.back() == '.' || s.back() == ' ')) s.pop_back(); return s; } -// Truncates to at most `max` bytes without splitting a UTF-8 sequence — a split one -// would leave the name ill-formed, which isValidEntryName refuses outright. +// Truncates to at most `max` bytes without splitting a UTF-8 sequence (the rule +// itself is package_format.cpp's isWellFormedUtf8). A sequence landing exactly on +// the cut is dropped whole, one character short of `max`, rather than checked for +// cleanliness — over-truncating by one character is cheap insurance against a +// subtly wrong boundary check. std::string truncateUtf8(std::string s, std::size_t max) { if (s.size() <= max) return s; s.resize(max); @@ -36,12 +38,19 @@ std::string truncateUtf8(std::string s, std::size_t max) { } // `name` with `suffix` inserted before its extension, trimmed so the result still -// fits the entry-name cap. +// fits the entry-name cap. `suffix.size() + ext.size()` can exceed the cap on its +// own (a long extension, a two-digit disambiguation suffix) — clamped rather than +// subtracted unchecked, which would underflow the size_t `room` below and turn +// truncateUtf8 into a silent no-op. An extension that alone leaves no room even +// after the whole stem is dropped is dropped too; uniqueEntryName's own floor +// covers what even that cannot fix. std::string insertSuffix(const std::string& name, const std::string& suffix) { const std::size_t dot = name.find_last_of('.'); const bool hasExt = dot != std::string::npos && dot > 0; std::string stem = hasExt ? name.substr(0, dot) : name; - const std::string ext = hasExt ? name.substr(dot) : std::string(); + std::string ext = hasExt ? name.substr(dot) : std::string(); + if (suffix.size() >= kMaxEntryNameBytes) return std::string(); + if (ext.size() > kMaxEntryNameBytes - suffix.size()) ext.clear(); const std::size_t room = kMaxEntryNameBytes - suffix.size() - ext.size(); stem = truncateUtf8(std::move(stem), room); return stem + suffix + ext; @@ -54,18 +63,21 @@ bool nameTaken(const std::string& candidate, const std::vector& tak } // A transport name distinct from every name already claimed, under the format's own -// case-folding equivalence (two names differing only by ASCII case would extract onto -// one file on Windows and default APFS). +// case-folding equivalence (package_format.h's sameEntryName). std::string uniqueEntryName(const std::string& base, const std::vector& taken) { if (!nameTaken(base, taken)) return base; - // Bounded by construction: each iteration either returns or collides with a - // distinct member of `taken`, and the suffixed names are pairwise distinct. - std::string candidate = base; + // Each iteration either returns a name both valid and distinct from `taken`, or + // advances to the next suffix; taken.size() + 2 attempts is enough by pigeonhole + // now that insertSuffix cannot underflow. The floor below is the residual case + // validity alone can still fail — an extension so long insertSuffix must drop it + // on every attempt tried here. for (std::size_t n = 2; n <= taken.size() + 2; ++n) { - candidate = insertSuffix(base, "_" + std::to_string(n)); + const std::string candidate = insertSuffix(base, "_" + std::to_string(n)); if (!nameTaken(candidate, taken) && isValidEntryName(candidate)) return candidate; } - return candidate; + // Floors like sanitizeEntryName's own "entry" floor: always valid, regardless of + // how base's own extension behaved. + return sanitizeEntryName("entry_" + std::to_string(taken.size() + 2)); } // What BankModel::add and the manifest's nested-path rule together accept — the pair diff --git a/src/core/package/export_plan.h b/src/core/package/export_plan.h index 77df29d..0d7505a 100644 --- a/src/core/package/export_plan.h +++ b/src/core/package/export_plan.h @@ -80,10 +80,8 @@ ExportPlan planExport(const ExportInputs& in); // separators, reserved characters and control bytes to '_', an over-long name // truncated on a UTF-8 boundary, and an underscore prefix for the reserved forms // ("." / ".." / a DOS device name). Never returns a name isValidEntryName refuses. -// -// A bank ingested on macOS/Linux legitimately holds names Windows cannot spell, and -// relaying the codec's one indistinguishable refusal would make a single such file -// an unactionable total failure of the whole export. +// Why sanitize rather than relay the codec's refusal: this directory's own +// CLAUDE.md, "Obligation on the export track." std::string sanitizeEntryName(const std::string& rawFileName); } // namespace reasampler::package diff --git a/src/shell/actions/package_export_action.cpp b/src/shell/actions/package_export_action.cpp index 9b83c6c..fcaebe3 100644 --- a/src/shell/actions/package_export_action.cpp +++ b/src/shell/actions/package_export_action.cpp @@ -164,7 +164,8 @@ void doBankPackageExport(const ReaSamplerSession& session, const std::string& ba const std::string suggested = projectDir + "/" + capture::sanitizeStem(bankName) + ".rsbank"; std::string dest; - if (!pickPackageSavePath(suggested, dest)) return; // user cancelled the picker + bool appended = false; + if (!pickPackageSavePath(suggested, dest, &appended)) return; // user cancelled the picker ExportRequest req; req.projectDir = projectDir; @@ -172,6 +173,11 @@ void doBankPackageExport(const ReaSamplerSession& session, const std::string& ba req.destAbsPath = dest; req.exportTimestamp = static_cast(std::time(nullptr)); req.allowIncomplete = allowIncomplete; + // The dialog's own overwrite confirm covered exactly this path when the picker + // did not need to append `.rsbank` to reach it — asking again would be a second + // prompt for the same consent. An appended path is one the dialog never saw, so + // that case still falls through to exportBank's own refusal and the confirm below. + req.allowOverwrite = !appended; ExportOutcome out = exportBank(session, req); if (out.status == ExportStatus::RefusedDestinationExists) { diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 0ab8adc..a24737f 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -5,12 +5,12 @@ The filesystem and dialog acts behind bank-package export/import: streaming package 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. +journal and its rollback delete (`package_rollback`), the two file pickers +(`package_pickers`), and the promptless export verb (`export_bank`) — the import verb +does not live here yet. The package format itself (magic, manifest, entry layout) +stays `core/package`'s business. 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 +prompting skin (`shell/actions/package_export_action`), not this seam. ## Invariants @@ -47,17 +47,17 @@ belong to the verbs. writer could win the race against. Collision handling (auto-rename) remains the import plan's job upstream. The package writer itself DOES replace an existing destination — the export save dialog's own overwrite confirm is the consent — and - that asymmetry is deliberate. **Known gap, obligation on the export verb:** - `pickPackageSavePath`'s own `.rsbank` re-append (see its Gotcha below) can turn a - confirmed path `X` into a write target `X.rsbank` that the dialog never asked about. - The export verb MUST re-check `fileStatus()` on the path actually handed to - `PackageFileWriter` — after any extension append — and get its own consent if that - re-checked path is `Present`; the dialog's confirm only ever covered the pre-append - path. Not fixed at this seam: prompting is verb-level UX, and `pickPackageSavePath` - has no caller yet, so the gap is latent, not live. **Closed on the export side:** - `exportBank` re-checks `fileStatus()` on the post-append path and refuses - `RefusedDestinationExists` until the caller sets `allowOverwrite`, which - `package_export_action` does only after its own confirm naming that exact path. + that asymmetry is deliberate. **Closed, both halves.** `pickPackageSavePath`'s own + `.rsbank` re-append (see its Gotcha below) can turn a confirmed path `X` into a write + target `X.rsbank` that the dialog never asked about, so `exportBank` re-checks + `fileStatus()` on the path actually handed to `PackageFileWriter` — after any + extension append — and refuses `RefusedDestinationExists` until the caller sets + `allowOverwrite`. The caller does not always re-prompt to get there: + `pickPackageSavePath` reports whether it appended (`outAppended`), and + `package_export_action` pre-grants `allowOverwrite` whenever it did NOT — an + unappended path is exactly what the dialog's own confirm already covered, so asking + again would be a second prompt for the same consent. Only an appended path, one the + dialog never saw, still costs the verb's own confirm naming that exact path. - **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.** The citation and the full discriminator live at `package_rollback.cpp`'s header. "Did this call create it" is structural: only exclusively-created paths are @@ -79,8 +79,8 @@ belong to the verbs. - `package_path` — header-only; the ONE UTF-8-narrow → `fs::path` conversion, so the encoding contract has a single enforcement point. - `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. -- `export_bank` — the promptless export verb, in three composable public steps: `surveyBankExport` (the read-only plan, report-before-acting), `digestSources` (measures each entry's length + `hashBytes` digest, one payload at a time), and `writePackageFile` (prefix, then each payload re-read and re-verified against that digest before it is appended, then commit). `exportBank` composes the three and gates on the plan verdict, the incomplete confirm and the destination confirm. The session arrives **const** — every mutator on it is non-const, so "an export writes no ext state, opens no undo point and never bumps the generation" is enforced by the type rather than remembered. Reads the session through inline accessors only, which is why its tests link and run without a DAW. +- `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`; `pickPackageSavePath` also reports whether it appended `.rsbank` (`outAppended`), the signal `package_export_action` uses to skip a redundant overwrite confirm. `pickPackageForImport` stays compile-only until the import verb lands; neither picker can be exercised in a unit test. +- `export_bank` — the promptless export verb, in three composable public steps: `surveyBankExport` (the read-only plan, report-before-acting), `digestSources` (measures each entry's length + `hashBytes` digest, one payload at a time), and `writePackageFile` (prefix, then each payload re-read and re-verified against that digest before it is appended, then commit). `exportBank` composes the three and gates on the plan verdict, the incomplete confirm and the destination confirm. The session arrives **const**: `saveToActiveProject`, `bumpBankGeneration` and `writeAssignmentRequest` are the session's only non-const acts, so a const session cannot reach them and "an export writes no ext state, opens no undo point and never bumps the generation" holds by the type rather than by memory (`pruneReclaim`, the sole file-deletion path, is const too and sits outside this claim). Reads the session through inline accessors only, which is why its tests link and run without a DAW. ## Gotchas diff --git a/src/shell/package/CMakeLists.txt b/src/shell/package/CMakeLists.txt index caa87da..6f2f148 100644 --- a/src/shell/package/CMakeLists.txt +++ b/src/shell/package/CMakeLists.txt @@ -1,7 +1,8 @@ # The filesystem + dialog seam for bank packages. package_io / package_rollback are # REAPER-free (standard filesystem only), so the pure-library/test helpers fit and -# their tests run without a DAW. The export/import verbs that drive all three targets -# are not in this directory yet. +# their tests run without a DAW. export_bank, the promptless export verb, lives here +# too for the same reason (ReaSamplerSession's inline accessors keep it REAPER-free); +# the import verb does not live here yet. reasampler_pure_library(package_io SOURCES package_io.cpp) reasampler_test(package_io LINK package_io) @@ -11,9 +12,15 @@ reasampler_test(package_rollback LINK package_rollback) # export_bank reads the live session through ReaSamplerSession's INLINE accessors only, # so it pulls in no REAPER-facing TU and its tests link (and run) without a DAW. +# bank_book / tail_control / origin_ledger / tracking_authority / prune_reconcile / +# app_version / view_mode_model are session.h's own transitive includes (BankBook::bank() +# in particular is out-of-line, in bank_book.cpp) — declared here, on the library that +# actually needs them, rather than left for every consumer to enumerate. reasampler_pure_library(export_bank SOURCES export_bank.cpp - LINK PUBLIC export_plan bank_package package_io PRIVATE capture_paths wav_codec) + LINK PUBLIC export_plan bank_package package_io + PRIVATE capture_paths wav_codec bank_book tail_control origin_ledger + tracking_authority prune_reconcile app_version view_mode_model) reasampler_test(export_bank LINK export_bank bank_book slot_map view_mode_model tail_control origin_ledger tracking_authority prune_reconcile app_version capture_paths) diff --git a/src/shell/package/export_bank.h b/src/shell/package/export_bank.h index a6deae8..d78ee3f 100644 --- a/src/shell/package/export_bank.h +++ b/src/shell/package/export_bank.h @@ -2,8 +2,12 @@ // stream, commit. No prompts and no message boxes (shell/actions/ // package_export_action is the skin). The session arrives CONST, which is how "an // export writes no ext state, opens no undo point and never bumps the bank -// generation" is enforced rather than remembered — every mutator on the session is -// non-const. Blocking I/O: UI-thread actions only. +// generation" is enforced rather than remembered — those three acts +// (saveToActiveProject, bumpBankGeneration, writeAssignmentRequest) are exactly the +// session members that are non-const (session.h:113,108,147). Constness does not +// block every mutation, though: pruneReclaim (session.h:140-141) is const and is +// the system's sole file-deletion path — irrelevant to export, but not something a +// const session forbids in general. Blocking I/O: UI-thread actions only. #pragma once diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index c268742..f057f03 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -29,6 +29,7 @@ namespace fs = std::filesystem; namespace { std::atomic g_alivePayloads{0}; +std::atomic g_peakAlivePayloads{0}; } // --------------------------------------------------------------------------- @@ -36,7 +37,13 @@ std::atomic g_alivePayloads{0}; PayloadBuffer::PayloadBuffer(std::vector bytes) : bytes_(std::move(bytes)), counted_(!bytes_.empty()) { - if (counted_) g_alivePayloads.fetch_add(1, std::memory_order_relaxed); + if (counted_) { + const int now = g_alivePayloads.fetch_add(1, std::memory_order_relaxed) + 1; + int peak = g_peakAlivePayloads.load(std::memory_order_relaxed); + while (now > peak && !g_peakAlivePayloads.compare_exchange_weak( + peak, now, std::memory_order_relaxed)) { + } + } } PayloadBuffer::~PayloadBuffer() { release(); } @@ -60,6 +67,7 @@ PayloadBuffer& PayloadBuffer::operator=(PayloadBuffer&& other) noexcept { } int PayloadBuffer::alive() { return g_alivePayloads.load(std::memory_order_relaxed); } +int PayloadBuffer::highWaterMark() { return g_peakAlivePayloads.load(std::memory_order_relaxed); } void PayloadBuffer::release() { if (counted_) g_alivePayloads.fetch_sub(1, std::memory_order_relaxed); diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index adf60ec..384a06b 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -33,6 +33,11 @@ public: // Buffers currently holding at least one byte, process-wide. static int alive(); + // The largest alive() has ever been, process-wide. A point-in-time alive() == 0 + // check after a call returns cannot fail on a whole-package-in-memory shape that + // allocated N buffers and freed them all one at a time — highWaterMark() can, + // since it is never reset. + static int highWaterMark(); private: void release(); diff --git a/src/shell/package/package_pickers.cpp b/src/shell/package/package_pickers.cpp index 16395a6..da04894 100644 --- a/src/shell/package/package_pickers.cpp +++ b/src/shell/package/package_pickers.cpp @@ -47,7 +47,8 @@ bool pickPackageForImport(std::string& outAbsPath) { return runPicker(1, "Import bank package", "", outAbsPath); } -bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath) { +bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath, + bool* outAppended) { // [verify — DAW] GetUserFileName takes no owner window, so the dialog's parenting // is REAPER's to do; the previous Win32 path passed GetMainHwnd() explicitly. if (!runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath)) { @@ -56,7 +57,9 @@ bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPa // GetUserFileName has no lpstrDefExt equivalent (the old Win32 picker's // ofn.lpstrDefExt = L"rsbank"); whether mode 0 appends one itself from // kExtList is [verify — DAW], so append it ourselves whenever it's missing. - if (!hasCaseInsensitiveSuffix(outAbsPath, ".rsbank")) outAbsPath += ".rsbank"; + const bool appended = !hasCaseInsensitiveSuffix(outAbsPath, ".rsbank"); + if (appended) outAbsPath += ".rsbank"; + if (outAppended) *outAppended = appended; return true; } diff --git a/src/shell/package/package_pickers.h b/src/shell/package/package_pickers.h index 9f60512..136a6d9 100644 --- a/src/shell/package/package_pickers.h +++ b/src/shell/package/package_pickers.h @@ -18,7 +18,12 @@ bool pickPackageForImport(std::string& outAbsPath); // suggestedPath is a bare file name ("MyBank.rsbank") or a full path — a full one // also seeds the dialog's starting directory, which is how a caller keeps the picker // off REAPER's process working directory. True with outAbsPath set iff the user chose -// a destination; the dialog's own overwrite confirm has already run by then. -bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath); +// a destination; the dialog's own overwrite confirm has already run by then, against +// the path the user actually chose — NOT necessarily outAbsPath, if the `.rsbank` +// re-append below fires. outAppended, when non-null, is set to whether it fired: the +// caller's signal that its own overwrite consent may not cover the returned path +// (see this directory's CLAUDE.md). +bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath, + bool* outAppended = nullptr); } // namespace reasampler diff --git a/src/shell/panel/panel_bank_ops.cpp b/src/shell/panel/panel_bank_ops.cpp index f946c4f..a45cb99 100644 --- a/src/shell/panel/panel_bank_ops.cpp +++ b/src/shell/panel/panel_bank_ops.cpp @@ -251,7 +251,7 @@ enum : unsigned int { } // namespace // Shows the right-click context menu for a named-bank TAB: activate / rename / delete -// / evacuate that bank, plus a create entry. Drives the id-keyed ops. +// / evacuate / export that bank, plus a create entry. Drives the id-keyed ops. void showTabMenu(int screenX, int screenY, const std::string& bankId) { if (!book()) return; const Bank* bk = book()->bank(bankId); diff --git a/tests/test_export_bank.cpp b/tests/test_export_bank.cpp index 25e1b23..33a6c1b 100644 --- a/tests/test_export_bank.cpp +++ b/tests/test_export_bank.cpp @@ -86,10 +86,11 @@ struct Fixture { } void addSample(const std::string& id, const std::string& fileName, - std::size_t bytes, std::uint8_t seed, bool writeToDisk = true) { + std::size_t bytes, std::uint8_t seed, bool writeToDisk = true, + const std::string& displayName = "") { model::Sample s; s.id = id; - s.displayName = id; + s.displayName = displayName.empty() ? id : displayName; s.relativePath = std::string("reasampler_bank/") + fileName; s.sampleRate = 48000; s.channelCount = 2; @@ -146,6 +147,11 @@ static void testHealthyExportCarriesEveryPayloadByteExact() { CHECK(out.entriesWritten == 3); CHECK(out.excluded.empty()); CHECK(PayloadBuffer::alive() == 0); + // Point-in-time alive() == 0 alone cannot fail on a whole-package-in-memory + // shape (N buffers allocated and freed one at a time still ends at 0); the + // high-water mark can, across this three-entry export and everything the test + // binary ran before it — it must never exceed the "at most one payload" claim. + CHECK(PayloadBuffer::highWaterMark() == 1); const package::DecodedPackage decoded = decodeFromDisk(fx.destPath()); CHECK(decoded.status == package::PackageReadability::Readable); @@ -176,17 +182,51 @@ static void testHealthyExportCarriesEveryPayloadByteExact() { CHECK(PayloadBuffer::alive() == 0); } +// Every occurrence of `"key":"value"` in `json`, value returned raw (escape-aware +// only enough to not stop early on an escaped quote — this file's own writer output +// never nests an unescaped quote, so that is sufficient here). +static std::vector jsonStringValuesForKey(const std::string& json, + const std::string& key) { + std::vector values; + const std::string marker = "\"" + key + "\":\""; + std::size_t pos = 0; + while ((pos = json.find(marker, pos)) != std::string::npos) { + std::size_t i = pos + marker.size(); + while (i < json.size() && json[i] != '"') { + if (json[i] == '\\') ++i; // skip the escaped char too + ++i; + } + values.push_back(json.substr(pos + marker.size(), i - (pos + marker.size()))); + pos = i; + } + return values; +} + +// True for a value opening with an ':' drive-relative prefix (":" alone is +// JSON's own key separator, so this is checked on isolated VALUES, never on raw text). +static bool looksLikeDriveForm(const std::string& value) { + return value.size() >= 2 && std::isalpha(static_cast(value[0])) && + value[1] == ':'; +} + static void testEmittedManifestBytesCarryNoPath() { Fixture fx("nopath"); + // The bank's own display name AND a sample's displayName each carry a literal + // '/' — free text, unlike the entry `name` / nested `relativePath` fields this + // test actually polices (docs/product/bank-package.md:282-289: the destination is + // derived from the entry name, never from free text). Present in the fixture so + // the scan below proves it is scoped correctly rather than merely holding by + // accident on names that happen not to collide with the rule. + CHECK(fx.session.book().renameBank(fx.bankId, "Drums/Bus")); // Both source records carry a directory component; neither may reach the file. - fx.addSample("s1", "kick.wav", 200, 3); + fx.addSample("s1", "kick.wav", 200, 3, /*writeToDisk=*/true, "Kick / alt take"); fx.addSample("s2", "snare take 2.wav", 200, 9); CHECK(exportBank(fx.session, fx.request()).status == ExportStatus::Written); - // Scan the MANIFEST REGION of the emitted file, located by walking the frozen - // header the way a reader does: magic | fv | minReader | len+semver | len+JSON. - // The binary length fields are deliberately excluded — a length whose byte - // happens to be 0x2F is not a separator. + // Locate the MANIFEST REGION of the emitted file by walking the frozen header the + // way a reader does: magic | fv | minReader | len+semver | len+JSON. The binary + // length fields are deliberately excluded — a length whose byte happens to be + // 0x2F is not a separator. const std::vector file = readFile(fx.destPath()); CHECK(file.size() > 20); auto le32 = [&](std::size_t at) { @@ -202,22 +242,31 @@ static void testEmittedManifestBytesCarryNoPath() { CHECK(manifestAt + manifestLen <= file.size()); const std::string manifest(reinterpret_cast(file.data() + manifestAt), manifestLen); - CHECK(manifest.find("kick.wav") != std::string::npos); // the scan is looking at the manifest - CHECK(manifest.find('/') == std::string::npos); - CHECK(manifest.find('\\') == std::string::npos); - CHECK(manifest.find("..") == std::string::npos); - CHECK(manifest.find("reasampler_bank") == std::string::npos); - // ':' cannot be banned outright — it is JSON's own key separator — so the check - // is for the drive form specifically: a string value opening with ':'. - // With '/' and '\\' already absent, that covers the drive-relative spelling too. - bool driveForm = false; - for (std::size_t i = 0; i + 2 < manifest.size(); ++i) - if (manifest[i] == '"' && - std::isalpha(static_cast(manifest[i + 1])) && - manifest[i + 2] == ':') - driveForm = true; - CHECK(!driveForm); + + // The boundary, pinned rather than assumed: free text legitimately carries '/'. + CHECK(manifest.find("Drums/Bus") != std::string::npos); + CHECK(manifest.find("Kick / alt take") != std::string::npos); + + // The rule itself: scoped to the two fields the importer derives a destination + // from — the entry `name` and the nested Sample's own `relativePath` — never to + // `displayName` or the manifest's `bankDisplayName`. + const std::vector names = jsonStringValuesForKey(manifest, "name"); + const std::vector relPaths = jsonStringValuesForKey(manifest, "relativePath"); + CHECK(!names.empty()); + CHECK(!relPaths.empty()); + for (const std::string& v : names) { + CHECK(v.find('/') == std::string::npos); + CHECK(v.find('\\') == std::string::npos); + CHECK(v.find("..") == std::string::npos); + CHECK(!looksLikeDriveForm(v)); + } + for (const std::string& v : relPaths) { + CHECK(v.find('/') == std::string::npos); + CHECK(v.find('\\') == std::string::npos); + CHECK(v.find("..") == std::string::npos); + CHECK(!looksLikeDriveForm(v)); + } } static void testFailureInjectedMidWriteLeavesNoPackageAndSparesThePriorFile() { @@ -252,6 +301,39 @@ static void testFailureInjectedMidWriteLeavesNoPackageAndSparesThePriorFile() { CHECK(PayloadBuffer::alive() == 0); } +static void testCommitFailureIsAGenuineMidWriteAbandon() { + // The mid-READ injection above (a source vanishing between digest and stream) is + // not what "mid-write" names in export_bank.cpp:115-118/122-125 — those guard a + // failure IN the write itself: appendPayload's stream going bad, or commit's + // rename failing. A directory squatting on the destination (test_package_io.cpp's + // own precedent for a real, not simulated, commit failure) makes every payload + // stream fine and only the final rename fail. + Fixture fx("commitfail"); + fx.addSample("s1", "kick.wav", 300, 1); + fx.addSample("s2", "snare.wav", 300, 2); + + std::error_code ec; + fs::create_directory(utf8Path(fx.destPath()), ec); + CHECK(!ec); + + ExportSurvey survey = surveyBankExport(fx.session, fx.projectDir, fx.bankId); + CHECK(survey.plan.verdict == package::ExportVerdict::Ready); + package::PackageManifest manifest = survey.plan.manifest; + const std::vector sources = {fx.absPathOf("kick.wav"), fx.absPathOf("snare.wav")}; + std::string failed; + CHECK(digestSources(manifest, sources, failed)); + const std::optional encoded = package::encodePackage(manifest); + CHECK(encoded.has_value()); + + const ExportOutcome out = writePackageFile(*encoded, manifest, sources, fx.destPath()); + CHECK(out.status == ExportStatus::WriteFailed); + CHECK(fs::is_directory(utf8Path(fx.destPath()))); // the squatting dir is untouched + CHECK(!exists(fx.destPath() + ".rsbanktmp")); // commit()'s own self-clean ran + CHECK(PayloadBuffer::alive() == 0); + + fs::remove(utf8Path(fx.destPath()), ec); +} + static void testPayloadChangedBetweenDigestAndStreamAborts() { Fixture fx("changed"); fx.addSample("s1", "kick.wav", 500, 1); @@ -280,14 +362,27 @@ static void testExportTouchesNoProjectState() { fx.addSample("s1", "kick.wav", 400, 5); fx.addSample("s2", "snare.wav", 400, 6); - // The ext-state blob IS the serialized book (shell/persist/ext_state_io), so - // byte-identity of that string is byte-identity of what a persist would write. - const std::string extStateBefore = fx.session.book().serialize(); + // saveToActiveProject writes seven keys (shell/persist/ext_state_io.cpp): `banks`, + // the legacy-key clear, `view_state`, the tail setting, the tracking ledger, the + // version stamp, and the bank-generation counter. This asserts byte-identity of + // the three that have an in-memory string to diff (`banks`, `view_state`, the tail + // setting) plus bankGeneration() (the bank-generation-counter key IS its + // serialization). The legacy-key clear and the version stamp are session-external, + // nothing here to diff against. The tracking ledger has no public accessor to diff + // either, but needs none: exportBank/digestSources/writePackageFile all take the + // session by `const&`, and ReaSamplerSession::recordCreated — the ledger's one + // writer (session.h) — is non-const, so it is not reachable through this call at + // all; the compiler enforces "untouched" here rather than a runtime check proving it. + const std::string bankBookBefore = fx.session.book().serialize(); + const std::string viewBefore = fx.session.view().serialize(); + const std::string tailBefore = capture::serializeTailSetting(fx.session.tail()); const std::int64_t generationBefore = fx.session.bankGeneration(); CHECK(exportBank(fx.session, fx.request()).status == ExportStatus::Written); - CHECK(fx.session.book().serialize() == extStateBefore); + CHECK(fx.session.book().serialize() == bankBookBefore); + CHECK(fx.session.view().serialize() == viewBefore); + CHECK(capture::serializeTailSetting(fx.session.tail()) == tailBefore); CHECK(fx.session.bankGeneration() == generationBefore); } @@ -359,6 +454,7 @@ int main() { testHealthyExportCarriesEveryPayloadByteExact(); testEmittedManifestBytesCarryNoPath(); testFailureInjectedMidWriteLeavesNoPackageAndSparesThePriorFile(); + testCommitFailureIsAGenuineMidWriteAbandon(); testPayloadChangedBetweenDigestAndStreamAborts(); testExportTouchesNoProjectState(); testEmptyBankExportsAsAValidZeroEntryPackage(); diff --git a/tests/test_export_plan.cpp b/tests/test_export_plan.cpp index f29c356..3ccd784 100644 --- a/tests/test_export_plan.cpp +++ b/tests/test_export_plan.cpp @@ -167,6 +167,27 @@ static void testHostileNamesAreRepairedNotRelayed() { } } +static void testUniqueNameSurvivesLongExtensionUnderflow() { + // insertSuffix computes room = kMaxEntryNameBytes - suffix.size() - ext.size() in + // size_t; an extension long enough that even a two-digit "_10" suffix pushes the + // sum past the cap must not wrap that subtraction. Ten same-named entries force + // the tenth collision into double digits against a 253-byte extension (253 + 3 = + // 256, one over kMaxEntryNameBytes). + const std::string hostileName = "a." + std::string(252, 'x'); // 254 bytes, otherwise valid + std::vector candidates; + for (int i = 0; i < 10; ++i) + candidates.push_back(present("s" + std::to_string(i), "reasampler_bank/" + hostileName)); + + const ExportPlan p = planExport(bankOf(candidates)); + CHECK(p.verdict == ExportVerdict::Ready); + CHECK(p.manifest.entries.size() == 10); + for (const PackageEntry& e : p.manifest.entries) CHECK(isValidEntryName(e.fileName)); + for (std::size_t i = 0; i < p.manifest.entries.size(); ++i) + for (std::size_t j = i + 1; j < p.manifest.entries.size(); ++j) + CHECK(!sameEntryName(p.manifest.entries[i].fileName, + p.manifest.entries[j].fileName)); +} + static void testCaseFoldedCollisionsAreDisambiguated() { const ExportPlan p = planExport(bankOf({ present("s1", "reasampler_bank/Kick.wav"), @@ -253,6 +274,7 @@ int main() { testUnreadableFileStaysDistinctFromMissing(); testUnrepresentableRecordRefusesWholeExport(); testHostileNamesAreRepairedNotRelayed(); + testUniqueNameSurvivesLongExtensionUnderflow(); testCaseFoldedCollisionsAreDisambiguated(); testSanitizeNeverReturnsANameTheCodecRefuses(); testPlannedManifestSatisfiesTheCodec(); diff --git a/tests/test_package_io.cpp b/tests/test_package_io.cpp index d6237f6..eaddba9 100644 --- a/tests/test_package_io.cpp +++ b/tests/test_package_io.cpp @@ -72,7 +72,9 @@ static void testPayloadCounterTracksMovesNotCopies() { } static void testStreamingRoundTripHoldsOnePayload() { - const std::string dest = "pkg_io_scratch.rsbank"; + std::error_code destEc; + const std::string dest = + (fs::temp_directory_path(destEc) / "pkg_io_scratch.rsbank").generic_string(); const std::vector header = patternBytes(16, 0xA0); const std::vector> entries = { patternBytes(1000, 1), patternBytes(500, 2), patternBytes(1, 3)};