Make the exhaustive tests exhaustive, and stop the record overclaiming
Audit correction. The manifest test was documented -- in its own doc comment, in DECISIONS.md, and in P3's commit message -- as "exhaustive single-byte perturbation". It tried three XOR deltas per byte. The claim was false as executed, and the "guard is total" conclusion leaned on it. Both tests now do what their names say: every_single_byte_replacement_of_a_manifest_is_rejected -- each byte, each of the 255 other values. 0.15s. compression_none_rejects_a_non_zero_parameter_byte -- every one of the 255 non-zero parameter bytes, plus a round-trip of all 256 values through Zstd and Reserved, so the strictness is shown to be confined to None. And the totality claim is re-seated where it belongs: on the argument, not on a finite test. manifest_id is derived from the body, so a body edit fails the id check and an id edit fails the derivation; encode_body sorts and deduplicates every vector, so an out-of-order or duplicated encoding cannot round-trip. The test is evidence for that argument over ONE constructed manifest, and is blind to multi-byte perturbations entirely. Both the doc comment and DECISIONS.md now say so. Worth recording: restoring the leniency fails the codec test and the index test, and leaves the manifest test GREEN -- the guard rejects those bytes whatever the sub-codec does. That is not a weak test. It is the asymmetry that hid the bug, and it locks the guard rather than the codec. A suite where every test fails on every mutation would be telling us less. No codec or wire-format change; the strict branch was already correct. Gate: fmt clean, clippy 0, 30 targets / 1012 passed / 0 failed, docs 0 under -D warnings, conformance 8/8, zero golden churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7de9e479c4
commit
09a7f62802
|
|
@ -446,18 +446,28 @@ lenient, **non-injective** codec, inherited by every structure embedding a
|
|||
Its visibility depended entirely on whether the embedder had a whole-value
|
||||
re-encode guard:
|
||||
|
||||
- `Manifest::decode` **has** one, and it is *total* — verified by exhaustive
|
||||
single-byte perturbation, every one rejected. It caught this. (`encode_body`
|
||||
sorts and deduplicates every vector, and `manifest_id` is derived from the
|
||||
body, which is why the guard is complete here where `MaterializedState`'s is
|
||||
not — see `epiphany-ops/DECISIONS.md` §"Push 5 / P2".)
|
||||
- `Manifest::decode` **has** one, and it is total *by argument*: `manifest_id`
|
||||
is derived from the body, so a body edit fails the id check and an id edit
|
||||
fails the derivation; and `encode_body` sorts and deduplicates every vector,
|
||||
so an out-of-order or duplicated encoding cannot round-trip. That is what
|
||||
makes the guard complete here where `MaterializedState`'s is not (see
|
||||
`epiphany-ops/DECISIONS.md` §"Push 5 / P2"). It caught this defect.
|
||||
|
||||
The accompanying test is exhaustive over every single-byte *replacement* of
|
||||
one constructed manifest (each byte × the 255 other values) — evidence for the
|
||||
argument, not a proof of totality, and blind to multi-byte perturbations. An
|
||||
earlier revision of this record claimed "verified by exhaustive single-byte
|
||||
perturbation" while the test actually tried three XOR deltas per byte. Caught
|
||||
in review; the test now does what the sentence says, and the sentence no
|
||||
longer carries the weight of the proof.
|
||||
- `OperationIndex::decode` has **no** guard; it validates per-site instead. It
|
||||
accepted both byte strings while its own doc promised to *"reject (never
|
||||
normalizing) any non-canonical form"*. That promise was false.
|
||||
|
||||
Fixed at the source rather than papered over at the index: a non-zero `None`
|
||||
parameter is now rejected. Exhaustive sweep (every byte × every value, plus an
|
||||
8-byte extreme-integer window) finds no remaining non-injective site.
|
||||
parameter is now rejected, for every one of the 255 non-zero values. A sweep of
|
||||
one `OperationIndex` payload (every byte × every value, plus an 8-byte
|
||||
extreme-integer window) finds no remaining non-injective site in it.
|
||||
|
||||
**This contradicted ratified spec text**, which said the byte was *"present but
|
||||
zero, and ignored on read"*. Escalated rather than fixed unilaterally; the user
|
||||
|
|
@ -474,8 +484,12 @@ index corpus was built from real `OperationIndex::build` output. The smoke tests
|
|||
now assert on a `WireFuzzCoverage` so that can never silently regress. 1.5M
|
||||
inputs across five seeds, ~1s each, clean after the fix.
|
||||
|
||||
Three regressions, each mutation-verified by restoring the leniency:
|
||||
`compression_none_rejects_a_non_zero_parameter_byte` (the codec),
|
||||
`a_lenient_compression_byte_is_rejected_rather_than_normalized` (the index, the
|
||||
surface that exposed it), and `every_single_byte_perturbation_of_a_manifest_is_rejected`
|
||||
(the guard's totality, and the asymmetry that hid the bug).
|
||||
Three regressions. Restoring the leniency fails exactly two of them:
|
||||
`compression_none_rejects_a_non_zero_parameter_byte` (the codec) and
|
||||
`a_lenient_compression_byte_is_rejected_rather_than_normalized` (the index —
|
||||
the surface that exposed it). The third,
|
||||
`every_single_byte_replacement_of_a_manifest_is_rejected`, stays **green** under
|
||||
that mutation, because the guard rejects the bytes whatever the sub-codec does.
|
||||
That is not a weak test; it is the asymmetry, and it locks the guard rather than
|
||||
the codec. A regression suite where every test fails on every mutation would be
|
||||
telling us less, not more.
|
||||
|
|
|
|||
|
|
@ -327,26 +327,30 @@ mod tests {
|
|||
CompressionAlgorithm::None
|
||||
);
|
||||
|
||||
for param in [1u8, 0x7F, 0xFF] {
|
||||
let lenient = vec![0, param];
|
||||
// Exhaustive: every non-zero parameter byte, not three representatives.
|
||||
for param in 1u16..=255 {
|
||||
let lenient = vec![0, param as u8];
|
||||
assert!(
|
||||
CompressionAlgorithm::decode(&mut Reader::new(&lenient)).is_err(),
|
||||
"None with parameter {param:#04x} must be rejected, never normalized to zero"
|
||||
);
|
||||
}
|
||||
|
||||
// The parameter is meaningful for the other two, so it round-trips.
|
||||
for algo in [
|
||||
CompressionAlgorithm::Zstd { level: 0xFF },
|
||||
CompressionAlgorithm::Reserved(0xFF),
|
||||
] {
|
||||
let mut w = Writer::new();
|
||||
algo.encode(&mut w);
|
||||
let bytes = w.into_bytes();
|
||||
assert_eq!(
|
||||
CompressionAlgorithm::decode(&mut Reader::new(&bytes)).unwrap(),
|
||||
algo
|
||||
);
|
||||
// And the parameter round-trips for every value of the two variants that
|
||||
// give it meaning, so the strictness is confined to `None`.
|
||||
for param in 0u16..=255 {
|
||||
for algo in [
|
||||
CompressionAlgorithm::Zstd { level: param as u8 },
|
||||
CompressionAlgorithm::Reserved(param as u8),
|
||||
] {
|
||||
let mut w = Writer::new();
|
||||
algo.encode(&mut w);
|
||||
let bytes = w.into_bytes();
|
||||
assert_eq!(
|
||||
CompressionAlgorithm::decode(&mut Reader::new(&bytes)).unwrap(),
|
||||
algo
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -709,19 +709,24 @@ mod tests {
|
|||
use crate::chunk::{chunk_id, ChunkKind};
|
||||
use crate::ids::SnapshotId;
|
||||
|
||||
/// The manifest's whole-value re-encode guard is **total**: every single-byte
|
||||
/// perturbation is rejected. `manifest_id` is derived from the body, so a
|
||||
/// body edit fails the id check and an id edit fails the derivation; and
|
||||
/// `encode_body` sorts and deduplicates every vector, so an out-of-order
|
||||
/// encoding cannot round-trip either.
|
||||
/// Exhaustive over **every single-byte replacement of this constructed
|
||||
/// manifest**: each of its bytes, each of the 255 other values. All rejected.
|
||||
///
|
||||
/// This is what makes the guard complete *here* and not in
|
||||
/// That is a finite check, not a proof of totality. Totality rests on the
|
||||
/// *argument*: `manifest_id` is derived from the body, so a body edit fails
|
||||
/// the id check and an id edit fails the derivation; and `encode_body` sorts
|
||||
/// and deduplicates every vector, so an out-of-order or duplicated encoding
|
||||
/// cannot round-trip. The test is evidence for that argument over one
|
||||
/// manifest, not a substitute for it — and multi-byte perturbations are out
|
||||
/// of its reach entirely.
|
||||
///
|
||||
/// The argument is what makes the guard complete *here* and not in
|
||||
/// `MaterializedState` (epiphany-ops), whose encoder writes its `Vec` fields
|
||||
/// verbatim, nor in `OperationIndex`, which has no guard at all. The lenient
|
||||
/// `CompressionAlgorithm::None` parameter byte was invisible through the
|
||||
/// manifest for exactly this reason, and visible through the index.
|
||||
#[test]
|
||||
fn every_single_byte_perturbation_of_a_manifest_is_rejected() {
|
||||
fn every_single_byte_replacement_of_a_manifest_is_rejected() {
|
||||
use crate::chunk::{ChunkRef, CompressionAlgorithm};
|
||||
use crate::ids::SchemaVersion;
|
||||
|
||||
|
|
@ -743,16 +748,19 @@ mod tests {
|
|||
assert_eq!(Manifest::decode(&bytes).unwrap().encode(), bytes);
|
||||
|
||||
for i in 0..bytes.len() {
|
||||
for delta in [1u8, 0x7F, 0xFF] {
|
||||
let mut b = bytes.clone();
|
||||
b[i] ^= delta;
|
||||
if b == bytes {
|
||||
let original = bytes[i];
|
||||
for value in 0u16..=255 {
|
||||
let value = value as u8;
|
||||
if value == original {
|
||||
continue;
|
||||
}
|
||||
let mut b = bytes.clone();
|
||||
b[i] = value;
|
||||
match Manifest::decode(&b) {
|
||||
Err(_) => {}
|
||||
Ok(decoded) => panic!(
|
||||
"byte {i} ^ {delta:#04x} was accepted; re-encode matches input: {}",
|
||||
"byte {i} = {value:#04x} (was {original:#04x}) was accepted; \
|
||||
re-encode matches input: {}",
|
||||
decoded.encode() == b
|
||||
),
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue