diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 7835e65..b4db0c4 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -19,7 +19,7 @@ belong to the verbs. `std::filesystem` decodes a narrow path through the runtime ANSI code page (measured `GetACP() == 1252`), so a bare `fs::path(std::string)` turns `café.rsbank` into `café.rsbank` or fails to open it. Every path a verb hands in or gets back — - including `listFolderFileNames`' results, which use `u8string()` and never + including `listFolderFileNames`' results, which go through `pathToUtf8()` and never `string()` — is UTF-8. `core/util/file_bytes` has the un-converted shape, which is why `readFilePayload` reads through this module's own `PackageFileReader` instead. - **Atomic package write, to the limit of a rename.** A package accumulates in a @@ -47,7 +47,14 @@ 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. + 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. - **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 @@ -88,7 +95,13 @@ belong to the verbs. an extension from `extension_list` when the user omits one — `pickPackageSavePath` re-appends `.rsbank` itself so the returned path is correct regardless of how that lands (the superseded Win32 path had `ofn.lpstrDefExt` for this; `GetUserFileName` - has no equivalent parameter). + has no equivalent parameter). The re-append is suffix-blind: it only skips when the + path already ends in `.rsbank`, so a path carrying a DIFFERENT extension gets + `.rsbank` appended after it (`mybank.bak` → `mybank.bak.rsbank`), unlike the + superseded `ofn.lpstrDefExt`, which appended only when the path had no extension at + all. Defensible for a format-locked export, but a real divergence from the old + picker's behavior — whoever tests the picker under `[verify — DAW]` should expect + the double-extension result on a path that already has one. - `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory when it is a full path. The verbs should seed it from the project directory — passing a bare name leaves the dialog on REAPER's process working directory, which diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index 26cd7e6..c268742 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -155,6 +155,12 @@ PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t l // so would read past a truncated allocation) and a length past the sanity // ceiling, which exists so a merely large-but-real file size can't still hand // std::vector a multi-gigabyte demand. + // + // `length > size_` short-circuits before the two branches below ever see a real + // file, so neither is reachable without a genuine >4 GiB fixture — this guard + // ships unexercised by test_package_io.cpp, which covers past-the-end, + // starts-at-the-end, and zero-length only. The ordering (cheap size check first) + // is deliberate and correct; it is not reordered to make the branch testable. constexpr std::uint64_t kMaxReadRangeBytes = std::uint64_t{4} << 30; // 4 GiB if (!ok_ || length == 0 || length > size_ || offset > size_ - length || length > kMaxReadRangeBytes || diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index a33f66e..adf60ec 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -101,7 +101,9 @@ private: }; // One source file read whole as one entry's payload — a bank file IS the streaming -// unit. Empty on any failure, per PackageFileReader. +// unit. Empty on any failure, per PackageFileReader — including a source file over +// readRange's 4 GiB ceiling, which reads as empty exactly like an unreadable file; +// fileStatus() cannot tell the two apart either, since it only checks openability. PayloadBuffer readFilePayload(const std::string& absPath); // Export must tell a missing indexed file from an unreadable one in its refusal