From f2f168f6a10a24c924516808f414baba52b1c080 Mon Sep 17 00:00:00 2001 From: Michael Nguyen Date: Thu, 23 Jul 2026 12:32:10 -0700 Subject: [PATCH] fix(plugins): seed proactive company scopes before worker setup() + events.subscribe resolver parity (LOOA-695) (#10113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [x] I searched the GitHub PR list for similar PRs (dedup search). No open PR touches the proactive `events.subscribe` ordering path; #10103 (merged) is the predecessor whose ordering bug this fixes. ## Thinking Path The gateway worker's outbound push path is permanently dead (`eventSubscriptions: 0`, `notifier.received: 0`, `decisions.delivered: 0`). The plugin loader authorizes the worker's **proactive company scopes only AFTER `startWorker` resolves**, but a proactive plugin issues its one-shot `events.subscribe` calls from `setup()` — which runs *while `startWorker` is still awaiting the worker's initialize response*. So at subscribe time `proactiveCompanyScopes` is still empty → `contextForWorkerMessage` resolves no scope → the governed-access gate rejects every subscribe with `company context is required`. The gateway subscribes once and never retries, so `eventSubscriptions` stays 0 for the worker's life. This is an **ordering bug in the #10103 fix**, not a new method — same #9557 governed-access class as `config.get` (#10092) and `state.get` (#10103). Confirmed live at the 18:21:21Z worker respawn on `3093c5e` (host log), and again at the 19:01:04Z restart (still `events.subscribe: company context is required`, `eventSubscriptions:0`). ## What Changed 1. **Loader ordering** (`plugin-loader.ts`): load `registry.listConfigs(pluginId)` in a new step 4b **before** `startWorker`, and thread the configured company set into `WorkerStartOptions.proactiveCompanyScopes` so the worker handle is authorized *before the child process issues any host call*. The same rows are reused for startup config delivery (step 5b) — no second `listConfigs` round-trip. The runtime config-change path (`routes/plugins.ts`) still refreshes scopes via `setProactiveCompanyScopes` (unchanged). 2. **Handle seed** (`plugin-worker-manager.ts`): `createPluginWorkerHandle` seeds its `proactiveCompanyScopes` set from options at creation, before spawn. 3. **Resolver/gate parity** (`plugin-worker-manager.ts`): `referencedCompanyId(method, params)` now mirrors the SDK gate `requestedCompanyScope` exactly in the functional direction — adds `events.subscribe → params.filter.companyId` (how `ctx.events.on(name, { companyId }, fn)` issues its subscribe), and declines the gate's wildcard cases (`companies.list`, `scopeKind:"company"` without `scopeId`) so proactive access only ever grants a **single explicit configured company, never "all"**. Answers LOOA-693 AC#4 (host/gate extraction parity) in the functional direction. ## Tests New `plugin-worker-manager.test.ts` cases (drive a real worker): - a `setup()`-time `events.subscribe({ filter: { companyId } })` for an options-seeded company is **admitted** (fails on prior code — no options seed, no filter parity); - an unconfigured company stays **denied**; - an unseeded worker stays **denied**. Full `plugin-worker-manager.test.ts` suite: **21 passed**. Server `tsc --noEmit`: clean. All PR CI green (typecheck, server/workspace suites, e2e, build, security scans). ## Risks - **Scope-widening risk (primary).** The change grants proactive host access keyed off configured company rows. Mitigated by: the authorized set is exactly `registry.listConfigs(pluginId).map(companyId)`; wildcard cases (`companies.list`, company-scoped key without `scopeId`) resolve to `null`, never `{ kind: all }`; empty/whitespace ids dropped; an empty config set grants zero proactive access. This is the surface SecurityEngineer must sign off (see Security gate). - **In-invocation path unchanged.** Calls carrying a host-issued `paperclipInvocationId` keep the existing strict single-company match; the proactive branch only applies when there is no invocation id — so no regression to the enforced request path. - **Blast radius.** Loader step 4b is best-effort: a `listConfigs` failure logs and proceeds with an empty seed (fails closed — no push, not a crash), matching today's behavior. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`) via Claude Code (agent: CTO). ## Security gate Touches the company-scope resolution path (same surface as #10103). Routed through **SecurityEngineer review before merge** (tracked on LOOA-696) — must not widen beyond configured companies; in-invocation strict single-company match untouched; wildcard cases deliberately declined in the proactive direction. ## Verification once live - Host log clean of `events.subscribe: company context is required` at worker start - loader logs `eventSubscriptions: N>0` - beat `notifier.received` / `decisions.delivered` move on real issue/approval activity Parent: LOOA-629 (outbound push half of "gateway active"). LOOA-695. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 --- .../plugin-worker-invocation-scope.cjs | 9 ++ .../__tests__/plugin-worker-manager.test.ts | 89 +++++++++++++ server/src/services/plugin-loader.ts | 118 ++++++++++-------- server/src/services/plugin-worker-manager.ts | 53 ++++++-- 4 files changed, 210 insertions(+), 59 deletions(-) diff --git a/server/src/__tests__/fixtures/plugin-worker-invocation-scope.cjs b/server/src/__tests__/fixtures/plugin-worker-invocation-scope.cjs index 610609819e..8300a60689 100644 --- a/server/src/__tests__/fixtures/plugin-worker-invocation-scope.cjs +++ b/server/src/__tests__/fixtures/plugin-worker-invocation-scope.cjs @@ -32,6 +32,15 @@ function sendNestedHostRequest(originalRequest, invocationId) { namespace: params.namespace || "ns", stateKey: params.stateKey || "key", } + : hostMethod === "events.subscribe" + ? { + // The subscribe shape the SDK issues from setup() via + // ctx.events.on(name, { companyId }, fn): the requested company lives in + // filter.companyId, NOT a top-level companyId. The host resolver must + // mirror the SDK gate and read it from there (LOOA-695). + eventPattern: params.eventPattern || "issue.updated", + filter: { companyId: requestedCompanyId }, + } : { companyId: requestedCompanyId, }; diff --git a/server/src/__tests__/plugin-worker-manager.test.ts b/server/src/__tests__/plugin-worker-manager.test.ts index 0100af5f08..216313a7d0 100644 --- a/server/src/__tests__/plugin-worker-manager.test.ts +++ b/server/src/__tests__/plugin-worker-manager.test.ts @@ -639,3 +639,92 @@ describe("plugin proactive company scope (LOOA-629)", () => { } }); }); + +describe("plugin proactive events.subscribe: options-seeded scope + filter parity (LOOA-695)", () => { + // The chat gateway subscribes to issue.*/approval.* from setup() via + // ctx.events.on(name, { companyId }, fn), which the SDK turns into a proactive + // (no-invocation) events.subscribe whose company lives in params.filter.companyId. + // Two things had to hold for outbound push to work and neither did before this + // fix: + // (1) the authorized company set must be present BEFORE the worker's setup() + // calls land — the loader used to set it only after startWorker resolved, + // so it was seeded via WorkerStartOptions at handle creation instead; + // (2) the host's proactive-scope resolver (referencedCompanyId) must derive + // events.subscribe's company from filter.companyId, mirroring the SDK + // gate (requestedCompanyScope). + // Each case drives a real worker so the subscribe flows through the manager's + // context resolution exactly as it does in production. + function makeEventsHandle(seededCompanies: readonly string[]) { + const eventsSubscribe = vi.fn(async () => undefined); + const hostHandlers = createHostClientHandlers({ + pluginId: "test.plugin", + capabilities: ["events.subscribe"], + services: { + events: { subscribe: eventsSubscribe }, + } as unknown as HostServices, + }); + const handle = createPluginWorkerHandle("test.plugin", { + entrypointPath: INVOCATION_SCOPE_WORKER_ENTRYPOINT, + manifest: TEST_MANIFEST, + config: {}, + instanceInfo: { instanceId: "instance-1", hostVersion: "1.0.0" }, + apiVersion: 1, + hostHandlers, + // Seeded at handle creation — the loader now threads the plugin's + // configured companies here BEFORE startWorker, never via a post-start + // setProactiveCompanyScopes call. + proactiveCompanyScopes: seededCompanies, + }); + return { handle, eventsSubscribe }; + } + + it("admits a setup()-time events.subscribe for a company seeded via WorkerStartOptions", async () => { + const { handle, eventsSubscribe } = makeEventsHandle(["company-1"]); + try { + await handle.start(); + // No post-start setProactiveCompanyScopes call: the seed from options is + // the only authorization, exactly as it is when the worker subscribes + // during setup() before startWorker resolves. + await handle.call("getData", { + params: { mode: "omit", hostMethod: "events.subscribe", requestedCompanyId: "company-1" }, + } as unknown as HostToWorkerMethods["getData"][0]); + expect(eventsSubscribe).toHaveBeenCalledTimes(1); + expect(eventsSubscribe.mock.calls[0]?.[0]).toMatchObject({ + filter: { companyId: "company-1" }, + }); + } finally { + await handle.stop().catch(() => undefined); + } + }); + + it("still denies a proactive events.subscribe for a company outside the seeded set", async () => { + const { handle, eventsSubscribe } = makeEventsHandle(["company-1"]); + try { + await handle.start(); + await expect(handle.call("getData", { + params: { mode: "omit", hostMethod: "events.subscribe", requestedCompanyId: "company-2" }, + } as unknown as HostToWorkerMethods["getData"][0])).rejects.toMatchObject({ + code: PLUGIN_RPC_ERROR_CODES.INVOCATION_SCOPE_DENIED, + message: expect.stringContaining("company context is required"), + }); + expect(eventsSubscribe).not.toHaveBeenCalled(); + } finally { + await handle.stop().catch(() => undefined); + } + }); + + it("denies a proactive events.subscribe when no company is seeded", async () => { + const { handle, eventsSubscribe } = makeEventsHandle([]); + try { + await handle.start(); + await expect(handle.call("getData", { + params: { mode: "omit", hostMethod: "events.subscribe", requestedCompanyId: "company-1" }, + } as unknown as HostToWorkerMethods["getData"][0])).rejects.toMatchObject({ + code: PLUGIN_RPC_ERROR_CODES.INVOCATION_SCOPE_DENIED, + }); + expect(eventsSubscribe).not.toHaveBeenCalled(); + } finally { + await handle.stop().catch(() => undefined); + } + }); +}); diff --git a/server/src/services/plugin-loader.ts b/server/src/services/plugin-loader.ts index 17ad598aef..5df3f62cf6 100644 --- a/server/src/services/plugin-loader.ts +++ b/server/src/services/plugin-loader.ts @@ -2142,6 +2142,35 @@ export function pluginLoader( // the same configChanged path an operator config-save uses. const config: Record = {}; + // ------------------------------------------------------------------ + // 4b. Load stored company configs BEFORE starting the worker + // ------------------------------------------------------------------ + // The worker authorizes its proactive (no-invocation) company scopes from + // its configured companies. A proactive plugin — e.g. the chat gateway — + // issues its one-shot events.subscribe calls from setup(), which runs + // while startWorker is still awaiting the worker's initialize response, so + // the authorized company set must be seeded onto the worker handle BEFORE + // startWorker spawns the process — not after startWorker resolves. + // Setting it afterwards (the previous ordering) was too late for those + // setup()-time subscribes: the governed-access gate rejected every one + // with "company context is required" and outbound push stayed dead + // (eventSubscriptions: 0) for the worker's life (LOOA-695). The same rows + // drive startup config delivery in step 5b below. Listing is best-effort: + // if it fails the worker still starts, just with no proactive access. + let configRows: Awaited> = []; + try { + configRows = await registry.listConfigs(pluginId); + } catch (listErr) { + log.debug( + { + pluginId, + pluginKey, + err: listErr instanceof Error ? listErr.message : String(listErr), + }, + "plugin-loader: could not list stored configs before worker start", + ); + } + // ------------------------------------------------------------------ // 5. Spawn worker process // ------------------------------------------------------------------ @@ -2155,6 +2184,12 @@ export function pluginLoader( hostHandlers, autoRestart: true, env: buildPluginWorkerEnv({ manifest, instanceInfo }), + // Authorize the worker to act on each configured company from its + // proactive loops/timers (LOOA-629). Seeded here so it is in place + // before any setup()-time worker→host call (LOOA-695). The authorized + // set is exactly the plugin's configured companies — proactive access + // never reaches an unconfigured company. + proactiveCompanyScopes: configRows.map((row) => row.companyId), }; // Repo-local plugin installs can resolve workspace TS sources at runtime @@ -2187,60 +2222,39 @@ export function pluginLoader( // (METHOD_NOT_IMPLEMENTED) or is momentarily unavailable simply keeps the // runtime ctx.config.get(companyId) model. onConfigChanged is idempotent // for well-behaved plugins, so replaying an unchanged config is safe. - try { - const configRows = await registry.listConfigs(pluginId); - - // Authorize the worker to act on each configured company from its - // proactive loops (LOOA-629). A proactive plugin (e.g. the chat - // gateway's notifier drain) makes company-scoped worker→host calls - // outside any host-issued invocation; without this the governed-access - // gate rejects them with "company context is required". The authorized - // set is exactly the plugin's configured companies — proactive access - // never reaches an unconfigured company. - workerManager.setProactiveCompanyScopes( - pluginId, - configRows.map((row) => row.companyId), - ); - - for (const row of configRows) { - try { - await workerManager.call(pluginId, "configChanged", { - config: (row.configJson ?? {}) as Record, - companyId: row.companyId, - }); - } catch (configErr) { - // A single-tenant worker fails closed (CROSS_TENANT_CONFIG) rather - // than collapse onto a second company's config — surface that at - // warn so the misconfiguration (multiple distinct companies - // configured for a single-tenant plugin) is visible, instead of - // being lost in the best-effort debug stream. - const code = (configErr as { code?: number } | null)?.code; - const details = { - pluginId, - pluginKey, - companyId: row.companyId, - code, - err: configErr instanceof Error ? configErr.message : String(configErr), - }; - if (code === PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG) { - log.warn( - details, - "plugin-loader: startup config delivery rejected — single-tenant plugin configured for multiple companies", - ); - } else { - log.debug(details, "plugin-loader: startup config delivery skipped for company"); - } - } - } - } catch (listErr) { - log.debug( - { + // + // Reuses the `configRows` loaded in step 4b (which also seeded the + // worker's proactive company scopes before startup); no second listConfigs + // round-trip is needed here. + for (const row of configRows) { + try { + await workerManager.call(pluginId, "configChanged", { + config: (row.configJson ?? {}) as Record, + companyId: row.companyId, + }); + } catch (configErr) { + // A single-tenant worker fails closed (CROSS_TENANT_CONFIG) rather + // than collapse onto a second company's config — surface that at + // warn so the misconfiguration (multiple distinct companies + // configured for a single-tenant plugin) is visible, instead of + // being lost in the best-effort debug stream. + const code = (configErr as { code?: number } | null)?.code; + const details = { pluginId, pluginKey, - err: listErr instanceof Error ? listErr.message : String(listErr), - }, - "plugin-loader: could not list stored configs for startup delivery", - ); + companyId: row.companyId, + code, + err: configErr instanceof Error ? configErr.message : String(configErr), + }; + if (code === PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG) { + log.warn( + details, + "plugin-loader: startup config delivery rejected — single-tenant plugin configured for multiple companies", + ); + } else { + log.debug(details, "plugin-loader: startup config delivery skipped for company"); + } + } } // ------------------------------------------------------------------ diff --git a/server/src/services/plugin-worker-manager.ts b/server/src/services/plugin-worker-manager.ts index ced8262be1..25e6882a67 100644 --- a/server/src/services/plugin-worker-manager.ts +++ b/server/src/services/plugin-worker-manager.ts @@ -183,6 +183,17 @@ export interface WorkerStartOptions { execArgv?: string[]; /** Environment variables passed to the child process. */ env?: Record; + /** + * Companies this worker may act on from proactive (no-invocation) worker→host + * calls — the plugin's configured companies. Seeded onto the handle at + * creation, BEFORE the child process spawns, so a proactive plugin that + * issues host calls during setup() (e.g. the chat gateway's one-shot + * `events.subscribe`, which runs while `startWorker` is still awaiting the + * initialize response) is already authorized when those calls arrive. The set + * can still be replaced at runtime via `setProactiveCompanyScopes` (e.g. on a + * config change). Never widens access beyond the listed companies (LOOA-695). + */ + proactiveCompanyScopes?: readonly string[]; /** * Callback for stream notifications from the worker (streams.open/emit/close). * The host wires this to the PluginStreamBus to fan out events to SSE clients. @@ -420,7 +431,18 @@ export function createPluginWorkerHandle( // A no-invocation call that references one of these companies resolves to // that company's scope; a call referencing any other company stays denied, // and in-invocation calls keep their strict single-company match. + // + // Seeded from options at handle creation — before the child process is + // spawned — so a proactive plugin's setup()-time host calls (which land while + // `startWorker` is still awaiting initialize) are authorized in time. The + // loader used to call setProactiveCompanyScopes only AFTER startWorker + // resolved, which was too late for the gateway's one-shot events.subscribe + // and left outbound push permanently dead (LOOA-695). const proactiveCompanyScopes = new Set(); + for (const id of options.proactiveCompanyScopes ?? []) { + const trimmed = readNonEmptyString(id); + if (trimmed) proactiveCompanyScopes.add(trimmed); + } // Optional methods reported by the worker during initialization let supportedMethods: string[] = []; @@ -584,21 +606,37 @@ export function createPluginWorkerHandle( } /** - * Extract the company a worker→host call references, mirroring the SDK + * Extract the single company a worker→host call references, mirroring the SDK * governed-access gate's own derivation (host-client-factory.ts - * `requestedCompanyScope`): an explicit `companyId`, or a company-scoped - * state key (`scopeKind: "company"` + `scopeId`). Returns null when the call - * references no specific company (e.g. `companies.list`, instance-scoped - * state), so proactive resolution only ever grants a single, explicit - * company — never a wildcard. + * `requestedCompanyScope`) so a proactive call resolves to exactly the company + * the gate would require: + * - explicit `params.companyId`; + * - a company-scoped state key (`scopeKind: "company"` + `scopeId`); + * - `events.subscribe`'s `params.filter.companyId` (how the SDK's + * `ctx.events.on(name, { companyId }, fn)` issues its subscribe). + * + * Returns null whenever the gate treats the call as a wildcard (`companies.list`, + * a `scopeKind: "company"` key with no `scopeId`) or as referencing no company + * (instance-scoped state, an unfiltered subscribe). A wildcard is deliberately + * NOT granted proactively: proactive resolution only ever admits a single, + * explicit company, never "all". This keeps the resolver and the gate in + * lockstep in the functional direction (LOOA-693 AC#4 / LOOA-695). */ - function referencedCompanyId(params: unknown): string | null { + function referencedCompanyId(method: string, params: unknown): string | null { + // Gate returns { kind: "all" } for companies.list regardless of params — + // never a single company — so proactive access declines it here. + if (method === "companies.list") return null; if (!isRecord(params)) return null; const direct = readNonEmptyString(params.companyId); if (direct) return direct; if (params.scopeKind === "company") { + // scopeId present → that company; absent → wildcard ("all") in the gate, + // which we never grant proactively → null. return readNonEmptyString(params.scopeId); } + if (method === "events.subscribe" && isRecord(params.filter)) { + return readNonEmptyString(params.filter.companyId); + } return null; } @@ -615,6 +653,7 @@ export function createPluginWorkerHandle( // applies when the worker is NOT inside a host-issued invocation (which // would carry an id and keep its strict single-company match below). const proactiveCompanyId = referencedCompanyId( + message.method, (message as { params?: unknown }).params, ); if (proactiveCompanyId && proactiveCompanyScopes.has(proactiveCompanyId)) {