P1: Upgrade to graphql-yoga 5 and Node 22 LTS (#176) - #194
Conversation
|
Nice upgrade — clean and minimal (great that it came down to a single typed cast for the OTel provider), and thanks for the thorough build/lint/image verification. Since this is the one change in the production-readiness train that could subtly affect the mina-explorer client, I verified the highest-risk axis empirically: yoga 5's GraphQL-over-HTTP status codes. The Explorer's archive client throws on any non-2xx before it reads the GraphQL body ( Good news — it's safe. I ran both 4.0.4 and 5.21.2 with the Explorer's exact request shape (
Both only switch to 400 under One suggestion to keep it safe: the 200-on-validation-error behavior is an implicit content-negotiation default, so a future yoga bump could flip it unnoticed. A tiny integration test alongside the existing ones would pin the Explorer contract: // the exact shape the mina-explorer client sends (no Accept header)
const res = await fetch(endpoint, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ query: '{ __definitelyNotAField }' }),
});
assert.equal(res.status, 200); // must NOT be 400
const body = await res.json();
assert.match(body.errors[0].message, /Cannot query field/); // Explorer fallback keys on thisMinor nit while here: |
|
One more small thing, on the Docker side: this bumps both stages to a floating FROM node:22-alpine@sha256:<digest> AS build
# ...
FROM node:22-alpine@sha256:<digest>(Once #192 lands, its Dependabot |
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — and thank you for actually running both versions against the Explorer's request shape rather than reasoning about it. All three done in Digest pin. Both stages now pin
The contract test. Added Your reasoning for why it's worth pinning is in the file: the 200 is an implicit content-negotiation default, so nothing about a future bump would announce a flip. This turns "someone checked once" into a standing guard — and since this PR is the upgrade itself, the guard belongs here rather than downstream. |
Closes #206. ## Problem The `v0.0.9` tag push failed at the **`Update npm`** step — [run 30540591005](https://github.com/o1-labs/Archive-Node-API/actions/runs/30540591005) — before dependencies were installed, before tests, and before `npm publish`: ``` npm error code EBADENGINE npm error Not compatible with your version of node/npm: npm@12.0.2 npm error notsup Required: {"node":"^22.22.2 || ^24.15.0 || >=26.0.0"} npm error notsup Actual: {"npm":"10.8.2","node":"v20.20.2"} ``` `npm@latest` has moved to 12.0.2, which dropped Node 20. The job pins `node-version: '20'`, so the install is refused. Nothing was published — npm still shows only `0.0.6`. This is an upstream change, not a repo regression: it breaks **any** tag pushed from today onward. ## Change ```diff - # Ensure npm 11.5.1 or later is installed - name: Update npm - run: npm install -g npm@latest + run: npm install -g npm@11 ``` `npm@11` is currently 11.19.0, engines `^20.17.0 || >=22.9.0` — satisfied by the job's Node 20.20.2, and well past the 11.5.1 that trusted publishing / OIDC requires. Pinning the major keeps patch updates flowing without another silent engine break; the unqualified `latest` is what made this a time bomb. ## Why not bump Node to 22 More future-proof, but wider: the publish job also runs the full `npm test`, so changing its Node version changes the runtime the release is validated against. #194 (`P1: Upgrade to graphql-yoga 5 and Node 22 LTS`) is already open and is the right place for that. Once it lands, this pin can be revisited. ## Verification This workflow only runs on `v*` tag pushes and `workflow_dispatch`, so PR CI cannot exercise it. The engine claim is checked directly against the registry: ``` $ npm view npm@11 version -> 11.19.0 $ npm view npm@11 engines -> { node: '^20.17.0 || >=22.9.0' } $ npm view npm@12 engines -> { node: '^22.22.2 || ^24.15.0 || >=26.0.0' } ``` Node 20.20.2 satisfies `^20.17.0` and does not satisfy `^22.22.2`, which is exactly the observed pass/fail split. Real proof comes from the retriggered `v0.0.9` publish after this merges. ## Follow-up `v0.0.9` is tagged at `3d254f7` but published nothing. Re-running the failed run will not help — a tag-push event uses the workflow file as it exists at the tagged commit, which still has the broken step. The tag needs to be moved to the commit containing this fix and re-pushed. Since no npm version was consumed, that is a clean operation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Verdict: MERGEABLE ✅ Second pass, focused purely on the wire protocol. I installed the exact resolved trees — What I checked
One correction to the framing in the first review comment, in the PR's favour: Non-blocking nits1. Rebase conflict silently reverts #207. The branch is 3 behind --- a/.github/workflows/publish-npm.yml
+++ b/.github/workflows/publish-npm.yml
@@
- uses: actions/setup-node@v4
with:
node-version: '22'
- # Trusted publishing needs npm 11.5.1 or later. Pin the major rather than
- # tracking `latest`: npm 12 requires Node >= 22.22.2, so `npm@latest`
- # fails EBADENGINE against the Node 20 pinned above.
+ # Trusted publishing needs npm 11.5.1 or later. Pin the major rather than
+ # tracking `latest` so a future npm major cannot change publish semantics
+ # or raise its Node floor out from under the version pinned above.
- name: Update npm
run: npm install -g npm@11( 2. --- a/.github/workflows/run-tests.yaml
+++ b/.github/workflows/run-tests.yaml
@@
- name: Checkout code
uses: actions/checkout@v2
+ - uses: actions/setup-node@v4
+ with:
+ node-version: '22'
+
- name: Install dependencies
run: npm ci3. No --- a/package.json
+++ b/package.json
@@
"author": "O(1) Labs <build@o1labs.org>",
"license": "ISC",
+ "engines": {
+ "node": ">=22.12.0"
+ },
"devDependencies": {4. --- a/package.json
+++ b/package.json
@@
- "graphql-yoga": "^5.21.2",
+ "graphql-yoga": "5.21.2",(Fine to skip if the intent is to let #192's Dependabot drive yoga bumps — the contract test is the real guard either way.) 5. Contract test could guard more once #195 lands. It builds a bare test('graphql-response+json is opt-in only', async () => {
const yoga = buildYoga(/* real config */);
const res = await yoga.fetch('http://localhost/', {
method: 'POST',
headers: { 'content-type': 'application/json', accept: 'application/graphql-response+json' },
body: JSON.stringify({ query: '{ __definitelyNotAField }' }),
});
// documents that 400 is reachable *only* under this Accept — if a future
// yoga makes it the default, the 200 test above starts failing and this
// one explains why.
assert.strictEqual(res.status, 400);
});6. FYI, not a defect: Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d689ff1 to
0227094
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving. I re-verified the round-1 findings against the rebased head (0227094) rather than the diff alone, plus the two things the rebase could plausibly have broken.
The rebase hazard is clear. .github/workflows/publish-npm.yml:38 still pins npm install -g npm@11. #207's fix survived — only the surrounding comment was rewritten (the rationale moved from "npm 12 requires Node ≥22.22.2 so npm@latest fails EBADENGINE against Node 20" to a version-agnostic phrasing). I read the whole file at the head rather than trusting the diff, because a revert here would have been invisible in a green CI run. It's clean, and that was the single highest-risk item in this PR.
Node 22 is now genuinely exercised. run-tests.yaml:28-30 adds the missing actions/setup-node@v4 with node-version: '22' inside the Run-Tests job (the required check), after checkout and before npm ci. I pulled the actual job log for this head (run 32755595604, job 97522270868): node-version: 22 → node: v22.23.2. Same for unit-tests. The lightnet suite this PR cited as Node-22 evidence is now actually running on 22 rather than on the runner default.
The dependency tree is the one that was validated in round 1. Read the lockfile at head: graphql-yoga 5.21.2, @envelop/core 5.5.1, @envelop/graphql-jit 11.1.1, @envelop/disable-introspection 9.1.1, @envelop/opentelemetry 9.1.1, @envelop/types 5.2.1, single graphql 16.8.1. Exactly the tree round 1 tested, so those results carry over. No envelop major mismatch — this PR actually removes one that exists on main today (root @envelop/core 4.0.3 alongside a nested 5.5.1 under @graphql-tools/executor-common).
Downstream contracts re-checked independently. I unpacked the published @envelop/disable-introspection@9.1.1 tarball: it is a thin wrapper around graphql-js's NoSchemaIntrospectionCustomRule, identical in kind to v5. Running that rule directly: { __typename } → OK, { nope } → Cannot query field "nope" on type "Query"., { __schema … } → blocked. So mina-explorer's tier detection and mina-explorer-api's { __typename } probe are both safe even with introspection disabled — __typename returns String, not an introspection type.
The base image digest is real. sha256:16e22a55… resolves on Docker Hub to a genuine multi-arch node:22-alpine index (annotation org.opencontainers.image.version: 22-alpine, base alpine:3.24, created 2026-06-23; amd64/arm/arm64/s390x). I pulled and ran it: node v22.23.1, which satisfies the new engines floor. Both stages use the same digest, which is right.
tests/unit/yoga-http-contract.test.ts pins the right three things — validation error = HTTP 200, message matches /Cannot query field/, valid query unaffected — using the Explorer's exact request shape (POST, content-type: application/json, no Accept header). Confirmed it actually executes in CI: # Subtest: GraphQL-over-HTTP contract, 3 pass / 0 fail.
Non-blocking nits
-
Docs still say Node 20.
docs/getting-started.md:20(Node.js 20+) and:53(Node 20+ supports --env-file) now contradict theengines: ">=22.12.0"this PR adds, and Node 20 went EOL 2026-04-30. It won't hard-break (no.npmrc, soengine-strictis off and npm only warns), but this PR creates the inconsistency and it's a two-line fix:-- **Path A:** Node.js 20+ (`node --version`) and a reachable archive-node Postgres database +- **Path A:** Node.js 22+ (`node --version`) and a reachable archive-node Postgres database
-Or via a `.env` file (Node 20+ supports `--env-file` natively): +Or via a `.env` file (Node 22+ supports `--env-file` natively):
-
.github/workflows/nightly-devnet-dump.yaml:56is stillnode-version: '20'and isn't in this diff. Every other workflow is migrated; after merge that nightly runs an EOL runtime against a package declaring>=22.12.0and will logEBADENGINEonnpm ci. Worth bumping in the same PR. (live-integration.yaml:28was already on 22.) -
enginesis declarative only — no.npmrc, so npm warns rather than fails. Fine as-is, but it is a real breaking change for npm consumers; #198's versioning policy now lists "minimum Node runtime" as a breaking-change category, so this is the first thing that category applies to. -
The cast at
src/server/plugins.ts:28papers over a duplicate@opentelemetry/api— root1.4.1(pinned bysdk-trace-base@1.21.0/exporter-jaeger@1.21.0) vs1.9.1nested under@envelop/opentelemetry@9.1.1. Two copies means two context globals; harmless here because the provider is passed explicitly and there's no other instrumentation, but the cast is the symptom, not the cause. Bumpingsdk-trace-base+exporter-jaegerto 2.x would dedupe and let the cast go. Follow-up. -
The new test builds a bare
createYoga, notbuildServer— so it pins yoga's default HTTP contract, not the production stack's (no jit plugin, no introspection disabling, no CORS, no masking). A future plugin that rewrites status codes would slip past it. Threading a stubbedGraphQLContextintobuildServerwould close that.
Merge ordering — please read before merging. This PR and #189 both rewrite the same two FROM lines and disagree on the base: this one moves to node:22-alpine@sha256:16e22a55…, #189 pins node:20-alpine@sha256:fb4cd12c…. Both digests are genuine official images. This is a textual and semantic conflict of the same class as the npm@11 pin — a careless resolution silently reverts one side. Merge this PR first, then rebase #189 onto it, keeping the node:22 digest and layering tini/HEALTHCHECK on top. Resolving the other way would enshrine an EOL base. I verified that apk add --no-cache tini and BusyBox wget both work identically on the node:22 base, so #189's hardening is safe on top of this.
Also note src/server/plugins.ts is touched by this PR, #188 and #193 in the same ~20 lines — all semantically compatible, just needs a careful three-way resolution.
Downstream: none. Validation errors stay HTTP 200, the Cannot query field marker is byte-identical graphql-js text, masking default unchanged, no schema change.
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0227094 to
81acf79
Compare
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
81acf79 to
ae45307
Compare
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ae45307 to
f9b0ac0
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-approving after the rebase (the previous approval was dismissed by the force-push).
The rebase changed real content here, so I re-checked at the head rather than from the diff. Both changes are correct adaptations to a main that moved:
1. The Dockerfile hazard is resolved the right way round. When I reviewed, this PR replaced an unpinned FROM node:20-alpine. #189 has since merged and pinned node 20 by digest, so the PR now replaces the pinned line:
-FROM node:20-alpine@sha256:fb4cd12c85ee03686f6af5362a0b0d56d50c58a04632e6c0fb8363f609372293
+FROM node:22-alpine@sha256:16e22a550f3863206a3f701448c45f7912c6896a62de43add43bb9c86130c3e2
Both stages bumped together, and #189's hardening survives intact at the head: tini installed and ENTRYPOINT ["/sbin/tini", "--"] (line 38), USER nodeuser (28), HEALTHCHECK on /healthcheck via BusyBox wget -qO- (33–34). Node 20 went EOL 2026-04-30 and engines: ">=22.12.0" does not accept it, so this ordering — #189 first, then this — was the correct one.
2. package.json no longer adds the keywords array — #198 merged it in the meantime. Correct, not a drop.
Re-verified the things that a rebase could have silently reverted:
publish-npm.yml:39still readsnpm install -g npm@11— #207's fix survived; only the comment around it was rewritten. Checked at the head, because a revert here would be invisible in green CI.run-tests.yamlhassetup-node@v4withnode-version: '22', andenginesdeclares">=22.12.0".- Lockfile still carries full
resolved/integritymetadata for all 1579 entries; the delta versus main is the envelop duplicate removal plus the yoga 5 tree, as before.
Non-blocking: docs still say "Node.js 20+"; nightly-devnet-dump.yaml is still on node 20.
Bring the server stack onto current majors: - graphql-yoga 4 → 5, @envelop/core 4 → 5, @envelop/graphql-jit 6 → 11, @envelop/disable-introspection 5 → 9, @envelop/opentelemetry 5 → 9. - Node 20 → 22 LTS across the Dockerfile, Volta pin, and the lint / unit-test / publish / smoke-load workflows. The only code change required is a targeted cast where useOpenTelemetry now types its provider argument against a duplicate @opentelemetry/api copy; the cast targets the exact expected parameter type. Build, unit tests, lint, and prettier all pass; the node:22 image builds and runs v22.x locally. Note: the OpenTelemetry SDK is intentionally not bumped here — the modern OTel packages drop the Jaeger exporter in favour of OTLP, so clearing the remaining @opentelemetry/* audit advisories is a separate Jaeger→OTLP migration. Closes #176. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f9b0ac0 to
b286715
Compare
What & why
Part of the production-readiness epic (#163). Closes #176.
Key server deps were a major behind. This brings them current:
@envelop/core4 → 5,@envelop/graphql-jit6 → 11,@envelop/disable-introspection5 → 9,@envelop/opentelemetry5 → 9Code impact
Minimal — a single targeted cast:
@envelop/opentelemetry@9types its provider argument against a duplicate@opentelemetry/apicopy, soprovideris cast to the exact parameter typeuseOpenTelemetryexpects. Everything else compiled unchanged.Verification
npm run build— cleannpm run test:unit— all passnpm run lint/npx prettier --debug-check .— cleanFollow-up (not in this PR)
The OpenTelemetry SDK is intentionally not bumped — modern OTel drops the Jaeger exporter for OTLP, so clearing the remaining
@opentelemetry/*audit highs is a separate Jaeger→OTLP migration. With Yoga on 5, the graphql-armor meta package (#164 currently uses the individual sub-plugins due to the old envelop-4 peer) could also be revisited.🤖 Generated with Claude Code