Skip to content

fix(bundler): preserve framework loader paths#5930

Closed
killagu wants to merge 1 commit intonextfrom
agent/egg-dev/0263e5d5
Closed

fix(bundler): preserve framework loader paths#5930
killagu wants to merge 1 commit intonextfrom
agent/egg-dev/0263e5d5

Conversation

@killagu
Copy link
Copy Markdown
Contributor

@killagu killagu commented May 6, 2026

Summary

  • record framework eggPaths and resolved plugin path metadata into the startup manifest
  • replay bundled loader metadata before runtime plugin resolution so framework config/plugin files do not collapse onto the app output dir
  • normalize the new loader manifest extension and patch generated worker framework paths for bundled runtime startup

Tests

  • pnpm exec vitest run packages/core/test/loader/mixin/load_plugin.test.ts --testTimeout 20000
  • cd tools/egg-bundler && pnpm exec vitest run test/EntryGenerator.test.ts test/ManifestLoader.test.ts --testTimeout 20000
  • pnpm --filter @eggjs/core typecheck
  • pnpm --filter @eggjs/egg-bundler typecheck
  • pnpm --filter @eggjs/egg-bundler lint
  • pnpm exec oxfmt --check packages/core/src/loader/egg_loader.ts packages/core/test/loader/mixin/load_plugin.test.ts tools/egg-bundler/src/lib/EntryGenerator.ts tools/egg-bundler/src/lib/ManifestLoader.ts tools/egg-bundler/test/EntryGenerator.test.ts tools/egg-bundler/test/ManifestLoader.test.ts

Summary by CodeRabbit

  • New Features

    • Bundle manifest support for the egg loader so startup manifest data can influence loader egg paths, plugin metadata, and resolution.
    • Public exports for manifest handling (ManifestStore, StartupManifest) to enable manifest-based bundle configuration.
    • Improved framework path patching and virtualization for bundled applications.
  • Tests

    • Added/updated tests for manifest-driven loader behavior, egg loader entries, framework path patching, and plugin/path resolution.

Copilot AI review requested due to automatic review settings May 6, 2026 09:25
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an eggLoader manifest extension end-to-end: bundler normalizes and emits an eggLoader section (eggPaths + plugin metadata), runtime EntryGenerator emits path-patching helpers, and EggLoader reads/applies/collects manifest plugin info and eggPaths during plugin load. Tests and core exports updated to exercise this flow.

Changes

Loader Manifest Integration

Layer / File(s) Summary
Data Model
packages/core/src/loader/egg_loader.ts, tools/egg-bundler/src/lib/ManifestLoader.ts
Introduce LOADER_MANIFEST_EXTENSION = 'eggLoader' and interfaces LoaderManifestPluginInfo / LoaderManifestExtension to model eggPaths and plugin metadata.
Bundler Manifest Normalization
tools/egg-bundler/src/lib/ManifestLoader.ts
#normalizeExtensions recognizes eggLoader, normalizes eggPaths and plugin path entries to manifest-relative form.
Runtime Entry (path patching)
tools/egg-bundler/src/lib/EntryGenerator.ts
Emit runtime helpers (__packageName, __packageRoot, __toOutputPath, __loaderEggPaths, __fallbackFrameworkPaths, __frameworkPaths, __isOutputRootPath, __patchFrameworkPaths) and apply path patches to Application/Agent before wiring framework modules.
Core Loader Wiring
packages/core/src/loader/egg_loader.ts
EggLoader consults bundle store for manifest-provided eggPaths in getEggPaths; loadPlugin() invokes #applyManifestPluginInfo(allPlugins) to merge manifest plugin fields and #collectLoaderManifestExtension() to prepare serialized loader extension. Adds helpers #toManifestAbsolute, #toManifestRelative, and #isBundleOutputRootPath.
Tests & Public API
packages/core/test/loader/mixin/load_plugin.test.ts, tools/egg-bundler/test/EntryGenerator.test.ts, tools/egg-bundler/test/ManifestLoader.test.ts, packages/core/src/index.ts
Tests added/updated to validate applying bundled eggLoader entries and framework-path virtualization. ManifestStore and StartupManifest exported from core index for test usage; tests reset bundle store in teardown.

Sequence Diagram

sequenceDiagram
    participant Bundler as Egg Bundler
    participant MS as ManifestStore
    participant Loader as EggLoader
    participant App as Application

    Bundler->>Bundler: normalize `eggLoader` extension (eggPaths, plugins)
    Bundler->>Bundler: emit runtime path-patching helpers in EntryGenerator
    Bundler->>MS: produce StartupManifest / set bundle store

    App->>Loader: instantiate EggLoader(baseDir)
    Loader->>MS: query bundle store for manifest extension
    MS-->>Loader: return `eggLoader` (eggPaths, plugins)

    Loader->>Loader: getEggPaths() — use manifest eggPaths if present
    Loader->>Loader: loadPlugin()
    Loader->>Loader: applyManifestPluginInfo(allPlugins)
    Loader->>Loader: collectLoaderManifestExtension()
    Loader-->>App: plugins and paths resolved using manifest
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • eggjs/egg#5906: Related manifest/bundle-store work consumed by these changes.
  • eggjs/egg#5844: Prior EggLoader manifest consumption and related wiring.
  • eggjs/egg#5921: Related bundler runtime path-mapping and manifest integration.

Suggested reviewers

  • fengmk2
  • gxkl
  • jerryliang64
  • akitaSummer

"I hop through manifests, tidy and bright,
I map eggPaths by soft moonlight.
I patch the framework, stitch each trail,
Bundle plugins snug within my tail.
Tests pass as I bound out of sight."

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding manifest support to preserve framework loader paths during bundling, which is the core objective across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/egg-dev/0263e5d5

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 6, 2026

Deploying egg with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7a1c06f
Status: ✅  Deploy successful!
Preview URL: https://72e7f907.egg-cci.pages.dev
Branch Preview URL: https://agent-egg-dev-0263e5d5.egg-cci.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 6, 2026

Deploying egg-v3 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7a1c06f
Status: ✅  Deploy successful!
Preview URL: https://c8ee0cfa.egg-v3.pages.dev
Branch Preview URL: https://agent-egg-dev-0263e5d5.egg-v3.pages.dev

View logs

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.23%. Comparing base (aca1168) to head (7a1c06f).
⚠️ Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #5930      +/-   ##
==========================================
+ Coverage   85.18%   85.23%   +0.04%     
==========================================
  Files         668      668              
  Lines       19284    19330      +46     
  Branches     3782     3801      +19     
==========================================
+ Hits        16428    16475      +47     
+ Misses       2464     2463       -1     
  Partials      392      392              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a mechanism to persist and restore the EggLoader state using a manifest extension named eggLoader. It includes changes to EggLoader for collecting and applying manifest information, updates to the egg-bundler to handle these extensions during the bundling process, and patches the framework's customEggPaths to ensure correct path resolution in bundled environments. Feedback is provided regarding the use of absolute path resolution in isBundleOutputRootPath checks to ensure consistency across different working directories.

Comment thread packages/core/src/loader/egg_loader.ts Outdated
}

#isBundleOutputRootPath(filepath: string): boolean {
return path.resolve(filepath) === path.resolve(this.options.baseDir);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When checking if a path matches the bundle output root, it is safer to resolve filepath relative to this.options.baseDir rather than the current working directory (process.cwd()). This ensures consistent behavior regardless of where the process was started, especially in environments where the application might be executed from a different directory.

Suggested change
return path.resolve(filepath) === path.resolve(this.options.baseDir);
return path.resolve(this.options.baseDir, filepath) === path.resolve(this.options.baseDir);
References
  1. When using a global bundle store, verify that its baseDir matches the requested baseDir to prevent using an incorrect manifest in multi-application environments.

),
);
const __isOutputRootPath = (filepath: unknown) =>
typeof filepath === 'string' && path.resolve(filepath) === path.resolve(__outputDir);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Resolving filepath relative to __outputDir ensures that relative paths are correctly handled even if the process is started from a directory other than the bundle output root. This improves the robustness of the generated entry script.

Suggested change
typeof filepath === 'string' && path.resolve(filepath) === path.resolve(__outputDir);
typeof filepath === 'string' && path.resolve(__outputDir, filepath) === path.resolve(__outputDir);
References
  1. When filtering file paths, avoid broad substring checks like includes('node_modules'). Instead, match against exact path segments to prevent incorrect matches on similarly named directories or files.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the startup manifest and bundled runtime bootstrap so Egg’s loader can preserve framework eggPaths and plugin path metadata across bundling, preventing framework config/plugin resolution from collapsing onto the bundle output directory.

Changes:

  • Record loader metadata (eggPaths + plugin path/deps metadata) into a new extensions.eggLoader startup-manifest extension and replay it at runtime during plugin/eggPaths resolution.
  • Normalize the new eggLoader extension paths when loading manifests in @eggjs/egg-bundler.
  • Patch bundled worker entry to virtualize framework customEggPaths() so framework paths resolve under <outputDir>/node_modules/... before startEgg() runs.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/core/src/loader/egg_loader.ts Collects and replays extensions.eggLoader (eggPaths + plugin metadata) to stabilize loader resolution in bundled runtime.
packages/core/test/loader/mixin/load_plugin.test.ts Adds coverage that bundled loader manifest eggPaths/plugin paths are applied; resets bundle store after tests.
tools/egg-bundler/src/lib/ManifestLoader.ts Normalizes extensions.eggLoader paths via the existing moduleMap normalization flow.
tools/egg-bundler/src/lib/EntryGenerator.ts Injects runtime patching of framework customEggPaths() to ensure node_modules-based framework paths in the bundle output.
tools/egg-bundler/test/ManifestLoader.test.ts Updates manifest normalization expectations to include the new eggLoader extension output.
tools/egg-bundler/test/EntryGenerator.test.ts Adds an execution test verifying framework eggPaths are virtualized prior to startEgg().
tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap Updates canonical worker-entry snapshot to reflect the new runtime patch logic.

Comment on lines +293 to +300
const __packageRoot = (specifier: string) => path.join(__outputDir, 'node_modules', ...specifier.split('/'));
const __frameworkPaths = Array.from(
new Set(
[__packageRoot(__framework), __framework === 'egg' ? undefined : __packageRoot('egg')].filter(
(filepath): filepath is string => typeof filepath === 'string',
),
),
);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/core/src/loader/egg_loader.ts (1)

954-997: 💤 Low value

LGTM — apply/collect lifecycle is ordered correctly.

#applyManifestPluginInfo runs at line 477 (after #extendPlugins, before getPluginPath/#mergePluginConfig in the loop), so manifest paths can preempt the lookup-by-package fallback when the bundled config sets path to the bundle root sentinel. The (!plugin.path || this.#isBundleOutputRootPath(plugin.path)) predicate is the right gate — explicit non-root paths from app/custom plugin configs still win.

#collectLoaderManifestExtension at line 526 runs after the resolution loop, so every entry in this.allPlugins already has plugin.path populated by getPluginPath, and #mergePluginConfig has filled version/dependencies/etc. from package.json.

One subtle but correct design choice worth calling out: this method intentionally uses this.manifest while getEggPaths uses ManifestStore.getBundleStore(). That asymmetry is required because getEggPaths is invoked from the constructor at line 162 — before this.manifest is assigned at line 186-188 — whereas #applyManifestPluginInfo runs during loadPlugin() after construction. Worth a one-line comment to prevent future "consistency" refactors that would break the constructor path.

📝 Optional clarifying comment
   `#applyManifestPluginInfo`(allPlugins: Record<string, EggPluginInfo>): void {
+    // NOTE: `getEggPaths` reads the bundle store directly because it runs
+    // in the constructor before `this.manifest` is assigned. Here we can
+    // safely go through `this.manifest`, which already accounts for the
+    // bundle store via ManifestStore.load fallback.
     const extension = this.manifest.getExtension(LOADER_MANIFEST_EXTENSION) as LoaderManifestExtension | undefined;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/loader/egg_loader.ts` around lines 954 - 997, Add a
one-line clarifying comment above the private method
`#collectLoaderManifestExtension` explaining why it accesses this.manifest
directly (instead of using ManifestStore.getBundleStore() like getEggPaths
does): note that getEggPaths is called from the constructor before this.manifest
is assigned, whereas `#collectLoaderManifestExtension` runs later during
loadPlugin(), so the asymmetry is intentional to avoid breaking the constructor
path; reference getEggPaths, ManifestStore.getBundleStore, the constructor, and
this.manifest in the comment so future maintainers understand the reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 954-997: Add a one-line clarifying comment above the private
method `#collectLoaderManifestExtension` explaining why it accesses this.manifest
directly (instead of using ManifestStore.getBundleStore() like getEggPaths
does): note that getEggPaths is called from the constructor before this.manifest
is assigned, whereas `#collectLoaderManifestExtension` runs later during
loadPlugin(), so the asymmetry is intentional to avoid breaking the constructor
path; reference getEggPaths, ManifestStore.getBundleStore, the constructor, and
this.manifest in the comment so future maintainers understand the reason.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c045f664-b1e6-4374-a5e9-ff6cf7bb95ce

📥 Commits

Reviewing files that changed from the base of the PR and between 0389f2d and 8ba5f29.

⛔ Files ignored due to path filters (1)
  • tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/core/src/loader/egg_loader.ts
  • packages/core/test/loader/mixin/load_plugin.test.ts
  • tools/egg-bundler/src/lib/EntryGenerator.ts
  • tools/egg-bundler/src/lib/ManifestLoader.ts
  • tools/egg-bundler/test/EntryGenerator.test.ts
  • tools/egg-bundler/test/ManifestLoader.test.ts

@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 8ba5f29 to 1972f2a Compare May 6, 2026 09:54
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tools/egg-bundler/src/lib/EntryGenerator.ts`:
- Around line 298-304: Replace the current reconstruction of __frameworkPaths
with the eggPaths recorded in the loader manifest (use
manifest.extensions.eggLoader.eggPaths as the primary source) and only fall back
to the existing heuristic that builds from __framework and __packageRoot when
that manifest value is missing or empty; update the code that defines
__frameworkPaths to prefer manifest.extensions.eggLoader.eggPaths (while keeping
type filtering like the current .filter((filepath): filepath is string => ...))
and leave the current array-from-new-Set/__packageRoot logic as the fallback to
ensure customEggPaths() works for stacked/nested frameworks.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d5c1d666-79ed-4178-a69f-a81d11026e77

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba5f29 and 1972f2a.

⛔ Files ignored due to path filters (1)
  • tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/core/src/loader/egg_loader.ts
  • packages/core/test/loader/mixin/load_plugin.test.ts
  • tools/egg-bundler/src/lib/EntryGenerator.ts
  • tools/egg-bundler/src/lib/ManifestLoader.ts
  • tools/egg-bundler/test/EntryGenerator.test.ts
  • tools/egg-bundler/test/ManifestLoader.test.ts

Comment thread tools/egg-bundler/src/lib/EntryGenerator.ts
Copilot AI review requested due to automatic review settings May 6, 2026 10:08
@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 1972f2a to 8af2469 Compare May 6, 2026 10:08
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 398-405: The current logic in EggLoader replaces runtime eggPaths
with the manifest snapshot when a bundleStore manifest provides
extension.eggPaths; instead, merge those manifest paths with the existing
runtime paths from customEggPaths() so worker-preserved non-root paths are kept.
In the block that reads bundleStore via ManifestStore.getBundleStore() and
inspects (bundleStore.getExtension(LOADER_MANIFEST_EXTENSION) as
LoaderManifestExtension), combine extension.eggPaths (mapped through
this.#toManifestAbsolute) with the result of this.customEggPaths() (or whatever
runtime source EggLoader uses) rather than returning only the manifest list,
producing a unified de-duplicated array used by EggLoader/startup.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 856e1936-aab6-4736-ae52-0b3ca5675656

📥 Commits

Reviewing files that changed from the base of the PR and between 1972f2a and 8af2469.

⛔ Files ignored due to path filters (1)
  • tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/core/src/loader/egg_loader.ts
  • packages/core/test/loader/mixin/load_plugin.test.ts
  • tools/egg-bundler/src/lib/EntryGenerator.ts
  • tools/egg-bundler/src/lib/ManifestLoader.ts
  • tools/egg-bundler/test/EntryGenerator.test.ts
  • tools/egg-bundler/test/ManifestLoader.test.ts

Comment thread packages/core/src/loader/egg_loader.ts
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 8af2469 to 652508e Compare May 6, 2026 10:46
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/core/src/loader/egg_loader.ts (2)

70-82: 💤 Low value

Optional: dedupe LoaderManifestExtension/LoaderManifestPluginInfo across packages.

The same shape is declared again in tools/egg-bundler/src/lib/ManifestLoader.ts (lines 41-50) with a slightly different surface (an extra [key: string]: unknown index signature). Since the bundler reads from the same on-disk manifest the runtime serializes here, divergence between the two definitions can silently drift (e.g. fields added to runtime LoaderManifestPluginInfo like version are not represented on the bundler side). Consider exporting these from @eggjs/core (e.g. alongside StartupManifest) and importing them in ManifestLoader.ts so both sides stay in lock-step.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/loader/egg_loader.ts` around lines 70 - 82, The two types
LoaderManifestPluginInfo and LoaderManifestExtension are duplicated between
packages and can drift; export these interfaces from the core package (where
they are currently declared) and update the
tools/egg-bundler/src/lib/ManifestLoader.ts to import them instead of
redeclaring; ensure the exported symbols (LoaderManifestPluginInfo,
LoaderManifestExtension) are added to the public API (e.g., index exports) so
ManifestLoader.ts can consume the exact same shapes and remove its local
duplicate declaration.

987-1003: 💤 Low value

Recollecting into a bundle store overwrites the persisted extension at runtime — verify this is intentional.

When this.manifest is the bundle store (i.e. runtime startup with a bundled manifest), setExtension('eggLoader', …) registers a fresh extension into #extensionCollector, which getExtension returns in preference to this.data.extensions.eggLoader. The newly-collected payload is derived from this.allPlugins/this.eggPaths which already carry the manifest data merged into them, so functionally it's a near round-trip; but this.eggPaths here also includes paths preserved from customEggPaths() that may not normalize cleanly (#toManifestRelative will produce ../… for paths outside baseDir). If any later code path re-reads eggLoader from the same store, it will see the runtime-merged form rather than the bundler-normalized one. If the intent is to only collect during manifest-generation phase, gating this on metadataOnly/non-bundle store would make the contract more obvious.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/loader/egg_loader.ts` around lines 987 - 1003, The method
`#collectLoaderManifestExtension` currently unconditionally calls
this.manifest.setExtension(...) which overwrites a bundled runtime store
extension with a runtime-merged payload derived from this.allPlugins and
this.eggPaths (processed via `#toManifestRelative`), causing non-normalized paths
from customEggPaths to replace the bundler-normalized data; change this to only
collect/set the extension when operating in manifest-generation mode (e.g., when
metadataOnly is true or when this.manifest is not the bundle store) so bundled
manifests are not mutated at runtime—add a guard in
`#collectLoaderManifestExtension` that checks the store mode (or a metadataOnly
flag) before calling setExtension/getExtension and leave existing bundled
extension data intact when running from a bundle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/core/src/loader/egg_loader.ts`:
- Around line 70-82: The two types LoaderManifestPluginInfo and
LoaderManifestExtension are duplicated between packages and can drift; export
these interfaces from the core package (where they are currently declared) and
update the tools/egg-bundler/src/lib/ManifestLoader.ts to import them instead of
redeclaring; ensure the exported symbols (LoaderManifestPluginInfo,
LoaderManifestExtension) are added to the public API (e.g., index exports) so
ManifestLoader.ts can consume the exact same shapes and remove its local
duplicate declaration.
- Around line 987-1003: The method `#collectLoaderManifestExtension` currently
unconditionally calls this.manifest.setExtension(...) which overwrites a bundled
runtime store extension with a runtime-merged payload derived from
this.allPlugins and this.eggPaths (processed via `#toManifestRelative`), causing
non-normalized paths from customEggPaths to replace the bundler-normalized data;
change this to only collect/set the extension when operating in
manifest-generation mode (e.g., when metadataOnly is true or when this.manifest
is not the bundle store) so bundled manifests are not mutated at runtime—add a
guard in `#collectLoaderManifestExtension` that checks the store mode (or a
metadataOnly flag) before calling setExtension/getExtension and leave existing
bundled extension data intact when running from a bundle.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d2e32a6-4448-463d-818f-c79a43191718

📥 Commits

Reviewing files that changed from the base of the PR and between 8af2469 and 652508e.

⛔ Files ignored due to path filters (1)
  • tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/core/src/loader/egg_loader.ts
  • packages/core/test/loader/mixin/load_plugin.test.ts
  • tools/egg-bundler/src/lib/EntryGenerator.ts
  • tools/egg-bundler/src/lib/ManifestLoader.ts
  • tools/egg-bundler/test/EntryGenerator.test.ts
  • tools/egg-bundler/test/ManifestLoader.test.ts

@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 652508e to 82e60d2 Compare May 6, 2026 11:47
Copilot AI review requested due to automatic review settings May 6, 2026 11:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment on lines +317 to +326
Object.defineProperty(proto, 'customEggPaths', {
configurable: true,
value: function () {
const originalPaths = typeof original === 'function' ? original.call(this) : [];
const preservedPaths = Array.isArray(originalPaths)
? originalPaths.filter((p: unknown) => !__isOutputRootPath(p))
: [];
return Array.from(new Set([...__frameworkPaths, ...preservedPaths]));
},
});
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tools/egg-bundler/src/lib/EntryGenerator.ts (1)

313-327: ⚡ Quick win

Defensive check for non-configurable customEggPaths is missing in the generated __patchFrameworkPaths.

Object.defineProperty throws TypeError: Cannot redefine property: customEggPaths when the property descriptor has configurable: false. Standard ES class methods are configurable, but if a framework uses Object.defineProperty(..., { configurable: false }) to define customEggPaths, the bundled worker startup will crash with no fallback.

🛡️ Proposed defensive patch in the emitted template
 const __patchFrameworkPaths = (Clazz: unknown) => {
   const proto = (Clazz as { prototype?: Record<string, unknown> } | undefined)?.prototype;
   if (!proto) return;
   const original = proto.customEggPaths;
-  Object.defineProperty(proto, 'customEggPaths', {
-    configurable: true,
-    value: function () {
-      const originalPaths = typeof original === 'function' ? original.call(this) : [];
-      const preservedPaths = Array.isArray(originalPaths)
-        ? originalPaths.filter((p: unknown) => !__isOutputRootPath(p))
-        : [];
-      return Array.from(new Set([...__frameworkPaths, ...preservedPaths]));
-    },
-  });
+  try {
+    Object.defineProperty(proto, 'customEggPaths', {
+      configurable: true,
+      value: function () {
+        const originalPaths = typeof original === 'function' ? original.call(this) : [];
+        const preservedPaths = Array.isArray(originalPaths)
+          ? originalPaths.filter((p: unknown) => !__isOutputRootPath(p))
+          : [];
+        return Array.from(new Set([...__frameworkPaths, ...preservedPaths]));
+      },
+    });
+  } catch {
+    // customEggPaths is non-configurable; fall back to proto assignment (may silently fail in strict mode)
+    (proto as { customEggPaths?: unknown }).customEggPaths = function (this: unknown) {
+      const originalPaths = typeof original === 'function' ? (original as (...a: unknown[]) => unknown).call(this) : [];
+      const preservedPaths = Array.isArray(originalPaths)
+        ? originalPaths.filter((p: unknown) => !__isOutputRootPath(p))
+        : [];
+      return Array.from(new Set([...__frameworkPaths, ...preservedPaths]));
+    };
+  }
 };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/egg-bundler/src/lib/EntryGenerator.ts` around lines 313 - 327, The
patch must defensively handle non-configurable customEggPaths descriptors:
inside __patchFrameworkPaths inspect Object.getOwnPropertyDescriptor(proto,
'customEggPaths'); if no descriptor or descriptor.configurable is true, use
Object.defineProperty as before; if descriptor exists and configurable is false
but descriptor.writable is true, assign proto.customEggPaths = function (...) {
... } (preserving original via the original variable and using __frameworkPaths
and __isOutputRootPath); if descriptor exists and configurable is false and
writable is false, do nothing (skip patch) to avoid TypeError. Ensure you
reference __patchFrameworkPaths, proto, customEggPaths, original,
__frameworkPaths and __isOutputRootPath when making the checks and fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tools/egg-bundler/src/lib/EntryGenerator.ts`:
- Around line 313-327: The patch must defensively handle non-configurable
customEggPaths descriptors: inside __patchFrameworkPaths inspect
Object.getOwnPropertyDescriptor(proto, 'customEggPaths'); if no descriptor or
descriptor.configurable is true, use Object.defineProperty as before; if
descriptor exists and configurable is false but descriptor.writable is true,
assign proto.customEggPaths = function (...) { ... } (preserving original via
the original variable and using __frameworkPaths and __isOutputRootPath); if
descriptor exists and configurable is false and writable is false, do nothing
(skip patch) to avoid TypeError. Ensure you reference __patchFrameworkPaths,
proto, customEggPaths, original, __frameworkPaths and __isOutputRootPath when
making the checks and fallback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c5e92c67-aee9-4a83-82e9-6dc65c44876c

📥 Commits

Reviewing files that changed from the base of the PR and between 652508e and 82e60d2.

⛔ Files ignored due to path filters (1)
  • tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • packages/core/src/loader/egg_loader.ts
  • packages/core/test/loader/mixin/load_plugin.test.ts
  • tools/egg-bundler/src/lib/EntryGenerator.ts
  • tools/egg-bundler/src/lib/ManifestLoader.ts
  • tools/egg-bundler/test/EntryGenerator.test.ts
  • tools/egg-bundler/test/ManifestLoader.test.ts

@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 82e60d2 to 72a3259 Compare May 6, 2026 12:50
Copilot AI review requested due to automatic review settings May 6, 2026 14:57
@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 72a3259 to 1fb5f8d Compare May 6, 2026 14:57
@killagu killagu force-pushed the agent/egg-dev/0263e5d5 branch from 1fb5f8d to 7a1c06f Compare May 6, 2026 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants