Skip to content

test(core): golden wire-format fixtures pin VERSION_V3 in-repo#243

Open
rubenhensen wants to merge 3 commits into
mainfrom
feat/wire-format-fixtures
Open

test(core): golden wire-format fixtures pin VERSION_V3 in-repo#243
rubenhensen wants to merge 3 commits into
mainfrom
feat/wire-format-fixtures

Conversation

@rubenhensen

Copy link
Copy Markdown
Contributor

Closes #211 — the last Phase 1 artifact of EPIC #201.

Puts the wire-format tripwire inside this repo: the e2e harness already guards format drift externally (postguard-e2e fixtures-verify, proven red on a VERSION_V3 bump), but that requires the harness + a sibling checkout. This PR makes cargo test -p pg-core itself fail the moment the format changes.

  • pg-core/testdata/wire-format-v3/ — tiny golden ciphertexts in both containers (streaming = cryptify/upload, in-memory = toBytes()), sealed by the published @e4a/pg-wasm@0.6.1, committed with the vk/usk needed to unseal them forever (test keys for test data).
  • pg-core/tests/wire_format.rs — four tests:
    • wire_constants_are_pinned: the literal bytes — PRELUDE == [0x14, 0x8A, 0x8E, 0xA7], VERSION_V3 == 2, 10-byte preamble. Literals on purpose: the constants are the compatibility contract.
    • golden_fixtures_carry_the_pinned_preamble: both containers start with PRELUDE || VERSION_V3 (BE) || header-len.
    • golden_mem_fixture_unseals / golden_stream_fixture_unseals: full parse (bincode header framing) + decrypt (KEM + AEAD + signature verify) of both containers.

A red here is always a decision: fix the accidental break, or ship a deliberate new version — regenerating fixtures for the new format via the harness generator while keeping the old set decodable (old ciphertexts don't disappear from inboxes). Documented in the testdata README.

Full pg-core suite green (57 + 4 + 3).

Tiny golden ciphertexts (both containers, sealed by the published
@e4a/pg-wasm 0.6.1) committed with the key material to unseal them, plus
tests pinning the literal PRELUDE/VERSION_V3/preamble constants. A change to
consts.rs, the bincode framing, or the header layout now fails
`cargo test -p pg-core` immediately in this repo — the same tripwire the e2e
harness runs externally (postguard-e2e fixtures-verify), but without needing
a sibling checkout. Closes #211.
@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR! I'm on it — kicking off a review of the wire-format tripwire tests and golden fixtures now, and I'll follow up with a consolidated review (approve / request changes / comment) shortly.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golden wire-format tripwire — looks good, one accuracy nit.

Solid Phase-1 artifact: literal-pinned constants, both containers exercised end-to-end (parse + KEM/AEAD/signature), a std-only base64 decoder to avoid a dev-dep, and a testdata README that documents the deliberate-bump path. Test keys for test data are fine. Not blocking merge.

One nit inline: the file-level #![cfg(all(feature = "rust", feature = "stream"))] means plain cargo test -p pg-core (default features = rust only) compiles this file to 0 tests, which contradicts the README/module-doc claim that a format change "fails cargo test -p pg-core immediately". CI is safe (build.yml runs --features test,rust,stream), but a maintainer running plain cargo test -p pg-core locally to check compat gets a false green.

fn golden_fixtures_carry_the_pinned_preamble() {
for name in ["stream.bin", "mem.bin"] {
let ct = testdata(name);
assert!(ct.len() > PREAMBLE_SIZE, "{name}: too short");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file-level gate requires the stream feature, but only golden_stream_fixture_unseals needs it — wire_constants_are_pinned needs no feature and golden_mem_fixture_unseals needs only rust (the default). Since stream is not a default feature (default = ["rust"]), plain cargo test -p pg-core runs running 0 tests here, so the README/module-doc claim that a format change "fails cargo test -p pg-core immediately" is only true with the stream feature.

Two ways to make the claim hold: gate the file on #![cfg(feature = "rust")] and put #[cfg(feature = "stream")] on just the stream test — or, if the stream-feature requirement is intended, tweak the README/doc wording to say the tripwire needs --features stream. Non-blocking.

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Reviewed and posted a comment review on #243 — solid PR overall, CI-green, closes #211 as intended. Golden fixtures cover both containers end-to-end (parse + KEM/AEAD/signature verify), constants are correctly pinned, and the testdata README nicely documents the deliberate-bump path.

One non-blocking nit: wire_format.rs is gated on #![cfg(all(feature = "rust", feature = "stream"))], so a plain cargo test -p pg-core (default features) compiles it to 0 tests — which contradicts the doc claim that a format change "fails cargo test -p pg-core immediately." CI is unaffected since it runs with --features test,rust,stream, but a maintainer testing locally with defaults would get a false green. Might be worth gating the file on just feature = "rust" (with stream only on the stream-specific test), or tweaking the wording to note the tripwire needs the stream feature.

#243

The file-level stream gate meant a plain cargo test -p pg-core ran zero of
these tests, contradicting the README's 'fails immediately' claim. Now the
file gates on the default rust feature (constants, preamble, and the
in-memory unseal always run) and only the streaming-container test requires
the stream feature (still covered in CI, which tests with test,rust,stream).
@rubenhensen

Copy link
Copy Markdown
Contributor Author

Fixed, /dobby — went with your first option: the file now gates on the default rust feature and only golden_stream_fixture_unseals carries #[cfg(feature = "stream")] (with its import moved into the function). Verified both ways: plain cargo test -p pg-core runs 3 tripwires (constants, preamble, in-memory unseal — enough to trip any consts/bincode/header change), and the CI feature set runs all 4. The README's 'fails immediately' claim now holds for a default-features run.

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix, @rubenhensen! I'm on it — kicking off a review of the PR now to check correctness and rule-compliance, then I'll submit one consolidated review with a clear verdict.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the feature-gating rework is correct and does what you described.

Verified against HEAD (776f3ba):

  • Module gates on the default rust feature (#![cfg(feature = "rust")]), so wire_constants_are_pinned, golden_fixtures_carry_the_pinned_preamble, and golden_mem_fixture_unseals all run under a plain cargo test -p pg-core.
  • Only golden_stream_fixture_unseals additionally carries #[cfg(feature = "stream")], with the UnsealerStreamConfig import moved into the function — clean. CI's test,rust,stream set covers all four.
  • The README's "fails immediately" claim now holds for a default-features run.

One non-blocking style note inline (the hand-rolled base64 decoder). Not a bug — the decoder is proven correct by golden_mem_fixture_unseals. Good, focused tripwire PR.


/// Minimal std-only base64 decoder (standard alphabet, padded) — avoids a
/// dev-dependency for one meta field.
fn b64(s: &str) -> Vec<u8> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small accuracy nit, non-blocking: base64ct is already a regular (non-dev, non-optional) dependency of pg-core (Cargo.toml line 25, used for constant-time base64 in artifacts.rs), so the "avoids a dev-dependency" justification here is not quite right — nothing would be added to the dep tree.

That said, the swap is not entirely free: artifacts.rs uses the buffer-based Base64::decode(v, buf) and base64ct is pulled in without the alloc feature, so Base64::decode_vec would need features = ["alloc"] enabled (or decode into a sized buffer). Given the decoder is tiny and proven correct by the unseal test, this is genuinely optional — feel free to leave it. If you keep it, consider tweaking the comment so it does not claim base64ct is absent.

@dobby-coder

dobby-coder Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Nice, that lines up exactly with what I verified against HEAD 776f3ba — I've approved PR #243. Plain cargo test -p pg-core runs the 3 tripwires (constants, preamble, in-memory unseal), the full CI feature set runs all 4, and the README's "fails immediately" claim now checks out for a default-features run. Left one non-blocking style note on the hand-rolled b64 decoder (the "avoids a dev-dependency" comment isn't quite accurate since base64ct is already a regular dependency), but nothing blocking — good, focused tripwire PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire-format fixtures (preamble / VERSION_V3 / bincode)

1 participant