feat: refactor the Skills detail pane into collapsible sections with an internally-scrolling file viewer

[cliffhall]

Closes #2263

Refactors the Skills detail pane. Before this, the whole pane was one DetailScroll column: the SKILL.md sat below the manifest table behind a View SKILL.md button, the frontmatter block pushed it further out of view, and nothing could be put away.

Now the pane is a flex column that does not scroll as a whole. Conformance, Resources, Frontmatter and Skill Resource are four collapsible sections of one accordion, and the file viewer claims whatever height the others leave and scrolls internally.

The layout bug this had to solve

The obvious implementation — give the viewer flex: 1 1 auto so it fills the remainder — is wrong, and wrong in a way that only shows up with a realistic file. flex-basis: auto makes an item's basis its content height, and this panel's content is a rendered document: 1,795px for the data-analysis fixture. That basis joins the sum the container distributes, the column becomes wildly over-constrained, and the shrink factors crush the siblings. Measured:

item[Conformance]     h=50   panel h=2      (content 108)
item[Resources]       h=0    panel h=0      (content 184)
item[Skill Resource]  h=586                 (content 1795)

Conformance got a 2px panel and Resources zero height, both spilling over the headers below. Collapsing the viewer removed the giant basis and everything laid out correctly, so the symptom appeared on collapse-then-reopen rather than on load.

Two things fix it, and both are load-bearing:

• viewerFlex gives the viewer a basis of 0, so it contributes nothing to that sum and simply takes what the others leave.
• Every open section carries a mih floor. The disclosure CSS sets min-height: 0 on an active item — necessary for its panel to scroll rather than overflow, but also what permits the collapse to nothing. A Mantine mih prop is an inline style, so it bounds the shrink without touching the shared stylesheet.

Verified by measurement rather than by eye — the four section heights sum to exactly the container height in both configurations, and collapse→reopen now returns to identical geometry:

	Conformance	Resources	Frontmatter	Skill Resource	Σ (container 683)
three open	156	195	48 (closed)	284	683
all four open	156	195	133	199	683

Frontmatter follows the displayed file

The Frontmatter section used to render selected.frontmatter — a property of the skill, not of the file on screen — so switching to reference.md left SKILL.md's fields sitting there.

Both halves now come from one splitSkillFile(text) call: the section renders the frontmatter half, the viewer renders the body half. Deriving both from a single split is what makes them incapable of disagreeing, and a file with no frontmatter renders no section at all rather than leaving a stale one behind.

It shows the raw YAML the server served, not a re-serialised object — this repo carries no YAML parser, and for a conformance tool the bytes on the wire are the more useful answer anyway: a file that disagrees with what the server listed is exactly what you want to see.

Two conservatisms, because a display helper must never eat content: only a fence at the very start of the file counts (a --- elsewhere is a horizontal rule), and a file that opens with --- but never closes the fence is not frontmatter and is returned whole rather than truncated to nothing.

The rest

• View SKILL.md is gone. The skill's own file loads on selection. SEP-2640 is explicit that a resources/read of a SKILL.md is not a load and confers no standing, so this claims nothing on the user's behalf — but it is a deliberate change from Support the Skills extension (SEP-2640): skills/list, skills/get, and digest verification #2234's "fetch only what is asked for" posture, called out here rather than absorbed. The read is an effect; useValueChange still drops the previous skill's results during render, so there is no stale frame for the effect to fix.
• The manifest is interactive. highlightOnHover, and each URI is a full-width, left-aligned button that swaps the file in the viewer. The row on display is marked aria-current.
• The viewer heading is static — Skill Resource, with the displayed file's name to its right, so it no longer changes shape as the file changes.
• Both whole-skill actions moved to the pane header. Fetch with skills/get and Verify all act on the skill rather than on a section — and a button inside an Accordion.Control would toggle that section on its way to firing. Verify all is now reachable with Resources collapsed.
• The shield icon is off Fetch with skills/get. It is a claim about integrity, so it stays on the two controls that actually check it (Verify all and the per-row Verify). skills/get re-fetches an entry and compares it against the listing — a consistency check, not a digest verification.
• The Conformance badge goes yellow for a warnings-only entry. It was green whenever errorCount === 0, which reads as "nothing to see" and hid the only signal the section carries for a dynamic-resources or size-limit-exceeded finding.

Mantine conventions

Reviewed against the component rules in AGENTS.md while the screen was open, including code that predates this issue:

• Flat CSS properties moved out of call sites and into the theme. ThemeText gains sectionHeading, skillTitle, monoCaption and emptyState; a new src/theme/Table.ts (ThemeTable, manifest variant) takes the manifest's font size. The .withProps() constants now carry layout only.
• The accordion uses the app's existing disclosure variant (ResourceControls: sections scroll prematurely (equal height-division) + accordion chevrons point up/down instead of right/down #1462, as ResourceControls does) rather than a bespoke scroll region — pinned headers, per-panel scrolling, and the same chevron and hover treatment.
• The URI button carries only structural props (fullWidth, justify, size); the frontmatter block is wrapped in a FramedContent (Paper, withBorder) rather than styled in place.
• splitSkillFile lives in src/utils/ — a pure transform with no I/O, and a component file that also exports a function defeats React Fast Refresh.

Test servers

data-analysis's SKILL.md grows from two lines to a ~60-line document. Not decoration: every file in that fixture was short enough that the viewer never scrolled, so a regression that let the whole pane scroll again would have looked identical to the fix. Digest and size are both derived from the text, so the fixture stays self-consistent and still verifies green.

Testing

npm run local:gate passes. New and changed code clears ≥90 on all four coverage dimensions.

Conformance is the one place a verdict is reported

The screen had three sources of judgement rendered in three different places: static findings in Conformance, per-file digest verification beside the manifest table, and the skills/get comparison in a region of its own. They now all render in the Conformance section.

That is what resolves Copilot's first finding structurally rather than cosmetically — see the review thread.

• A red N digest mismatch(es) badge joins the errors/warnings badge in the header. Kept separate because it is a runtime result; folding it into the error count would make that number change meaning after a click. It appears only once something has failed, since a standing "0 digest mismatches" would read as a verified result before anything had been checked.
• Conformance auto-collapses for an entry with no findings — its badge already carries the whole answer — and auto-opens whenever a verification, a fetch, or a new selection gives it something to say. tampered-notes is the case that forces this: structurally clean, bytes wrong.
• The badge is yellow for a warnings-only entry, not green. Green read as "nothing to see" for the one signal a dynamic-resources or size-limit-exceeded finding carries.

A dynamic skill states its case once

resources: "dynamic" produced two banners saying the same thing, plus an empty Resources section whose only content explained its own emptiness. The prose banner now renders in Conformance and the Resources section is dropped entirely — header and all, not merely emptied. The finding still counts toward the warning total, because suppressing the count would make a dynamic skill indistinguishable from a verified one.

Copy and ordering

• "This entry breaks a requirement of its own" implied the entry defines the requirement. It does not — SEP-2640 does, and the entry fails it. Now: "The returned entry does not conform to SEP-2640, so this is not simply a skill that changed since it was listed."
• A skills/get entry's findings moved below the JSON they annotate, so each one reads as an annotation on what you have just been shown.

Table and controls

• Verify has its own column. Sharing a cell with the verdict badge staggered the buttons, because badge width tracks its label — —, checking…, verified and mismatch are all different sizes.
• Both header actions take ListChangedIndicator's Refresh style (sm + subtle), and the sidebar Refresh loses a glyph no other list has.
• Expand/collapse-all uses the shared ListToggle element — the same one ResourceControls puts on its disclosure accordion — rather than a bespoke button.

Before / After

Before. One scrolling column: the manifest, then the skills/get result, then SKILL.md behind a View SKILL.md button, then the frontmatter — everything competing for the same scroll.

After. Collapsible sections, the file viewer taking the remaining height and scrolling on its own, and the frontmatter shown once — split out of the file on display rather than repeated inside it.

Screenshots

Captured headlessly against the built prod bundle connected to test-servers/configs/skills-http.json — the views #2251 documented, re-shot against the refactored pane.

The Skills tab. Four skills over two skills/list pages; the Protocol panel shows both calls, which is what proves the cursor walk ran.

A conforming skill, verified. Both files hash to the digests the manifest advertised. Conformance is collapsed here — the entry is clean, and its badge says so.

skills/get, the extension's second required method. The verdict renders in Conformance, which the fetch opens; the findings sit below the entry they annotate.

A digest mismatch. notes.md advertises a well-formed digest of bytes the server does not serve. The row badge flips to MISMATCH, the header gains 1 DIGEST MISMATCH(ES), and the alert names the file with both digests.

resources: "dynamic". One banner, in Conformance, and no Resources section at all. The badge is yellow — the entry's one finding is a warning.

A name/path disagreement. Served from wrong-folder/ while claiming the name right-name — the one structural invariant SEP-2640 states outright.

Connection Info. Unchanged by this PR, included for parity with #2251's set.

🤖 Generated with Claude Code

https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB

[Copilot]

🟡 Changes recommended

Resource MIME/frontmatter handling and an unbounded results panel can corrupt displayed content or collapse the main accordion.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Refactors the web Skills detail pane into collapsible, independently scrolling sections with an interactive resource viewer.

Changes:

• Adds the accordion-based layout, automatic SKILL.md loading, and clickable manifest resources.
• Splits raw frontmatter from displayed Markdown and adds supporting theme variants.
• Expands fixtures and tests for scrolling, selection, concurrency, and frontmatter behavior.

File summaries

File	Description
test-servers/src/skills.ts	Expands the data-analysis fixture for realistic scrolling.
clients/web/src/utils/splitSkillFile.ts	Adds frontmatter/body splitting.
clients/web/src/utils/splitSkillFile.test.ts	Tests frontmatter parsing edge cases.
clients/web/src/theme/theme.ts	Registers the Table theme.
clients/web/src/theme/Text.ts	Adds Skills typography variants.
clients/web/src/theme/Table.ts	Adds manifest table styling.
clients/web/src/theme/index.ts	Exports the Table theme.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx	Implements the redesigned detail pane and resource viewer.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx	Updates and expands screen behavior tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Fixed, though structurally rather than with the cap you suggested — flagging that so the difference is visible rather than implied.

You were right about the mechanism: that region's basis was its content while the accordion's was 0, so a large fetched entry starved the accordion and left its mih items overflowing. I first took the suggestion literally and added ScrollArea.Autosize with mah="50%", following ToolResultPanel's NonLinkCap.

It was then pointed out in review that the skills/get verdict is a conformance statement and belongs in the Conformance section rather than in a region of its own. Moving it there deletes the competing region entirely — FetchResultScroll and its cap are gone, and the content is now bounded by the accordion's own scrolling root. The failure mode you identified can no longer occur, because the two siblings that were competing are now one.

The same class of bug had already bitten this PR once: the file viewer originally used flex: 1 1 auto, whose content-sized basis crushed Conformance to a 2px panel and Resources to zero height. viewerFlex documents that, and it is why the finding read as credible immediately.

[cliffhall]

Fixed as suggested — both halves of this were real.

Splitting now keys on an effective MIME rather than on the payload arm:

• inferMimeFromUri is extracted out of ResourcePreviewPanel into clients/web/src/utils/inferMimeFromUri.ts and shared by both screens, so the suffix table has one copy rather than two that can drift. The Skills screen resolves preview.mimeType ?? inferMimeFromUri(uri) ?? "text/markdown" — markdown as the last resort rather than octet-stream, since skill files are markdown by construction under SEP-2640.
• Only a markdown resource is split, so a textual multi-document YAML file keeps its first document.
• A markdown resource served as a base64 blob is decoded with tryDecodeBase64ToUtf8 and split like a text one, so it loses its frontmatter to the Frontmatter section and does not keep it in the viewer.

One consequence worth naming: the split and the Frontmatter section are now driven by a single splitSkillFile call, so the section cannot show one file's frontmatter beside another file's body, and a file with no frontmatter renders no section at all rather than leaving the previous file's on screen. Covered by splitSkillFile.test.ts (fence-at-start only, unterminated fence returns the file whole, CRLF, frontmatter-only) and by a screen test that switches between a file with frontmatter and one without.

[cliffhall]

Copilot review round 1 — both findings addressed

Mirrored here because inline replies get hidden once the fixing commit is pushed.

1. Unbounded skills/get results collapsing the accordion — fixed, but not with the suggested cap, so flagging the difference rather than letting "fixed" read as compliance.

The mechanism was exactly as described: that region's basis was its content while the accordion's was 0, so a large fetched entry starved the accordion. I first applied the suggestion literally (ScrollArea.Autosize + mah="50%", following ToolResultPanel's NonLinkCap). Review then established that a skills/get verdict is a conformance statement and belongs in the Conformance section, and moving it there deletes the competing region outright — FetchResultScroll and its cap are both gone. The failure mode can no longer occur because the two competing siblings are now one.

Worth noting the same class of bug had already bitten this PR: the file viewer originally used flex: 1 1 auto, whose content-sized basis crushed Conformance to a 2px panel and Resources to zero height. viewerFlex documents that.

2. Frontmatter split keyed on the payload arm rather than the resource type — fixed as suggested.

Splitting now keys on an effective MIME. inferMimeFromUri is extracted out of ResourcePreviewPanel into src/utils/inferMimeFromUri.ts and shared, so the suffix table has one copy. Only markdown is split, so a multi-document YAML file keeps its first document; and a markdown resource served as a base64 blob is decoded and split like a text one.

Also in this push, from maintainer review

The pane reported judgement from three sources in three places; all three now render in Conformance, with a red N digest mismatch(es) badge, auto-collapse for a clean entry and auto-open whenever there is something to say. A dynamic skill states its case once. Verify moved to its own table column so the buttons align. Copy fixed where it implied the entry defines the rule it breaks.

Two defects the gate caught, recorded so they are not re-litigated

• inferMimeFromUri.ts landed at 75% branch coverage — extracting a helper into a new file gives it its own per-file gate, and it arrived without a test. Now 100% on all four dimensions.
• The new action column shipped with an empty <th>, which is an axe empty-table-header violation — an unnamed column in header navigation. My code comment had asserted the empty header was deliberate; that was wrong. It now carries a VisuallyHidden "Actions" label.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

MIME handling can misrender valid resources, mismatch labeling is inaccurate, and the central layout regression lacks browser coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 4
• Review effort level: Balanced

[cliffhall]

Copilot review round 2 — all four findings addressed (418448ed)

Mirrored here because inline replies get hidden once the fixing commit is pushed.

1. Markdown fallback applied to every manifest resource. Correct, and the reasoning was wrong rather than the code: markdown is right for a skill's own SKILL.md (SEP-2640 makes it markdown by construction) and wrong for the rest of a manifest, which the SEP expects to carry supporting scripts, examples and assets with their own types. An extensionless untyped blob was being decoded and rendered as markdown. The fallback is now scoped to the skill's own file; anything else yields undefined and lets ContentViewer apply its octet-stream/text default.

2. The mismatch badge misreported size failures. verifySkillResource reports both size and digest disagreements as status: "mismatch", so a size-only failure was counted under a digest label. Of the two remedies offered I took the rename, not the filter: filtering to actualDigest !== undefined would drop size failures from the count, and hiding a real verification failure is worse than mislabelling one. The badge now reads N mismatch(es); the alerts still distinguish the two by title.

⚠️ @cliffhall — this changes the wording you asked for ("X Digest Mismatches"). Flagging rather than silently altering it; happy to go back to a digest-only count if you'd rather, but then a size mismatch needs its own badge or it disappears from the header.

3. isMarkdownMime rejected valid MIMEs. The most dangerous of the four, because it failed silently: text/markdown; charset=utf-8 is ordinary and ContentViewer accepts it, but the raw equality check rejected it, which skipped the split — the YAML fence stayed in the rendered document and the Frontmatter section disappeared, with nothing erroring. Now normalises to the base lowercase type, with parameter and casing cases added to the helper test.

4. No browser-level regression assertion for the layout. The best catch of the four — the behaviour motivating the entire refactor had no guard. Added LongSkillDocument, modelled on ResourcesScreen's ManyResources, which supplies a ~40-paragraph SKILL.md and asserts: every section keeps non-zero height; sections tile without overlapping the header below; the viewer scrolls within its own panel; the detail pane does not scroll as one column; and collapse→reopen restores identical geometry — the sequence the original bug needed to reveal itself.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

The detail column lacks a definite height, and generic MIME declarations prevent valid Markdown frontmatter from being split.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Copilot review round 3 — one fixed, one declined (ae335823)

Mirrored here because inline replies get hidden once the fixing commit is pushed.

1. A generic declared MIME outranked URI inference — fixed. Precedence was declared ?? inferred, which contradicted the policy the extracted helper itself documents: servers commonly serve a file as text/plain or application/octet-stream, so the suffix is the better signal. A valid SKILL.md served with either generic type was therefore not recognised as markdown — its YAML stayed in the viewer and the Frontmatter section disappeared.

A specific declared type still wins (a server answering text/csv knows its own resource); only the generic ones yield to the suffix, via isGenericMime, normalised for parameters and casing. Tested both directions at the screen level: text/plain on a SKILL.md splits, text/csv on a .md URI does not.

2. "DetailCard is not a flex container, so the column has no definite height" — declined as a false positive. Full evidence is on the thread; in short:

• Mantine's own Card.css sets display: flex; flex-direction: column on the card root, so the theme's default branch not restating it is immaterial.
• DetailCard still carries h: "100%" — DetailScroll was removed, but it was never the source of the height.
• Measured live: DetailCard h=844 → DetailColumn h=802 → AccordionRoot h=683, with the four sections summing to exactly 683.
• LongSkillDocument (added last round, in response to round 2's finding that this had no browser-level guard) asserts the exact behaviour claimed broken — viewer scrolls internally, detail pane does not scroll as one column — in a real browser, and passes. Were there no definite height, that second assertion would fail.

Per AGENTS.md, not every suggestion needs implementing where there is a good reason; happy to revisit if there is a case those measurements miss.

npm run local:gate passes.

[Copilot]

🔵 Needs a closer look

Initial clean selections retain incorrect accordion state, and the Resources panel contains an unreachable branch.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:542

• useValueChange intentionally skips the first render, so this all-open initializer leaves Conformance expanded when the screen mounts with an already-selected clean skill (for example, a restored SkillsUiState). The promised clean-entry auto-collapse only works after a later selection change. Seed this state from the initially selected entry’s findings, then keep the existing render-time synchronization for subsequent changes; cover the initially-selected case directly.

This issue also appears on line 1311 of the same file.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1311

• This condition is unreachable because the enclosing render guard is !isDynamic, while isDynamic is defined by this exact comparison. Remove the dead dynamic branch and render the manifest table directly; the dynamic explanation already lives in Conformance as intended.

                      {selected.resources === DYNAMIC_RESOURCES ? (

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 4 — both findings addressed (d8f0e772)

Posted at PR level because this round produced 0 inline comments; both findings arrived in the review's Suppressed comments block, so there are no threads to reply to. Worth stating plainly: a round reporting "0 new comments" was carrying two real defects, and reading only the inline list would have ended this cycle with both shipped.

1. The clean-entry auto-collapse never applied on mount (SkillsScreen.tsx:542). Correct, and the gap was in my reasoning rather than the code: useValueChange deliberately does not fire on the first render, so a screen mounting with a skill already selected — a restored SkillsUiState is exactly that — showed Conformance expanded, and the documented rule only began working after some later selection change. I wrote the rule entirely in the change handler and never considered the initial state.

openSections now seeds through initialOpenSections(skills, selectedSkillUri), applying the same judgement up front, while the render-time synchronisation continues to handle every subsequent change. Two tests mount directly on a selection — one clean (collapsed), one with findings (expanded) — rather than reaching the state by clicking, so they actually exercise the initial path.

2. Unreachable dynamic branch (SkillsScreen.tsx:1311). Also correct. The Resources panel still held selected.resources === DYNAMIC_RESOURCES ? <Alert/> : <ManifestTable/> inside a {!isDynamic && …} guard, and isDynamic is that exact comparison — so the alert arm could never render. It is residue from moving the dynamic banner into Conformance: I added the outer guard and left the inner ternary. The manifest table now renders directly; 8 lines of dead JSX removed.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Non-shrinking metadata sections can push the resource viewer below large valid manifests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Copilot review round 5 — addressed (fa03c74b)

One finding, and the sharpest of the review: it caught a fix that traded one bug for another.

Non-shrinking metadata sections push the viewer below a large manifest. SECTION_FLEX was changed to 0 0 auto earlier in this PR to stop a squeezed section slicing its content mid-line — a Digest mismatch alert cut in half by the header beneath it, which read as broken because a macOS overlay scrollbar is invisible until hover. That worked, and reintroduced the original defect at scale: a conforming manifest may declare 512 rows, and a section holding its full intrinsic height pushes the file viewer off the bottom of the pane. Reaching the file then means scrolling past the manifest — which is the "the file is behind the manifest" problem this refactor exists to end.

0 1 auto with the existing OPEN_SECTION_MIN_HEIGHT floor satisfies both: a section gives up space until it hits the floor, its panel scrolls internally from there, and the viewer keeps the remainder. The floor is what stops the shrink becoming a crush — a panel at the floor scrolls rather than clips. Both wrong directions are recorded on the constant now, so neither is re-derived.

Added LongManifest, the sibling story to LongSkillDocument, with a 121-file manifest. It asserts the viewer's header stays within the card, the Resources panel scrolls internally, and the pane does not scroll as one column — and it is verified to detect the defect rather than merely pass:

SECTION_FLEX	Long Manifest	other 8 stories
0 1 auto (fixed)	pass	pass
0 0 auto (defect)	fail	pass

npm run local:gate passes.

---

Where this review stands

round	findings	outcome
1	2	both fixed
2	4	all fixed
3	2	1 fixed, 1 declined with evidence (Mantine's own Card.css supplies the flex container)
4	2 (suppressed block; 0 inline)	both fixed
5	1	fixed

11 findings, 10 fixed. Every fix gated before pushing.

[Copilot]

🟡 Changes recommended

An unbounded resource filename can still disrupt the accordion layout, and one text constant bypasses the theme-variant convention.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:297

• size="sm" leaves typography in the component even though this refactor moves Text typography into ThemeText variants (see clients/web/src/theme/Text.ts:54-79) and the comment above says these constants carry layout only. Add a skillDescription Text variant and keep only the clamping behavior here.

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Copilot review round 9 — both addressed (1a2e3753)

1. The resource file name was unbounded in a pinned control. Fifth instance of the class, and the first where the fix was genuinely awkward — the Skill Resource control never scrolls, so it cannot absorb a long name the way a panel can. Three attempts, each exposed by re-running the guard against the un-fixed code:

attempt	control growth
lineClamp: 1	+30.6px — text stopped wrapping, the row wrapped the caption onto its own line
+ nowrap row	+21.7px — Mantine's Accordion label wrapper still would not let it shrink
+ maw: "50%"	0px

Flexbox wraps before it shrinks, and min-width: 0 only helps when every ancestor also permits shrinking; a hard cap gives the ellipsis a definite width regardless.

⚠️ The first version of that guard passed against the defect — it asserted an absolute height < 120px, which an unclamped name satisfies because it wraps to a few lines, not dozens. It now measures the control with a short name and asserts the height is unchanged after selecting the hostile one. Without re-running it against the un-fixed code I would have reported a regression test that proved nothing.

2. SkillDescription kept size: "sm" at the call site — typography left in the component, directly beneath a comment of mine asserting that these constants carry layout only and that typography belongs in a ThemeText variant. Added a skillDescription variant; only the clamp stays.

npm run local:gate passes. 495 unit tests, 10 stories.

---

round	findings	outcome
1	2	both fixed
2	4	all fixed
3	2	1 fixed, 1 declined with evidence
4	2 (suppressed; 0 inline)	both fixed
5	1	fixed
6	4	all fixed
7	4	all fixed
8	1	fixed, + 1 self-found a11y defect
9	2	both fixed

22 findings, 21 fixed.

[Copilot]

🔵 Needs a closer look

Raw frontmatter can be reformatted and the consolidated verdict UI contains contradictory or incomplete semantics.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1583

• This does not always display the served YAML verbatim. ContentViewer routes text/yaml through its plain-text branch, which auto-detects and pretty-prints JSON (ContentViewer.tsx:256-271). Since JSON mappings are valid YAML, frontmatter such as {"name":"a","description":"b"} is reserialized despite this section promising raw bytes. Use a raw-text mode/renderer that disables the JSON heuristic for frontmatter.
  clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1273
• After a digest/size mismatch, this section shows a green “Conforms” alert alongside the red runtime failure. Now that all verdicts live in Conformance, the unqualified title contradicts the verification result; use “No structural issues” so it accurately describes only the static check summarized by the body.
  clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1517
• The PR description says the shield remains on both integrity-checking controls, but the per-row RowVerifyButton still renders only the text label. Add MdVerifiedUser to the shared row-button definition so the stated icon semantics are applied consistently to “Verify all” and each row’s “Verify”.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 10 — all three addressed (10c8f5de)

Posted at PR level: this round produced 0 inline comments and three findings in the Suppressed comments block, so there are no threads to reply to. That is the second such round here — the inline count alone would have ended this cycle twice with real defects shipped.

All three are places where my own words did not match the code.

1. The "raw bytes" promise was false. The Frontmatter section carried a comment saying it shows "the raw YAML the server served, not a re-serialised object". ContentViewer's plain-text branch pretty-prints anything that parses as JSON (ContentViewer.tsx:256-271), and every JSON mapping is also valid YAML — so a frontmatter of {"name":"a","description":"b"} came back re-serialised, directly beneath a comment promising verbatim bytes. For a conformance tool, silently reformatting what the server sent is the one thing this section must not do. It now renders through CodeHighlight directly, which does not inspect the content.

2. "Conforms" contradicted the mismatch beside it. Consolidating every verdict into Conformance means the green alert can sit immediately above a red Digest mismatch. An unqualified "Conforms" then contradicts what is on screen — the static checks passing says nothing about the bytes. Retitled "No structural issues", which is exactly what its body summarises. Notably this contradiction was created by the consolidation earlier in this PR, and I never revisited the title.

3. The PR description was ahead of the code. It stated the shield stays on "the controls that actually check integrity — Verify all and the per-row Verify". The per-row button rendered text only. The claim was the correct one, so the button now carries the icon; giving only one of two identical operations an icon made it look decorative.

npm run local:gate passes. 495 unit tests, 10 stories.

---

round	findings	outcome
1	2	both fixed
2	4	all fixed
3	2	1 fixed, 1 declined with evidence
4	2 (suppressed; 0 inline)	both fixed
5	1	fixed
6	4	all fixed
7	4	all fixed
8	1	fixed, + 1 self-found a11y defect
9	2	both fixed
10	3 (suppressed; 0 inline)	all fixed

25 findings, 24 fixed.

[Copilot]

🔵 Needs a closer look

StrictMode duplicates automatic reads, and scroll positions persist across skill changes.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:912

• SkillsScreen runs under StrictMode (main.tsx:13) and ScreenStage remounts it while the selected-skill UI state persists. On such a remount this effect is replayed, so it issues two identical resources/read RPCs and records duplicate Protocol entries. Track the automatically loaded manifest key in a ref so the StrictMode replay is a no-op.

This issue also appears on line 1225 of the same file.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1226

• The accordion and its native scroll containers are reused when the selected skill changes, so a long Resources or Conformance panel retains its old scrollTop. Switching to another long skill can therefore show its manifest/findings partway down and hide the beginning. Keying the accordion by manifestKey resets those DOM scroll positions while preserving the controlled openSections state.

            <Accordion
              multiple

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 11 — both addressed (ede9658d)

Posted at PR level: 0 inline comments, both findings in the Suppressed comments block. Third such round here.

1. The automatic SKILL.md read fired twice. I had actually noticed this possibility while writing the effect and dismissed it in a note as "harmless — a duplicate resources/read in dev". That was wrong twice over. ScreenStage remounts this screen while the selected-skill UI state persists, so it is not dev-only; and this app is a protocol inspector, where a phantom request in the Protocol panel that the user's own client would never send is the tool misreporting the conversation, not merely a wasted round trip. An autoReadKey ref now makes the replay a no-op.

2. Panel scroll positions survived a skill change. scrollTop lives on the DOM node, not in React state, so switching between two long manifests showed the new one part-way down with its first rows hidden — indistinguishable from missing data. The accordion is keyed by manifestKey, giving every panel a fresh scroll container; openSections is controlled, so it survives the remount.

The test for finding 1 is a specification, not a regression guard

Stating this plainly because a passing test would otherwise imply more than it should. I wrote the test, then removed the guard to check it — and it still passed. Three shapes were tried:

test shape	detects missing guard?
plain re-render with identical props	no
wrapped in <StrictMode>, select after mount	no
wrapped in <StrictMode>, selection present at mount	no

This environment does not double-invoke mount effects, so the replay cannot be reproduced here; the guard is only observable in a real dev-mode browser. The test therefore pins the contract — one automatic read per selection — and carries a comment saying explicitly that a pass is not evidence the duplicate-read defect is fixed. The fix stands on the reasoning in the finding, which I agree with.

npm run local:gate passes. 496 unit tests, 10 stories.

---

round	findings	outcome
1–3	8	7 fixed, 1 declined with evidence
4	2 (suppressed; 0 inline)	both fixed
5–7	9	all fixed
8	1	fixed, + 1 self-found a11y defect
9	2	both fixed
10	3 (suppressed; 0 inline)	all fixed
11	2 (suppressed; 0 inline)	both fixed

27 findings, 26 fixed.

[Copilot]

🟡 Changes recommended

Automatic-read state is mishandled across screen remounts and temporarily missing selected entries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:927

• When the selected skill temporarily disappears from skills, selectedUri becomes undefined and the render invalidates the preview, but this return leaves the old manifest key cached. If the identical entry later reappears, its key matches the cache, so no read runs and the viewer remains empty. Reset the guard while no selected entry exists.

  useEffect(() => {
    if (selectedUri === undefined) return;
    if (autoReadKey.current === manifestKey) return;
    autoReadKey.current = manifestKey;
    showResource(selectedUri, manifestKey);
  }, [manifestKey, selectedUri, showResource]);

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Copilot review round 12 — both addressed

One inline comment and one suppressed, both on the same eight lines. Both are correct, and the first one retracts a claim I made in round 11.

1. autoReadKey does not survive a ScreenStage remount — and should not try to

Right, and I overstated it last round. ScreenStage unmounts inactive screens, so returning to Skills mints a fresh ref initialized to null and the read fires again. A ref cannot promise anything across a remount, so the comment I wrote claiming it guarded that path was simply false.

Of the two remedies offered I took the second, because the first would be trading a legitimate request for a cache: previewState is local state too and dies with the same unmount, so on return the viewer is empty and that read is what refills it. Suppressing it would mean hoisting the preview bytes above ScreenStage and showing bytes fetched under a connection that may no longer exist. So the read on remount is correct behavior, and what changed is the contract: the guard's scope is now stated as one mount — StrictMode effect replay — in both the code comment and the test name (issues exactly one automatic read per selection within a mount), with the remount case documented as deliberate rather than as a gap.

2. The guard stranded the viewer when an entry left the list and came back

Real bug, and the more serious of the two. When the selected entry leaves skills — a refresh in flight, a disconnect — selectedUri goes undefined, useValueChange empties the preview, and the old manifestKey stays cached in the ref. If the identical entry reappears, its key matches the cache, the read is skipped, and the viewer is blank with no way to recover short of reselecting. Clearing the guard on the way out is the fix.

This one is a regression guard, not a specification. Restoring the defect (if (selectedUri === undefined) return;):

test	with fix	with defect restored
re-reads when the selected entry leaves the list and comes back	pass	fail
issues exactly one automatic read per selection within a mount	pass	pass

The second row is the non-redundancy check — the older test does not cover this path, so the new one is carrying its own weight. The older test remains a specification for the reason stated in round 11 and now says so while pointing at the new test as the actual guard.

npm run local:gate passes.

---

round	findings	outcome
1–3	8	7 fixed, 1 declined with evidence
4	2 (suppressed; 0 inline)	both fixed
5–7	9	all fixed
8	1	fixed, + 1 self-found a11y defect
9	2	both fixed
10	3 (suppressed; 0 inline)	all fixed
11	2 (suppressed; 0 inline)	both fixed
12	2 (1 inline, 1 suppressed)	both fixed; one retracts a round-11 claim

29 findings, 28 fixed.

[Copilot]

🟢 Approval recommended

The refactor satisfies the linked acceptance criteria with comprehensive unit and browser-level regression coverage.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation satisfies the issue scope with comprehensive unit and browser-level regression coverage and no unresolved defects found.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review cycle complete — two consecutive clean approvals

Rounds 13 and 14 both came back 🟢 Approval recommended, 0 comments, no suppressed block, on the identical tree — nothing was pushed between them, so the second is a confirmation rather than a re-review of new code. Two rounds rather than one because on this PR the inline comments have posted after the body more than once, and three of the last six rounds carried their only findings in the Suppressed comments block while the body read as clean.

Final tally

round	findings	outcome
1–3	8	7 fixed, 1 declined with evidence
4	2 (suppressed; 0 inline)	both fixed
5–7	9	all fixed
8	1	fixed, + 1 self-found a11y defect
9	2	both fixed
10	3 (suppressed; 0 inline)	all fixed
11	2 (suppressed; 0 inline)	both fixed
12	2 (1 inline, 1 suppressed)	both fixed; one retracted a round-11 claim of mine
13, 14	0	approval, twice

29 findings, 28 fixed, 1 declined with evidence — the round-3 DetailCard flex claim, declined because Mantine's own Card.css already sets display: flex; flex-direction: column.

The two findings worth remembering

Sizing. Five separate defects were the same shape: any server-controlled variable-height content that is a sibling of the zero-basis viewer sizes the layout. flex: 1 1 auto gave the viewer a content-sized basis and crushed Conformance to 2px; 0 0 auto fixed the visible symptom and reintroduced the original bug at 512-row scale, which round 5 caught. The answer that survives both is 0 1 auto with an explicit floor.

Guards. Two tests in this PR looked like regression guards and were not, and mutation testing is the only thing that told them apart. HostileHeader's first assertion passed against the defect until it compared against its own baseline; the StrictMode auto-read test could not be made to fail in any of three shapes, so it is labelled in code as a specification, not a guard, with a comment saying a pass is not evidence the defect is fixed. Round 12's reappearance test is the real guard — it fails with the defect restored while the older test still passes, proving both detection and non-redundancy.

npm run local:gate passes: 497 unit tests, 10 stories.