fix(contracts): use static imports for provider registry to fix vmForks jiti failures#49661
fix(contracts): use static imports for provider registry to fix vmForks jiti failures#49661kimptoc wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes 8 pre-existing test failures on Linux CI by resolving a Key observations:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugins/contracts/registry.ts
Line: 109-113
Comment:
**`captureRegistrations` duplicates existing `capturePluginRegistration` utility**
`captured-registration.ts` already exports a `capturePluginRegistration` helper that does exactly this — call `register` on a freshly created `CapturedPluginRegistration`:
```ts
export function capturePluginRegistration(params: {
register(api: OpenClawPluginApi): void;
}): CapturedPluginRegistration { ... }
```
The new `captureRegistrations` function is functionally identical, just with a slightly different call signature. Consider reusing the existing utility to avoid divergence:
```suggestion
function buildStaticProviderEntries(): ProviderContractEntry[] {
return staticBundledProviderPlugins.flatMap((plugin) => {
const captured = capturePluginRegistration(plugin);
return captured.providers.map((provider) => ({ pluginId: plugin.id, provider }));
});
}
```
This would also let the standalone `captureRegistrations` function be removed entirely.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugins/contracts/registry.ts
Line: 116-119
Comment:
**Static provider build is outside the error boundary**
`buildStaticProviderEntries()` is called before the `try` block, so any exception thrown inside a plugin's `register()` call (e.g. an unexpected `undefined` during plugin boot) will propagate uncaught all the way up to the module-level initializer at line 146. That would crash the entire module, whereas the original code gracefully caught all jiti failures and stored them in `providerContractLoadError`.
Consider wrapping the static build in its own guard so partial failures are recoverable:
```suggestion
function loadBundledProviderRegistry(): ProviderContractEntry[] {
let staticEntries: ProviderContractEntry[] = [];
try {
staticEntries = buildStaticProviderEntries();
} catch (error) {
providerContractLoadError = error instanceof Error ? error : new Error(String(error));
return staticEntries;
}
const staticPluginIdSet = new Set(staticBundledProviderPlugins.map((p) => p.id));
```
This keeps the graceful-degradation contract that already exists for the jiti path.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Merge branch 'main' ..." |
![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: c55fca72ea
ℹ️ 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".
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
from
c55fca7 to
4db8211
Compare
CI verification: fix confirmed workingCompared
The static import fix in All other failures ( The |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
from
5a7f231 to
edebd67
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edebd6754e
ℹ️ 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".
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
2 times, most recently
from
46eb7fb to
5b1c0c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b1c0c8e2a
ℹ️ 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".
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
from
5b1c0c8 to
37a71bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a71bc03f
ℹ️ 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".
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
from
37a71bc to
312569f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 312569f609
ℹ️ 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".
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
2 times, most recently
from
711c14e to
cb7108c
Compare
…istry loadBundledProviderRegistry() now captures providers from all statically-imported bundled plugins first (reliable in all vitest pool modes), then supplements with jiti only for any plugins not covered. Key properties: - Per-plugin isolation: one register() failure skips only that plugin; others load normally - jiti fallback: failed static captures fall back to jiti (succeededPluginIds tracks only plugins that actually succeeded, so failures aren't also excluded from jiti) - Graceful degradation: jiti errors are caught and stored in providerContractLoadError Fixes vmForks incompatibility (jiti's CJS transform cannot set 'require' on the VM context's read-only getter) and vi.mock() divergence (jiti creates separate CJS module instances that bypass vitest's ES module mock intercept). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kimptoc
force-pushed
the
fix/runtime-contract-vmforks
branch
from
cb7108c to
2464aa7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2464aa7fab
ℹ️ 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".
| const staticBundledProviderPlugins: RegistrablePlugin[] = [ | ||
| amazonBedrockPlugin, | ||
| anthropicPlugin, | ||
| byteplusPlugin, | ||
| chutesPlugin, |
There was a problem hiding this comment.
loadBundledProviderRegistry() now assumes staticBundledProviderPlugins mirrors bundledProviderPlugins, but the new array already diverges in this file: bundledProviderPlugins still includes falPlugin and does not include perplexityPlugin (src/plugins/contracts/registry.ts:431-466). That matters whenever this fallback path runs and resolvePluginProviders() throws, because the catch now returns only staticEntries; in that case fal disappears even though its module is already statically imported, while succeededPluginIds can suppress the wrong plugin IDs. Deriving this list from bundledProviderPlugins (or asserting the sets match) would keep the fallback from silently losing provider coverage.
Useful? React with 👍 / 👎.
|
as per contrib guidelines leaving for core team to address ci failures |
1 participant
Summary
Upstream (#4ac9024de) added static imports for all bundled plugins but
loadBundledProviderRegistry()still usesresolvePluginProviders()(jiti-only). This PR updatesloadBundledProviderRegistry()to use those static imports viacapturePluginRegistration()first, with jiti as a supplement-only fallback.Root causes fixed:
requireon the vitest vmForks VM context, which marks it read-only getter →TypeError: Cannot set property require. StaticcapturePluginRegistration()calls bypass jiti entirely.vi.mock()patches.Key properties of the fix:
register()failure skips only that plugin, others load normallysucceededPluginIdstracks only plugins that actually succeeded static capture, so failures aren't also excluded from the jiti supplementproviderContractLoadErrorCI evidence
src/plugins/contracts/runtime.contract.test.tsfailures in Linux test shard 1:runtime.contract.test.tsThe 1 remaining failure is an OOM timeout (pre-existing on main, unrelated to vmForks).
All other failures in the shard (
runtime-web-tools,fal,command-secret-gateway, OOM) are identical on main — pre-existing and unrelated to this PR.Test plan
pnpm test -- src/plugins/contracts/runtime.contract.test.ts --pool=vmForkspasses all tests locallyruntime.contract.test.tsHuman Verification (required)