Skip to content

Commit 5bad7c9

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 a raw stderr writer chosen visibly in that function's own body, not inherited from a shared library default. 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 5bad7c9

6 files changed

Lines changed: 78 additions & 41 deletions

File tree

src/plugins/data-only.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,16 @@ export async function loadDataOnlyPlugin(
8080
} = {},
8181
): Promise<DataOnlyPlugin | null> {
8282
const cwd = opts.cwd ?? process.cwd();
83-
// Prefer diagnostics collector; else explicit onWarning; else stderr default.
84-
const onWarning = resolvePluginWarningHandler(opts);
83+
// Prefer diagnostics collector; else explicit onWarning; else raw stderr —
84+
// chosen here (not a hidden library default) for standalone/test callers
85+
// that supply neither.
86+
const onWarning = resolvePluginWarningHandler(
87+
opts.diagnostics !== undefined
88+
? { diagnostics: opts.diagnostics }
89+
: opts.onWarning !== undefined
90+
? { onWarning: opts.onWarning }
91+
: { onWarning: (msg: string) => process.stderr.write(`plugins: ${msg}\n`) },
92+
);
8593

8694
const [nativeManifest, claudeManifest, agents, commands, skillCmds] = await Promise.all([
8795
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: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,34 +15,23 @@ export function createPluginLoadDiagnostics(): PluginLoadDiagnostics {
1515
return { warnings: [] };
1616
}
1717

18-
/**
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).
21-
*/
22-
export function pluginWarningSink(
23-
diag: PluginLoadDiagnostics | undefined,
24-
fallback: (msg: string) => void = (msg) => process.stderr.write(`plugins: ${msg}\n`),
25-
): (msg: string) => void {
26-
if (diag !== undefined) {
27-
return (msg) => {
28-
diag.warnings.push(msg);
29-
};
30-
}
31-
return fallback;
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+
};
3223
}
3324

3425
/**
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.
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).
3830
*/
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);
31+
export function resolvePluginWarningHandler(
32+
opts: { diagnostics: PluginLoadDiagnostics } | { onWarning: (msg: string) => void },
33+
): (msg: string) => void {
34+
return "diagnostics" in opts ? pluginWarningSink(opts.diagnostics) : opts.onWarning;
4635
}
4736

4837
/**

src/plugins/loader.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -116,9 +116,17 @@ export async function loadPluginEntry(
116116
} = {},
117117
): Promise<PluginModule | null> {
118118
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);
119+
// Prefer diagnostics collector when provided so batch discovery can
120+
// summarize; explicit onWarning is for tests / one-off sinks; raw stderr
121+
// is chosen here (not a hidden library default) for callers that supply
122+
// neither.
123+
const onWarning = resolvePluginWarningHandler(
124+
opts.diagnostics !== undefined
125+
? { diagnostics: opts.diagnostics }
126+
: opts.onWarning !== undefined
127+
? { onWarning: opts.onWarning }
128+
: { onWarning: (msg: string) => process.stderr.write(`plugins: ${msg}\n`) },
129+
);
122130
const origin = opts.origin;
123131
let target = entryPath;
124132
let pluginDir = entryPath;
@@ -657,12 +665,13 @@ export async function discoverClaudeInstalledPlugins(
657665
try {
658666
parsed = JSON.parse(raw);
659667
} catch {
660-
// Prefer collector when present; else one stderr line (default sink).
668+
// Prefer collector when present; else raw stderr — chosen here for
669+
// callers (tests, standalone use) with no collector.
661670
resolvePluginWarningHandler(
662-
opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
663-
)(
664-
`failed to parse ${registryPath}`,
665-
);
671+
opts.diagnostics !== undefined
672+
? { diagnostics: opts.diagnostics }
673+
: { onWarning: (msg: string) => process.stderr.write(`plugins: ${msg}\n`) },
674+
)(`failed to parse ${registryPath}`);
666675
return [];
667676
}
668677
if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
@@ -675,9 +684,12 @@ export async function discoverClaudeInstalledPlugins(
675684

676685
const pluginsRoot = resolve(home, ".claude", "plugins");
677686
// Default expand-skip sink respects diagnostics when provided so discovery
678-
// can emit one summary; explicit onExpandSkip (tests) still wins.
687+
// can emit one summary; explicit onExpandSkip (tests) still wins; raw
688+
// stderr is the deliberate fallback when neither is given.
679689
const warnExpand = resolvePluginWarningHandler(
680-
opts.diagnostics !== undefined ? { diagnostics: opts.diagnostics } : {},
690+
opts.diagnostics !== undefined
691+
? { diagnostics: opts.diagnostics }
692+
: { onWarning: (msg: string) => process.stderr.write(`plugins: ${msg}\n`) },
681693
);
682694
const onExpandSkip =
683695
opts.onExpandSkip

src/plugins/tool-plugins.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ export async function resolveToolPlugins(args: {
5252
diagnostics?: PluginLoadDiagnostics;
5353
}): Promise<ToolPlugin[]> {
5454
const onWarning = resolvePluginWarningHandler(
55-
args.diagnostics === undefined ? {} : { diagnostics: args.diagnostics },
55+
args.diagnostics !== undefined
56+
? { diagnostics: args.diagnostics }
57+
: { onWarning: (msg: string) => process.stderr.write(`plugins: ${msg}\n`) },
5658
);
5759
const out: ToolPlugin[] = [];
5860
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)