fix: add generated footer to close_issue comment body - #47584
Conversation
Issue comments posted when auto-closing via safe-outputs now include the generated workflow footer (run URL, workflow name) just like other safe-output message types (close-pull-request, add-comment, etc.). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds the standard safe-output footer to auto-close issue comments.
Changes:
- Reuses the shared
buildCommentBodyhelper. - Relaxes existing body assertions for appended footer content.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/close_issue.cjs |
Adds footer generation to issue-closing comments. |
actions/setup/js/close_issue.test.cjs |
Updates comment-body assertions. |
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: 1
- Review effort level: Medium
| expect(commentCalls.length).toBe(1); | ||
| // Should use the body from message, not the config comment | ||
| expect(commentCalls[0].body).toBe("Custom body from message"); | ||
| expect(commentCalls[0].body).toContain("Custom body from message"); |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47584 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. ✅ |
There was a problem hiding this comment.
Review: Non-blocking observation
The fix is correct and mirrors the existing pattern in close_pull_request.cjs. An existing comment already flags the weakened test assertions. No new blocking issues found.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 19.8 AIC · ⌖ 4.45 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Review: fix: add generated footer to close_issue comment body
The implementation in close_issue.cjs is correct and consistent with the pattern in close_pull_request.cjs.
Non-blocking concern in the tests: both updated assertions (toContain(body)) are too weak to catch a regression — they pass even if buildCommentBody is reverted to returning sanitizedBody directly. Adding a footer-sentinel assertion (e.g., toContain('<!-- gh-aw')) at both test sites would make the tests actually guard the behavior this PR fixes.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 13.3 AIC · ⌖ 4.55 AIC · ⊞ 5K
| expect(result.success).toBe(true); | ||
| expect(result.alreadyClosed).toBe(true); | ||
| expect(commentBody).toBe("Closing comment with details"); | ||
| expect(commentBody).toContain("Closing comment with details"); |
There was a problem hiding this comment.
Same concern as line 416: .toContain(body) alone doesn't confirm the footer was appended. The test would still pass if buildCommentBody is reverted to returning sanitizedBody directly. Add an assertion on a footer-specific token, e.g.:
expect(commentBody).toContain("<!-- gh-aw"); // footer sentinel@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes on test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Weak regression tests: Both updated assertions use
.toContainbut never assert that the footer was actually appended, so the bug this PR fixes is not guarded against re-introduction.
Positive Highlights
- ✅ Correct fix: delegating to
buildCommentBodymatches the established pattern inclose_pull_request.cjs - ✅ Minimal, focused change — only the odd-one-out code path is touched
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 14.3 AIC · ⌖ 4.58 AIC · ⊞ 6.7K
Comment /matt to run again
| expect(result.success).toBe(true); | ||
| expect(result.alreadyClosed).toBe(true); | ||
| expect(commentBody).toBe("Closing comment with details"); | ||
| expect(commentBody).toContain("Closing comment with details"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Same weakened assertion as line 416 — .toContain passes even if the footer is absent, so this test does not guard the regression fixed by this PR. Add a footer-specific assertion to confirm the footer was actually appended.
💡 Suggested fix
expect(commentBody).toContain("Closing comment with details");
// confirm footer was appended
expect(commentBody.length).toBeGreaterThan("Closing comment with details".length);Or match a known footer marker string from buildCommentBody.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
|
|
@copilot please run the Unresolved review items:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 112ee44. I tightened the close_issue regression tests to explicitly assert footer presence ( |
|
🎉 This pull request is included in a new release. Release: |
Comments posted when auto-closing an issue via safe-outputs were missing the standard workflow footer (run URL, workflow name, tracker ID). Every other safe-output message type (
close-pull-request,add-comment, etc.) includes this footer via the sharedbuildCommentBodyhelper inclose_entity_helpers.cjs—close_issuewas the odd one out, returning the sanitized body directly.Changes
close_issue.cjs: ImportbuildCommentBodyfromclose_entity_helpers.cjsand delegate to it, identical to the existing pattern inclose_pull_request.cjs:close_issue.test.cjs: Update two assertions from.toBe(body)→.toContain(body)to accommodate the appended footer.