fix(plugin-loader): deliver stored config to freshly-started plugin workers (#10092)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - One capability is first-party **plugins** that run as isolated workers spawned by the host `plugin-loader`, reading company-scoped config through a governed `ctx.config.get(companyId)` channel. > - A **proactive** plugin (e.g. a chat gateway that opens a Slack Socket Mode connection at startup) does its company work from `setup()`, where there is **no company-scoped invocation** — so `ctx.config.get()` is rejected with `company context is required`. > - The worker swallows that error and falls back to its default (feature-off) config, so the plugin comes up **inert** even though correct config exists in the database. > - This is a regression from #9557 ("governed access contracts"), which changed `plugin-loader.ts` `activatePlugin` from loading stored config into the worker bootstrap to `const config = {}`. > - This pull request replays each configured company's stored config to the freshly-started worker over the **same `configChanged` host→worker path an operator config-save already uses**. > - The benefit is that proactive plugins receive their config on worker start (both server boot and operator enable) without weakening the governed-access surface. ## Linked Issues or Issue Description No public GitHub issue — describing in-PR (bug): **Bug.** After a proactive plugin's worker spawns, it never receives its stored config. Governed access (`packages/plugins/sdk/src/host-client-factory.ts`) only resolves `config.get` inside a company-scoped invocation (event/action/tool, or explicit `params.companyId`). Proactive plugins operate from `setup()` where no such scope exists, so `config.get()` fails with `company context is required`, the worker falls back to defaults, and the feature stays disabled despite valid DB config. - Regression introduced by #9557. - Related follow-up (latent multi-company hardening): #10096. ## What Changed - `plugin-registry.ts`: add read-only `listConfigs(pluginId)` returning all stored company config rows for a plugin (scoped `where eq(pluginConfig.pluginId, pluginId)`). - `plugin-loader.ts`: after the worker starts in `activatePlugin`, replay each company's stored config through the existing `configChanged` host→worker RPC — one `{ config, companyId }` per row, the same payload shape as the operator config-save path in `routes/plugins.ts`. Best-effort and idempotent; covers both server-boot `loadAll` and operator enable. - test: DB-backed `plugin-config-startup-delivery.test.ts` covering `registry.listConfigs` completeness and cross-plugin isolation. ## Verification - `tsc --noEmit` on `@paperclipai/server` — clean. - New `plugin-config-startup-delivery.test.ts` (embedded-postgres, 3 cases) — pass. - Full PR CI green: typecheck, all server/e2e/serialized test shards, build, canary dry-run, verify, and the security scanners (Snyk, Socket, Superagent, Greptile). ## Risks - **Low functional risk.** Adds an outbound host→worker push that mirrors the already-shipped operator-save path. A worker without an `onConfigChanged` handler (or momentarily unavailable) simply keeps the runtime `ctx.config.get(companyId)` model. - **Startup fan-out.** One `configChanged` per configured company at activation (sequential, default RPC timeout). `plugin_config` rows are writable only by instance-admins, so fan-out size is operator-controlled — not a remote surface. - **No secret-handling change.** `configJson` is delivered as-is, exactly as `config.get`/operator-save already deliver it. No new secret sink; catch-blocks log only ids + `err.message` at debug, never `configJson`. - **Latent multi-company behavior (pre-existing, not introduced here).** The worker-side `configChanged` dispatch forwards only `config` (drops `companyId`), and `listConfigs` has no `ORDER BY`, so a plugin configured for **more than one** company would apply a nondeterministic last-write-wins global config. This is existing SDK behavior — operator-save already pushes into the same handler — and is **not reachable by the single-company consumer this fix targets**. Greptile flagged this shape (4/5). It is tracked and fixed as a separate, non-blocking hardening PR (#10096): thread `companyId` through `onConfigChanged`, deterministic ordering, bounded fan-out. ## Model Used Claude — Anthropic `claude-opus-4-8` (Opus 4.8), extended thinking, with tool use / code execution via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes:` / `Closes` / `Refs` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change and contains no internal Paperclip ticket id — branch predates this rule; not renaming an open PR mid-review - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes — no doc surface; internal SDK/host behavior only - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups — 4/5; two latent multi-company items triaged as non-blocking and fixed in follow-up #10096 (see Risks) - [x] I will address all Greptile and reviewer comments before requesting merge — addressed: triaged as non-blocking follow-up in #10096 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Michael Nguyen
GitHub
parent
e1cc63328a
commit
1ebf5254b6
|
|
@ -0,0 +1,136 @@
|
|||
import { randomUUID } from "node:crypto";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import { companies, createDb, pluginConfig, plugins } from "@paperclipai/db";
|
||||
import { pluginRegistryService } from "../services/plugin-registry.js";
|
||||
import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "./helpers/embedded-postgres.js";
|
||||
|
||||
/**
|
||||
* LOOA-629: a plugin worker is spawned once per plugin (not per company) with
|
||||
* an empty bootstrap config, and can only read company-scoped config from
|
||||
* inside a company-scoped invocation. A proactive plugin (e.g. the chat
|
||||
* gateway) has no such invocation at setup(), so the loader must replay every
|
||||
* configured company's config to the freshly-started worker. That replay reads
|
||||
* the config rows via `registry.listConfigs(pluginId)`, which this exercises.
|
||||
*/
|
||||
|
||||
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
||||
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
||||
|
||||
if (!embeddedPostgresSupport.supported) {
|
||||
console.warn(
|
||||
`Skipping plugin config startup-delivery tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
||||
);
|
||||
}
|
||||
|
||||
function issuePrefix(id: string) {
|
||||
return `T${id.replace(/-/g, "").slice(0, 6).toUpperCase()}`;
|
||||
}
|
||||
|
||||
describeEmbeddedPostgres("registry.listConfigs (startup config delivery)", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
|
||||
beforeAll(async () => {
|
||||
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-plugin-config-delivery-");
|
||||
db = createDb(tempDb.connectionString);
|
||||
}, 20_000);
|
||||
|
||||
afterEach(async () => {
|
||||
await db.delete(pluginConfig);
|
||||
await db.delete(plugins);
|
||||
await db.delete(companies);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await tempDb?.cleanup();
|
||||
});
|
||||
|
||||
async function seedPlugin(pluginKey: string, installOrder: number) {
|
||||
const pluginId = randomUUID();
|
||||
await db.insert(plugins).values({
|
||||
id: pluginId,
|
||||
pluginKey,
|
||||
packageName: `@paperclipai/${pluginKey}`,
|
||||
version: "0.0.1",
|
||||
apiVersion: 1,
|
||||
categories: ["automation"],
|
||||
manifestJson: {
|
||||
id: pluginKey,
|
||||
apiVersion: 1,
|
||||
version: "0.0.1",
|
||||
displayName: pluginKey,
|
||||
description: "Test plugin",
|
||||
author: "Paperclip",
|
||||
categories: ["automation"],
|
||||
capabilities: [],
|
||||
entrypoints: { worker: "./dist/worker.js" },
|
||||
},
|
||||
status: "ready",
|
||||
installOrder,
|
||||
});
|
||||
return pluginId;
|
||||
}
|
||||
|
||||
async function seedCompany() {
|
||||
const companyId = randomUUID();
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: `Co ${companyId.slice(0, 6)}`,
|
||||
issuePrefix: issuePrefix(companyId),
|
||||
});
|
||||
return companyId;
|
||||
}
|
||||
|
||||
it("returns every company-scoped config row for a plugin", async () => {
|
||||
const registry = pluginRegistryService(db);
|
||||
const pluginId = await seedPlugin("paperclip.gateway-test", 1);
|
||||
const companyA = await seedCompany();
|
||||
const companyB = await seedCompany();
|
||||
|
||||
await registry.upsertConfig(pluginId, companyA, {
|
||||
companyId: companyA,
|
||||
configJson: { slackBotToken: "xoxb-a", slackAppToken: "xapp-a" },
|
||||
});
|
||||
await registry.upsertConfig(pluginId, companyB, {
|
||||
companyId: companyB,
|
||||
configJson: { slackBotToken: "xoxb-b", slackAppToken: "xapp-b" },
|
||||
});
|
||||
|
||||
const rows = await registry.listConfigs(pluginId);
|
||||
expect(rows).toHaveLength(2);
|
||||
|
||||
const byCompany = new Map(rows.map((r) => [r.companyId, r]));
|
||||
expect(byCompany.get(companyA)?.configJson).toMatchObject({ slackBotToken: "xoxb-a" });
|
||||
expect(byCompany.get(companyB)?.configJson).toMatchObject({ slackBotToken: "xoxb-b" });
|
||||
});
|
||||
|
||||
it("only returns rows for the requested plugin (no cross-plugin bleed)", async () => {
|
||||
const registry = pluginRegistryService(db);
|
||||
const pluginId = await seedPlugin("paperclip.gateway-test", 1);
|
||||
const otherPluginId = await seedPlugin("paperclip.other-test", 2);
|
||||
const companyA = await seedCompany();
|
||||
|
||||
await registry.upsertConfig(pluginId, companyA, {
|
||||
companyId: companyA,
|
||||
configJson: { marker: "mine" },
|
||||
});
|
||||
await registry.upsertConfig(otherPluginId, companyA, {
|
||||
companyId: companyA,
|
||||
configJson: { marker: "theirs" },
|
||||
});
|
||||
|
||||
const rows = await registry.listConfigs(pluginId);
|
||||
expect(rows).toHaveLength(1);
|
||||
expect(rows[0]?.configJson).toMatchObject({ marker: "mine" });
|
||||
});
|
||||
|
||||
it("returns an empty list when the plugin has no configured companies", async () => {
|
||||
const registry = pluginRegistryService(db);
|
||||
const pluginId = await seedPlugin("paperclip.gateway-test", 1);
|
||||
const rows = await registry.listConfigs(pluginId);
|
||||
expect(rows).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -2137,6 +2137,8 @@ export function pluginLoader(
|
|||
// ------------------------------------------------------------------
|
||||
// Plugin configuration is company-scoped. Workers receive an empty
|
||||
// bootstrap config and must use ctx.config.get(companyId) at runtime.
|
||||
// Stored config is delivered right after the worker starts (step 5b) via
|
||||
// the same configChanged path an operator config-save uses.
|
||||
const config: Record<string, unknown> = {};
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
|
|
@ -2169,6 +2171,52 @@ export function pluginLoader(
|
|||
"plugin-loader: worker started",
|
||||
);
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 5b. Deliver stored configuration to the freshly-started worker
|
||||
// ------------------------------------------------------------------
|
||||
// The worker is spawned with an empty bootstrap config and is expected to
|
||||
// read company-scoped config via ctx.config.get(companyId). That call
|
||||
// only resolves inside a company-scoped invocation (event/action/tool),
|
||||
// so a proactive plugin that does company work from setup() — e.g. the
|
||||
// chat gateway opening a Slack Socket Mode connection — can never read
|
||||
// its own config and comes up inert. Replay each configured company's
|
||||
// config through the same configChanged path an operator config-save
|
||||
// uses (routes/plugins.ts), so the worker receives it at startup.
|
||||
// Best-effort: a worker that doesn't implement onConfigChanged
|
||||
// (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);
|
||||
for (const row of configRows) {
|
||||
try {
|
||||
await workerManager.call(pluginId, "configChanged", {
|
||||
config: (row.configJson ?? {}) as Record<string, unknown>,
|
||||
companyId: row.companyId,
|
||||
});
|
||||
} catch (configErr) {
|
||||
log.debug(
|
||||
{
|
||||
pluginId,
|
||||
pluginKey,
|
||||
companyId: row.companyId,
|
||||
err: configErr instanceof Error ? configErr.message : String(configErr),
|
||||
},
|
||||
"plugin-loader: startup config delivery skipped for company",
|
||||
);
|
||||
}
|
||||
}
|
||||
} catch (listErr) {
|
||||
log.debug(
|
||||
{
|
||||
pluginId,
|
||||
pluginKey,
|
||||
err: listErr instanceof Error ? listErr.message : String(listErr),
|
||||
},
|
||||
"plugin-loader: could not list stored configs for startup delivery",
|
||||
);
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 6. Sync job declarations and register with scheduler
|
||||
// ------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -288,6 +288,22 @@ export function pluginRegistryService(db: Db) {
|
|||
.where(and(eq(pluginConfig.pluginId, pluginId), eq(pluginConfig.companyId, companyId)))
|
||||
.then((rows) => rows[0] ?? null),
|
||||
|
||||
/**
|
||||
* List every company-scoped configuration row for a plugin.
|
||||
*
|
||||
* Plugin config is company-scoped, but a worker is spawned once per plugin
|
||||
* (not per company). Callers such as the plugin loader use this to replay
|
||||
* each configured company's config to a freshly-started worker, so a
|
||||
* proactive plugin that never runs inside a company-scoped invocation (and
|
||||
* therefore cannot resolve `ctx.config.get(companyId)`) still receives its
|
||||
* configuration at startup.
|
||||
*/
|
||||
listConfigs: (pluginId: string) =>
|
||||
db
|
||||
.select()
|
||||
.from(pluginConfig)
|
||||
.where(eq(pluginConfig.pluginId, pluginId)),
|
||||
|
||||
/**
|
||||
* Create or fully replace a plugin's company-scoped configuration.
|
||||
* If a config row already exists for the plugin/company pair it is replaced;
|
||||
|
|
|
|||
Loading…
Reference in New Issue