Skip to content

Commit a6ed466

Browse files
committed
Close the last raw-stderr default in the plugin diagnostics chain
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.
1 parent d933f1e commit a6ed466

6 files changed

Lines changed: 80 additions & 41 deletions

File tree

src/plugins/data-only.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { loadDataOnlyCommands } from "./data-only-commands.js";
77
import { loadSkillCommands } from "./skill-commands.js";
88
import {
99
resolvePluginWarningHandler,
10+
stderrPluginWarning,
1011
type PluginLoadDiagnostics,
1112
} from "./diagnostics.js";
1213

@@ -80,8 +81,14 @@ export async function loadDataOnlyPlugin(
8081
} = {},
8182
): Promise<DataOnlyPlugin | null> {
8283
const cwd = opts.cwd ?? process.cwd();
83-
// Prefer diagnostics collector; else explicit onWarning; else stderr default.
84-
const onWarning = resolvePluginWarningHandler(opts);
84+
// Prefer diagnostics collector; else explicit onWarning; else stderrPluginWarning.
85+
const onWarning = resolvePluginWarningHandler(
86+
opts.diagnostics !== undefined
87+
? { diagnostics: opts.diagnostics }
88+
: opts.onWarning !== undefined
89+
? { onWarning: opts.onWarning }
90+
: { onWarning: stderrPluginWarning },
91+
);
8592

8693
const [nativeManifest, claudeManifest, agents, commands, skillCmds] = await Promise.all([
8794
readManifestJson(pluginDir),

src/plugins/diagnostics.test.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,10 @@ describe("plugin load diagnostics wiring", () => {
9797
"agents/a.md": "---\nskills: [nope, also-missing]\n---\nbody\n",
9898
});
9999
const diag = createPluginLoadDiagnostics();
100-
const stderrLines: string[] = [];
101-
const sink = pluginWarningSink(diag, (msg) => stderrLines.push(msg));
100+
const sink = pluginWarningSink(diag);
102101

103-
// Sink itself must not hit fallback when diag is set.
104102
sink("should only land in diag");
105103
expect(diag.warnings).toEqual(["should only land in diag"]);
106-
expect(stderrLines).toEqual([]);
107104

108105
diag.warnings.length = 0;
109106
const mod = await loadPluginEntry(dir, {

src/plugins/diagnostics.ts

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -15,34 +15,28 @@ export function createPluginLoadDiagnostics(): PluginLoadDiagnostics {
1515
return { warnings: [] };
1616
}
1717

18+
/** Build an onWarning callback that records into `diag`. */
19+
export function pluginWarningSink(diag: PluginLoadDiagnostics): (msg: string) => void {
20+
return (msg) => {
21+
diag.warnings.push(msg);
22+
};
23+
}
24+
1825
/**
19-
* Build an onWarning callback that records into `diag` when provided, else
20-
* falls back to the given sink (default: one stderr line per message).
26+
* Resolve the warning sink for a load call. There is no default: callers
27+
* must decide between a diagnostics collector (batched into one summary,
28+
* safe mid-frame) and an explicit onWarning (e.g. a raw stderr writer for
29+
* headless paths where no frame is being held).
2130
*/
22-
export function pluginWarningSink(
23-
diag: PluginLoadDiagnostics | undefined,
24-
fallback: (msg: string) => void = (msg) => process.stderr.write(`plugins: ${msg}\n`),
31+
export function resolvePluginWarningHandler(
32+
opts: { diagnostics: PluginLoadDiagnostics } | { onWarning: (msg: string) => void },
2533
): (msg: string) => void {
26-
if (diag !== undefined) {
27-
return (msg) => {
28-
diag.warnings.push(msg);
29-
};
30-
}
31-
return fallback;
34+
return "diagnostics" in opts ? pluginWarningSink(opts.diagnostics) : opts.onWarning;
3235
}
3336

34-
/**
35-
* Resolve the warning sink for a load call. Prefer a diagnostics collector when
36-
* provided (so batch callers can emit one summary); else an explicit onWarning;
37-
* else one stderr line per message.
38-
*/
39-
export function resolvePluginWarningHandler(opts: {
40-
diagnostics?: PluginLoadDiagnostics;
41-
onWarning?: (msg: string) => void;
42-
}): (msg: string) => void {
43-
if (opts.diagnostics !== undefined) return pluginWarningSink(opts.diagnostics);
44-
if (opts.onWarning !== undefined) return opts.onWarning;
45-
return pluginWarningSink(undefined);
37+
/** Named raw-stderr choice: `{ onWarning: stderrPluginWarning }`. */
38+
export function stderrPluginWarning(msg: string): void {
39+
process.stderr.write(`plugins: ${msg}\n`);
4640
}
4741

4842
/**

src/plugins/loader.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { parsePluginManifest, type PluginManifest } from "./manifest.js";
1010
import { loadDataOnlyPlugin } from "./data-only.js";
1111
import {
1212
resolvePluginWarningHandler,
13+
stderrPluginWarning,
1314
type PluginLoadDiagnostics,
1415
} from "./diagnostics.js";
1516
import {
@@ -116,9 +117,15 @@ export async function loadPluginEntry(
116117
} = {},
117118
): Promise<PluginModule | null> {
118119
const cwd = opts.cwd ?? process.cwd();
119-
// Prefer diagnostics collector when provided so batch discovery can summarize;
120-
// explicit onWarning is for tests / one-off sinks; default is stderr per line.
121-
const onWarning = resolvePluginWarningHandler(opts);
120+
// Prefer diagnostics collector so batch discovery can summarize; explicit
121+
// onWarning for tests; else stderrPluginWarning.
122+
const onWarning = resolvePluginWarningHandler(
123+
opts.diagnostics !== undefined
124+
? { diagnostics: opts.diagnostics }
125+
: opts.onWarning !== undefined
126+
? { onWarning: opts.onWarning }
127+
: { onWarning: stderrPluginWarning },
128+
);
122129
const origin = opts.origin;
123130
let target = entryPath;
124131
let pluginDir = entryPath;
@@ -657,12 +664,12 @@ export async function discoverClaudeInstalledPlugins(
657664
try {
658665
parsed = JSON.parse(raw);
659666
} catch {
660-
// Prefer collector when present; else one stderr line (default sink).
667+
// Prefer collector when present; else stderrPluginWarning.
661668
resolvePluginWarningHandler(
662-
opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
663-
)(
664-
`failed to parse ${registryPath}`,
665-
);
669+
opts.diagnostics !== undefined
670+
? { diagnostics: opts.diagnostics }
671+
: { onWarning: stderrPluginWarning },
672+
)(`failed to parse ${registryPath}`);
666673
return [];
667674
}
668675
if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
@@ -674,10 +681,12 @@ export async function discoverClaudeInstalledPlugins(
674681
}
675682

676683
const pluginsRoot = resolve(home, ".claude", "plugins");
677-
// Default expand-skip sink respects diagnostics when provided so discovery
678-
// can emit one summary; explicit onExpandSkip (tests) still wins.
684+
// Default expand-skip sink: diagnostics when provided, else
685+
// stderrPluginWarning; explicit onExpandSkip (tests) still wins.
679686
const warnExpand = resolvePluginWarningHandler(
680-
opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
687+
opts.diagnostics !== undefined
688+
? { diagnostics: opts.diagnostics }
689+
: { onWarning: stderrPluginWarning },
681690
);
682691
const onExpandSkip =
683692
opts.onExpandSkip

src/plugins/tool-plugins.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { PluginCredentialField } from "./manifest.js";
55
import { scrubSecrets } from "../web/secret-scrub.js";
66
import {
77
resolvePluginWarningHandler,
8+
stderrPluginWarning,
89
type PluginLoadDiagnostics,
910
} from "./diagnostics.js";
1011

@@ -52,7 +53,9 @@ export async function resolveToolPlugins(args: {
5253
diagnostics?: PluginLoadDiagnostics;
5354
}): Promise<ToolPlugin[]> {
5455
const onWarning = resolvePluginWarningHandler(
55-
args.diagnostics === undefined ? {} : { diagnostics: args.diagnostics },
56+
args.diagnostics !== undefined
57+
? { diagnostics: args.diagnostics }
58+
: { onWarning: stderrPluginWarning },
5659
);
5760
const out: ToolPlugin[] = [];
5861
for (const cand of args.candidates) {

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,3 +247,32 @@ describe("interactive plugin diagnostics never hit raw stderr", () => {
247247
expect(writes).toBe(0);
248248
});
249249
});
250+
251+
// The interactive paths above always hand `resolveToolPlugins` a diagnostics
252+
// collector. Headless/standalone callers (exec's tool-plugin resolution,
253+
// direct unit tests) may supply neither `diagnostics` nor `onWarning` —
254+
// `resolveToolPlugins` still resolves that case, but now via its own
255+
// explicit stderr fallback rather than delegating to
256+
// `resolvePluginWarningHandler`'s old default. This pins that the fallback
257+
// still fires exactly once per warning (not silently dropped) when no
258+
// collector is in play, matching a genuinely headless call site.
259+
describe("resolveToolPlugins without a diagnostics collector", () => {
260+
test("falls back to one explicit stderr write per failure", async () => {
261+
const candidate: ToolPluginCandidate = {
262+
id: "throws",
263+
name: "Throws",
264+
credentials: [],
265+
factory: () => {
266+
throw new Error("boom");
267+
},
268+
};
269+
const { result: tools, writes } = await withStderrCapture(async () =>
270+
resolveToolPlugins({
271+
candidates: [candidate],
272+
pluginConfig: { throws: { enabled: true, consented: true } },
273+
}),
274+
);
275+
expect(tools).toEqual([]);
276+
expect(writes).toBe(1);
277+
});
278+
});

0 commit comments

Comments
 (0)