Skip to content

feat: import postguard-website as apps/website - #134

Merged
rubenhensen merged 657 commits into
mainfrom
feat/import-website
Jul 28, 2026
Merged

feat: import postguard-website as apps/website#134
rubenhensen merged 657 commits into
mainfrom
feat/import-website

Conversation

@rubenhensen

@rubenhensen rubenhensen commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

B2 of the monorepo consolidation (#123). Closes #125. Imports postguard-website as apps/website with history, wires it into the workspace, drops release-please in favour of changesets, and ports its checks and docker delivery.

The payoff, concretely

apps/website/node_modules/@e4a/pg-js -> ../../../../packages/pg-js. The website builds against the workspace SDK, so an SDK change type-checks and tests its biggest consumer in the same PR. That is the point of B2, and it kills the drift class the audit ranked #1 — the same class that left the Outlook add-in a major behind for months.

What is here

  • History-preserving subtree merge (plain-git read-tree --prefix; neither git subtree nor filter-repo is installed): 644 website commits reachable, blame and git log -- apps/website/… work through the merge, no SHAs rewritten. Imported at website origin/main d94a4f3 (v1.8.0), i.e. including #327, #328 and the 1.8.0 release.
  • Workspace wiring: @e4a/pg-jsworkspace:* (was ^2.0.1, so this is also a real bump to 2.3.4), app lockfile deleted in favour of the root pnpm-lock.yaml.
  • Submodules kept, re-declared at root with apps/website/-prefixed paths. They are load-bearing — docker-compose.yml builds cryptify and pg-pkg from source and bind-mounts their trees — and the compose contexts stay app-relative, so they still resolve.
  • release-please dropped; changesets versions the app via privatePackages: { version: true, tag: false } — it bumps apps/website/package.json without publishing (the package is private: true), and the docker version tag follows that. One release tool in the repo, which is the point: two of them is what froze postguard-business releases for two months.
  • Checks ported to .github/workflows/website.yml: svelte-check, unit, e2e, lint + lint:css, nginx/crossplane. Deliberately not path-filtered — filtering to apps/website/** would hide exactly the SDK-change-breaks-consumer case this repo exists to catch, and the suite is seconds.
  • Docker delivery ported, with both landmines defused (below).
  • Two isolated commits keep the diff readable: the prettier-3.9 reformat and the changeset.

A live bug fixed on the way

VITE_APP_VERSION lived in .env behind an x-release-please-version marker that release-please was configured to rewrite via extra-files. That substitution never fired: package.json reached 1.8.0 while .env still said 1.0.0, so GlitchTip release tags and every crash report have claimed 1.0.0 since the marker was added. It now comes from package.json in vite.config.ts (process.env.VITE_APP_VERSION ??=, because env.ts reads import.meta.env[name] dynamically, which define cannot reach). Verified in the built bundle: VITE_APP_VERSION:"1.8.0". Carries the patch changeset.

The two landmines, defused

  1. Image name. The old job pushed to ghcr.io/${{ github.repository }}, which in this repo resolves to postguard-js. postguard-ops pins ghcr.io/encryption4all/postguard-website in prod tfvars, so a naive port would have published to a name nothing deploys and prod would have silently stopped receiving images. The workflow now hardcodes IMAGE: ghcr.io/encryption4all/postguard-website; there are zero github.repository references left outside the comment explaining why.
  2. Dockerfile context. Stage 1 ran npm ci on a website-only context, which cannot resolve a workspace:* dependency. It now builds from the repo root: copy workspace manifests → pnpm install --frozen-lockfile --ignore-scripts → copy sources → build pg-js → build the site. Runtime COPY paths are root-relative and a root .dockerignore replaces the app-local one.

Verified locally

pg-js 235/235 (unchanged). Website: build ✔, test:unit 20/20, check 916 files, 0 errors, 0 warnings, lint ✔, lint:css ✔. changeset status clean.

The reworked Docker stage was actually built, not just reasoned about: docker build --target frontend succeeds, and the resulting image contains the real site (200.html, _app, about, blog, …) with VITE_APP_VERSION:"1.8.0" baked in. The brotli/nginx stages are untouched upstream layers.

After merge, in order

  1. Archive encryption4all/postguard-website promptly — until it is archived both repos publish :edge to the same image, which is transient but confusing.
  2. Transfer its open issues here (none are open right now, so this is only about future filings) and leave a pointer README.
  3. postguard-ops needs no change: same image name, same tag scheme (:edge on main, :x.y.z from the app version).

Next in the umbrella: B3 (tb-addon), B4 (outlook + the v1→v2 migration), B5 (examples), then B6 (single envelope implementation).

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

…R image for irma-server

- Change VITE_FILEHOST_URL to https://is.staging.yivi.app in .env.staging
- Remove irmago submodule (use pre-built container image instead)
- Use ghcr.io/privacybydesign/irma:latest for irma-server in dev compose
…facts

Submodule-based dev setup and prod image compose
Build-time env files (.env.main, .env.stable, .env.staging) baked URLs
into the frontend, requiring a separate image per environment. Replace
with a single .env containing placeholder tokens that are substituted
at container startup via sed.
Pre-compressed .br/.gz files become stale after sed placeholder
replacement at container startup. Dynamic brotli/gzip compression
in nginx handles this instead.
Split server block into default.conf.template with envsubst variables
for PKG_UPSTREAM_URL and IRMA_UPSTREAM_URL. Add entrypoint script that
replaces placeholder tokens in built JS files at container startup.
Remove gzip_static/brotli_static, fix access_log path typo, use
worker_processes auto.
Rewrite Dockerfile to compile brotli from source (no pkg-oss
dependency), using nginx 1.27.4 as runtime base. Consolidate
build-main/build-stable into single build script. Update CI
workflows and .dockerignore. Add runtime env vars to
docker-compose.prod.yml and remove broken volume mount.
Support runtime environment variables for Kubernetes deployment
PKG server is always proxied at /pkg relative to the app origin,
so hardcode that path instead of requiring a runtime env variable.
Use relative /pkg URL instead of VITE_PKG_URL env var
The root already runs changesets, and two release tools in one repo is what froze postguard-business releases for two months. The website is private:true, so changesets versions it without publishing (privatePackages.version) and its docker tag can follow that version.

VITE_APP_VERSION moves from a release-please-rewritten .env line to package.json, read in vite.config.ts. That substitution never actually fired: package.json was at 1.8.0 while .env still said 1.0.0, so GlitchTip release tags and crash reports have reported 1.0.0 since the marker was added.
The site depends on @e4a/pg-js as workspace:*, so a website-only context can no longer resolve it. Stage 1 installs the workspace with --ignore-scripts, then builds pg-js before the site. Runtime COPY paths are repo-root relative and a root .dockerignore replaces the app-local one.
The deleted package-lock.json had prettier frozen at 3.8.3; the workspace lockfile resolves 3.9.6, in the declared ^3.8.3 range, which changed formatting in these files. Isolated so the import diff stays reviewable.
@rubenhensen
rubenhensen force-pushed the feat/import-website branch from 99c7bc2 to 3133a17 Compare July 28, 2026 12:07
@rubenhensen
rubenhensen marked this pull request as ready for review July 28, 2026 12:08
@rubenhensen

Copy link
Copy Markdown
Contributor Author

/dobby please review. Rebuilt from scratch at website origin/main d94a4f3 (the earlier draft imported a 24-commit-stale local main — caught by the missing test:unit/test:e2e scripts), so this now includes #327/#328/1.8.0.

Two things worth your attention specifically:

  1. The Dockerfile rework — I built --target frontend for real and inspected the image (site present, version 1.8.0 baked), but the full multi-arch build only runs in CI here.
  2. The VITE_APP_VERSION rewiring: it must survive both a fresh clone and the docker build, and env.ts reads import.meta.env[name] dynamically so define is not an option — please sanity-check the process.env ??= approach in vite.config.ts.

Also flagging for the record: this drops release-please here per maintainer decision, so the website's CHANGELOG.md is now historical and changesets owns future entries.

@rubenhensen

Copy link
Copy Markdown
Contributor Author

CI caught a third facet of the image-name landmine, and it needs one org action from you.

The docker builds failed with:

denied: permission_denied: write_package
  ghcr.io/encryption4all/postguard-website

GHCR grants package write per repository. The postguard-website package belongs to the postguard-website repo, so this repo's GITHUB_TOKEN cannot push to it — regardless of packages: write on the job. My token lacks read:packages, so I can't inspect or change it.

Before/after this merges, one-time grant:
GHCR → postguard-website package → Package settingsManage Actions accessAdd repositorypostguard-js → role Write.

(The alternative — publishing under a new package name — would force a postguard-ops tfvars change and break the "ops needs no change" property, so it is the worse option.)

Meanwhile, so the PR can be green and this isn't blocked: docker now builds on PRs without pushing, and publishes only from main. That keeps the valuable part (the Dockerfile is validated on every PR, including the workspace-root rework) and removes a cross-repo permission dependency from PR runs. The one casualty is :pr-N preview tags — one push: falsetrue flip restores them once access is granted. Nothing pins :pr-N (ops pins :edge and :x.y.z), so this is low-risk.

Note the ordering this implies: the first push to main after merge will fail its publish step until the grant is in place. Either do the grant first, or expect one red main run and re-run it after.

@dobby-coder
dobby-coder Bot requested a review from Iso5786 July 28, 2026 12:22
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed writeup, rubenhensen! I'm on it — reviewing B2 now, with a close look at the subtree merge integrity, the workspace/lockfile wiring for @e4a/pg-js, the submodule path re-declarations, and the changesets/docker delivery swap-in. I'll follow up with a review shortly.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Request changes

The import itself is well executed and the two landmines the PR body calls out are genuinely defused: IMAGE is hardcoded so prod keeps receiving images, and the root-context Dockerfile resolves workspace:* (CI proves it, both Build (amd64) and Build (arm64) are green). History is reachable through the subtree merge, the VITE_APP_VERSION fix is real and I confirmed 1.8.0 lands in the bundle, and the decision not to path-filter website.yml is the right one for the drift class this PR targets.

Two things should be fixed before merge, plus the docs.

1. The security overrides are silently inert (apps/website/package.json:60). pnpm ignores npm overrides, and a pnpm.overrides block is only honoured in the root manifest. The lockfile confirms the downgrade: cookie 1.1.1 -> 0.6.0 and esbuild 0.27.7 -> 0.19.12, both the only copies in the tree, both the versions the comment on line 59 calls vulnerable. Limited exploitability, but a security pin that stopped working without any warning is the thing that bites at the next bump.

2. docker compose up is broken (apps/website/docker/dev.Dockerfile:6). The README's recommended dev entry point fails on COPY package.json package-lock.json ./ now that the app lockfile is gone, and npm ci could not resolve workspace:* even if the COPY were fixed. The dev image needs the same repo-root-context rework the prod one got.

3. Stale docs. apps/website/README.md:37 still documents release-please as the release process, apps/website/README.md:21 still says npm install, and apps/website/CLAUDE.md:5 still says Release: Release-please with npm ci guidance throughout. Root CLAUDE.md still says packages/pg-js is "the only package so far". A PR whose whole purpose is killing docs drift should not leave the drift in its own docs, and CLAUDE.md is auto-loaded agent context, so a wrong line there compounds.

Not anchorable inline (the file is not in the diff)

package.json (root), lines 11-12: the aggregate scripts silently skip the new app. pnpm -r test and pnpm -r typecheck match nothing in apps/website, which defines test:unit / test:e2e / check instead of test / typecheck. Verified: pnpm -r test reports Scope: 2 of 3 workspace projects and then runs only packages/pg-js, with no warning about the skip. pnpm -r build does pick the app up. .github/workflows/integration.yml uses those same two aggregate scripts for its Typecheck and Test steps, so they now under-report on exactly the consumer this PR imported in order to catch SDK breakage.

CI as a whole is still covered, because website.yml is unfiltered and runs the app's own suite. But a local pnpm test reads as green while the website's 20 unit tests never ran. Cheapest fix is two aliases in apps/website/package.json: "test": "pnpm test:unit" and "typecheck": "pnpm check".

Verification

  • pg-js 235/235 unchanged. Website: build ok, test:unit 20/20, check 916 files / 0 errors / 0 warnings, lint ok, lint:css ok, changeset status clean.
  • All PR checks green: Svelte Check, Unit Tests, E2E Tests, Lint, nginx config test, Build amd64, Build arm64. Finalize Docker manifest reports as skipped, which is correct for a PR and still reports a check run, so it can be a required check.
  • VITE_APP_VERSION:"1.8.0" confirmed in the built bundle.
  • Rule check found one additional nit in website.yml:236 (unguarded command substitution into $GITHUB_OUTPUT), commented inline. The pipefail, path-filter, pull_request trigger-types and conventional-commit rules all pass.

Comment thread apps/website/package.json Outdated
Comment thread apps/website/docker/dev.Dockerfile Outdated
Comment thread apps/website/README.md Outdated
Comment thread apps/website/CLAUDE.md Outdated
Comment thread apps/website/package.json Outdated
Comment thread .github/workflows/website.yml Outdated
Comment thread .github/workflows/website.yml Outdated
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Reviewed and requested changes on #134 — the import itself is solid (workspace SDK linking works, both landmines called out in the PR body are genuinely defused, CI's green across the board), but I found three things worth fixing before merge:

  1. The security overrides went silently inert — pnpm doesn't honor overrides in a workspace member, so cookie and esbuild quietly downgraded back to the CVE-flagged versions the code comment says they're pinned against. Needs hoisting to root pnpm-workspace.yaml.
  2. docker compose up is broken — the dev Dockerfile still copies a lockfile that no longer exists and can't resolve workspace:*. It needs the same root-context rework the prod Dockerfile got.
  3. Stale docs — README and both CLAUDE.md files still describe the old release-please/npm workflow, which undercuts a PR whose whole point is killing docs drift.

Also flagged as non-blocking nits: pnpm -r test/typecheck silently skip the new app's tests, husky no-ops from the workspace member, a dead tag path in a main-only job, and an unguarded command substitution into $GITHUB_OUTPUT in website.yml.

Full review is here: review

Both blocking findings from review.

pnpm ignores an npm `overrides` block in a workspace member, and even `pnpm.overrides` is only honoured in the root manifest — so the import silently downgraded cookie 1.1.1 -> 0.6.0 (CVE-2024-47764) and esbuild 0.27.7 -> 0.19.12 (GHSA-67mh-4wv8-2f99), the exact versions the block existed to avoid. Moved to root pnpm.overrides and verified the lockfile now resolves 1.1.1 / 0.27.7.

dev.Dockerfile still copied the deleted apps/website/package-lock.json and ran npm ci, which cannot resolve workspace:* anyway, so `docker compose up` — the README's recommended start — was broken. It now builds from the repo root like the prod image, and compose mounts the root with anonymous volumes for both node_modules trees. Verified: the dev image builds and /repo/apps/website/node_modules/@e4a/pg-js resolves inside it.

Also: apps/website/.dockerignore deleted (both builds now use the root context, so it was never consulted); husky hoisted to the repo root where .git is, since `prepare: husky` in a subdirectory silently no-ops and lint-staged had stopped running; dead type=ref,event=pr tag path removed from a job that only runs on main; the version step no longer hides a failing $(node -p) behind echo's exit status; README/CLAUDE.md (app and root) updated for changesets, pnpm and the overrides/husky rules.
@rubenhensen

Copy link
Copy Markdown
Contributor Author

/dobby all seven addressed and verified against the tree, not just described. Both blockers were right, and the first one was a security regression I introduced:

1. Inert overrides. Confirmed exactly as you measured — the lockfile had cookie@0.6.0 and esbuild@0.19.12, the versions the block existed to avoid. Moved to root pnpm.overrides, reinstalled, and the lockfile now resolves cookie@1.1.1 / esbuild@0.27.7. I also wrote the rule into root CLAUDE.md so the next person adding a pin puts it in the right manifest.

2. Broken dev stack. dev.Dockerfile now builds from the repo root like the prod image, and compose mounts ../.. with anonymous volumes for both node_modules trees (root store + the app's, which holds the pg-js symlink) — your point about the symlink dangling outside the mount was the part I would have missed. Verified by building it: the image comes up and /repo/apps/website/node_modules/@e4a/pg-js resolves inside it.

On .dockerignore: you were right that the PR body's "replaced" was wrong, but the resolution is simpler than picking which governs which — now that the dev build also uses the root context, the app-local file is never consulted by either build, so I deleted it rather than leaving a dead file that looks live.

Husky hoisted to the repo root where .git is, hook running pnpm --filter postguard-website exec lint-staged. It proved itself immediately — the hook fired on the fix commit and reformatted four staged files.

Also fixed: the dead type=ref,event=pr path, and the fail-open echo "value=$(node -p …)" (now two statements so a failing substitution actually aborts instead of publishing :edge with the version tag silently missing). README + app/root CLAUDE.md updated for changesets and pnpm.

Re-verified after all of it: pg-js 235/235, website 20/20 unit, build ✔, lint ✔, svelte-check 916 files 0/0.

@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Got it — I'll review the updated PR #134 against the tree now, checking that all seven points (overrides, dev stack, .dockerignore, husky) land as described, and post one consolidated review.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at a968bcf. All seven earlier findings are genuinely addressed — I checked each against the tree rather than the description, and the security fix in particular is real: the root lockfile now resolves cookie@1.1.1 and esbuild@0.27.7 as the only copies, and a scratch test on the pinned pnpm 10.32.1 confirmed the root override mechanism is what's doing it. CI is green on every job, including the prod image on both arches (Build (amd64) / Build (arm64)), nginx config parse, and the 10-minute e2e run.

Two new blockers, both in the dev stack the last commit rewrote. Neither is caught by CI, because no job builds dev.Dockerfile.

  1. dev.Dockerfile:24pnpm dev -- --host does not pass --host. Measured in this app: pnpm forwards the -- verbatim, vite prints Network: use --host to expose and binds 127.0.0.1:5173, so nginx's proxy_pass http://postguard-website:5173 is refused and http://localhost:8080 502s. Drop the separator.
  2. dev.Dockerfile:16 — the dev container never has a built @e4a/pg-js. --ignore-scripts skips pg-js's prepare, dist/ is gitignored so it isn't in the bind-mounted host tree either, and the ../..:/repo mount shadows anything the image built. The site boots and / returns 200, but the @e4a/pg-js import 500s — the shell renders with encryption dead, which is worse than not booting.

A third thing also blocks docker compose up, imported rather than introduced: docker/irma/jwt_privkey.pem is mounted by both compose files but does not exist and nothing generates it (docker-compose.yml:71).

Two worth fixing, three nits inline. On the overrides: the pins work, but pnpm now prints [WARN] The "pnpm" field in package.json is no longer read by pnpm ... "pnpm.overrides" on every command in this repo. That warning is wrong today, and that is exactly why it is worth moving — the failure mode this block exists to prevent is silently inert pins, and CLAUDE.md now names the deprecated location as the rule. overrides: in pnpm-workspace.yaml resolves identically with no warning.

One minor thing with no inline anchor: .gitmodules uses git@github.com: URLs for cryptify and postguard. Both repos are public, so https:// would let anyone run step 1 of the README quickstart (git submodule update --init --recursive) without an SSH key.

I dropped one candidate finding rather than reporting it: the pre-commit hook only reaching apps/website is fine — packages/pg-js has no prettier/eslint devDependency and no lint script, so there is nothing there for lint-staged to run, and the comment on line 1 reads correctly.

Comment thread apps/website/docker/dev.Dockerfile Outdated
Comment thread apps/website/docker/dev.Dockerfile
Comment thread apps/website/docker-compose.yml
Comment thread package.json Outdated
Comment thread apps/website/package.json
Comment thread apps/website/.gitlab-ci.yml Outdated
Comment thread apps/website/playwright.config.ts Outdated
Comment thread apps/website/.env
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Nice, thanks for the thorough turnaround. I re-checked all seven against the tree (not just the description) and posted a fresh review: changes requested, 8 inline comments.

The overrides fix checks out — root lockfile now resolves cookie@1.1.1 / esbuild@0.27.7 only, confirmed on pinned pnpm 10.32.1. CI's green across the board, including both prod-image arches and the e2e run.

Two new blockers turned up in the dev stack this last commit touched (neither is covered by CI, since no job builds dev.Dockerfile):

  1. dev.Dockerfile:24pnpm dev -- --host doesn't actually pass --host through; drop the -- separator or vite stays bound to 127.0.0.1 and nginx 502s.
  2. dev.Dockerfile:16 — the dev container never builds @e4a/pg-js, so the site loads but the pg-js import 500s.

Also imported (not introduced by you): docker/irma/jwt_privkey.pem is mounted by both compose files but doesn't exist and nothing generates it.

Plus 2 should-fix (the pnpm.overrides location itself — pnpm-workspace.yaml is the right home, otherwise pnpm warns on every command; missing test/typecheck aliases so root pnpm test silently skips the website) and 3 nits (dead .gitlab-ci.yml, npm run vs pnpm drift in the Playwright config, .env placeholder vs the README default). I dropped the husky finding — packages/pg-js has no lint tooling, so there's nothing there for lint-staged to miss.

Full review: #134 (review)

All eight from review.

Blockers, all three on the `docker compose up` path this PR set out to repair and none covered by CI (no job builds dev.Dockerfile):
- `pnpm dev -- --host` never passed --host: pnpm forwards the separator verbatim and vite reads it as end-of-options, so vite stayed on 127.0.0.1 inside the container and nginx's proxy_pass got refused. Separator dropped.
- The dev container had no built @e4a/pg-js. install --ignore-scripts skips its prepare, packages/pg-js/dist is gitignored so a fresh clone has none, and the bind mount shadows anything the image built. It is now built in CMD against the mounted sources, with an added anonymous volume for packages/pg-js/node_modules so the generators can find @e4a/pg-wasm.
- docker/irma/jwt_privkey.pem was mounted by both compose files but never existed and nothing generated it; Docker silently creates a directory there and irma-server dies. Added scripts/gen-irma-key.sh (idempotent, refuses the Docker-created directory) and put it in the README's start sequence.

Should-fix:
- pnpm.overrides moved from root package.json to pnpm-workspace.yaml, which is where pnpm 10 wants it — the package.json field warned on every command.
- The website defined neither `test` nor `typecheck`, and `pnpm -r` skips a package that lacks the script without a word, so root `pnpm test` reported green having run only pg-js. Added both as aliases.

Nits: deleted the dead .gitlab-ci.yml (same reasoning as the app-local .dockerignore); playwright.config.ts now runs pnpm, matching what CLAUDE.md already claimed; added .env.development so a local dev run does not resolve VITE_FILEHOST_URL to the literal placeholder.

Verified by running it, not by reading it: the dev image builds, and from a dist-less copy of the tree the container builds pg-js into the mount, vite reports Network: http://172.17.0.2:5173/, GET / is 200, and `import('@e4a/pg-js')` resolves inside the container.
@rubenhensen

Copy link
Copy Markdown
Contributor Author

/dobby all eight fixed, and I ran the dev stack this time rather than reasoning about it.

The three docker compose up blockers. Your pnpm dev -- --host measurement was exactly right — I reproduced it, and the separator is gone. pg-js is now built in CMD against the mounted sources rather than baked in, plus an anonymous volume for packages/pg-js/node_modules so its generators can still find @e4a/pg-wasm under the bind mount (your point that the image-built copy gets shadowed is what made the runtime build the only option). And gen-irma-key.sh generates the key, refuses to run when Docker has already created a directory in its place, and is now step two of the README's start sequence.

Verified by running it, from fresh-clone conditions (copy of the tree with no packages/pg-js/dist):

✔ Build complete in 1022ms        # pg-js built into the mount
> vite dev --host
➜  Network: http://172.17.0.2:5173/   # was: "use --host to expose"
GET / -> 200
import('@e4a/pg-js') OK — exports: DecryptionError, IdentityMismatchError, NetworkError, …

Worth noting: my first attempt at that verification was invalid — the sandbox path I work in isn't shareable with Docker, so the bind mount silently produced a partial tree. Copying under $HOME first is what made it a real test. Same class as the stale-main import: the test that looks like it ran and didn't.

pnpm test skipping the website was the best catch of the eight — same silent-green shape as the manifest one on #267. Both aliases added; root pnpm test now runs 235 + 20.

Overrides moved to pnpm-workspace.yaml (warning gone, resolution still cookie@1.1.1/esbuild@0.27.7), .gitlab-ci.yml deleted, playwright config on pnpm, and .env.development added so a local dev run stops resolving VITE_FILEHOST_URL to the literal placeholder.

All eight threads resolved. The generated key is gitignored and I confirmed it never entered the index.

@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed follow-up, rubenhensen — I'm on it. Digging into all eight fixes now, including the docker compose up blockers and the pnpm test website-skipping catch, and I'll post a consolidated review on this PR shortly.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve

Rule check clean, all eight earlier findings verified fixed at 19f5bb7, CI fully green (13 checks, nothing in flight). Five non-blocking notes below, one of which is a line this PR introduced and then invalidated in its own next commit; worth the one-line edit before you merge.

One correction to my previous cycle: the pnpm typecheck / --threshold warning finding was wrong. I unpacked svelte-check@4.7.4 (the resolved version) and read the exit path. --threshold builds a display filter only (createFilter, index.js:130096), and its commander default is already 'warning' (index.js:128685). The exit code is errorCount === 0 && (!opts.failOnWarnings || warningCount === 0), so only --fail-on-warnings makes a warning red. Root pnpm typecheck and the CI Svelte Check job therefore gate identically, and adding --threshold warning to check would change nothing. No action needed; sorry for the noise.

While in there I did notice a real but unmeasured asymmetry: check passes --tsconfig ./tsconfig.json, CI does not, and svelte-check skips openAllDocuments when --tsconfig is set. The two runs can cover different file sets in either direction. Both are green today, so this is a note rather than a finding.

What I checked (rules that apply to this diff):

  • pnpm-ignores-overrides-outside-the-root-manifest — both halves now compliant. The lockfile carries a top-level overrides: block and resolves cookie@1.1.1 / esbuild@0.27.7; the only other cookie entry is @types/cookie@0.6.0, which is types-only. And pnpm -r test / pnpm -r typecheck both reach the website now via the two aliases.
  • actions-default-shell-has-no-pipefailwebsite.yml has no unguarded pipe in any run: block, and the finalize version step already splits the echo "value=$(...)" shape apart deliberately.
  • pr-close-issue-keywordsCloses #125 on its own line, base is main; Part of encryption4all/postguard#247 is correctly a non-closing ref.
  • standardized-readmes — logo, docs link, role, Development, Releasing, MIT all present.
  • vite-build-never-evaluates-app-modules — the .env.development comment describes a runtime consequence and does not claim the build catches anything. Correct as written.
  • spa-fallback-false-positive — does not apply to your verification: GET / -> 200 came from the vite dev server, not from adapter-static behind nginx, and the import('@e4a/pg-js') probe is body-level evidence anyway.
  • compose-run-starts-only-that-services-depends-on, docker-bind-mount-restart, conventional-commit-pr-titles, sveltekit-csp-report-only-needs-report-uri — no violation (no CSP config in svelte.config.js, no test service in compose, no deploy step mutating a bind-mounted file).

On provenance, since it changes how two of the notes should be graded. I ran blame through the subtree merge on each. docker-compose.prod.yml:88 traces to e0b6c20 (2026-04-30) and the GlitchTip comment to a23d1cb (2026-06-04), both in the imported repo, so they are debt this PR relocates rather than introduces. Not merge blockers on an import PR. CLAUDE.md:95 traces to a968bcf, which is in this PR, and 19f5bb7 is what made it false.

On the two Docker notes: I could not run the daemon in this sandbox, so --renew-anon-volumes rests on the documented flag semantics and the root-ownership one on reading the Dockerfile (no USER, CMD writing into the bind mount). Both are Linux-host developer-experience items, not correctness, and both are plausibly invisible on Docker Desktop, which may be why your run looked clean.

Comment thread CLAUDE.md
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. The package scripts listed below live in `packages/pg-js/package.json` (working-directory rule: line 11).
Monorepo, pnpm workspaces, release via changesets. `packages/pg-js` = `@e4a/pg-js`, the published TypeScript SDK; `apps/website` = the PostGuard site (private, versioned by changesets but not published; its docker image is `ghcr.io/encryption4all/postguard-website`). Remaining apps join per encryption4all/postguard-js#123. The package scripts listed below live in `packages/pg-js/package.json` (working-directory rule: line 11); the website's live in `apps/website/package.json` and run via `pnpm --filter postguard-website <script>`.

Security overrides (`cookie`, `esbuild`) live in the ROOT `package.json` under `pnpm.overrides`. pnpm ignores an `overrides` block in a workspace member entirely — that is how both pins silently regressed to vulnerable versions during the website import. Add new pins at the root only.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the one item I would fix before merging, because it is now false and it is agent-loaded context.

The line says security overrides live in the ROOT package.json under pnpm.overrides and to "Add new pins at the root only." 19f5bb7 moved them to pnpm-workspace.yaml, and root package.json no longer has a pnpm block. Blame confirms the paragraph came in with a968bcf and the next commit in the same PR invalidated it.

Consequence is mild but real: the next agent adding a CVE pin follows this line, puts it in root package.json, and gets the pnpm 10 warning about a field that is on its way out. The pin would still apply, so this is a wrong-signpost problem rather than a dead-pin problem. Point it at pnpm-workspace.yaml's overrides: block; the comment you already wrote there carries the reasoning worth reusing.

environment:
- PKG_UPSTREAM_URL=http://postguard-pkg:8087
- IRMA_UPSTREAM_URL=http://irma-server:8088
- VITE_FILEHOST_URL=http://cryptify-fileshare:8000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

VITE_FILEHOST_URL=http://cryptify-fileshare:8000 is a Docker-internal DNS name, but this value ends up in the browser. docker/entrypoint.sh seds __VITE_FILEHOST_URL_PLACEHOLDER__ into the served *.js, src/lib/env.ts:9 exports it as FILEHOST_URL, and it is used both as pg-js's cryptifyUrl (src/lib/postguard.ts:19) and to build the download link embedded in the outgoing email (EmailPreviewModal.svelte:65). A browser cannot resolve cryptify-fileshare, so uploads fail and every emailed link is dead.

cryptify publishes 8000:8000 in this same file and .env.dev uses http://localhost:8000, so http://localhost:8000 (or the deployment's real host, matching the --url http://localhost:8088 convention two services up) is the working value. PKG_UPSTREAM_URL / IRMA_UPSTREAM_URL are correct as internal names, since those are nginx proxy_pass targets resolved server-side.

Not blocking: blame puts this on e0b6c20 (2026-04-30) in the imported repo, so it is carried-forward debt, and the real deployment sets its own value through postguard-ops. It bites whoever first tries this file as a local prod smoke stack. A one-line fix here or a follow-up issue, your call.

# as end-of-options, and the flag is silently dropped — leaving vite on
# 127.0.0.1 inside the container and nginx's proxy_pass getting refused.
EXPOSE 5173
CMD ["sh", "-c", "pnpm --filter @e4a/pg-js build && pnpm --filter postguard-website dev --host"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This CMD runs as root (node:24-alpine, no USER) against the bind-mounted host tree, so on a Linux host docker compose up writes root:root files into the developer's working copy: packages/pg-js/dist/ plus the four gitignored generated sources the prebuild step produces (src/util/wasm-binary.ts, src/util/pg-wasm-shim.js, src/yivi/yivi-css-text.ts, src/util/version.ts).

Order matters for how it surfaces: fresh clone, docker compose up first, then a host-side pnpm install fails EACCES in prepare when the generators try to rewrite those paths, with nothing pointing at Docker as the cause.

Either add user: "${UID:-1000}:${GID:-1000}" to the compose service or warn about it next to the gen-irma-key.sh step in the README. Docker Desktop on macOS and Windows remaps ownership and hides this, which is likely why your run was clean. Read from the Dockerfile rather than reproduced, since I have no daemon here.

# shadowed by the bind mount above. Both are needed: pnpm puts the
# workspace store at the root and a per-package node_modules (which
# holds the pg-js symlink) in the app.
- /repo/node_modules

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The three anonymous node_modules volumes are the right fix for the bind-mount shadowing. One follow-on worth documenting: Compose preserves anonymous volumes when it recreates a container, which is exactly what --renew-anon-volumes / -V exists to opt out of.

So after a lockfile change, docker compose up --build rebuilds the image and reinstalls, then re-attaches the OLD anonymous volumes over /repo/node_modules and the two others. The container keeps the stale dependency tree and the --frozen-lockfile guarantee is silently void. It presents as "the dependency I just added isn't there", with no error to search for.

Worth adding docker compose up --build --renew-anon-volumes to the README start sequence, next to the gen-irma-key.sh line. This is from the documented flag semantics, not reproduced (no daemon in my sandbox).

Separately, the comment on line 94 still reads "Both are needed" now that a third mount sits below it.


# GlitchTip — open-source, Sentry-compatible error tracker.
# Reports posted by the website's CrashReport button land here.
# UI at http://localhost:8000 (override via GLITCHTIP_PORT if 8000 is taken).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment is wrong on both counts. GlitchTip's UI is on 8001 (PORT=8001, ports: 8001:8001, GLITCHTIP_DOMAIN=http://localhost:8001), and 8000 is cryptify's port in this same file, so the line sends you to a different service. GLITCHTIP_PORT does not exist anywhere in the repo either; grep finds only this line, and the port is hardcoded in three places.

Either fix the number and drop the override sentence, or wire ${GLITCHTIP_PORT:-8001} through the mapping, PORT and GLITCHTIP_DOMAIN. Nit, and carried forward from a23d1cb (2026-06-04) rather than introduced here.

@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Approved: review #4801873978. All eight findings check out fixed at 19f5bb7, rules clean, CI green.

One correction on my end first: the svelte-check --threshold warning finding was wrong. I read svelte-check@4.7.4's actual exit logic — --threshold only filters what's displayed, and the default is already warning; only --fail-on-warnings changes the exit code. So pnpm typecheck and CI already gate identically, and that flag would've been a no-op. Retracted, and I saved a memory rule so it doesn't come back next cycle. (There is a real but separate asymmetry between check's --tsconfig flag and CI's lack of one that can make them cover different files — both happen to be green today, so just a note, not a finding.)

Five non-blocking notes, one worth a quick fix before merge:

  • CLAUDE.md:95 still says the security overrides live in root package.json19f5bb7 moved them to pnpm-workspace.yaml. Same PR wrote and then invalidated this line, so worth a one-line correction.
  • docker-compose.prod.yml:88 and the GlitchTip port comment (docker-compose.yml:113) are real but carried forward from the upstream repo via the subtree merge, not introduced here — not blockers on an import PR.
  • dev.Dockerfile:29 (root-owned build artifacts) and the anonymous-volume note (docker-compose.yml:97, re-attaching stale volumes after lockfile changes) are Linux-host DX gaps that likely don't show up on Docker Desktop, which is probably why your run looked clean.

Nice catch flagging the sandbox bind-mount issue on your end too — good parallel to my --threshold slip, both cases of evidence that looked real and wasn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

monorepo: import postguard-website as apps/website (history-preserving)

1 participant