chore(pg-js): typecheck tests and scripts, not just src - #154
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
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.
`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.
|
Note on CI: this PR currently runs only That resolves itself: once #153 merges and this retargets to Verified locally in the meantime: |
|
/dobby solve conflicts |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
The premise is right and the second-config approach is the correct call: tsconfig.json really is the build config, rootDir: "src" really does turn every test file into TS6059, and the 18 errors were fixed rather than suppressed. Checked all of it against a real install on this branch (TypeScript 6.0.3).
One blocking problem, and it is the mirror image of the bug this PR fixes: widening the check narrowed two others. Details inline on tsconfig.typecheck.json. Short version — typecheck no longer runs the src-only config, and two settings in the new one are looser than the old one was:
types: ["node"]makesBufferandprocesslegal insrc. Confirmed: adding both tosrc/util/identity.tsgives exit 0 under the new config, TS2591 under the old one. This is a browser SDK that reachesprocessthrough a deliberateglobalThiscast, so that was a real guard.declaration: falsedrops TS4023/TS4094/TS2742, which are only reported when declaration emit is on. Confirmed with a synthetic probe: exit 2 undertsconfig.json, exit 0 undertsconfig.typecheck.json. tsdown does not typecheck, so nothing catches these now, and this package publishes.d.ts.
Both are fixed by one line, and both passes are already green on this branch:
"typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.typecheck.json"Two stale comments in files this PR does not touch
Both say tests are outside the typecheck, which this PR makes false. GitHub will not take inline comments on unchanged files, so they are here:
tests/envelope-archival.test.ts:62— "tsconfig.json'sinclude: ["src"]keeps tests out ofpnpm typecheck, so the mismatch would not surface there." It surfaces now. Verified: restoring the bareBuffer.from(...)in thedataslot fails with TS2322 at line 73. This is the exact comment the PR body cites as the motivating case, so it is the one place a reader is most likely to trust it. Suggest pointing it at the new config instead.tests/envelope-forward.test.ts:61— "tsconfig.json'sinclude: ["src"]means nothing here would flag the drift." It does now: swapping inBuffer.from(await result.attachment.arrayBuffer())fails with TS2322 at line 113 against the local reader shim'sArrayBufferLike.
CLAUDE.md was updated in this PR; these two were not.
What checked out
- Not a no-op against its base. Two-dot diff
origin/feat/envelope-compat-gate..origin/chore/typecheck-testsis non-empty across all 7 files. - "green across all 7 projects" holds. All 7 workspace members declare a
typecheckscript, sopnpm -r typecheckinintegration.ymlskips none of them, andexamples.ymlalready guards the examples against a missing or empty one. - No stale docs left behind. Grepped the repo for
tsc --noEmit; the only pg-js references are the two CLAUDE.md lines this PR updates. The add-on hits are their own separate configs. @types/node@^22.20.1matches the>=22floor in both the root and pg-jsengines, as the comment claims.types: ["node"]is genuinely required, not cargo-culted: with it omitted thenode:*imports fail with TS2591 even though@types/nodeis installed.
Also: declarationMap and sourceMap at lines 22-23 are inert under noEmit and can go. declaration is the only one of the three that changes anything, and per the above it should go back to inheriting.
|
Reviewed #154 and confirmed your approach is right — The blocking one: widening the check narrowed two settings from the old src-only config. "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.typecheck.json"Also flagged: two now-false "tests are outside typecheck" comments (in the review body since those files are untouched), a stale Everything else checks out — not a no-op, all 7 workspace Review: #154 (review) |
|
Got it — I'm on it now. I'll take a look at PR #154, resolve the merge conflicts, and update the PR directly. |
#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.
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.
|
Conflicts resolved in e3d0a7e, and the three review comments in 96574e4. Where the conflicts came from#153 merged to
The merge commit turns out to change nothing at all against the pre-merge tip: Verified by patch-content identity rather than by reading hunks:
The review commentsAll three are in 96574e4, each answered on its own thread, all three resolved. In short:
Also worth knowing
|
1 participant
Stacked on #153 — base is
feat/envelope-compat-gate, so merge that first. Retargets tomainon its own once #153 lands.pnpm typecheckonly ever looked atsrc. A test could hand a public API a shape its own types do not declare and the check stayed green.That is not hypothetical: it is how a
Bufferreached anExtractCiphertextOptions.attachmentsslot declared asArrayBuffer, found by review on #153 rather than by any check. It matters concretely — for the small envelope fixtures thatBufferlands at a non-zerobyteOffsetinside Node's shared 8 KiB pool, so a reader written against the declaredArrayBufferwould work for real add-in callers and misbehave against the compat gate.Why a second config rather than a wider
includetsconfig.jsonis the build config: tsdown reads it, and it setsrootDir: "src", which turns every file undertests/intoTS6059instead of a checked file. Sotypechecknow runstsc --noEmit -p tsconfig.typecheck.json, which extends it, dropsrootDir, and coverssrc,testsandscripts. The build path is untouched.Two settings there are load-bearing:
types: ["node"]. TypeScript 6 no longer pulls everynode_modules/@typespackage into the program automatically. Without this,Buffer,processand everynode:*import fail withTS2591telling you to install@types/node— while@types/nodeis installed and sitting innode_modules. 29 of the 54 initial errors were this. Pinned to the major inengines(>=22) so a test cannot quietly use an API the floor lacks.allowJson,checkJsoff, sotests/api-surface.test.tscan importscripts/lib/api-surface.mjswithout holding that script to strict mode it was never written for.The 18 real errors, all fixed rather than suppressed
tests/api.test.tsFileStateliterals omitting the requiredrecoveryToken, sostoreChunkreturnedrecoveryToken: undefinedagainst a type promising astring. Two call sites in the same file already passed it; the rest now match.tests/zip.test.tsUint8Arrayintonew Blob([...]). Since TS 5.7 the array is generic over its backing buffer andBlobPartwants an ArrayBuffer-backed view, so the two helpers are typedUint8Array<ArrayBuffer>.tests/email-attributes.test.ts.toff anAttrConItem, which isAttrReq | AttrDisconand carrieston only one side. Narrowed withArray.isArrayastypes.tsdocuments, which also strengthens the assertion: the entry must be a single attribute, not a disjunction.No
@ts-expect-error, noany, no relaxedstrict.Verified by breaking it
Reintroducing the
Bufferinto theArrayBufferslot:tsc -p tsconfig.typecheck.jsonTS2322, exit 2tsc -p tsconfig.json(the old, src-only check)vitest run envelope-archivalThe last two rows are the point: neither the previous typecheck nor the test suite catches this class, which is why it took a human reading the diff.
pnpm -r typecheckgreen across all 7 projects, pg-js suite 305 passing, build and API report clean.Part of encryption4all/postguard#247.