build: convert to a pnpm workspace monorepo (packages/pg-js) - #132
Conversation
Workspace scaffold for the monorepo consolidation (#123, #124): pnpm-workspace.yaml with packages/* and apps/*, private root package.json, SDK moved wholesale to packages/pg-js. package.json now carries the real version (2.3.3); the semantic-release placeholder, @semantic-release/exec devDep, npm override and .releaserc.json are gone. client-version test updated to assert the version matches package.json instead of the old dev sentinel.
|
Thanks for the detailed writeup, Ruben! I'm on it — reviewing the monorepo split in #132 now (package moves, the changesets/release-tooling swap, and the CI workflow updates) and I'll follow up with a review directly on this PR. |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
The move itself is clean — git renames kept the history, the workspace layout matches the plan in #123, and all five checks are green including the Bun and Deno legs I would have expected to argue with pnpm's symlinked node_modules. I re-ran the rule sweep against the diff and it came back empty.
Three things were left behind by the move, and one of them ships to npm.
Worth fixing before merge
packages/pg-js/package.jsonneedsrepository.directory. Without it npm resolves the README's relative image path against the repo root, so the logo on the@e4a/pg-jspage 404s from the next publish onward.packages/pg-js/README.mdstill tells contributors that releases come from semantic-release and that development starts withnpm install. That file is the README npm publishes.- The command table at the top of
CLAUDE.mdis still npm-only and still assumes the package sits at the repo root. The Releases/CI section below it was updated carefully; the table every agent reads first was not.
Two optional items are inline: the 0.0.0 guard in generate-version.mjs, and a SHA pin for changesets/action.
Things I checked so you don't have to
- npm's latest
@e4a/pg-jsis 2.3.3, matching the version now inpackages/pg-js/package.json, so the first push to main finds nothing to publish rather than re-publishing. main's ruleset requires one approving review and no status checks, which is what makes the Version Packages PR note below worth a look.- No pnpm "ignored build scripts" warnings in the CI logs, so no dependency needs an
onlyBuiltDependenciesallowlist under pnpm 10.
One thing I could not check: changesets/action can only open the Version Packages PR if "Allow GitHub Actions to create and approve pull requests" is enabled for this repo. My token gets a 403 on that setting. Worth confirming before the first release so it doesn't dead-end.
| "sideEffects": false, | ||
| "description": "Browser SDK for PostGuard — end-to-end encrypted file sharing with identity-based encryption", | ||
| "license": "MIT", | ||
| "repository": { |
There was a problem hiding this comment.
repository needs a "directory": "packages/pg-js" entry now that the package no longer sits at the repo root.
npmjs.com rewrites relative links in a published README against the repository root, honouring repository.directory when it is present. packages/pg-js/README.md:1 embeds ./img/pg_logo.svg, and img/ moved to packages/pg-js/img/ in this PR — so from the next publish the logo on the @e4a/pg-js page 404s, and the "Repository" link on that page points at the monorepo root instead of the package directory.
One line:
"repository": {
"type": "git",
"url": "git+https://github.com/encryption4all/postguard-js.git",
"directory": "packages/pg-js"
},|
|
||
| ## Releasing | ||
|
|
||
| Releases are handled by [semantic-release](https://semantic-release.gitbook.io/) on the `main` branch. When commits land on `main`, semantic-release determines the next version from conventional commit messages and publishes to npm automatically. |
There was a problem hiding this comment.
The README came across verbatim, so this section still says releases are handled by semantic-release from conventional commit messages — the exact thing this PR removes.
This is the README published to npm, so it is the release doc consumers and contributors land on. It should describe the changesets flow (add a changeset, merge, the Version Packages PR publishes), matching the wording you already added to CLAUDE.md:82.
| Install dependencies and build: | ||
|
|
||
| ```bash | ||
| npm install |
There was a problem hiding this comment.
The "## Development" block still says npm install / npm run prebuild / npm run build, and npm run test at :84.
With packageManager: pnpm@10.32.1 at the root and package-lock.json deleted, running npm install in this directory now creates a stray package-lock.json and a non-hoisted tree that diverges from the pnpm workspace CI installs from. Should be pnpm.
(The npm install @e4a/pg-js in Quick Start at :12 is correct — that one is for consumers.)
| Monorepo. `packages/pg-js` = `@e4a/pg-js`, the TypeScript SDK (the only package so far; apps join per encryption4all/postguard-js#123). pnpm workspaces; release via changesets. All SDK commands below run inside `packages/pg-js`. | ||
|
|
||
| ## Build pipeline (gitignored generated sources) | ||
| `src/util/wasm-binary.ts`, `src/yivi/yivi-css-text.ts`, and `src/util/version.ts` are gitignored and generated at build time by `scripts/generate-wasm-base64.mjs`, `scripts/generate-yivi-css.mjs`, and `scripts/generate-version.mjs`. Tests transitively import them. `prebuild`, `pretypecheck`, `pretest`, and `pretest:watch` all run all three generators, so a fresh-clone `npm test` works; CI runs `typecheck` before `test`. |
There was a problem hiding this comment.
This line still says a fresh-clone npm test works, and the same npm assumption survives in two more places outside this hunk:
- the "Common commands" table at :13–:18 (
npm install,npm run build,npm run dev,npm run typecheck,npm test,npm run test:watch) - :29 (
npm run dev,npm run prebuild,npm run build,npm test)
Post-move these need to be pnpm and to say they run from packages/pg-js — the root build/test/typecheck are pnpm -r wrappers, so pnpm dev and pnpm exec vitest run tests/api.test.ts only work from the package directory. The Releases/CI section and the agent notes below got updated; the table at the top is the part an agent reads first.
| // the semantic-release placeholder (or any 0.0.0 sentinel). | ||
| // 1. process.env.PG_JS_VERSION — optional override for special builds. | ||
| // 2. package.json `version` — the real version, maintained by changesets. | ||
| // 3. `0.0.0-dev` fallback — for dev builds with a 0.0.0 sentinel version. |
There was a problem hiding this comment.
Nit on the guard this comment describes (!v.startsWith('0.0.0') at :21).
With semantic-release gone, package.json's version is authoritative and the only value worth rejecting is the placeholder, which the v !== PLACEHOLDER check on the same line already covers. Keeping the startsWith('0.0.0') rejection means a genuine 0.0.0-* version — a changeset pre prerelease on a future workspace package that has not reached 0.0.1 — would ship a build whose X-POSTGUARD-CLIENT-VERSION header reads 0.0.0-dev while npm carries the real version. The new assertion at tests/client-version.test.ts:26 would then fail with no obvious cause.
Suggest dropping the startsWith guard. PLACEHOLDER at :13 is also now a dead reference to removed tooling.
|
|
||
| - name: Release | ||
| run: npx semantic-release | ||
| - name: Version or publish (changesets) |
There was a problem hiding this comment.
Note rather than a blocker: the "Version Packages" PR that changesets/action opens gets no pre-merge CI. PRs created with the default GITHUB_TOKEN do not trigger pull_request-triggered workflows, so neither integration.yml nor pr-title.yml runs on it, and main's ruleset requires an approving review but no status checks.
The publish itself is still gated — needs: integration on this job means the push of the version-bump commit to main runs the full matrix before the release step, same as under semantic-release. So the gap is the review-time signal on that PR, not an unverified publish.
If you want checks on it anyway, the usual fix is a PAT or GitHub App token for the action so the PR triggers workflows; a push-on-changeset-release/main trigger for integration.yml also works and keeps the token as-is.
| - name: Release | ||
| run: npx semantic-release | ||
| - name: Version or publish (changesets) | ||
| uses: changesets/action@v1 |
There was a problem hiding this comment.
changesets/action@v1 is a mutable major tag on a third-party action, running in the one job that holds id-token: write, contents: write and npm publish authority.
The repo pins every other action by major tag too, so this is a consistency call rather than a defect — but this is the only step that can push to npm, and the package publishes with provenance. Pinning to a full commit SHA (changesets/action@<sha> # v1.x.y) removes the retag risk on that step specifically.
|
Reviewed and requested changes on the PR review — the move itself is clean and everything checks out (all 5 CI legs green, 235/235 tests, versions line up so the first push won't misfire a publish), but a few doc/metadata items got left behind by the migration:
Plus two optional nits (a redundant One thing I couldn't verify myself: |
…sion guard, action pin, CI on version PR
|
All review items addressed in the latest commit:
235/235 tests + typecheck re-verified locally after the version-guard change. |
|
/dobby all seven review items are addressed on the branch (summary in my comment above) — please re-review. |
|
Got it — thanks for the summary! I'm re-reviewing PR #132 now that all seven items have been addressed. |
There was a problem hiding this comment.
Re-review of the branch after the seven-item round. I checked out 0a5299b and ran everything: pnpm install --frozen-lockfile clean (prepare builds 2.27 MB dist), pnpm -r typecheck exit 0, 235/235 vitest tests pass. The move itself is clean — git detected the renames, so src/, tests/, tsconfig.json, tsdown.config.ts and vitest.config.ts all show zero content drift. .releaserc.json, the 0.0.0-managed-by-semantic-release placeholder and the overrides.npm CVE pin are all correctly gone; grep undici pnpm-lock.yaml and grep semantic-release pnpm-lock.yaml both return 0, so that override had nothing left to pin. The changesets/action SHA does deref to v1.9.0.
Requesting changes on two items, both small. The rest is non-blocking.
Blocking
-
integration.yml— the newpush: branches: [changeset-release/main]trigger will not fire. GitHub suppresses workflow runs for all events triggered byGITHUB_TOKENexceptworkflow_dispatch/repository_dispatch, not justpull_request. The version PR still gets no CI signal, and the comment on lines 6-8 states the restriction too narrowly. Either pass a PAT/App token tochangesets/actionviagithub-token(at which point the existingpull_requesttrigger covers it and thepushtrigger can go), or drop the trigger and the comment rather than leave a mechanism that silently does nothing. -
No changeset file in the PR —
pnpm changeset statusexits 1 with "Some packages have been changed but no changesets were found" (verified locally). Nothing in CI gates on it, but it means the two npm-facing fixes from the last round,repository.directoryand the rewritten package README, do not reach npm until some unrelated future release happens to ship. A patch changeset here makes them land.
Non-blocking — five inline notes: the publishConfig.provenance mechanism is misattributed (pnpm doesn't read it), the client-version test can't survive its own documented PG_JS_VERSION override, two CLAUDE.md docs slips, one registry-url gotcha worth knowing if the first trusted publish is rejected, and the root README dropping the standard encryption4all format.
| # The Version Packages PR is opened with GITHUB_TOKEN, which triggers no | ||
| # pull_request workflows — build its branch on push instead so the PR | ||
| # still carries a CI signal. | ||
| push: |
There was a problem hiding this comment.
Blocking. This trigger will not fire, so the Version Packages PR still gets no CI signal.
changesets/action pushes changeset-release/main using GITHUB_TOKEN (default commit-mode: git-cli, and GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} is what delivery.yml passes it). GitHub suppresses workflow runs for all events triggered by GITHUB_TOKEN except workflow_dispatch and repository_dispatch — not just pull_request. That is the part the comment on lines 6-8 gets wrong: it states the restriction as pull_request-only, which is why push looks like a way around it.
Getting CI onto the version PR needs a PAT or GitHub App token handed to changesets/action via its github-token input. At that point the existing pull_request trigger already covers the branch and this push trigger can go away. Otherwise please drop the trigger and the comment — a mechanism that silently does nothing is worse than the known gap, because the PR body now claims the version PR carries a CI signal.
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
Blocking. There is no changeset file in this PR, so pnpm changeset status fails:
🦋 error Some packages have been changed but no changesets were found. Run `changeset add` to resolve this error.
(verified locally at this HEAD, exit 1.) Nothing in CI gates on it, so it will not go red — but the consequence is that the two npm-facing fixes from the last round never ship: repository.directory, added specifically so the README logo stops 404ing on npmjs.com, and the rewritten package README. Both sit in packages/pg-js waiting for a version bump that only arrives when some unrelated future PR happens to add a changeset.
A patch changeset here makes the fix actually land, and it exercises the new release path end to end on the PR that introduces it.
| ], | ||
| "publishConfig": { | ||
| "access": "public", | ||
| "provenance": true |
There was a problem hiding this comment.
Non-blocking, but the PR description credits the wrong mechanism: publishConfig.provenance: true is inert under pnpm.
changeset publish shells out to pnpm publish --access public --tag latest [--no-git-checks] — confirmed in @changesets/cli/dist, and note it passes no --provenance. pnpm 10.32 then resolves provenance solely from the provenance npm-config key (CLI flag / npm_config_provenance / npmrc); its publishConfig-override whitelist is manifest fields only (bin, engines, type, main, module, types, exports, os, cpu, …), and provenance is not in it. I checked the whitelist in the installed pnpm.
In practice provenance will still be attached if the npm trusted-publisher binding works, since pnpm does its own OIDC token exchange. The risk is that there is no fail-loud if OIDC silently drops out. Setting NPM_CONFIG_PROVENANCE: true in the release step's env makes pnpm honour it directly and hard-error with "Provenance generation in GitHub Actions requires write access to the id-token permission" if the permission ever goes missing.
| // version.ts is generated from package.json, which carries the real | ||
| // version now that changesets maintains it (no more release placeholder). | ||
| const pkg = JSON.parse(readFileSync(new URL('../package.json', import.meta.url), 'utf8')); | ||
| expect(appVersion).toBe(pkg.version); |
There was a problem hiding this comment.
This assertion ignores the PG_JS_VERSION env override that generate-version.mjs documents as resolution step 1, so the override path is untestable — PG_JS_VERSION=9.9.9-test pnpm test fails:
AssertionError: expected '9.9.9-test' to be '2.3.3'
(verified locally: pretest regenerates version.ts from the env var, then the test compares against package.json.) Keeping both paths green:
expect(appVersion).toBe(process.env.PG_JS_VERSION?.trim() || pkg.version);| @@ -1,93 +1,10 @@ | |||
| # <p align="center"><img src="./img/pg_logo.svg" height="128px" alt="PostGuard" /></p> | |||
| # postguard-js | |||
There was a problem hiding this comment.
Rule check (standardized-readmes): the root README no longer follows the encryption4all format. Every repo's README is meant to carry the PostGuard logo, the > For full documentation, visit docs.postguard.eu/repos/postguard-js line, a ## Development section, a ## Releasing section, and ## License (MIT). The old root README had all six; this one has none of them, and img/pg_logo.svg moved under packages/pg-js/ so there is no logo asset at the root any more.
Unlike the a11y-style findings on a move PR, this is not carried-forward debt — the file that complied is the one this PR replaced, so the repo front page regressed. packages/pg-js/README.md correctly keeps the full format, so the fix is small: copy img/pg_logo.svg back to the root, and add the logo, docs link, and a short License line to this index. Development/Releasing can reasonably just point at packages/pg-js.
| Monorepo. `packages/pg-js` = `@e4a/pg-js`, the TypeScript SDK (the only package so far; apps join per encryption4all/postguard-js#123). pnpm workspaces; release via changesets. All SDK commands below run inside `packages/pg-js`. | ||
|
|
||
| ## Build pipeline (gitignored generated sources) | ||
| `src/util/wasm-binary.ts`, `src/yivi/yivi-css-text.ts`, and `src/util/version.ts` are gitignored and generated at build time by `scripts/generate-wasm-base64.mjs`, `scripts/generate-yivi-css.mjs`, and `scripts/generate-version.mjs`. Tests transitively import them. `prebuild`, `pretypecheck`, `pretest`, and `pretest:watch` all run all three generators, so a fresh-clone `npm test` works; CI runs `typecheck` before `test`. |
There was a problem hiding this comment.
One npm reference the pnpm conversion missed: "…all run all three generators, so a fresh-clone npm test works". The adjacent sentence on line 98 was converted to pnpm, so this reads as an oversight rather than a deliberate npm-consumer mention like the package README's Quick Start.
|
|
||
| ## Overview | ||
| `@e4a/pg-js`, the TypeScript SDK. Release: semantic-release. | ||
| Monorepo. `packages/pg-js` = `@e4a/pg-js`, the TypeScript SDK (the only package so far; apps join per encryption4all/postguard-js#123). pnpm workspaces; release via changesets. All SDK commands below run inside `packages/pg-js`. |
There was a problem hiding this comment.
Contradicts line 11. Line 11 says "All commands from the repo root unless noted"; this line says "All SDK commands below run inside packages/pg-js". Both describe command working directories in the same file. Since the Package scripts list at lines 104-108 is what "below" actually refers to, saying so explicitly — or dropping the sentence and leaning on the line-11 rule — avoids the clash.
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: |
There was a problem hiding this comment.
Worth knowing for the trusted-publishing fallback, no change needed now. registry-url makes setup-node write a project-level .npmrc containing //registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}, and pnpm 10 deliberately ignores project-level registry credentials. I confirmed the warning text in the installed pnpm: "Ignored project-level auth setting … environment variables are not expanded in registry credentials that come from a project .npmrc" — it warns and continues unauthenticated.
Harmless while OIDC does the auth. But if the first real publish is rejected and you reach for an NPM_TOKEN secret, adding NODE_AUTH_TOKEN will silently not work; it needs pnpm config set or a user-level ~/.npmrc instead.
|
Re-reviewed at Two blockers, both small:
Six non-blocking notes too, including |
… provenance mechanism, doc fixes
|
/dobby round 2 addressed:
Please re-review. |
|
On it — kicking off round 2 review of PR #132 now, checking the CI trigger decision, the new patch changeset, the provenance env var fix, the client-version test, CLAUDE.md, and the README restore. Will follow up with a consolidated review shortly. |
There was a problem hiding this comment.
Approving. Both round-2 blockers are resolved, and I re-ran the pipeline against HEAD 82717a2 rather than trusting the checklist.
What I verified myself
- Clean clone,
pnpm install --frozen-lockfileon pnpm 10.32.1 (resolved from thepackageManagerpin): thepreparehook fires inpackages/pg-js, runs all three generators and tsdown. The README line "runs the prebuild generators + a full build viaprepare" is accurate. pnpm -r typecheckclean;pnpm -r testgives 18 files / 235 tests passing.pnpm -rskips the workspace root, so thepnpm -r buildwrapper does not recurse into itself.changesets/action@a45c4d5dereferences to the commit behind tagv1.9.0. Pin is correct.packages/pg-js/package.jsonat 2.3.3 matchesdist-tags.lateston npm, so the firstchangeset versionlands on 2.3.4 with no gap.- Dropping the root
overrides: { npm: "^11.18.0" }is right. That pin only existed because@semantic-release/npmpulled thenpmpackage, and its bundled undici, into the tree.npm,undiciandsemantic-releaseare all absent frompnpm-lock.yamlnow, so the CVE chain is gone with the tooling rather than being silently unpinned. changeset publishappends--no-git-checksfor pnpm >= 5 (getPublishToolin @changesets/cli 2.31.1), so the publish will not trip pnpm's clean-tree check on the runner.pnpm/action-setupruns beforesetup-nodein all four jobs, which is whatcache: 'pnpm'needs.id-token: writeis on the workflow, so OIDC trusted publishing keeps working.
Four notes inline, none blocking.
One thing that is not a code change: the org standardized-README rule assumes a root img/pg_logo.svg. The root README now points at packages/pg-js/img/pg_logo.svg, which renders correctly on GitHub, and repository.directory makes npmjs.com resolve the package README's ./img/... back to the same file. The rule is what needs updating for a monorepo shape, not this PR.
| - name: Version or publish (changesets) | ||
| uses: changesets/action@a45c4d594aa4e2c509dc14a9f2b3b67ba3780d0d # v1.9.0 | ||
| with: | ||
| version: pnpm version-packages |
There was a problem hiding this comment.
version: pnpm version-packages runs changeset version on its own, so the Version Packages PR never refreshes pnpm-lock.yaml.
Harmless today, and I confirmed it. Running pnpm version-packages at this HEAD touches only packages/pg-js/package.json (2.3.3 to 2.3.4), deletes the changeset, and writes a new CHANGELOG.md. The lockfile is byte-identical afterwards and pnpm install --frozen-lockfile still reports "Lockfile is up to date". A lockfile importer records dependency specifiers, not the package's own version, so nothing goes stale.
It starts to bite once apps/* land, which is why pnpm-workspace.yaml:3 already lists them. As soon as an app depends on @e4a/pg-js with a plain range rather than workspace:*, changeset version rewrites that range, the lockfile goes stale in the version commit, and the release job fails at pnpm install --frozen-lockfile with ERR_PNPM_OUTDATED_LOCKFILE, by which point the tag and PR are already in flight.
The changesets pnpm recipe wraps both steps: version: pnpm version-packages && pnpm install --lockfile-only, or fold the install into the version-packages script. Cheap to add now, awkward to debug later.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '24' |
There was a problem hiding this comment.
Closing the round-2 "watch the first real publish" item. I traced the publish path: the OIDC trusted-publishing exchange is npm's, not pnpm's, and it already works.
- Registry metadata for the current release shows the mechanism in use.
@e4a/pg-js@2.3.3was published by_npmUser: GitHub Actions <npm-oidc-no-reply@github.com>withtrustedPublisher: {id: github}, anddist.attestations.provenancecarries a SLSA v1 predicate. - pnpm 10.32.1, the version this pin resolves to, has no OIDC code.
ACTIONS_ID_TOKENdoes not appear anywhere in its 7.8 MB bundle.pnpm publishpacks a tarball and shells out throughrunNpm(npmPath, ...), whereconst npm = npmPath ?? "npm", so it lands on whatevernpmis on PATH. - The token exchange lives in npm's own
lib/utils/oidc.jsand needs npm >= 11.5.1.
So the publish now depends on whichever npm ships with the runner's Node 24, currently 11.x, which is fine. The deleted root overrides: { npm: "^11.18.0" } used to make that version explicit as a side effect. No change needed; if you want the floor back, npm i -g npm@11 before the changesets step restores it.
The same trace confirms the round-2 provenance fix is real rather than cargo-culted: provenance: Boolean is in pnpm's rc option types (defaulting to false), and pnpm inherits the environment into the npm child either way, so NPM_CONFIG_PROVENANCE: 'true' is picked up.
| @@ -26,7 +28,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co | |||
| - `scripts/generate-wasm-base64.mjs` — reads `node_modules/@e4a/pg-wasm/web/index_bg.wasm`, writes `src/util/wasm-binary.ts` (base64 of the WASM) AND `src/util/pg-wasm-shim.js` (a patched copy of pg-wasm's `index.js` with wasm-bindgen's `new URL("index_bg.wasm", import.meta.url)` default-value branch stripped — that branch never fires at runtime but webpack 5 fails on it because no separate WASM file ships in our dist). | |||
There was a problem hiding this comment.
Anchoring here because the line itself sits just outside the hunk: CLAUDE.md:26 says "prebuild, pretypecheck, and pretest all run two generator scripts", and only two bullets follow. There are three. Line 31 immediately below says "All three generated files", and the agent notes at line 96 name generate-version.mjs as the third.
Pre-existing, and untouched by this PR, so not a regression. Worth the one added bullet anyway, because generate-version.mjs is exactly the script this PR changes the semantics of: it now reads the real package.json version that changesets maintains, instead of taking it from a semantic-release prepareCmd. A later agent reading this section would not learn that script exists.
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
Heads-up on a behaviour change nothing in the diff mentions: release tag naming changes.
semantic-release tagged v2.3.3, v2.3.2 and so on, and those tags are on the repo now. In a pnpm workspace, changesets tags <name>@<version>: @changesets/cli 2.31.1 computes tool !== "root" ? \${name}@${newVersion}` : `v${newVersion}`, and toolis"pnpm"here. So the next release tags and GitHub-releases as@e4a/pg-js@2.3.4, not v2.3.4`.
Nothing in the repo references the old scheme, so this breaks no build. It does split the releases page into two naming eras and would break any external link built as /releases/tag/v<version>. Fine if that is the intended trade for the monorepo shape; just worth knowing before the first publish rather than after.
|
Round 2 changes look solid — re-reviewed against a clean clone at Both prior blockers are resolved (no pre-merge CI on the Version Packages PR is documented and gated correctly, and the patch changeset lands 2.3.4 with Left 4 non-blocking notes inline for later:
Also flagged (not a code issue): the root README pointing at |
…eck real Addresses the review on #144. The `image` job had no `needs:`, so it published in parallel with `check`, `test`, `urls` and `nginx` — a red `urls` would not have stopped the same bundle shipping, inverting the thing this workflow is built around. Gated on all four. `platforms: linux/amd64,linux/arm64` drops back to amd64, which is what the standalone repo actually published. arm64 was new here, there is no setup-qemu-action, and nothing exercised it before a tag — so the first run of an emulated production webpack over a WASM-embedding bundle would have been the release itself. The `urls` job now reads the two files that carry the hostnames as well as this workflow's env. #132 was a wrong `ARG` default in the Dockerfile, which is exactly the copy a workflow-only check cannot see. It also gained a lookup fallback chain and a guard that fails when no DNS tool is present: my first local test of the check was invalid because macOS has no `getent`, which made every host look dead — the same single-tool assumption would have made the job vacuously green on a runner image without it. Verified by reintroducing #132's exact bug: the check catches `fileshare.postguard.eu` and passes the four live hosts. `check` built with `ADDIN_PUBLIC_URL` unset, so webpack fell back to the production add-in origin and validated a manifest pairing that origin with staging backends — a combination that never ships, and the value that drives the manifest rewrite `pnpm validate` then checks. CI moves to Node 24, matching the Dockerfile stage that builds the shipped bundle and both sibling app workflows. apps/outlook-addon/.dockerignore is deleted: Docker only reads the one at the context root, so it went dead when the context moved. `**/.env` and `**/.env.*` added to the root file, which it used to cover. Changesets for the add-in and for postguard-website, so this lands in one merge rather than two — the sync-addons retarget is a real behaviour change to the download page. Noted in the workflow: until the standalone repo's release.yml is neutralised, both repos push `:edge` and `:latest` to the same package.
Six review notes from the approving round, each verified by running it rather than reading the diff for it. Validate dist/manifest.xml. `pnpm validate` checked the source manifest, which still carries the localhost URLs, so the copy the release job attaches and admins sideload was never validated by anything. Adds `validate:dist` and points CI at it. Scope the shipped AppDomains to the build's own origins. The source manifest lists the production and staging add-in origins so either can be built from it, and the localhost entry is rewritten per target, so every production manifest allowlisted addin.staging.postguard.eu, staging.postguard.eu and two spellings of its own origin. A production build now also fails outright when an origin it needs is missing: an absent AppDomain breaks displayDialogAsync at send time, inside Outlook, which no CI job observes. Mutation-tested by removing the yivi.app entry -- webpack exits 1. Guard the two files the URL check greps. `set -e` is deliberately absent there, so grep's exit 2 on a missing file did not abort and a rename would have degraded the job to an env-only check that still passed -- with no visible change, because those files currently duplicate the env values exactly, so even the host count stayed at 6. Re-tested #132's exact bug afterwards: still caught, still exit 1. Move the Dockerfile ARG/ENV below the install. A build arg participates in the cache key of every instruction after its ARG, so the edge and tag builds -- which differ only in those four values -- could never share the install layer. Verified both ways with two consecutive builds: before, the second re-ran `pnpm install` in 27.7s; after, that layer is CACHED and only webpack re-runs. Build via the package script in the image. `exec webpack --mode production` duplicated what `build` already defines, so the image could drift from what CI and the release job produce. Keep the addon mirror quiet before the first monorepo release, without making it quiet afterwards. Repointing the outlook target at this repo left nothing matching outlook-addin-v* until the first tag, which the sync reported as a failure every 6h. It now skips once -- but only while no matching release has ever been mirrored. Once one has, the cached tag matches the pattern and a pattern that stops matching fails loudly, so the skip clears itself rather than becoming a permanent hole. Not changed, and deliberately: apps/tb-addon/.env.example was flagged as carrying a dead Cryptify host, but fileshare.staging.postguard.eu does resolve -- only the production fileshare.* host is dead. Refs #144
Five findings from the second approving round, plus the version decision that was held back from the last commit. Sync manifest <Version> from package.json, and re-baseline to 1.0.0. Outlook keys sideloaded and centrally-deployed updates on that field; it was hardcoded at 1.0.0.0 while package.json said 0.4.0, so every release shipped a manifest an admin could not distinguish from the previous one. The two cannot simply be synced: office-addin-manifest rejects any <Version> below 1.0, so 0.4.0 -> 0.4.0.0 fails validation (measured: exit 1, "Manifest Version Too Low"). package.json now matches what the manifest has always declared, `version-packages` syncs it on every bump, and CI refuses a commit where the two disagree. The add-in changeset is dropped in favour of setting 1.0.0 directly, so the first monorepo release can be tagged outlook-addin-v1.0.0 rather than needing a version round trip past it. Take the release build out of the publish path. `pnpm release` was `pnpm -r build && changeset publish`, so all three apps' build preconditions sat in front of the pg-js npm publish -- and the previous commit made that worse, because the add-in's build now fail-closes on the VALUE of POSTGUARD_WEBSITE_URL, with the set of acceptable values living in a manifest in a different app. Setting vars.POSTGUARD_WEBSITE_URL to an origin that manifest does not list would have failed `pnpm -r build` before `changeset publish` and blocked the pg-js release. @e4a/pg-js is the only publishable package here, so the build is now filtered to it, which removes the class rather than enumerating it. `needs: integration` still runs the full recursive build before anything is released. Build both origin sets in `check`. Only edge was built, so the new manifest assertion never ran against the production config on a PR: drop a production AppDomain and the PR stayed green, with the failure landing in the tag-time image build instead. Both are now built and both are validated, for a few seconds of webpack. Validate the manifest that is actually released. `validate:dist` ran only in `check`, against the edge build, and the two differ in exactly what validation inspects -- SourceLocation, icon origins, scoped AppDomains. So the previous commit's claim that the sideloaded file is the validated one was true of the edge artifact and not the released one. Derive the URL check's host list from the files instead of a pattern. `https://[a-z0-9.-]+\.postguard\.eu` required a subdomain label, so it matched neither file's bare-apex defaults; postguard.eu was checked only because PROD_WEBSITE_URL happens to hold the same string, which is the coincidence this job exists to stop depending on. It also could not see a default that drifted out of the family, and missed yivi.app, which the build now refuses to produce a manifest without. Re-mutation-tested: renaming the Dockerfile, #132's dead subdomain, and a bare-apex default drifting to a dead domain outside the family all exit 1 -- the last of those was missed before. Plus a guard against an empty host list, since `grep -v` filtering everything would otherwise leave a green job. Build the image on PRs, without pushing. `image` only runs on a push, so the Dockerfile -- repo-root context, new COPY paths, ARGs moved below the install -- was never built before merge. Mirrors website.yml, needs no package write grant, and makes the ARG-placement cache win observable in CI. Verified the built artifact rather than the config: the manifest inside the image carries <Version>1.0.0.0</Version> and exactly the three production AppDomains. Refs #144
* feat(outlook-addon): move CI and releases into the monorepo B4 part two, completing #127. Part one (#141) brought the source and the workspace SDK; this brings the pipeline. New .github/workflows/outlook-addon.yml, path-unfiltered so an SDK change is tested against this consumer: - check (lint, prettier, typecheck, build, manifest validate), test, nginx — ported from the old repo's ci.yml onto pnpm - image: :edge on push to main, :<version> and :latest on an outlook-addin-v* tag, with the production manifest.xml attached to the release - urls: asserts every host baked into the bundle actually resolves That last job is new, and it exists because nothing caught postguard-outlook-addon#132: the production add-in shipped CRYPTIFY_URL=https://fileshare.postguard.eu, a hostname that does not resolve, so file sending failed for every user while CI stayed green and the add-in loaded fine. The check is a DNS lookup per configured host and fails the PR instead of a user. The image name is hardcoded rather than derived from github.repository, which resolves to postguard-js here while postguard-ops pins postguard-outlook-addon. Release tags are app-scoped, since a bare v* would collide with @e4a/pg-js's changesets tags and the pre-monorepo v2.3.3-style ones. release-please's config and manifest are deleted; changesets owns the version. The Dockerfile is reworked the way the website's was: repo-root context, pnpm workspace install, pg-js built before the app. Its build stage moves from node:20-alpine to node:24-alpine because pg-js declares engines node >=22, which is what the first build attempt failed on. Also corrected the dead Cryptify hostname in this copy. The fix in postguard-outlook-addon#133 did not reach here: the import predated it, so the two diverged immediately. Verified by building the image and reading the artifact — both bundles reference storage.postguard.eu and the manifest carries no localhost or fileshare strings at all. apps/website/scripts/sync-addons.mjs mirrors the Outlook manifest for the download page; its target now points at this repo with a tagPattern, because a bare manifest.xml pattern over a shared release namespace would consider every pg-js release. * fix(outlook-addon): gate the image on the checks, and make the URL check real Addresses the review on #144. The `image` job had no `needs:`, so it published in parallel with `check`, `test`, `urls` and `nginx` — a red `urls` would not have stopped the same bundle shipping, inverting the thing this workflow is built around. Gated on all four. `platforms: linux/amd64,linux/arm64` drops back to amd64, which is what the standalone repo actually published. arm64 was new here, there is no setup-qemu-action, and nothing exercised it before a tag — so the first run of an emulated production webpack over a WASM-embedding bundle would have been the release itself. The `urls` job now reads the two files that carry the hostnames as well as this workflow's env. #132 was a wrong `ARG` default in the Dockerfile, which is exactly the copy a workflow-only check cannot see. It also gained a lookup fallback chain and a guard that fails when no DNS tool is present: my first local test of the check was invalid because macOS has no `getent`, which made every host look dead — the same single-tool assumption would have made the job vacuously green on a runner image without it. Verified by reintroducing #132's exact bug: the check catches `fileshare.postguard.eu` and passes the four live hosts. `check` built with `ADDIN_PUBLIC_URL` unset, so webpack fell back to the production add-in origin and validated a manifest pairing that origin with staging backends — a combination that never ships, and the value that drives the manifest rewrite `pnpm validate` then checks. CI moves to Node 24, matching the Dockerfile stage that builds the shipped bundle and both sibling app workflows. apps/outlook-addon/.dockerignore is deleted: Docker only reads the one at the context root, so it went dead when the context moved. `**/.env` and `**/.env.*` added to the root file, which it used to cover. Changesets for the add-in and for postguard-website, so this lands in one merge rather than two — the sync-addons retarget is a real behaviour change to the download page. Noted in the workflow: until the standalone repo's release.yml is neutralised, both repos push `:edge` and `:latest` to the same package. * fix(ci): run the URL check from the repo root, and scope the .env exclusion Two self-inflicted failures from the previous commit. The `Baked URLs resolve` job inherits this workflow's `defaults.run.working-directory: apps/outlook-addon`, so the file paths in the check resolved one level deep and grep exited 2 (file not found) instead of matching anything. Pinned to the repo root. The irony is that the job existed to stop a silent config mistake and shipped with one. Adding `**/.env` to the root .dockerignore broke the website image build with "Missing required environment variable: VITE_FILEHOST_URL". apps/website commits its .env deliberately — it carries the placeholder the container entrypoint substitutes at start — so the exclusion has to be scoped to the add-in, which is what the app-local .dockerignore did before the context moved. Verified both: the website's frontend stage builds again, and the check finds all five hosts from the repo root. * fix(outlook-addon): gate the shipped manifest, not the source one Six review notes from the approving round, each verified by running it rather than reading the diff for it. Validate dist/manifest.xml. `pnpm validate` checked the source manifest, which still carries the localhost URLs, so the copy the release job attaches and admins sideload was never validated by anything. Adds `validate:dist` and points CI at it. Scope the shipped AppDomains to the build's own origins. The source manifest lists the production and staging add-in origins so either can be built from it, and the localhost entry is rewritten per target, so every production manifest allowlisted addin.staging.postguard.eu, staging.postguard.eu and two spellings of its own origin. A production build now also fails outright when an origin it needs is missing: an absent AppDomain breaks displayDialogAsync at send time, inside Outlook, which no CI job observes. Mutation-tested by removing the yivi.app entry -- webpack exits 1. Guard the two files the URL check greps. `set -e` is deliberately absent there, so grep's exit 2 on a missing file did not abort and a rename would have degraded the job to an env-only check that still passed -- with no visible change, because those files currently duplicate the env values exactly, so even the host count stayed at 6. Re-tested #132's exact bug afterwards: still caught, still exit 1. Move the Dockerfile ARG/ENV below the install. A build arg participates in the cache key of every instruction after its ARG, so the edge and tag builds -- which differ only in those four values -- could never share the install layer. Verified both ways with two consecutive builds: before, the second re-ran `pnpm install` in 27.7s; after, that layer is CACHED and only webpack re-runs. Build via the package script in the image. `exec webpack --mode production` duplicated what `build` already defines, so the image could drift from what CI and the release job produce. Keep the addon mirror quiet before the first monorepo release, without making it quiet afterwards. Repointing the outlook target at this repo left nothing matching outlook-addin-v* until the first tag, which the sync reported as a failure every 6h. It now skips once -- but only while no matching release has ever been mirrored. Once one has, the cached tag matches the pattern and a pattern that stops matching fails loudly, so the skip clears itself rather than becoming a permanent hole. Not changed, and deliberately: apps/tb-addon/.env.example was flagged as carrying a dead Cryptify host, but fileshare.staging.postguard.eu does resolve -- only the production fileshare.* host is dead. Refs #144 * fix(outlook-addon): sync the manifest version, and close four gate gaps Five findings from the second approving round, plus the version decision that was held back from the last commit. Sync manifest <Version> from package.json, and re-baseline to 1.0.0. Outlook keys sideloaded and centrally-deployed updates on that field; it was hardcoded at 1.0.0.0 while package.json said 0.4.0, so every release shipped a manifest an admin could not distinguish from the previous one. The two cannot simply be synced: office-addin-manifest rejects any <Version> below 1.0, so 0.4.0 -> 0.4.0.0 fails validation (measured: exit 1, "Manifest Version Too Low"). package.json now matches what the manifest has always declared, `version-packages` syncs it on every bump, and CI refuses a commit where the two disagree. The add-in changeset is dropped in favour of setting 1.0.0 directly, so the first monorepo release can be tagged outlook-addin-v1.0.0 rather than needing a version round trip past it. Take the release build out of the publish path. `pnpm release` was `pnpm -r build && changeset publish`, so all three apps' build preconditions sat in front of the pg-js npm publish -- and the previous commit made that worse, because the add-in's build now fail-closes on the VALUE of POSTGUARD_WEBSITE_URL, with the set of acceptable values living in a manifest in a different app. Setting vars.POSTGUARD_WEBSITE_URL to an origin that manifest does not list would have failed `pnpm -r build` before `changeset publish` and blocked the pg-js release. @e4a/pg-js is the only publishable package here, so the build is now filtered to it, which removes the class rather than enumerating it. `needs: integration` still runs the full recursive build before anything is released. Build both origin sets in `check`. Only edge was built, so the new manifest assertion never ran against the production config on a PR: drop a production AppDomain and the PR stayed green, with the failure landing in the tag-time image build instead. Both are now built and both are validated, for a few seconds of webpack. Validate the manifest that is actually released. `validate:dist` ran only in `check`, against the edge build, and the two differ in exactly what validation inspects -- SourceLocation, icon origins, scoped AppDomains. So the previous commit's claim that the sideloaded file is the validated one was true of the edge artifact and not the released one. Derive the URL check's host list from the files instead of a pattern. `https://[a-z0-9.-]+\.postguard\.eu` required a subdomain label, so it matched neither file's bare-apex defaults; postguard.eu was checked only because PROD_WEBSITE_URL happens to hold the same string, which is the coincidence this job exists to stop depending on. It also could not see a default that drifted out of the family, and missed yivi.app, which the build now refuses to produce a manifest without. Re-mutation-tested: renaming the Dockerfile, #132's dead subdomain, and a bare-apex default drifting to a dead domain outside the family all exit 1 -- the last of those was missed before. Plus a guard against an empty host list, since `grep -v` filtering everything would otherwise leave a green job. Build the image on PRs, without pushing. `image` only runs on a push, so the Dockerfile -- repo-root context, new COPY paths, ARGs moved below the install -- was never built before merge. Mirrors website.yml, needs no package write grant, and makes the ARG-placement cache win observable in CI. Verified the built artifact rather than the config: the manifest inside the image carries <Version>1.0.0.0</Version> and exactly the three production AppDomains. Refs #144 * fix(outlook-addon): validate the release manifest before publishing the image On a tag the image published `:<version>` and `:latest` before the released manifest was built and validated, so a validation failure left the image live with no GitHub release and no manifest attached. That state is quiet in a way the previous commit made worse: admins keep sideloading the old manifest against a new bundle, and sync-addons.mjs finds nothing matching `outlook-addin-v*` and takes its keep-serving path, so the website goes on mirroring the pre-migration artifact with nothing recording the mismatch. Not hypothetical. `office-addin-manifest validate` is a network call to Microsoft's acceptance-test service, so an outage or a rate-limit on their side is enough to produce it. The fix is ordering, not the `docker create` extraction that stays deferred: the manifest build, its validation, and the two setup steps need nothing from the pushed image, and `Resolve the target` has already run. The tag path is now resolve -> build manifest -> validate -> push -> release, at no wall-clock cost, which makes the gate the changeset describes actually gate. Report deduplicated AppDomains separately from dropped ones. `dropped` collected two different reasons, so a production build logged `https://addin.postguard.eu/` as "not used by this build" one line after keeping the canonical spelling of that same origin — telling anyone debugging a send-time dialog failure precisely the opposite of the truth. Widen the lint and format globs past `src/**`. `.husky/pre-commit` runs lint-staged over `*.{js,mjs,ts}`, so a narrower CI glob was strictly weaker than the hook it complements, and it excluded the two files this work actually wrote: webpack.config.js, which carries the manifest transform, and scripts/. Widening required formatting test/render-body.test.ts and test/verified-sender.test.ts, which had drifted — the cost of the gap rather than an argument against closing it, since the hook would otherwise have rewritten them under whoever next touched either file. Refs #144 * docs(outlook-addon): correct the CLAUDE.md lines f7bf80b made stale Widening the CI globs put test/**/*.ts under eslint and prettier, so the parenthetical saying tests are "still outside the src-scoped eslint/prettier CI globs" now reads as permission to leave a test unformatted — which fails `check`. Two durable properties of `office-addin-manifest validate` are recorded with the commands, since both were only found by running it and one of them decided the add-in's version: it is a network call to Microsoft's acceptance-test service, so an outage fails it with nothing wrong locally (the reason it must not sit after anything that publishes), and it rejects a `<Version>` below 1.0, which is why the app is versioned from 1.0.0 rather than continuing 0.y.z. The entry also now distinguishes `validate` from `validate:dist`. The CI list gains what the last two commits added: both origin sets are built and validated, and the image builds on a PR without pushing. Refs #144 * docs(outlook-addon): point the build refusal at files that exist, restore two ignores Three non-blocking notes from the gate, none of them behavioural: - The production-build refusal ended `see .env.example.`, but this app has no such file (only apps/tb-addon does) and the deleted app-local .dockerignore took the `!.env.example` negation with it, so nothing signals one is coming. Point at the four ARG defaults in the Dockerfile and the workflow env block that overrides them instead. - CLAUDE.md still described `Baked URLs resolve` as asserting `*.postguard.eu` hosts. That was true of the first implementation; the job now derives its list from every https host in the two files, precisely because the narrow pattern needed a subdomain label and so missed the bare-apex defaults and yivi.app. - Consolidating the app-local .dockerignore into the root one kept the .env exclusions but dropped `docs`, `CLAUDE.md`, `README.md` and `*.log`. Those then enter the context via `COPY apps/outlook-addon/ apps/outlook-addon/`, so a docs-only edit busts the source layer and forces a full webpack pass. (`lib` is not restored — it does not exist in this tree.) Verified: eslint --max-warnings=0 and prettier --check pass on the CI globs, tsc --noEmit clean, check-version agrees, the production build emits both AppDomain log lines, 39/39 tests with 0 cancelled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(website,outlook-addon): declare the mirror's bootstrap window, reach lint parity Two non-blocking notes from the approving round. The first corrects a durability claim the previous changeset made and did not have. The mirror's bootstrap window is now declared, not inferred. The skip for "no release matches this pattern yet" was gated on the cached tag not matching, which read as self-clearing: mirror one outlook-addin-v* release and a pattern that later stops matching throws. That is true per container and not durably. `DOWNLOADS_DIR` is image content rather than a mounted volume -- docker/Dockerfile populates it from the build and sync-addons-loop.sh points at it -- so every fresh container restarts from the committed v0.1.5 metadata, whose tag never matches. A website deploy therefore re-armed the permissive path, leaving the loud regression check live only between a successful sync and the next deploy. So the window is a `bootstrap: true` flag on the target, in committed code that no deploy resets. Only a flagged target may treat the absence as expected; for anything else it still throws. And the flag cannot outlive its window: once a matching release exists it reports itself as stale on every run until removed, which is warned rather than thrown so the download page keeps updating while it is still set. Verified all three paths against the live API: flag set with no match warns and exits 0; flag removed with no match throws and exits 1, now independent of the cached tag, so a fresh container behaves identically; flag set with a match present emits the stale-flag warning. Removing it is an after-merge step alongside the first tag. Lint and format globs now match the pre-commit hook. lint-staged covers *.{js,mjs,ts} AND *.{json,md,yml,yaml,css,html} package-wide, so the previous widening -- src, test, scripts, webpack.config.js -- still claimed a parity it did not have, and the comment above it said so. Nine tracked files were unformatted: the two test files fixed last round, plus docs/outlook-quirks.md, four src templates, taskpane.css, and the yivi dialog. Reformatted, which is the argument for closing the gap rather than against it, since the hook would have rewritten them under whoever next touched a taskpane template. The template diff is cosmetic and was checked rather than assumed: `<!DOCTYPE html>` to `<!doctype html>` (case-insensitive per spec), text reflow inside an <aside> and a <button> where HTML collapses whitespace in normal flow anyway, and multi-line CSS font-family and transition values. Both origin sets still build, the production manifest still validates, and the built yivi dialog still links yivi.min.css relatively. Also `scripts/**` rather than `scripts/*` so a subdirectory cannot slip out, `eslint.config.mjs` added to both gates, and a .prettierignore for dist: CI checks formatting before any build so it cannot matter there, but a config change emitting minified HTML would otherwise fail the format gate on generated output. Refs #144 --------- Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
B1 of the monorepo consolidation (#123). Closes #124.
What moved
Everything SDK-related went wholesale to
packages/pg-js(git renames, history preserved): src, tests, scripts, img, tsconfig, tsdown/vitest configs, README, package.json. Root now holdspnpm-workspace.yaml(packages/*+apps/*, ready for the app imports), a private root package.json, and.changeset/.Release tooling: semantic-release → changesets
packages/pg-js/package.jsoncarries the real version (2.3.3); the0.0.0-managed-by-semantic-releaseplaceholder,.releaserc.json,@semantic-release/execand the npm override are gone.delivery.ymlnow runschangesets/action: merging a changeset to main opens a Version Packages PR; merging that publishes viapnpm releasewith npm provenance (publishConfig.provenance,id-token: writekept).generate-version.mjsunchanged in behavior — it reads the now-real package.json version; local builds embed 2.3.3 instead of0.0.0-dev. The client-version test now asserts the header matches package.json rather than the dev sentinel.CI
integration.yml: all three legs (Node 22/24, Bun, Deno) switched topnpm install --frozen-lockfile+pnpm -r; runtime-specific test/smoke steps run withworking-directory: packages/pg-js.Verified locally
pnpm install (prepare builds: 2.27 MB dist),
pnpm -r typecheckclean, 235/235 vitest tests pass, smoke green (8 checks).Reviewer notes
nodeModulesDirhandling is the knob.Part of encryption4all/postguard#247 (workstream B).