test(compat): HEAD sample sealer plus published pg-core reader crate - #265
Conversation
Rust half of the bidirectional wire-compat gate (#251): today only committed old fixtures are read by a new reader, so a "believed-additive" wire change ships green and breaks SDKs pinned in the field. - pg-core/examples/seal-samples: seals a deterministic sample set with HEAD in both container modes, plus a private-signing and a multi-segment case. Everything derives from one seed, so two runs of the same tree are byte-identical and a diff means the format moved. - pg-core/tests/sample_sealer.rs: holds the sealer to that in normal CI -- determinism, manifest/file agreement, and a HEAD read-back. - pg-compat: a crate deliberately outside the workspace, depending on published pg-core =0.6.1 from crates.io, that opens the sealed set. The version list is the support window from #252; adding one is a manifest line and a reader! invocation. The wire-compat-rust job that runs the two together is not in this commit (the App cannot push .github/workflows); the YAML is a PR comment. Refs #260, #251, #247
|
There was a problem hiding this comment.
VERDICT: request-changes
Rules sweep plus the consolidated review, cycle 1. Fourteen rules applied (the Rust, CI-workflow, patch-in-comment, PR-process and prose sets); all pass. The proposed YAML matches path-filtered-job-cannot-be-a-required-check: step filter rather than an on: paths: key, the workflow file itself included in the filter, and dorny/paths-filter flagged as new third-party with the drop-the-filter alternative offered. The CLAUDE.md wording matches patch-in-comment-gate-is-not-live-yet by naming the job as not yet a gate and saying where the patch lives. Closes #260 is a firing keyword. CI is green on all 22 checks with nothing in flight.
One blocking issue, in two places. It is a documentation-accuracy problem, not a hole in the gate.
The gate does not produce the diagnostic three files promise
Reproduced on this HEAD. Added pub injected: u32 as the first field of Header (pg-core/src/client/header.rs:104), set it in the single constructor at header.rs:167, resealed, ran the gate:
running 1 test
memory allocation of 21474836480 bytes failed
error: test failed, to rerun pass `--test wire_compat`
Caused by:
process didn't exit successfully: ... (signal: 6, SIGABRT: process abort signal)
The shifted bytes make bincode read a garbage length prefix, Unsealer::new attempts a 20 GiB allocation, and the process aborts on the first case. The failures: Vec<String> accumulator in pg-compat/src/lib.rs never gets to run, so there is no per-case list and no invalid length 0, expected struct Header with 3 elements. The run also left a 3.1 MB core in pg-compat/, which the root .gitignore does not cover: it ignores target, not core, and git check-ignore confirms the file is untracked and unignored.
The gate still goes red, which is the property that matters. What is wrong is that pg-compat/README.md:131, CLAUDE.md:9 and the PR body's verification table each describe an outcome the gate does not deliver for exactly the breakage class it exists to catch. CLAUDE.md is auto-loaded agent context, so a later run reads "fails at header decode", meets an allocation abort, and has no reason to connect the two.
Either correct the three claims, or isolate each case in a subprocess so one abort does not take the whole run with it. catch_unwind will not help, because an abort is not a panic. Correcting the wording is cheaper and it is the honest option. Add core to .gitignore either way.
Not blocking
Four nits inline. Plus one thing that is not this PR's:
Unsealer::<Vec<u8>, UnsealerMemoryConfig>::new aborting the process on a malformed 1231-byte header that clears the preamble_checked length bounds is pre-existing on main, not introduced here, and it deserves its own issue. CLAUDE.md lists malformed-ciphertext panics as a pitfall class with fixes already shipped; this is a surviving instance, and an abort is worse than a panic because it cannot be contained. It matters for pg-pkg, which parses attacker-supplied ciphertext. GitHub cannot anchor an inline comment to header.rs because the file is not in this diff, so the note lives here.
Staying in draft until the fix lands.
A published reader handed a shifted header does not return an error. It reads a garbage length prefix, attempts a ~20 GiB allocation and the process aborts, so the first broken case took the whole run with it and the gate printed no per-case list at all. An abort is not a panic, so catch_unwind cannot contain it; the case has to be its own process. pg-compat now builds a pg-compat-case binary that opens one case with one published pg-core, and the gate spawns one per case. A case that dies is reported by name with the signal and the child's last stderr line, and the remaining cases still run. Children run under target/ so an abort's core dump stays out of the working tree; core is gitignored as well. The README, CLAUDE.md and the sealer are corrected to match: they claimed a header decode error, which is not what the gate produces. Also from review: - stream-privsig case. The stream arm gated with_priv_signing_key on case.private_signing and no stream case set it, so the branch was dead and a break confined to the payload trailer passed green. - Plaintext mismatches name the first differing offset. A key-schedule or nonce-derivation change corrupts content while preserving length, and the old message printed only the two equal lengths. - The manifest contract list documents sender and sealedBy, which #261 needs to check header signatures against. - A note on the artifact hand-off: wire-compat-rust uploads nothing on a PR that misses the path filter, so an unconditional download in #261's job fails rather than skips. Refs #260
The guard moved into the parent test when each case gained its own process, so the hand-run path documented in the README lost it: a version bump reached published pg-core as shifted bytes and came back as an allocation abort instead of the one line naming the mismatch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It does carry sender.public.ts and sender.private.ts, which a JS reader (#261) needs because they are part of the policy the header signature is computed over. Determinism holds because the value is a constant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kernel.core_pattern is core.%p on many hosts, so a hand-run abort drops a file the bare core entry misses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
VERDICT: approve
Rules sweep plus the consolidated review, cycle 2. The cycle-1 blocker is fixed at the root rather than papered over in the docs: each case now runs in its own child process, so a first-position Header field produces a five-line list naming every case instead of one SIGABRT that ends the run. I re-ran that repro on this branch and got the list.
Rules checked against the delta since 68086ae: the Rust set, the CI-workflow set, patch-in-comment, the PR-process set and the prose set. All pass. Three specific ones worth recording, since they are the ones that usually bite:
patch-in-comment-gate-is-not-live-yet: the CLAUDE.md bullet still says thewire-compat-rustjob is not yet a gate and names the comment it lives in. The README's new section calls it "the proposed job". Neither claims a gate that does not exist.actions-default-shell-has-no-pipefail: norun:block in the proposed YAML contains a shell pipe, and the two multi-line ones declareshell: bashanyway. GitHub cannot anchor an inline comment to a file outside the diff, so the YAML review lives here.- The YAML still drives the code correctly now that
pg-compathas a second binary target.cargo test --manifest-path pg-compat/Cargo.toml --lockedbuildspg-compat-casebefore the integration test looks it up throughCARGO_BIN_EXE_pg-compat-case, so the job needs no edit.
No bugs. Three nits from the review, all fixed on the branch rather than sent back for another cycle.
Fixed here
fix(compat): check the wire version in the per-case child too (51ff1f6). The wire_version guard moved into the parent when cases gained their own processes, and the child had no equivalent, so the hand-run path in README:36 lost it. Verified both directions: with wireVersion forced to 3 in a copied set, pg-compat-case ... 0.6.1 mem now prints pg-core 0.6.1: sample set claims wire version 3, this reader speaks 2 and exits 1, where before it handed the bytes to 0.6.1. The parent still reports the bump once per reader, not once per case.
docs(compat): the manifest carries no wall-clock timestamp (0c09dbb). The determinism section said the manifest carries no timestamp, but the schema block 70 lines above it shows sender.public.ts and sender.private.ts, both 1704067200. Those values are part of the policy the header signature is computed over, so #261 has to read them. The determinism claim itself is intact, because the value is a constant in sample_set.rs, not a clock.
chore: ignore core.<pid> dumps as well as core (3779735). kernel.core_pattern is core.%p on many hosts, so the bare entry missed the file on exactly the hand-run it was added for.
Verified locally on 3779735
cargo fmt and cargo clippy -- -D warnings on pg-compat (outside the workspace, so no CI matrix covers it), the sealer, and the full gate: 6 unit tests plus the integration test, green. All 21 CI checks green, nothing in flight.
Not this PR
Carried forward from cycle 1 and still worth its own issue: Unsealer::<Vec<u8>, UnsealerMemoryConfig>::new aborts the process rather than returning Err on a malformed header that clears the preamble length bounds. That is pre-existing on main, and it matters for pg-pkg, which parses ciphertext it did not produce.
Flipping to ready.
…ATIBILITY.md Review follow-ups on the Node half of the wire-compat gate. An error raised before the first reader call left `verifyCase` as an unhandled rejection: the child exited 1 with an empty stdout, and `childFailures` fell through to the last stderr line that is not a stack frame, which for Node's uncaught-error dump is its own version banner. A missing `stream.bin` reported `reader exited with status 1 before it could report: Node.js v22.23.1`, with the errno and the path gone. That is the state a partial artifact download in CI reaches. It is reported on stdout now like every other failure, and `gate-teeth.test.mjs` holds it there. `COMPATIBILITY.md` and the package README both promised that `test/manifest.test.mjs` fails when `src/readers.mjs` and the document's reader list drift. It did not: the test compared `readers()` against a literal in the test file, so adding a version to the document alone stayed green, which is the drift direction that matters. `src/support-window.mjs` parses the fenced `<registry> <package> <versions...>` block instead and the test compares the `npm` rows, refusing a document with no block and one with no npm row rather than comparing against nothing. Also: each reader's declared `version` is compared against the `package.json` of the alias npm resolved, so the two-place edit cannot run one version while labelling it another; the coverage invariant counts readers pointed at a case rather than readers that pushed a name whether or not the case opened; `process.exitCode` replaces `process.exit()` so a hand-run on macOS, where pipe writes are asynchronous, cannot lose the failure lines it is being read for; and `pg-compat/README.md` describes the JS job as pending and gating on the seal outcome the sealing job publishes, which is what the proposed YAML does, rather than as a job that re-runs the path filter. CLAUDE.md's pointer to the `wire-compat-rust` YAML follows it from the merged #265 to #266. Refs #261, #251, #247. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#267) * test(compat): published pg-wasm/pg-js readers open HEAD-sealed samples The Node half of the wire-compat gate (#261). `pg-compat-js` opens the sample set the Rust half seals with the npm readers in COMPATIBILITY.md's support window: `@e4a/pg-wasm` 0.6.1 through both of its unsealers, and `@e4a/pg-js` 2.3.3 and 1.11.0 through `PostGuard#open`, the entry point consumers actually use. Versions are installed side by side as `npm:` aliases and pinned by a committed lockfile. Each case runs in its own process for the reason the Rust half does it: a reader handed a shifted header can die on the allocation rather than returning an error, and one dead case should not take the run down. Two limitations of the published SDKs are declared rather than hidden, because both would otherwise look like a gate that reads nothing. pg-js is stream-mode only in both directions, so the memory-mode cases are the pg-wasm reader's alone; and pg-js 1.x never surfaces a private signing policy, so for that reader the gate requires it absent. The run prints which reader opened which case, and fails when a case is opened by no reader or a reader opens no case. Also corrects `pg-compat/README.md`, which had memory mode as what pg-js `toBytes` produces — it seals with `sealStream`. The `wire-compat-js` job that runs this in CI cannot be pushed by the app; the YAML is in a PR comment. Refs #247, #251 Closes #261 * docs(compat): drop em dashes from pg-compat-js README prose * test(compat): report reader errors, and check the window against COMPATIBILITY.md Review follow-ups on the Node half of the wire-compat gate. An error raised before the first reader call left `verifyCase` as an unhandled rejection: the child exited 1 with an empty stdout, and `childFailures` fell through to the last stderr line that is not a stack frame, which for Node's uncaught-error dump is its own version banner. A missing `stream.bin` reported `reader exited with status 1 before it could report: Node.js v22.23.1`, with the errno and the path gone. That is the state a partial artifact download in CI reaches. It is reported on stdout now like every other failure, and `gate-teeth.test.mjs` holds it there. `COMPATIBILITY.md` and the package README both promised that `test/manifest.test.mjs` fails when `src/readers.mjs` and the document's reader list drift. It did not: the test compared `readers()` against a literal in the test file, so adding a version to the document alone stayed green, which is the drift direction that matters. `src/support-window.mjs` parses the fenced `<registry> <package> <versions...>` block instead and the test compares the `npm` rows, refusing a document with no block and one with no npm row rather than comparing against nothing. Also: each reader's declared `version` is compared against the `package.json` of the alias npm resolved, so the two-place edit cannot run one version while labelling it another; the coverage invariant counts readers pointed at a case rather than readers that pushed a name whether or not the case opened; `process.exitCode` replaces `process.exit()` so a hand-run on macOS, where pipe writes are asynchronous, cannot lose the failure lines it is being read for; and `pg-compat/README.md` describes the JS job as pending and gating on the seal outcome the sealing job publishes, which is what the proposed YAML does, rather than as a job that re-runs the path filter. CLAUDE.md's pointer to the `wire-compat-rust` YAML follows it from the merged #265 to #266. Refs #261, #251, #247. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(compat): check the alias package, not just its version The installed-version check closed the version half of the two-place edit and left the package half open: an alias resolving to the right version of the wrong package passed while every message carried the declared package name, which is the mislabelled support window the check exists to catch. `installedPackage` returns `{name, version}` and the test asserts both. Two docs pointers with it. `pg-compat/README.md` called #261 a PR; the work item is issue #261 and the YAML is a comment on PR #267. `COMPATIBILITY.md` claimed the whole `crates.io` line is spelled out in `pg-compat/Cargo.toml`, which pins 0.6.1 only; 0.5.10 is declared in the window and opened by no gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: run the JS half of the wire-compat gate * docs: the reader list is enforced by both gate halves, not a review rule * docs: state the gates' real reader-list coverage; track the gaps in #268 * fix(pg-compat-js): close three silent-green gaps found in review - stderrTail kept the LAST non-frame stderr line, which for a Node crash dump is the 'Node.js vX' banner — the real error was dropped on exactly the run where the message matters most (a child that died before it could report). Now takes the first informative line and filters the banner/preamble too. - readManifest refused a set with no cases but not a case with no recipients; verify.mjs loops over recipients, so an empty list reported as opened cleanly without checkOpened ever running. - The coverage invariants were computed from the manifest, so a sealer that stopped emitting the memory-mode cases kept them satisfied and both halves went green having lost the only coverage memory mode has. The case-name set is now asserted explicitly. Verified against a freshly sealed set: 22/22 pass, and removing the mem cases from the manifest now fails the assertion (it previously passed). * style(pg-compat-js): rewrap the lines over the package's ~100-char limit Ten measured lines plus the one prose line I left over 80 when editing pg-compat/README.md in place. No behaviour change; the suite still passes 22/22. Cheaper now than as churn when a formatter lands. * fix(pg-compat-js): unref the pkg stub so a leaked one cannot hang the case process The child ends with `process.exitCode = ...` rather than process.exit(), because pipe writes are async on macOS and exiting outright truncates the failure lines the gate exists to print. So it exits only when the loop drains, and a listening server is a live handle: a stub that outlived its case would hold the child open until runCase's 300s spawnSync timeout, turning a one-line failure into a stall. Today nothing reaches that: startPkgStub is the last fallible statement in the pg-js reader's load(), and verify.mjs always unloads. But that invariant lives in two other files and nothing stated or enforced it, which is what made this worth closing rather than just recording. Three parts: unref() so the leak cannot hang anything, a comment at the one place the invariant can be violated, and a test that spawns a child leaking a stub and asserts it still exits. Verified: the probe exits in 0s (it would previously have run to the timeout), and the full gate still passes 23/23 against a freshly sealed set with all five cases opened — unref does not stop the stub serving, since the in-flight request and pg-js's own client socket keep the loop alive. --------- Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ruben Hensen <ruben.hensen@protonmail.com>
Rust half of the bidirectional wire-compat gate. Closes #260; part of #251 (epic #247, workstream C).
Today only one direction is tested: committed old fixtures opened by a new reader (
pg-core/tests/wire_format.rs). The reverse is what breaks external consumers, and nothing checks it. This adds the two halves that do.Sealer
cargo run -p pg-core --features stream --example seal-samples -- <dir>Seals a sample set with the code in this tree. Five cases: in-memory, in-memory with a private signing key, streaming, streaming with a private signing key, and streaming across two AEAD segments (which also carries a populated
size_hint). Every container is sealed for two recipients, so the multi-recipient KEM entries in the header get read too.The two private-signing cases are not redundant. In memory mode the signature goes in the header; in stream mode it goes in the encrypted payload trailer. They are different wire surfaces and a break confined to one of them passes the other.
Everything derives from one 32-byte seed (master keys, USKs, signatures, IVs), so two runs of the same tree produce byte-identical files. That is what makes a diff between two sets mean "the wire format moved" rather than "the RNG moved". The manifest carries no timestamp or commit hash for the same reason.
pg-core/tests/sample_sealer.rskeeps the sealer honest inside the existingTest workspacejob: determinism, manifest-names-every-file (a case a reader cannot find is a case it skips), and a HEAD read-back of every case for every recipient.Reader
pg-compat/depends on publishedpg-core =0.6.1from crates.io and opens the sealed bytes. It sits outside the workspace on purpose: the rootCargo.tomlexcludeis what stops cargo resolvingpg-coreto../pg-coreand turning the gate into a tautology. It keeps its ownCargo.lockand runs with--locked.The dependency list is the support window from #252. Adding a version is a renamed manifest entry plus one
reader!invocation; the check body is a macro because each published version is a separate crate with separate types.A missing or empty artifact set fails. A gate that reads nothing and passes is worse than no gate.
One process per case
Each case is opened by a separate
pg-compat-casechild process. That is the part of the diff to look at hardest, because it is a response to how the gate failed rather than to how it was specified.A published reader handed a shifted header does not return an error. It reads a garbage length prefix, attempts a 20 GiB allocation, and the process aborts. In one process that killed the whole run on the first case, so the gate printed no per-case list for exactly the breakage class it exists to catch.
catch_unwindis no help, because an abort is not a panic.With one child per case, a case that dies is reported by name, with the signal and the child's last stderr line, and the other cases still run:
The children run under
target/, so an abort's core dump stays out of the working tree;coreis gitignored too, for anyone who runs a reader by hand.The five unit tests in
pg-compat/src/lib.rscover the harness itself, including one whose child aborts on SIGABRT for real rather than through a mock. They fail against the previous single-process shape, which had nowhere for them to hook in.Artifact layout
Documented in
pg-compat/README.md, including the manifest schema, what each field means and why the plaintext is a sibling file rather than base64. #261 (the Node old-reader half) consumes the same directory as a job artifact, so treat it as a contract:schemaVersionis there to be bumped when it changes.vk.jsonandusk-*.jsonare shaped like real PKG responses, so the same files feed a Rust reader and a JS one with no translation step.Two things there aimed squarely at #261:
sender.publicandsender.privateare written up alongside the rest of the contract, because they are what a JS reader checks the header signature against. And the proposed job uploads nothing on a PR that misses its path filter, so awire-compat-jsjob that downloads the artifact unconditionally will hard-fail rather than skip. That is in the README under "Consuming the set from another job".Does it actually go red
Checked by hand on this branch, reverted after:
VERSION_V3: 2→3HeaderHeaderRead that last row before taking a green run as permission. The header is a length-prefixed region and bincode ignores trailing bytes, so a field appended at the end of
Headerdoes still open with 0.6.1. Anywhere else shifts every following byte. It is written down inCLAUDE.mdand the README.The CI job
The
wire-compat-rustjob is not in this diff, because the App cannot push.github/workflows/. The YAML is in a PR comment below for a maintainer to apply; it path-filters onpg-core/**,pg-wasm/**andpg-compat/**, and uploads the sealed set so #261'swire-compat-jscan download it. Until someone lands it, nothing here gates a PR, which is howCLAUDE.mddescribes it.pg-compatis in none of the per-crate CI matrices either, so its fmt and clippy invocations are in the proposed YAML and written down inCLAUDE.md.Notes
pg-coregains one dev-dependency,rand_chacha.StdRngis explicitly not guaranteed reproducible across rand releases andChaCha20Rngis; it is already in the tree via rand'sstd_rng, so the dependency graph does not grow.required-features = ["rust", "stream"], socargo buildand a default-featurecargo checkskip it.cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream(67 pass), the pg-compat suite (6 unit plus the gate),cargo build --workspace,cargo fmt --all -- --checkfrom the repo root, and fmt and clippy forpg-compat.Unsealer::newaborting the process on a malformed header is pre-existing onmainand reachable frompg-pkg, which parses attacker-supplied ciphertext.