fix(create_issue): prevent report content ending up in issue title - #47282
Conversation
… minLength validation - safe_outputs_auto_create_issue.md: add explicit title/body guidance so auto-injected create_issue runs don't encourage agents to put report content in the title field - create_issue.cjs / safe_outputs_handlers.cjs: change the no-title fallback from using the *entire* body to using only the first non-empty line (stripping any markdown heading marker), keeping the title concise - intent_probe.cjs: mirror the same first-line fallback so probe-detection sees the same effective title as the actual handler - mcp_scripts_mcp_server_http.cjs: add missing validateStringMinLengths call (already present in mcp_server_core.cjs) so the minLength:20 constraint on body is enforced on the HTTP MCP transport path as well" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Prevents issue report content from being used as the entire issue title.
Changes:
- Derives missing titles from the first meaningful body line.
- Clarifies title/body guidance.
- Adds HTTP minimum-length validation.
Show a summary per file
| File | Description |
|---|---|
actions/setup/md/safe_outputs_auto_create_issue.md |
Clarifies issue formatting. |
actions/setup/js/safe_outputs_handlers.cjs |
Updates title fallback during collection. |
actions/setup/js/mcp_scripts_mcp_server_http.cjs |
Adds minimum-length validation. |
actions/setup/js/intent_probe.cjs |
Aligns probe title resolution. |
actions/setup/js/create_issue.cjs |
Updates issue creation fallback. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| // Validate minLength constraints from the schema. | ||
| const tooShort = validateStringMinLengths(args, tool.inputSchema); | ||
| if (tooShort.length) { | ||
| const details = tooShort.map(v => `'${v.field}' is too short (minimum ${v.minLength} characters, got ${v.actualLength})`).join(", "); | ||
| throw new Error(`Invalid arguments: ${details}`); | ||
| } |
| const firstBodyLine = (message.body ?? "") | ||
| .split("\n") | ||
| .map(l => l.replace(/^#+\s*/, "").trim()) | ||
| .find(l => l.length > 0); |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47282 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is well-structured and addresses all three root causes correctly.
Validation concern (existing comment at line 108): After reviewing the actual delegation path in mcp_http_transport.cjs, the server.tool() call wraps the handler with the new minLength validation before registering with _coreServer via registerTool. When handleRequest delegates to mcp_server_core, it calls the already-wrapped handler — so the validation does fire correctly on the HTTP path. The existing review comment appears to be based on a misreading of the delegation chain; the fix is sound.
Test coverage (existing comment at line 786): Adding a multiline-body regression test is a worthwhile follow-up, but is not blocking.
The prompt improvement to safe_outputs_auto_create_issue.md and mirroring of the fallback in intent_probe.cjs are clean and consistent.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 28.6 AIC · ⌖ 4.52 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: All three fix sites (create_issue, safe_outputs_handlers, intent_probe) lack regression tests for multiline bodies and markdown headings. The existing tests pass with a single-line body, so they would have been green even before this fix.
- HTTP minLength validation placement looks correct: The new
validateStringMinLengthscall inmcp_scripts_mcp_server_http.cjsis within the per-tool registration callback, which runs beforetool.handler. The HTTP transport delegateshandleRequesttomcp_server_core, but the schema-level validation registered here fires first on the SDK path, so the placement is sound. - Prompt guidance improvement: The update to
safe_outputs_auto_create_issue.mddirectly addresses the root-cause vagueness.
Positive Highlights
- ✅ Root cause analysis is thorough — all three contributing causes identified and fixed
- ✅ Consistent fix across create_issue, safe_outputs_handlers, and intent_probe
- ✅ Heading-stripping regex (
/^#+\s*/) is clean and covers## H2style headings - ✅ Graceful fallback chain: first non-empty line → "Agent Output"
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 34.7 AIC · ⌖ 4.84 AIC · ⊞ 6.7K
Comment /matt to run again
| // Use the first non-empty line of the body as the title fallback rather than | ||
| // the entire body, so the title stays concise and the body remains intact. | ||
| const firstBodyLine = (entry.body || "") | ||
| .split("\n") |
There was a problem hiding this comment.
[/tdd] Same test gap in safe_outputs_handlers.test.cjs — the create_issue title fallback is tested only with a single-line body. The multiline/heading scenario that triggered this bug has no coverage here either.
💡 Suggested regression cases
Add to the create_issue handler tests:
it('uses first non-empty line of body as title when title is absent', async () => {
// multiline with markdown heading
const entry = { body: '# My Report\n\nFull content here.' };
// ... call handler and assert resolved title === 'My Report'
});
it('falls back to Agent Output when body is blank', async () => {
const entry = { body: ' \n\n ' };
// ... assert resolved title === 'Agent Output'
});@copilot please address this.
There was a problem hiding this comment.
Non-blocking concerns
The core fix (first-non-empty-line title fallback + HTTP minLength gating) is reasonable, but three issues warrant attention before this approach is considered complete.
Review themes
-
Three-way code duplication — identical first-non-empty-line logic in
create_issue.cjs,safe_outputs_handlers.cjs, andintent_probe.cjswith no shared utility. Already diverging once (PR description callsintent_probe.cjsa "mirror" update — manual synchronisation that will drift). -
MCP error code lost on HTTP path —
new Error(...)for minLength violations vs the structured-32602object on the stdio path. Transport-specific error shapes break protocol consistency. -
Setext/thematic-break lines become issue titles — the ATX-only regex won't skip
---or===lines when they appear as the first non-empty line in agent-generated markdown bodies.
The existing comments about HTTP validation bypass (line 108) and missing regression tests (line 786) remain outstanding.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 41.2 AIC · ⌖ 4.92 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/create_issue.cjs:16
Duplicated logic across 3 files with no shared utility: the first-non-empty-line extraction is copy-pasted verbatim into create_issue.cjs, safe_outputs_handlers.cjs, and intent_probe.cjs — three divergence points for future bugs.
<details>
<summary>💡 Suggested fix</summary>
Extract to a shared helper (e.g. in mcp_scripts_validation.cjs or a dedicated utils module):
function extractFirstBodyLine(body) {
return (body || "")
.split("\n")
.map(l => l.replace(/^#+\s*/…
</details>
<details><summary>actions/setup/js/mcp_scripts_mcp_server_http.cjs:58</summary>
**minLength violation throws a plain `Error`, not a structured JSON-RPC error**: the stdio path returns error code `-32602` (Invalid params), but this HTTP path throws `new Error(...)` which the MCP SDK will translate into a different error shape — losing the protocol error code.
<details>
<summary>💡 Suggested fix</summary>
Throw a structured MCP error consistent with what `mcp_server_core.cjs` does:
```js
const err = new Error(`Invalid arguments: ${details}`);
err.code = -32602;
throw err;…
</details>
<details><summary>actions/setup/js/create_issue.cjs:14</summary>
**Setext-style horizontal rules (`---`, `===`) will be returned as a title**: the regex `/^#+\s*/` only strips ATX-style markdown headers (leading `#`). A body like `\n---\nActual content` has `---` as its first non-empty line, which passes through unchanged and becomes the issue title.
<details>
<summary>💡 Suggested fix</summary>
Extend the skip condition to also reject common non-title lines:
```js
.find(l => l.length > 0 && !/^[-=]{3,}$/.test(l))This covers setext underlines and th…
|
@copilot run pr-finisher skill |
…ngth coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran a PR-finisher pass and pushed commit |
🤖 PR Triage
Score breakdown: Impact 38 + Urgency 20 + Quality 14 Fixes issue-title/body split in
|
|
🎉 This pull request is included in a new release. Release: |
In githubnext/gh-aw-workshop#1836, an auto-injected
create_issuesafe output produced an issue where the entire content was in the title and the body was empty/trivial. Three contributing causes:Root causes & fixes
Vague auto-inject prompt (
safe_outputs_auto_create_issue.md): only said "create a GitHub issue" — no title/body split guidance. Added explicit instruction: title is a short headline (≤ 100 chars), body carries the full content.Entire-body title fallback (
create_issue.cjs,safe_outputs_handlers.cjs): whentitlewas absent,title = message.bodyassigned the whole body string as the title. For bodies ≤ 128 chars (thesanitizeTitlehard limit), all content ended up only in the title. Changed to use the first non-empty line of body instead:Missing
minLengthvalidation on HTTP MCP path (mcp_scripts_mcp_server_http.cjs):mcp_server_core.cjsenforcedminLength: 20onbodyviavalidateStringMinLengths, but the HTTP transport did not. A 1-char body like"."passed the required-field check, letting agents keep all content intitle. Added the missing call, consistent with the stdio path.intent_probe.cjs: mirrored the first-line fallback so probe-detection computes the same effective title as the actual handler.