fix(plugins): keep session follow alive across renderer reloads and inside scope

[SeanLiu326]

Fixes #1753 Fixes #1754

Fixes #1753
Fixes #1754

Summary

• Reload survival ([Bug] Renderer reload resets the session-push counter, permanently freezing pi.session.getActive on a stale session #1753): session pushes now carry a per-renderer-load bootId (crypto.randomUUID() in useAppShellRuntime), and the main-side gate resets its high-water mark whenever the boot id changes. Within one boot, the out-of-order guard behaves exactly as before.
• Scope parity ([Bug] session:activated fan-out skips the activation-scope check that session.getActive enforces #1754): broadcastSessionEvent builds a per-recipient scope filter for selection events (SESSION_SCOPED_EVENTS) — pluginActiveInProject(pluginId, sessionProjects.get(sessionId)), falling back to the existing workspace-relative check for sessions without a project — and passes it to plugins.broadcastEvent and pluginViews.broadcast (both gained an optional filter parameter). Project-scoped plugins and views no longer receive other projects' session identity.

Rationale

As reported: the high-water mark sat over a counter that a renderer reload (the automatic render-process-gone recovery) restarts from one, permanently freezing pi.session.getActive on the pre-reload session (#1753); and the push side skipped the activation-scope check the pull side (session.getActive, ui.revealView) enforces, leaking other projects' session ids to project-scoped plugins (#1754).

Affected specs / ADRs / E2E scenarios

• docs/spec/07-plugins/03-* §session describes the snapshot/selection surface; the scope rule now covers pushes as well — spec wording, if wanted, is a one-line follow-up for the maintainers (happy to add).
• E2E plugin-session-*: NOT RUN — the new coverage is a real-process behavioral test plus source contracts (below).

Validation (on latest origin/main)

• New behavioral test in plugin-session-follow.test.mjs over the real PluginRuntime with two forked plugin processes and message spies: with the scope filter, session:activated reaches exactly the in-scope plugin process — out-of-scope receives nothing. Full file: 10 pass / 0 fail.
• New contract test: the gate resets on boot-id change, keeps the out-of-order refusal within a boot, the renderer owns a per-load boot id, and the payload carries it.
• tsc --noEmit in apps/desktop: pass.

Compatibility / remaining risk

• Renderer builds that do not send bootId (none exist — renderer and main ship together) keep the old strict behavior: the reset branch requires a boot id.
• Project-scoped plugins stop receiving out-of-project session:activated pushes — that is the point; plugins that legitimately followed multiple projects must declare the corresponding scope.

[muzimu217]

Reviewed both halves; they match what #1753 and #1754 asked for, and the scope fix picks the semantics I flagged as the risky one — correctly.

#1754 scope parity — right predicate. The filter keys on the announced session's project (sessionProjects.get(sessionId)), not on the receiving view's own window, and it is applied to both delivery surfaces (plugins.broadcastEvent(..., scopeFilter) and pluginViews.broadcast(..., scopeFilter)). That is exactly the line session.getActive holds, so the pull and push halves now agree. Falling back to the workspace-relative check when the session has no project is the right degenerate case.

#1753 boot id — the guard still does its job. Reading the gate: a changed bootId resets the mark and accepts that push; within one boot the comparison is unchanged; and when bootId is absent (seq > 0, bootId === null) it falls into seq >= activeSessionSeq, which is the old behavior — so an older renderer cannot be frozen by this change. Good.

Two notes, neither blocking:

1. pluginActiveInProject is optional (pluginActiveInProject?:), so a caller that forgets to wire it silently loses the scope filter and we are back to [Bug] session:activated fan-out skips the activation-scope check that session.getActive enforces #1754 with no test failure. Since isPluginActiveInProject was already required, making this one required too (or asserting it at construction) would keep the regression from reappearing quietly.
2. The boot id is generated per renderer load in useAppShellRuntime, which means two windows open at once have different ids — each will reset the other's high-water mark on every push. That is harmless for correctness (each push is still applied) but it makes the out-of-order guard ineffective across windows. Worth a comment stating the guard is per-window, or keying the map by window id if cross-window ordering matters.

Tests: the added plugin-session-follow.test.mjs cases cover the scope filter and the reload path, which is the right surface for both.

[vastsa]

I confirmed both reported paths on current main: the renderer-owned sequence can restart below Main's old high-water mark after reload, and session:activated currently reaches plugins without the pull API's project-scope check. The boot ID and recipient filter appear to address those root causes.

I can't merge this head yet: the required JS CI job fails in test/main-wiring-contract.test.mjs (the assertions at lines 201 and 225 still require the old unfiltered broadcast calls/signatures). Please update those contracts to assert the filtered delivery behavior and rerun CI. The PR also reports the plugin-session E2E as not run; please record the relevant reload/scope user path on a candidate or identify the existing E2E that covers it. Thanks for fixing both the stale-session and cross-project scope paths.

[muzimu217]

Confirmed both of your points, and the CI failure is narrower than it looks — it is two source-shape assertions that pin the old call signature, not a behavior problem.

apps/desktop/test/main-wiring-contract.test.mjs:

:207  assert.match(fanOut, /plugins\.broadcastEvent\(event, \[payload\]\)/);
:231  assert.match(services, /plugins\.broadcastEvent\(event, \[payload\]\)/);

Both assert the literal two-argument call. Your change passes a third argument (scopeFilter), so the regex no longer matches and the three failures ("a finished turn is announced to every plugin surface", "the selected session is announced to every plugin surface", "host panel events reach docked views as well as detached windows") all come from that one pattern. Widening them to /plugins\.broadcastEvent\(event, \[payload\](?:, [^)]+)?\)/ — or better, asserting the three delivery surfaces are still present and letting the filter be an optional argument — takes the job green without weakening what those tests are for.

Two things about your head that I verified while reading it:

1. The scope predicate is the right one: it keys on sessionProjects.get(sessionId) (the announced session's project), not on the receiving view's own window, and it reaches both plugins.broadcastEvent and pluginViews.broadcast. That is what makes the push side agree with session.getActive.
2. The boot-id gate degrades correctly — with no bootId it falls through to seq >= activeSessionSeq, so an older renderer cannot be frozen.

Non-blocking, from earlier: pluginActiveInProject is an optional dependency, so a future caller that forgets to wire it silently loses the filter with no test failing. Making it required (as isPluginActiveInProject already is) would keep #1754 from coming back quietly.

[SeanLiu326]

Both asks are in ce8ffae:

1. CI contracts: the two main-wiring-contract assertions (:207/:231) now accept the optional scope-filter argument — /plugins\.broadcastEvent\(event, \[payload\](?:, scopeFilter)?\)/ and the same for pluginViews.broadcast — which takes the three failures green without weakening what they pin (the three delivery surfaces and their isolated catches are still asserted).
2. pluginActiveInProject is now required on the plugin-services dependency (matching isPluginActiveInProject), so a future caller that forgets the wiring fails at construction instead of silently losing the filter.
3. Documented the per-window property: the boot-id comment in plugin-ui-ipc.ts now states that with two windows open the out-of-order guarantee is per renderer load, since only one window's session is on screen at a time.

On the E2E ask: there is no existing scenario covering the reload/scope user path — scripts/e2e-plugin-* covers ui-slots, detail focus, import deps, jsx runtime and the reviews drawer, none of them session follow (#1745 shipped without one). Running one locally is also blocked: the runners need a built host-core binary and this Windows box has no Rust toolchain. So recorded as NOT RUN — no existing scenario; candidate authoring needed. The behavioral proof in-repo forks the real plugin host processes over real IPC and drives the actual fan-out (the spy test asserts the out-of-scope process receives nothing), which is the strongest non-E2E evidence available. Happy to author the E2E scenario as a follow-up if you want it on the same PR.

[muzimu217]

One more assertion of the same shape — this is why the job is still red after ce8ffae8:

apps/desktop/test/plugin-work-panel-views.test.mjs:285

assert.match(viewHostSource, /broadcast\(event: string, payload: unknown\)/);

Your plugin-view-host.ts signature gained the optional filter, so this no longer matches. Widening it the same way you did for the other two — /broadcast\(event: string, payload: unknown(?:, filter\?:[^)]*)?\)/ — should take the run green; the rest of that test (the channel name and the three main-side call sites) is unaffected and still pins what matters.

Everything else in ce8ffae8 looks right: the two main-wiring-contract assertions now accept the optional argument, and making pluginActiveInProject required closes the silent-regression hole I flagged.

[muzimu217]

Still the same single assertion on ce8ffae8 — the head has not changed since my last note, so the fix below is still pending. This is now the only thing between the PR and green.

apps/desktop/test/plugin-work-panel-views.test.mjs:285:

// before
assert.match(viewHostSource, /broadcast\(event: string, payload: unknown\)/);

// after — accepts the optional filter your view host now takes
assert.match(viewHostSource, /broadcast\(event: string, payload: unknown(?:, filter\?:[^)]*)?\)/);

If the exact parameter name differs, /broadcast\(event: string, payload: unknown(?:, [^)]*)?\)/ covers it without weakening the assertion (the method must still be named broadcast, still take the event and payload, and the rest of that test still pins the channel name and the three main-side call sites).

Nothing else fails: Head contains latest base, Rust host-core format/lint/test, Rust Windows / transcript cache, and Trusted extensions Electron E2E are all green, and the two main-wiring-contract assertions you fixed in this head are passing now.

[vastsa]

Both reported failures (#1753 and #1754) are real, and the implementation is aimed at the right causes. I can’t merge this head yet:

1. The JS CI lane still fails at apps/desktop/test/plugin-work-panel-views.test.mjs:285: its source assertion expects the old two-argument broadcast signature and rejects the new optional filter. Update that assertion to accept the optional argument while preserving the event/payload and channel assertions.
2. The new behavioral test exercises PluginRuntime.broadcastEvent directly, but not the production path that derives the filter from sessionProjects in createPluginServices and passes it to both process and docked-view delivery. Please add a regression test for a session:activated event on an out-of-scope session, proving that an in-scope plugin receives it and an out-of-scope plugin/view does not. The boot-id behavior currently has source-shape assertions only; add an observable test that accepts a new renderer boot and still rejects an older sequence within that boot.
3. The PR still targets 589e580; current main is 87a48b9. Refresh it, then run the relevant E2E for E2E-267 (or add the missing automated scenario). The current E2E report says NOT RUN, which leaves the renderer-to-main session-follow path unverified.

The direct filter and boot-id approach look sound, but the current tests do not yet prove the production wiring or the affected user path, and CI is red. Thanks for addressing the earlier contract assertions and making the scope dependency required.

[muzimu217]

I applied my suggested patch to your head locally and ran the two affected suites — so this is now a verified fix, not a suggestion:

apps/desktop/test/plugin-work-panel-views.test.mjs:285
- assert.match(viewHostSource, /broadcast\(event: string, payload: unknown\)/);
+ assert.match(viewHostSource, /broadcast\(event: string, payload: unknown(?:, [^)]*)?\)/);

Result on ce8ffae8 + that one line: plugin-work-panel-views + main-wiring-contract → 23/23 pass.

That is the whole remaining fix; the other five checks are already green. For reference, the view host signature it needs to match is plugin-view-host.ts:144 — broadcast(event: string, payload: unknown, filter?: (pluginId: string) => boolean): void.

(One note if you run these locally: the suites read source files relative to the repo root, so run them from apps/desktop, not from a scratch directory — otherwise they fail with ENOENT on src/components/workpanel/WorkPanel.tsx, which looks like a real failure but is only the working directory.)

[SeanLiu326]

All three points addressed in c419e99 (rebased on main @ 87a48b9 — the base-refresh commit):

1. plugin-work-panel-views.test.mjs:285: the view-host broadcast assertion now accepts the optional filter argument while still pinning the method name, event, payload and channel.

2. Production wiring and the boot-id behavior are now observable:

   • The push gate moved into the dependency-free plugin-session-push-gate.ts (the handler routes through it), and a new unit test drives the real gate: a new renderer boot resets the mark and the low sequences flow again, while an older sequence within that boot stays ignored — exactly the acceptance/older-rejection pair you asked for.
   • The scope-filter derivation moved into plugin-session-scope.ts (sessionEventScopeFilter) and a new unit test drives it: for session:activated on an out-of-scope session the filter passes the in-scope plugin and fails the out-of-scope one; a session without a project checks against null (not another session's map value); other session events return undefined and fan out unconditionally.

3. E2E-267: recorded as NOT RUN, and the missing automated scenario is added in this revision — there is no existing E2E covering the session-follow path (scripts/e2e-plugin-* covers ui-slots, detail focus, import deps, jsx runtime, reviews drawer), and running one locally is blocked: the runners need a built host-core binary and this Windows box has no Rust toolchain. The added scenarios verify the renderer→main path at its production seams (the gate the handler routes through, the filter createPluginServices derives from sessionProjects, and the filtered fan-out over real forked plugin processes). If you want the full Electron-boot scenario too, that needs a candidate with a host-core binary — I can't produce one from this machine.

[muzimu217]

Good — the third assertion is fixed. The remaining red is a different one, and it is mechanical:

AssertionError: unexpected plugin UI dependency: ../plugin-session-push-gate

apps/desktop/test/plugin-provider-catalog-ipc.test.mjs:19-28 compiles plugin-ui-ipc with a stubbed require and asserts every requested id is in that stub table. Since plugin-ui-ipc now imports your new ../plugin-session-push-gate module, the stub table needs an entry for it — the test is deliberately strict so a new dependency cannot slip into that surface unnoticed, which is working as intended.

Minimal addition, mirroring the shape the module exports:

"../plugin-session-push-gate": {
  // whatever registerPluginUiIpc actually calls from it — the gate factory
  // and any predicate it uses
},

If you paste the module's exported names I can be exact; from the diff it looks like the gate constructor plus the per-recipient filter helper are what the IPC layer consumes.

Two notes:

1. Everything else in c419e999 looks right — the view-host assertion now accepts the optional filter, the gate moved into an injectable dependency (which also makes the boot-id behavior testable, the thing I asked for), and the head is rebased on 87a48b9 with Head contains latest base green.
2. When this lands, the main-wiring-contract and plugin-work-panel-views assertions I flagged are all covered, so that class of "source-shape pins break when a signature grows an optional argument" is fully closed for this PR.

[muzimu217]

Exact patch, from reading your head rather than guessing:

plugin-ui-ipc.ts:14 imports { createSessionPushGate } from "../plugin-session-push-gate", so the stub entry is one line:

"../plugin-session-push-gate": { createSessionPushGate: () => () => ({ ignored: false }) },

The shape matters only as far as the test exercises it: plugin-ui-ipc.ts:52 calls createSessionPushGate() and :72 calls the result as (input) => ({ ignored }). Returning a function that always answers { ignored: false } keeps every existing assertion in that suite on its current path (no push is dropped), which is what those tests are about — they are not testing the gate itself, that is covered by your new plugin-session-push-gate tests.

If any of those assertions depend on a push being ignored, return { ignored: true } for that specific case instead — but from what I read, the suite only asserts categories and handler wiring, so false is the safe default.

[vastsa]

Both reported failures are real, and the updated implementation closes their root causes: a changed renderer boot id resets the main-process sequence gate, while session:activated is scoped using the announced session's project before delivery to plugin processes and docked views. The production fan-out contract now pins that same filter on both surfaces; the app-scoped panel remains excluded.

I added only the missing strict IPC stub and regression coverage for the production fan-out and the registered session IPC handler. The contributor's original implementation and commits are preserved.

Local validation on head 19aaedf: focused plugin/session suites 55/55 passed; workspace dependency build for desktop passed; desktop typecheck passed; pnpm check:pr-base passed with origin/main at 639f049; git diff --check origin/main...HEAD passed. The interactive E2E-267 desktop journey was not run; the updated IPC, plugin-process and wiring behavior is covered by the focused tests.

[muzimu217]

The stub landed correctly — 19aaedf46's plugin-provider-catalog-ipc.test.mjs:32 now carries "../plugin-session-push-gate": { createSessionPushGate: gateFactory }, injectable, which is better than the constant I sketched. The JS job is green on 39358272062, so that thread is closed.

The remaining red on your branch is the same repo-level CI breakage I just diagnosed on #1759: the new "provider optional client certificate E2E" step from this morning's #1724 merge launches Electron without xvfb-run, so Missing X server or $DISPLAY kills it deterministically on every branch containing current main — including main itself (run 39358378847). Nothing in your diff; the one-line ci.yml fix is in the #1759 note. Until that lands, Electron E2E reds on PR branches are noise, not signal.