Remediate Ε-W3-T1 package-compat-fixtures review findings

Freeze *.rsbank as binary via .gitattributes; add a truncated additive_forward fixture proving the exact-size proof beats TooNew; enumerate the fixture dir to catch orphaned files; make fixture-size checks fatal instead of just logged; pin fixture version asserts as literals, not build-relative.
This commit is contained in:
2026-08-02 15:39:04 -04:00
parent 9521b5339f
commit 3fd3214ff8
8 changed files with 173 additions and 61 deletions
+28 -11
View File
@@ -12,20 +12,29 @@ The reason is the whole point of the corpus. These bytes exist to catch a format
that quietly breaks a compatibility direction. A fixture regenerated by the build that
broke it agrees with that build by construction and catches nothing — which is exactly
the failure mode a version ladder exists to prevent. The same argument forbids a test
that builds its own fixture at run time.
that builds its own fixture at run time. The repo-root `.gitattributes` (`*.rsbank
binary`) keeps this mechanical: without it, git's NUL-sniffing heuristic could
text-classify a future short/ASCII fixture and CRLF-mangle a line ending on a Windows
checkout, silently breaking the frozen-bytes premise.
If a fixture stops decoding, the answer is never to re-cut the fixture. Either the format
change was structural (bump `minReaderVersion`, add a new fixture, and leave the old one
asserting the refusal) or it is a regression.
A fixture's BYTES are frozen forever; a fixture's ASSERTION is not. `additive_forward.rsbank`
and `refuse_structural.rsbank` carry version pairs one step past THIS build's ladder (2/1
and 2/2). When a future build's own `kPackageFormatVersion` reaches 2, `refuse_structural.rsbank`
classifies `Readable` under the new ladder — its bytes never claimed to need more than
format 2 — so that build re-aims the assertion (and adds a new synthetic pair one step
past the NEW ladder); it never re-cuts the fixture. If a truncation or hostile-name
fixture ever changes classification, that is a regression, never a ladder consequence.
## Provenance
`v1_shipping.rsbank` was produced by running this repo's own export verb (`exportBank`)
at version **1.4.0** over a one-sample bank, and copying the emitted file here verbatim.
Every other fixture is derived from those bytes: the truncations are prefixes of them,
and the synthetic packages reuse their manifest region under different version integers
or a hand-written hostile manifest (the encoder refuses to write one, which is why those
could not come from the verb).
Every other fixture is derived from those bytes: eight of the nine truncations are
prefixes of `v1_shipping.rsbank` (the ninth, `trunc_additive_forward.rsbank`, is a prefix
of `additive_forward.rsbank` itself — a prefix of a prefix, still frozen bytes, never
regenerated), and the synthetic packages reuse their manifest region under different
version integers or a hand-written hostile manifest (the encoder refuses to write one,
which is why those could not come from the verb).
Payloads are one 300-byte 16-bit mono WAV. The properties under test are structural —
version integers, framing arithmetic, name validation — so a larger payload proves
@@ -46,9 +55,10 @@ build, exactly as this one was, and recording the build's version here.
### Truncation — one file per distinct decode failure site
Each is a prefix of `v1_shipping.rsbank` (907-byte prefix + 300-byte payload = 1207
bytes). All classify `Malformed`; none may classify `TooNew`, since "install a newer
build" does not fix a partial download.
The first eight are prefixes of `v1_shipping.rsbank` (907-byte prefix + 300-byte payload
= 1207 bytes), so `formatVersion` never exceeds this build's on that path. All classify
`Malformed`; none may classify `TooNew`, since "install a newer build" does not fix a
partial download.
| File | Bytes | Site the cut lands in |
|---|---|---|
@@ -61,6 +71,13 @@ build" does not fix a partial download.
| `trunc_payload_middle.rsbank` | 1057 | Inside the first payload. |
| `trunc_one_short.rsbank` | 1206 | One byte short of the total. |
`trunc_additive_forward.rsbank` is the ninth: a 983-byte prefix of `additive_forward.rsbank`
(25-byte frozen header/manifest-length region + 958-byte manifest = 983), cut exactly at
ITS payload boundary. `formatVersion` here is 2 — one past this build's — so this is the
one truncation that proves the exact-size-proof failure stays `Malformed` even when
`formatVersion > kPackageFormatVersion`, rather than relabeling to `TooNew` (the parse
branch is the only one that relabels — see `src/core/package/CLAUDE.md`).
### Hostile names — refused at decode, before any planner
The two naming fields carry different rules (`src/core/package/CLAUDE.md`), so each
Binary file not shown.
+84 -16
View File
@@ -10,8 +10,10 @@
#include "../src/core/package/bank_package.h"
#include <algorithm>
#include <cstdint>
#include <cstdio>
#include <filesystem>
#include <string>
#include <vector>
@@ -80,15 +82,18 @@ static PackageManifest expectedV1Manifest() {
// A fixture that failed to open reads as an empty buffer, and an empty buffer decodes
// Malformed — which would let half this file pass vacuously. Every suite loads through
// here.
static std::vector<std::uint8_t> load(const char* name, std::size_t expectedSize) {
std::vector<std::uint8_t> bytes = packageFixtureBytes(name);
if (bytes.size() != expectedSize) {
// here. A size mismatch is fatal to the caller (false), not just recorded: proceeding
// with a short or empty buffer would let classifyFixture/le32At index out of range and
// the TooNew test's hand-lifted manifest slice overread the heap, rather than fail clean.
static bool load(const char* name, std::size_t expectedSize, std::vector<std::uint8_t>& out) {
out = packageFixtureBytes(name);
if (out.size() != expectedSize) {
std::printf("FAIL: fixture %s is %zu bytes, expected %zu (path: %s)\n", name,
bytes.size(), expectedSize, packageFixturePath(name).c_str());
out.size(), expectedSize, packageFixturePath(name).c_str());
++g_fail;
return false;
}
return bytes;
return true;
}
static bool containsToken(const std::vector<std::uint8_t>& bytes, const std::string& token) {
@@ -112,7 +117,8 @@ static PackageReadability classifyFixture(const std::vector<std::uint8_t>& bytes
}
static void testV1FixtureDecodesToTheRecordTheShippingBuildWrote() {
const std::vector<std::uint8_t> bytes = load(kV1File, kV1TotalSize);
std::vector<std::uint8_t> bytes;
if (!load(kV1File, kV1TotalSize, bytes)) return;
CHECK(classifyFixture(bytes) == PackageReadability::Readable);
const DecodedPackage dec = decodePackage(bytes, bytes.size());
@@ -147,7 +153,8 @@ static void testV1FixtureDecodesToTheRecordTheShippingBuildWrote() {
// root, one on the entry, one inside the nested Sample blob — and must still decode to
// exactly what the v1 fixture decodes to.
static void testAdditiveForwardFixtureReadsWithEveryKnownFieldIntact() {
const std::vector<std::uint8_t> bytes = load("additive_forward.rsbank", 1283);
std::vector<std::uint8_t> bytes;
if (!load("additive_forward.rsbank", 1283, bytes)) return;
// Non-vacuity: the unknown keys are genuinely in the bytes, so the equality below
// is "skipped without error", not "there was nothing to skip".
@@ -159,8 +166,10 @@ static void testAdditiveForwardFixtureReadsWithEveryKnownFieldIntact() {
const DecodedPackage dec = decodePackage(bytes, bytes.size());
CHECK(dec.status == PackageReadability::Readable);
CHECK(dec.header.formatVersion == kPackageFormatVersion + 1);
CHECK(dec.header.minReaderVersion == kPackageMinReaderVersion);
// Literal, not kPackageFormatVersion-relative: these are the FIXTURE's frozen
// version pair (2/1), not this build's — see the file header note.
CHECK(dec.header.formatVersion == 2);
CHECK(dec.header.minReaderVersion == 1);
CHECK(dec.header.writerVersion == "1.9.0");
// Every known field, end to end: same manifest the v1 fixture yields.
@@ -172,14 +181,16 @@ static void testAdditiveForwardFixtureReadsWithEveryKnownFieldIntact() {
// --- direction 2: a structural newer writer is refused whole -----------------
static void testRefuseFixtureIsTooNewAndStillNamesTheWriter() {
const std::vector<std::uint8_t> bytes = load("refuse_structural.rsbank", kV1TotalSize);
std::vector<std::uint8_t> bytes;
if (!load("refuse_structural.rsbank", kV1TotalSize, bytes)) return;
CHECK(classifyFixture(bytes) == PackageReadability::TooNew);
const DecodedPackage dec = decodePackage(bytes, bytes.size());
CHECK(dec.status == PackageReadability::TooNew);
// The three facts the refusal message owes the user.
CHECK(dec.header.formatVersion == kPackageFormatVersion + 1);
CHECK(dec.header.minReaderVersion == kPackageFormatVersion + 1);
// The three facts the refusal message owes the user. Literal, not
// kPackageFormatVersion-relative — this is the FIXTURE's frozen pair (2/2).
CHECK(dec.header.formatVersion == 2);
CHECK(dec.header.minReaderVersion == 2);
CHECK(dec.header.writerVersion == "1.9.0");
// Nothing else: no manifest, no layout, no partial success.
@@ -223,7 +234,8 @@ static const Truncation kTruncations[] = {
static void testEveryTruncationIsMalformedNeverTooNew() {
for (const Truncation& t : kTruncations) {
const std::vector<std::uint8_t> bytes = load(t.file, t.size);
std::vector<std::uint8_t> bytes;
if (!load(t.file, t.size, bytes)) continue;
const DecodedPackage dec = decodePackage(bytes, bytes.size());
if (dec.status != PackageReadability::Malformed) {
std::printf("FAIL: %s (%s) classified %s, expected Malformed\n", t.file, t.site,
@@ -247,6 +259,27 @@ static void testEveryTruncationIsMalformedNeverTooNew() {
}
}
// --- truncation: the additive-forward ladder direction, not just v1 ---------
// Every truncation above is a prefix of v1_shipping.rsbank (formatVersion == ours), so
// none of them ever puts formatVersion > kPackageFormatVersion on the exact-size-proof
// failure path — the one relabeling branch (src/core/package/CLAUDE.md: "the parse
// branch is the ONLY one that relabels") never gets exercised from the TooNew side.
// This fixture is additive_forward.rsbank (formatVersion 2, one past ours) cut exactly
// at its manifest/payload boundary: the manifest parses whole, so the failure is the
// exact-size proof, not a parse failure — it must stay Malformed, not relabel to TooNew.
static void testAdditiveForwardTruncatedAtPayloadBoundaryStaysMalformed() {
std::vector<std::uint8_t> bytes;
if (!load("trunc_additive_forward.rsbank", 983, bytes)) return;
CHECK(classifyFixture(bytes) == PackageReadability::Readable);
const DecodedPackage dec = decodePackage(bytes, bytes.size());
CHECK(dec.status == PackageReadability::Malformed);
CHECK(dec.manifest.entries.empty());
CHECK(dec.layout.empty());
CHECK(dec.prefixSize == 0);
}
// --- hostile names: refused at decode, before any planner exists -------------
// The two fields carry DIFFERENT rules (src/core/package/CLAUDE.md): the entry name may
@@ -292,7 +325,8 @@ static void testEveryHostileNameIsRefusedAtDecode() {
CHECK(isValidNestedSamplePath("kick.wav"));
for (const HostileFixture& h : kHostiles) {
const std::vector<std::uint8_t> bytes = load(h.file, h.size);
std::vector<std::uint8_t> bytes;
if (!load(h.file, h.size, bytes)) continue;
if (!containsToken(bytes, jsonEscaped(h.offending))) {
std::printf("FAIL: %s does not carry the form it is named for (%s)\n", h.file,
h.offending);
@@ -318,12 +352,46 @@ static void testEveryHostileNameIsRefusedAtDecode() {
}
}
// --- inventory: every fixture on disk is exercised by SOME test above --------
// This TU's own tables (the three named fixtures plus kTruncations and kHostiles) are
// the fullest account of the corpus in the tree — every other consumer (the round-trip
// harness, the README) tests a subset of these same files. Enumerating the fixture
// directory here and failing on anything absent from this list is the one check that
// catches a fixture added to disk but never wired into a table: a silent coverage drop
// that would otherwise leave a green suite.
static void testEveryFixtureOnDiskIsInSomeTable() {
std::vector<std::string> known = {kV1File, "additive_forward.rsbank",
"refuse_structural.rsbank",
"trunc_additive_forward.rsbank"};
for (const Truncation& t : kTruncations) known.push_back(t.file);
for (const HostileFixture& h : kHostiles) known.push_back(h.file);
std::error_code ec;
for (const auto& entry :
std::filesystem::directory_iterator(REASAMPLER_PACKAGE_FIXTURE_DIR, ec)) {
if (entry.path().extension() != ".rsbank") continue;
const std::string name = entry.path().filename().string();
if (std::find(known.begin(), known.end(), name) == known.end()) {
std::printf("FAIL: fixture %s exists on disk but is in no table in this file\n",
name.c_str());
++g_fail;
}
}
if (ec) {
std::printf("FAIL: could not list fixture directory: %s\n", ec.message().c_str());
++g_fail;
}
}
int main() {
testV1FixtureDecodesToTheRecordTheShippingBuildWrote();
testAdditiveForwardFixtureReadsWithEveryKnownFieldIntact();
testRefuseFixtureIsTooNewAndStillNamesTheWriter();
testEveryTruncationIsMalformedNeverTooNew();
testAdditiveForwardTruncatedAtPayloadBoundaryStaysMalformed();
testEveryHostileNameIsRefusedAtDecode();
testEveryFixtureOnDiskIsInSomeTable();
if (g_fail == 0) {
std::printf("package_compat_tests: all passed\n");
+57 -32
View File
@@ -67,13 +67,16 @@ static void writeBytes(const std::string& path, const std::vector<std::uint8_t>&
static_cast<std::streamsize>(bytes.size()));
}
// Copies a committed fixture to the path the verb will be pointed at. Fails loudly on an
// empty read: an unreadable corpus would otherwise let every "must refuse" suite pass.
static bool stageFixture(const Scratch& scratch, const char* fixture) {
// Copies a committed fixture to the path the verb will be pointed at. Fails loudly on a
// size mismatch (empty included): an unreadable OR mangled corpus would otherwise let
// every "must refuse" suite pass, since a garbled fixture still refuses, just not for
// the reason under test.
static bool stageFixture(const Scratch& scratch, const char* fixture,
std::size_t expectedSize) {
const std::vector<std::uint8_t> bytes = packageFixtureBytes(fixture);
if (bytes.empty()) {
std::printf("FAIL: fixture %s read as 0 bytes (path: %s)\n", fixture,
packageFixturePath(fixture).c_str());
if (bytes.size() != expectedSize) {
std::printf("FAIL: fixture %s is %zu bytes, expected %zu (path: %s)\n", fixture,
bytes.size(), expectedSize, packageFixturePath(fixture).c_str());
++g_fail;
return false;
}
@@ -116,7 +119,7 @@ static ImportLanding runImport(const Scratch& scratch, ReaSamplerSession& sessio
// importing and re-exporting it closes export -> import -> export over frozen bytes.
static void testV1FixtureReExportsByteIdenticalPayloads() {
Scratch scratch("roundtrip");
if (!stageFixture(scratch, "v1_shipping.rsbank")) return;
if (!stageFixture(scratch, "v1_shipping.rsbank", 1207)) return;
ReaSamplerSession session;
int births = 0;
@@ -128,7 +131,7 @@ static void testV1FixtureReExportsByteIdenticalPayloads() {
const std::vector<std::vector<std::uint8_t>> sourcePayloads =
payloadsOf(packageFixtureBytes("v1_shipping.rsbank"));
CHECK(sourcePayloads.size() == 1);
if (sourcePayloads.size() != 1) { CHECK(false); return; }
// The landed file is the package's payload verbatim — the first half of the claim.
const std::string landed = scratch.bankDir() + "/" + landing.plan.entries[0].destFileName;
@@ -153,7 +156,7 @@ static void testV1FixtureReExportsByteIdenticalPayloads() {
static void testRefuseFixtureRefusesTheWholeImportAndNamesTheWriter() {
Scratch scratch("refuse");
if (!stageFixture(scratch, "refuse_structural.rsbank")) return;
if (!stageFixture(scratch, "refuse_structural.rsbank", 1207)) return;
ReaSamplerSession session;
const BankBook before = session.book();
@@ -162,8 +165,10 @@ static void testRefuseFixtureRefusesTheWholeImportAndNamesTheWriter() {
session.book(), kTag, journal);
CHECK(landing.outcome == ImportOutcome::TooNew);
CHECK(landing.header.formatVersion == package::kPackageFormatVersion + 1);
CHECK(landing.header.minReaderVersion == package::kPackageFormatVersion + 1);
// Literal, not kPackageFormatVersion-relative: this is the FIXTURE's frozen pair
// (2/2), not this build's (see test_package_compat.cpp's file header note).
CHECK(landing.header.formatVersion == 2);
CHECK(landing.header.minReaderVersion == 2);
CHECK(landing.header.writerVersion == "1.9.0");
// Nothing planned, nothing on disk, nothing in the index.
CHECK(landing.plan.entries.empty());
@@ -173,18 +178,27 @@ static void testRefuseFixtureRefusesTheWholeImportAndNamesTheWriter() {
// The same eight cuts test_package_compat classifies, driven through the verb's
// incremental prefix reader — the one caller that can ask requiredPrefixSize for more
// bytes than the file holds.
static const char* kTruncationFixtures[] = {
"trunc_magic.rsbank", "trunc_version_pair.rsbank",
"trunc_writer_semver.rsbank", "trunc_manifest_length.rsbank",
"trunc_manifest_body.rsbank", "trunc_payload_start.rsbank",
"trunc_payload_middle.rsbank", "trunc_one_short.rsbank",
// bytes than the file holds. Sizes match test_package_compat.cpp's kTruncations.
struct TruncationFixture {
const char* file;
std::size_t size;
};
static const TruncationFixture kTruncationFixtures[] = {
{"trunc_magic.rsbank", 2},
{"trunc_version_pair.rsbank", 10},
{"trunc_writer_semver.rsbank", 18},
{"trunc_manifest_length.rsbank", 23},
{"trunc_manifest_body.rsbank", 466},
{"trunc_payload_start.rsbank", 907},
{"trunc_payload_middle.rsbank", 1057},
{"trunc_one_short.rsbank", 1206},
};
static void testEveryTruncationRefusesTheImportAsMalformed() {
for (const char* fixture : kTruncationFixtures) {
Scratch scratch(std::string("trunc_") + fixture);
if (!stageFixture(scratch, fixture)) continue;
for (const TruncationFixture& fixture : kTruncationFixtures) {
Scratch scratch(std::string("trunc_") + fixture.file);
if (!stageFixture(scratch, fixture.file, fixture.size)) continue;
ReaSamplerSession session;
const BankBook before = session.book();
@@ -192,7 +206,7 @@ static void testEveryTruncationRefusesTheImportAsMalformed() {
const ImportLanding landing = landPackage(scratch.importPath(), scratch.projectDir(),
session.book(), kTag, journal);
if (landing.outcome != ImportOutcome::Malformed) {
std::printf("FAIL: %s imported as outcome %d, expected Malformed\n", fixture,
std::printf("FAIL: %s imported as outcome %d, expected Malformed\n", fixture.file,
static_cast<int>(landing.outcome));
++g_fail;
}
@@ -202,19 +216,30 @@ static void testEveryTruncationRefusesTheImportAsMalformed() {
}
}
static const char* kHostileFixtures[] = {
"hostile_name_dotdot.rsbank", "hostile_name_parent_slash.rsbank",
"hostile_name_parent_backslash.rsbank", "hostile_name_subdir_slash.rsbank",
"hostile_name_drive_absolute.rsbank", "hostile_name_unc_absolute.rsbank",
"hostile_path_dotdot_slash.rsbank", "hostile_path_dotdot_backslash.rsbank",
"hostile_path_rooted.rsbank", "hostile_path_drive_absolute.rsbank",
"hostile_path_unc_absolute.rsbank",
// Sizes match test_package_compat.cpp's kHostiles.
struct HostileFixtureFile {
const char* file;
std::size_t size;
};
static const HostileFixtureFile kHostileFixtures[] = {
{"hostile_name_dotdot.rsbank", 624},
{"hostile_name_parent_slash.rsbank", 633},
{"hostile_name_parent_backslash.rsbank", 634},
{"hostile_name_subdir_slash.rsbank", 634},
{"hostile_name_drive_absolute.rsbank", 643},
{"hostile_name_unc_absolute.rsbank", 646},
{"hostile_path_dotdot_slash.rsbank", 641},
{"hostile_path_dotdot_backslash.rsbank", 640},
{"hostile_path_rooted.rsbank", 635},
{"hostile_path_drive_absolute.rsbank", 643},
{"hostile_path_unc_absolute.rsbank", 646},
};
static void testEveryHostileNameIsRefusedBeforeThePlannerRuns() {
for (const char* fixture : kHostileFixtures) {
Scratch scratch(std::string("hostile_") + fixture);
if (!stageFixture(scratch, fixture)) continue;
for (const HostileFixtureFile& fixture : kHostileFixtures) {
Scratch scratch(std::string("hostile_") + fixture.file);
if (!stageFixture(scratch, fixture.file, fixture.size)) continue;
ReaSamplerSession session;
const BankBook before = session.book();
@@ -222,7 +247,7 @@ static void testEveryHostileNameIsRefusedBeforeThePlannerRuns() {
const ImportLanding landing = landPackage(scratch.importPath(), scratch.projectDir(),
session.book(), kTag, journal);
if (landing.outcome != ImportOutcome::Malformed) {
std::printf("FAIL: %s imported as outcome %d, expected Malformed\n", fixture,
std::printf("FAIL: %s imported as outcome %d, expected Malformed\n", fixture.file,
static_cast<int>(landing.outcome));
++g_fail;
}