Add generated footer to status comment updates - #51165
Conversation
…ror.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories.
|
|
There was a problem hiding this comment.
Pull request overview
Adds generated footers to workflow completion status comments.
Changes:
- Appends attribution and traceability footer data.
- Adds success/failure footer tests.
- Updates assertions for footer-suffixed messages.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/notify_comment_error.cjs |
Builds and appends the generated footer. |
actions/setup/js/notify_comment_error.test.cjs |
Tests footer attribution and adjusts message 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: 2
- Review effort level: Balanced
| triggeringPRNumber, | ||
| triggeringDiscussionNumber, | ||
| }); | ||
| message += "\n\n" + markdownParts.footer; |
| (process.env.GH_AW_AGENT_CONCLUSION = "success"), | ||
| await eval(`(async () => { ${notifyCommentScript}; await main(); })()`)); | ||
| const callArgs = mockGithub.request.mock.calls[0][1]; | ||
| expect(callArgs.body).toMatch(/Generated by \[test-workflow\]/); |
There was a problem hiding this comment.
The change correctly adds the generated footer to status comment updates in notify_comment_error.cjs, following the same pattern used in add_comment.cjs, comment_memory.cjs, and create_discussion.cjs. No duplication risk since message is freshly assembled. Test updates are correct: toContain properly replaces toMatch since the footer now follows the success message. New footer tests cover both success and failure paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.5 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Test Quality Sentinel 🧪 — PR #51165
Test Quality Score: 100/100 ✅ Excellent
Overview
This PR adds a focused test to verify footer generation works correctly in failure paths. The test complements existing footer success-path tests with appropriate edge-case coverage.
Changed Test Files:
actions/setup/js/notify_comment_error.test.cjs(+10 lines)
Production Code Impact:
actions/setup/js/notify_comment_error.cjs(+19 lines)- Test-to-code ratio: 0.53:1 (excellent — well below 2:1 threshold)
Test Analysis
New Test Function: should include the generated footer even when agent fails
- Location: notify_comment_error.test.cjs:387
- Design Invariant:
behavioral_contract - Value:
high_value - Type:
design_test
Coverage:
- ✅ Verifies footer rendering on agent failure (covers "failure" conclusion state)
- ✅ Complements success-path test with edge-case coverage
- ✅ 1 assertion: regex match on generated footer text
- ✅ No violations
Quality Metrics
| Metric | Result | Status |
|---|---|---|
| Design tests | 100% (1/1) | ✅ |
| Edge-case tests | 100% (1/1) | ✅ |
| Test-to-code ratio | 0.53:1 | ✅ |
| Mock violations | 0 | ✅ |
| Implementation-only tests | 0% | ✅ |
Conclusion
✅ Test quality is excellent. The new test verifies a critical design contract (footer generation across different agent states) with appropriate assertions and edge-case coverage. No quality violations detected.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on one correctness issue.
📋 Key Themes & Highlights
Key Themes
- Sanitize-before-footer ordering (correctness): The footer is appended to
messageat line 278 before the threesanitizeContentcalls at lines 331, 348, and 386. The sibling handleradd_comment.cjssanitizes first then appends footer. The XML<!-- aw:... -->markers could be stripped byremoveXmlComments, silently breaking traceability. - Env var scattered reads (minor):
GH_AW_WORKFLOW_SOURCE/GH_AW_WORKFLOW_SOURCE_URLare read mid-function, whileworkflowName/runUrlare read at the top — inconsistent style. - Test duplication (minor): The two new footer tests are near-identical; a shared
beforeEachwould reduce drift risk.
Positive Highlights
- ✅ Good parity fix — the missing footer was a real gap vs. other safe-output handlers.
- ✅ The existing
toMatch(/completed successfully!$/)→toContain(...)fix is correct and clean. - ✅
assembleMarkdownBodyPartsis reused rather than reimplemented — good deep-module usage.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37 AIC · ⊞ 7.1K
Comment /matt to run again
| triggeringPRNumber, | ||
| triggeringDiscussionNumber, | ||
| }); | ||
| message += "\n\n" + markdownParts.footer; |
There was a problem hiding this comment.
[/codebase-design] The footer is appended to message at line 278 before sanitizeContent is called at lines 331, 348, and 386. In add_comment.cjs the order is reversed: sanitize first, then append footer. The XML <!-- aw:... --> markers and attribution text may be stripped or mangled by removeXmlComments or other sanitizer passes.
💡 Suggested fix
Mirror the add_comment.cjs pattern — sanitize the user-facing body first, then append the footer:
const sanitizedMessage = sanitizeContent(message);
const fullMessage = sanitizedMessage + "\n\n" + markdownParts.footer;
// then use fullMessage in all three code pathsThis ensures the XML traceability markers are never passed through the sanitizer.
@copilot please address this.
| } | ||
|
|
||
| // Append the generated footer (attribution + XML marker) | ||
| const workflowSource = process.env.GH_AW_WORKFLOW_SOURCE ?? ""; |
There was a problem hiding this comment.
[/codebase-design] GH_AW_WORKFLOW_SOURCE and GH_AW_WORKFLOW_SOURCE_URL are re-read from process.env here, but workflowName and runUrl were already read at lines 114–115 from the same source. Consider reading all workflow env vars together near the top of main() for consistency and to make the full set of dependencies obvious at a glance.
@copilot please address this.
| describe("footer in status comment", () => { | ||
| it("should include the generated footer in the updated comment body", async () => { | ||
| ((process.env.GH_AW_COMMENT_ID = "123456"), | ||
| (process.env.GH_AW_RUN_URL = "https://github.com/owner/repo/actions/runs/123"), |
There was a problem hiding this comment.
[/tdd] The two new footer tests share identical env setup and differ only in GH_AW_AGENT_CONCLUSION. Extracting the shared setup into a beforeEach (or a helper) would make it clear which variables are the test-specific inputs vs. shared context, and reduce copy-paste risk when new env vars are added to the footer path.
@copilot please address this.
There was a problem hiding this comment.
Small, focused fix that adds the missing footer to status comments. The core change works, but the footer is functionally incomplete (missing engine/source metadata not wired through) and new tests only cover one code path.
Themes
- Missing wiring:
GH_AW_WORKFLOW_SOURCE/GH_AW_WORKFLOW_SOURCE_URL/engine metadata env vars are never set for the conclusion job, so the appended footer will consistently render without install instructions or engine attribution that other footer call sites (e.g.add_comment.cjs) include. - Test coverage gap: only the update-comment path is tested for the footer; append-only comment creation and discussion comment paths reuse the same
messagebut aren't verified to include the footer.
Neither issue is blocking for this incremental fix, but both should be addressed to fully deliver on the PR's stated goal of parity with other safe-output comment handlers.
🔎 Code quality review by PR Code Quality Reviewer · auto · 84.2 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/notify_comment_error.cjs:278
This footer append silently omits install instructions and engine/workflow-source metadata that other footer call sites include, and will render with empty workflowSource/workflowSourceURL since the conclusion job's env-var builder never populates GH_AW_WORKFLOW_SOURCE.
actions/setup/js/notify_comment_error.test.cjs:83
New tests only cover the update-comment path; the append-only (new issue/PR comment and discussion) branches that reuse the same message with the footer appended have no test coverage.
|
@copilot failed checks: lint-js: https://github.com/github/gh-aw/actions/runs/31210178563/job/92972103820 please refresh the branch if needed and run the
|
… marker Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in the latest commit. The footer is now assembled before the delivery branches but appended after |
|
🎉 This pull request is included in a new release. Release: |
Status comments updated at workflow completion (
notify_comment_error.cjs) were missing the generated footer (attribution line + XML traceability marker) present in all other safe-output comment handlers.Changes
notify_comment_error.cjs: ImportassembleMarkdownBodyPartsfrommarkdown_body_helpers.cjsand append the footer after the full message body (run status, detection warning, noop messages, generated assets) is assembled — beforesanitizeContent.notify_comment_error.test.cjs: Add two tests asserting footer presence on success and failure conclusions. Fix two assertions that matchedcompleted successfully!$(end-of-string) — now usetoContainsince footer follows the message.