fix(plugins): 5 bugs on the plugin path (3 Windows-only, 2 all-platform)

open-sse/handlers/chatCore.ts

@@ -477,6 +477,12 @@ export async function handleChatCore({
     return {
       success: false,
       status: 403,
+      // Label the source: this 403 is our own policy decision, not the provider
+      // rejecting us. Unlabelled, it is indistinguishable from a real upstream 403
+      // and gets the connection banned. Matches the type already sent to the client
+      // in pluginOnRequest.ts.
+      errorType: "plugin_block",
+      errorCode: "plugin_block",
       error: "Request blocked by plugin",
       response: pluginGate.response,
     };

open-sse/services/combo/comboPredicates.ts

@@ -182,8 +182,16 @@ export function isRequestScopedUpstreamFailure(error?: {
  * #7177: whether handleSingleModelChat should skip the connection-level cooldown
  * (markAccountUnavailable) for a failed attempt — client disconnects, a 401 when the
  * connection has extra keys to rotate through, a known request-scoped upstream failure
- * (e.g. context overflow — not a connection health signal), or our own self-inflicted
- * timeout all mean the connection itself is healthy and should not be cooled down.
+ * (e.g. context overflow — not a connection health signal), a plugin refusing the
+ * request (our own policy, not a provider fault — see below), or our own
+ * self-inflicted timeout all mean the connection itself is healthy and should not be
+ * cooled down.
+ *
+ * A plugin block (`plugin_block`) is our own policy decision, not the provider
+ * rejecting us. Banning the account here would let a working security plugin destroy
+ * the connection it protects — one block would ban the provider for every later
+ * request, valid ones included — and would trigger a pointless retry loop across other
+ * accounts the plugin would refuse identically.
  */
 export function shouldSkipConnDisable(
   result: { status: number; errorCode?: string | null; errorType?: string | null },
@@ -195,6 +203,8 @@ export function shouldSkipConnDisable(
     result.status === 499 ||
     result.errorCode === "client_disconnected" ||
     result.errorType === "client_disconnected" ||
+    result.errorCode === "plugin_block" ||
+    result.errorType === "plugin_block" ||
     (is401 && hasExtraKeys) ||
     isRequestScopedUpstreamFailure({ code: result.errorCode, type: result.errorType }) ||
     isSelfInflictedUpstreamTimeout(result.status, result.errorType, provider)

src/lib/plugins/hooks.ts

@@ -260,10 +260,42 @@ export interface Plugin {
   onUninstall?: (payload: unknown) => Promise<void> | void;
 }
 
+/**
+ * Reload plugins left active in the DB, once per process.
+ *
+ * The `hooks` map above is module state and does not survive a restart, while the DB
+ * keeps status='active' — so without this every active plugin silently stops applying
+ * after a reboot while the UI still reports it as active. Hooks are fail-open, so a
+ * plugin that exists to *block* traffic fails open.
+ *
+ * This lives on the request path on purpose. Booting it from instrumentation-node.ts
+ * does not work: Next.js gives instrumentation its own module graph, so its `hooks` map
+ * is a different instance from the one route handlers read. Verified — plugins loaded
+ * there register into a copy nothing consults, and leak an unused child process.
+ *
+ * The import is dynamic because manager.ts imports this module.
+ */
+let pluginBoot: Promise<void> | null = null;
+
+function ensurePluginsLoaded(): Promise<void> {
+  if (!pluginBoot) {
+    pluginBoot = (async () => {
+      const { pluginManager } = await import("./manager");
+      await pluginManager.loadAll();
+    })().catch((err: unknown) => {
+      // Retry on the next request rather than wedging every later call.
+      pluginBoot = null;
+      log.error("hooks.boot_failed", { error: err instanceof Error ? err.message : String(err) });
+    });
+  }
+  return pluginBoot;
+}
+
 /**
  * Run onRequest hooks — blocking. Plugins can modify body/metadata or block with 403.
  */
 export async function runOnRequest(ctx: PluginContext): Promise<PluginResult> {
+  await ensurePluginsLoaded();
   return emitHookBlocking("onRequest", ctx);
 }
 

src/lib/plugins/loader.ts

@@ -44,10 +44,14 @@ export interface LoadedPlugin {
 
 const PLUGIN_HOST_SCRIPT = `
 import { createRequire } from "node:module";
+import { pathToFileURL } from "node:url";
 const require = createRequire(import.meta.url);
 
+// pathToFileURL: on Windows a bare absolute path ("C:\\\\...") makes import()
+// throw ERR_UNSUPPORTED_ESM_URL_SCHEME ("C:" is parsed as a URL scheme), so no
+// plugin could ever load. file:// URLs work on every platform.
 const pluginPath = process.argv[2];
-const plugin = await import(pluginPath);
+const plugin = await import(pathToFileURL(pluginPath).href);
 const exports = plugin.default || plugin;
 
 // Send ready signal
@@ -313,7 +317,13 @@ export async function loadPlugin(
  * Uses allowlist approach — only pass explicitly safe vars.
  */
 function getFilteredEnv(permissions: Permission[]): Record<string, string> {
-  const safeKeys = ["PATH", "HOME", "USER", "LANG", "LC_ALL", "NODE_ENV"];
+  // SystemRoot/windir are not optional on Windows: node aborts during
+  // InitializeOncePerProcessInternal ("Assertion failed: ncrypto::CSPRNG") before
+  // running any script, because its CSPRNG lives under %SystemRoot%. Without these
+  // the child dies instantly, every hook times out, and — hooks being fail-open —
+  // plugins silently stop applying. They carry no secrets.
+  const platformKeys = process.platform === "win32" ? ["SystemRoot", "windir"] : [];
+  const safeKeys = ["PATH", "HOME", "USER", "LANG", "LC_ALL", "NODE_ENV", ...platformKeys];
   const extendedSafeKeys = [...safeKeys, "PORT", "HOSTNAME", "TZ", "TMPDIR"];
   const allowedKeys = permissions.includes("env") ? extendedSafeKeys : safeKeys;
   const env: Record<string, string> = {};

src/lib/plugins/manager.ts

@@ -373,7 +373,12 @@ class PluginManager {
   async activate(name: string): Promise<void> {
     const row = getPluginByName(name);
     if (!row) throw new Error(`Plugin '${name}' not found`);
-    if (row.status === "active") return;
+    // Guard on in-memory state, not DB status. DB status survives a restart but
+    // loadedPlugins/hooks do not, so a DB-only check makes activate() a no-op for
+    // every plugin already marked active — including the loadAll() boot path, whose
+    // whole job is reloading exactly those. Hooks being fail-open, the plugin then
+    // silently enforces nothing while the UI still reports it as active.
+    if (row.status === "active" && this.loadedPlugins.has(name)) return;
 
     const manifest = JSON.parse(row.manifest) as PluginManifestWithDefaults;
 
@@ -388,7 +393,7 @@ class PluginManager {
     const resolvedEntry = await realpath(entryPoint).catch(() => null);
     if (
       !resolvedEntry ||
-      (!resolvedEntry.startsWith(resolvedPluginDir + "/") && resolvedEntry !== resolvedPluginDir)
+      (!resolvedEntry.startsWith(resolvedPluginDir + sep) && resolvedEntry !== resolvedPluginDir)
     ) {
       throw new Error(`Plugin '${name}' entry point escapes plugin directory`);
     }

tests/unit/combo-skip-conn-disable-plugin-block-7806.test.ts

@@ -0,0 +1,70 @@
+import assert from "node:assert/strict";
+import test from "node:test";
+
+/**
+ * #7806: a plugin block must not ban the provider connection.
+ *
+ * A plugin returning 403 is our own policy decision, not the provider rejecting us.
+ * Left unlabelled, chatCore's plugin-gate 403 is indistinguishable from a real
+ * upstream 403 and flows through markAccountUnavailable -> classifyProviderError(403)
+ * -> FORBIDDEN -> resolveTerminalConnectionStatus -> test_status='banned', destroying
+ * a healthy connection the plugin was protecting. `shouldSkipConnDisable` must
+ * recognize the `plugin_block` errorType/errorCode the same way it already recognizes
+ * `client_disconnected`, so the connection-level cooldown is skipped.
+ */
+
+const { shouldSkipConnDisable } =
+  await import("../../open-sse/services/combo/comboPredicates.ts");
+
+const BASE_ARGS = { is401: false, hasExtraKeys: false, provider: "test-provider" } as const;
+
+test("plugin_block errorType skips connection disable", () => {
+  assert.equal(
+    shouldSkipConnDisable(
+      { status: 403, errorType: "plugin_block" },
+      BASE_ARGS.is401,
+      BASE_ARGS.hasExtraKeys,
+      BASE_ARGS.provider
+    ),
+    true,
+    "a plugin-blocked request (errorType) must not cool down a healthy connection"
+  );
+});
+
+test("plugin_block errorCode skips connection disable", () => {
+  assert.equal(
+    shouldSkipConnDisable(
+      { status: 403, errorCode: "plugin_block" },
+      BASE_ARGS.is401,
+      BASE_ARGS.hasExtraKeys,
+      BASE_ARGS.provider
+    ),
+    true,
+    "a plugin-blocked request (errorCode) must not cool down a healthy connection"
+  );
+});
+
+test("client_disconnected still skips connection disable (existing coverage)", () => {
+  assert.equal(
+    shouldSkipConnDisable(
+      { status: 499, errorType: "client_disconnected" },
+      BASE_ARGS.is401,
+      BASE_ARGS.hasExtraKeys,
+      BASE_ARGS.provider
+    ),
+    true
+  );
+});
+
+test("an unrelated 403 (real upstream forbidden) does NOT skip connection disable", () => {
+  assert.equal(
+    shouldSkipConnDisable(
+      { status: 403, errorCode: "forbidden" },
+      BASE_ARGS.is401,
+      BASE_ARGS.hasExtraKeys,
+      BASE_ARGS.provider
+    ),
+    false,
+    "only a plugin_block-labelled 403 is self-inflicted — a real provider 403 must still cool down"
+  );
+});

tests/unit/plugins-manager-restart-reload-7806.test.ts

@@ -0,0 +1,130 @@
+import { describe, it, beforeEach } from "node:test";
+import assert from "node:assert/strict";
+import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from "node:fs";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+
+/**
+ * #7806: plugins left active in the DB never come back after a server restart.
+ *
+ * `activate()` used to early-return whenever the DB row said status==='active',
+ * without checking in-memory state. `loadedPlugins` (and the hooks map) die with the
+ * process while the DB row persists — so after a restart, `activate()` (and therefore
+ * `loadAll()`, which calls it for every active row) silently skipped the load for
+ * exactly the plugins that needed it. Hooks are fail-open, so a plugin that exists to
+ * *block* traffic would fail open forever after a restart, with the UI still reporting
+ * it as active.
+ *
+ * This simulates the post-restart state without an actual process restart: install
+ * and activate a plugin (DB row status='active', plugin present in the in-memory
+ * `loadedPlugins` map), then clear the in-memory map directly — mirroring what a
+ * fresh process boot does to module state while leaving the DB row untouched — and
+ * assert that `activate()` / `loadAll()` actually reload it rather than treating the
+ * DB's status='active' as proof it is already loaded.
+ */
+
+const mod = await import("../../src/lib/plugins/manager.ts");
+const db = await import("../../src/lib/db/plugins.ts");
+const { getDbInstance } = await import("../../src/lib/db/core.ts");
+
+function makeTmpPlugin(name: string, manifest: Record<string, unknown> = {}) {
+  const tmp = mkdtempSync(join(tmpdir(), "mgr-restart-test-"));
+  const pluginDir = join(tmp, name);
+  mkdirSync(pluginDir, { recursive: true });
+  writeFileSync(
+    join(pluginDir, "plugin.json"),
+    JSON.stringify({ name, version: "1.0.0", ...manifest })
+  );
+  writeFileSync(
+    join(pluginDir, "index.js"),
+    `module.exports = { onRequest: async (ctx) => ({ metadata: { banner: "hello" } }) };`
+  );
+  return pluginDir;
+}
+
+/** Simulate a process restart: DB row stays 'active', in-memory state is gone. */
+function simulateRestart(name: string) {
+  // loadedPlugins is private — this is exactly the module-state loss a real restart
+  // causes (a fresh process never had this entry in the first place).
+  (mod.pluginManager as unknown as { loadedPlugins: Map<string, unknown> }).loadedPlugins.delete(
+    name
+  );
+}
+
+describe("pluginManager reload after restart (#7806)", () => {
+  const testPlugins: string[] = [];
+  const tmpDirs: string[] = [];
+
+  beforeEach(() => {
+    getDbInstance();
+    for (const name of testPlugins) {
+      try {
+        db.deletePlugin(name);
+      } catch {
+        // not present — fine
+      }
+    }
+    testPlugins.length = 0;
+  });
+
+  it("activate() reloads a plugin whose DB row is 'active' but is absent from loadedPlugins", async () => {
+    const name = "restart-reload-activate-test";
+    const dir = makeTmpPlugin(name);
+    testPlugins.push(name);
+    tmpDirs.push(dir);
+    try {
+      await mod.pluginManager.install(dir);
+      await mod.pluginManager.activate(name);
+      assert.ok(mod.pluginManager.getLoaded(name), "sanity: plugin is loaded after activate()");
+
+      simulateRestart(name);
+      assert.equal(
+        mod.pluginManager.getLoaded(name),
+        undefined,
+        "sanity: in-memory state cleared, mirroring a fresh process"
+      );
+
+      const dbRow = db.getPluginByName(name);
+      assert.equal(dbRow!.status, "active", "DB row still says active across the 'restart'");
+
+      await mod.pluginManager.activate(name);
+
+      assert.ok(
+        mod.pluginManager.getLoaded(name),
+        "activate() must reload the plugin instead of treating DB status='active' as a no-op"
+      );
+    } finally {
+      // Deactivate to kill the reloaded child process — otherwise it dangles and
+      // keeps the test runner's event loop alive after the suite finishes.
+      await mod.pluginManager.deactivate(name).catch(() => {});
+      rmSync(dir.split("/").slice(0, -1).join("/"), { recursive: true, force: true });
+    }
+  });
+
+  it("loadAll() reloads active plugins missing from loadedPlugins (boot path)", async () => {
+    const name = "restart-reload-loadall-test";
+    const dir = makeTmpPlugin(name);
+    testPlugins.push(name);
+    tmpDirs.push(dir);
+    try {
+      await mod.pluginManager.install(dir);
+      await mod.pluginManager.activate(name);
+      assert.ok(mod.pluginManager.getLoaded(name), "sanity: plugin is loaded after activate()");
+
+      simulateRestart(name);
+      assert.equal(mod.pluginManager.getLoaded(name), undefined, "sanity: state cleared");
+
+      await mod.pluginManager.loadAll();
+
+      assert.ok(
+        mod.pluginManager.getLoaded(name),
+        "loadAll() must reload every DB-active plugin missing from loadedPlugins"
+      );
+    } finally {
+      // Deactivate to kill the reloaded child process — otherwise it dangles and
+      // keeps the test runner's event loop alive after the suite finishes.
+      await mod.pluginManager.deactivate(name).catch(() => {});
+      rmSync(dir.split("/").slice(0, -1).join("/"), { recursive: true, force: true });
+    }
+  });
+});