Make onSkip required, delete the raw-stderr default entirely

Three review rounds each turned up one more expandPluginPath call
site writing raw stderr mid-frame: the default onSkip was an optional
parameter with a silent, harmful fallback, so every call site was a
fresh chance to get it wrong and nothing caught it.

Delete defaultExpandSkip and make onSkip a required field on
ExpandPluginPathOptions. This turns every call site into a compile
error until it picks a handler on purpose, which is how this round
found the fourth instance: expandExistingPluginMembers called
expandPluginPath with no onSkip, reachable from runner.ts's startup
trust-migration path seven lines before the discovery call already
fixed, and from exec/runner.ts where raw stderr is legitimate.

expandExistingPluginMembers now takes a required onSkip too, so a
skipped member's reason still reaches diagnostics even though the
member itself is correctly dropped from the returned list (trust
decisions must not pre-grant a directory that could appear later).
Interactive callers pass expandSkipDiagnosticsHandler; exec passes an
explicit stderr writer built from the newly exported formatExpandSkip,
making that choice visible at its own call site instead of an
invisible library default.

expandSkipDiagnosticsHandler is now the ordinary handler (its
diagnostics parameter is required, no more undefined fallback), and
every internal call site that used to spread a conditional { onSkip }
object now always passes one.

Updated two pre-existing tests that asserted the old stderr-default
behavior to instead assert onSkip is required and receives every
skip, and added coverage for a skip reached through
expandExistingPluginMembers.

Author: TheGreatAxios

src/exec/runner.ts

@@ -54,7 +54,11 @@ import type {
 } from "../permission/types.js";
 import { createAgentToolset, type AgentToolset, type OperatorResult } from "../agent/tools.js";
 import { collectToolPlugins, resolveToolPlugins } from "../plugins/tool-plugins.js";
-import { expandExistingPluginMembers } from "../plugins/loader.js";
+import {
+  expandExistingPluginMembers,
+  formatExpandSkip,
+  type ExpandPluginPathSkip,
+} from "../plugins/loader.js";
 import { isPluginTrusted, loadProjectTrust } from "../trust/project-trust.js";
 import {
   isPathPluginTrusted,
@@ -233,9 +237,15 @@ export async function runExec(config: Config): Promise<ExecResult> {
     let projectTrust = await loadProjectTrust(config.cwd);
     const isProjectPluginTrusted = (pluginPath: string) => isPluginTrusted(projectTrust, pluginPath);
     // One-shot migration only when the path-trust file does not exist yet.
+    // Headless exec has no frame to corrupt, so a skipped marketplace member
+    // writes straight to stderr here — an explicit choice at this call site,
+    // not `expandPluginPath` falling back to it on its own.
     let pathTrust = await migratePathTrustFromPluginPaths(
       config.settings?.pluginPaths ?? [],
-      (p) => expandExistingPluginMembers(p, config.cwd),
+      (p) =>
+        expandExistingPluginMembers(p, config.cwd, (skip: ExpandPluginPathSkip) => {
+          process.stderr.write(`plugins: ${formatExpandSkip(skip)}\n`);
+        }),
       undefined,
       { onMigrated: reportPathTrustMigration },
     );

src/plugins/loader.ts

@@ -255,39 +255,55 @@ export type ExpandPluginPathOptions = {
    * tree is allowed — multi-level relatives ok).
    */
   containRoot?: string;
-  /** Called for each skipped marketplace source (never silent). */
-  onSkip?: (skip: ExpandPluginPathSkip) => void;
+  /**
+   * Called for each skipped marketplace source (never silent). Required —
+   * not optional with a stderr default — because an optional sink with a
+   * silent fallback is exactly the shape that let three review rounds each
+   * turn up one more call site writing raw stderr mid-frame in the
+   * interactive TUI (CL-5411). Making it required turns every call site
+   * into a compile error until it picks a handler on purpose:
+   * `expandSkipDiagnosticsHandler(diagnostics)` for a batching caller,
+   * an explicit stderr writer for a headless caller where that is correct
+   * and visible (see `src/exec/runner.ts`), or `() => {}` to state on the
+   * record that a caller is deliberately ignoring skips.
+   */
+  onSkip: (skip: ExpandPluginPathSkip) => void;
 };
 
-/** Default skip reporter: stderr, same shape as Claude discovery. */
-function defaultExpandSkip(skip: ExpandPluginPathSkip): void {
-  const where = skip.resolved !== undefined ? ` → ${skip.resolved}` : "";
-  process.stderr.write(
-    `plugins: skipped marketplace source ${JSON.stringify(skip.source)} (${skip.reason})${where}\n`,
-  );
-}
-
-function formatExpandSkip(skip: ExpandPluginPathSkip): string {
+/** One-line description of a skip, shared by every `onSkip` sink (diagnostics or stderr). */
+export function formatExpandSkip(skip: ExpandPluginPathSkip): string {
   const where = skip.resolved !== undefined ? ` → ${skip.resolved}` : "";
   return `marketplace source ${JSON.stringify(skip.source)} skipped (${skip.reason})${where}`;
 }
 
 /**
- * Build an `onSkip` handler that collects into `diagnostics` instead of
- * `expandPluginPath`'s raw-stderr default. Every discovery path that already
- * threads a diagnostics collector through must pass this, or a skipped
- * marketplace member bypasses the collector entirely — not merely misrouted,
- * dropped, since nothing else observes `defaultExpandSkip`'s stderr write.
- * Returns undefined (falls back to the default) when no collector is given,
- * e.g. a direct caller with no batching in play.
+ * The ordinary `onSkip` handler: collect into `diagnostics` instead of
+ * writing raw stderr, so a skipped marketplace member lands in the same
+ * end-of-batch summary as every other plugin-load warning.
  */
 export function expandSkipDiagnosticsHandler(
-  diagnostics?: PluginLoadDiagnostics,
-): ((skip: ExpandPluginPathSkip) => void) | undefined {
-  if (diagnostics === undefined) return undefined;
+  diagnostics: PluginLoadDiagnostics,
+): (skip: ExpandPluginPathSkip) => void {
   return (skip) => diagnostics.warnings.push(formatExpandSkip(skip));
 }
 
+/**
+ * `onSkip` when no diagnostics collector is in play: one explicit stderr
+ * line, module-private and only reached by an internal caller's own
+ * deliberate choice (see `resolveExpandSkip` below) — never `expandPluginPath`
+ * falling back to it on its own.
+ */
+function stderrExpandSkip(skip: ExpandPluginPathSkip): void {
+  process.stderr.write(`plugins: ${formatExpandSkip(skip)}\n`);
+}
+
+/** Diagnostics when given, else the explicit stderr line — no silent option. */
+function resolveExpandSkip(
+  diagnostics?: PluginLoadDiagnostics,
+): (skip: ExpandPluginPathSkip) => void {
+  return diagnostics !== undefined ? expandSkipDiagnosticsHandler(diagnostics) : stderrExpandSkip;
+}
+
 /**
  * Containment check with symlink safety. Lexical reject first; when both the
  * candidate and the contain root exist, realpath both and re-check so a symlink
@@ -334,11 +350,11 @@ function defaultContainRoot(marketplaceRoot: string): string {
 
 export async function expandPluginPath(
   path: string,
-  opts: ExpandPluginPathOptions = {},
+  opts: ExpandPluginPathOptions,
 ): Promise<string[]> {
   const marketplaceRoot = resolve(path);
   const report = (skip: ExpandPluginPathSkip): void => {
-    (opts.onSkip ?? defaultExpandSkip)(skip);
+    opts.onSkip(skip);
   };
 
   // 1. Declared marketplace: relative `source` list, contained under containRoot
@@ -445,13 +461,17 @@ export async function expandPluginPath(
 // Resolve a registered pluginPaths entry to the member plugin directories that
 // exist on disk: relative entries resolve against cwd, marketplace roots expand
 // to their members. Missing paths are dropped so trust decisions made from this
-// list never pre-grant a directory that could appear later with other content.
+// list never pre-grant a directory that could appear later with other content
+// — that drop is deliberate, but a *skipped* member (bad source, escape) still
+// has a reason worth reaching the caller's diagnostics, so `onSkip` is
+// required rather than silently defaulted (see `ExpandPluginPathOptions`).
 export async function expandExistingPluginMembers(
   registeredPath: string,
   cwd: string,
+  onSkip: (skip: ExpandPluginPathSkip) => void,
 ): Promise<string[]> {
   const abs = isAbsolute(registeredPath) ? registeredPath : resolve(cwd, registeredPath);
-  const members = await expandPluginPath(abs);
+  const members = await expandPluginPath(abs, { onSkip });
   const existing = await Promise.all(members.map((m) => pathExists(m)));
   return members.filter((_, i) => existing[i]);
 }
@@ -480,8 +500,7 @@ async function scanPluginsDir(
     // Each entry may itself be a marketplace, so expand before loading. A
     // skipped member routes into `diagnostics` when the caller has one, same
     // as loadPluginEntry below — otherwise it would bypass the collector.
-    const onSkip = expandSkipDiagnosticsHandler(diagnostics);
-    const dirs = await expandPluginPath(join(dir, entry), onSkip !== undefined ? { onSkip } : {});
+    const dirs = await expandPluginPath(join(dir, entry), { onSkip: resolveExpandSkip(diagnostics) });
     for (const d of dirs) {
       const abs = resolve(d);
       if (originRequiresTrust(origin) && isTrusted !== undefined && !isTrusted(abs)) {
@@ -562,11 +581,11 @@ export async function loadPluginsFromPaths(
 ): Promise<PluginModule[]> {
   // A skipped member routes into `diagnostics` when the caller has one, same
   // reasoning as scanPluginsDir — otherwise it bypasses the collector.
-  const onSkip = expandSkipDiagnosticsHandler(opts.diagnostics);
+  const onSkip = resolveExpandSkip(opts.diagnostics);
   const resolved = await Promise.all(
     paths.map(async (p) => {
       const abs = isAbsolute(p) ? p : join(cwd, p);
-      return expandPluginPath(abs, onSkip !== undefined ? { onSkip } : {});
+      return expandPluginPath(abs, { onSkip });
     }),
   );
   // Anything under <cwd>/.corbits/plugins/ is project origin no matter how it

src/tui/plugin-diagnostics-sink.test.ts

@@ -9,7 +9,9 @@ import {
 } from "../plugins/diagnostics.js";
 import {
   discoverUserPlugins,
+  expandExistingPluginMembers,
   expandPluginPath,
+  expandSkipDiagnosticsHandler,
   loadPluginEntry,
   type ExpandPluginPathSkip,
 } from "../plugins/loader.js";
@@ -167,6 +169,39 @@ describe("interactive plugin diagnostics never hit raw stderr", () => {
     expect(writes).toBe(0);
   });
 
+  test("expandExistingPluginMembers: a skipped source is dropped from the result but reaches diagnostics", async () => {
+    // This is the fourth site the same bug turned up in: it wraps
+    // expandPluginPath for a registered `pluginPaths` entry (runner.ts's
+    // startup migration/trust-seed call), so its `onSkip` is required too —
+    // there is no default to silently fall through to anymore. Dropping the
+    // member from the returned list is correct (trust decisions must not
+    // pre-grant a directory that could appear later), but the skip reason
+    // must still surface somewhere, not vanish.
+    const root = await mkdtemp(join(tmpdir(), "diag-existing-members-"));
+    const marketDir = join(root, "market");
+    await mkdir(join(marketDir, ".claude-plugin"), { recursive: true });
+    await writeFile(
+      join(marketDir, ".claude-plugin", "marketplace.json"),
+      JSON.stringify({
+        name: "demo",
+        plugins: [{ name: "bad", source: "/etc/not-a-plugin" }],
+      }),
+    );
+    const { writes } = await withStderrCapture(async () => {
+      const diag = createPluginLoadDiagnostics();
+      const members = await expandExistingPluginMembers(
+        marketDir,
+        root,
+        expandSkipDiagnosticsHandler(diag),
+      );
+      expect(members).toEqual([]);
+      const message = formatPluginWarningsSummary(diag.warnings);
+      expect(message).toBeDefined();
+      expect(diag.warnings.some((w) => w.includes("/etc/not-a-plugin"))).toBe(true);
+    });
+    expect(writes).toBe(0);
+  });
+
   test("startup agent-profile resolution: a malformed profile stays silent on stderr", async () => {
     // Same call shape as runner.ts's startup `resolveAgentPluginProfiles`
     // (over `executablePlugins()` and the full `settings.plugins` config),

src/tui/runner.ts

@@ -393,17 +393,21 @@ export async function runTUI(initialConfig: Config): Promise<number> {
   // global path-trust entry, load metadata-only (no import).
   // Claude Code marketplace installs are opt-in via settings.discoverClaudePlugins.
   let projectTrust: ProjectTrustStore = await loadProjectTrust(config.cwd);
+  // Declared before the migration call below so a skipped marketplace member
+  // (bad pluginPaths entry) collects into the same summary as discovery,
+  // rather than defaulting to stderr — `onSkip` on expandExistingPluginMembers
+  // is required precisely so this can't be forgotten at a call site.
+  const pluginLoadDiag = createPluginLoadDiagnostics();
   // One-shot: seed global path trust from pluginPaths only when the store file
   // does not exist yet (legacy per-cwd grants). Later boots load the store as-is.
   let pathTrust: PathTrustStore = await migratePathTrustFromPluginPaths(
     config.settings?.pluginPaths ?? [],
-    (p) => expandExistingPluginMembers(p, config.cwd),
+    (p) => expandExistingPluginMembers(p, config.cwd, expandSkipDiagnosticsHandler(pluginLoadDiag)),
     undefined,
     { onMigrated: reportPathTrustMigration },
   );
   const isProjectPluginTrusted = (pluginPath: string) => isPluginTrusted(projectTrust, pluginPath);
   const isRegisteredPathTrusted = (pluginPath: string) => isPathPluginTrusted(pathTrust, pluginPath);
-  const pluginLoadDiag = createPluginLoadDiagnostics();
   const pluginModules = await discoverSessionPlugins({
     cwd: config.cwd,
     ...(config.settings?.pluginPaths !== undefined
@@ -897,13 +901,11 @@ export async function runTUI(initialConfig: Config): Promise<number> {
       // Persist global path trust only once it resolves to a real plugin, so a
       // bogus path never leaves a dangling entry. Expand marketplaces so each
       // member is trusted (exact-path match on reload). `onSkip` collects into
-      // `addDiag` instead of the default stderr write — same reasoning as
+      // `addDiag` instead of a raw stderr write — same reasoning as
       // `loadPluginEntry` above.
-      const addSkipHandler = expandSkipDiagnosticsHandler(addDiag);
-      const members = await expandPluginPath(
-        abs,
-        addSkipHandler !== undefined ? { onSkip: addSkipHandler } : {},
-      );
+      const members = await expandPluginPath(abs, {
+        onSkip: expandSkipDiagnosticsHandler(addDiag),
+      });
       pathTrust = await trustPathPlugins(members.length > 0 ? members : [abs]);
       // Replace any existing descriptor/candidate with the same id so re-adding
       // refreshes rather than duplicates.

tests/unit/path-plugin-trust.test.ts

@@ -7,6 +7,7 @@ import {
   discoverUserPlugins,
   expandPluginPath,
   loadPluginsFromPaths,
+  type ExpandPluginPathSkip,
 } from "../../src/plugins/loader.js";
 import {
   isPathPluginTrusted,
@@ -17,6 +18,13 @@ import {
 } from "../../src/trust/path-trust.js";
 import { isPluginTrusted, loadProjectTrust, trustPlugin } from "../../src/trust/project-trust.js";
 
+// `onSkip` is required on `expandPluginPath` — no default sink to fall back
+// to. These fixtures expect every declared member to resolve, so a skip here
+// is a test-fixture bug; fail loudly instead of silently passing it through.
+function failOnSkip(skip: ExpandPluginPathSkip): never {
+  throw new Error(`unexpected marketplace skip: ${JSON.stringify(skip)}`);
+}
+
 async function writeCommandPlugin(dir: string, id: string, marker?: string): Promise<void> {
   await mkdir(dir, { recursive: true });
   await writeFile(
@@ -202,7 +210,7 @@ describe("path plugin trust across working directories", () => {
         "utf8",
       );
 
-      const members = await expandPluginPath(root);
+      const members = await expandPluginPath(root, { onSkip: failOnSkip });
       expect(members).toEqual([alpha, beta]);
       await trustPathPlugins(members, home);
       const pathTrust = await loadPathTrust(home);
@@ -236,7 +244,7 @@ describe("path plugin trust across working directories", () => {
         "utf8",
       );
 
-      const members = await expandPluginPath(root);
+      const members = await expandPluginPath(root, { onSkip: failOnSkip });
       expect(members).toEqual([sibling]);
       await trustPathPlugins(members, home);
       const pathTrust = await loadPathTrust(home);

tests/unit/plugin-marketplace.test.ts

@@ -178,7 +178,12 @@ test("path expand reports skips via onSkip (never silent when callback set)", as
   }
 });
 
-test("default expand path reports skips to stderr (non-silent without onSkip)", async () => {
+test("onSkip is required — every skip reaches the caller's handler, none silent", async () => {
+  // `expandPluginPath` has no default sink: `onSkip` is a required field on
+  // its options (CL-5411 round 4) so a caller cannot forget it and fall
+  // through to a raw stderr write. This drives the real function with an
+  // explicit collecting handler and confirms every skip reaches it — stderr
+  // stays untouched, since there is no implicit fallback left to reach it.
   const base = await mkdtemp(join(tmpdir(), "corbits-mkt-stderr-"));
   try {
     const root = join(base, "marketplace");
@@ -198,14 +203,13 @@ test("default expand path reports skips to stderr (non-silent without onSkip)",
       writes.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8"));
       return true;
     }) as typeof process.stderr.write;
+    const skips: ExpandPluginPathSkip[] = [];
     try {
-      const members = await expandPluginPath(root);
+      const members = await expandPluginPath(root, { onSkip: (s) => skips.push(s) });
       expect(members).toEqual([]);
-      expect(writes.some((w) => w.includes("skipped marketplace source"))).toBe(true);
-      expect(writes.some((w) => w.includes("absolute"))).toBe(true);
-      expect(writes.some((w) => w.includes("missing"))).toBe(true);
-      // Default reporter uses the original relative source string for missing.
-      expect(writes.some((w) => w.includes("./plugins/missing"))).toBe(true);
+      expect(writes).toEqual([]);
+      expect(skips.some((s) => s.reason === "absolute" && s.source === "/tmp/not-a-plugin")).toBe(true);
+      expect(skips.some((s) => s.reason === "missing" && s.source === "./plugins/missing")).toBe(true);
     } finally {
       process.stderr.write = origWrite;
     }