From 3847c90d90b0636f72f7feea8a781a12800a5feb Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 01:42:10 -0700 Subject: [PATCH 1/5] Route interactive plugin diagnostics through the log sink, not stderr The TUI holds the alternate screen for the whole session, so a raw process.stderr.write mid-frame corrupts the rendered transcript. Four call sites in runner.ts (initial discovery, trust-grant, verify, agent-profile resolution) called emitPluginWarningSummary with no sink argument, which defaults to a bare stderr write. addPath was already patched to fold its warnings into the UI result message, and exec/runner.ts already routes through the structured logger, but the other four spots still hit the raw default. Add emitPluginWarningLog, which reuses the already-installed @intx/log file sink (~/.corbits/logs/corbits.log) instead of adding a second suppression mechanism, and switch all four call sites to it. --- src/plugins/diagnostics.test.ts | 20 ++++++++++++++++++++ src/plugins/diagnostics.ts | 18 ++++++++++++++++++ src/tui/plugin-diagnostics-sink.test.ts | 17 +++++++++++++++++ src/tui/runner.ts | 10 +++++----- 4 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 src/tui/plugin-diagnostics-sink.test.ts diff --git a/src/plugins/diagnostics.test.ts b/src/plugins/diagnostics.test.ts index 0e1b10687..09ed23a7e 100644 --- a/src/plugins/diagnostics.test.ts +++ b/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,6 +72,25 @@ 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({ diff --git a/src/plugins/diagnostics.ts b/src/plugins/diagnostics.ts index 26739f804..2b2cf24ed 100644 --- a/src/plugins/diagnostics.ts +++ b/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[]; }; @@ -84,3 +89,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)); +} diff --git a/src/tui/plugin-diagnostics-sink.test.ts b/src/tui/plugin-diagnostics-sink.test.ts new file mode 100644 index 000000000..e58af4adc --- /dev/null +++ b/src/tui/plugin-diagnostics-sink.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, test } from "bun:test"; +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; + +// The TUI holds the alternate screen for the whole interactive session, so any +// plugin-diagnostics summary that lands on raw stderr corrupts the rendered +// frame instead of showing up as a single controlled line (see CL-5411). +// `emitPluginWarningSummary` defaults to a raw `process.stderr.write` sink +// when called with no second argument; interactive callers must route through +// `emitPluginWarningLog` (the structured-logger sink) instead. +describe("runner.ts plugin diagnostics", () => { + test("never calls emitPluginWarningSummary with its raw-stderr default", async () => { + const src = await readFile(join(import.meta.dir, "runner.ts"), "utf8"); + const bareCalls = src.match(/emitPluginWarningSummary\([^,)]+\)/g) ?? []; + expect(bareCalls).toEqual([]); + }); +}); diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 38b0bb26c..489bb4ec5 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -50,7 +50,7 @@ import { refreshCodexInstructions } from "../auth/codex/instructions.js"; import { expandExistingPluginMembers, expandPluginPath, loadPluginEntry, type PluginOrigin } from "../plugins/loader.js"; import { createPluginLoadDiagnostics, - emitPluginWarningSummary, + emitPluginWarningLog, formatPluginWarningsSummary, } from "../plugins/diagnostics.js"; import { @@ -410,7 +410,7 @@ export async function runTUI(initialConfig: Config): Promise { isRegisteredPathTrusted, diagnostics: pluginLoadDiag, }); - emitPluginWarningSummary(pluginLoadDiag); + emitPluginWarningLog(pluginLoadDiag); // Mutable list so trusting a project/path plugin can replace a metadata-only stub // with a fully loaded module without restarting the process. let livePluginModules = pluginModules; @@ -766,7 +766,7 @@ export async function runTUI(initialConfig: Config): Promise { origin: stub.origin, diagnostics: trustDiag, }); - emitPluginWarningSummary(trustDiag); + emitPluginWarningLog(trustDiag); if (full !== null) { livePluginModules = livePluginModules.map((m) => m.manifest?.id === id ? full : m, @@ -813,7 +813,7 @@ export async function runTUI(initialConfig: Config): Promise { { [id]: { enabled: true } }, { diagnostics: verifyDiag }, ); - emitPluginWarningSummary(verifyDiag); + emitPluginWarningLog(verifyDiag); if (profiles.length === 0) return { ok: false, message: "No valid agent profiles found" }; return { ok: true, message: `loaded — ${profiles.length} profile${profiles.length === 1 ? "" : "s"}` }; } @@ -946,7 +946,7 @@ export async function runTUI(initialConfig: Config): Promise { config.settings?.plugins ?? {}, { diagnostics: profileDiag }, ); - emitPluginWarningSummary(profileDiag); + emitPluginWarningLog(profileDiag); const initialProfiles = await loadAgentProfiles(profilesDir, pluginAgentProfiles); let liveAgentProfiles = initialProfiles; From 38ce4964244c31b422f76a14c481bcee4706a750 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 02:15:23 -0700 Subject: [PATCH 2/5] Catch the remaining raw-stderr sites, and stop silencing warnings Two more plugin-diagnostics call sites reachable from the interactive TUI still hit a raw stderr write, corrupting the alt-screen frame the same way the first four did: resolveToolPlugins (a throwing tool- plugin factory) and expandPluginPath's default onSkip (a skipped marketplace member from add-by-path). Both now collect into a diagnostics object instead. The previous fix also introduced a regression: three of the four sites routed their warnings to the log file only, and nothing in the TUI reads that file, so a broken skill ref became invisible instead of loud. Apply the addPath pattern (already in this diff) to the other three: verify and the trust-grant path now fold warnings into their existing message/notify channel, and the fire-and-forget startup paths (discovery, tool-plugin resolution) queue a transcript row shown once the shell mounts instead of dropping silently. Replaced the source-grep regression test with a behavioral one that drives the five real code paths (discovery, trust-grant, verify, add-path, tool-resolve) with process.stderr.write instrumented, since a text scan of one function name in one file could never have caught either of the two sites above. --- src/plugins/admin.ts | 5 +- src/plugins/tool-plugins.ts | 18 ++- src/tui-opentui/command-surfaces.ts | 17 ++- src/tui/plugin-diagnostics-sink.test.ts | 162 ++++++++++++++++++++++-- src/tui/runner.ts | 56 +++++++- 5 files changed, 234 insertions(+), 24 deletions(-) diff --git a/src/plugins/admin.ts b/src/plugins/admin.ts index 88a0c4f95..5eec4abde 100644 --- a/src/plugins/admin.ts +++ b/src/plugins/admin.ts @@ -26,7 +26,10 @@ export type PluginsAdmin = { list: () => PluginDescriptor[]; getConfig: () => Record; getWebOverride: () => string | undefined; - saveConfig: (id: string, cfg: PluginConfig) => Promise | 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; verify: (id: string, credentials: Record) => Promise; // Register a plugin from an arbitrary file/dir path, persisting it so it loads diff --git a/src/plugins/tool-plugins.ts b/src/plugins/tool-plugins.ts index bea43a4e3..174b42f89 100644 --- a/src/plugins/tool-plugins.ts +++ b/src/plugins/tool-plugins.ts @@ -3,6 +3,10 @@ import type { PluginModule } from "./loader.js"; import type { PluginConfig } from "../config/settings.js"; import type { PluginCredentialField } from "./manifest.js"; import { scrubSecrets } from "../web/secret-scrub.js"; +import { + resolvePluginWarningHandler, + type PluginLoadDiagnostics, +} from "./diagnostics.js"; // A discovered plugin that contributes agent tools: a "tool"-kind manifest plus // the factory the loader captured. @@ -37,18 +41,28 @@ export function isToolPluginActive(config: Record, id: str } // Instantiate every enabled+consented tool plugin. A factory that throws is -// logged and skipped rather than aborting the run. +// reported and skipped rather than aborting the run. Pass `diagnostics` from +// an interactive caller (the TUI holds the alternate screen for the whole +// session — a bare stderr write mid-frame corrupts it); without it this falls +// back to one stderr line per failure, same as `resolvePluginWarningHandler` +// elsewhere in the plugin loader. export async function resolveToolPlugins(args: { candidates: ToolPluginCandidate[]; pluginConfig: Record; + diagnostics?: PluginLoadDiagnostics; }): Promise { + const onWarning = resolvePluginWarningHandler( + args.diagnostics === undefined ? {} : { diagnostics: args.diagnostics }, + ); const out: ToolPlugin[] = []; for (const cand of args.candidates) { if (!isToolPluginActive(args.pluginConfig, cand.id)) continue; try { out.push(await cand.factory(args.pluginConfig[cand.id]?.credentials ?? {})); } catch (err) { - process.stderr.write(`tool-plugin: failed to start "${cand.id}": ${scrubSecrets(err instanceof Error ? err.message : String(err))}\n`); + onWarning( + `tool-plugin: failed to start "${cand.id}": ${scrubSecrets(err instanceof Error ? err.message : String(err))}`, + ); } } return out; diff --git a/src/tui-opentui/command-surfaces.ts b/src/tui-opentui/command-surfaces.ts index 2973e1772..7ff75ea80 100644 --- a/src/tui-opentui/command-surfaces.ts +++ b/src/tui-opentui/command-surfaces.ts @@ -86,7 +86,12 @@ export type PermissionsSurfaceDeps = { export type PluginsSurfaceDeps = { readonly list: () => readonly PluginEntry[] - readonly setEnabled: (id: string, enabled: boolean) => Promise | void + // A trust-grant load can surface skill-miss and similar warnings; the + // optional message is shown via `deps.notify` at the call site. + readonly setEnabled: ( + id: string, + enabled: boolean, + ) => Promise<{ message?: string } | void> | void /** Persists credential values for the plugin (does not enable/verify it). */ readonly saveCredentials: ( id: string, @@ -814,7 +819,10 @@ export function openPluginsSurface(shell: AppShell, deps: CommandSurfaceDeps): v return } void Promise.resolve(plugins.setEnabled(target.id, !target.enabled)).then( - () => openPluginsSurface(shell, deps), + (result) => { + if (result?.message !== undefined) deps.notify(result.message) + openPluginsSurface(shell, deps) + }, (err: unknown) => deps.notify(`Plugin update failed: ${errorText(err)}`), ) }, @@ -847,7 +855,10 @@ export function openPluginsSurface(shell: AppShell, deps: CommandSurfaceDeps): v case "t": if (target.needsTrust !== true) return false void Promise.resolve(plugins.setEnabled(target.id, true)).then( - () => openPluginsSurface(shell, deps), + (result) => { + if (result?.message !== undefined) deps.notify(result.message) + openPluginsSurface(shell, deps) + }, (err: unknown) => deps.notify(`Trust failed: ${errorText(err)}`), ) return true diff --git a/src/tui/plugin-diagnostics-sink.test.ts b/src/tui/plugin-diagnostics-sink.test.ts index e58af4adc..17e739bef 100644 --- a/src/tui/plugin-diagnostics-sink.test.ts +++ b/src/tui/plugin-diagnostics-sink.test.ts @@ -1,17 +1,157 @@ import { describe, expect, test } from "bun:test"; -import { readFile } from "node:fs/promises"; +import { mkdir, mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; import { join } from "node:path"; +import { + createPluginLoadDiagnostics, + emitPluginWarningLog, + formatPluginWarningsSummary, +} from "../plugins/diagnostics.js"; +import { expandPluginPath, loadPluginEntry, type ExpandPluginPathSkip } from "../plugins/loader.js"; +import { resolveAgentPluginProfiles } from "../plugins/agent-plugins.js"; +import { resolveToolPlugins, type ToolPluginCandidate } from "../plugins/tool-plugins.js"; +import { discoverSessionPlugins } from "../session/runtime-assembly.js"; // The TUI holds the alternate screen for the whole interactive session, so any -// plugin-diagnostics summary that lands on raw stderr corrupts the rendered -// frame instead of showing up as a single controlled line (see CL-5411). -// `emitPluginWarningSummary` defaults to a raw `process.stderr.write` sink -// when called with no second argument; interactive callers must route through -// `emitPluginWarningLog` (the structured-logger sink) instead. -describe("runner.ts plugin diagnostics", () => { - test("never calls emitPluginWarningSummary with its raw-stderr default", async () => { - const src = await readFile(join(import.meta.dir, "runner.ts"), "utf8"); - const bareCalls = src.match(/emitPluginWarningSummary\([^,)]+\)/g) ?? []; - expect(bareCalls).toEqual([]); +// of the real plugin-loading paths runner.ts drives at startup / enable / +// verify / add-path / tool-resolve time must never write to raw stderr — a +// bare write lands mid-frame and corrupts the rendered transcript (CL-5411). +// This instruments process.stderr.write around each real code path runner.ts +// calls (not a source grep for one function name), so it catches the bug +// class regardless of which function or file the write comes from. + +async function withStderrCapture(fn: () => Promise): Promise<{ result: T; writes: number }> { + const original = process.stderr.write.bind(process.stderr); + let writes = 0; + process.stderr.write = ((..._args: unknown[]) => { + writes++; + return true; + }) as typeof process.stderr.write; + try { + const result = await fn(); + return { result, writes }; + } finally { + process.stderr.write = original; + } +} + +async function makeAgentPluginWithMissingSkill(): Promise { + const dir = await mkdtemp(join(tmpdir(), "diag-behavior-")); + const agentsDir = join(dir, "agents"); + await mkdir(agentsDir, { recursive: true }); + await writeFile( + join(agentsDir, "a.md"), + "---\nskills: [does-not-exist]\n---\nbody\n", + ); + await writeFile( + join(dir, "plugin.json"), + JSON.stringify({ id: "diag-behavior", name: "diag-behavior", kind: "agent" }), + ); + return dir; +} + +describe("interactive plugin diagnostics never hit raw stderr", () => { + test("startup discovery: a plugin with a missing skill ref stays silent on stderr", async () => { + const pluginDir = await makeAgentPluginWithMissingSkill(); + const cwd = await mkdtemp(join(tmpdir(), "diag-cwd-")); + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + await discoverSessionPlugins({ + cwd, + pluginPaths: [pluginDir], + isProjectPluginTrusted: () => true, + isRegisteredPathTrusted: () => true, + diagnostics: diag, + }); + emitPluginWarningLog(diag); + }); + expect(writes).toBe(0); + }); + + test("trust-grant / enable: loading a plugin with a missing skill ref stays silent on stderr", async () => { + const pluginDir = await makeAgentPluginWithMissingSkill(); + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + await loadPluginEntry(pluginDir, { cwd: pluginDir, origin: "path", diagnostics: diag }); + // Same fold-into-message pattern the fix applies in runner.ts's + // `saveConfig` — never a bare `emitPluginWarningSummary(diag)`. + const message = formatPluginWarningsSummary(diag.warnings); + expect(message).toBeDefined(); + }); + expect(writes).toBe(0); + }); + + test("verify: an agent profile that fails schema validation stays silent on stderr", async () => { + // resolveAgentPluginProfiles validates AgentProfileSchema (requires `id`); + // build a module with a malformed profile directly rather than round- + // tripping through markdown, since that's the exact shape runner.ts's + // `verify` handler passes in from an already-loaded module. + const mod = { + manifest: { id: "malformed-agent", name: "Malformed Agent", kind: "agent" as const }, + agentPlugin: { agents: [{ description: "missing the required id field" }] }, + }; + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + const profiles = await resolveAgentPluginProfiles( + [mod], + { "malformed-agent": { enabled: true } }, + { diagnostics: diag }, + ); + expect(profiles).toEqual([]); + const message = formatPluginWarningsSummary(diag.warnings); + expect(message).toBeDefined(); + }); + expect(writes).toBe(0); + }); + + test("add-path: a marketplace with a skipped member (outside contain root) stays silent on stderr", async () => { + const root = await mkdtemp(join(tmpdir(), "diag-market-")); + 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: [ + // Absolute source is always skipped — same shape as a real bad entry. + { name: "bad", source: "/etc/not-a-plugin" }, + ], + }), + ); + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + const members = await expandPluginPath(marketDir, { + onSkip: (skip: ExpandPluginPathSkip) => { + diag.warnings.push(`marketplace source ${JSON.stringify(skip.source)} skipped (${skip.reason})`); + }, + }); + expect(members).toEqual([]); + const message = formatPluginWarningsSummary(diag.warnings); + expect(message).toBeDefined(); + }); + expect(writes).toBe(0); + }); + + test("tool-resolve: a throwing tool-plugin factory stays silent on stderr", async () => { + const candidate: ToolPluginCandidate = { + id: "throws", + name: "Throws", + credentials: [], + factory: () => { + throw new Error("boom"); + }, + }; + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + const tools = await resolveToolPlugins({ + candidates: [candidate], + pluginConfig: { throws: { enabled: true, consented: true } }, + diagnostics: diag, + }); + expect(tools).toEqual([]); + const message = formatPluginWarningsSummary(diag.warnings); + expect(message).toBeDefined(); + }); + expect(writes).toBe(0); }); }); diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 489bb4ec5..bc1147ad1 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -47,7 +47,13 @@ import { createInferenceDependencies } from "../provider/inference-dependencies. import { getValidCodexToken } from "../auth/codex/session.js"; import { getValidXaiToken } from "../auth/xai/session.js"; import { refreshCodexInstructions } from "../auth/codex/instructions.js"; -import { expandExistingPluginMembers, expandPluginPath, loadPluginEntry, type PluginOrigin } from "../plugins/loader.js"; +import { + expandExistingPluginMembers, + expandPluginPath, + loadPluginEntry, + type ExpandPluginPathSkip, + type PluginOrigin, +} from "../plugins/loader.js"; import { createPluginLoadDiagnostics, emitPluginWarningLog, @@ -411,6 +417,14 @@ export async function runTUI(initialConfig: Config): Promise { diagnostics: pluginLoadDiag, }); emitPluginWarningLog(pluginLoadDiag); + // Fire-and-forget startup diagnostics (this + tool-plugin resolution below) + // have no result channel back to an operator action, unlike verify/add-path/ + // trust-grant. A log-only summary is invisible — nobody watches + // ~/.corbits/logs/corbits.log — so these are also queued as transcript rows + // once the shell mounts (see `systemRow` calls after `mountRunnerHost`). + const startupPluginNotices: string[] = []; + const discoveryNotice = formatPluginWarningsSummary(pluginLoadDiag.warnings); + if (discoveryNotice !== undefined) startupPluginNotices.push(discoveryNotice); // Mutable list so trusting a project/path plugin can replace a metadata-only stub // with a fully loaded module without restarting the process. let livePluginModules = pluginModules; @@ -667,6 +681,7 @@ export async function runTUI(initialConfig: Config): Promise { // Tool plugins are wired in only when enabled AND consented. const toolPluginCandidates = collectToolPlugins(executablePlugins()); // Web and tool plugin resolution are independent, so resolve them concurrently. + const toolPluginDiag = createPluginLoadDiagnostics(); const [activeWeb, extraToolPlugins] = await Promise.all([ resolveWebProviderFromPlugins({ candidates: webPluginCandidates, @@ -676,9 +691,13 @@ export async function runTUI(initialConfig: Config): Promise { resolveToolPlugins({ candidates: toolPluginCandidates, pluginConfig: config.settings?.plugins ?? {}, + diagnostics: toolPluginDiag, }), ]); if (activeWeb !== undefined) setActiveWebProviderBrand(webBrand(activeWeb.name)); + emitPluginWarningLog(toolPluginDiag); + const toolPluginNotice = formatPluginWarningsSummary(toolPluginDiag.warnings); + if (toolPluginNotice !== undefined) startupPluginNotices.push(toolPluginNotice); // /plugins UI backend: discovered plugin descriptors plus live, persisted // config (enabled flag, credentials, web override, extra paths) written to the @@ -745,6 +764,11 @@ export async function runTUI(initialConfig: Config): Promise { getWebOverride: () => liveWebOverride, saveConfig: async (id, cfg) => { livePluginConfig = { ...livePluginConfig, [id]: cfg }; + // Warnings from the trust-grant load below are collected, not logged: + // like `addPath`, the caller has a result channel back to the operator + // (the command surface's `deps.notify`), so fold them into the returned + // message instead of a log line nobody watches. + let trustGrantMessage: string | undefined; // Enabling a project/path plugin records trust and full-loads code. if (cfg.enabled === true) { const stub = livePluginModules.find((m) => m.manifest?.id === id); @@ -766,7 +790,7 @@ export async function runTUI(initialConfig: Config): Promise { origin: stub.origin, diagnostics: trustDiag, }); - emitPluginWarningLog(trustDiag); + trustGrantMessage = formatPluginWarningsSummary(trustDiag.warnings); if (full !== null) { livePluginModules = livePluginModules.map((m) => m.manifest?.id === id ? full : m, @@ -798,6 +822,7 @@ export async function runTUI(initialConfig: Config): Promise { registerCommandPlugin(mod.commandPlugin!); } await persistPluginSettings(); + return trustGrantMessage === undefined ? undefined : { message: trustGrantMessage }; }, setWebOverride: async (id) => { liveWebOverride = id; @@ -813,9 +838,13 @@ export async function runTUI(initialConfig: Config): Promise { { [id]: { enabled: true } }, { diagnostics: verifyDiag }, ); - emitPluginWarningLog(verifyDiag); if (profiles.length === 0) return { ok: false, message: "No valid agent profiles found" }; - return { ok: true, message: `loaded — ${profiles.length} profile${profiles.length === 1 ? "" : "s"}` }; + // Fold warnings into the message (same pattern as `addPath`) instead of + // logging them: "loaded — N profiles" must not read identically whether + // or not a profile's skill ref actually resolved. + const warnings = formatPluginWarningsSummary(verifyDiag.warnings); + const base = `loaded — ${profiles.length} profile${profiles.length === 1 ? "" : "s"}`; + return { ok: true, message: warnings === undefined ? base : `${base} (${warnings})` }; } // Tool plugins verify by loading (the factory must construct without // error and yield at least one tool). @@ -867,8 +896,17 @@ export async function runTUI(initialConfig: Config): Promise { if (descriptor === undefined) return { ok: false, message: "Invalid plugin manifest" }; // 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). - const members = await expandPluginPath(abs); + // member is trusted (exact-path match on reload). `onSkip` collects into + // `addDiag` instead of the default stderr write — same reasoning as + // `loadPluginEntry` above. + const members = await expandPluginPath(abs, { + onSkip: (skip: ExpandPluginPathSkip) => { + const where = skip.resolved !== undefined ? ` → ${skip.resolved}` : ""; + addDiag.warnings.push( + `marketplace source ${JSON.stringify(skip.source)} skipped (${skip.reason})${where}`, + ); + }, + }); pathTrust = await trustPathPlugins(members.length > 0 ? members : [abs]); // Replace any existing descriptor/candidate with the same id so re-adding // refreshes rather than duplicates. @@ -2047,7 +2085,7 @@ export async function runTUI(initialConfig: Config): Promise { }, setEnabled: async (id, enabled) => { const existing = pluginsAdmin.getConfig()[id] ?? {}; - await pluginsAdmin.saveConfig(id, { ...existing, enabled }); + return (await pluginsAdmin.saveConfig(id, { ...existing, enabled })) ?? undefined; }, saveCredentials: async (id, credentials) => { const existing = pluginsAdmin.getConfig()[id] ?? {}; @@ -2229,6 +2267,10 @@ export async function runTUI(initialConfig: Config): Promise { }); }); + // Surface fire-and-forget startup plugin diagnostics now that the shell has + // a transcript to write into (queued above, before `host` existed). + for (const notice of startupPluginNotices) systemRow(notice); + await host.waitUntilExit(); // Quitting mid-stream is an abnormal end for the in-flight cycle: nothing // downstream delivers its terminal event once the app is gone. From 702ccf9c46ca8b1a3b26172bb07db12c871ceea5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 02:33:49 -0700 Subject: [PATCH 3/5] Collect discovery-time marketplace skips, surface profile warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more gaps in the plugin-diagnostics fix: scanPluginsDir and loadPluginsFromPaths both call expandPluginPath with no onSkip, so a marketplace member skipped during discovery (a bad source under .corbits/plugins/ or a registered pluginPaths entry) fell through to the raw-stderr default. Unlike the earlier sites, this one bypassed the diagnostics collector entirely rather than just misrouting to the log, so the warning was lost outright. Added expandSkipDiagnosticsHandler in loader.ts and wired it into both call sites, plus addPath in runner.ts (replacing its duplicate inline version). profileDiag, the startup agent-profile resolution, was still logged only and never reached startupPluginNotices — one of the sites the previous commit's message claimed to have fixed but did not. Gave it the same treatment as its discovery and tool-plugin siblings. Extended the behavioral test with the marketplace-discovery-skip case (asserting the warning lands in diagnostics, not just that stderr stays quiet) and the startup agent-profile path. --- src/plugins/loader.ts | 33 ++++++++++++-- src/tui/plugin-diagnostics-sink.test.ts | 59 ++++++++++++++++++++++++- src/tui/runner.ts | 19 ++++---- 3 files changed, 98 insertions(+), 13 deletions(-) diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 61b61df4a..d18cedfe6 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -267,6 +267,27 @@ function defaultExpandSkip(skip: ExpandPluginPathSkip): void { ); } +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. + */ +export function expandSkipDiagnosticsHandler( + diagnostics?: PluginLoadDiagnostics, +): ((skip: ExpandPluginPathSkip) => void) | undefined { + if (diagnostics === undefined) return undefined; + return (skip) => diagnostics.warnings.push(formatExpandSkip(skip)); +} + /** * 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 @@ -456,8 +477,11 @@ 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 onSkip = expandSkipDiagnosticsHandler(diagnostics); + const dirs = await expandPluginPath(join(dir, entry), onSkip !== undefined ? { onSkip } : {}); for (const d of dirs) { const abs = resolve(d); if (originRequiresTrust(origin) && isTrusted !== undefined && !isTrusted(abs)) { @@ -536,10 +560,13 @@ export async function loadPluginsFromPaths( diagnostics?: PluginLoadDiagnostics; } = {}, ): Promise { + // 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 resolved = await Promise.all( paths.map(async (p) => { const abs = isAbsolute(p) ? p : join(cwd, p); - return expandPluginPath(abs); + return expandPluginPath(abs, onSkip !== undefined ? { onSkip } : {}); }), ); // Anything under /.corbits/plugins/ is project origin no matter how it diff --git a/src/tui/plugin-diagnostics-sink.test.ts b/src/tui/plugin-diagnostics-sink.test.ts index 17e739bef..e4d9f283a 100644 --- a/src/tui/plugin-diagnostics-sink.test.ts +++ b/src/tui/plugin-diagnostics-sink.test.ts @@ -7,7 +7,12 @@ import { emitPluginWarningLog, formatPluginWarningsSummary, } from "../plugins/diagnostics.js"; -import { expandPluginPath, loadPluginEntry, type ExpandPluginPathSkip } from "../plugins/loader.js"; +import { + discoverUserPlugins, + expandPluginPath, + loadPluginEntry, + type ExpandPluginPathSkip, +} from "../plugins/loader.js"; import { resolveAgentPluginProfiles } from "../plugins/agent-plugins.js"; import { resolveToolPlugins, type ToolPluginCandidate } from "../plugins/tool-plugins.js"; import { discoverSessionPlugins } from "../session/runtime-assembly.js"; @@ -132,6 +137,58 @@ describe("interactive plugin diagnostics never hit raw stderr", () => { expect(writes).toBe(0); }); + test("startup discovery: a marketplace member skipped during discovery is collected, not lost", async () => { + // Reproduces the exact shape reported against `scanPluginsDir` / + // `loadPluginsFromPaths`: a project-local `.corbits/plugins/` entry that + // is itself a marketplace with one bad (absolute) `source`. Both call + // `expandPluginPath` internally; without `onSkip` wired to `diagnostics`, + // the skip bypasses the collector entirely (not merely misrouted to the + // log — genuinely dropped, since nothing else observes the raw write). + const cwd = await mkdtemp(join(tmpdir(), "diag-cwd-market-")); + const marketDir = join(cwd, ".corbits", "plugins", "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(); + await discoverUserPlugins(cwd, { diagnostics: diag }); + // The skip must land in the collector, not just avoid stderr — a write + // that silently vanishes without reaching diagnostics is the same + // lost-warning bug reached by a different route. + 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), + // distinct from the verify-time call above which targets one plugin id. + const mod = { + manifest: { id: "startup-agent", name: "Startup Agent", kind: "agent" as const }, + agentPlugin: { agents: [{ description: "missing the required id field" }] }, + }; + const { writes } = await withStderrCapture(async () => { + const diag = createPluginLoadDiagnostics(); + const profiles = await resolveAgentPluginProfiles( + [mod], + { "startup-agent": { enabled: true } }, + { diagnostics: diag }, + ); + expect(profiles).toEqual([]); + const message = formatPluginWarningsSummary(diag.warnings); + expect(message).toBeDefined(); + }); + expect(writes).toBe(0); + }); + test("tool-resolve: a throwing tool-plugin factory stays silent on stderr", async () => { const candidate: ToolPluginCandidate = { id: "throws", diff --git a/src/tui/runner.ts b/src/tui/runner.ts index bc1147ad1..ab4c9d3ba 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -50,8 +50,8 @@ import { refreshCodexInstructions } from "../auth/codex/instructions.js"; import { expandExistingPluginMembers, expandPluginPath, + expandSkipDiagnosticsHandler, loadPluginEntry, - type ExpandPluginPathSkip, type PluginOrigin, } from "../plugins/loader.js"; import { @@ -899,14 +899,11 @@ export async function runTUI(initialConfig: Config): Promise { // member is trusted (exact-path match on reload). `onSkip` collects into // `addDiag` instead of the default stderr write — same reasoning as // `loadPluginEntry` above. - const members = await expandPluginPath(abs, { - onSkip: (skip: ExpandPluginPathSkip) => { - const where = skip.resolved !== undefined ? ` → ${skip.resolved}` : ""; - addDiag.warnings.push( - `marketplace source ${JSON.stringify(skip.source)} skipped (${skip.reason})${where}`, - ); - }, - }); + const addSkipHandler = expandSkipDiagnosticsHandler(addDiag); + const members = await expandPluginPath( + abs, + addSkipHandler !== undefined ? { onSkip: addSkipHandler } : {}, + ); pathTrust = await trustPathPlugins(members.length > 0 ? members : [abs]); // Replace any existing descriptor/candidate with the same id so re-adding // refreshes rather than duplicates. @@ -985,6 +982,10 @@ export async function runTUI(initialConfig: Config): Promise { { diagnostics: profileDiag }, ); emitPluginWarningLog(profileDiag); + // Same fire-and-forget reasoning as the discovery/tool-plugin notices above: + // this runs before `host` exists, so it is queued rather than dropped. + const profileNotice = formatPluginWarningsSummary(profileDiag.warnings); + if (profileNotice !== undefined) startupPluginNotices.push(profileNotice); const initialProfiles = await loadAgentProfiles(profilesDir, pluginAgentProfiles); let liveAgentProfiles = initialProfiles; From d933f1e0b11f5f73f8b20c8e0a00a877266e70b2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 02:48:36 -0700 Subject: [PATCH 4/5] 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. --- src/exec/runner.ts | 14 ++++- src/plugins/loader.ts | 77 +++++++++++++++---------- src/tui/plugin-diagnostics-sink.test.ts | 35 +++++++++++ src/tui/runner.ts | 18 +++--- tests/unit/path-plugin-trust.test.ts | 12 +++- tests/unit/plugin-marketplace.test.ts | 18 +++--- 6 files changed, 126 insertions(+), 48 deletions(-) diff --git a/src/exec/runner.ts b/src/exec/runner.ts index ae69fde13..585fce223 100644 --- a/src/exec/runner.ts +++ b/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 { 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 }, ); diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index d18cedfe6..182928f1b 100644 --- a/src/plugins/loader.ts +++ b/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 { 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 { 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 { // 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 /.corbits/plugins/ is project origin no matter how it diff --git a/src/tui/plugin-diagnostics-sink.test.ts b/src/tui/plugin-diagnostics-sink.test.ts index e4d9f283a..416dd8025 100644 --- a/src/tui/plugin-diagnostics-sink.test.ts +++ b/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), diff --git a/src/tui/runner.ts b/src/tui/runner.ts index ab4c9d3ba..9872ab2d4 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -393,17 +393,21 @@ export async function runTUI(initialConfig: Config): Promise { // 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 { // 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. diff --git a/tests/unit/path-plugin-trust.test.ts b/tests/unit/path-plugin-trust.test.ts index f5fd86626..542e40caf 100644 --- a/tests/unit/path-plugin-trust.test.ts +++ b/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 { 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); diff --git a/tests/unit/plugin-marketplace.test.ts b/tests/unit/plugin-marketplace.test.ts index 07e7b2ec7..21291ff96 100644 --- a/tests/unit/plugin-marketplace.test.ts +++ b/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; } From a6ed4665150797fb504d7d7a782381468f93ebff Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 07:41:09 -0700 Subject: [PATCH 5/5] Close the last raw-stderr default in the plugin diagnostics chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolvePluginWarningHandler still fell back to a raw stderr write when given neither a diagnostics collector nor an explicit onWarning, the same silent-default shape that produced four rounds of one-off fixes at the call-site level. Make the choice a required discriminated union (diagnostics or onWarning, no third option) so the compiler enumerates every caller. pluginWarningSink drops its own fallback parameter, since that was only ever reachable through the branch just removed. Each of the five call sites the compiler surfaced now builds its own explicit union locally: real callers that hold a diagnostics collector keep using it, and the handful of standalone/test call sites that pass neither get the raw stderr writer via a single named export, stderrPluginWarning, instead of five copies of the same inline lambda — greppable, and one place to change if the prefix or destination ever does. Extends the behavioural stderr-capture test to pin that resolveToolPlugins still falls back correctly (one write per warning) when called with no collector at all. --- src/plugins/data-only.ts | 11 +++++-- src/plugins/diagnostics.test.ts | 5 +--- src/plugins/diagnostics.ts | 40 +++++++++++-------------- src/plugins/loader.ts | 31 ++++++++++++------- src/plugins/tool-plugins.ts | 5 +++- src/tui/plugin-diagnostics-sink.test.ts | 29 ++++++++++++++++++ 6 files changed, 80 insertions(+), 41 deletions(-) diff --git a/src/plugins/data-only.ts b/src/plugins/data-only.ts index 915284ffc..ba5a37271 100644 --- a/src/plugins/data-only.ts +++ b/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 { 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), diff --git a/src/plugins/diagnostics.test.ts b/src/plugins/diagnostics.test.ts index 09ed23a7e..4040a56c9 100644 --- a/src/plugins/diagnostics.test.ts +++ b/src/plugins/diagnostics.test.ts @@ -97,13 +97,10 @@ describe("plugin load diagnostics wiring", () => { "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, { diff --git a/src/plugins/diagnostics.ts b/src/plugins/diagnostics.ts index 2b2cf24ed..a66e6fc6a 100644 --- a/src/plugins/diagnostics.ts +++ b/src/plugins/diagnostics.ts @@ -15,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`); } /** diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 182928f1b..6e4a5b988 100644 --- a/src/plugins/loader.ts +++ b/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 { 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; @@ -657,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)) { @@ -674,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 diff --git a/src/plugins/tool-plugins.ts b/src/plugins/tool-plugins.ts index 174b42f89..b00a069bb 100644 --- a/src/plugins/tool-plugins.ts +++ b/src/plugins/tool-plugins.ts @@ -5,6 +5,7 @@ import type { PluginCredentialField } from "./manifest.js"; import { scrubSecrets } from "../web/secret-scrub.js"; import { resolvePluginWarningHandler, + stderrPluginWarning, type PluginLoadDiagnostics, } from "./diagnostics.js"; @@ -52,7 +53,9 @@ export async function resolveToolPlugins(args: { diagnostics?: PluginLoadDiagnostics; }): Promise { const onWarning = resolvePluginWarningHandler( - args.diagnostics === undefined ? {} : { diagnostics: args.diagnostics }, + args.diagnostics !== undefined + ? { diagnostics: args.diagnostics } + : { onWarning: stderrPluginWarning }, ); const out: ToolPlugin[] = []; for (const cand of args.candidates) { diff --git a/src/tui/plugin-diagnostics-sink.test.ts b/src/tui/plugin-diagnostics-sink.test.ts index 416dd8025..d9e20156f 100644 --- a/src/tui/plugin-diagnostics-sink.test.ts +++ b/src/tui/plugin-diagnostics-sink.test.ts @@ -247,3 +247,32 @@ describe("interactive plugin diagnostics never hit raw stderr", () => { expect(writes).toBe(0); }); }); + +// The interactive paths above always hand `resolveToolPlugins` a diagnostics +// collector. Headless/standalone callers (exec's tool-plugin resolution, +// direct unit tests) may supply neither `diagnostics` nor `onWarning` — +// `resolveToolPlugins` still resolves that case, but now via its own +// explicit stderr fallback rather than delegating to +// `resolvePluginWarningHandler`'s old default. This pins that the fallback +// still fires exactly once per warning (not silently dropped) when no +// collector is in play, matching a genuinely headless call site. +describe("resolveToolPlugins without a diagnostics collector", () => { + test("falls back to one explicit stderr write per failure", async () => { + const candidate: ToolPluginCandidate = { + id: "throws", + name: "Throws", + credentials: [], + factory: () => { + throw new Error("boom"); + }, + }; + const { result: tools, writes } = await withStderrCapture(async () => + resolveToolPlugins({ + candidates: [candidate], + pluginConfig: { throws: { enabled: true, consented: true } }, + }), + ); + expect(tools).toEqual([]); + expect(writes).toBe(1); + }); +});