Skip to content

ci: bidirectional envelope-compat gate with an append-only fixture corpus - #153

Merged
rubenhensen merged 3 commits into
mainfrom
feat/envelope-compat-gate
Jul 30, 2026
Merged

ci: bidirectional envelope-compat gate with an append-only fixture corpus#153
rubenhensen merged 3 commits into
mainfrom
feat/envelope-compat-gate

Conversation

@rubenhensen

@rubenhensen rubenhensen commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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.md in encryption4all/postguard:

  • Forward — the SDK window is "the last two majors" of @e4a/pg-js, and "email envelopes are the @e4a/pg-js layer 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.
  • Archival — "Read support for stored artifacts is not part of this window. It never drops, whatever happens to the SDK version that wrote the bytes." Unconditional, so this direction can never be narrowed by dropping a major.

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 from result.attachment or result.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.ts makes 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.encrypted attachment name, the application/postguard content 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/postguard has no archival-side coverage by construction: extract.ts matches 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.mjs 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.

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.

mutation result
rename the attachment extractCiphertext looks for archival: 3 failures
break the uuid regex archival: 2 failures
empty the corpus coverage assertion fires
rename the attachment HEAD emits forward: 5 failures, published readers return null — the tb#85 signature
drop the tier-1 attachment while keeping the tier forward: 2 failures, one per reader
drop the tier-3 uuid after a successful upload forward: 2 failures, one per reader
change the content type forward: 1 failure
change the uuid route forward: 5 failures
modify / delete / rename a fixture, against a base holding the corpus append-only guard: exit 1 each
modify / delete / rename a fixture, against a base predating it exit 0, by design — see the correction below
add a fixture allowed, exit 0

The 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:check runs from packages/pg-js, so its repo-relative pathspec resolved to packages/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 the Baked URLs resolve bug in outlook-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 report A and a tamper, a git rm and a git mv each still exit 0 — verified with a tampered and a deleted fixture in the same commit, which the script reported as 3 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 reports was MODIFIED, was DELETED and was 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: producedBy now 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.yml now also subscribes to edited. Both envelope-compat and api-surface derive 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.md says 1.x leaves the window "when the Outlook migration lands (postguard-outlook-addon#125) and telemetry shows no 1.x traffic". The migration landed — Outlook builds against workspace 2.3.4 and production has served it since outlook-addin-v1.0.0. So the first condition is met and only telemetry remains. I kept 1.11.0 as 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 version the 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.md now 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-postguard header, which is #129's third part. Four producers disagree — pg-js writes 0.1, tb-addon writes decrypted after 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

  • Forward expectations derive from result.tier for both the attachment and the uuid, closing the case where dropping either flipped a branch instead of failing.
  • Both tests pass an exact-length ArrayBuffer, which is what ExtractCiphertextOptions declares. Buffer.from lands at a non-zero byteOffset inside Node's shared pool for the small fixtures. tsconfig.json has include: ["src"], so this could not surface in pnpm typecheck.
  • The archival markers case is labelled as the corpus lint it is.
  • producedBy records 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.yml subscribes to edited.
  • Reverted an unrelated escape in package.json's description, dropped the now-misleading "run this from the repository root" advice, and wrapped git rev-parse --show-toplevel so running outside a repository reports through fail rather than a raw stack trace.
  • CLAUDE.md documents the append-only rule, the reader aliases, the derive-from-tier invariant and the fact that tests are outside tsconfig.json's include. Its "api:gate is NOT yet a CI step" paragraph was stale: the api-surface job runs it.

Full pg-js suite 305 passing, typecheck clean, actionlint clean on the changed workflow.

Part of encryption4all/postguard#247 (workstream C).

…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
@dobby-coder

dobby-coder Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Got it — I'm on PR #153. I'll review the corpus/harness changes against COMPATIBILITY.md's forward (latest 1.x/2.x readers) and archival (never-drop) guarantees, check that all three tiers are covered including the tier-3 no-attachment case, and confirm the postguard.encrypted attachment name and MIME expectations are asserted before landing a consolidated review.

@dobby-coder
dobby-coder Bot requested a review from Iso5786 July 30, 2026 12:52

@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.

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: 0 is 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 declares shell: bash, so it gets pipefail rather than the default bash -e.
  • The corpus assertion (tiers equals exactly tier1/tier2/tier3) closes the vacuous-pass hole a readdirSync glob would otherwise leave.
  • The reader aliases resolve to real registry tarballs — pnpm-lock.yaml records integrity hashes for both, not a link: to the workspace copy — so the forward direction really does run against published code.
  • No .gitattributes concern: 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) {

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 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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]);

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 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

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.

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]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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'),

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 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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",

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 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.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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',

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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', () => {

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 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread packages/pg-js/package.json Outdated
"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",

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.`);

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@dobby-coder

dobby-coder Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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:

  1. The forward test derives its expected outcome from the very output it's checking (result.attachment in envelope-forward.test.ts:100), so dropping the tier-1 attachment just flips branches instead of failing — the exact "silently stopped emitting" bug (postguard-tb-addon#85) this gate is meant to catch would sail through. Asserting from result.tier instead is a small fix, and the tier-3 uploadUuid case has the same gap.
  2. The append-only corpus guard isn't actually exercised on this branch — the merge base predates the corpus, so the PR body's mutation table isn't reproducible here. Worth a correction in the PR description since the mechanism itself checks out fine against a base that already has the corpus.

Seven more non-blocking notes are in the review (stale reader pins, a Buffer/ArrayBuffer mismatch invisible because tsconfig.json excludes tests from typechecking, a weak fixture-only assertion, missing provenance, a stray character in package.json, and a CI trigger gap on integration.yml), plus two things outside the diff worth a look: CLAUDE.md's test section doesn't mention the new append-only rule, and it also has a stale note about api:gate not being wired into CI.

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.
@rubenhensen

Copy link
Copy Markdown
Contributor Author

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 tier === 'tier3' || tier === 'tier1' ? null the reviewed test passes 8/8 while the fixed one fails both tier-1 cases, and the same holds for the tier-3 uuid. Your finding reproduced before it was closed.

Append-only guard. Your reading was right, and it is a body correction. The table now separates a base that holds the corpus (MODIFIED / DELETED / RENAMED (R100), exit 1, all three verified) from a base that predates it (exit 0 with a tampered and a deleted fixture in the same commit). The body states that the corpus is protected from the first branch starting after this merges, and that these four fixtures are covered by review rather than by the script.

CLAUDE.md, ## Tests. Taken. It now documents the two new files and the job that runs them, the append-only rule with envelope:check / envelope:fixtures, the reader aliases and their staleness, and the derive-from-tier invariant with a note that it looks simplifiable and is not. It also records that tsconfig.json has include: ["src"], so a test can pass a shape the public types do not declare with pnpm typecheck still green — the Buffer/ArrayBuffer case you found.

Stale api:gate paragraph. Corrected: the api-surface job invokes it at integration.yml:169. Thanks for catching it while reading past the diff.

On include: ["src"] more broadly — I measured widening it. 54 errors, of which 29 are TS2591 for Buffer / process / node:* because @types/node is not a dependency anywhere in the workspace. The remaining ~25 are real and sit in four files, mostly FileState mocks in api.test.ts plus implicit any in api-surface.test.ts. That is worth doing and does not belong in this PR; it needs a devDependency and touches tests unrelated to envelopes.

Ready for another look.

@rubenhensen

Copy link
Copy Markdown
Contributor Author

/dobby review and approve if you think there are no problems.

@dobby-coder

dobby-coder Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

On it! I'll review PR #153 now and approve if everything checks out, or leave feedback if I spot issues.

@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. 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.

  1. producedBy in 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 — 93ac061 is 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 reword provenance() to match.
  2. envelope-forward.test.ts:104 overstates the invariant it derives from — tier 2 also ends with uploadUuid === null for uploadToCryptify: false, for canUpload === false, and when upload() throws (envelope.ts warns and falls through). Deriving from the tier is right; the invariant needs "given buildAtHead's inputs". Your archival corpus already carries tier2-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.
  3. envelope-forward.test.ts:127 never 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 no uuid= anywhere in its 3487 chars — a reader that matched a spurious uuid= 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(); }.
  4. integration.yml:12edited is the right fix for the stale-check-run problem, and it is what our own notes say to do for a base-dependent verdict. But on: is workflow-level, so it now also re-runs node (22 and 24), bun and deno, 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.
  5. generate-envelope-fixtures.mjs:144 and :151 both pass payload(76_000), so the two tier-2 fixtures commit byte-identical blobs and the same expect.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",

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.

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.

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.

"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) {

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 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]

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.

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),

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.

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.

@dobby-coder

dobby-coder Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Reviewed and approved — PR #153 is now APPROVED with mergeStateStatus: CLEAN, so it's ready for you to merge.

Both issues from my earlier CHANGES_REQUESTED are properly fixed: forward expectations now derive from result.tier for both the attachment and the uuid, and I re-ran the tier-derivation mutations against old and fixed code in the same vitest run. All 30 checks pass, including the new Envelope compatibility job.

I left 5 non-blocking inline notes — only the first is worth fixing before merge:

  1. producedBy in all four fixtures wasn't actually emitted by the generator (predates the commit that would've written it) — hand-authored, which the dirty-tree marker exists to catch.
  2. A test comment overstates its invariant (misses other uploadUuid === null cases).
  3. The tier-1 uuid negative isn't asserted against a published reader — latent gap, not live.
  4. The edited trigger fix is right but re-runs all five jobs workflow-wide; scoping it at job level would avoid that.
  5. Both tier-2 fixtures share the same payload/digest, duplicating ~100 KB.

Also agree with keeping 1.11.0 open — only telemetry is outstanding there.

@rubenhensen

Copy link
Copy Markdown
Contributor Author

Follow-up opened as #154, stacked on this branch: it points pnpm typecheck at a tsconfig.typecheck.json covering src, tests and scripts, and fixes the 18 real errors that surfaced — no suppressions.

It retires the include: ["src"] gotcha this PR documents rather than leaving it in CLAUDE.md permanently, and it keeps that note accurate by rewriting it on the follow-up branch.

Worth knowing for review order: #154 is based on feat/envelope-compat-gate, so integration.yml does not report on it at all until this merges and it retargets to main.

@rubenhensen
rubenhensen merged commit eb02727 into main Jul 30, 2026
36 checks passed
@rubenhensen
rubenhensen deleted the feat/envelope-compat-gate branch July 30, 2026 15:55
dobby-coder Bot added a commit that referenced this pull request Jul 30, 2026
#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.
rubenhensen added a commit that referenced this pull request Jul 30, 2026
* 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>
rubenhensen added a commit that referenced this pull request Jul 30, 2026
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.
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.

ci: bidirectional envelope-compat gate — HEAD-built envelopes must parse with published readers, HEAD parsers must read all archived envelopes

1 participant