ci: track the public API surface as a reviewed snapshot

[dobby-coder]

Tracks the public type surface of @e4a/pg-js as a committed snapshot and fails the build when the code and the snapshot disagree.

Closes #130.
Part of encryption4all/postguard#247 (workstream D).

The snapshot

packages/pg-js/etc/pg-js.api.md is rendered from the rolled-up dist/index.d.mts by scripts/api-report.mjs. It holds 57 declarations: the 53 named in the export clause, plus PostGuardBase, EmailHelpers, EmailAttributes and SenderIdentity. Those four never appear in src/index.ts, but they reach consumers through inheritance and property types, so they belong in the contract.

Two normalisations keep the file quiet. private members are dropped, and members are sorted by name. Neither reordering a class body nor renaming an internal field produces a diff. protected members stay, since a subclass can see them. A name that owns several statements, such as an overload set or a merged interface, keeps all of them.

What gates today

postbuild runs api-report.mjs --check, so pnpm build fails when the built types no longer match the committed report. That means CI's Build step fails too, on all three runtime lanes, with no workflow change. Same shape as the .NET analyzer in encryption4all/postguard-dotnet#52: the check rides a command CI already runs.

The practical effect is that an API change cannot reach main without a diff in a file a reviewer reads.

What needs you to apply it

The version half is pnpm api:gate. It classifies the report diff against the merge base of the base ref and HEAD, then compares that with the largest bump the pending changesets declare for @e4a/pg-js:

report diff	required bump
declaration or export removed	major
existing signature or member type changed	major
overload added to or dropped from a set	major
required member added to an interface	major
base class or type parameter list changed	major
value export narrowed to export type	major
alias re-pointed at another declaration	major
trailing optional or rest parameter added	minor
new declaration or export	minor
type-only export widened to a value	minor

Anything not provably additive counts as major. The one exception is the trailing-optional-parameter case, which is the common additive change and would otherwise force a pointless major. When the classifier is wrong about a specific diff, the honest fix is a note on the PR, not a looser rule.

The comparison is against the merge base, not the base tip, so an API change that lands on main after the branch forked is not blamed on the branch. That needs a clone deep enough to find a common ancestor: fetch-depth: 0 in CI. When there is none, the script says so instead of guessing.

api:gate is not wired into CI here, because the App has no workflows permission. The job is in a comment below and needs a commit from you. CLAUDE.md states that it is pending; if you apply the patch onto this branch I will tighten that line before merge.

Verification

Both halves, probed against real commits with scratch branches and reverted afterwards:

probe	result
added export function probeAddition without updating the report	pnpm -r build failed in postbuild
narrowed extractUploadUuid to string, no changeset	failed, required major, declared none
same, with a patch changeset	failed, required major, declared patch
same, with a major changeset	passed
added an optional prefix parameter, patch changeset	failed, required minor, declared patch
same, with a minor changeset	passed
branch with no API change, unrelated API change landed on the base after the fork	passed, no change reported
dropped the middle of three probeOverload overloads	failed, required major
newly exported the already-declared PostGuardBase, patch changeset	failed, required minor
same, with a minor changeset	passed
made RecipientBuilder type-only, minor changeset	failed, required major

tests/api-surface.test.ts adds 43 unit tests over the renderer, the classifier, the base-ref resolution and the changeset reader. Three of them build a real git repo under mkdtemp so the merge-base behaviour is pinned by git rather than by a mock. Full suite is 278 tests in 19 files, green locally on Node 22.23.1, Bun 1.3.14 and Deno 2.9.4. The three git-backed tests spawn git through child_process, so they were worth running on all three runtimes rather than Node alone. CI pins Deno 2.8.0, which only the lane can confirm. pnpm typecheck is clean.

One historical note, from rendering the export list at every tag: v1.0.3 added YiviSessionError in a patch release, and the gate would have asked for a minor. I found no post-1.0 removal, though a name-level scan cannot see signature changes, which is the class of break this snapshot exists to catch.

Review round 2

Four classifier holes found in review, all fixed in 66e8ba4 with a regression test each:

• the diff was taken against the base branch tip, so a branch that changed no public API started demanding a major release as soon as someone else's API change landed on main;
• declarations were keyed by name in a Map, so an overload set or a merged interface collapsed to its last statement and dropping a non-last overload classified as no change at all;
• an added export was never reported, which contradicted both the table above and the report intro;
• the export comparison reduced each side to a set of names, so a value export downgraded to export type and a re-pointed alias were both silent.

Two knock-ons of the first one are in the same commit: the CI patch comment now uses fetch-depth: 0, and the CLAUDE.md bullet no longer calls a depth-1 fetch sufficient.

Two existing test expectations changed, both to include the new export-added line. The class and interface header comparison now dedupes headers, so collapsing two blocks of one interface is not itself a change.

Review round 3

One blocking finding, the collision rename, fixed in 372ef1d with four tests.

A new module that reuses one of the report's internal type names made the gate demand a major release for a purely additive change. rolldown resolves the collision by suffixing one of the two declarations (EmailAttributes$1) and which one it picks follows module order, so keying the comparison on the printed name compared the old declaration against a different one that had inherited its name. Four of the 57 tracked declarations are never named in src/index.ts, so reusing one of those names is not a source-level conflict and nothing in the diff hints that a rename happened. The same keying also erased a genuine break: when the newcomer happens to hold the old shape under the old name, a real required-member removal on the shadowed declaration dropped out of the reported list.

Declarations now carry an id rolldown does not choose. For a declaration named in the export clause it is the public name it is exported under; for the rest it is the route that reaches it from an exported declaration, walked breadth-first, since a declaration is only in the rollup because something exported references it. classify matches the two models on that id, falls back to the printed name for whatever is left over, and rewrites the references inside each printed text to the matched pair before comparing. A rename therefore produces no change lines of its own, only a note that requires no bump and explains the report diff.

Two knock-ons in the same commit. Export aliases are compared through the same pairing, so export { PostGuard$1 as PostGuard } is no longer read as a re-pointed alias, which was a second false major of the same class. The CLAUDE.md section records the rolldown behaviour.

probe, against 0caae41	before	after
new module declaring interface EmailAttributes, re-exported as ProbeEmailAttributes	major, four false lines	minor, the added export
same, newcomer holding the old shape, real domain renamed to domainAttr	major, the domain removal missing from the list	major, the removal named on the declaration it happened to
exported declaration is the suffixed one	major, read as a re-pointed alias	minor, the added export

tests/api-surface.test.ts is now 47 tests. Three of the new ones assert the full detail list rather than the level, and all three fail against 0caae41; a fourth keeps the internal markers out of the rendered report. Full suite is 282 tests in 19 files, green on Node 22.23.1, Bun 1.3.14 and Deno 2.8.0, which is the version CI pins. pnpm typecheck clean, pnpm build green with postbuild --check, and the committed report is byte-identical after --update, so the fix changes no tracked surface.

Not included

No changeset. Nothing here changes the published package (files is ["dist"]), so a release would ship an identical tarball.

[dobby-coder]

CI job patch (needs a maintainer to apply)

The App has no workflows permission, so this half could not go in the commit. It adds an API surface job to integration.yml that runs pnpm api:gate: the report freshness check, then the changeset bump check against the base branch.

Updated in review: the gate now compares against the merge base of the base branch and HEAD, not the base tip, so the job needs real history. fetch-depth: 0 on the checkout, and the base-branch fetch is no longer --depth=1. A depth-1 fetch has no common ancestor to find, and api:gate fails with that message rather than guessing.

The job has no if:, so it reports a status on every PR and can be a required check. On the workflow_call from delivery.yml there is no base ref, main is the fallback, and the comparison comes out empty.

Verified with git apply --check against 0caae41, and the patched file parses as YAML with the expected job and step list.

diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml
index d02bc24..74cecc1 100644
--- a/.github/workflows/integration.yml
+++ b/.github/workflows/integration.yml
@@ -42,6 +42,49 @@ jobs:
               working-directory: packages/pg-js
               run: node scripts/smoke.mjs
 
+    api-surface:
+        name: API surface
+        runs-on: ubuntu-latest
+        steps:
+            - name: Checkout
+              uses: actions/checkout@v6
+              with:
+                  # api:gate compares against the merge base of the base branch
+                  # and HEAD. A shallow clone has no common ancestor to find, and
+                  # the script fails with that message rather than guessing.
+                  fetch-depth: 0
+
+            - name: Setup pnpm
+              uses: pnpm/action-setup@v4
+
+            - name: Setup Node.js
+              uses: actions/setup-node@v6
+              with:
+                  node-version: '22'
+                  cache: 'pnpm'
+
+            - name: Install dependencies
+              run: pnpm install --frozen-lockfile
+
+            # actions/checkout leaves the base branch out of the local refs. On
+            # the workflow_call from delivery.yml there is no base ref, so main is
+            # the fallback and the comparison comes out empty.
+            - name: Fetch the base branch
+              shell: bash
+              run: |
+                  BASE="${GITHUB_BASE_REF:-main}"
+                  git fetch --no-tags origin "+refs/heads/$BASE:refs/remotes/origin/$BASE"
+
+            - name: Build
+              run: pnpm -r build
+
+            # Fails when the report is stale (also caught by postbuild above) or
+            # when the pending changeset is smaller than the API diff needs.
+            - name: Public API surface gate
+              working-directory: packages/pg-js
+              shell: bash
+              run: pnpm api:gate
+
     bun:
         name: Bun ${{ matrix.bun }}
         runs-on: ubuntu-latest

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 1. Self-authored PR, so this is a COMMENT review with the verdict on the first line.

The shape of this is right, and the split the issue asked for is the split that shipped: the freshness half rides postbuild, so it is live on merge across all three runtime lanes with no workflow edit, and only the base-ref half sits in the patch comment. The rule sweep over the CI-gate, patch-in-comment, workflow-shell, required-check and PR-title rules came back clean, and CLAUDE.md correctly marks api:gate as pending rather than live.

What blocks it is the classifier. Four holes, all reproduced against 5c1ec36, three of which make the gate pass silently on exactly the break class the snapshot exists to catch.

1. gate() compares against the base branch tip, not the merge-base, so an untouched PR fails once main moves ahead. Reproduced in a scratch repo: base report on main, branch off, docs-only commit, then land interface Beta on main. --gate --base main on the untouched branch prints major interface `Beta` was removed / major `Beta` is no longer exported, then required bump major, pending changesets declare none, exit 1. This branch happens to sit level with main right now, so it is latent until the next merge.

2. Top-level overload sets collapse. declarations is a Map keyed by name, so a three-overload declare function go renders in the report as only declare function go(a: boolean): void, and dropping the string overload classifies as none with zero changes. Merged interfaces lose their first block the same way.

3. An added export is never reported. Adding an already-declared type to the export clause returns level: 'none', which contradicts both the table in the PR body and the intro this file renders.

4. The export comparison reduces each side to a Set of .name, discarding the isTypeOnly and from fields buildModel records. export { C } → export type { C } is none, and an alias re-target with both types still exported is none.

Inline comments have the reproductions and the fixes. Two knock-ons of (1): the --depth=1 fetch in the patch comment cannot compute a merge-base, and the CLAUDE.md bullet documents that depth-1 fetch as sufficient — both need updating with the fix, in this PR.

One non-blocking note on the dropped-overload test, which currently passes for the wrong reason.

No changeset needed here, and the embargo guard found nothing: no advisory reference in the title, body, or linked issue. Keeping the PR in draft until the classifier holes are closed.

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 2. Self-authored PR, so this is a COMMENT review with the verdict on the first line.

Rule check: clean

I ran the rules that plausibly bear on this diff. None fired.

• ci-gate-can-ride-an-existing-build-script: the split is the one the rule asks for. I confirmed pnpm build fails on a stale report, and prepare still calls tsdown directly, so a fresh pnpm install does not trip the new postbuild.
• patch-in-comment-gate-is-not-live-yet: the branch touches no workflow file (git diff origin/main...HEAD -- .github/ is empty), and CLAUDE.md, the README paragraph and the PR body all scope their claims correctly. CLAUDE.md says api:gate is not yet a CI step and names where the patch lives; the README only claims the pnpm build half, which is live.
• actions-default-shell-has-no-pipefail: both steps with a shell body in the patch declare shell: bash.
• path-filtered-job-cannot-be-a-required-check: the api-surface job carries no on: paths: filter and no if:, so it always reports and can be made required.
• conventional-commit-pr-titles, writing-rules, no-justification-paragraphs-for-simple-changes, vitest-mock-hoisting, single-repo-lessons-belong-in-that-repos-claude-md: nothing. The PR body is 1,040 words with zero em dashes and zero bold spans, inside the caps.

CI is green on all five lanes at 0caae41, and pnpm build passes with postbuild --check on a clean tree.

One blocking finding

The collision-rename bug carried forward from review. I reproduced it end to end myself rather than taking it on trust, and it reproduces exactly as written: adding one module that happens to reuse an existing internal type name makes the gate demand a major for a purely additive change. Details and the reproduction are in the inline comment.

It blocks because of who hits it. The four declarations that are in the report but never named in src/index.ts produce no source-level conflict when a new file reuses their name, so nothing in the diff hints that anything was renamed. The contributor sees a major demanded for adding an export, and the gate's own error message invites them to argue with it on the PR. A gate that is wrong in that shape gets routed around.

One correction to how that finding was written up. It also said the same keying can cancel out a genuine removal and suppress a true major. The first half of that holds and I reproduced it: with a colliding declaration in place, a real required-member removal on the shadowed type vanishes from the reported change list entirely. The second half does not, at least not in any case I could build. The gate still came out major, because a non-re-exported declaration is in the rollup precisely because something references it, and the rename perturbs that reference. So the level stays right while the stated reason is wrong. That is a smaller defect than "suppresses a true major", but it is still a reviewer reading a list that no longer mentions the actual break, so the fix should cover it.

Sending this back to the coder for the keying fix.

[dobby-coder]

VERDICT: approve

Rules Dobby 2, cycle 3. Self-authored PR, so this is a COMMENT review with the verdict on the first line.

Rule check: clean

I ran the rules that plausibly bear on this diff against the round-4 delta and the docs the PR adds. None fired.

• ci-gate-can-ride-an-existing-build-script: the split is the one the rule asks for. The freshness half rides postbuild, which pnpm -r build already invokes on all three CI lanes. Only the version half, which needs a base ref, sits in the comment patch.
• patch-in-comment-gate-is-not-live-yet: the CLAUDE.md bullet says api:gate is NOT yet a CI step and names where the patch lives. I confirmed no workflow file mentions api:gate, api-report or api-surface, so the docs and the tree agree.
• pr-close-issue-keywords: Closes #130. on its own line, same repo, targeting main.
• conventional-commit-pr-titles: pr-title.yml exists here and the title parses.
• single-repo-lessons-belong-in-that-repos-claude-md: all three durable lessons went into this repo's CLAUDE.md, not the shared rule set.
• writing-rules: 474 words of added prose in CLAUDE.md and README, zero em dashes, zero bold spans, no banned words or transitions. Counted rather than estimated.

Both round-4 findings: fixed on the branch in 17964e6

The loop limit is reached, so rather than spend a fourth coder cycle on two one-liners I verified them and fixed them here.

The type-parameter route (the blocking one)

Confirmed against the shipped lib before touching anything. collectReferences walked heritage clauses and members but not statement.typeParameters, so a declaration reachable only that way never got claimed by the BFS in assignIds. It fell through to ?<name> and was matched on the printed name, which is the keying this module exists to replace. On the rolldown collision shape it classified a purely additive change as major:

=== constraint-only reachable ===       === control, also on a member ===
level: major                            level: minor
  major `Constraint.p` was removed        none  ...is now printed as...
  major `Constraint.probeOnly` added      minor interface `Constraint` was added
  major `C` changed its declaration       minor `ProbeConstraint` is now exported

The route was the only difference between those two runs. After the fix the constraint-only case returns exactly the control's result. A type-parameter default reproduced identically and is covered too.

This was latent, not live. None of the 57 tracked declarations is generic, and I confirmed all 57 resolve to a real id with zero ? fallbacks both before and after. But until it landed, the CLAUDE.md bullet and the PR body overstated the guarantee, and now they do not.

The rename note wording

The note read the rollup renamed X to Y, but the branch fires on any matched pair whose printed names differ, including a rename made in src/. That tells a reviewer the bundler did it when nothing in the source changed a name. The two causes are not distinguishable from the two reports alone, so the note now states what was actually observed: `X` is now printed as `Y`. Three existing assertions moved with it.

Verification

• Two regression tests added, one for a constraint and one for a default. I proved they have teeth by disabling the fix with an inverse edit and watching both fail, then restoring it.
• 284 tests in 19 files green, up from 282.
• pnpm typecheck clean. pnpm -r build green, and postbuild --check reports the committed report matches the build, so the fix moves no tracked surface and etc/pg-js.api.md is byte-identical.

Loop limit

This is the third and final rules cycle on this PR. Both outstanding findings are resolved rather than carried, so nothing is left open and the PR is going ready for review.

One thing still needs you. api:gate is not a gate until someone applies the workflow patch from the comment above, because the App cannot push .github/workflows/. The postbuild --check half is live on merge either way. If you apply the patch onto this branch, the CLAUDE.md line saying it is pending should be tightened in the same PR.

[rubenhensen]

Workflow patch applied verbatim onto this branch, so the API surface job now runs. Please re-review the applied commit.

Note the base moved under you: the website import (#134) merged, so main now carries apps/website and this repo has a second workspace package. Nothing in your diff touches it, but a rebase before merge would be worth it — and if the gate should eventually cover more than packages/pg-js, that is worth saying explicitly in the job comment rather than leaving it implied.