Skip to content

Commit 703d823

Browse files
Keep the landing hero when a startup diagnostic arrives (#415)
* Count each missing plugin skill once in the load summary A skill referenced by three plugins and absent from the search path is one missing skill, not three: the operator installs it once and all three resolve. Counting raw warnings made the summary both wrong and self-contradicting, reading "7 skills missing" above a list of nine names. * Keep the landing hero when a startup diagnostic arrives Anything the runner says before the first turn lands while the landing still owns the screen, and a transcript row there reaches clearLandingMark and takes the whole composition with it — the mark, the guidance beside it, and the centred prompt box. The previous fix routed the MCP and hook producers away from the transcript one at a time and the plugin producer kept the defect. Routing the runner's own notice helper through the shell's notice path instead fixes every producer at once, including the ones nobody has written yet, and gives the constraint a single owner rather than a rule each call site has to remember. The gutter label goes with it. A system row's text already says what it is, so stamping it "command" only leaked wiring into a column the operator reads. * Let snow fall over the landing mark The sky above the ridgeline was dead space. A sparse field of pixel snow drifts through it on the clock the mark already runs on, so the hero has motion even while the silhouette is held. Density and fall rate stay low deliberately: the mark has to keep reading as a mark, and a storm would turn the one legible thing on the screen into texture. Flakes never land on mountain coverage, and they stop entirely when the mark is held still or is fading out, so the decoration never outlives the thing it drifts over. Absorbed from the standalone snow branch rather than reimplemented; the taste question it was open on is settled by making snow part of the default landing. * Stop labelling transcript rows with the wiring that produced them The meta column is the operator's: it says what a row is about, and it is read at a glance beside every row in the transcript. A row labelled "palette" says only which part of the code emitted it, which is a fact about us and not about their session — and the three rows carrying it already open with "palette:" in their own text, so the column was repeating a word it sat next to. * Set the landing's two doors as a pair The key and its description were joined by a single space, so the two lines started their descriptions on different columns and read as two unrelated notes rather than as the set they are. A fixed key column lines them up. The version moves a row away from them for the same reason: sitting flush under the two keys it read as a third door, when it is only a statement of what is running. * Name the notice path for what it carries The wrapper module added nothing the shell function did not already do. It introduced no type and narrowed no export, so a producer reaching for appendStreamRow directly was exactly as easy with it as without, and the history it documented reads better on the function itself. The name was also wrong. The path carries unknown commands, unavailable modals and provider failures, none of which happen at startup, and a name that lies to the next reader is how this constraint got lost twice.
1 parent 3b6cf52 commit 703d823

10 files changed

Lines changed: 342 additions & 61 deletions

File tree

src/plugins/diagnostics.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,33 @@ describe("formatPluginWarningsSummary", () => {
5050
expect(summary).toContain("1 skill missing");
5151
expect(summary).toContain("1 other warning");
5252
});
53+
54+
test("names a skill once however many sources missed it", () => {
55+
// The same skill missing from three plugins is one missing skill, not
56+
// three: the operator installs it once to fix all of them.
57+
const summary = formatPluginWarningsSummary([
58+
'agent a: skill "brand-identity" referenced but not found in skill search path',
59+
'agent a: skill "style" referenced but not found in skill search path',
60+
'agent b: skill "philosophy" referenced but not found in skill search path',
61+
'agent b: skill "style" referenced but not found in skill search path',
62+
'agent c: skill "philosophy" referenced but not found in skill search path',
63+
'agent c: skill "style" referenced but not found in skill search path',
64+
'agent c: skill "brand-identity" referenced but not found in skill search path',
65+
]);
66+
expect(summary).toBe(
67+
"plugins: 3 skills missing: brand-identity, style, philosophy",
68+
);
69+
});
70+
71+
test("mixed-warning count also counts distinct skills", () => {
72+
const summary = formatPluginWarningsSummary([
73+
'agent a: skill "style" referenced but not found in skill search path',
74+
'agent b: skill "style" referenced but not found in skill search path',
75+
"other problem",
76+
]);
77+
expect(summary).toContain("1 skill missing (style)");
78+
expect(summary).toContain("1 other warning");
79+
});
5380
});
5481

5582
describe("emitPluginWarningSummary", () => {

src/plugins/diagnostics.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,31 +43,40 @@ export function stderrPluginWarning(msg: string): void {
4343
* One-line summary for a batch of load warnings. Skill-miss messages are
4444
* collapsed to `N skills missing: a, b, c`; mixed warnings get a count line.
4545
* Returns undefined when there is nothing to report.
46+
*
47+
* Skill names are deduplicated because a skill is missing once no matter how
48+
* many plugins referenced it — the operator installs it once to fix all of
49+
* them — and the count is taken from the deduplicated list so the number can
50+
* never disagree with the names printed beside it.
4651
*/
4752
export function formatPluginWarningsSummary(
4853
warnings: readonly string[],
4954
): string | undefined {
5055
if (warnings.length === 0) return undefined;
5156

52-
const skillMisses: string[] = [];
57+
const missedSkills = new Set<string>();
58+
let skillMissWarnings = 0;
5359
for (const w of warnings) {
5460
const m = /skill "([^"]+)" referenced but not found/.exec(w);
55-
if (m?.[1] !== undefined) skillMisses.push(m[1]);
61+
if (m?.[1] === undefined) continue;
62+
skillMissWarnings += 1;
63+
missedSkills.add(m[1]);
5664
}
5765

58-
if (skillMisses.length > 0 && skillMisses.length === warnings.length) {
59-
const n = skillMisses.length;
60-
return `plugins: ${n} skill${n === 1 ? "" : "s"} missing: ${skillMisses.join(", ")}`;
66+
const names = [...missedSkills];
67+
const n = names.length;
68+
69+
if (n > 0 && skillMissWarnings === warnings.length) {
70+
return `plugins: ${n} skill${n === 1 ? "" : "s"} missing: ${names.join(", ")}`;
6171
}
6272

63-
if (skillMisses.length > 0) {
64-
const n = skillMisses.length;
65-
const other = warnings.length - n;
66-
return `plugins: ${n} skill${n === 1 ? "" : "s"} missing (${skillMisses.join(", ")}); ${other} other warning${other === 1 ? "" : "s"}`;
73+
if (n > 0) {
74+
const other = warnings.length - skillMissWarnings;
75+
return `plugins: ${n} skill${n === 1 ? "" : "s"} missing (${names.join(", ")}); ${other} other warning${other === 1 ? "" : "s"}`;
6776
}
6877

69-
const n = warnings.length;
70-
return `plugins: ${n} warning${n === 1 ? "" : "s"} during load`;
78+
const total = warnings.length;
79+
return `plugins: ${total} warning${total === 1 ? "" : "s"} during load`;
7180
}
7281

7382
/**

src/tui-opentui/landing.test.ts

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {
1717
isLanding,
1818
paintLanding,
1919
streamRowCount,
20-
surfaceStartupNotice,
20+
surfaceSystemNotice,
2121
} from "./shell"
2222
import { makeOperatorQuestion, openOperatorOverlay } from "./overlays"
2323
import {
@@ -149,13 +149,18 @@ describe("landing screen", () => {
149149
mark.length,
150150
)
151151
expect(painted.indexOf(mark.at(-1) as string)).toBeLessThan(top)
152-
// The two doors sit beside the mark, not under it.
152+
// The two doors sit beside the mark, not under it, and their
153+
// descriptions share one column — ragged, the pair reads as two
154+
// unrelated lines rather than as a set.
155+
const descriptionColumns = new Set<number>()
153156
for (const hint of LANDING_HINTS) {
154157
const row = painted.find((line) => line.includes(hint.rest))
155158
expect(row).toBeDefined()
156159
expect(row).toContain(hint.key)
157160
expect(row!.indexOf(hint.key)).toBeGreaterThan(0)
161+
descriptionColumns.add(row!.indexOf(hint.rest))
158162
}
163+
expect(descriptionColumns.size).toBe(1)
159164
// The version sits with the hints, and cannot drift from package.json.
160165
expect(LANDING_VERSION).toBe(`v${pkg.version}`)
161166
expect(h.captureCharFrame()).toContain(LANDING_VERSION)
@@ -513,7 +518,7 @@ describe("landing screen", () => {
513518

514519
const mcpError =
515520
"mcp github did not connect (ECONNREFUSED) — its tools are unavailable; /mcp for detail"
516-
surfaceStartupNotice(shell, mcpError)
521+
surfaceSystemNotice(shell, mcpError)
517522
await settle(h)
518523

519524
// The mountain stays; the notice strip carries the wording.
@@ -541,4 +546,63 @@ describe("landing screen", () => {
541546
}
542547
}, SIZE)
543548
})
549+
550+
test("startup plugin diagnostics keep the mountain too", async () => {
551+
// CL-5718: CL-5618 routed MCP and hook notices away from the transcript
552+
// but left plugin diagnostics going through the runner's own system-row
553+
// helper, so any missing skill wiped the whole hero on load. The flush is
554+
// a named seam now precisely so no producer of a startup diagnostic gets
555+
// to decide this again.
556+
await withTestRenderer(async (h) => {
557+
const shell = createAppShell(h.renderer, {
558+
terminal: { columns: 80, rows: 24 },
559+
wireKeys: false,
560+
run: "idle",
561+
})
562+
try {
563+
await settle(h)
564+
expect(isLanding(shell)).toBe(true)
565+
const before = markRows(h)
566+
expect(before.length).toBeGreaterThan(0)
567+
568+
const summary = "plugins: 3 skills missing: brand-identity, style, philosophy"
569+
surfaceSystemNotice(shell, summary)
570+
await settle(h)
571+
572+
expect(isLanding(shell)).toBe(true)
573+
expect(markRows(h).length).toBe(before.length)
574+
expect(streamRowCount(shell)).toBe(0)
575+
expect(noticeText(shell)).toContain("3 skills missing")
576+
} finally {
577+
shell.dispose()
578+
}
579+
}, SIZE)
580+
})
581+
582+
test("a flushed startup notice never carries a plumbing gutter label", async () => {
583+
// The transcript must never label a row "command": a system row's text
584+
// already says what it is, and the meta column is the operator's, not the
585+
// wiring's.
586+
await withTestRenderer(async (h) => {
587+
const shell = createAppShell(h.renderer, {
588+
terminal: { columns: 80, rows: 24 },
589+
wireKeys: false,
590+
run: "idle",
591+
})
592+
try {
593+
await settle(h)
594+
surfaceSystemNotice(shell, "plugins: 1 skill missing: style")
595+
appendStreamRow(shell, { role: "user", text: "first prompt" })
596+
await settle(h)
597+
598+
expect(isLanding(shell)).toBe(false)
599+
const frame = h.captureCharFrame()
600+
expect(frame).toContain("1 skill missing")
601+
expect(frame).not.toContain("command")
602+
expect(frame).not.toContain("overlay")
603+
} finally {
604+
shell.dispose()
605+
}
606+
}, SIZE)
607+
})
544608
})

src/tui-opentui/landing.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,25 @@ export const LANDING_HINTS: readonly {
6565
{ key: "?", rest: "for shortcuts" },
6666
]
6767

68+
/**
69+
* Columns held for the key, so the descriptions beside them start on one
70+
* column. Ragged, the pair reads as two unrelated lines rather than as a set.
71+
*/
72+
export const LANDING_KEY_WIDTH = LANDING_HINTS.reduce(
73+
(widest, hint) => Math.max(widest, hint.key.length),
74+
0,
75+
)
76+
77+
/** Air between the key column and the description it labels. */
78+
const LANDING_KEY_GAP = 2
79+
6880
/** Columns the hint block needs, its longest line deciding. */
6981
export const LANDING_HINT_WIDTH = Math.max(
70-
LANDING_HINTS.reduce((widest, hint) => Math.max(widest, hint.key.length + 1 + hint.rest.length), 0),
82+
LANDING_HINTS.reduce(
83+
(widest, hint) =>
84+
Math.max(widest, LANDING_KEY_WIDTH + LANDING_KEY_GAP + hint.rest.length),
85+
0,
86+
),
7187
LANDING_VERSION.length,
7288
)
7389

@@ -336,17 +352,30 @@ function createHintBlock(ctx: CliRenderer): BoxRenderable {
336352
backgroundColor: UI.ground,
337353
})
338354
LANDING_HINTS.forEach((hint, index) => {
355+
const gap = " ".repeat(
356+
LANDING_KEY_WIDTH - hint.key.length + LANDING_KEY_GAP,
357+
)
339358
block.add(
340359
new TextRenderable(ctx, {
341360
id: `shell-landing-hint-${index}`,
342361
height: 1,
343362
content: new StyledText([
344363
fgChunk(UI.text)(hint.key),
345-
fgChunk(UI.textDim)(` ${hint.rest}`),
364+
fgChunk(UI.textDim)(`${gap}${hint.rest}`),
346365
]),
347366
}),
348367
)
349368
})
369+
// The build is a fact about what is running, not a third door. Flush against
370+
// the two keys it read as one of them.
371+
block.add(
372+
new TextRenderable(ctx, {
373+
id: "shell-landing-version-gap",
374+
height: 1,
375+
content: "",
376+
fg: UI.ground,
377+
}),
378+
)
350379
block.add(
351380
new TextRenderable(ctx, {
352381
id: "shell-landing-version",
@@ -364,7 +393,9 @@ function createHintBlock(ctx: CliRenderer): BoxRenderable {
364393
*/
365394
export function fitLandingMark(above: LandingAbove, grid: MarkGrid | null): void {
366395
above.grid = grid
367-
const rows = grid?.rows ?? LANDING_HINTS.length + 1
396+
// With no mark, the hero is exactly the hint block: the two keys, the blank
397+
// row, and the version.
398+
const rows = grid?.rows ?? LANDING_HINTS.length + 2
368399
above.hero.height = rows
369400
above.markColumn.visible = grid !== null
370401
above.markColumn.width = grid?.cols ?? 0

0 commit comments

Comments
 (0)