Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a WIP that experiments with moving installs/CI commands to pnpm, while also improving WireMock-based NHS Login test stubs by making JWT issuer configurable and preventing stale JWKS caching via unique kids per run.
Changes:
- Add a per-process nonce to JWT
kidvalues to force JWKS re-fetches between runs. - Introduce
WIREMOCK_JWT_ISSUERwiring through test config, global setup, and the WireMock mapping push script. - Update local/CI tooling: switch install + Playwright commands to pnpm, add pnpm to mise tooling, and bump Postgres images to 17.9.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/users/wiremockAuthMappings.ts | Appends a per-run nonce to kid to avoid serving stale cached JWKS keys. |
| tests/scripts/push-wiremock-mappings.ts | Adds WIREMOCK_JWT_ISSUER support and uses shared OS Places mapping helper. |
| tests/global-setup.ts | Uses configured JWT issuer when generating WireMock auth/userinfo mappings. |
| tests/configuration/EnvironmentVariables.ts | Adds WIREMOCK_JWT_ISSUER env var. |
| tests/configuration/EnvironmentConfiguration.ts | Adds wiremockJwtIssuer to config with env overrides. |
| package.json | Switches postinstall/start to pnpm-based installs/commands. |
| .github/actions/init-mise/action.yaml | Switches caching/install flow to pnpm store. |
| .github/workflows/playwright-e2e.yaml | Uses pnpm exec playwright ... and starts app via pnpm script. |
| .mise.toml | Adds pnpm tool and changes install task to pnpm install --frozen-lockfile. |
| mise.lock | Adds pnpm tool lock entry and updates locked tool metadata. |
| local-environment/scripts/database/Dockerfile | Bumps Postgres base image to 17.9. |
| local-environment/docker-compose.yml | Bumps Postgres service image to 17.9. |
| lambdas/src/lib/db/db-client.integration.test.ts | Updates the Postgres testcontainer image to 17.9 and tidies client init. |
Comments suppressed due to low confidence (1)
.mise.toml:75
- The
install-npmtask now runspnpm install --frozen-lockfile, but there are nopnpm-lock.yamlfiles in the repo (onlypackage-lock.json), so this task will fail as soon as it runs.
Suggested fix: add/commit pnpm lockfile(s) (or switch this task back to npm ci until pnpm lockfiles are in place); if keeping pnpm here, consider renaming the task to avoid confusion.
[tasks.install-npm]
description = "Install pnpm dependencies"
run = "pnpm install --frozen-lockfile"
package.json
Outdated
| "postinstall": "pnpm -C ui install --frozen-lockfile && pnpm -C lambdas install --frozen-lockfile && pnpm -C tests install --frozen-lockfile", | ||
| "test": "npm --prefix ui run test && npm --prefix lambdas run test", | ||
| "test:playwright": "UI_BASE_URL=$(terraform -chdir=local-environment/infra output -raw ui_url) API_BASE_URL=$(terraform -chdir=local-environment/infra output -raw api_base_url) npm --prefix tests run test:chrome", | ||
| "format": "(git diff --name-only --diff-filter=ACMR -z; git diff --name-only --cached --diff-filter=ACMR -z) | sort -zu | xargs -0 sh -c '[ \"$#\" -gt 0 ] && npm run format:pre-commit -- \"$@\"' sh", | ||
| "format:pre-commit": "prettier --write --ignore-unknown --", | ||
| "lint": "npm --prefix ui run lint && npm --prefix lambdas run lint && npm --prefix tests run lint", | ||
| "build:lambdas": "npm --prefix lambdas run build", | ||
| "package:lambdas": "npm --prefix lambdas run package", | ||
| "start": "npm ci && npm run local:start", | ||
| "start": "pnpm install --frozen-lockfile && pnpm run local:start", | ||
| "stop": "npm run local:stop", |
There was a problem hiding this comment.
The root scripts now run pnpm ... --frozen-lockfile, but the repo currently only has package-lock.json files (no pnpm-lock.yaml in root/ui/lambdas/tests), so postinstall/start will fail immediately when pnpm enforces the lockfile.
Suggested fix: either commit the appropriate pnpm-lock.yaml file(s) (and remove/stop generating the package-lock.json equivalents), or temporarily drop --frozen-lockfile until the pnpm lockfiles are added and stable.
| tests/node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} | ||
| path: ${{ steps.pnpm-store.outputs.path }} | ||
| key: pnpm-store-${{ runner.os }}-${{ hashFiles('**/pnpm-lock.yaml') }} |
There was a problem hiding this comment.
This cache key hashes **/pnpm-lock.yaml, but the repository does not currently contain any pnpm-lock.yaml files, so the key will be effectively constant and the cache will be stale/ineffective.
Suggested fix: add the pnpm lockfile(s) as part of the migration, or key the cache off the existing package-lock.json files until pnpm lockfiles exist.
| key: pnpm-store-${{ runner.os }}-${{ hashFiles('**/pnpm-lock.yaml') }} | |
| key: pnpm-store-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} |
| const wiremockBaseUrl = process.env.WIREMOCK_BASE_URL ?? "http://localhost:8080"; | ||
|
|
||
| /** Catch-all OS Places /find stub — returns a single dummy address for any postcode query. */ | ||
| function createOSPlacesCatchAllMapping(): WireMockMapping { | ||
| return { | ||
| priority: 100, // lower priority than per-test stubs | ||
| request: { | ||
| method: "GET", | ||
| urlPath: "/find", | ||
| queryParameters: { | ||
| query: { matches: ".*" }, | ||
| }, | ||
| }, | ||
| response: { | ||
| status: 200, | ||
| headers: { "Content-Type": "application/json" }, | ||
| jsonBody: { | ||
| header: { | ||
| uri: "http://wiremock/find", | ||
| query: "query=SW1A1AA", | ||
| offset: 0, | ||
| totalresults: 1, | ||
| format: "JSON", | ||
| dataset: "DPA", | ||
| lr: "EN", | ||
| maxresults: 100, | ||
| epoch: "95", | ||
| output_srs: "EPSG:27700", | ||
| }, | ||
| results: [ | ||
| { | ||
| DPA: { | ||
| UPRN: "100023336956", | ||
| UDPRN: "52640002", | ||
| ADDRESS: "10 DOWNING STREET, LONDON, SW1A 1AA", | ||
| BUILDING_NUMBER: "10", | ||
| THOROUGHFARE_NAME: "DOWNING STREET", | ||
| POST_TOWN: "LONDON", | ||
| POSTCODE: "SW1A 1AA", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| const wiremockJwtIssuer = process.env.WIREMOCK_JWT_ISSUER ?? wiremockBaseUrl; | ||
|
|
There was a problem hiding this comment.
wiremockJwtIssuer defaults to wiremockBaseUrl (localhost), which changes the previous default behaviour where JWTs were issued with iss=http://wiremock:8080; for local Docker runs the login lambda typically validates against the Docker-internal issuer, so this default is likely to produce tokens with an issuer the lambda rejects.
Suggested fix: default wiremockJwtIssuer to http://wiremock:8080 (or leave it undefined so createWireMockAuthManifest() falls back to its internal default) and only use WIREMOCK_BASE_URL as the issuer when explicitly targeting a remote WireMock.
| "test": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js", | ||
| "test:watch": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js --watch", | ||
| "serve:static": "npm run build && npx http-server build -p 8085 -c-1 --proxy http://localhost:8085?", | ||
| "serve:static": "pnpm run build && pnpm exec http-server build -p 8085 -c-1 --proxy http://localhost:8085?", |
There was a problem hiding this comment.
serve:static now uses pnpm exec http-server, but http-server is not listed in dependencies/devDependencies, so this script will fail unless the package is installed globally; either add http-server as a devDependency or switch to pnpm dlx http-server to preserve the previous npx behavior.
| "serve:static": "pnpm run build && pnpm exec http-server build -p 8085 -c-1 --proxy http://localhost:8085?", | |
| "serve:static": "pnpm run build && pnpm dlx http-server build -p 8085 -c-1 --proxy http://localhost:8085?", |
| ```bash | ||
| cd test | ||
| npm install | ||
| npx playwright install | ||
| pnpm install | ||
| pnpm exec playwright install | ||
| ``` |
There was a problem hiding this comment.
This README instructs cd test, but the directory in this repo is tests/ (and later the tree example also uses test/), so the setup steps will fail if followed as written.
| const wiremockBaseUrl = process.env.WIREMOCK_BASE_URL ?? "http://localhost:8080"; | ||
|
|
||
| /** Catch-all OS Places /find stub — returns a single dummy address for any postcode query. */ | ||
| function createOSPlacesCatchAllMapping(): WireMockMapping { | ||
| return { | ||
| priority: 100, // lower priority than per-test stubs | ||
| request: { | ||
| method: "GET", | ||
| urlPath: "/find", | ||
| queryParameters: { | ||
| query: { matches: ".*" }, | ||
| }, | ||
| }, | ||
| response: { | ||
| status: 200, | ||
| headers: { "Content-Type": "application/json" }, | ||
| jsonBody: { | ||
| header: { | ||
| uri: "http://wiremock/find", | ||
| query: "query=SW1A1AA", | ||
| offset: 0, | ||
| totalresults: 1, | ||
| format: "JSON", | ||
| dataset: "DPA", | ||
| lr: "EN", | ||
| maxresults: 100, | ||
| epoch: "95", | ||
| output_srs: "EPSG:27700", | ||
| }, | ||
| results: [ | ||
| { | ||
| DPA: { | ||
| UPRN: "100023336956", | ||
| UDPRN: "52640002", | ||
| ADDRESS: "10 DOWNING STREET, LONDON, SW1A 1AA", | ||
| BUILDING_NUMBER: "10", | ||
| THOROUGHFARE_NAME: "DOWNING STREET", | ||
| POST_TOWN: "LONDON", | ||
| POSTCODE: "SW1A 1AA", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| const wiremockJwtIssuer = process.env.WIREMOCK_JWT_ISSUER ?? wiremockBaseUrl; | ||
|
|
||
| async function main(): Promise<void> { | ||
| console.log(`Pushing WireMock mappings to: ${wiremockBaseUrl}`); | ||
| console.log(`JWT issuer: ${wiremockJwtIssuer}`); |
There was a problem hiding this comment.
wiremockJwtIssuer defaults to wiremockBaseUrl (http://localhost:8080), but the local Terraform config sets NHS_LOGIN_BASE_ENDPOINT_URL to the Docker-internal http://wiremock:8080, so running this script with defaults will generate tokens with an iss that the login-lambda will reject; consider defaulting WIREMOCK_JWT_ISSUER to http://wiremock:8080 for localhost runs (or update the usage docs to require setting it).
| # Dependency directories | ||
| node_modules/ | ||
| package-lock.json | ||
| pnpm-lock.yaml |
There was a problem hiding this comment.
.prettierignore no longer ignores package-lock.json, but package-lock.json files still exist under ui/, lambdas/, and tests/, so Prettier/pre-commit may start reformatting them and create noisy diffs; consider keeping package-lock.json ignored until those files are removed.
| pnpm-lock.yaml | |
| pnpm-lock.yaml | |
| package-lock.json |
|
Do you think it worth looking into https://pnpm.io/workspaces |
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.