Repository navigation
feat: accept Next.js 16.4.4 on the native Node path - #437
Open
soleil-colza wants to merge 10 commits into
Open
soleil-colza wants to merge 10 commits into
soleil-colza wants to merge 10 commits into
Conversation
The independent producer compiles its fixed public Pages fixture with Next.js 16.4.3 by default, as before, and with 16.4.4 under --next=16.4.4. A 16.4.4 run replaces only the manifest and lock, from fixture-16.4.4/, which differ from 16.4.3's in the release and its integrities alone; the published getHandler source is the same in both. The workflow runs default and off for each release, with the 16.4.4 artifacts named apart.
pnpm/action-setup refused the pinned 12.15.0 beside the repository's packageManager, 12.16.1, so no job of the producer ran. It reads the fixture's packageManager instead, the same 12.15.0 the producer checks.
… feat/native-next-16-4-4-20261102
….4.3 The native Node path pinned next@16.4.3 alone. Of what it pins, 16.4.4 changes only package.json, the twelve server runtimes and the page template factory; every other reviewed source is the same bytes. Each release now carries those, by exact bytes (node-next-release.ts), and a lookup accepts the reviewed bytes of either. Where the package is known, the bytes must be its own release's: the Pages proof refuses a package holding another release's copy of a runtime or the template, and the cache policy rewrite, the template factory and the cache write check the release the package.json names. The WebSocket and require-hook bundles are the same in both releases, so their profiles accept either package.
16.4.4 reverts the strict module factory chunks, so its Node runtime no longer installs a chunk's leading strict factory table. Its three body fingerprints, from the release's snapshot, join 16.4.3's, and a runtime proof says whether it installs such a table. Beside one that does not, a chunk leading with one is refused rather than read as strict.
… Functions
Next.js 16.4.4 compiles in fast-jwt where 16.4.3 had jsonwebtoken. Each
release now has its own fixed-source profile (node-jwt-compiler-profiles.ts),
chosen by the release the package is. fast-jwt names no code compiler;
its one Node loader read is the process.binding("buffer") its copy of
safer-buffer guards with !o.kStringMaxLength, which Node.js 24 and 26
never reach, and that read alone is exempt from the closure walker. It
has no semver dependency, and loads buffer, node:crypto, node:util and
util by those exact names.
The bootstrap guard accepts the fast-jwt record, answers the node:
builtins, and seals the crypto functions fast-jwt takes as it loads. A
CI-only test signs and verifies preview data through the guard.
The official fixtures build with 16.4.3 unless UPWIND_NATIVE_NEXT names another reviewed release, and fixtures/next-native, which builds through the whole server output, moves to 16.4.4.
|
Arkor — Preview deployments for arkorlab-upwind Use the checkbox below to choose whether this pull request gets preview deployments.
Checking this builds the latest commit and future pushes. Unchecking stops queued and future builds; a running build can finish and an existing preview stays available. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait...
 |
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
 [Medium impact] The PR appears safe to merge; the remaining test-coverage finding is non-blocking.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+15
to
+20
| vi.mock(import('../src/node-jwt-compiler-proof.ts'), async () => { | ||
| const sites = await import('../src/node-jwt-fast-sites.ts'); | ||
| return { | ||
| nodeJwtCompilerSite: (proof: unknown, _file: string, _code: string, program: unknown) => { | ||
| const node = proof === undefined ? undefined : sites.nodeFastJwtBindingSite(program); | ||
| return node === undefined ? undefined : { kind: 'loader', node }; |
There was a problem hiding this comment.
This test mocks nodeJwtCompilerSite and supplies an empty proof, so it skips the new profile’s real package, dependency and caller checks in qualifyNodeJwtCompiler. The guard integration test supplies its record by hand too. Both can pass while the adapter refuses fast-jwt builds.
Add a test that qualifies a real fast-jwt graph, uses the returned proof in the closure check, and checks the resulting guard record.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/adapter/test/node-jwt-fast-sites.test.ts
Line: 15-20
Comment:
**Fast-jwt tests bypass real qualification**
This test mocks `nodeJwtCompilerSite` and supplies an empty proof, so it skips the new profile’s real package, dependency and caller checks in `qualifyNodeJwtCompiler`. The guard integration test supplies its record by hand too. Both can pass while the adapter refuses fast-jwt builds.
Add a test that qualifies a real fast-jwt graph, uses the returned proof in the closure check, and checks the resulting guard record.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 49 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/adapter/src/node-manifest-turbopack.ts">
<violation number="1" location="packages/adapter/src/node-manifest-turbopack.ts:241">
P2: This classifier throws for a valid registration whose first element is an ordinary zero-argument call, instead of returning `false` as its contract says. Because `flatTables` applies it to every source without a catch, one such chunk aborts runtime-cohort certification; keep the `strictWrapper` probe inside the existing `try` or make that probe non-throwing.</violation>
</file>
<file name="packages/adapter/test/node-jwt-fast-sites.test.ts">
<violation number="1" location="packages/adapter/test/node-jwt-fast-sites.test.ts:15">
P2: This mock reduces the closure test to a synthetic site and `{}` proof, so it cannot catch fast-jwt qualification failures. Add a test that qualifies a real fast-jwt graph and checks the returned proof and guard record.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } | ||
| const first = listOf(table, 'elements')[0]; | ||
| return ( | ||
| isNode(first) && (first.type === 'ArrayExpression' || strictWrapper(first, true) !== undefined) |
There was a problem hiding this comment.
P2: This classifier throws for a valid registration whose first element is an ordinary zero-argument call, instead of returning false as its contract says. Because flatTables applies it to every source without a catch, one such chunk aborts runtime-cohort certification; keep the strictWrapper probe inside the existing try or make that probe non-throwing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/adapter/src/node-manifest-turbopack.ts, line 241:
<comment>This classifier throws for a valid registration whose first element is an ordinary zero-argument call, instead of returning `false` as its contract says. Because `flatTables` applies it to every source without a catch, one such chunk aborts runtime-cohort certification; keep the `strictWrapper` probe inside the existing `try` or make that probe non-throwing.</comment>
<file context>
@@ -228,6 +228,20 @@ function checkedTurbopackProgram(source: NodeManifestTurbopackSource): Node {
+ }
+ const first = listOf(table, 'elements')[0];
+ return (
+ isNode(first) && (first.type === 'ArrayExpression' || strictWrapper(first, true) !== undefined)
+ );
+}
</file context>
Suggested change
| isNode(first) && (first.type === 'ArrayExpression' || strictWrapper(first, true) !== undefined) | |
| isNode(first) && | |
| (first.type === 'ArrayExpression' || | |
| (() => { | |
| try { | |
| return strictWrapper(first, true) !== undefined; | |
| } catch { | |
| return false; | |
| } | |
| })()) |
| import { nodeFastJwtBindingSite } from '../src/node-jwt-fast-sites.ts'; | ||
|
|
||
| // The proof itself needs a whole qualified graph; here it stands for one that reads fast-jwt's site. | ||
| vi.mock(import('../src/node-jwt-compiler-proof.ts'), async () => { |
There was a problem hiding this comment.
P2: This mock reduces the closure test to a synthetic site and {} proof, so it cannot catch fast-jwt qualification failures. Add a test that qualifies a real fast-jwt graph and checks the returned proof and guard record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/adapter/test/node-jwt-fast-sites.test.ts, line 15:
<comment>This mock reduces the closure test to a synthetic site and `{}` proof, so it cannot catch fast-jwt qualification failures. Add a test that qualifies a real fast-jwt graph and checks the returned proof and guard record.</comment>
<file context>
@@ -0,0 +1,84 @@
+import { nodeFastJwtBindingSite } from '../src/node-jwt-fast-sites.ts';
+
+// The proof itself needs a whole qualified graph; here it stands for one that reads fast-jwt's site.
+vi.mock(import('../src/node-jwt-compiler-proof.ts'), async () => {
+ const sites = await import('../src/node-jwt-fast-sites.ts');
+ return {
</file context>
Review of the 16.4.4 support: - The Pages proof selects the release by the package.json's exact bytes as well as its version, as the other fixed-source proofs do; the suites copy the published 16.4.3 package.json instead of writing one. - The nested strict-table probe answers false for any program it cannot read, and the runtime cohort reads the chunks for it once. - A JWT profile's builtins are a frozen list, not a mutable Set. - The 16.4.4 producer lock may differ from 16.4.3's only in versions and in the integrities of the packages Next.js publishes. - The route-approval refusal is checked for fast-jwt too, and the docs say a Pages-only native build on 16.4.4 waits for its getHandler references.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #436. Native Node Functions accept Next.js 16.4.4 as well as 16.4.3.
Why
The native Node path qualified exact next@16.4.3 bytes and refused anything else. Since the scaffolder asks for
^16.4.x, a new native app now installs 16.4.4. Projects pinned to 16.4.3 must keep building, so both releases are supported.What 16.4.4 changes for this path
Of every source the path pins, 16.4.4 changes only these:
package.json.next-serverruntimes. They requirenext/dist/compiled/fast-jwtwhere 16.4.3 requiredjsonwebtoken(#100885).build/templates/app-page-runtime.js. The resume now emits the Turbopack bootstrap (#100907).Every other reviewed source is byte-identical between the two releases, which I checked against both installed packages: the route modules,
route-module.js,load-components.js, the evaluator, the templates,request-meta.js,route-kind.js,pages-handler.js,require-hook.jsand thewsbundle.Two more changes affect the path:
Changes
node-next-release.ts).package.json, runtime and template bytes. Lookups accept the reviewed bytes of either release.package.jsonnames.wsand require-hook profiles accept either release's package, since their bytes are the same in both.evaland noFunction.process.binding("buffer"), guarded by!o.kStringMaxLength. Node 24 and 26 exportkStringMaxLength, so that read never runs.node:builtins, and seals the crypto functions fast-jwt captures at load.--next=16.4.4. Its workflow now takes its pnpm version from the fixture: action-setup was refusing the pinned 12.15.0 next to the repository'spackageManager, so no producer job had been running.tools/native-addon-next-ci.mtsbuilds the official fixtures with a reviewed release named byUPWIND_NATIVE_NEXT(16.4.3 by default).fixtures/next-nativemoves to 16.4.4.Still to come in this PR
Checks run locally
/varvs/private/var).tsc, ESLint and oxfmt pass on every changed file, and so doknipandcheck:vocabulary.Review notes
prerender-native-modulein the Pages "proved" mode with jsonwebtoken retained. The 16.4.4 leg of the same job will do it for fast-jwt once the getHandler references are in. Until then, the unit test refuses a fast-jwt route approval the same way it refuses a jsonwebtoken one, and the closure test shows the binding read is the bundle's only refusal.Summary by CodeRabbit