fix(plugins): let a plugin's search provider reach global search

[nikhil-jangir-iitk]

Summary

A plugin's SearchProvider never reached global search. PluginContext.registerSearchProvider is the documented way for a plugin to contribute to "the application's global search (Spotlight)" (its KDoc in boss-plugin-api, and PLUGIN_DEVELOPMENT.md in boss-plugins), and the bundled bookmarks plugin calls it in register(). The call was dropped before reaching the host.

Reproduced in the released app (BOSS v9.5.17, Windows 11): with the bookmarks plugin loaded, a tab bookmarked into Favorites ("Page1 - Unified Sales Platform | Email, WhatsApp, SMS & More") shows in the Favorites panel, but after closing the tab, global search for Unified Sales lists only a Recent Pages result and no Bookmarks section.

Root cause

• DynamicPluginManager gives register() a TrackingPluginContext around a SandboxedPluginContext around the window's DefaultPlugin (all five register(trackingContext) call sites, including the out-of-process "UI in-process" branch).
• Only DefaultPlugin implements registerSearchProvider (into SearchRegistryImpl). Neither wrapper overrode it or unregisterSearchProvider, and both are plain : PluginContext classes, so the call reached the interface default, whose body is empty.
• GlobalSearchService gets bookmark results only from SearchRegistryImpl.providers, so the bookmarks plugin's BookmarkSearchProvider contributed nothing.

SandboxedPluginContext already states the rule above its UI extension overrides ("Without these overrides the PluginContext interface defaults (no-ops) would swallow plugin registrations silently"); search providers were not in that block. AGENTS.md records the same class of gap for mcpToolRegistry.

Fix

• TrackingPluginContext: forward both calls, and record the teardown with recordUiExtensionRegistration, exactly like panel menus, settings pages, deep-link handlers, shortcuts and status bar items, so unregisterAll removes the provider when the plugin is disabled or unloaded.
• SandboxedPluginContext: forward both calls, like the registries above them.
• Both wrappers also forward organisationMembersProvider, which plugin API 1.0.93 (on dev since fix(browser): integrate exact-frame editor routing with API 1.0.93 #693) added to PluginContext. DefaultPlugin does not implement it, so every plugin still gets null; the forwarding keeps a future host implementation from being swallowed the same way. Without it, the guard below fails on current dev for both wrappers.

No change to DefaultPlugin, SearchRegistryImpl or GlobalSearchService.

Guard

PluginContextWrapperForwardingTest checks that both wrappers declare every PluginContext member, so the next member added to the api cannot fall through to a silent default in one wrapper.

• It uses Kotlin reflection (as HomeActionRoutingTest does): the api is compiled with the default -jvm-default=enable, which adds a compatibility bridge for each default method to implementing classes, so Class.declaredMethods lists members a wrapper never overrode.
• It expects every member of PluginContext including inherited ones (less Any's), so splitting the api interface into a base and extensions cannot quietly narrow it. A third test pins that with a two-level interface.
• What it cannot see: members are matched by name, so an override that forwards to the wrong delegate, or drops the call, still passes; and a third wrapper has to be added to it. Both are in its KDoc and in AGENTS.md.
• Measured on the rebased branch with the organisationMembersProvider forwarding removed, it reports exactly [organisationMembersProvider] for each wrapper, and the original declared-members version of the check reports the same.

Verified

On dev d22d2bfeb (plugin API 1.0.93, #694 merged):

• Red first (original submission, on dev 5a2e8181): all 6 new tests failed.

• PluginSearchProviderForwardingTest (4) builds the production chain, TrackingPluginContext(SandboxedPluginContext(InProcessPluginSandbox, host)), with DefaultPlugin replaced by a recording host: register and unregister reach the host, unregisterAll removes the provider, and one plugin's teardown leaves another plugin's provider registered.

• With fix(plugins): keep a plugin's app-wide registrations when another window closes #694 merged, the multi-window case is now covered end to end. PluginRegistrationsAcrossWindowsTest left search out of its teardown-chain test because the wrappers did not forward it ("fix(plugins): let a plugin's search provider reach global search #670"); it now covers all seven kinds, so a closed window's plugin teardown is shown to keep the other window's search provider through the real wrappers and two real DefaultPlugins.

• :composeApp:desktopTest for ai.rever.boss.components.plugin.* and ai.rever.boss.search.*: 593 tests, 1 failure, DesktopFileScannerTest "follows a symlink" with FileSystemException: A required privilege is not held by the client (Windows without Developer Mode cannot create symlinks; unrelated, fails the same way without this change). :plugin-platform:plugin-sandbox:desktopTest: 150, 0 failures.

• ktlintCheck and detekt clean for composeApp and plugin-sandbox.

• Collision check: a 3-way merge of the six changed files against every open PR that changes them (57 PR/file pairs) adds 0 conflicts.

• Mutation-checked, each caught:

  Mutation	Caught by
  Tracking does not forward register	1
  Tracking does not record the unload teardown	2 (original run); 3 on the rebased branch, incl. the two-window teardown test
  Tracking teardown unregisters a different id	2
  Tracking does not forward unregister	1
  Tracking register override removed	4 (incl. guard)
  Sandboxed does not forward register	1
  Sandboxed does not forward unregister	3
  Sandboxed register override removed	2 (incl. guard)
  an unrelated Sandboxed override removed (mcpToolRegistry)	1 (guard)
  neither wrapper forwards organisationMembersProvider	2 (guard, both wrappers)
  the guard expects only members PluginContext declares itself	1 (inherited-member test)

Not covered

• The fix is not exercised in a running app. The bug was reproduced in the v9.5.17 release (above), but I could not run a build of this branch with sign-in configured, so the fixed behaviour is shown by the tests. A manual check: with the bookmarks plugin loaded, search for a bookmark's title.
• A provider that hangs blocks every category of global search. GlobalSearchService isolates a provider that throws, but has no per-provider timeout and joins all sources with awaitAll. That was unreachable while the registry was always empty; this change makes plugin code run there for the first time. It is a GlobalSearchService question, not a wrapper one.
• The pre-existing println calls in TrackingPluginContext.unregisterAll are left alone.

Update after review (2026-09-15)

• Rebased onto dev d22d2bfeb; the original commit applied with no conflicts.
• Review findings addressed: the guard checks inherited members and states what it cannot see; the two comments that list the recorded kinds, and the unregisterAll KDoc, name search providers; AGENTS.md records this as the second three-wrapper gap. The earlier multi-window note is resolved by fix(plugins): keep a plugin's app-wide registrations when another window closes #694, and its test now includes search.
• New from the rebase: organisationMembersProvider forwarding (above).
• Not changed: a sandbox-local copy of the guard in plugin-sandbox (PR CI runs build, which already runs this test).

[kshivang]

@claude Please review the current search-provider forwarding change, including registration teardown, plugin isolation and the wrapper coverage tests. Target is dev.

[kshivang]

@ninja-lifts Thank you for tracing the missing SearchProvider registration through both context wrappers and adding production-chain forwarding/teardown tests plus the Kotlin-reflection guard. I independently confirmed the actual consumer path: bookmarks registers its provider, DefaultPlugin forwards to SearchRegistryImpl, and GlobalSearchService reads that registry. This belongs in the host wrappers, with bookmark search behavior staying in the plugin.

The four-file change integrates cleanly with dev 6004233 and passes diff whitespace checks. No new source correctness defect is established by this review. The process-wide same-provider-id teardown limitation is real and accurately disclosed: the production registry replaces/removes by id, and the two-plugin test uses distinct ids. Do not interpret that test as evidence of same-id multi-window ownership safety. A future ownership fix should test register A, replace with B under the same id, then tear down A without removing B; it is broader registry work and not a duplicate of #593 or #644.

The current merge blocker is validation infrastructure: exact-head Build & Test run 34872616337 was action_required, with no check results, rather than failing source tests. I approved the fork workflow through the GitHub API. @claude was requested once in the preceding comment; no completed review exists yet. I am marking draft for this bounded validation handoff, not attributing the pending workflow/review to an author defect.

Next steps: let that run complete, inspect any concrete failures, and verify PluginSearchProviderForwardingTest, PluginContextWrapperForwardingTest, the search suite and plugin-sandbox tests against fresh dev/API 1.0.93, with ktlint/detekt. Distinguish a Windows symlink-privilege setup failure from a regression using the baseline as you already did. Review useful Claude findings independently, then mark ready once required CI is green. A local Gradle rerun in this sweep did not acquire the shared build lock; your reported test evidence is not represented as my execution. No app was launched and no fixed-build manual bookmark search or multi-window UI check was performed. Merge only to dev under the shared lock with the reviewed head match.

[claude]

Claude finished @kshivang's task in 5m 26s —— View job

---

Review: search-provider forwarding

• Read the PR diff against dev
• Check the forwarding in both wrappers against their sibling registrations
• Check the teardown path (recordUiExtensionRegistration / unregisterAll) and registry ownership
• Check plugin isolation and any other PluginContext implementations the fix misses
• Check the wrapper coverage test's strength
• Post review

Verdict: the fix is right, minimal and follows the established convention exactly. No blocking issues. Findings below are one documentation gap, two test-hardening suggestions and two nits — nothing that should hold the merge.

---

What the change gets right

The forwarding is byte-for-byte the sibling pattern. TrackingPluginContext.kt:509-517 sits next to the five other UI-extension registrations (:457-505) and does the same three things in the same order: capture the id, record the undo, then register. SandboxedPluginContext.kt:306-308 is a plain delegate like registerStatusBarItem above it. Both files already carry a comment block stating exactly why the override has to exist, and the new ones extend it rather than restating it.

Capturing provider.providerId into a local, not the provider, is load-bearing. tracker.recordUiExtensionRegistration(pluginId) { delegate.unregisterSearchProvider(id) } puts a String in the undo closure. Closing over provider instead would pin a plugin-classloader object in PluginRegistrationTracker.uiExtensionUndoByPlugin until clearPlugin ran — the same leak shape PluginRemoval warns about. The siblings do it this way and so does this.

Teardown really is covered on every path, not just unload. tracker.unregisterUiExtensions at TrackingPluginContext.kt:566 is reached from every unregisterAll() call site in DynamicPluginManager: the binary-incompatible and recoverable register failures (:1134, :1142), the already-unloaded state cleanup (:1457), unload (:1525), a partially-failed enablePlugin (:1671), reregisterInPlace (:1752, which is unregister-then-register so no duplicate-id warning), disablePlugin (:1864), and the RBAC hide path (:2447). That matters more for this registration than for most: SearchRegistryImpl is a process-wide object holding a live provider instance, so a missed teardown would leave GlobalSearchService calling into a closed classloader. And if one ever slips through, isolated("plugins") in GlobalSearchService.kt:237-245 catches LinkageError specifically for that case, so the degradation is a warning rather than a dead search.

Isolation is genuinely per plugin. The undo list is keyed by pluginId and unregisterUiExtensions does remove(pluginId), so test 4's assertion is structural, not incidental. One plugin's register failure cleanup cannot take another's provider.

Nothing else in this repo needed the same edit. I grepped for every production PluginContext implementation: only DefaultPlugin, TrackingPluginContext and SandboxedPluginContext exist, and SearchProvider appears nowhere under modules/ — so there is no kernel/out-of-process context that still swallows the call. The chain the PR describes is the whole chain.

---

Findings

1. The multi-window consequence belongs in the code or AGENTS.md, not only the PR body. (documentation)

SearchRegistryImpl is process-wide (SearchRegistryImpl.kt:17, an object) while DefaultPlugin and its DynamicPluginManager are per window (DefaultPlugin.kt:153-161, :352-361). DefaultPlugin.dispose() → dynamicPluginManager.disposeWindow() → dispose(closeTabsAcrossWindows = false) uninstalls every plugin that manager holds (DynamicPluginManager.kt:2358-2379), which now unregisters the provider by id from the shared registry. So closing the window whose manager loaded the bookmarks plugin removes bookmark results from global search in a window that is still open, and nothing re-registers short of a reload.

This is pre-existing behaviour for panels, MCP tool providers and status-bar items, and the PR's "Not covered" section states it accurately — I agree it is the right call, and owner-aware registration would be a change to the registries. The ask is only that the fact live next to the code: this repo's AGENTS.md already records the three-wrapper rule and names mcpToolRegistry as the precedent, and this is the same paragraph's subject matter. A sentence there (or above the new override) is what stops the next reader re-deriving it. Fix this →

2. The guard test misses members inherited from a PluginContext supertype. (test hardening)

PluginContextWrapperForwardingTest.kt:23-25 uses PluginContext::class.declaredMembers, which is declared-only. Today that is the whole interface, so the test is correct as written. But the api is an external artifact (ai.rever.boss.plugin.api, not in this repo) — if it is ever split so that PluginContext : SomeBaseContext, the guard silently narrows to the leaf interface and the exact bug it exists to catch becomes invisible again for every inherited member. PluginContext::class.members minus Any::class.members by name, or a walk over allSupertypes, costs one line and removes the dependency on an upstream refactor nobody here would see. Fix this →

3. The guard names two classes literally. (test hardening, lower value)

I confirmed those two are the only wrappers that exist, so this is not a gap today. Worth one line of KDoc saying a third PluginContext wrapper has to be added here, since the file is the only thing that will remind anyone.

Two things the guard deliberately cannot do, and the KDoc could say so in half a sentence: it matches by name, so an override that compiles but forwards to the wrong delegate, or drops the call, passes; and it will fail on any api version bump that adds a member. The second is the intended friction — the message names the missing members, which is the right failure — but a reader meeting it for the first time on an unrelated api bump will want the sentence.

4. unregisterAll's KDoc is now three registration kinds out of date. (nit, pre-existing)

TrackingPluginContext.kt:534-536 still reads "Unregister all panels and tab types registered by this plugin", while the body also does MCP providers, UI extensions and now search providers. The inline comments in the body are accurate; only the KDoc above it is stale. Cheap to correct in this PR since search providers are what just widened it.

5. println at TrackingPluginContext.kt:538,:542,:549,:551. (nit, pre-existing, not this PR's)

AGENTS.md requires BossLogger over println. Not introduced here and not something I would ask this PR to sweep — noting it because the new teardown runs through that function.

---

On the test suite itself

The four behavioural tests are well aimed: they build the real TrackingPluginContext(SandboxedPluginContext(InProcessPluginSandbox, host)) chain rather than testing either wrapper alone, which is the only shape that would have caught this bug — each wrapper in isolation looks fine, and the defect was that neither declared the member. RecordingHost standing in for DefaultPlugin is the same technique TrackingPluginContextMcpTest already uses for the sibling registration, so the two read as one pattern. The mutation table covers the interesting failures, including the one that matters most (teardown unregistering a different id).

The honest gap the PR already names is that nothing asserts the end-to-end DefaultPlugin → SearchRegistryImpl → GlobalSearchService path. I checked that path by hand and it holds: DefaultPlugin.kt:566 writes into SearchRegistryImpl.registerProvider, GlobalSearchService.kt:427 reads SearchRegistryImpl.providers.value, and convertPluginSearchResult (:480-504) always produces a BookmarkResult for the bookmarks category rather than dropping rows with a null url — so a registered provider's results do reach the dialog. A test there would have to mutate the process-wide SearchRegistryImpl singleton, which is a real pollution risk for a parallel test run; I would not add it just to close the table.

---

Verification limits on my side

I reviewed this statically. I could not compile or run the tests in this job: the boss-plugin-api artifact is not in the local Gradle cache and I am not able to fetch it here, so PluginContext itself was not readable. That means I could not independently confirm the PR's claim that the guard reports exactly [registerSearchProvider, unregisterSearchProvider] against dev and nothing else — PR CI is the check for that, and the claim is consistent with everything I could see. I also could not fetch origin/dev, so I reviewed the single commit 11b6f26 rather than a three-dot diff; the commit is the PR's only commit, so the content is the same.

Line lengths are within the 140-column .editorconfig limit in all four files, and both wrappers already have TooManyFunctions baseline entries (composeApp/detekt-baseline.xml:1485, plugin-platform/plugin-sandbox/detekt-baseline.xml:28), whose signatures are class-level — so adding two functions does not invalidate them. That is consistent with the author's report of a clean ktlintCheck and detekt.
· branch fix/plugin-search-provider-forwarding

[claude]

Claude finished @kshivang's task in 5m 22s —— View job

---

Review: search-provider forwarding through both plugin-context wrappers

• Read the diff (commit 11b6f26 against its parent 5a2e818)
• Verify the consumer path independently
• Review forwarding + teardown in TrackingPluginContext
• Review SandboxedPluginContext forwarding
• Review both new tests
• Check plugin isolation / registry ownership
• Check CI status
• Post review

Verdict

The diagnosis and the fix are correct, minimal, and land in the right place. I traced the whole path independently:

• PluginContext members are defaults, and TrackingPluginContext (TrackingPluginContext.kt:306) / SandboxedPluginContext (SandboxedPluginContext.kt:67) are the only two delegating implementations in the tree (DefaultPlugin.kt:161 is the real one). So the two overrides per wrapper is the complete set - there is no third wrapper the fix misses.
• DefaultPlugin.registerSearchProvider (DefaultPlugin.kt:566) is the sole writer into SearchRegistryImpl, and GlobalSearchService.searchPluginProviders (GlobalSearchService.kt:427) is the sole reader. Nothing in the host registers a provider directly on DefaultPlugin, so before this change the registry could only ever be empty - the bug report is exact, and the blast radius of the fix is "a category that never appeared now appears".
• The teardown is wired correctly: recordUiExtensionRegistration at TrackingPluginContext.kt:511 is consumed by tracker.unregisterUiExtensions(pluginId) in unregisterAll (TrackingPluginContext.kt:566), and providerId is captured at register time rather than read from plugin code during teardown, matching the five sibling registrations exactly. Record-before-register is also the sibling convention, and a delegate that throws just leaves an undo that calls SearchRegistryImpl.unregisterProvider on an absent id, which is a no-op.
• SandboxedPluginContext applies no permission gate to any sibling registry (panel menus, settings pages, deep links, shortcuts, status bar, MCP), so plain delegation is the consistent choice, not a dropped check.

No correctness defect found. Comments below are nits and follow-ups.

On the multi-window same-id limitation

Your disclosure is accurate, and I can make the reachability concrete rather than hypothetical, because it is worth writing down for whoever picks up the follow-up:

dynamicPluginManager is an instance by lazy on DefaultPlugin (DefaultPlugin.kt:258), so there is one manager per window. The persisted load runs once globally into the first window's manager (DefaultPlugin.kt:164-175), but loadExternalPlugins (DefaultPlugin.kt:1155) runs per window and a secondary window's manager starts empty - so window 2 installs and register()s bookmarks again. SearchRegistryImpl.registerProvider warns "Search provider already registered" and replaces by id, and window close reaches disposeWindow → uninstallPlugin(force = true) → unregisterAll (DynamicPluginManager.kt:2358-2379). So closing either window unregisters bookmarks process-wide, and the other window's still-loaded plugin stops contributing to search until relaunch.

That is strictly better than today (nothing registered in any window), and it is the same shape as McpToolRegistryImpl and the other recorded registrations, so I agree this is registry-ownership work and not a reason to change these wrappers. Agreed too that the two-plugin test uses distinct ids and is not evidence about same-id ownership; the register-A / replace-with-B / tear-down-A case belongs with that future change.

Nits

1. Two enumerating comments now omit search providers. TrackingPluginContext.kt:562-565 lists the kinds unregisterAll tears down ("panel menu items, settings pages, deep-link handlers, shortcuts, status-bar widgets") and the tracker KDoc at TrackingPluginContext.kt:88-96 lists the same set. The mechanism needs no edit (that is the point of the recorded-undo design, as the comment says), but both lists are now incomplete for a reader auditing lifecycle coverage. Fix this →

2. The guard compares names, and only direct declarations. missingFrom maps to it.name, so a wrapper declaring a same-named member with a different signature would pass; and PluginContext::class.declaredMembers covers only what PluginContext itself declares, so if the api ever splits the interface into a base plus extensions, inherited members would silently stop being checked - the guard would go green while a new gap opened. Flattening supertypes (excluding Any, whose members no wrapper declares) would close both. Not a blocker: the current shape does catch the class of bug that motivated it, and the failure message names the members.

3. Guard placement. The SandboxedPluginContext half lives in composeApp/src/desktopTest, so :plugin-platform:plugin-sandbox:desktopTest does not cover its own class. The dependency direction forces this for a combined test (plugin-sandbox cannot see TrackingPluginContext), and PR CI runs build, so merges are gated. Still, a sandbox-local copy of the one assertion would protect that module's own check/publish path - the same reasoning AGENTS.md records for the $stable field tests ("co-location alone does not put a test on the publish path").

Follow-up this PR makes live (not a change to request here)

searchPluginProviders now actually executes plugin code for the first time. isolated() contains a failing provider, but there is no per-provider timeout and all nine sources join on awaitAll (GlobalSearchService.kt:184-197), so a provider that hangs hangs every category of global search, not just its own. Pre-existing design, previously unreachable because the registry was always empty. Worth an issue against GlobalSearchService rather than a change in this PR, whose stated scope (no change to DefaultPlugin, SearchRegistryImpl or GlobalSearchService) is the right one.

What I could not verify

• No test execution. Gradle invocation was not permitted in this environment (command approval denied), so I did not reproduce your 557/150 test counts, the red-first result, or the mutation table. Review is static only.
• No fresh-dev integration check. git fetch origin dev was also denied, so I reviewed commit 11b6f26 against its parent 5a2e818 rather than against dev at 6004233. I cannot confirm the merge is clean or that the api pin is 1.0.93 from here.
• CI is still queued, not yet run: run 34872616337 reports queued with no results, consistent with your note that it was action_required and has now been approved. The Windows symlink-privilege failure you baselined in DesktopFileScannerTest is unrelated to this diff on inspection (no shared code path), but confirm it against the run's baseline as you planned.
• Line lengths are within the 140 ktlint limit in all four files; I did not run ktlintCheck or detekt themselves.

I have no blocking findings. Recommend merging once that run is green.
| branch fix/plugin-search-provider-forwarding

[nikhil-jangir-iitk]

Thanks @kshivang. Rebased onto dev d22d2bfeb (API 1.0.93, #694 merged). The rebase surfaced one thing: API 1.0.93 added PluginContext.organisationMembersProvider, which neither wrapper declared, so the forwarding guard failed for both. Follow-up 6060a0ebe forwards it (the host has no implementation yet, so plugins still get null).

From the reviews: the guard now includes inherited members and states what it cannot see, the listing comments and unregisterAll KDoc name search providers, and AGENTS.md records the gap. With #694 merged, its two-window teardown test now covers search too.

593 + 150 tests (1 known Windows symlink-privilege failure), ktlint/detekt clean, the new mutations caught (4/4), 0 added conflicts across 57 PR/file pairs.

[nikhil-jangir-iitk]

Rebased to clear the merge conflict. No content change: git range-diff reports both patches identical (=), same 6 files, +234/-8.

Same inherited-history cause as the earlier rebase on this branch. The #713 integration collapsed the individual PR commits, so the old base 77c4a4071 is no longer an ancestor of dev and the diff was being computed against a stale merge base. The PR's file list drops from 86 to 6.

New head 395753b31 sits on dev 928ee3647. Current dev c0584c451 is two commits ahead of that and git merge-tree against it is clean. The focused suite passes 13/13 locally on this head, including the wrapper forwarding guard that covers organisationMembersProvider.

New exact-head CI is running; the prior head's checks do not validate this commit.

[kshivang]

@ninja-lifts The current six-file change and follow-up are reviewed. Both wrappers now forward search-provider registration/removal and organisationMembersProvider; tracked teardown uses the existing owner-aware host registry. The inherited-member guard and the updated two-window teardown test address the prior review, and #694 supplies the ownership behavior that the earlier handoff lacked. The name-only guard limitation is accurately documented; extra sandbox-local duplicate guards or unrelated logging cleanup are not required for this fix. Provider timeouts remain a separate search-service improvement, not a reason to duplicate or broaden these wrappers.

I downloaded exact-head 395753b CI artifacts: all 13 focused forwarding/guard/two-window tests and 150 sandbox tests pass, with no failures or skips. All ten Build & Test checks pass, including Linux/macOS/Windows and quality (ktlint/detekt). A new local build was queued under the shared lock but stopped before Gradle began because the existing exact-head evidence is sufficient; no new local pass is claimed. Integration with refreshed dev is clean. No application was launched or visual bookmark search performed. Your code/tests/follow-up remain your contribution; this sweep made no source repairs. Prior original-work score 9/10 is preserved. Proceeding only with the locked exact-head squash merge into dev after one final refresh.