feat: wrap skill install step summary in progressive disclosure details block - #48905
Conversation
…ive disclosure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Wraps frontmatter skill installation details in a collapsible GitHub Actions step-summary section for consistency.
Changes:
- Adds a
<details>block around installation results. - Removes the emoji from the failure heading and verifies disclosure markup.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/install_frontmatter_skills.cjs |
Builds and renders the collapsible skill summary. |
actions/setup/js/install_frontmatter_skills.test.cjs |
Verifies the summary includes disclosure markup. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business directories). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the changes are clean and consistent with the project's progressive disclosure conventions. LGTM.
📋 Review Notes
Positive Highlights
- ✅ Single
addRawcall replaces the previous multi-call pattern — simpler interface, richer behaviour - ✅
<details>wrapping is consistent with how other step-summary sections are rendered - ✅ Regression test updated to assert
<details>is present - ✅ Removing the
⚠️emoji from the failures sub-heading aligns with project conventions
Minor Observation
The new test assertion (stringContaining('<details>')) covers the happy path. There is no existing test that exercises the failures code-path after the refactor (to confirm the failure lines still appear inside the block). This is low risk given the logic is straightforward, but a complementary assertion in a failure scenario would fully close the gap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.2 AIC · ⌖ 4.82 AIC · ⊞ 6.8K
Comment /matt to run again
There was a problem hiding this comment.
The change correctly wraps skill install details in a details block for cleaner step summaries. One non-blocking but notable issue: skill install failures are now hidden inside the collapsed block, and the warning emoji was removed — reducing their visibility. Consider rendering failures outside the details block or using 'details open' when failures are present.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.5 AIC · ⌖ 4.8 AIC · ⊞ 5.1K
| core.summary.addRaw(`### Frontmatter skills installed\n\n<details>\n<summary>Skill install details</summary>\n\n${body}\n</details>\n\n`); | ||
| await core.summary.write(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Skill install failures are now hidden inside the collapsed <details> block by default, and the
Consider either:
- Keeping failures visible outside the
<details>block (only collapse the success detail), or - Reopening the
<details>block by default when failures exist:<details open>
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit — writeSkillSummary now uses <details open> when failures.length > 0, so failures are visible by default without needing to expand the block. A test assertion for this behavior was added to the failure test case.
There was a problem hiding this comment.
Verdict: no blocking issues
Simple, correct refactor: builds a body string then wraps it once in a <details> block, matching the existing pattern used elsewhere. Test coverage for the new <details> wrapper was added.
Notes considered
The grumpy sub-agent flagged that f.skill/f.error values are interpolated unsanitized into the raw HTML/markdown body, which could theoretically break out of the <details> block if a skill spec or error message contained markdown/HTML-breaking sequences. This is pre-existing behavior carried over unchanged from the original addRaw calls (same interpolation, just built differently), not a regression introduced by this diff, so it's out of scope for this review — the diff itself is low risk.
Minor, non-blocking suggestion: consider adding a test for the empty-failures case to lock in that the #### Skill install failures section is omitted correctly inside the new wrapper, but this isn't required for merge.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 23.2 AIC · ⌖ 7.07 AIC · ⊞ 7.5K
Comment /review to run again
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
Verdict
|
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The skills installation section of the activation job step summary was rendered flat, unlike other sections which use collapsible
<details>blocks.Changes
install_frontmatter_skills.cjs—writeSkillSummarynow emits a singleaddRawcall wrapping all install details (directory, requested refs, installed count, failures) in a<details><summary>Skill install details</summary>…</details>block under the### Frontmatter skills installedh3. Also removes the⚠️emoji from the failures sub-heading per project convention.install_frontmatter_skills.test.cjs— adds assertion thataddRawis called with a string containing<details>.