feat: add support for disabling default plugins and configuring - #3221
Conversation
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: manucorporat <127379+manucorporat@users.noreply.github.com>
Merge conflicts resolved in 7e4547e — the |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
PR #3221 adds deployment-level controls for refusing framework default plugins and narrowing mounted integration platforms. The current head is a merge commit, but the effective PR diff remains the same 14-file framework/configuration change. The schema and direct adapter tests are useful, and the opt-in route gating preserves existing behavior when no platform allow-list is configured.
This is a standard-risk change because it changes shared runtime initialization and deployment configuration semantics.
Key Findings
- 🔴 HIGH — Generated edge workers call the new plugin-disable helper, which routes through the Node-oriented app-config store and bare
process.env; this can prevent edge workers from mounting defaults. - 🟡 MEDIUM — Unknown integration platform names are thrown by the allow-list helper but swallowed by best-effort default-plugin bootstrap, leaving integrations silently absent.
- 🟡 MEDIUM — Invalid disabled-plugin names are likewise swallowed by bootstrap, allowing a partially initialized Node app to appear healthy.
All three findings were independently reconfirmed by multiple fresh reviewers against the latest head. No frontend files or user-facing UI flows changed.
🧪 Browser testing: Skipped — PR only modifies backend/config/docs, no UI impact
| // check that `bootstrapDefaultPlugins` runs has to happen here too — | ||
| // otherwise the same config withholds a plugin on Node hosts and mounts it | ||
| // on the edge. | ||
| pluginCalls.push(` if (typeof ${varName} === "function" && !isDefaultPluginDisabled(${JSON.stringify(stem)})) { |
There was a problem hiding this comment.
🔴 Keep generated edge plugin gating edge-safe
The generated edge entry calls isDefaultPluginDisabled() for every default plugin, but that helper delegates to getAppConfig(), which unconditionally reads bare process.env. Edge runtimes without Node’s process can therefore throw ReferenceError: process is not defined during plugin mounting, even with no disabled plugins configured. Use the worker’s runtime environment mechanism or inject the resolved disabled-slot list at build time instead of calling the Node-oriented config store.
Additional Info
Confirmed by 3 parallel reviewers; packages/core/src/app-config/store.ts:121 reads process.env directly.
There was a problem hiding this comment.
Not reproducible — the same generated worker entry already requires process.env before this line. It statically mounts defaultAgentChatPlugin, whose mount body reads bare process.env.NODE_ENV (agent-chat-plugin.ts:654) and calls getAppConfig() (:659), and defaultCoreRoutesPlugin, which does the same. isDefaultPluginDisabled() adds no requirement the edge bundle did not already have, so gating it here would not make edge mounting process-free. Leaving as is.
An unknown slot in `plugins.disabled` threw inside the best-effort default-plugin auto-mount catch, which warned and resolved — leaving the app with every default route missing and the deployment reporting success. Read the list in `getH3App()` before that catch is installed. An `integrations.platforms` name no adapter provides now throws a typed `AppConfigurationError`, which the per-plugin auto-mount catch rethrows instead of skipping the slot.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ugins # Conflicts: # docs/environment-variables.md # packages/core/src/app-config/schema.ts
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Review Summary
The latest head was reviewed after the merge update. The previous snapshotting and bootstrap-reporting concerns were verified fixed and resolved. The existing edge process.env concern remains open and was deliberately not reposted.
Two fresh reviewers independently identified one new medium-severity regression in configuration precedence. The synchronous validation call in getH3App() resolves and parses the environment layer before a later server plugin can register a higher-priority defineAppConfig() override. Therefore an invalid AGENT_NATIVE_DISABLED_PLUGINS environment value can fail startup even when the app layer supplies a valid replacement, contradicting the documented app-over-environment precedence. Focused configuration, integration, and framework-handler tests pass, but the existing precedence test covers only a parseable environment value.
This remains standard risk because it affects shared runtime initialization and deployment configuration. The PR remains backend/config/docs-only.
🧪 Browser testing: Skipped — PR only modifies backend/config/docs, no UI impact
| // mount set is read again inside bootstrap: auto-mount can start before a | ||
| // server plugin has called `defineAppConfig()`, and this early read only | ||
| // sees the environment layer. | ||
| getDisabledDefaultPlugins(); |
There was a problem hiding this comment.
🟡 Honor app-level configuration overrides before validating environment values
getDisabledDefaultPlugins() resolves and validates the environment layer synchronously before a later server plugin can call defineAppConfig(). If AGENT_NATIVE_DISABLED_PLUGINS contains an invalid value but the app layer supplies a valid plugins.disabled override, getH3App() throws before the documented app-over-environment precedence can apply. Defer validation until the final merged configuration is available, or otherwise avoid parsing a lower-priority environment layer as a complete schema before app overrides are installed.
Additional Info
Confirmed independently by 2 fresh reviewers; focused tests cover precedence only when the environment value is parseable.
…ration platforms