Skip to content

Plugins: dedupe manifest registry rows and suppress benign bundled shadow warnings#58796

Open
PowerMai wants to merge 2 commits intoopenclaw:mainfrom
PowerMai:fix/plugins-manifest-registry-dedupe
Open

Plugins: dedupe manifest registry rows and suppress benign bundled shadow warnings#58796
PowerMai wants to merge 2 commits intoopenclaw:mainfrom
PowerMai:fix/plugins-manifest-registry-dedupe

Conversation

@PowerMai
Copy link
Copy Markdown

@PowerMai PowerMai commented Apr 1, 2026

Summary

  • When two plugin candidates share the same manifest id but resolve to different on-disk roots, keep one plugins[] row by applying resolveDuplicatePrecedenceRank in place and skipping an extra append for the losing candidate (fixes duplicate registry rows).
  • Suppress warn-level duplicate diagnostics for the benign case where bundled shadows a redundant global or workspace copy that is not tracked as an installed plugin (reduces noisy stderr during config validation, e.g. editor ACP).

Test plan

  • pnpm test -- src/plugins/manifest-registry.test.ts
  • pnpm check

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 1, 2026

Greptile Summary

This PR fixes two related issues in the plugin manifest registry: (1) duplicate plugins[] rows caused by the old code falling through to records.push even when an existing entry was found for the same ID, and (2) noisy warn-level diagnostics during config validation (e.g. editor ACP) when a bundled plugin legitimately shadows a redundant auto-discovered global or workspace copy.

Key changes:

  • The duplicate-row bug is fixed by restructuring the if (existing) branch to always continue after resolving precedence in place, so records.push is only reached for genuinely new IDs.
  • A new shouldSuppressBenignBundledShadowDuplicate helper correctly suppresses warn diagnostics when bundled shadows a non-installed global or any workspace copy, while preserving warnings for intentional config-origin overrides and explicitly-installed global plugins.
  • The diagnostic message in the "existing wins" branch was quietly fixed to show existing.candidate.source (the winner's path) rather than the old candidate.source (the loser's path), making the message self-consistent with the "candidate wins" branch.
  • Tests are updated to assert both zero duplicate warnings and registry.plugins.toHaveLength(1), directly verifying the deduplication fix.

Confidence Score: 5/5

Safe to merge — logic is correct, tests directly verify both the deduplication fix and the suppression behaviour, and no P0/P1 issues were found.

The structural fix (always continue after resolving an existing entry) is sound and directly tested. The suppression helper correctly guards all cases: config origins are never suppressed, explicitly-installed globals still emit warnings, and the workspace suppression is valid because matchesInstalledPluginRecord unconditionally returns false for non-global origins, so 'installed workspace plugin' is not a real state in this data model. No regressions were found in the updated test suite.

No files require special attention.

Reviews (1): Last reviewed commit: "Plugins: dedupe manifest registry rows a..." | Re-trigger Greptile

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Apr 1, 2026
View reviewed changes
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: 1ad047b8d3

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

Comment thread src/plugins/manifest-registry.ts
});
seenIds.set(manifest.id, { candidate, recordIndex: existing.recordIndex });
}
continue;
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 1, 2026

Choose a reason for hiding this comment

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

P1 Badge Keep duplicate manifest roots available to loader

Avoid unconditionally continue-ing after duplicate-id resolution here, because loadOpenClawPlugins relies on manifestRegistry.plugins keyed by rootDir (src/plugins/loader.ts builds manifestByRoot and skips any candidate without a manifest record). With the losing duplicate root removed from records, that candidate is dropped entirely instead of being emitted as a disabled "overridden by ..." plugin row, which regresses duplicate-precedence visibility and status behavior for bundled/global or bundled/workspace collisions.

Useful? React with 👍 / 👎.

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes commands Command implementations agents Agent runtime and tooling size: M and removed size: S labels Apr 1, 2026
@vincentkoc vincentkoc mentioned this pull request Apr 26, 2026
Open
@BingqingLyu BingqingLyu mentioned this pull request Apr 27, 2026
Open
@openclaw-barnacle
Copy link
Copy Markdown

openclaw-barnacle Bot commented Apr 30, 2026

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Apr 30, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 1, 2026 •
edited
Loading

Codex review: found issues before merge.

Summary
The PR dedupes manifest-registry rows by plugin id, adds a root-dir manifest lookup map for loader duplicate handling, suppresses selected bundled shadow warnings, and updates related tests and changelog entries.

Reproducibility: yes. The reviewed regressions have source-level reproduction paths in current tests: loader provenance coverage expects allowlisted plugins to stay quiet, and manifest-registry code shows installed-plugin ledger state is part of duplicate precedence.

Next step before merge
Maintainer review is needed because the PR overlaps protected maintainer work in #72409 and changes duplicate-warning product policy beyond a mechanical repair.

Security
Cleared: The diff changes plugin registry/loader diagnostics and tests but does not add dependencies, workflows, lifecycle hooks, credential handling, package resolution, or downloaded code execution paths.

Review findings

  • [P2] Keep allowlisted plugins out of provenance warnings — src/plugins/loader.ts:773-778
  • [P2] Preserve installed-index duplicate checks in shadow suppression — src/plugins/manifest-registry.ts:368-373
  • [P3] Add contributor attribution to changelog entries — CHANGELOG.md:22-24
Review details

Best possible solution:

Keep the PR open for maintainer review, then either rebase it as a narrow recordsByRootDir/loader-visibility change or align the warning suppression policy explicitly with #72409 while preserving allowlist and installed-ledger semantics.

Do we have a high-confidence way to reproduce the issue?

Yes. The reviewed regressions have source-level reproduction paths in current tests: loader provenance coverage expects allowlisted plugins to stay quiet, and manifest-registry code shows installed-plugin ledger state is part of duplicate precedence.

Is this the best way to solve the issue?

No. The recordsByRootDir seam is plausible, but the PR also removes shipped allowlist warning suppression and does not preserve current installed-ledger duplicate semantics; a narrower rebased patch is safer.

Full review comments:

  • [P2] Keep allowlisted plugins out of provenance warnings — src/plugins/loader.ts:773-778
    This removes the allowlist exemption from untracked-provenance warnings, so a non-bundled plugin explicitly trusted via plugins.allow will emit loaded without install/load-path provenance. Current main has regression coverage expecting that case to stay quiet.
    Confidence: 0.9
  • [P2] Preserve installed-index duplicate checks in shadow suppression — src/plugins/manifest-registry.ts:368-373
    The new benign-shadow suppression helper classifies globals without threading the installed-plugin ledger through matchesInstalledPluginRecord. A managed installed global that is only present in the install index can be treated as redundant and lose the duplicate warning.
    Confidence: 0.82
  • [P3] Add contributor attribution to changelog entries — CHANGELOG.md:22-24
    The added user-facing changelog bullets omit contributor attribution. Repo policy asks added user-facing entries to credit the appropriate non-bot contributor, here likely Thanks @PowerMai.
    Confidence: 0.78

Overall correctness: patch is incorrect
Overall confidence: 0.86

What I checked:

  • Current main dedupes manifest registry rows but preserves duplicate warning policy: The manifest-registry test on current main expects one row for bundled/global distinct duplicates and still expects a duplicate warning naming the global plugin overridden by bundled. (src/plugins/manifest-registry.test.ts:345, 9bedcff904dd)
  • Current main keeps allowlisted plugins out of untracked provenance warnings: warnAboutUntrackedLoadedPlugins accepts an allowlist and skips diagnostics when the loaded non-bundled plugin id is explicitly allowlisted. (src/plugins/loader-provenance.ts:230, 9bedcff904dd)
  • Regression coverage protects the allowlist provenance behavior: The loader test case named does not warn when loaded non-bundled plugin is in plugins.allow expects no warning for an allowlisted global plugin. (src/plugins/loader.test.ts:6161, 9bedcff904dd)
  • PR removes the allowlist provenance guard: The PR diff removes the allowlist parameter and allowSet skip from warnAboutUntrackedLoadedPlugins, and updates the corresponding test to expect a warning. (src/plugins/loader.ts:773, df9b11f8582d)
  • Current duplicate precedence uses installed-plugin ledger records: Current main threads installRecords through matchesInstalledPluginRecord and resolveDuplicatePrecedenceRank, so suppression logic must preserve managed installed-global state. (src/plugins/manifest-registry.ts:473, 9bedcff904dd)
  • Related maintainer follow-up remains open: The related PR fix(plugins): suppress false duplicate-id warnings across origins #72409 is open, authored by a member, labeled maintainer, and explicitly carries the narrower duplicate-warning follow-up rather than declaring this PR superseded. (bfa867e28c02)

Likely related people:

  • steipete: Recent commits on manifest-registry and loader/provenance paths include duplicate override diagnostics, installed override warnings, and loader helper splits. (role: recent maintainer; confidence: high; commits: c35ed548bf0c, 713cc74bffef, 4aedffd37aed; files: src/plugins/manifest-registry.ts, src/plugins/loader-provenance.ts, src/plugins/loader.ts)
  • shakkernerd: Recent main history shows plugin manifest/index and duplicate-rank work adjacent to the registry behavior this PR changes. (role: adjacent maintainer; confidence: medium; commits: 6ed642a86ddb, 9e086d6ed833, 53c2dbe9e92c; files: src/plugins/manifest-registry.ts, src/plugins/loader.ts)
  • vincentkoc: Owns the open maintainer-labeled duplicate-warning follow-up fix(plugins): suppress false duplicate-id warnings across origins #72409 and has recent manifest metadata work in the same plugin control-plane area. (role: related follow-up owner; confidence: medium; commits: bfa867e28c02, 8c2bc951a943, 1fc5b2b7032c; files: src/plugins/manifest-registry.ts)

Remaining risk / open question:

Codex review notes: model gpt-5.5, reasoning high; reviewed against 9bedcff904dd.

@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

agents Agent runtime and tooling cli CLI command changes commands Command implementations size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@PowerMai