Skip to content

fix(plugins): prefer higher-precedence manifests for duplicate plugin ids#41626

Merged
steipete merged 1 commit intoopenclaw:mainfrom
Tortes:bugfix/plugin-manifest-duplicate-precedence
Apr 20, 2026
Merged

fix(plugins): prefer higher-precedence manifests for duplicate plugin ids#41626
steipete merged 1 commit intoopenclaw:mainfrom
Tortes:bugfix/plugin-manifest-duplicate-precedence

Conversation

@Tortes
Copy link
Copy Markdown
Contributor

@Tortes Tortes commented Mar 10, 2026

Summary

  • suppress duplicate warnings when the same physical plugin is discovered through multiple origins
  • keep the highest-precedence manifest record for a plugin id when duplicate candidates are found
  • continue warning when different physical plugin directories reuse the same manifest id
  • ensure duplicate warnings point at the candidate that is actually overridden
  • add regression coverage for same-path and distinct-path duplicate scenarios

Context

PLUGIN_ORIGIN_RANK already defines precedence across discovery origins, but duplicate manifest handling did not fully respect that precedence.

Before this change:

  • the same plugin discovered more than once through different sources could still emit a duplicate warning
  • duplicate-id handling could replace the retained record, but the warning metadata could still describe the wrong candidate as the overridden one

After this change:

  • if two candidates resolve to the same physical plugin directory, the registry keeps the higher-precedence origin and skips the duplicate warning
  • if two different physical directories share the same plugin id, the registry still emits a warning while keeping the higher-precedence candidate in the registry
  • the diagnostic source now matches the candidate that actually lost precedence

Related issue

Implementation details

  • compare duplicate candidates by direct root path equality first, then by realpath when needed
  • treat same-physical-path rediscovery as deduplication instead of a conflicting duplicate
  • preserve the winning record index while updating diagnostics to describe the overridden candidate accurately
  • extend manifest registry tests to cover same-physical-path deduplication and distinct-path precedence conflicts

Testing

  • npx vitest run src/plugins/manifest-registry.test.ts

Notes

This stays intentionally narrow to manifest-registry duplicate resolution so it does not change plugin discovery order or broader registry loading behavior.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 10, 2026

Greptile Summary

This PR fixes a pre-existing bug where distinct-path plugins sharing the same id would both be inserted into the manifest registry; it now retains only the highest-precedence record and always emits exactly one warning per collision. The refactor also eagerly builds the PluginManifestRecord before the deduplication check, eliminating repeated calls to buildRecord across branches.

  • Logic fix (distinct-path duplicates): The old code fell through to records.push(...) for every duplicate regardless of precedence, producing multiple records with the same plugin id. The new code replaces the existing record in-place when the candidate has higher precedence, then continues, so exactly one record survives per id.
  • Same-path branch untouched: The silent-dedup path for symlinked / identical rootDir candidates is preserved as-is.
  • Diagnostic message accuracy: When the higher-precedence candidate is processed second and replaces the existing record, the emitted warning sets source: candidate.source — pointing at the winning plugin — while the message text says "later plugin may be overridden". This is misleading: the plugin referenced by source is the one that won, not the one that was dropped. See inline comment for a suggested correction.
  • Test coverage: A new regression test for the distinct-path / precedence case is added and passes. The reverse ordering (higher-precedence candidate arrives first) is not explicitly tested for distinct-path duplicates, though the implementation handles it correctly.

Confidence Score: 4/5

  • PR is safe to merge; the core deduplication logic is correct and well-tested, with one minor diagnostic message inaccuracy worth addressing.
  • The fix correctly replaces the buggy "add both records" behaviour with in-place precedence-based replacement and a single warning. The continue restructuring is clean and the same-path branch is unaffected. The only concern is that the source field and message text of the emitted diagnostic point to the winning plugin rather than the losing one when the later candidate has higher precedence, which could confuse users reading the warning. This is a clarity issue, not a correctness bug in the registry state itself.
  • src/plugins/manifest-registry.ts — review the diagnostic source and message text in the distinct-path duplicate branch (lines 238–247).

Last reviewed commit: 4707aa7

Comment thread src/plugins/manifest-registry.ts Outdated
@Tortes Tortes force-pushed the bugfix/plugin-manifest-duplicate-precedence branch 2 times, most recently from 61907ab to 9daecf9 Compare March 11, 2026 03:29
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9daecf9ccb

ℹ️ 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".

Comment thread src/plugins/manifest-registry.ts Outdated
@Tortes Tortes force-pushed the bugfix/plugin-manifest-duplicate-precedence branch from 9daecf9 to 7222355 Compare March 11, 2026 16:12
@openclaw-barnacle openclaw-barnacle Bot added channel: googlechat Channel integration: googlechat extensions: memory-core Extension: memory-core and removed channel: googlechat Channel integration: googlechat extensions: memory-core Extension: memory-core labels Mar 11, 2026
… ids

- keep higher-precedence manifests when duplicate plugin ids are discovered
- report overridden duplicate manifest source
- align loader tests with current duplicate-precedence behavior
@steipete steipete force-pushed the bugfix/plugin-manifest-duplicate-precedence branch from 0a6bcdb to 1fe83b8 Compare April 20, 2026 19:48
@steipete steipete merged commit 3d19f01 into openclaw:main Apr 20, 2026
10 checks passed
@steipete
Copy link
Copy Markdown
Contributor

steipete commented Apr 20, 2026

Landed in 3d19f01 after rebasing and resolving the duplicate-manifest behavior against current main.

Validation:

  • pnpm test src/plugins/manifest-registry.test.ts src/plugins/loader.test.ts passed.
  • pnpm lint:core passed.
  • pnpm check:changed stopped in core-test typecheck on pre-existing src/pairing/pairing-store.test.ts crypto.randomInt mock typing errors (TS2344, TS2345). That file is unchanged from the landing base: git diff --quiet temp/landpr-41626...HEAD -- src/pairing/pairing-store.test.ts returned 0.

Rebased PR head before merge: Tortes@1fe83b8

Thanks @Tortes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants