Greptile Summary

This PR suppresses false duplicate plugin-id warnings that fired when the same plugin ID was legitimately discovered across different origins (e.g., a bundled plugin also installed globally). It introduces two new per-ID tracking maps — seenRootsById and seenOriginsById — so warnings now fire only for genuine same-origin root collisions. The existing seenIds resolution path is preserved for precedence arbitration; the new structures only control diagnostic emission.

The logic is correct across the tested scenarios: cross-origin same-root promotion updates origin tracking before the seenIds block runs, preventing phantom same-origin warnings after promotion. The two P2 notes above (single-field SeenRootEntry wrapper and lingering empty Sets after promotion) are cosmetic and have no impact on correctness.

Confidence Score: 4/5

Safe to merge; logic is correct and tests cover cross-origin suppression, same-origin warnings, and the promotion edge case.

No P0 or P1 issues found. The implementation correctly suppresses only cross-origin duplicates while preserving same-origin collision warnings and handling origin promotion. Two minor P2 style observations do not affect behavior.

No files require special attention; manifest-registry.ts logic is well-tested and the two new tracking maps behave consistently with the existing seenIds path.

This is a comment left during a code review.
Path: src/plugins/manifest-registry.ts
Line: 89-91

Comment:
**`SeenRootEntry` is a single-field wrapper**

`SeenRootEntry` wraps only `candidate: PluginCandidate` with no other fields. This is fine for extensibility, but the type could be eliminated in favour of `Map<string, PluginCandidate>` to simplify the data model. If there is a plan to attach more per-root metadata in follow-up work the wrapper makes sense; otherwise it is a no-op indirection.

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/manifest-registry.ts
Line: 527-534

Comment:
**Stale empty Sets linger in `originsForId` after promotion**

`promoteSeenOriginRoot` deletes `rootKey` from the `fromOrigin` Set but leaves that (now-empty) Set in `originsForId`. In practice this has no correctness impact because `hadOriginRoot` correctly reads the empty size as `false`. However, it means `originsForId` silently accumulates empty Sets for every origin that ever had a root promoted away, which could be confusing when debugging or extending this code later.

Consider cleaning up empty Sets after deletion:

```ts
params.originsForId.get(params.fromOrigin)?.delete(params.rootKey);
const remaining = params.originsForId.get(params.fromOrigin);
if (remaining?.size === 0) {
  params.originsForId.delete(params.fromOrigin);
}
addSeenOriginRoot(params.originsForId, params.toOrigin, params.rootKey);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(plugins): suppress false duplicate-i..." | Re-trigger Greptile}

Codex review: needs changes before merge.

Keep this PR open. It is maintainer-authored and has the protected maintainer label, so this cleanup workflow must not auto-close it. Current main already suppresses same-physical-root duplicate plugin warnings, but it does not match the PR branch's latest diagnostic policy because main still suppresses explicit installed-global/bundled duplicate warnings while the PR would make those warn again and add origin-bucket tracking. That policy and the active security-review/CI state need maintainer judgment.

Required change before merge:

Keep this PR open for maintainer review. The best path is to decide the duplicate plugin-id diagnostic policy explicitly: if the goal is only same-physical-root false-positive suppression, current main already covers that; if maintainers want the PR's origin-bucket tracking and restored installed-global/bundled warning visibility, land it only after the security-review concern and CI failure are resolved, with #42099/#42192 attribution preserved.

Best possible solution:

Keep this PR open for maintainer review. The best path is to decide the duplicate plugin-id diagnostic policy explicitly: if the goal is only same-physical-root false-positive suppression, current main already covers that; if maintainers want the PR's origin-bucket tracking and restored installed-global/bundled warning visibility, land it only after the security-review concern and CI failure are resolved, with #42099/#42192 attribution preserved.

What I checked:

• Protected maintainer gate: Live PR metadata from the GitHub API shows this PR is open, authored by vincentkoc with author association MEMBER, and labeled maintainer, size: M, and clownfish. The review policy requires keeping maintainer-authored or protected-label items open. (bfa867e28c02)
• Current main duplicate handling: Current main keeps a global seenIds map, suppresses duplicates when roots are identical or resolve to the same realpath, ranks distinct-root duplicates by precedence, suppresses intentional installed-global/bundled duplicates, and emits duplicate plugin id detected warnings for the remaining conflicts. (src/plugins/manifest-registry.ts:650, 5fe81cdf527b)
• Current main tests encode mixed suppression/warning policy: Tests on current main expect distinct-root bundled/global and config/bundled duplicates to warn, explicit installed globals overriding bundled plugins to produce zero duplicate warnings, and same physical roots via symlink or identical rootDir paths to produce zero duplicate warnings. (src/plugins/manifest-registry.test.ts:345, 5fe81cdf527b)
• PR branch changes the warning policy: The PR diff modifies only CHANGELOG.md, src/plugins/manifest-registry.ts, and src/plugins/manifest-registry.test.ts; it adds seenRootsById/seenOriginsById, removes isIntentionalInstalledBundledDuplicate, keeps distinct-root cross-origin warnings, and updates installed-global/bundled tests to expect a warning. (src/plugins/manifest-registry.ts:82, bfa867e28c02)
• Security review pass: The diff does not add dependencies, workflows, package scripts, lockfiles, generated/vendor code, or secret handling. The security-sensitive part is diagnostic visibility for plugin ID shadowing across origins; Aisle had posted an in-progress security review, while Greptile marked the logic correct with only P2 style notes. (bfa867e28c02)
• CI state: GitHub check-runs for the PR head show one failing check, checks-fast-bundled, with several checks still in progress at inspection time, so the PR is not in a ready-to-close/ready-to-land state from automation alone. (bfa867e28c02)

Likely related people:

• steipete: Recent main-branch commits touched the manifest registry and tests, including 713cc74bffef which added the current installed-global/bundled duplicate-warning suppression that this PR revisits. (role: recent maintainer of duplicate-warning policy; confidence: high; commits: 713cc74bffef, b74f35ee6f97, a60f15c6118f; files: src/plugins/manifest-registry.ts, src/plugins/manifest-registry.test.ts)
• shakkernerd: Recent history shows repeated changes in the manifest registry and installed plugin index path, including 6ed642a86ddb which routed duplicate ranking through pending install records. (role: recent duplicate-precedence and install-index maintainer; confidence: high; commits: 6ed642a86ddb, 9e086d6ed833, c19f8a522310; files: src/plugins/manifest-registry.ts, src/plugins/manifest-registry.test.ts, src/plugins/installed-plugin-index-record-reader.ts)
• Tortes: Commit 3d19f018abaf introduced the higher-precedence duplicate manifest selection and tests that remain central to the current warning behavior. (role: introduced current duplicate-precedence behavior; confidence: medium; commits: 3d19f018abaf; files: src/plugins/manifest-registry.ts, src/plugins/manifest-registry.test.ts)
• vincentkoc: Although also the PR author, this person has recent merged history in the same plugin manifest/control-plane area and is the maintainer coordinating this replacement PR. (role: active maintainer and recent plugin manifest contributor; confidence: medium; commits: 1fc5b2b7032c, 8c2bc951a943, 888448facc1b; files: src/plugins/manifest-registry.ts, src/plugins/manifest-registry.test.ts, docs/plugins/architecture.md)
• shadril238: Commit 788ea6e9d1c2 introduced the earlier same-physical-root/symlink duplicate-warning suppression and colocated manifest-registry tests that this PR builds around. (role: earlier false-positive duplicate-warning contributor; confidence: medium; commits: 788ea6e9d1c2; files: src/plugins/manifest-registry.ts, src/plugins/manifest-registry.test.ts)

Remaining risk / open question:

• Auto-closing would violate the maintainer-author/protected-label guard for this workflow.
• The PR changes visibility for plugin ID shadowing diagnostics across origins, which is a security and operations policy decision rather than a purely mechanical cleanup.
• Current main and the PR branch are not equivalent: current main suppresses explicit installed-global/bundled duplicate warnings; the PR branch makes those warn again.
• The PR head had a failing checks-fast-bundled check and several checks still in progress at inspection time.
• Linked issue fix(plugins): false-positive duplicate plugin ID warning on gateway start (msteams) #42099 and related open PR Plugins: dedupe manifest registry rows and suppress benign bundled shadow warnings #58796 mean the broader duplicate-registry policy cluster is still active.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 5fe81cdf527b.