Route interactive plugin diagnostics through the log sink, not stderr - #369
Conversation
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.
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.
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.
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.
Review summary — four rounds, held open for human reviewReady for your call. The final round changed approach rather than patching another site, which is why I stopped here. What this fixesPlugin diagnostics were writing raw to stderr and corrupting the OpenTUI alt-screen frame. Four separate rounds of review each turned up one more site doing it — which was the signal that patching call sites was the wrong approach. The last round removed the cause: Verified independently: The other half — warnings had gone silentRouting to the log sink initially made three sites invisible, with no log viewer anywhere in the TUI. Now: Residual worth your judgment, not a blocker
Also confirmed: both issues' correctness criteria were already on Typecheck and build clean. 4046 pass / 0 fail, identical under |
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.
5bad7c9 to
a6ed466
Compare
1 participant
Summary
Investigated both CL-5411 and CL-5409. Both issues' correctness bullets
turned out to already be implemented and tested on
origin/main:../agents/sources):expandPluginPathinsrc/plugins/loader.tsalready resolves in-tree and sibling../agents/xsources under the correct contain root, with fullcoverage in
src/plugins/claude-plugins.test.tsandtests/unit/plugin-marketplace.test.ts.resolveSkillBodyinsrc/extensions/skills.tsalready resolves bare and path-like refswith containment + symlink-realpath checks, fully covered by
tests/unit/skills.test.ts.What needed fixing was CL-5411's other bullet: stderr dumping under
the prompt. This went through four rounds.
Rounds 1–3 fixed call sites one at a time — six total across
runner.ts,tool-plugins.ts, andloader.ts— each writing rawstderr with no diagnostics collector in play, reachable from the
interactive TUI's discovery, trust-grant, verify, add-path, and
tool-resolve paths. Three rounds each turned up exactly one more site.
Round 4 changed the approach instead of patching a fourth site.
expandPluginPath'sonSkipwas an optional parameter defaulting toa raw stderr write (
defaultExpandSkip) — an optional field with asilent, harmful default is exactly the shape that let three rounds
each find one more place omitting it, with nothing to catch it. So:
defaultExpandSkipand madeonSkipa required fieldon
ExpandPluginPathOptions. Every call site is now a compile erroruntil it supplies a handler on purpose.
expandExistingPluginMemberscalledexpandPluginPathwith noonSkip, reached fromrunner.ts's startup trust-migration path(seven lines before the discovery call fixed in round 3) and from
exec/runner.ts.expandExistingPluginMembersnow takes a requiredonSkiptoo — the member is still correctly dropped from thereturned list (trust decisions must not pre-grant a directory that
could appear later), but the reason now reaches diagnostics
instead of vanishing.
expandSkipDiagnosticsHandleris now the ordinary handler (no moreoptional/undefined fallback) rather than an opt-in override.
exec/runner.tspasses an explicit stderr writer built from a newlyexported
formatExpandSkip— headless mode legitimately wantsstderr, and that choice is now visible at its own call site instead
of being an invisible library default.
(
tests/unit/plugin-marketplace.test.ts,tests/unit/path-plugin-trust.test.ts) that asserted the oldstderr-default behavior, and added coverage for a skip reached
through
expandExistingPluginMembers.docs/PLUGINS.mdneeds no update — the sink mechanism is internal, notpart of the discovery/trust/manifest contract the doc describes.
Net diff across all four commits: +420/-40 lines. Not net-negative
overall (round 4 alone is +126/-48, mostly doc comments explaining why
onSkipis required and updated test coverage), but the shape at eachcall site is simpler now — no more
onSkip !== undefined ? {onSkip} : {}conditional spreads, just{ onSkip }.Verification
rounds. Round 4: reverted
loader.tsto its pre-round-4 version andconfirmed the new
expandExistingPluginMemberscoverage fails(diagnostics stayed empty because the skip went straight to a raw
write), then restored and confirmed green.
bun run typecheck— clean.bun run build— clean.bun run test— 4046 pass, 0 fail.bun test ./src ./tests ./evals --randomize --seed 42— 4046 pass,0 fail (order-independence preserved).
tree (marketplace
../agents/resolution + skill bundling with 0stderr writes; throwing tool-plugin factory and skipped marketplace
members each collecting a warning with 0 stderr writes; a
project-local
.corbits/plugins/marketplace with a bad sourcegoing from
writes: 1, diag.warnings: []towrites: 0, diag.warnings: [...]).Closes CL-5411
Closes CL-5409