plugins: suppress false duplicate-id warning across origins#42192
plugins: suppress false duplicate-id warning across origins#42192macarronesc wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
Implemented and validated. I investigated the duplicate warning path in the plugin manifest registry and identified that duplicate diagnostics were too broad for cross-origin discovery scenarios. The fix now only warns when distinct plugin roots collide within the same origin bucket, while allowing expected cross-origin precedence behavior without noise. Included test coverage updates to verify:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fffec2f08
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| rootsForId.set(rootKey, { | ||
| candidate, | ||
| recordIndex: existingSameRoot.recordIndex, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Record promoted roots in the new origin bucket
When existingSameRoot is hit, the code updates rootsForId and immediately continues, but it never adds that root to originsForId for the current (possibly higher-precedence) origin. This causes missed duplicate diagnostics if ordering is bundled A -> global A (same root, promotion) -> global B (different root): hadOriginRoot is false for global B, so a real same-origin duplicate is silently suppressed. The previous logic warned in this case because the promoted candidate became the tracked seenIds entry.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR refines the plugin manifest deduplication logic in Key observations:
Confidence Score: 3/5
Last reviewed commit: 8fffec2 |
| const existingSameRoot = rootsForId.get(rootKey); | ||
| if (existingSameRoot) { | ||
| // Same physical plugin discovered through multiple origins/sources. | ||
| // Keep only one record and pick the highest-precedence origin. | ||
| if ( | ||
| PLUGIN_ORIGIN_RANK[candidate.origin] < PLUGIN_ORIGIN_RANK[existingSameRoot.candidate.origin] | ||
| ) { | ||
| records[existingSameRoot.recordIndex] = buildRecord({ | ||
| manifest, | ||
| candidate, | ||
| manifestPath: manifestRes.manifestPath, | ||
| schemaCacheKey, | ||
| configSchema, | ||
| }); | ||
| rootsForId.set(rootKey, { | ||
| candidate, | ||
| recordIndex: existingSameRoot.recordIndex, | ||
| }); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
seenOriginsById not updated on same-root origin promotion
When a same-root candidate is processed with a higher-precedence origin (e.g. a root already seen as bundled now appearing as config), rootsForId is correctly updated, but seenOriginsById is not adjusted. The old origin's set keeps the root, and the new origin's set never gains it.
This leads to a missed warning in the following scenario:
- Plugin X,
root1, origin=bundled→originsForId["bundled"] = {root1},rootsForId[root1] = {bundled} - Plugin X,
root1, origin=config(same physical path) →existingSameRootfires; record updated to config;rootsForId[root1] = {config}— butoriginsForId["bundled"]still holdsroot1andoriginsForId["config"]is still empty. - Plugin X,
root2, origin=config(different physical path) →existingSameRootmisses;rootsForOrigin = originsForId.get("config")= empty Set;hadOriginRoot = false→ no warning emitted, even though two distinct config-origin roots now exist for the same ID.
This violates the stated invariant: "Warn only when distinct roots collide within the same origin bucket."
Suggested fix: when promoting a same-root entry to a higher-precedence origin, migrate the root from the old origin's bucket to the new one:
if (PLUGIN_ORIGIN_RANK[candidate.origin] < PLUGIN_ORIGIN_RANK[existingSameRoot.candidate.origin]) {
// Migrate rootKey from the old origin bucket to the new one.
originsForId.get(existingSameRoot.candidate.origin)?.delete(rootKey);
const newOriginRoots = originsForId.get(candidate.origin) ?? new Set<string>();
if (!originsForId.has(candidate.origin)) {
originsForId.set(candidate.origin, newOriginRoots);
}
newOriginRoots.add(rootKey);
records[existingSameRoot.recordIndex] = buildRecord({
manifest,
candidate,
manifestPath: manifestRes.manifestPath,
schemaCacheKey,
configSchema,
});
rootsForId.set(rootKey, {
candidate,
recordIndex: existingSameRoot.recordIndex,
});
}
continue;Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/manifest-registry.ts
Line: 214-234
Comment:
`seenOriginsById` not updated on same-root origin promotion
When a same-root candidate is processed with a higher-precedence origin (e.g. a root already seen as `bundled` now appearing as `config`), `rootsForId` is correctly updated, but `seenOriginsById` is not adjusted. The old origin's set keeps the root, and the new origin's set never gains it.
This leads to a missed warning in the following scenario:
1. Plugin X, `root1`, origin=`bundled` → `originsForId["bundled"] = {root1}`, `rootsForId[root1] = {bundled}`
2. Plugin X, `root1`, origin=`config` (same physical path) → `existingSameRoot` fires; record updated to config; `rootsForId[root1] = {config}` — **but** `originsForId["bundled"]` still holds `root1` and `originsForId["config"]` is still empty.
3. Plugin X, `root2`, origin=`config` (different physical path) → `existingSameRoot` misses; `rootsForOrigin = originsForId.get("config")` = empty Set; `hadOriginRoot = false` → **no warning emitted**, even though two distinct config-origin roots now exist for the same ID.
This violates the stated invariant: *"Warn only when distinct roots collide within the same origin bucket."*
Suggested fix: when promoting a same-root entry to a higher-precedence origin, migrate the root from the old origin's bucket to the new one:
```typescript
if (PLUGIN_ORIGIN_RANK[candidate.origin] < PLUGIN_ORIGIN_RANK[existingSameRoot.candidate.origin]) {
// Migrate rootKey from the old origin bucket to the new one.
originsForId.get(existingSameRoot.candidate.origin)?.delete(rootKey);
const newOriginRoots = originsForId.get(candidate.origin) ?? new Set<string>();
if (!originsForId.has(candidate.origin)) {
originsForId.set(candidate.origin, newOriginRoots);
}
newOriginRoots.add(rootKey);
records[existingSameRoot.recordIndex] = buildRecord({
manifest,
candidate,
manifestPath: manifestRes.manifestPath,
schemaCacheKey,
configSchema,
});
rootsForId.set(rootKey, {
candidate,
recordIndex: existingSameRoot.recordIndex,
});
}
continue;
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
This PR fixes issue #42099 by refining plugin manifest deduplication logic to avoid false-positive duplicate ID warnings during gateway startup.
Root cause
The registry warned on duplicate plugin IDs too broadly, even when the same plugin ID appeared across different discovery origins as part of normal precedence/shadowing behavior.
What changed
Tests
Updated and added tests in manifest-registry tests to cover:
Safety and impact
Fixes #42099