Remove nested Markdown headers from mcp-clis prompt section#35922
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
mcp-clis prompt section
There was a problem hiding this comment.
Pull request overview
This pull request normalizes the mcp-clis prompt content by removing nested Markdown headings inside the <mcp-clis>...</mcp-clis> block to avoid unintended header semantics, and adds a regression test to prevent reintroducing ##/###-style headings.
Changes:
- Removed H2/H3 Markdown headings from
actions/setup/md/mcp_cli_tools_prompt.mdand replaced them with plain label lines (e.g.,How to use:). - Added a unit test asserting the mounted MCP CLI prompt file does not contain Markdown heading syntax while preserving expected label text.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/md/mcp_cli_tools_prompt.md | Removes nested ##/### headings from the <mcp-clis> prompt content and replaces them with non-heading labels. |
| pkg/workflow/mcp_cli_mount_test.go | Adds a regression test that reads the prompt file and checks for absence of Markdown heading markers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35922 does not have the 'implementation' label and has only 24 new lines (≤100) in business logic directories. has_implementation_label=false, requires_adr_by_default_volume=false, has_custom_config=false. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correct, focused change. Two minor test-coverage gaps noted inline — both non-blocking.
Review summary
What the PR does
Removes ##/### Markdown headings from inside the <mcp-clis> prompt block and replaces them with plain-text labels. Adds a regression test that reads the actual file and asserts no headings are present.
Findings
- Missing label-presence assertions (line 228):
assert.Containsonly checks"How to use:"; the two other converted labels are not verified. - Blockquote regex depth (line 224):
(>\s*)?matches only one level of>nesting;(>\s*)*would be more correct.
Both are low-severity test completeness gaps that are easy to fix. The prompt change itself is correct and unambiguous.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1M
| prompt := string(content) | ||
| assert.NotContains(t, prompt, "\n## ") | ||
| assert.NotContains(t, prompt, "\n### ") | ||
| assert.Contains(t, prompt, "How to use:") |
There was a problem hiding this comment.
Incomplete label-presence assertions: only "How to use:" is verified; the other two converted labels are not checked.
💡 Suggested fix
If ### Notes or ### Passing Multiple or Complex Arguments (Preferred) ever regressed back to Markdown headings, the NotRegexp assertion would catch the ### — but there would be no check that the plain-text replacement (Notes: / Passing multiple or complex arguments (preferred):) is actually present. Add the two missing assertions to make the intent explicit:
assert.Contains(t, prompt, "How to use:")
assert.Contains(t, prompt, "Passing multiple or complex arguments (preferred):")
assert.Contains(t, prompt, "Notes:")Without these, a future edit could replace those labels with something else entirely and the test would still pass.
| require.NoError(t, err) | ||
| content, err := os.ReadFile(filepath.Clean(filepath.Join(wd, "../../actions/setup/md", section.Content))) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Regex covers only one > blockquote level: (>\s*)? matches at most one leading >, so headings inside nested blockquotes would pass the assertion silently.
💡 Suggested fix
Low risk for the current file, but the pattern can be tightened at negligible cost:
assert.NotRegexp(t, `(?m)^(>\s*)*##\s+`, prompt, "prompt must not contain H2 Markdown headings")
assert.NotRegexp(t, `(?m)^(>\s*)*###\s+`, prompt, "prompt must not contain H3 Markdown headings")(>\s*)* handles arbitrary nesting depth, and because ## already implies ### would also be caught by the H2 pattern, you could optionally collapse to a single check: (?m)^(>\s*)*#{2,}\s+.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Correct targeted fix: H2/H3 headings inside
<mcp-clis>carried unintended rendering semantics in structured prompt blocks. Replacing them with plain colon-terminated labels is the right normalisation. - Regression test present: The test reads the file from disk and asserts both the heading-free invariant and that the expected label text survives — good habit for prompt-content bugs.
Positive Highlights
- ✅ Tight scope — only the affected prompt file and one test, no collateral changes
- ✅ Test uses
NotRegexpwith multiline anchors to catch headings even inside blockquote context (> ##) - ✅ PR description clearly explains root cause and lists each changed label
Minor suggestion
Only How to use: is asserted present; adding the other two replaced labels (Passing multiple or complex arguments (preferred): and Notes:) would make the test a complete specification of the new format. See inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1M
| prompt := string(content) | ||
| assert.NotContains(t, prompt, "\n## ") | ||
| assert.NotContains(t, prompt, "\n### ") | ||
| assert.Contains(t, prompt, "How to use:") |
There was a problem hiding this comment.
[/tdd] The test verifies How to use: is present after the heading strip, but the other two replaced labels (Passing multiple or complex arguments (preferred): and Notes:) are not asserted.
💡 Suggested additions
Adding the two missing assertions keeps the test spec-complete and prevents the other labels from being accidentally dropped in a future edit:
assert.Contains(t, prompt, "How to use:")
assert.Contains(t, prompt, "Passing multiple or complex arguments (preferred):")
assert.Contains(t, prompt, "Notes:")Not blocking — the heading-removal invariant is the core concern — but completing the trio makes the test a full specification of the new label format.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
|
The
mcp-clisprompt content was emitting H2/H3 Markdown headings even though it is already wrapped in<mcp-clis>...</mcp-clis>. This caused unintended header rendering semantics inside a structured prompt block.Prompt content normalization
/actions/setup/md/mcp_cli_tools_prompt.mdto remove nested Markdown headers.How to use:,Passing multiple or complex arguments (preferred):,Notes:).## MCP Servers Mounted as Shell CLI Commands) from inside the block.Regression coverage
/pkg/workflow/mcp_cli_mount_test.goto assert the mounted prompt file no longer contains##/###headings while preserving expected section text.