fix: Node 26 transform flag; operator .env precedence + import order; MCP negotiation test; provider-view cross-process coverage (integration) - #554
Merged
Conversation
…orts (Node 26 drops --experimental-transform-types) runScript spawned every plain .ts script under node --experimental-transform-types. Node 26 removed the flag (nodejs/node#61803) and rejects it as a bad option (exit code 9), so the Verify (Node 26) leg failed on every main push. typeScriptTransformFlags (core/runtime.ts) decides from process.allowedNodeEnvironmentFlags: the transform flag where the binary accepts it (Node 22, 24), nothing on Node 26, which strips types unflagged. Unit-tested against the flag sets of each release line.
🦋 Changeset detectedLatest commit: e8f942a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09323c7b56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| */ | ||
| export const typeScriptTransformFlags = ( | ||
| allowedFlags: ReadonlySet<string> = process.allowedNodeEnvironmentFlags, | ||
| ): readonly string[] => Object.freeze(allowedFlags.has(TRANSFORM_TYPES_FLAG) ? [TRANSFORM_TYPES_FLAG] : []); |
There was a problem hiding this comment.
When the parent runs on Node 26 with NODE_OPTIONS=--no-strip-types, this branch returns no CLI flag even though the spawned child inherits that environment, so importing any ordinary typed .ts script fails instead of running. Node exposes the positive --strip-types option, and command-line options override inherited NODE_OPTIONS; select it when the transform flag is unavailable so the harness explicitly restores the TypeScript loading it requires.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Accepted — fixed in fa1164e. typeScriptTransformFlags now returns the first flag the binary accepts, strongest first: --experimental-transform-types (Node 22, 24), then --strip-types (Node 26), so the harness always names the TypeScript loading it needs on the command line and an inherited NODE_OPTIONS=--no-strip-types is outranked. Verified: on 26.8.1, NODE_OPTIONS=--no-strip-types node file.ts fails and … node --strip-types file.ts runs; on 22/24 the transform flag already outranks the negative switch. Covered by a new script-dispatch test that sets the version-appropriate switch in NODE_OPTIONS and passes on 22/24/26 (it failed on 26 before this commit).
commit: |
…ONS=--no-strip-types cannot switch TypeScript loading off Codex review on #554: with no command-line flag the child inherited the environment's --no-strip-types and failed on every typed .ts source. The helper now picks the first flag the binary accepts, strongest first: --experimental-transform-types (22, 24), then --strip-types (26). Covered by a script-dispatch test that sets the version-appropriate switch in NODE_OPTIONS and expects the source run to succeed regardless.
…fix framework-mode.md ordering (from #556)
…luate and below manifest env defaults (#469) Two findings from the #538 self-review. Precedence: a host merges the stdio server's manifest `env` block into the child environment, so the shell could not tell a manifest default from a host export and reserved both — manifest env beat the file, contrary to the documented `manifest < .env < .env.local < process.env`. The emitted stdio entry now embeds the server's normalized `env` block as build-time literals and `applyOperatorEnv` takes it as `manifestEnv`: a present variable is reserved only when its value differs from the embedded default, so a passed-through default yields to the file while a host or operator export is kept. An operator export equal to the default is indistinguishable from the pass-through and yields too; a default carrying a path token never equals its expanded value and is always kept. Host manifests are unchanged. Import timing: the layer was a statement after the consumer imports, and ESM evaluates static imports first, so module-level `process.env` reads in hook handlers and CLI route/provider modules never saw the file. A dynamic `import()` after the statement does not help either — Rspack inlines a single-chunk bundle into one scope and places the dynamic target ahead of the static imports. The layer is now a generated virtual module (`agent-bundle/launch-env-layer`) that every stdio entry, hook wrapper, and artifact CLI bin imports first, with the server module, handler, routes, providers, and state definition as static imports after it; the build marks generated modules side-effectful so a consumer `"sideEffects": false` cannot drop the bare import. The MCP shell's `loadEntry` becomes a static import for the same reason, so the console guard now covers the factory call and the running server rather than the module's top-level evaluation. Tests build each shell through the real pipeline and run it under node with a `process.env` read at module top level: manifest-only key takes the file, host-exported key keeps the host value, absent key takes the file, `AGENT_BUNDLE_ENV_FILE=none` restores the previous behaviour.
ScriptedAlchemy
changed the title
…so module-scope writes never reach the protocol stream (#469) The env-precedence follow-up made the generated stdio entry import the server module statically so the operator .env layer lands by import order — but that put the module's top level ahead of the console guard that `runGeneratedStdioMcpEntry` installs in the shell body. A `console.log` or `process.stdout.write` at module scope in a consumer's server or tool module reached stdout, which carries JSON-RPC framing, contradicting the documented guarantee that redirection precedes the consumer module's evaluation. The stdio shell now imports a generated prelude (`agent-bundle/stdio-prelude`) as its first import: it calls `redirectConsoleToStderr` from `agent-bundle/mcp-entry`, then applies the operator .env layer with the server's manifest env defaults. Hook wrappers and the artifact CLI bin keep the env-only layer (`agent-bundle/launch-env-layer`) — they legitimately write stdout. The guard has one implementation: `redirectConsoleToStderr` returns the guard already installed (recognised by `process.stdout.write` still being its redirect) instead of stacking a second, which would capture the redirect as the original and restore stdout to stderr; the lifecycle adopts the prelude's guard and restores raw stdout from it before serving. Tests: a built stdio entry whose server module writes `console.log('hello')` and `process.stdout.write('raw\n')` at module scope, driven by a real stdio client through initialize, tools/list, and tools/call, asserts both land on stderr (fails on the previous code: stderr held only the factory-time line); the entry-shell unit tests pin the prelude as the stdio entry's first import and the env-only layer for hook wrappers and the CLI bin; the mcp-entry unit test pins guard adoption and re-install after restore.
…ty so a consumer wrapper cannot stack a second guard (#469) Adoption by identity (`process.stdout.write === redirectedWrite`) broke the moment a consumer module wrapped `process.stdout.write` at module scope: the lifecycle's `redirectConsoleToStderr()` saw a foreign function, installed a second guard with the wrapper recorded as the original, and restoring for the protocol stream handed stdout to the wrapper — which still forwarded to the first redirect, so every JSON-RPC frame left on stderr and the client hung in initialize. The rule is now: while a guard is installed, `redirectConsoleToStderr()` returns it whatever `process.stdout.write` has become; `restoreProtocolStdout()` restores the real original the guard owns, writes one stderr line if a module replaced the write in the meantime (the replacement is discarded — stdout is the protocol channel and wrapping it is unsupported), and clears the installed guard so a later call installs anew. Tests: the mcp-entry unit test wraps the redirect, adopts the same guard, restores to the real stdout, and installs fresh afterwards (fails on a677371 at the adoption step); the packed stdio test's server module now also wraps `process.stdout.write` at module scope and the real client still completes initialize, tools/list, and tools/call with the wrapper's output and the warning on stderr (hangs to timeout on a677371).
… restore cannot clobber a fresh guard (#469) Two holders of the same guard could restore twice: after the first restore and a fresh install, the stale restore overwrote the fresh redirect with the old original while `installedGuard` still named the fresh guard, so adoption returned a guard that was no longer installed. A plain double restore also emitted the foreign-wrapper warning twice. The guard now records that it has restored and returns immediately on later calls.
ScriptedAlchemy
enabled auto-merge (squash)
September 4, 2026 22:46
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
1 participant
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.
Symptom
Every
mainpush fails theVerify (Node 26)leg — e.g. run 33907910106 for a69673b; 55026f0 failed identically. All plain-script tests inpackages/agent-bundle/tests/projection/script-dispatch.test.tssee exit code 9 withThe leg only runs on
mainpushes and manual dispatch (PRs run Node 24 alone), so nothing blocked and it failed silently.Root cause
runScript(agent-bundle/test,src/test/script.ts) spawns every plain.tsscript asnode --experimental-transform-types …overprocess.execPath. Node 26 removed that flag outright as a semver-major change (nodejs/node#61803, rationale in nodejs/typescript#51) and rejects it as a bad option before any script runs. This is production code —runScriptis a public export consumers call from their own Rstest suites — so any consumer testing plain scripts on Node 26 hit the same exit-9 failure.Node 26 flag facts, verified against the local 26.8.1 binary (
process.allowedNodeEnvironmentFlags,--help,process.features.typescript) andCHANGELOG_V26.md:--experimental-transform-typesbad option, exit 9)--transform-types(stable successor?)--strip-types/--experimental-strip-types--experimental-strip-typesonlyprocess.features.typescript'strip''strip''strip'(@types/node26 types it as'strip' | false— no'transform')enumin a.tsERR_UNSUPPORTED_TYPESCRIPT_SYNTAX, and no flag can enable itSo there is no transform flag to swap in: Node 26 does type stripping only.
Fix
typeScriptTransformFlags(allowedFlags = process.allowedNodeEnvironmentFlags)insrc/core/runtime.ts— feature detection, not version sniffing: the first flag the binary accepts, strongest first —--experimental-transform-types(Node 22, 24), then--strip-types(Node 26);[]for a binary accepting neither. The child runs the same binary as the parent (process.execPath), so the parent's flag set is authoritative. Naming--strip-typesexplicitly (rather than relying on the Node 26 default) means an inheritedNODE_OPTIONS=--no-strip-typescannot switch the source run off — a command-line flag outranksNODE_OPTIONS.src/test/script.tsspreads it into the spawn argv;--disable-warning=ExperimentalWarningstays unconditional (it also coversmodule.registerHooks).enum,namespace, parameter properties) now fails in the harness with Node's ownERR_UNSUPPORTED_TYPESCRIPT_SYNTAX, exactly asnode file.tsdoes there — the bundledscripts/<name>.mjsis unaffected because Rslib/SWC lowers it. Documented in the testing guide (en + zh) and the changeset.Verification
script-dispatch.test.ts→ 16 plain-script failures with exit 9.pnpm exec rstest --config rstest.projection.config.ts packages/agent-bundle/tests/projection/script-dispatch.test.ts: 49/49 pass under Node 22.23.2, 24.19.0, and 26.8.1; fullpnpm test:projectionunder Node 26: green.tests/core.test.tsrun the helper against the observed flag sets of Node 22, 24, and 26, a set with neither flag, and this process's real set; pass on Node 22, 24, and 26.--no-experimental-strip-typeson 22,--no-strip-typeson 24/26) inNODE_OPTIONSand expects a plain typed script to run anyway; it fails on Node 26 without the--strip-typesfallback and passes with it.pnpm typecheck✓,pnpm lint✓,pnpm test:unit✓ (3331 passed),pnpm docs:site:build✓ (language parity ok).Self-review
Reviewer:
gpt-5.6-sol-medium(change-risk-reviewer) on the integration diff vsorigin/main(head 9d3bc2f).runGeneratedStdioMcpEntryinstalled in the shell body, so a module-scopeconsole.log/process.stdout.writein a consumer's server or tool module reached the protocol stream — contradicting the documented guarantee that redirection precedes the consumer module's evaluation. → fixed in a677371: the stdio shell's first import is now a generated prelude (agent-bundle/stdio-prelude) that callsredirectConsoleToStderrfromagent-bundle/mcp-entryand then applies the operator.envlayer; hook wrappers and the artifact CLI bin keep the env-onlyagent-bundle/launch-env-layer. One guard implementation:redirectConsoleToStderrreturns the guard already installed instead of stacking a second, so the lifecycle adopts the prelude's guard. New tests:mcp.test.ts"redirects stdout written at module scope by the server module to stderr before the protocol stream opens" (real stdio client, initialize + tools/list + tools/call; fails on 9d3bc2f),entry-shell.test.tsprelude-first / env-only-layer pins,mcp-entry.test.tsguard adoption.docs/entry-conventions.mdand changeset469-env-precedence-followup.mdupdated.gpt-5.6-sol-medium, delta 9d3bc2f…a67737162): guard adoption by write identity stacked a second guard under a consumer wrapper overprocess.stdout.writeand restored stdout to the wrapper-over-stderr, so every JSON-RPC frame left on stderr → fixed in a279643 (while a guard is installedredirectConsoleToStderrreturns it whateverprocess.stdout.writehas become;restoreProtocolStdoutrestores the real original, warns once on stderr if a module replaced the write, and clears the installed guard; unit test wraps the redirect and fails on a677371 at adoption, packed test wraps at module scope and hung on a677371); no other findings.gpt-5.6-sol-medium, delta a677371…a2796437d):restoreProtocolStdout()not idempotent — a stale or repeated restore overwrote a fresh guard's redirect and warned twice → fixed in 88db802 (once-only restore; later calls are no-ops; two unit tests, both failing on a279643); no other findings.Review threads
core/runtime.ts): with no command-line flag on Node 26, a child inheritingNODE_OPTIONS=--no-strip-typesfails on every typed.tssource — accepted and fixed in fa1164e: the helper falls back to--strip-typeswhere the transform flag is unavailable, with a script-dispatch test covering the inherited-NODE_OPTIONSscenario on every supported Node line.Absorbed
fix/469-followup-env-precedence, cherry-pickedc71431851, feat(install): load the installed pack's operator .env in emitted MCP entries, hook wrappers, and the artifact CLI (#469) #538 itself is already onmain): fixes the two findings from feat(install): load the installed pack's operator .env in emitted MCP entries, hook wrappers, and the artifact CLI (#469) #538's self-review. Precedence — the emitted stdio entry embeds the server's normalized manifestenvblock as build-time literals andapplyOperatorEnvtakes it asmanifestEnv, reserving a present variable only when its value differs from the embedded default, so a passed-through manifest default yields to.envwhile a host or operator export is kept (documented ordermanifest < .env < .env.local < process.envnow holds). Import timing — the launch-env layer became a generated virtual moduleagent-bundle/launch-env-layerimported first by every stdio entry, hook wrapper, and artifact CLI bin (a post-statement dynamicimport()was probed and Rspack still hoisted the target's evaluation), with the server module, handler, routes, providers, and state definition as static imports after it; generated modules are marked side-effectful so a consumer"sideEffects": falsecannot drop the bare import, and the MCP shell'sloadEntryis a static import for the same reason. Tests build each shell through the real pipeline and run it undernodewith a module-top-levelprocess.envread. Docs (docs/entry-conventions.md, MCP authoring / installation / runtime-environment pages, en + zh) and changeset469-env-precedence-followup.md(#554)./tmp/pr556-port.patch, aligned to feat(providers): hand context providers the request identity, lineage tree, and read-only state/notices handles (#459) #552'sinbox+publishednotices view):docs/framework-mode.mdordering fix, and cross-process coverage intests/cli-routes-build.test.tsproving the provider request view (identity, lineage, state/notices handles) across the built CLI surfaces. Test + docs only, no changeset..superpowers/sdd/2026-08-14-…scratch notes and appends.superpowers/to the root.gitignoreso agent-tooling scratch can never be committed again. No changeset (nothing underpackages/;Changeset presentonly requires one for publishable-package changes).mainasced3998eband arrives here through the merge oforigin/main.Changesets on this branch:
node26-transform-types.mdand469-env-precedence-followup.md, both(#554);pnpm changeset status --since=origin/mainreports oneagent-bundlepatch bump.Combined-tree gate (head 9d3bc2f)
pnpm build✓ ·pnpm typecheck✓ ·pnpm lint✓ ·pnpm test:unit✓ (3370) ·pnpm test:projection✓ (172, Node 22) ·script-dispatch.test.tsunder Node 26.8.1 ✓ (49) · integrationmcp,hooks,build,package-build,emitted-artifact-effect-surface,artifact-cli-bin,cli-routes-build✓ (102) ·launch-env,entry-shell,hook-handler-contract,inspect-bundler✓ (48) · packedpacked-stdio-projection✓ ·pnpm docs:site:build✓ (parity ok).