From b99027bc4ca5d5a03543b1f923141cb5174ee765 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 14:36:13 -0400 Subject: [PATCH] docs: collapse the landed Phase E track specs to Landed form Five tracks across W1 and W2 now point at COMPLETED.md. W3 and the phase header stay live. --- docs/PLAN.md | 456 ++++++++++----------------------------------------- 1 file changed, 88 insertions(+), 368 deletions(-) diff --git a/docs/PLAN.md b/docs/PLAN.md index 3a2883d..0583928 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2408,6 +2408,10 @@ capture backend.** ### Ε-W1 — The contract, the filesystem, and the ledger's new kind +**All three tracks have landed** — Ε-W1-T1 (`package-format`), Ε-W1-T2 +(`package-fs-shell`), and Ε-W1-T3 (`import-origin-kind`) — see `docs/COMPLETED.md` for +the full narrative of each. + **Depends on:** nothing in this phase. **Three tracks, disjoint by directory** — the split is by *what each track's inputs are*, which is why they genuinely parallelize: T1 knows only bytes and structs, T2 knows only paths and bytes, T3 knows only the ledger. @@ -2430,212 +2434,63 @@ T3 touches no package code at all. #### Ε-W1-T1 — `package-format` -**Goal.** The container and its version ladder, entirely pure — the contract every later -track consumes, landed once so nothing downstream re-litigates the shape. - -**Spec:** `docs/product/bank-package.md` §"The container", §"Version tagging", §"What a -package carries", §"What a package deliberately does NOT carry", §"Memory". - -**Surface boundary — owns:** new `src/core/package/package_format` (the magic, the header -layout, `kPackageFormatVersion`, `kPackageMinReaderVersion`, and -`classifyPackageVersion(formatVersion, minReader) -> Readable | TooNew | Malformed`), new -`src/core/package/package_manifest` (the manifest model + its JSON codec), new -`src/core/package/bank_package` (header encode, prefix decode, entry-layout arithmetic), the -directory's `CMakeLists.txt` and `CLAUDE.md`, and one appended `add_subdirectory` line in the -root `CMakeLists.txt`. **Does not own:** `export_plan` / `import_plan` (Ε-W2), anything under -`shell/`, `core/model`, or `core/tracking`. - -**Behavior.** -- **The container is the hand-rolled `RSBK` (Ε-F1, ruled).** Magic `RSBK`, a fixed - little-endian header carrying the two version fields, a length-prefixed JSON manifest, then - each entry's payload concatenated in manifest order. Framing is built on `core/wire/bytes.h` - (`putLE` / `ByteReader`) and the manifest on `core/json` — both already owned and tested - here. **No ZIP, no compressor, no new third-party source in the build**; a link edge to - `vendor/WDL/WDL/zlib/` means the ruling was misread. -- **Two version integers, not one.** `formatVersion` = what this writer emitted; - `minReaderVersion` = the oldest reader that can read it safely. The reader's whole rule is - `minReaderVersion <= kPackageFormatVersion`. An **additive** change (a new optional - manifest key, a new `Sample` field with a defined absent-value) bumps `formatVersion` - only; a **structural** change bumps both. **Growing a persisted enum's vocabulary is - structural here, not additive** — `BankModel::deserialize` *rejects* an out-of-range - `SourceMode` or `Tier` rather than degrading it (`bank_model.cpp:232-239`, `:339-346`), - and every enum a package carries rides inside the nested `BankModel` blob, so a new - `SourceMode` or `Tier` value bumps both integers. The header carries the writer's semver - (`version::stampVersion()`) alongside them, informational, so a refusal message can name - what to install. -- **The ladder is documented the way `origin_ledger.cpp:8-21` documents its own** — a header - comment listing every shipped version and what changed, with the read-and-validate rule - stated, not implied. -- **Unknown manifest keys are skipped** (the `bank_book_json.cpp:182` behaviour), and - **unknown persisted enum integers — the manifest's own, not `BankModel`'s nested ones, which - reject per the bullet above — degrade to their defined `Unknown` equivalent**, never to - the numeric default and never to a parse failure (`core/wire/CLAUDE.md`'s `BakeStatus` rule, - verbatim). Both are pinned by tests, not left to inheritance. -- **The manifest nests `BankModel`'s own serialization verbatim**, exactly as - `bank_book_json.cpp:15-20` nests it, so per-sample shape has one owner and a future - `Sample` field reaches packages for free. Per entry the manifest adds only: the bare file - name, the byte length, and a `hashBytes` digest (`core/capture/wav_codec.h:143`) — - `hashBytes`, **not** `hashWavContent`, because the latter deliberately skips chunks - (`wav_codec.h:145-151`) and so cannot answer "did these bytes survive." -- **The bank's `slot_map` rides along** — display positions are part of what the user built. -- **Framing only, never a payload.** `bank_package` produces the header bytes and an ordered - `[{ name, offset, length }]` layout; it never holds, copies, or hashes an entry's audio. - Decode is symmetric: prefix in, manifest + layout out. -- **Path expression is structurally impossible.** Entry names are validated to contain no - `/`, `\`, `:`, no leading separator, and no `..` component, on both encode and decode. - -**Acceptance criteria.** -- `decodePackage(encodePackage(x)) == x` over a manifest fixture exercising every field, - including every `Sample` optional in both present and absent states. -- A synthetic header with `minReaderVersion` above this build classifies `TooNew` and **no - manifest is produced** — the decode does not half-succeed. -- A synthetic header with `formatVersion` above this build but `minReaderVersion` at or below - it classifies `Readable`, and its unknown manifest keys are skipped without error. This is - the additive-forward-compatibility claim, and it is the reason the two-integer design - exists; a test that does not exercise it leaves the design unproven. -- Truncated input at every byte offset in a valid package returns `Malformed` — never UB, - never a partial manifest, never a read past the buffer. Hostile-input hardening at the - `bank_model.h:204-206` standard. -- Entry names containing `..`, a separator, or an absolute prefix are rejected on encode - *and* rejected on decode. Both directions, because a package can arrive from anywhere. -- No file in the new directory exceeds ~600 lines; the three-module split above is the - responsibility seam, and a fourth module is preferred over a bisection if one is needed. -- `package_format_tests`, `package_manifest_tests`, `bank_package_tests` all run without - REAPER or a DAW. - -**Open questions.** **No [Daniel] questions — Ε-F1 is RULED** (proprietary `RSBK`), so this -track is dispatchable as written. **[propose at review]** whether `package_format` and -`bank_package` are genuinely two modules or one — the split is proposed on responsibility -grounds (constants and classification vs. offset arithmetic) and may collapse if the -arithmetic turns out to be twenty lines. +**Landed** — see `docs/COMPLETED.md` for the full narrative. The pure `src/core/package/` +codec for the hand-rolled `RSBK` container (Ε-F1, ruled — no ZIP, no compressor, no link +edge to `vendor/WDL/WDL/zlib/`): a fixed little-endian header carrying two version +integers — `formatVersion` (what the writer emitted) and `minReaderVersion` (the oldest +reader that can read it safely) — a length-prefixed JSON manifest, and payloads +concatenated in manifest order. `classifyPackageVersion` answers `Readable`/`TooNew`/ +`Malformed`; a `TooNew` header refuses whole, producing no manifest, so the refusal can +still name the writer's semver rather than half-succeeding. The ladder's one +counterintuitive rule rides with the contract: **growing a persisted enum's vocabulary is +structural, not additive** — `BankModel::deserialize` *rejects* an out-of-range `SourceMode` +or `Tier` rather than degrading it (`bank_model.cpp:232-239`, `:339-346`), and every enum a +package carries rides inside the nested `BankModel` blob, so a new value bumps both +integers, where a new `Sample` field with a defined absent-value bumps `formatVersion` +alone. Landed as three modules: `package_format` (the contract, the version ladder, and +three name-validation rules — +`isValidEntryName`, `sameEntryName`'s ASCII-case fold, `isValidNestedSamplePath`), +`package_manifest` (the manifest model + JSON codec, carrying the bank's `slot_map` and a +whole-file `hashBytes` digest per entry — deliberately not `hashWavContent`, which skips +chunks and so cannot answer "did these bytes survive"), and `bank_package` +(framing/layout arithmetic: `encodePackage`/`decodePackage`/`requiredPrefixSize`, never +holding or hashing a payload itself). Hostile input is refused, never UB, at every byte +offset. #### Ε-W1-T2 — `package-fs-shell` -**Goal.** Every filesystem and dialog act the two verbs need, landed behind an API that knows -nothing about what a package contains — so it can be authored, reviewed, and tested in -parallel with the format it will carry. - -**Spec:** `docs/product/bank-package.md` §"Where it lives", §"Failure modes", §"Memory". - -**Surface boundary — owns:** new `src/shell/package/package_io` (read a file's bytes, write -bytes through temp + atomic rename, read one bank file, write one landed file, enumerate the -bank folder's existing names, and the rollback delete), the file-picker seam for both verbs — -**one picker, REAPER's own, on every platform**, so this track carries **no** -`#ifdef _WIN32` / `#else swell/swell.h` split. That split is a real pattern in this codebase -(`src/shell/panel/draw_kit.cpp:11-15`, `src/shell/persist/prune_fs.cpp:35-38`); it is simply -not this track's shape, because REAPER owns the dialog. Also owns the -directory's `CMakeLists.txt` and `CLAUDE.md`, and one appended `add_subdirectory` line in the -root `CMakeLists.txt`. **Does not own:** `export_bank` / `import_bank` (Ε-W2), anything under -`core/`, and — emphatically — `prune_fs`, which stays the deletion authority. - -**Behavior.** -- **Atomic write.** A package is written to a temp path in the destination directory and - renamed on complete success. A failed or interrupted write leaves no `.rsbank` behind. This - is the mono-collapse precedent (Ψ-W2-T2, temp file + atomic rename) applied to a much - larger file. -- **Streaming, both ways.** Append one payload at a time on write; seek and read one payload - at a time on read. The API must make holding the whole package awkward, not merely - discouraged. -- **The rollback delete is the carve-out, cited.** Its TU header cites - `src/shell/persist/prune_fs.cpp:5-11` and states the discriminator it satisfies — this call - created the file, and no index ever referenced it — rather than restating the carve-out's - text. Anything that does not satisfy that discriminator is not this function's business. -- **Both pickers are REAPER's own, and they are symmetric.** `GetUserFileName(int mode, - const char* caption, const char* initial_file_or_path, const char* extension_list, char* - fnOutNeedBig, int fnOutNeedBig_sz)` — **verified**, - `vendor/reaper-sdk/sdk/reaper_plugin_functions.h:3790`, documented at `:3788` — serves - both directions: `mode=0` ("choose a new file") is export's destination picker, `mode=1` - ("existing file") is import's source picker. `extension_list` takes the - `'ReaSampler banks|*.rsbank|All files|*.*'` form, and `initial_file_or_path` may be a bare - `'.rsbank'` to set the default extension. There is **no `#ifdef _WIN32` / SWELL split and - no wide-char round trip here** — REAPER owns the dialog on every platform, so no - `GetSaveFileNameW` and no `BrowseForSaveFile`. `GetUserFileNameForRead` is explicitly - "Superseded, see GetUserFileName" (`:3796`) and is not used. -- **No fallback path.** `src/app/main.cpp:15` defines `REAPERAPI_IMPLEMENT` *without* - `REAPERAPI_MINIMAL`, so the resolver walks the full table — `GetUserFileName` included - (`reaper_plugin_functions.h:9084`) — and `main.cpp:292-293` aborts the extension load if - any single function fails to resolve. No REAPER build that loads this extension can lack - `GetUserFileName`, which makes a fallback unreachable code. -- **No REAPER project state is touched here.** No ext-state read or write, no undo block, no - generation bump; those belong to the verbs in Ε-W2. - -**Acceptance criteria.** -- A write interrupted before completion leaves the destination path absent or holding its - prior contents — never a partial new file. Tested by injecting a failure at the writer seam. -- Reading and writing a multi-entry package never holds more than one entry's payload; the - test asserts against a seam counter, not against a memory measurement. -- The rollback deletes exactly the files it was given and nothing else, and is a no-op on a - path it did not write. -- **There is exactly one picker call site and it is REAPER's.** Both verbs reach - `GetUserFileName` — export with `mode=0`, import with `mode=1` — and no symbol named - `GetSaveFileNameW`, `BrowseForSaveFile`, or `GetUserFileNameForRead` appears anywhere in - `src/shell/package/`, nor any platform `#ifdef` in the picker's TU. Greppable, so it stays - true. `[verify — DAW]` — the picker is not exercised in a live REAPER session by this track. -- No file exceeds ~600 lines; the picker lives in its own TU with its own header. **Not** for - the `drag_out` / `drag_out_win` reason — that precedent isolates a *Win32-only* TU, and - there is no platform split here — but because the picker is the only REAPER-facing part of - an otherwise REAPER-free, unit-tested module: folding it into a shared header would drag - `reaper_plugin_functions.h` into the testable seam's include graph. - -**Open questions.** **[propose at review]** where the `extension_list` and default-extension -strings live — this track's picker TU, or the Ε-W2 verbs that call it. They are user-facing -text, and the verbs own the rest of the user-facing text; the counter-argument is that they -are picker plumbing and only one picker exists. **[verify — DAW]** two things the header does -not answer: (1) whether the `mode=0` picker **appends** an extension when the user types a -bare name — `:3788` documents that `initial_file_or_path` may be `'.rsbank'` "to set the -default extension," but not that the dialog enforces it on return, so the verb may still have -to append `.rsbank` itself; (2) **dialog parenting** — `GetUserFileName`'s signature -(`:3790`) takes no owner window, where the abandoned Win32 `OPENFILENAME` path would have -passed `GetMainHwnd()`, so modality against the REAPER main window is unobserved. +**Landed** — see `docs/COMPLETED.md` for the full narrative. `src/shell/package/`: +streaming, atomic package filesystem I/O (`package_io`'s `PackageFileWriter`/ +`PackageFileReader`, at most one entry's payload materialized at a time, backed by a +`.rsbanktmp` sibling that reaches the destination only through a `commit()` rename — +process-crash atomic, not power-loss atomic, deliberately, since an `fsync` over a whole +sample bank is a real stall) and the rollback journal (`package_rollback`'s +`LandedFileJournal`, citing the `prune_fs.cpp` carve-out rather than restating it, +disarmed only after the caller's own write has returned success). `package_pickers` is the +one picker seam, REAPER's own in both directions as specified — `GetUserFileName` with +`mode=0` for export's destination and `mode=1` for import's source, so no platform `#ifdef`, +no SWELL `BrowseForSaveFile`, no Win32 `GetSaveFileNameW`, and no `GetUserFileNameForRead`, +which the SDK header marks superseded by `GetUserFileName`. No fallback path was needed +either: `main.cpp` aborts the extension load if any API pointer fails to resolve, so no +REAPER build that loads the extension can lack it. The picker is `[verify — DAW]` in both +directions, never exercised in a live REAPER session. #### Ε-W1-T3 — `import-origin-kind` -**Goal.** Give the ledger a birth-record kind for a package import, so an imported file is -tracked from the moment it lands rather than becoming a permanently unreclaimable foreign -file — landed as its own track, with its own review, because it edits safety-critical -territory that nothing else in this phase touches. - -**Spec:** `docs/product/bank-package.md` §"What a package deliberately does NOT carry" (the -origin-ledger bullet); `src/core/tracking/CLAUDE.md` for the invariants it must not weaken. - -**Surface boundary — owns:** `src/core/tracking/origin_ledger.{h,cpp}` and its tests, -exclusively. **Does not own:** `tracking_authority` (no decision changes), `shell/persist`, -or any consumer. - -**Behavior.** -- **Append `OriginKind::PackageImport` as value 5.** Append only — `Capture`=1, `Ingest`=2, - `Recapture`=3, `Resample`=4 keep their integers, per `core/tracking/CLAUDE.md`'s - "PERSISTED INTEGERS — never renumber, only append". -- **A build that does not know value 5 degrades it to `Unknown`**, which is the existing - `kindFromInt` behaviour and is the safe direction: the path is still owned, so still - protected; only the kind detail is lost. This is the *field-vocabulary* rule, and it must - stay distinct from the *document-version* rule right beside it, which blocks - (`origin_ledger.cpp:18-21`). -- **Nothing else changes.** No new field, no version bump, no lineage semantics. A new enum - value in an append-only vocabulary is precisely the change that does **not** need `"v"` to - move, and demonstrating that is part of the point. -- **`recordCreated` needs no signature change** — it already takes an `OriginKind` - (`src/shell/persist/session.h:95`). Confirm that in the same pass; if it turns out - otherwise, that discovery is this track's, not Ε-W2's. - -**Acceptance criteria.** -- A ledger containing a kind-5 record round-trips through serialize/deserialize unchanged. -- A record carrying an *unrecognized* kind integer (6, 99, negative) loads as `Unknown` and - the ledger loads `Loaded`, not `Unreadable` — the vocabulary gap does not halt prune. -- `kLedgerVersion` is **unchanged** at 2, and a test asserts it, so the append-vs-bump - distinction is pinned rather than assumed. -- `pruneProtection`'s output is unchanged for every existing kind — this track alters no - decision. - -**Open questions.** **[propose at review]** whether the kind is named `PackageImport` or -folded onto the existing `Ingest`. The plan's recommendation is a distinct value: `Ingest` -means "the user brought in a file," which is close, but losing the distinction makes a future -"where did this bank come from" question unanswerable, and an appended integer costs nothing. +**Landed** — see `docs/COMPLETED.md` for the full narrative. `OriginKind::PackageImport` +appended to the tracking ledger as value 5 — package-sourced vs `Ingest`'s user-picked. +Append-only, per `core/tracking/CLAUDE.md`'s persisted-integer rule; an unrecognized kind +degrades to `Unknown` rather than failing the parse, and `kLedgerVersion` stays at 2 — a +vocabulary addition, not a document-version bump. No decision surface changed: +`pruneProtection`'s output is unaffected for every existing kind. --- ### Ε-W2 — The two verbs +**Both tracks have landed** — Ε-W2-T1 (`bank-export`) and Ε-W2-T2 (`bank-import`) — see +`docs/COMPLETED.md` for the full narrative of each. + **Depends on Ε-W1 — all three tracks.** T1 for the format the verbs speak, T2 for every filesystem act they perform, T3 for the kind their birth records carry. No part of either verb is authorable against a format that has not settled. @@ -2660,182 +2515,47 @@ second. #### Ε-W2-T1 — `bank-export` -**Goal.** One bank leaves the project as one file, or the export refuses and says why. - -**Spec:** `docs/product/bank-package.md` §"Failure modes" (export rows), §"What a package -carries". - -**Surface boundary — owns:** new `core/package/export_plan` (pure: which entries, what -names, what is missing, and therefore whether the export may proceed), new -`shell/package/export_bank` (the promptless verb — takes a `ReaSamplerSession&`, returns an -outcome, **no prompts and no message boxes**, mirroring `src/shell/bank_ops/`), new -`shell/actions/package_export_action` (the bindable-action skin, mirroring `prune_action`), -one registration line in `src/app/main.cpp`, one panel menu row. **Does not own:** anything -on the import side, `bank_ops`, or `persist`. - -**Behavior.** -- **The exported unit is one bank** — the pool included, since the pool is structurally a bank - (`core/model/CLAUDE.md`'s pool-privileges section). Whole-book export is an explicit - non-goal of this phase and is preserved as an additive future by the manifest's shape, not - by a promise. -- **Refuse-if-incomplete, report-before-acting.** An index entry whose file is missing or - unreadable stops the export by default; the "export the N present entries" path exists only - behind an explicit confirm that lists what is absent, distinguishing missing from - unreadable. This is prune's dry-run-then-confirm discipline applied to a non-destructive - act, and it is deliberate: a silently-incomplete package is discovered on the far side, in - another project, weeks later. -- **The project is not touched.** No ext-state write, no generation bump, no undo point. An - export that mutates project state is a defect, and the acceptance criteria name it as one. -- **Nothing is re-encoded.** Payload bytes are copied and hashed. `wav_codec` is not asked to - rebuild anything. -- **A new FOREVER-STABLE command id** is minted through `version::channelCommandId(suffix)` - per the root `CLAUDE.md` action contract, with its per-channel display name through - `channelActionName`. - -**Acceptance criteria.** -- `planExport` is pure and total over its inputs: a bank with a missing file, an unreadable - file, zero samples, and one sample all classify without touching a filesystem. -- Exporting a bank and re-reading the package yields, for every entry, a `hashBytes` digest - equal to the source file's — asserted per entry, not in aggregate. -- The exported manifest contains no absolute path and no path separator, asserted by a test - that scans the emitted bytes rather than by inspecting the model. -- A failure injected mid-write leaves no `.rsbank` at the destination and the prior file, if - any, intact. -- Project ext state is byte-identical before and after an export, and `bankGeneration()` is - unchanged — a direct assertion, because "we did not mean to write anything" is not a - property that survives without one. -- 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 — 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. +**Landed** — see `docs/COMPLETED.md` for the full narrative. New `core/package/export_plan` +(pure: which entries, what names, what is missing, and therefore whether the export may +proceed — verdict `Ready`/`Incomplete`/`Refused`) and `shell/package/export_bank` (the +promptless verb, in three composable public steps — `surveyBankExport`, `digestSources`, +`writePackageFile` — arriving with a **const** `ReaSamplerSession&`, so "writes no ext +state, opens no undo point, never bumps the generation" holds by the type rather than by +memory), plus `shell/actions/package_export_action`, one `main.cpp` action-table row, and +one panel bank-menu row. Nothing is re-encoded; payloads are copied and hashed. The +exported unit is one bank — the pool included, since the pool is structurally one +`BankIndex` among many — and whole-book export stays out of scope for the phase. Both open +questions were answered at review: affordance ships as **both** the bindable action and +the panel row, and the default file name derives from the bank's display name through +`capture_paths::sanitizeStem`. #### Ε-W2-T2 — `bank-import` -**Goal.** A package becomes a **new** bank in this project — completely, or not at all — -with every one of the four collision classes answered explicitly rather than by whatever the -model happens to do. - -**Spec:** `docs/product/bank-package.md` §"Identity and collision on import" (including the -auto-suffix rule), §"Failure modes" (import rows), §"Import under a degraded tracking -ledger", §"Version tagging: both directions". - -**Surface boundary — owns:** new `core/package/import_plan` (pure: the id remap table, the -parent remap, the per-entry write / skip-already-present / rename disposition, the -destination bank name after uniqueness folding), new `shell/package/import_bank` (the -promptless verb), new `shell/actions/package_import_action`, the panel's `WM_DROPFILES` route -for a `.rsbank` (routing only — the existing ingest route for audio files is untouched), one -registration line in `src/app/main.cpp`, one panel menu row, and — the **only** `core/model/` -edit in the phase — **one additive public `const` member on `BankBook`** (recommended -`std::string uniqueDisplayName(const std::string& seed) const`), so the auto-suffix probe runs -behind the model's own name fold. **Does not own:** anything on the export side, -`bank_book`'s *rules* (consumed, never re-implemented — the new member exposes the existing -fold, it does not add a second one), `origin_ledger` (Ε-W1-T3's). - -**Behavior.** -- **The ledger guard runs FIRST — before the file picker opens (Ε-F3, ruled: refuse).** If - `tracking::ledgerDegraded(status)` holds for the project's loaded ledger status - (`Unreadable` or `FutureVersion`; `core/tracking/origin_ledger.h:94`, `:100-101`), the - import **refuses outright** — no picker, no bytes read, no confirm-and-proceed path, no - opt-out. `Fresh` and `Loaded` both proceed. **Do not key this on - `PruneReport::blockedByTracking`**: that flag also fires on undecodable `rsusage_*` keys, - which govern deletion-time protection and have nothing to do with writing birth records. - The refusal is a `ShowConsoleMsg` block mirroring `prune_action.cpp:30-69` in structure and - tone, with two variants (malformed / newer-build) and every recovery line naming **this - build's** namespace through `version::extStateNamespace()`. Exact wording in the spec doc. - **Export is deliberately not gated this way** — that is Ε-W2-T1's, and it stays ungated. -- **Version gate second, before any byte is written.** `minReaderVersion` above this build - refuses the whole package and reports through `ShowMessageBox` - (**verified**, `reaper_plugin_functions.h:6546`) naming three things: the package's - requirement, this build's ceiling, and the writer's semver. Two of the three is not enough - to act on. A malformed or truncated package reports **distinctly** — the two failures have - opposite recoveries, which is exactly why `origin_ledger.cpp:178-185` separates them. -- **Four collisions, four answers.** (1) **Sample id** — remint every id and remap - `Provenance::parentSampleId` (`bank_model.h:45-50`) through the same map, to the reminted - parent when it came in the same package and cleared otherwise; a foreign id never enters - the index. (2) **File name** — never overwrite; mint a fresh unique name through - `capture_paths::deriveBankPaths`, silently, counted in the summary. (3) **Content hash** — - consult `BankModel::findByHash` **before writing the payload**; on a hit, skip the write - entirely and let the entry collapse, so a dedup never manufactures an orphan. (4) **Bank - display name** — **auto-suffix, no prompt** (Ε-F2, ruled). Seed = the package's recorded - source bank name **verbatim** (or the literal `Imported bank` if absent/blank); take the - first of `seed`, `seed + " 2"`, `seed + " 3"`, … whose fold is free in the destination - book, ascending from 2. Four points that decide the behaviour and must not be re-invented: - the seed is **never re-parsed** (`"Drums 2"` colliding lands as `"Drums 2 2"`, not - `"Drums 3"` — a bare trailing integer is indistinguishable from `"Kit 808"`); the probe - **fills gaps** (first-free, not highest-plus-one, so it is a pure function of the current - name set); the probe **terminates** by pigeonhole within `B + 1` candidates for `B` banks, - so **no arbitrary cap**; and the fold is `BankBook`'s own (`bank_book.h:263-269`), reached - through the new public member, never re-implemented in `import_plan`. Sample display names - are **not** suffixed, and `slot_map` positions are untouched. -- **Always a new bank; never a merge (Ε-F2, ruled).** The import creates a bank — it never - merges into an existing one, never lands into the pool, and offers no target picker. A - pool export therefore lands as a **named** bank `"Pool 2"`, which is correct, not a glitch. - This track ships **one** action, not two; merge-into-existing is out of scope for the - phase, and move/copy already cover the after-the-fact case. -- **Birth records at landing.** Every landed file goes through - `ReaSamplerSession::recordCreated(sample, OriginKind::PackageImport)` at the same point the - `Sample` is added, in the same straight-line block, per `core/tracking/CLAUDE.md`'s - no-silent-gaps invariant. An import that lands a file without a record is the exact failure - that section exists to prevent. -- **All-or-nothing, with rollback.** Any failure after the first write deletes the files - *this call wrote* and abandons the index mutation. The rollback cites the - `prune_fs.cpp:5-11` carve-out; it does not restate it, and it does not reach outside the set - it wrote. -- **One Ctrl-Z for the index, and the file residue is stated, not implied.** The index - mutation batches through `persistBankOp` (`Undo_BeginBlock2` / `Undo_EndBlock2`, verified at - `reaper_plugin_functions.h:7758` / `:7806`). Undo does **not** un-write the files; they - remain as orphans until a prune reclaims them — the same designed window a non-empty bank - delete already produces (`core/model/CLAUDE.md`'s sample-removal section). The user-facing - summary says so. -- **`bumpBankGeneration()` on success** (`session.h:121`), so live instances reload. -- **No timeline item is placed. Ever.** -- **A new FOREVER-STABLE command id**, minted the same way T1's is. - -**Acceptance criteria.** -- `planImport` is pure and total, and every one of the four collision classes has a test that - exercises it without a filesystem: colliding ids, colliding file names, a hash already - present, and a colliding bank name. -- Importing a package built from bank B back into the project that already contains B lands - a **new** bank named `"B 2"`, with every id reminted, no entry lost, and B itself - unmutated. Importing it a third time lands `"B 3"`. -- The suffix probe is pinned by pure tests over a name set, covering at minimum: a free seed - (no suffix applied), a case/whitespace-folded collision (`"drums"` blocks `"Drums"`), a gap - (`"Drums"` + `"Drums 3"` present ⇒ `"Drums 2"`), a seed that already ends in a number - (`"Drums 2"` colliding ⇒ `"Drums 2 2"`), an absent/blank recorded name (⇒ `Imported bank`), - and a package whose source bank was the pool (⇒ `"Pool 2"`, a named bank). -- A degraded ledger (`Unreadable` and `FutureVersion`, both asserted) refuses the import with - **no picker shown, zero files written, zero index mutation**, and the message names the - channel-correct ext-state namespace. An undecodable `rsusage_*` key with an otherwise - `Loaded` ledger **does not** block — asserted, because the tempting reuse of - `blockedByTracking` would silently make it. -- An entry whose payload fails its `hashBytes` check aborts the import with **zero** files - landed and **zero** index mutation — asserted on both, since either alone would pass a - weaker test. -- A write failure injected at entry k of n leaves exactly zero files from this import on - disk and the index unchanged. -- Every landed file has a ledger birth record with kind `PackageImport`, asserted by reading - the ledger after the import, not by counting calls. -- `minReaderVersion` above the build: nothing written, message names all three facts. - `formatVersion` above the build with `minReaderVersion` at or below it: **imports cleanly**, - unknown keys skipped. Both directions asserted, in this track, against real package bytes. -- No timeline item exists after an import; the arrange is byte-identical. -- **DAW-verification obligation** (to be discharged by Daniel, not by this track): import a - package produced on another machine, confirm the panel shows every sample with its - metadata, confirm a live ReaSampler 9000 instance picks up the new bank content on the - generation bump, and confirm one Ctrl-Z removes the index entries. - -**Open questions.** **No [Daniel] questions — Ε-F2 and Ε-F3 are both RULED** (new bank -always with an auto suffix; refuse on a degraded ledger), so this track is dispatchable as -written and ships one action. **[propose at review]** whether the import summary is a console -block, a message box, or both; the recommendation is a console block plus a one-line message -box, so the detail is copyable and the outcome is unmissable. Note the refusal path is -already fixed at a console block by the Ε-F3 spec, so this call is about the *success* -summary only. +**Landed** — see `docs/COMPLETED.md` for the full narrative. New +`core/package/import_plan` (pure: the id remap table, the parent remap, the per-entry +land/skip-already-present/rename disposition, and the destination bank's display name +after `BankBook`'s own uniqueness fold — reached through a new additive +`BankBook::uniqueDisplayName` member, the only `core/model/` edit in the phase), and on +the shell side a REAPER-free `import_landing` (decode, verify every payload's +`hashBytes` digest against the manifest BEFORE the bank folder is created, then land +through the rollback journal) plus a REAPER-facing `import_bank` (the only piece +touching the extension's project state — the undo-batched persist and the generation +bump), `shell/actions/package_import_action`, the panel's `.rsbank` drop route, one +`main.cpp` row, one panel menu row, and a new `src/core/util/ascii_ws.h`. The +tracking-ledger guard runs before the file picker opens (Ε-F3, ruled: refuse outright on +`Unreadable`/`FutureVersion`, no confirm-and-proceed); the version gate runs before any +byte is written; all four collision classes — sample id, file name, content hash, bank +display name — are answered explicitly, with the display-name collision auto-suffixed +and never prompted (Ε-F2, ruled: always a new bank, never a merge); birth records land +via `recordCreated(sample, OriginKind::PackageImport)` in the same straight-line block +as the index add; the index mutation is one Ctrl-Z, and the landed files' survival as +orphans until the next prune is stated in the user-facing summary, not left implicit. +**Beyond spec:** `import_plan`'s `spelledLikeABankFile` mints a fresh name even absent a +collision, whenever the package's own entry name isn't spelled the way +`deriveBankPaths` would spell it — counted separately from a genuine folder-name +collision (`sanitizeRenameCount` vs `collisionRenameCount`) so a hostile or +foreign-spelled entry name (e.g. an unexpected extension) always lands sanitized rather +than verbatim. ---