ci: bidirectional envelope-compat gate with an append-only fixture corpus - #153
Conversation
…rpus B6 (#129) rewrites the envelope, and envelope changes currently merge with nothing checking them across versions. This class has already shipped twice: pg-js >=1.1 silently stopped emitting body armor and broke Thunderbird detection (postguard-tb-addon#85), and the data.bin zip asymmetry broke the website's uuid decrypt path (#39). COMPATIBILITY.md pins both directions, and they are not symmetric. The SDK window is "the last two majors" of @e4a/pg-js, and it says email envelopes "carry the same guarantee" -- so the forward reader set is the latest published 1.x and 2.x, 1.11.0 and 2.3.4, installed under aliases as devDependencies. Archival is unconditional: "read support for stored artifacts is not part of this window. It never drops." Forward: an envelope built at HEAD, for each of the three tiers, must be read by every published reader in the window. Archival: HEAD must read every fixture in the committed corpus. The corpus is append-only and enforced. A fixture records bytes a sender actually emitted, so a failing archival test means the reader regressed -- editing the fixture to go green would erase the evidence instead. The check compares against the MERGE BASE rather than the base tip, so a fixture added to main after a branch starts is not blamed on that branch, and it fails rather than skips when it cannot find a base. Both directions assert their own wiring, per encryption4all/postguard#272: the corpus must be non-empty and cover all three tiers, and the reader list must be non-empty with the extractors present. Either could otherwise pass having checked nothing. Mutation-tested rather than trusted. Archival: renaming the attachment extractCiphertext looks for -> 3 failures; breaking the uuid regex -> 2; emptying the corpus -> the coverage assertion fires. Forward: renaming the attachment HEAD emits -> 5 failures, with the published readers returning null, which is the tb#85 signature exactly; changing the content type -> 1; changing the uuid route -> 5. Baseline green in every case afterwards. The gate also asserts the three things installed mail clients key on and that no existing unit test covers: the postguard.encrypted attachment name, the application/postguard content type, and the /decrypt|/download?uuid= routes. Deliberately NOT path-filtered to src/email/**, though #131 asks for it. A required status check that is path-filtered never reports on PRs which miss the filter, and branch protection then blocks them indefinitely. It runs in seconds. Not covered here, and #129's third part: the x-postguard header. Four producers disagree (pg-js writes "0.1", tb-addon writes "decrypted" after decrypt and a version on encrypt, cryptify stamps a pg-core version), and detection is a bare presence check, so every value passes and no test can tell them apart until the meaning is decided. Refs #131 Refs #129
pnpm envelope:check runs from packages/pg-js, so the repo-relative pathspec resolved to packages/pg-js/packages/pg-js/... and matched nothing. The check reported "none modified or removed" for every input, including a rewritten fixture -- a guard that could not fail. Found by mutation-testing it rather than by reading it, and it is the same working-directory shape as the Baked URLs job in outlook-addon.yml. Refs #131
|
Got it — I'm on PR #153. I'll review the corpus/harness changes against |
There was a problem hiding this comment.
The shape of this is right, and the asymmetry argument is the part worth keeping: forward is bounded by the support window, archival is not, and they need separate directions because of it. Two things should be fixed before this becomes a required check, both about the gate's own trustworthiness rather than about the envelope code.
1. The forward direction derives its expectation from the output under test. if (attachments.length === 0) comes from result.attachment, so dropping the tier-1 postguard.encrypted attachment flips which branch asserts and all four per-reader tier-1 cases pass. Since envelope-compat runs only these two files, the required check stays green on exactly the tb#85 signature it names. envelope.ts:169-171 makes tier→attachment a hard invariant, so asserting from result.tier is a small change. if (result.uploadUuid) has the same shape, and for tier 3 nothing else covers it.
2. The append-only guard protects nothing on this branch, so one row of the mutation table is not reproducible here. The merge base is 70fa425, which predates the corpus, so all four fixtures report A and a tamper/git rm/git mv each still exit 0. The mechanism itself is correct — against a base that already holds the corpus it reports MODIFIED/DELETED and exits 1 — so this is a PR-body correction, not a code change. Worth fixing because the whole argument for the guard is that the next reader can trust it.
Everything else is non-blocking: the reader pins go stale silently after the next changeset version, the archival replay feeds Buffer where ExtractCiphertextOptions declares ArrayBuffer (invisible because tsconfig.json has include: ["src"]), the archival markers case never calls into HEAD, producedBy carries no actual provenance, and the package.json description picked up an unrelated — escape. Details inline.
One finding could not be anchored inline because the file is outside the diff:
CLAUDE.md, the ## Tests section (line 68). It enumerates the suite file by file and none of this PR's additions appear: envelope-forward / envelope-archival, the pg-js-reader-v1/-v2 alias devDependencies the forward test needs installed, and — most importantly — the rule that tests/fixtures/envelopes/ is append-only. A contributor whose first encounter with that rule is a red Envelope compatibility check after editing a fixture has nothing in the repo's own guide to read. This file already carries exactly this kind of gotcha under "Prebuild generators (important)".
Unrelated and pre-existing, but noticed while reading the workflow: CLAUDE.md still says "api:gate is NOT yet a CI step. The job patch is a comment on #135 and needs a maintainer to apply it." The api-surface job runs pnpm api:gate today, so that paragraph is stale.
What I checked and found correct:
- Not path-filtering the job is the right call, and for the reason the comment gives. A job excluded by
on: paths:reports no status at all, and branch protection then waits forever on a check that correctly decided it had nothing to do. Keeping it trivially green is the fix, not a compromise. fetch-depth: 0is present, and the script fails rather than skips when it cannot find a merge base — so "cannot check" does not read as "clean".- The one multi-line
run:block declaresshell: bash, so it getspipefailrather than the defaultbash -e. - The corpus assertion (
tiersequals exactly tier1/tier2/tier3) closes the vacuous-pass hole areaddirSyncglob would otherwise leave. - The reader aliases resolve to real registry tarballs —
pnpm-lock.yamlrecords integrity hashes for both, not alink:to the workspace copy — so the forward direction really does run against published code. - No
.gitattributesconcern: the fixture digests are taken over base64-decoded bytes, not over the JSON files as read off disk, so a CRLF checkout does not reach any assertion. - PR title is valid Conventional Commits for
pr-title.yml.
|
|
||
| const recovered = reader.extractCiphertext({ attachments }) as Uint8Array | null; | ||
|
|
||
| if (attachments.length === 0) { |
There was a problem hiding this comment.
The forward direction picks its expectation from the output under test, so it cannot catch the regression it is named after.
if (attachments.length === 0) is derived from result.attachment, so when HEAD stops emitting the attachment the test switches to expect(recovered).toBeNull() and passes. expect(result.tier).toBe(tier) above still holds, so nothing else in the case fires.
src/email/envelope.ts:169-171 makes the invariant a hard one — tier === 'tier3' ? null : new File(...) — so patching it to tier === 'tier3' || tier === 'tier1' ? null : ... silently drops the tier-1 postguard.encrypted attachment and all four per-reader tier-1 cases still pass. That is exactly the tb#85 "silently stopped emitting" class this gate exists to catch. For tier 2 the same patch fails only the separate preserves the markers case (which happens to build 76 000 bytes). Because the envelope-compat job runs only these two files, the required check stays green; the thing that actually catches it today is the pre-existing tests/envelope.test.ts:25-26 in the broad node lane.
if (result.uploadUuid) on line 109 has the same shape. Tier 1 legitimately has no uuid (envelope.ts:85 short-circuits before the upload), but for tier 3 nothing else covers it — the preserves the markers case only builds tier 2, and its /\/(decrypt|download)\?uuid=/ assertion is what catches a tier-2 uuid regression.
Fix both from result.tier rather than from the values under test: tier1/tier2 must carry an attachment, tier3 must not; tier2/tier3 must carry a uuid, tier1 must not. The archival direction does not have this flaw — its branch keys on fixture.expect.ciphertextSha256, which the append-only guard freezes.
There was a problem hiding this comment.
Fixed in e06bf2a. Both expectations now come from result.tier: carriesAttachment = tier !== 'tier3', carriesUuid = tier !== 'tier1', asserted before extractCiphertext is called. The recovered bytes are compared against the payload rather than against attachments[0].data, so the assertion covers the whole chain instead of the reader round-tripping whatever it was handed.
Both mutations were run against the test file as you reviewed it and against the fixed one in the same vitest run:
| mutation | reviewed | fixed |
|---|---|---|
tier === 'tier3' || tier === 'tier1' ? null |
8/8 pass | 2 fail, tier1 attachment count: expected +0 to be 1 |
uploadUuid = tier === 'tier3' ? null : result.uuid |
8/8 pass | 2 fail, tier3 uploadUuid: expected null to be 'forward-uuid-0000' |
The left column is the vacuous pass you described, reproduced.
|
|
||
| let changes; | ||
| try { | ||
| changes = git(['diff', '--name-status', `${mergeBase}...HEAD`, '--', repoRelDir]); |
There was a problem hiding this comment.
The append-only guard is inert on this PR, so the description's mutation-test row for it cannot have been produced on this branch.
git diff <mergeBase>...HEAD compares against 70fa425, where the corpus does not exist, so every within-branch edit to a branch-added fixture is still A. Verified on this branch:
$ git diff --name-status $(git merge-base origin/main HEAD)...HEAD -- packages/pg-js/tests/fixtures/envelopes
A packages/pg-js/tests/fixtures/envelopes/tier1-url-fragment.json
A packages/pg-js/tests/fixtures/envelopes/tier2-attachment-and-link.json
A packages/pg-js/tests/fixtures/envelopes/tier2-upload-declined.json
A packages/pg-js/tests/fixtures/envelopes/tier3-cryptify-link-only.json
A tamper, a git rm and a git mv therefore all still print none modified or removed. and exit 0 — not exit 1 each as the table claims.
The semantics are right (editing a fixture your own branch added is legitimate) and the mechanism works: against a base that already contains the corpus it correctly reports tier1-url-fragment.json was MODIFIED / tier2-upload-declined.json was DELETED and exits 1. But the guard protects zero fixtures until a later branch, so the evidence line in the PR body should be corrected rather than left for the next reader to trust.
There was a problem hiding this comment.
Confirmed and corrected in the PR body — this was a description error, not a code change, exactly as you called it.
Verified both directions. Against a base that already holds the corpus it reports was MODIFIED, was DELETED and was RENAMED (R100) and exits 1 for each. Against 70fa425 it is inert: a tampered fixture and a git rm, committed together, still produced 3 fixtures present, 3 added on this branch, none modified or removed. and exit 0.
The table row is now split in two, and the body says the corpus is protected from the first branch that starts after this merges, with these four fixtures covered by review rather than by the script.
That window is also why producedBy is rewritten in this PR rather than later.
| # issue asks for it: a required status check that is path-filtered never | ||
| # reports on PRs which miss the filter, and branch protection then blocks them | ||
| # indefinitely. It runs in seconds, so it is cheap to let it be trivially green. | ||
| envelope-compat: |
There was a problem hiding this comment.
This job's verdict depends on the base branch, and the workflow will not re-run when the base changes.
pnpm envelope:check resolves its comparison from GITHUB_BASE_REF and takes a merge base, so the result is only valid against the base it ran against. integration.yml's trigger is a bare on: pull_request: branches: [main] with no types: key, which subscribes to opened, synchronize and reopened only. Retargeting a PR — by hand, or automatically when a stacked PR's base is deleted on merge — fires edited, which is not in that set. The check-run stays attached to the unchanged head sha and GitHub reports it as current, so a required check can pass on a comparison never made against the actual merge target.
Pre-existing rather than introduced: api-surface already runs pnpm api:gate off the same merge base with the same trigger, so this PR joins the debt rather than creating it. Flagging it here because envelope-compat is the job whose stated purpose is to become required, and one edit covers both:
on:
pull_request:
types: [opened, synchronize, reopened, edited]
branches: [main]There was a problem hiding this comment.
Applied, with your types: list verbatim. The comment above on: records why, and names both jobs that derive a verdict from a merge base rather than just this one, since api-surface has the same exposure.
actionlint clean on the result.
| ? [ | ||
| { | ||
| name: fixture.attachment.name, | ||
| data: Buffer.from(fixture.attachment.dataBase64, 'base64'), |
There was a problem hiding this comment.
The replay feeds a shape the public API does not declare, and nothing flags it.
ExtractCiphertextOptions.attachments is Array<{ name: string; data: ArrayBuffer }> (src/types.ts:354), but this passes Buffer.from(dataBase64, 'base64') and envelope-forward.test.ts:63 passes a Uint8Array. tsconfig.json has include: ["src"], so tests are never typechecked and the mismatch is invisible to pnpm typecheck.
It matters concretely: for the 512-byte tier-1 fixture that Buffer lands at a non-zero byteOffset inside Node's shared 8192-byte pool, so new DataView(buf) over the whole buffer sees the pool, not the fixture. A conforming extractCiphertext written against the declared ArrayBuffer would work for real add-in callers and misbehave against this gate. Passing new Uint8Array(...).buffer would replay what Thunderbird/Outlook actually send.
There was a problem hiding this comment.
Fixed. Both tests now build an exact-length ArrayBuffer via new Uint8Array(...).buffer, which is what ExtractCiphertextOptions declares, and the forward test passes await result.attachment.arrayBuffer() directly rather than wrapping it in a view.
Your point about include: ["src"] turned out to be the more interesting half. I measured it: widening it to cover tests yields 54 errors, but 29 are TS2591 for Buffer/process/node:* because @types/node is not installed anywhere in the workspace. The rest are ~25 real ones concentrated in four files, mostly FileState mocks in api.test.ts. Tractable, and being tracked separately rather than widening this PR.
| "@transcend-io/conflux": "^6.1.3" | ||
| }, | ||
| "devDependencies": { | ||
| "pg-js-reader-v1": "npm:@e4a/pg-js@1.11.0", |
There was a problem hiding this comment.
The reader pins go stale silently, which is the failure mode the rest of this PR is built to prevent.
envelope-forward.test.ts:11-12 documents these as "latest 1.x" and "latest 2.x", but nothing checks that. pg-js-reader-v2 is pinned to @e4a/pg-js@2.3.4 — the workspace's own current version — so today the 2.x lane compares HEAD against the last published release, which is a real and useful comparison. The problem is what happens next: after changeset version the pin does not move, so the gate quietly degrades to "compatible with an ever-older 2.x" while still reporting green and still carrying a comment that says "latest".
Worth either a check that each pin is the latest published release of its major, or a line in the release checklist next to pnpm api:update.
(Confirmed the aliases do resolve to the registry tarballs rather than linking the workspace copy — pnpm-lock.yaml records real integrity hashes for both @e4a/pg-js@1.11.0 and @e4a/pg-js@2.3.4.)
There was a problem hiding this comment.
Not fixed in code, documented instead. CLAUDE.md now carries it as a release step next to pnpm api:update, including dropping the v1 alias in a commit that says so when 3.0 takes 1.x out of the window, and the PR body says plainly that nothing asserts the pins.
A check that resolves the latest published release of each major from the registry would be better than a checklist line, but it puts a network call in the gate whose whole point is determinism, so it wants its own change rather than a late addition here.
| description, | ||
| // Recorded so a later reader can tell what produced these bytes without | ||
| // guessing from the shape. | ||
| producedBy: '@e4a/pg-js createEnvelope at the commit that added this file', |
There was a problem hiding this comment.
producedBy records no provenance. It is the constant string '@e4a/pg-js createEnvelope at the commit that added this file' in all four fixtures, while the comment above it says it exists "so a later reader can tell what produced these bytes without guessing from the shape" — it carries neither a version nor a commit, so it answers nothing the shape does not.
The corpus is append-only, so this is permanent for these four files. package.json's version is already read by scripts/generate-version.mjs, and git rev-parse HEAD is one call away.
There was a problem hiding this comment.
Fixed. The generator now derives it: @e4a/pg-js <version> createEnvelope, generated at postguard-js <sha>, with , working tree dirty appended when the package is dirty — without that a fixture generated from uncommitted work would name a commit whose tree never produced those bytes. It falls back to unknown commit outside a checkout rather than guessing.
The four existing fixtures are rewritten to @e4a/pg-js 2.3.4 createEnvelope, added in postguard-js 93ac061, which git log --diff-filter=A confirms for each. Doing it now is the point: the guard is inert against this branch's base, so this is the last commit where it is possible.
| expect(extractUploadUuid(fixture.htmlBody)).toBe(fixture.expect.uploadUuid); | ||
| }); | ||
|
|
||
| it('the markers installed clients key on are still present', () => { |
There was a problem hiding this comment.
the markers installed clients key on are still present asserts only fixture fields — fixture.attachment.name, fixture.attachment.contentType, fixture.htmlBody — and never calls into HEAD. Combined with the append-only guard that freezes those bytes, it can only ever fire on a newly added fixture: it is a corpus lint, not a reader gate.
So the description's "Both also assert the three things installed mail clients key on" holds only in the forward direction, and the application/postguard content type has no HEAD-side coverage at all, since src/email/extract.ts matches on att.name === 'postguard.encrypted' and never looks at the type. Worth either saying so in the comment or moving the case into the generator as a shape check on write.
There was a problem hiding this comment.
Agreed, and relabelled rather than rewritten. The case is now the markers installed clients key on are recorded in this fixture, with a comment saying it reads only fixture fields, never calls into HEAD, and can therefore only fire on a newly added fixture — a corpus lint, with the forward direction as the thing that holds HEAD to the markers.
The PR body claim you flagged is corrected too, including the part you noted about application/postguard having no HEAD-side coverage at all, since extract.ts matches on att.name and never reads the type.
| "type": "module", | ||
| "sideEffects": false, | ||
| "description": "Browser SDK for PostGuard — end-to-end encrypted file sharing with identity-based encryption", | ||
| "description": "Browser SDK for PostGuard \u2014 end-to-end encrypted file sharing with identity-based encryption", |
There was a problem hiding this comment.
Unrelated change: the description em dash was rewritten as the — escape. It parses to the same character so nothing breaks, and no script in this package writes package.json (generate-version.mjs only reads it), so this looks like an editor/tool artifact rather than an intended edit. Revert it to keep the diff to the gate.
There was a problem hiding this comment.
Reverted. git diff 70fa425 HEAD -- packages/pg-js/package.json is now only the two envelope:* scripts and the two reader aliases.
| // A corpus that matched nothing would let this exit 0 having checked nothing — | ||
| // the vacuous pass this whole gate exists to avoid. | ||
| if (!existsSync(absDir)) { | ||
| fail(`${repoRelDir} does not exist. Run this from the repository root.`); |
There was a problem hiding this comment.
Two small rough edges in the failure paths. The message says "Run this from the repository root", but after the working-directory fix in 0146ef5 the script resolves both the corpus and every git call itself — output is identical from packages/pg-js and from the repo root — so the advice is now misleading.
And git rev-parse --show-toplevel on line 35 is the one git call outside a try, so running outside a repository prints a raw Error: Command failed: git rev-parse --show-toplevel stack trace instead of the script's own envelope fixture check could not run: … message. It still exits 1, so it fails closed.
There was a problem hiding this comment.
Both fixed. The message is now just <dir> does not exist with a comment recording why there is no run-from-the-root advice: after 0146ef5 the script resolves the corpus and every git call itself, so its output is identical from either directory.
git rev-parse --show-toplevel is wrapped, with fail hoisted above it, so running outside a repository prints envelope fixture check could not run: <cwd> is not inside a git repository, so there is no history to compare instead of a raw execFileSync trace. It still exits 1 either way.
|
Posted a Request Changes review on postguard-js#153 with 9 inline comments. The forward/archival split is the right design, but two things undermine the gate's own trustworthiness before it goes required:
Seven more non-blocking notes are in the review (stale reader pins, a |
The forward direction branched on `attachments.length === 0` and on `result.uploadUuid`, both read off the output under test. A HEAD that silently stopped emitting the tier-1 attachment therefore took the tier-3 branch and all four per-reader tier-1 cases passed — the postguard-tb-addon#85 shape this gate is named after. Both expectations now come from `result.tier`, which src/email/envelope.ts makes a hard invariant, and the recovered ciphertext is compared against the payload rather than against what was fed to the reader. Also from the review on #153: - Both tests pass an exact-length ArrayBuffer, which is what ExtractCiphertextOptions declares. `Buffer.from` landed at a non-zero byteOffset inside Node's shared pool for the small fixtures, so a conforming reader wrapping the whole buffer would have seen the pool. Invisible to `pnpm typecheck` because tsconfig.json has `include: ["src"]`. - The archival markers case is relabelled as the corpus lint it is: every assertion reads a fixture field and none calls into HEAD. - `producedBy` records a real version and commit instead of a constant string that answered nothing the fixture's shape did not. Rewriting the four existing fixtures is legitimate only while the corpus is still branch-local and reports as added; once this merges the append-only guard forbids it, which is why it happens now. - integration.yml subscribes to `edited`, so retargeting a PR re-runs the two jobs whose verdict comes from a merge base: envelope-compat and api-surface. - Reverted an unrelated — escape in package.json's description, dropped now-misleading "run this from the repository root" advice, and wrapped `git rev-parse --show-toplevel` so running outside a repository reports through `fail` instead of a raw execFileSync stack trace. CLAUDE.md gains the append-only rule, the reader aliases, the derive-from-tier invariant and the untypechecked-tests gotcha. Its "api:gate is NOT yet a CI step" paragraph was stale — the api-surface job runs it.
|
All nine inline findings are answered above; pushed as e06bf2a. Summary of the two blocking ones and the two outside the diff. Forward direction. Fixed and mutation-tested against the version you reviewed: under Append-only guard. Your reading was right, and it is a body correction. The table now separates a base that holds the corpus (
Stale On Ready for another look. |
|
/dobby review and approve if you think there are no problems. |
|
On it! I'll review PR #153 now and approve if everything checks out, or leave feedback if I spot issues. |
There was a problem hiding this comment.
Approving. This clears the CHANGES_REQUESTED I left earlier — the two things it named are both closed, and closed the way I'd want: the tier derivation is now the source of both expectations, and you re-ran the two mutations against the old and the fixed file in the same vitest run rather than reasoning about them. All 30 checks green, including the new Envelope compatibility job (23s) and API surface.
The asymmetry argument holds up. I checked src/email/envelope.ts at HEAD rather than taking the PR body for it, and the append-only guard's own history — 0146ef5 rooting the git calls at the repository is the fix for a guard that could not fail, and the PR says so instead of quietly landing it.
Five non-blocking notes, ranked. The first one is the only one I'd actually fix before merge, because your own reasoning about the merge-base window applies to it: the four fixtures are editable now and not after this lands.
producedByin all four fixtures was not emitted by the generator. The corpus records…createEnvelope, added in postguard-js 93ac061;provenance()(generator line 44) emits…createEnvelope, generated at postguard-js <40-char sha>. Different wording, and a 7-char sha the script never produces —93ac061is also the commit that added these files, which the script could not have known when it wrote them. So these strings are hand-authored, which is the one thing the dirty-tree marker exists to prevent. Nothing reads the field and nothing fails; the cost is that the record is wrong, that the next fixture the script writes will be worded differently forever, and that once the guard is live on the following branch none of it is correctable. One line either way: regenerate the four, or rewordprovenance()to match.envelope-forward.test.ts:104overstates the invariant it derives from — tier 2 also ends withuploadUuid === nullforuploadToCryptify: false, forcanUpload === false, and whenupload()throws (envelope.tswarns and falls through). Deriving from the tier is right; the invariant needs "givenbuildAtHead's inputs". Your archival corpus already carriestier2-upload-declined, so mirroring it forward — the obvious next step — fails at line 108 for a reason this comment misdirects. CLAUDE.md's restatement is accurate; only this comment isn't.envelope-forward.test.ts:127never asserts the tier-1 uuid negative against a published reader. The archival side does cover HEAD's reader here, so the gap is forward-only, and I confirmed the tier-1 body carries/decrypt#ByZFZIOiweAEI0JhgJ--…with nouuid=anywhere in its 3487 chars — a reader that matched a spuriousuuid=inside that fragment would route the recipient at a Cryptify object nobody uploaded. Latent, not live. Same shape as the tier-3 negative three lines up:else { expect(reader.extractUploadUuid(result.htmlBody)).toBeNull(); }.integration.yml:12—editedis the right fix for the stale-check-run problem, and it is what our own notes say to do for a base-dependent verdict. Buton:is workflow-level, so it now also re-runsnode(22 and 24),bunanddeno, none of which read the base. From now on a description tweak re-runs all five lanes. Job-level scoping keeps the fix and drops the rest.generate-envelope-fixtures.mjs:144and:151both passpayload(76_000), so the two tier-2 fixtures commit byte-identical blobs and the sameexpect.ciphertextSha256— ~100 KB duplicated permanently, and a mix-up between them would pass the archival test. Distinct sizes cost nothing and this is the last chance to pick them.
Nothing here blocks. Rule check ran 12 rules over the workflow, the two scripts, the two tests and CLAUDE.md; the only hits were notes 2 and 4 above, independently. edited-for-a-base-dependent-verdict, un-path-filtering a would-be required check, shell: bash on the piped run: block, and flipping the api:gate line to present tense now that api-surface really does invoke it — all four match what we've written down.
On the 1.11.0 question you raised: I'd keep it too. The clause has two conditions and only one is met.
| { | ||
| "name": "tier1-url-fragment", | ||
| "description": "Small payload: ciphertext fits the recipient-side URL fragment, and the postguard.encrypted attachment is also present.", | ||
| "producedBy": "@e4a/pg-js 2.3.4 createEnvelope, added in postguard-js 93ac061", |
There was a problem hiding this comment.
This string was not produced by generate-envelope-fixtures.mjs. provenance() (line 44 of the generator) emits @e4a/pg-js <version> createEnvelope, generated at postguard-js <full 40-char sha> plus , working tree dirty when applicable. The corpus records added in postguard-js 93ac061 — different wording, and a 7-char sha the script never emits. 93ac061 is also the commit that added these four files, so the script could not have written it: a fixture generated in that tree would have named the parent commit plus the dirty marker.
So all four fixtures carry a hand-authored provenance line, which is the one thing the dirty-tree marker exists to prevent ("a fixture never names a commit whose tree did not produce it"). Nothing reads the field, so nothing fails. Two costs: the record is wrong, and the next fixture the script writes will be worded differently from these four forever. Same line in tier2-attachment-and-link.json, tier2-upload-declined.json and tier3-cryptify-link-only.json.
Worth closing now for exactly the reason the PR body gives for editing the fixtures here rather than later — after this merges the guard forbids it. Either regenerate the four with --force, or reword provenance() to emit what the corpus already records.
| // one of them take the other branch and pass — the exact | ||
| // postguard-tb-addon#85 shape this gate is named after. Both are hard | ||
| // invariants in src/email/envelope.ts: only tier 3 omits the | ||
| // attachment, and only tier 1 skips the Cryptify upload. |
There was a problem hiding this comment.
"only tier 1 skips the Cryptify upload" is not what envelope.ts does. uploadUuid also stays null for tier 2 when uploadToCryptify === false, when sealed.canUpload === false, and when sealed.upload() throws — that last one warns and falls through to the manual-upload body rather than rethrowing (only tier 3 rethrows). Line 108 holds only because buildAtHead always passes an uploadable stub and never sets uploadToCryptify: false.
Deriving from the tier is the right call and I am not asking you to change it. The invariant just needs the "given buildAtHead's inputs" qualifier — because the archival corpus already carries tier2-upload-declined, so mirroring that case on the forward side is the obvious next step, and it fails at line 108 for a reason this comment actively misdirects. CLAUDE.md's restatement of the rule is accurate; only this comment overstates it.
| expect(recovered).toBeNull(); | ||
| } | ||
|
|
||
| if (carriesUuid) { |
There was a problem hiding this comment.
The tier-1 negative for extractUploadUuid is never asserted against a published reader — if (carriesUuid) skips the call entirely for tier 1. The archival test does cover HEAD's own reader here (its expect.uploadUuid is null and the assertion is unconditional), so the gap is forward-only.
Why it is worth closing: tier 1 is the one tier whose body carries the whole ciphertext inline. I decoded the committed tier-1 fixture — the body is 3487 chars, contains /decrypt#ByZFZIOiweAEI0JhgJ--… and no uuid= anywhere. A published reader that matched a spurious uuid= inside that url-safe-base64 fragment would route the recipient at a Cryptify object that was never uploaded. Latent rather than live today.
Same shape as the tier-3 negative three lines up: else { expect(reader.extractUploadUuid(result.htmlBody)).toBeNull(); }.
| # never made against the actual merge target. | ||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, edited] |
There was a problem hiding this comment.
edited is the right fix for the stale-check-run problem and matches what we have written down for any verdict derived from a base ref — a retarget fires edited, the default types do not include it, and the stale check-run stays attached to the unchanged head sha and reads as current.
The scope is wider than the comment above it, though. on: is workflow-level, so edited now re-runs all five jobs: node (22 and 24 — recursive typecheck + build + test + smoke), bun, deno, plus the two that actually read the merge base. GitHub fires pull_request.edited for title and body edits as well as retargets, so from here on a description tweak re-runs the whole workflow.
Job-level scoping keeps the fix and drops the rest: if: github.event.action != 'edited' || github.event.changes.base != null on the three head-only lanes. changes.base is present only when the base ref actually changed, so title/body edits skip them and retargets still re-run everything that needs it.
| description: | ||
| 'Tier 2 with uploadToCryptify false: attachment only, no uuid link. Exercises the ' + | ||
| 'branch where a reader must fall back to the attachment.', | ||
| bytes: payload(76_000), |
There was a problem hiding this comment.
Both tier-2 fixtures use payload(76_000) (this line and line 144), so tier2-attachment-and-link and tier2-upload-declined commit byte-identical base64 blobs and the same expect.ciphertextSha256 (f09bbd1f…). Two consequences in a corpus that can never be edited again: ~100 KB of permanently duplicated payload, and the two fixtures' ciphertexts are indistinguishable by their own digests, so a mix-up between them would pass the archival test.
The comment above expect already reasons about corpus size ("storing them twice doubled every tier-2 fixture"). Distinct sizes cost nothing and this is the last chance to pick them.
|
Reviewed and approved — PR #153 is now Both issues from my earlier I left 5 non-blocking inline notes — only the first is worth fixing before merge:
Also agree with keeping |
|
Follow-up opened as #154, stacked on this branch: it points It retires the Worth knowing for review order: #154 is based on |
#153 landed on main as a squash commit, so main and this branch carry the same envelope-compat change under different shas. Resolutions, all three the same shape (keep this branch's addition, main's side adds nothing new in the conflicting region): - CLAUDE.md: keep the rewritten Tests paragraph describing tsconfig.typecheck.json; main still had the pre-#154 'tests are never typechecked' text. - packages/pg-js/package.json + pnpm-lock.yaml: keep the @types/node devDependency next to main's pg-js-reader-v1/v2 entries.
* ci: bidirectional envelope-compat gate with an append-only fixture corpus B6 (#129) rewrites the envelope, and envelope changes currently merge with nothing checking them across versions. This class has already shipped twice: pg-js >=1.1 silently stopped emitting body armor and broke Thunderbird detection (postguard-tb-addon#85), and the data.bin zip asymmetry broke the website's uuid decrypt path (#39). COMPATIBILITY.md pins both directions, and they are not symmetric. The SDK window is "the last two majors" of @e4a/pg-js, and it says email envelopes "carry the same guarantee" -- so the forward reader set is the latest published 1.x and 2.x, 1.11.0 and 2.3.4, installed under aliases as devDependencies. Archival is unconditional: "read support for stored artifacts is not part of this window. It never drops." Forward: an envelope built at HEAD, for each of the three tiers, must be read by every published reader in the window. Archival: HEAD must read every fixture in the committed corpus. The corpus is append-only and enforced. A fixture records bytes a sender actually emitted, so a failing archival test means the reader regressed -- editing the fixture to go green would erase the evidence instead. The check compares against the MERGE BASE rather than the base tip, so a fixture added to main after a branch starts is not blamed on that branch, and it fails rather than skips when it cannot find a base. Both directions assert their own wiring, per encryption4all/postguard#272: the corpus must be non-empty and cover all three tiers, and the reader list must be non-empty with the extractors present. Either could otherwise pass having checked nothing. Mutation-tested rather than trusted. Archival: renaming the attachment extractCiphertext looks for -> 3 failures; breaking the uuid regex -> 2; emptying the corpus -> the coverage assertion fires. Forward: renaming the attachment HEAD emits -> 5 failures, with the published readers returning null, which is the tb#85 signature exactly; changing the content type -> 1; changing the uuid route -> 5. Baseline green in every case afterwards. The gate also asserts the three things installed mail clients key on and that no existing unit test covers: the postguard.encrypted attachment name, the application/postguard content type, and the /decrypt|/download?uuid= routes. Deliberately NOT path-filtered to src/email/**, though #131 asks for it. A required status check that is path-filtered never reports on PRs which miss the filter, and branch protection then blocks them indefinitely. It runs in seconds. Not covered here, and #129's third part: the x-postguard header. Four producers disagree (pg-js writes "0.1", tb-addon writes "decrypted" after decrypt and a version on encrypt, cryptify stamps a pg-core version), and detection is a bare presence check, so every value passes and no test can tell them apart until the meaning is decided. Refs #131 Refs #129 * fix(ci): root the fixture check's git calls at the repository pnpm envelope:check runs from packages/pg-js, so the repo-relative pathspec resolved to packages/pg-js/packages/pg-js/... and matched nothing. The check reported "none modified or removed" for every input, including a rewritten fixture -- a guard that could not fail. Found by mutation-testing it rather than by reading it, and it is the same working-directory shape as the Baked URLs job in outlook-addon.yml. Refs #131 * fix(pg-js): derive forward envelope expectations from the tier The forward direction branched on `attachments.length === 0` and on `result.uploadUuid`, both read off the output under test. A HEAD that silently stopped emitting the tier-1 attachment therefore took the tier-3 branch and all four per-reader tier-1 cases passed — the postguard-tb-addon#85 shape this gate is named after. Both expectations now come from `result.tier`, which src/email/envelope.ts makes a hard invariant, and the recovered ciphertext is compared against the payload rather than against what was fed to the reader. Also from the review on #153: - Both tests pass an exact-length ArrayBuffer, which is what ExtractCiphertextOptions declares. `Buffer.from` landed at a non-zero byteOffset inside Node's shared pool for the small fixtures, so a conforming reader wrapping the whole buffer would have seen the pool. Invisible to `pnpm typecheck` because tsconfig.json has `include: ["src"]`. - The archival markers case is relabelled as the corpus lint it is: every assertion reads a fixture field and none calls into HEAD. - `producedBy` records a real version and commit instead of a constant string that answered nothing the fixture's shape did not. Rewriting the four existing fixtures is legitimate only while the corpus is still branch-local and reports as added; once this merges the append-only guard forbids it, which is why it happens now. - integration.yml subscribes to `edited`, so retargeting a PR re-runs the two jobs whose verdict comes from a merge base: envelope-compat and api-surface. - Reverted an unrelated — escape in package.json's description, dropped now-misleading "run this from the repository root" advice, and wrapped `git rev-parse --show-toplevel` so running outside a repository reports through `fail` instead of a raw execFileSync stack trace. CLAUDE.md gains the append-only rule, the reader aliases, the derive-from-tier invariant and the untypechecked-tests gotcha. Its "api:gate is NOT yet a CI step" paragraph was stale — the api-surface job runs it. * chore(pg-js): typecheck tests and scripts, not just src `pnpm typecheck` only ever looked at `src`, so a test could hand a public API a shape its types do not declare and stay green. That is how a `Buffer` reached an `ExtractCiphertextOptions.attachments` slot declared as `ArrayBuffer` (#153); for the small envelope fixtures that Buffer lands at a non-zero byteOffset inside Node's shared pool, so a conforming reader written against the declared type would have misbehaved against the gate. Widening `tsconfig.json` is not the fix: tsdown reads it as the build config and its `rootDir: "src"` turns every test file into TS6059. So `typecheck` now runs against `tsconfig.typecheck.json`, which extends it, drops `rootDir` and covers `src`, `tests` and `scripts`. `types: ["node"]` is explicit because TypeScript 6 no longer pulls every `node_modules/@types` package into the program automatically — without it, `Buffer`, `process` and every `node:*` import report TS2591 asking for `@types/node` while `@types/node` is installed. It is pinned to the major named in `engines` (>=22) so a test cannot quietly use an API the floor lacks. `allowJs` is on with `checkJs` off, so `tests/api-surface.test.ts` can import `scripts/lib/api-surface.mjs` without holding that script to strict mode. The 18 errors this surfaced, all real: - 15 `FileState` literals in `api.test.ts` omitted the required `recoveryToken`, so `storeChunk` returned `recoveryToken: undefined` against a type promising a string. Two call sites in the same file already passed it; the rest now match. - `zip.test.ts` passed a bare `Uint8Array` to `new Blob([...])`. Since TS 5.7 the array is generic over its backing buffer and `BlobPart` wants an ArrayBuffer-backed view, so the two helpers are typed `Uint8Array<ArrayBuffer>`. - `email-attributes.test.ts` read `.t` off an `AttrConItem`, which is `AttrReq | AttrDiscon` and only has `t` on one side. Narrowed with `Array.isArray` as types.ts documents, which also makes the assertion stronger: the entry must be a single attribute, not a disjunction. Stacked on feat/envelope-compat-gate: it retires the CLAUDE.md gotcha that branch adds rather than leaving it documented forever. * chore(pg-js): keep the src-only typecheck pass alongside the widened one Addresses the three review comments on #154. tsconfig.typecheck.json changes two program-wide settings, so running it INSTEAD of tsconfig.json quietly dropped coverage src had before: - types: ["node"] legalizes Buffer/process in src too, and this is a browser SDK that reaches process through an explicit globalThis cast. - declaration: false silences TS4023/TS4094/TS2742, which are only reported while declaration emit is on. tsdown does not typecheck, so the tsconfig.json pass is the only guard on the published .d.mts. typecheck now runs both passes. Verified by probe: appending a process/Buffer expression to src/util/identity.ts and adding a module exporting an anonymous class with a private member each exit 2 under the combined script and exit 0 under tsconfig.typecheck.json alone. Also: - tests/email-attributes.test.ts narrows with Array.isArray for real instead of casting to AttrReq, which is what the comment above it already claimed; the now-unused type import goes with it. - CLAUDE.md: say that scripts is in include for the import boundary only. With checkJs off and every script being .mjs, script bodies are not checked; what it buys is inferred shapes at the tests/api-surface.test.ts import of scripts/lib/api-surface.mjs. --------- Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com>
Nothing on `main` is a required status check today: the ruleset carries deletion, non_fast_forward and pull_request rules only, and legacy protection requires one review and no checks. So `Envelope compatibility` — built in #153 specifically to become required, and deliberately not path-filtered because a path-filtered required check never reports — runs on every PR and blocks nothing. Requiring the lanes as they are named today would trade that for a worse failure. `Node 22`, `Bun 1.3.14` and `Deno 2.8.0` are matrix-named, so the version IS the check name: bumping the matrix retires the required check silently, and main blocks with no failing job to point at. `integration-complete` has a fixed name, so a runtime bump stays a one-line matrix edit. Two details carry the guarantee: - `if: always()`, because without it the job is SKIPPED when a lane fails and GitHub counts a skipped check as PASSING for branch protection. The gate would report green on a red build. - an empty-`RESULTS` guard, because the loop over lane results runs zero times when `needs` resolves to nothing and the step would exit 0 having checked nothing. Verified by extracting the `run:` block and running it under the shell Actions actually uses (`bash --noprofile --norc -eo pipefail`), not a plain `bash`: all-success exits 0; failure, skipped, cancelled and empty each exit 1 with a reachable error annotation.
Closes #131. Prerequisite for B6 (#129), which rewrites exactly this code — envelope changes currently merge with nothing checking them across versions.
This class has already shipped twice: pg-js >=1.1 silently stopped emitting body armor and broke Thunderbird detection (postguard-tb-addon#85), and the data.bin zip asymmetry broke the website's uuid decrypt path (#39).
What the contract requires, and it is not symmetric
COMPATIBILITY.mdinencryption4all/postguard:@e4a/pg-js, and "email envelopes are the@e4a/pg-jslayer and carry the same guarantee". So the reader set is the latest published 1.11.0 and 2.3.4, installed under aliases as devDependencies. Real published packages, not mocks.The two directions
Forward — an envelope built at HEAD, for each of the three tiers, must be read by every published reader in the window.
Every forward expectation is derived from
result.tier, never fromresult.attachmentorresult.uploadUuid. Branching on the output under test lets a HEAD that silently stops emitting the tier-1 attachment take the tier-3 branch and pass, which is the tb#85 regression itself.envelope.tsmakes both invariants hard: only tier 3 omits the attachment, only tier 1 skips the Cryptify upload.Archival — HEAD must read every fixture in the committed corpus: all three tiers, including tier 3, which carries no attachment at all and is the asymmetry behind #39.
The forward direction holds HEAD to the three things installed mail clients key on, which no other unit test covers: the
postguard.encryptedattachment name, theapplication/postguardcontent type, and the/decrypt|/download?uuid=routes. The archival direction checks the first two against the fixture rather than against HEAD, so that case is a corpus lint — it rejects a fixture that does not record what a real sender emits, and it is labelled as such.application/postguardhas no archival-side coverage by construction:extract.tsmatches on the attachment name and never reads the content type.The corpus is append-only, and that is enforced
A fixture records bytes a sender actually emitted. If an archival test fails, the regression is in the reader — editing the fixture to go green would erase the evidence instead of fixing anything.
check-envelope-fixtures.mjscompares against the merge base rather than the base tip, so a fixture added to main after a branch starts is not blamed on that branch, and it fails rather than skips when it cannot find a base.Mutation-tested, and one mutation found a real bug
Every direction was verified by reintroducing the failure it claims to catch, not by reading it.
extractCiphertextlooks forThe two tier-derivation mutations were each run against the test file as first reviewed and against the fixed one in the same vitest run. The reviewed version passed all eight of its cases under both mutations; the fixed version failed the two cases for the affected tier. That is the finding from review, reproduced and then closed.
The append-only guard failed its own mutation test first.
pnpm envelope:checkruns frompackages/pg-js, so its repo-relative pathspec resolved topackages/pg-js/packages/pg-js/…and matched nothing — it reported "none modified or removed" for every input, including a rewritten fixture. A guard that could not fail. Fixed by rooting every git call at the repository; it is the same working-directory shape as theBaked URLs resolvebug inoutlook-addon.yml.Correction: the append-only guard protects nothing on this branch, and an earlier version of the table above did not say so. The merge base is
70fa425, which predates the corpus, so all four fixtures reportAand a tamper, agit rmand agit mveach still exit 0 — verified with a tampered and a deleted fixture in the same commit, which the script reported as3 fixtures present, 3 added on this branch, none modified or removed.The mechanism is correct: pointed at a base that already holds the corpus it reportswas MODIFIED,was DELETEDandwas RENAMED (R100)respectively and exits 1, verified for all three. So the guard starts protecting the corpus from the first branch that begins after this merges, and these four fixtures are protected by review rather than by the script.That window is also why the fixtures are edited in this PR rather than later:
producedBynow records a version and a commit instead of a constant string, and once this merges the guard forbids that change.Two deliberate departures from the issue
Not path-filtered to
src/email/**. A required status check that is path-filtered never reports on PRs which miss the filter, and branch protection then blocks them indefinitely. The job runs in seconds, so it is cheap to let it be trivially green.Fixtures live here, not in postguard-e2e#19. That issue is still open, so the shared corpus does not exist; waiting would leave envelope changes ungated while B6 rewrites them. Adopting e2e#19's corpus later is additive.
integration.ymlnow also subscribes toedited. Bothenvelope-compatandapi-surfacederive their verdict from a merge base, so retargeting a PR has to re-run them; the default trigger set does not, and the stale check-run stays attached to the unchanged head sha and reads as current.Worth a decision before merge
COMPATIBILITY.mdsays1.xleaves the window "when the Outlook migration lands (postguard-outlook-addon#125) and telemetry shows no1.xtraffic". The migration landed — Outlook builds against workspace2.3.4and production has served it sinceoutlook-addin-v1.0.0. So the first condition is met and only telemetry remains. I kept1.11.0as a reader, which is the conservative reading, but that is the version the clause is about.Nothing asserts that the two reader aliases are still the latest release of their major. After a
changeset versionthe 2.x pin needs moving by hand or the gate quietly degrades to "compatible with an ever-older 2.x" while still reporting green.CLAUDE.mdnow carries that as a release step; a check that reaches the registry from CI would be better and is not in this PR.Not covered here
The
x-postguardheader, which is #129's third part. Four producers disagree — pg-js writes0.1, tb-addon writesdecryptedafter decryption and a version on encrypt, cryptify stamps a pg-core version — and detection is a bare presence check ("x-postguard" in full.headers), so every value passes and no test can tell them apart until the meaning is decided.Changes since the first review round
result.tierfor both the attachment and the uuid, closing the case where dropping either flipped a branch instead of failing.ArrayBuffer, which is whatExtractCiphertextOptionsdeclares.Buffer.fromlands at a non-zerobyteOffsetinside Node's shared pool for the small fixtures.tsconfig.jsonhasinclude: ["src"], so this could not surface inpnpm typecheck.producedByrecords a real version and commit; the generator derives it, with a dirty-tree marker so a fixture never names a commit whose tree did not produce it.integration.ymlsubscribes toedited.—escape inpackage.json's description, dropped the now-misleading "run this from the repository root" advice, and wrappedgit rev-parse --show-toplevelso running outside a repository reports throughfailrather than a raw stack trace.CLAUDE.mddocuments the append-only rule, the reader aliases, the derive-from-tier invariant and the fact that tests are outsidetsconfig.json'sinclude. Its "api:gate is NOT yet a CI step" paragraph was stale: theapi-surfacejob runs it.Full pg-js suite 305 passing, typecheck clean,
actionlintclean on the changed workflow.Part of encryption4all/postguard#247 (workstream C).