[jsweep] jsweep: Clean add_comment.cjs#24020
Merged
Conversation
- Extract matchesWorkflowId() helper to eliminate duplicate comment filter logic shared between findCommentsWithTrackerId and findDiscussionCommentsWithTrackerId - Remove unused body and createdAt fields from GraphQL discussion comment mutations - Simplify variables construction with spread operator in commentOnDiscussion - Simplify resolvedTemporaryIds merge using nullish coalescing (??) - Add 6 new tests covering staged mode, max count, footer disabled, and hide-older-comments edge cases (no workflow ID, disabled by default) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the add_comment safe-output handler to remove duplicated workflow-ID matching logic, streamline discussion comment GraphQL mutations, and add test coverage for previously untested branches.
Changes:
- Extracted a shared
matchesWorkflowId(body, workflowId)helper used by both issue/PR and discussion comment filtering. - Simplified
commentOnDiscussionGraphQL mutation payloads (removed unused fields) and cleaned up variable construction / temp-id merge logic. - Added new tests covering staged mode, max-count enforcement, footer disabling, and hide-older-comments skip conditions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/js/add_comment.cjs | Refactors workflow-id matching + trims unused GraphQL fields and simplifies a few expressions. |
| actions/setup/js/add_comment.test.cjs | Adds coverage for staged mode/config paths and additional config guards (max/footer/hide). |
Comments suppressed due to low confidence (2)
actions/setup/js/add_comment.test.cjs:1939
- This test sets
process.env.GH_AW_WORKFLOW_NAMEand later deletes it. For isolation (and to avoid breaking runs where this env var is already set), restore the previous value in afinallyblock rather than unconditional delete.
process.env.GH_AW_WORKFLOW_NAME = "No-Footer Workflow";
let capturedBody = null;
mockGithub.rest.issues.createComment = async params => {
capturedBody = params.body;
return { data: { id: 1, html_url: "https://github.com/owner/repo/issues/8535#issuecomment-1" } };
};
actions/setup/js/add_comment.test.cjs:1964
- These tests mutate
process.env.GH_AW_WORKFLOW_IDviadelete/assignment but don’t restore any pre-existing value, and cleanup won’t run if an assertion throws. Please save the original value and restore it infinallyto keep the suite hermetic.
delete process.env.GH_AW_WORKFLOW_ID;
let listCommentsCalled = false;
mockGithub.rest.issues.listComments = async () => {
listCommentsCalled = true;
return { data: [] };
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1861
to
+1880
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | ||
|
|
||
| let createCommentCalled = false; | ||
| mockGithub.rest.issues.createComment = async () => { | ||
| createCommentCalled = true; | ||
| return { data: { id: 1, html_url: "https://github.com/owner/repo/issues/8535#issuecomment-1" } }; | ||
| }; | ||
|
|
||
| const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); | ||
|
|
||
| const message = { type: "add_comment", body: "Staged comment preview" }; | ||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.staged).toBe(true); | ||
| expect(result.previewInfo).toBeDefined(); | ||
| expect(result.previewInfo.itemNumber).toBe(8535); | ||
| expect(createCommentCalled).toBe(false); | ||
|
|
||
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; |
There was a problem hiding this comment.
This test mutates process.env.GH_AW_SAFE_OUTPUTS_STAGED but only deletes it at the end; that can leak state if the variable was already set, and it won’t run if the test fails mid-way. Please snapshot the original value and restore it in a try/finally (see actions/setup/js/add_reviewer.test.cjs:247-269 for the established pattern).
This issue also appears in the following locations of the same file:
- line 1933
- line 1958
Suggested change
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | |
| let createCommentCalled = false; | |
| mockGithub.rest.issues.createComment = async () => { | |
| createCommentCalled = true; | |
| return { data: { id: 1, html_url: "https://github.com/owner/repo/issues/8535#issuecomment-1" } }; | |
| }; | |
| const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); | |
| const message = { type: "add_comment", body: "Staged comment preview" }; | |
| const result = await handler(message, {}); | |
| expect(result.success).toBe(true); | |
| expect(result.staged).toBe(true); | |
| expect(result.previewInfo).toBeDefined(); | |
| expect(result.previewInfo.itemNumber).toBe(8535); | |
| expect(createCommentCalled).toBe(false); | |
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| const originalStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; | |
| try { | |
| let createCommentCalled = false; | |
| mockGithub.rest.issues.createComment = async () => { | |
| createCommentCalled = true; | |
| return { data: { id: 1, html_url: "https://github.com/owner/repo/issues/8535#issuecomment-1" } }; | |
| }; | |
| const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`); | |
| const message = { type: "add_comment", body: "Staged comment preview" }; | |
| const result = await handler(message, {}); | |
| expect(result.success).toBe(true); | |
| expect(result.staged).toBe(true); | |
| expect(result.previewInfo).toBeDefined(); | |
| expect(result.previewInfo.itemNumber).toBe(8535); | |
| expect(createCommentCalled).toBe(false); | |
| } finally { | |
| if (originalStaged === undefined) { | |
| delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; | |
| } else { | |
| process.env.GH_AW_SAFE_OUTPUTS_STAGED = originalStaged; | |
| } | |
| } |
pelikhan
reviewed
Apr 2, 2026
actions/setup/js/add_comment.cjs
Outdated
| * @param {string} workflowId - Workflow ID to match | ||
| * @returns {boolean} | ||
| */ | ||
| function matchesWorkflowId(body, workflowId) { |
Collaborator
There was a problem hiding this comment.
@copilot move helper to footer helper file with other generated footer related functions. Look for duplicate
Contributor
There was a problem hiding this comment.
Moved matchesWorkflowId into generate_footer.cjs alongside the related generateWorkflowIdMarker and getWorkflowIdMarkerContent functions, and updated add_comment.cjs to import it from there (35a4f6f).
…d add try/finally for env var restoration Agent-Logs-Url: https://github.com/github/gh-aw/sessions/504727d8-8965-469b-a4d2-21a060b97cc5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Addressed all review comments in 35a4f6f: wrapped all 4 env-var–mutating tests in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleaned
actions/setup/js/add_comment.cjs— a github-script context handler for adding comments to issues, PRs, and discussions.Changes
add_comment.cjsExtracted shared helper
matchesWorkflowId(body, workflowId)findCommentsWithTrackerIdandfindDiscussionCommentsWithTrackerIdduplicated the same multi-line filter predicate checking for workflow ID markers in HTML comments.filter(comment => matchesWorkflowId(comment.body, workflowId))call eachRemoved unused GraphQL fields
commentOnDiscussionfunction fetchedbodyandcreatedAtin both mutation responses, but onlyidandurlare ever usedSimplified variables construction
replyToId ? { dId, body, replyToId } : { dId, body }to{ dId, body, ...(replyToId ? { replyToId } : {}) }— single expression, cleanerSimplified
resolvedTemporaryIdsmergeif (resolvedTemporaryIds) { for...of }tofor...of Object.entries(resolvedTemporaryIds ?? {})— uses nullish coalescingadd_comment.test.cjsAdded 6 new tests (59 → 65 tests) covering previously untested paths:
GH_AW_SAFE_OUTPUTS_STAGEDisStagedMode()env pathconfig.staged: truefor discussionsisStagedMode()config pathmax: 2skips 3rd comment)maxCountguardfooter: false) — only XML marker, no attribution textincludeFooterfalse branchGH_AW_WORKFLOW_IDis setworkflowIdempty guardhide_older_commentsis false (default)Validation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js -- add_comment.test.cjs→ 65 passed ✓