Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions actions/setup/js/notify_comment_error.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { sanitizeContent } = require("./sanitize_content.cjs");
const { ERR_VALIDATION } = require("./error_codes.cjs");
const { parseBoolTemplatable } = require("./templatable.cjs");
const { resolveTopLevelDiscussionCommentId } = require("./github_api_helpers.cjs");
const { assembleMarkdownBodyParts } = require("./markdown_body_helpers.cjs");

/**
* Collect generated asset URLs from safe output jobs
Expand Down Expand Up @@ -258,6 +259,24 @@ async function main() {
});
}

// Append the generated footer (attribution + XML marker)
const workflowSource = process.env.GH_AW_WORKFLOW_SOURCE ?? "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

const workflowSourceURL = process.env.GH_AW_WORKFLOW_SOURCE_URL ?? "";
const triggeringIssueNumber = context.payload?.issue?.number;
const triggeringPRNumber = context.payload?.pull_request?.number;
const triggeringDiscussionNumber = context.payload?.discussion?.number;
const markdownParts = assembleMarkdownBodyParts({
includeFooter: true,
workflowName,
runUrl,
workflowSource,
workflowSourceURL,
triggeringIssueNumber,
triggeringPRNumber,
triggeringDiscussionNumber,
});
message += "\n\n" + markdownParts.footer;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 paths

This ensures the XML traceability markers are never passed through the sanitizer.

@copilot please address this.


// Add "needs-review" label when detection produced a warning
if (detectionConclusion === "warning") {
await tryAddNeedsReviewLabel(commentRepo);
Expand Down
24 changes: 22 additions & 2 deletions actions/setup/js/notify_comment_error.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ const mockCore = {
(process.env.GH_AW_SAFE_OUTPUT_JOBS = JSON.stringify({ create_issue: "issue_url" })),
await eval(`(async () => { ${notifyCommentScript}; await main(); })()`));
const callArgs = mockGithub.request.mock.calls[0][1];
expect(callArgs.body).toMatch(/completed successfully!$/);
expect(callArgs.body).toContain("completed successfully!");
}),
it("should handle empty safe output jobs gracefully", async () => {
((process.env.GH_AW_COMMENT_ID = "123456"),
Expand All @@ -340,7 +340,7 @@ const mockCore = {
(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(/completed successfully!$/);
expect(callArgs.body).toContain("completed successfully!");
}));
}),
describe("when safe_outputs job fails", () => {
Expand Down Expand Up @@ -373,5 +373,25 @@ const mockCore = {
await eval(`(async () => { ${notifyCommentScript}; await main(); })()`),
expect(mockGithub.request).toHaveBeenCalledWith("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}", expect.objectContaining({ body: expect.stringContaining("completed successfully!") })));
}));
}),
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"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

(process.env.GH_AW_WORKFLOW_NAME = "test-workflow"),
(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\]/);
}),
it("should include the generated footer even when agent fails", async () => {
((process.env.GH_AW_COMMENT_ID = "123456"),
(process.env.GH_AW_RUN_URL = "https://github.com/owner/repo/actions/runs/123"),
(process.env.GH_AW_WORKFLOW_NAME = "test-workflow"),
(process.env.GH_AW_AGENT_CONCLUSION = "failure"),
await eval(`(async () => { ${notifyCommentScript}; await main(); })()`));
const callArgs = mockGithub.request.mock.calls[0][1];
expect(callArgs.body).toMatch(/Generated by \[test-workflow\]/);
});
}));
}));
Loading