Merge pull request #369 from corbitsdev/cl-5411-5409-plugin-paths

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/admin.ts

@@ -26,7 +26,10 @@ export type PluginsAdmin = {
   list: () => PluginDescriptor[];
   getConfig: () => Record<string, PluginConfig>;
   getWebOverride: () => string | undefined;
-  saveConfig: (id: string, cfg: PluginConfig) => Promise<void> | void;
+  // A trust-grant load (enabling a metadata-only plugin) can surface skill-miss
+  // and similar warnings; the optional message lets the caller show them
+  // instead of dropping them on the floor.
+  saveConfig: (id: string, cfg: PluginConfig) => Promise<{ message?: string } | void> | void;
   setWebOverride: (id: string | undefined) => Promise<void> | void;
   verify: (id: string, credentials: Record<string, string>) => Promise<VerifyResult>;
   // Register a plugin from an arbitrary file/dir path, persisting it so it loads

src/plugins/data-only.ts

@@ -7,6 +7,7 @@ import { loadDataOnlyCommands } from "./data-only-commands.js";
 import { loadSkillCommands } from "./skill-commands.js";
 import {
   resolvePluginWarningHandler,
+  stderrPluginWarning,
   type PluginLoadDiagnostics,
 } from "./diagnostics.js";
 
@@ -80,8 +81,14 @@ export async function loadDataOnlyPlugin(
   } = {},
 ): Promise<DataOnlyPlugin | null> {
   const cwd = opts.cwd ?? process.cwd();
-  // Prefer diagnostics collector; else explicit onWarning; else stderr default.
-  const onWarning = resolvePluginWarningHandler(opts);
+  // Prefer diagnostics collector; else explicit onWarning; else stderrPluginWarning.
+  const onWarning = resolvePluginWarningHandler(
+    opts.diagnostics !== undefined
+      ? { diagnostics: opts.diagnostics }
+      : opts.onWarning !== undefined
+        ? { onWarning: opts.onWarning }
+        : { onWarning: stderrPluginWarning },
+  );
 
   const [nativeManifest, claudeManifest, agents, commands, skillCmds] = await Promise.all([
     readManifestJson(pluginDir),

src/plugins/diagnostics.test.ts

@@ -4,6 +4,7 @@ import { join } from "node:path";
 import { tmpdir } from "node:os";
 import {
   createPluginLoadDiagnostics,
+  emitPluginWarningLog,
   emitPluginWarningSummary,
   formatPluginWarningsSummary,
   pluginWarningSink,
@@ -71,19 +72,35 @@ describe("emitPluginWarningSummary", () => {
   });
 });
 
+describe("emitPluginWarningLog", () => {
+  test("never writes to stderr — interactive TUI holds the alt screen and a raw write corrupts the frame", () => {
+    const diag = createPluginLoadDiagnostics();
+    diag.warnings.push('agent a: skill "style" referenced but not found in skill search path');
+    const originalWrite = process.stderr.write.bind(process.stderr);
+    let stderrCalls = 0;
+    process.stderr.write = ((..._args: unknown[]) => {
+      stderrCalls++;
+      return true;
+    }) as typeof process.stderr.write;
+    try {
+      emitPluginWarningLog(diag);
+    } finally {
+      process.stderr.write = originalWrite;
+    }
+    expect(stderrCalls).toBe(0);
+  });
+});
+
 describe("plugin load diagnostics wiring", () => {
   test("collector records skill misses without calling stderr fallback", async () => {
     const dir = await makePlugin({
       "agents/a.md": "---\nskills: [nope, also-missing]\n---\nbody\n",
     });
     const diag = createPluginLoadDiagnostics();
-    const stderrLines: string[] = [];
-    const sink = pluginWarningSink(diag, (msg) => stderrLines.push(msg));
+    const sink = pluginWarningSink(diag);
 
-    // Sink itself must not hit fallback when diag is set.
     sink("should only land in diag");
     expect(diag.warnings).toEqual(["should only land in diag"]);
-    expect(stderrLines).toEqual([]);
 
     diag.warnings.length = 0;
     const mod = await loadPluginEntry(dir, {

src/plugins/diagnostics.ts

@@ -2,6 +2,11 @@
 // interactive TUI) accumulate warnings and emit a single summary instead of
 // writing one stderr line per miss mid-frame.
 
+import { getLogger } from "@intx/log";
+import { LOG_NAMESPACE_ROOT } from "../branding.js";
+
+const pluginDiagnosticsLogger = getLogger([LOG_NAMESPACE_ROOT, "plugins"]);
+
 export type PluginLoadDiagnostics = {
   warnings: string[];
 };
@@ -10,34 +15,28 @@ export function createPluginLoadDiagnostics(): PluginLoadDiagnostics {
   return { warnings: [] };
 }
 
+/** Build an onWarning callback that records into `diag`. */
+export function pluginWarningSink(diag: PluginLoadDiagnostics): (msg: string) => void {
+  return (msg) => {
+    diag.warnings.push(msg);
+  };
+}
+
 /**
- * Build an onWarning callback that records into `diag` when provided, else
- * falls back to the given sink (default: one stderr line per message).
+ * Resolve the warning sink for a load call. There is no default: callers
+ * must decide between a diagnostics collector (batched into one summary,
+ * safe mid-frame) and an explicit onWarning (e.g. a raw stderr writer for
+ * headless paths where no frame is being held).
  */
-export function pluginWarningSink(
-  diag: PluginLoadDiagnostics | undefined,
-  fallback: (msg: string) => void = (msg) => process.stderr.write(`plugins: ${msg}\n`),
+export function resolvePluginWarningHandler(
+  opts: { diagnostics: PluginLoadDiagnostics } | { onWarning: (msg: string) => void },
 ): (msg: string) => void {
-  if (diag !== undefined) {
-    return (msg) => {
-      diag.warnings.push(msg);
-    };
-  }
-  return fallback;
+  return "diagnostics" in opts ? pluginWarningSink(opts.diagnostics) : opts.onWarning;
 }
 
-/**
- * Resolve the warning sink for a load call. Prefer a diagnostics collector when
- * provided (so batch callers can emit one summary); else an explicit onWarning;
- * else one stderr line per message.
- */
-export function resolvePluginWarningHandler(opts: {
-  diagnostics?: PluginLoadDiagnostics;
-  onWarning?: (msg: string) => void;
-}): (msg: string) => void {
-  if (opts.diagnostics !== undefined) return pluginWarningSink(opts.diagnostics);
-  if (opts.onWarning !== undefined) return opts.onWarning;
-  return pluginWarningSink(undefined);
+/** Named raw-stderr choice: `{ onWarning: stderrPluginWarning }`. */
+export function stderrPluginWarning(msg: string): void {
+  process.stderr.write(`plugins: ${msg}\n`);
 }
 
 /**
@@ -84,3 +83,16 @@ export function emitPluginWarningSummary(
   const summary = formatPluginWarningsSummary(diag.warnings);
   if (summary !== undefined) write(summary);
 }
+
+/**
+ * Emit a diagnostics summary through the structured logger instead of raw
+ * stderr. Interactive callers (the TUI holds the alternate screen for the
+ * whole session) must use this, not the raw-stderr default above — a bare
+ * write lands mid-frame and corrupts the rendered transcript. The logger is
+ * already routed to `~/.corbits/logs/corbits.log` by `installFileLogSink`
+ * (first statement of `mainWithRunners`), so this reuses that sink rather
+ * than adding a second suppression path.
+ */
+export function emitPluginWarningLog(diag: PluginLoadDiagnostics): void {
+  emitPluginWarningSummary(diag, (line) => pluginDiagnosticsLogger.warn(line));
+}

src/plugins/loader.ts

@@ -10,6 +10,7 @@ import { parsePluginManifest, type PluginManifest } from "./manifest.js";
 import { loadDataOnlyPlugin } from "./data-only.js";
 import {
   resolvePluginWarningHandler,
+  stderrPluginWarning,
   type PluginLoadDiagnostics,
 } from "./diagnostics.js";
 import {
@@ -116,9 +117,15 @@ export async function loadPluginEntry(
   } = {},
 ): Promise<PluginModule | null> {
   const cwd = opts.cwd ?? process.cwd();
-  // Prefer diagnostics collector when provided so batch discovery can summarize;
-  // explicit onWarning is for tests / one-off sinks; default is stderr per line.
-  const onWarning = resolvePluginWarningHandler(opts);
+  // Prefer diagnostics collector so batch discovery can summarize; explicit
+  // onWarning for tests; else stderrPluginWarning.
+  const onWarning = resolvePluginWarningHandler(
+    opts.diagnostics !== undefined
+      ? { diagnostics: opts.diagnostics }
+      : opts.onWarning !== undefined
+        ? { onWarning: opts.onWarning }
+        : { onWarning: stderrPluginWarning },
+  );
   const origin = opts.origin;
   let target = entryPath;
   let pluginDir = entryPath;
@@ -255,16 +262,53 @@ 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 {
+/** 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}` : "";
-  process.stderr.write(
-    `plugins: skipped marketplace source ${JSON.stringify(skip.source)} (${skip.reason})${where}\n`,
-  );
+  return `marketplace source ${JSON.stringify(skip.source)} skipped (${skip.reason})${where}`;
+}
+
+/**
+ * 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 {
+  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;
 }
 
 /**
@@ -313,11 +357,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
@@ -424,13 +468,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]);
 }
@@ -456,8 +504,10 @@ async function scanPluginsDir(
 
   const results: PluginModule[] = [];
   for (const entry of entries) {
-    // Each entry may itself be a marketplace, so expand before loading.
-    const dirs = await expandPluginPath(join(dir, entry));
+    // 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 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)) {
@@ -536,10 +586,13 @@ export async function loadPluginsFromPaths(
     diagnostics?: PluginLoadDiagnostics;
   } = {},
 ): Promise<PluginModule[]> {
+  // A skipped member routes into `diagnostics` when the caller has one, same
+  // reasoning as scanPluginsDir — otherwise it bypasses the collector.
+  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);
+      return expandPluginPath(abs, { onSkip });
     }),
   );
   // Anything under <cwd>/.corbits/plugins/ is project origin no matter how it
@@ -611,12 +664,12 @@ export async function discoverClaudeInstalledPlugins(
   try {
     parsed = JSON.parse(raw);
   } catch {
-    // Prefer collector when present; else one stderr line (default sink).
+    // Prefer collector when present; else stderrPluginWarning.
     resolvePluginWarningHandler(
-      opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
-    )(
-      `failed to parse ${registryPath}`,
-    );
+      opts.diagnostics !== undefined
+        ? { diagnostics: opts.diagnostics }
+        : { onWarning: stderrPluginWarning },
+    )(`failed to parse ${registryPath}`);
     return [];
   }
   if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
@@ -628,10 +681,12 @@ export async function discoverClaudeInstalledPlugins(
   }
 
   const pluginsRoot = resolve(home, ".claude", "plugins");
-  // Default expand-skip sink respects diagnostics when provided so discovery
-  // can emit one summary; explicit onExpandSkip (tests) still wins.
+  // Default expand-skip sink: diagnostics when provided, else
+  // stderrPluginWarning; explicit onExpandSkip (tests) still wins.
   const warnExpand = resolvePluginWarningHandler(
-    opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
+    opts.diagnostics !== undefined
+      ? { diagnostics: opts.diagnostics }
+      : { onWarning: stderrPluginWarning },
   );
   const onExpandSkip =
     opts.onExpandSkip