From e21991975451205fb0a9731866ccdb9ab59308ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Feb 2026 06:51:54 +0000 Subject: [PATCH 1/3] Initial plan From f62dbe0d9afde0d9fc0c129d581ce40187343c93 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Feb 2026 07:02:16 +0000 Subject: [PATCH 2/3] Add footer configuration for PR review comments - Add Footer field to CreatePullRequestReviewCommentsConfig - Support "always", "none", "if-body" values - Support boolean true/false mapping - Update pr_review_buffer to support conditional footer logic - Update handler managers to apply footer config - Add comprehensive tests for Go and JavaScript Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/pr_review_buffer.cjs | 57 +++- actions/setup/js/pr_review_buffer.test.cjs | 271 +++++++++++++++++- .../setup/js/safe_output_handler_manager.cjs | 18 +- .../safe_output_unified_handler_manager.cjs | 18 +- pkg/workflow/compiler_safe_outputs_config.go | 1 + pkg/workflow/create_pr_review_comment.go | 25 ++ .../create_pr_review_comment_footer_test.go | 176 ++++++++++++ 7 files changed, 543 insertions(+), 23 deletions(-) create mode 100644 pkg/workflow/create_pr_review_comment_footer_test.go diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs index cc442e02b1..bc7e54abba 100644 --- a/actions/setup/js/pr_review_buffer.cjs +++ b/actions/setup/js/pr_review_buffer.cjs @@ -61,8 +61,8 @@ function createReviewBuffer() { /** @type {{workflowName: string, runUrl: string, workflowSource: string, workflowSourceURL: string, triggeringIssueNumber: number|undefined, triggeringPRNumber: number|undefined, triggeringDiscussionNumber: number|undefined} | null} */ let footerContext = null; - /** @type {boolean} Whether to include footer in review body (default: true, controlled by config.footer) */ - let includeFooter = true; + /** @type {string} Footer mode: "always" (default), "none", or "if-body" */ + let footerMode = "always"; /** * Add a validated comment to the buffer. @@ -120,13 +120,32 @@ function createReviewBuffer() { } /** - * Set whether to include footer in review body. - * Controlled by the `footer` config option (default: true). - * @param {boolean} value - Whether to include footer + * Set the footer mode for review body. + * Supported modes: + * - "always" (default): Always include footer + * - "none": Never include footer + * - "if-body": Only include footer if review body is non-empty + * For backward compatibility, also accepts boolean values (true -> "always", false -> "none") + * @param {string | boolean} value - Footer mode or boolean */ - function setIncludeFooter(value) { - includeFooter = value; - core.info(`PR review footer ${value ? "enabled" : "disabled"}`); + function setFooterMode(value) { + if (typeof value === "boolean") { + // Backward compatibility: map boolean to string mode + footerMode = value ? "always" : "none"; + core.info(`PR review footer mode set to "${footerMode}" (from boolean ${value})`); + } else if (typeof value === "string") { + // Validate string mode + if (value === "always" || value === "none" || value === "if-body") { + footerMode = value; + core.info(`PR review footer mode set to "${footerMode}"`); + } else { + core.warning(`Invalid footer mode: "${value}". Using default "always". Valid values: "always", "none", "if-body"`); + footerMode = "always"; + } + } else { + core.warning(`Invalid footer mode type: ${typeof value}. Using default "always".`); + footerMode = "always"; + } } /** @@ -182,9 +201,20 @@ function createReviewBuffer() { const event = reviewMetadata ? reviewMetadata.event : "COMMENT"; let body = reviewMetadata ? reviewMetadata.body : ""; - // Add footer to review body if enabled and we have footer context. - // Footer is always added (even for body-less reviews) to track which workflow submitted the review. - if (includeFooter && footerContext) { + // Determine if we should add footer based on footer mode + let shouldAddFooter = false; + if (footerMode === "always") { + shouldAddFooter = true; + } else if (footerMode === "none") { + shouldAddFooter = false; + } else if (footerMode === "if-body") { + // Only add footer if body is non-empty (has meaningful content) + shouldAddFooter = body.trim().length > 0; + core.info(`Footer mode "if-body": body is ${body.trim().length > 0 ? "non-empty" : "empty"}, ${shouldAddFooter ? "adding" : "skipping"} footer`); + } + + // Add footer to review body if we should and we have footer context + if (shouldAddFooter && footerContext) { body += generateFooterWithMessages( footerContext.workflowName, footerContext.runUrl, @@ -275,7 +305,7 @@ function createReviewBuffer() { reviewMetadata = null; reviewContext = null; footerContext = null; - includeFooter = true; + footerMode = "always"; } return { @@ -284,7 +314,8 @@ function createReviewBuffer() { setReviewContext, getReviewContext, setFooterContext, - setIncludeFooter, + setFooterMode, + setIncludeFooter: setFooterMode, // Backward compatibility alias hasBufferedComments, hasReviewMetadata, getBufferedCount, diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index de1f566976..cdd35dec85 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/actions/setup/js/pr_review_buffer.test.cjs @@ -503,7 +503,7 @@ describe("pr_review_buffer (factory pattern)", () => { }); describe("reset", () => { - it("should clear all state including includeFooter", () => { + it("should clear all state including footer mode", () => { buffer.addComment({ path: "test.js", line: 1, body: "comment" }); buffer.setReviewMetadata("body", "APPROVE"); buffer.setReviewContext({ @@ -512,7 +512,7 @@ describe("pr_review_buffer (factory pattern)", () => { pullRequestNumber: 1, pullRequest: { head: { sha: "abc" } }, }); - buffer.setIncludeFooter(false); + buffer.setFooterMode("none"); buffer.reset(); @@ -521,7 +521,7 @@ describe("pr_review_buffer (factory pattern)", () => { expect(buffer.hasReviewMetadata()).toBe(false); expect(buffer.getReviewContext()).toBeNull(); - // After reset, footer should be re-enabled (default: true) + // After reset, footer should be "always" (default) // Verify by submitting a review with footer context and checking body buffer.addComment({ path: "test.js", line: 1, body: "comment" }); buffer.setReviewMetadata("Review after reset", "COMMENT"); @@ -545,9 +545,272 @@ describe("pr_review_buffer (factory pattern)", () => { return buffer.submitReview().then(result => { expect(result.success).toBe(true); const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; - // Footer should be included since includeFooter was reset to true + // Footer should be included since footer mode was reset to "always" expect(callArgs.body).toContain("test-workflow"); }); }); }); + + describe("footer mode", () => { + it("should support 'always' mode (default)", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("", "APPROVE"); // Empty body + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("always"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 500, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-500", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Footer should be included even with empty body + expect(callArgs.body).toContain("test-workflow"); + }); + + it("should support 'none' mode", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("Review body", "COMMENT"); + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("none"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 501, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-501", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Footer should not be included + expect(callArgs.body).toBe("Review body"); + expect(callArgs.body).not.toContain("test-workflow"); + }); + + it("should support 'if-body' mode with non-empty body", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("Review body", "COMMENT"); + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("if-body"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 502, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-502", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Footer should be included because body is non-empty + expect(callArgs.body).toContain("Review body"); + expect(callArgs.body).toContain("test-workflow"); + }); + + it("should support 'if-body' mode with empty body", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("", "APPROVE"); // Empty body + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("if-body"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 503, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-503", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Footer should NOT be included because body is empty + // Body should be undefined (not included in API call) when empty + expect(callArgs.body).toBeUndefined(); + expect(callArgs.body || "").not.toContain("test-workflow"); + }); + + it("should support 'if-body' mode with whitespace-only body", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata(" \n ", "APPROVE"); // Whitespace-only body + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("if-body"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 504, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-504", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Footer should NOT be included because body is whitespace-only (trimmed length is 0) + // Original whitespace body is preserved in the API call + expect(callArgs.body).toBe(" \n "); + expect(callArgs.body).not.toContain("test-workflow"); + }); + + it("should map boolean true to 'always' mode", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("", "APPROVE"); + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode(true); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 505, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-505", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + expect(callArgs.body).toContain("test-workflow"); + }); + + it("should map boolean false to 'none' mode", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("Review body", "COMMENT"); + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode(false); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 506, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-506", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + expect(callArgs.body).toBe("Review body"); + expect(callArgs.body).not.toContain("test-workflow"); + }); + + it("should default to 'always' for invalid string mode", async () => { + buffer.addComment({ path: "test.js", line: 1, body: "comment" }); + buffer.setReviewMetadata("", "APPROVE"); + buffer.setReviewContext({ + repo: "owner/repo", + repoParts: { owner: "owner", repo: "repo" }, + pullRequestNumber: 42, + pullRequest: { head: { sha: "abc123" } }, + }); + buffer.setFooterContext({ + workflowName: "test-workflow", + runUrl: "https://github.com/owner/repo/actions/runs/123", + workflowSource: "owner/repo/workflows/test.md@v1", + workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", + }); + buffer.setFooterMode("invalid-mode"); + + mockGithub.rest.pulls.createReview.mockResolvedValue({ + data: { + id: 507, + html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-507", + }, + }); + + const result = await buffer.submitReview(); + + expect(result.success).toBe(true); + const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; + // Should default to "always" and include footer + expect(callArgs.body).toContain("test-workflow"); + }); + }); }); diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 771b277b26..624b4fd255 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -737,9 +737,21 @@ async function main() { // Create the shared PR review buffer instance (no global state) const prReviewBuffer = createReviewBuffer(); - // Apply footer config from submit_pull_request_review (if configured) - if (config.submit_pull_request_review?.footer === false) { - prReviewBuffer.setIncludeFooter(false); + // Apply footer config with priority: + // 1. create_pull_request_review_comment.footer (highest priority) + // 2. submit_pull_request_review.footer (fallback) + // 3. Default: "always" + let footerConfig = undefined; + if (config.create_pull_request_review_comment?.footer !== undefined) { + footerConfig = config.create_pull_request_review_comment.footer; + core.info(`Using footer config from create_pull_request_review_comment: ${footerConfig}`); + } else if (config.submit_pull_request_review?.footer !== undefined) { + footerConfig = config.submit_pull_request_review.footer; + core.info(`Using footer config from submit_pull_request_review: ${footerConfig}`); + } + + if (footerConfig !== undefined) { + prReviewBuffer.setFooterMode(footerConfig); } // Load and initialize handlers based on configuration (factory pattern) diff --git a/actions/setup/js/safe_output_unified_handler_manager.cjs b/actions/setup/js/safe_output_unified_handler_manager.cjs index 8ee799b47b..cb3bb86696 100644 --- a/actions/setup/js/safe_output_unified_handler_manager.cjs +++ b/actions/setup/js/safe_output_unified_handler_manager.cjs @@ -982,9 +982,21 @@ async function main() { // Create the shared PR review buffer instance (no global state) const prReviewBuffer = createReviewBuffer(); - // Apply footer config from submit_pull_request_review (if configured) - if (configs.regular?.submit_pull_request_review?.footer === false) { - prReviewBuffer.setIncludeFooter(false); + // Apply footer config with priority: + // 1. create_pull_request_review_comment.footer (highest priority) + // 2. submit_pull_request_review.footer (fallback) + // 3. Default: "always" + let footerConfig = undefined; + if (configs.regular?.create_pull_request_review_comment?.footer !== undefined) { + footerConfig = configs.regular.create_pull_request_review_comment.footer; + core.info(`Using footer config from create_pull_request_review_comment: ${footerConfig}`); + } else if (configs.regular?.submit_pull_request_review?.footer !== undefined) { + footerConfig = configs.regular.submit_pull_request_review.footer; + core.info(`Using footer config from submit_pull_request_review: ${footerConfig}`); + } + + if (footerConfig !== undefined) { + prReviewBuffer.setFooterMode(footerConfig); } // Load and initialize handlers based on configuration (factory pattern) diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index 59ae5d3425..567b20292e 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -303,6 +303,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("target", c.Target). AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddStringSlice("allowed_repos", c.AllowedRepos). + AddStringPtr("footer", c.Footer). Build() }, "submit_pull_request_review": func(cfg *SafeOutputsConfig) map[string]any { diff --git a/pkg/workflow/create_pr_review_comment.go b/pkg/workflow/create_pr_review_comment.go index be6b31400c..18ca8859da 100644 --- a/pkg/workflow/create_pr_review_comment.go +++ b/pkg/workflow/create_pr_review_comment.go @@ -15,6 +15,7 @@ type CreatePullRequestReviewCommentsConfig struct { Target string `yaml:"target,omitempty"` // Target for comments: "triggering" (default), "*" (any PR), or explicit PR number TargetRepoSlug string `yaml:"target-repo,omitempty"` // Target repository in format "owner/repo" for cross-repository PR review comments AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories that PR review comments can be added to (additionally to the target-repo) + Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review: "always" (default), "none", or "if-body" (only when review has body text) } // buildCreateOutputPullRequestReviewCommentJob creates the create_pr_review_comment job @@ -119,6 +120,30 @@ func (c *Compiler) parsePullRequestReviewCommentsConfig(outputMap map[string]any } prReviewCommentsConfig.TargetRepoSlug = targetRepoSlug + // Parse footer configuration + if footer, exists := configMap["footer"]; exists { + switch f := footer.(type) { + case string: + // Validate string values: "always", "none", "if-body" + if f == "always" || f == "none" || f == "if-body" { + prReviewCommentsConfig.Footer = &f + createPRReviewCommentLog.Printf("Footer control: %s", f) + } else { + createPRReviewCommentLog.Printf("Invalid footer value: %s (must be 'always', 'none', or 'if-body')", f) + } + case bool: + // Map boolean to string: true -> "always", false -> "none" + var footerStr string + if f { + footerStr = "always" + } else { + footerStr = "none" + } + prReviewCommentsConfig.Footer = &footerStr + createPRReviewCommentLog.Printf("Footer control (mapped from bool): %s", footerStr) + } + } + // Parse common base fields with default max of 10 c.parseBaseSafeOutputConfig(configMap, &prReviewCommentsConfig.BaseSafeOutputConfig, 10) } else { diff --git a/pkg/workflow/create_pr_review_comment_footer_test.go b/pkg/workflow/create_pr_review_comment_footer_test.go new file mode 100644 index 0000000000..1f3a36367d --- /dev/null +++ b/pkg/workflow/create_pr_review_comment_footer_test.go @@ -0,0 +1,176 @@ +//go:build !integration + +package workflow + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPRReviewCommentFooterConfig(t *testing.T) { + t.Run("parses footer string values", func(t *testing.T) { + testCases := []struct { + name string + value string + expected string + }{ + {name: "always", value: "always", expected: "always"}, + {name: "none", value: "none", expected: "none"}, + {name: "if-body", value: "if-body", expected: "if-body"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "create-pull-request-review-comment": map[string]any{ + "footer": tc.value, + }, + } + + config := compiler.parsePullRequestReviewCommentsConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + require.NotNil(t, config.Footer, "Footer should be set") + assert.Equal(t, tc.expected, *config.Footer, "Footer value should match") + }) + } + }) + + t.Run("parses footer boolean values", func(t *testing.T) { + testCases := []struct { + name string + value bool + expected string + }{ + {name: "true maps to always", value: true, expected: "always"}, + {name: "false maps to none", value: false, expected: "none"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "create-pull-request-review-comment": map[string]any{ + "footer": tc.value, + }, + } + + config := compiler.parsePullRequestReviewCommentsConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + require.NotNil(t, config.Footer, "Footer should be set") + assert.Equal(t, tc.expected, *config.Footer, "Footer value should be mapped from boolean") + }) + } + }) + + t.Run("ignores invalid footer values", func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "create-pull-request-review-comment": map[string]any{ + "footer": "invalid-value", + }, + } + + config := compiler.parsePullRequestReviewCommentsConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + assert.Nil(t, config.Footer, "Invalid footer value should be ignored") + }) + + t.Run("footer not set when omitted", func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "create-pull-request-review-comment": map[string]any{ + "side": "RIGHT", + }, + } + + config := compiler.parsePullRequestReviewCommentsConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + assert.Nil(t, config.Footer, "Footer should be nil when not configured") + }) +} + +func TestPRReviewCommentFooterInHandlerConfig(t *testing.T) { + t.Run("footer included in handler config", func(t *testing.T) { + compiler := NewCompiler() + footerValue := "if-body" + workflowData := &WorkflowData{ + Name: "Test", + SafeOutputs: &SafeOutputsConfig{ + CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 10}, + Side: "RIGHT", + Footer: &footerValue, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "Steps should not be empty") + + stepsContent := strings.Join(steps, "") + require.Contains(t, stepsContent, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") + + for _, step := range steps { + if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + if len(parts) == 2 { + jsonStr := strings.TrimSpace(parts[1]) + jsonStr = strings.Trim(jsonStr, "\"") + jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") + var handlerConfig map[string]any + err := json.Unmarshal([]byte(jsonStr), &handlerConfig) + require.NoError(t, err, "Should unmarshal handler config") + + reviewCommentConfig, ok := handlerConfig["create_pull_request_review_comment"].(map[string]any) + require.True(t, ok, "create_pull_request_review_comment config should exist") + assert.Equal(t, "if-body", reviewCommentConfig["footer"], "Footer should be in handler config") + } + } + } + }) + + t.Run("footer not in handler config when not set", func(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test", + SafeOutputs: &SafeOutputsConfig{ + CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 10}, + Side: "RIGHT", + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "Steps should not be empty") + + stepsContent := strings.Join(steps, "") + require.Contains(t, stepsContent, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") + + for _, step := range steps { + if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + if len(parts) == 2 { + jsonStr := strings.TrimSpace(parts[1]) + jsonStr = strings.Trim(jsonStr, "\"") + jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") + var handlerConfig map[string]any + err := json.Unmarshal([]byte(jsonStr), &handlerConfig) + require.NoError(t, err, "Should unmarshal handler config") + + reviewCommentConfig, ok := handlerConfig["create_pull_request_review_comment"].(map[string]any) + require.True(t, ok, "create_pull_request_review_comment config should exist") + _, hasFooter := reviewCommentConfig["footer"] + assert.False(t, hasFooter, "Footer should not be in handler config when not set") + } + } + } + }) +} From 84c58a6bed4acc3dfbb0c81c09a76ee9c390547b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Feb 2026 07:15:24 +0000 Subject: [PATCH 3/3] Move boolean-to-string conversion to Go compiler JavaScript now only handles string values ("always", "none", "if-body"). Boolean conversion (true->always, false->none) is performed in Go compiler before values reach JavaScript, per review feedback. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/pr_review_buffer.cjs | 10 +--- actions/setup/js/pr_review_buffer.test.cjs | 67 +--------------------- 2 files changed, 5 insertions(+), 72 deletions(-) diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs index bc7e54abba..bbd9909ea7 100644 --- a/actions/setup/js/pr_review_buffer.cjs +++ b/actions/setup/js/pr_review_buffer.cjs @@ -125,15 +125,11 @@ function createReviewBuffer() { * - "always" (default): Always include footer * - "none": Never include footer * - "if-body": Only include footer if review body is non-empty - * For backward compatibility, also accepts boolean values (true -> "always", false -> "none") - * @param {string | boolean} value - Footer mode or boolean + * Note: Boolean values are converted to strings in the Go compiler before reaching JavaScript. + * @param {string} value - Footer mode string */ function setFooterMode(value) { - if (typeof value === "boolean") { - // Backward compatibility: map boolean to string mode - footerMode = value ? "always" : "none"; - core.info(`PR review footer mode set to "${footerMode}" (from boolean ${value})`); - } else if (typeof value === "string") { + if (typeof value === "string") { // Validate string mode if (value === "always" || value === "none" || value === "if-body") { footerMode = value; diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index cdd35dec85..aeb9d09613 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/actions/setup/js/pr_review_buffer.test.cjs @@ -394,7 +394,7 @@ describe("pr_review_buffer (factory pattern)", () => { expect(callArgs.body).toContain("test-workflow"); }); - it("should skip footer when setIncludeFooter(false) is called", async () => { + it("should skip footer when setIncludeFooter('none') is called", async () => { buffer.addComment({ path: "test.js", line: 1, body: "comment" }); buffer.setReviewMetadata("Review body", "COMMENT"); buffer.setReviewContext({ @@ -409,7 +409,7 @@ describe("pr_review_buffer (factory pattern)", () => { workflowSource: "owner/repo/workflows/test.md@v1", workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", }); - buffer.setIncludeFooter(false); + buffer.setIncludeFooter("none"); mockGithub.rest.pulls.createReview.mockResolvedValue({ data: { @@ -718,69 +718,6 @@ describe("pr_review_buffer (factory pattern)", () => { expect(callArgs.body).not.toContain("test-workflow"); }); - it("should map boolean true to 'always' mode", async () => { - buffer.addComment({ path: "test.js", line: 1, body: "comment" }); - buffer.setReviewMetadata("", "APPROVE"); - buffer.setReviewContext({ - repo: "owner/repo", - repoParts: { owner: "owner", repo: "repo" }, - pullRequestNumber: 42, - pullRequest: { head: { sha: "abc123" } }, - }); - buffer.setFooterContext({ - workflowName: "test-workflow", - runUrl: "https://github.com/owner/repo/actions/runs/123", - workflowSource: "owner/repo/workflows/test.md@v1", - workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", - }); - buffer.setFooterMode(true); - - mockGithub.rest.pulls.createReview.mockResolvedValue({ - data: { - id: 505, - html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-505", - }, - }); - - const result = await buffer.submitReview(); - - expect(result.success).toBe(true); - const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; - expect(callArgs.body).toContain("test-workflow"); - }); - - it("should map boolean false to 'none' mode", async () => { - buffer.addComment({ path: "test.js", line: 1, body: "comment" }); - buffer.setReviewMetadata("Review body", "COMMENT"); - buffer.setReviewContext({ - repo: "owner/repo", - repoParts: { owner: "owner", repo: "repo" }, - pullRequestNumber: 42, - pullRequest: { head: { sha: "abc123" } }, - }); - buffer.setFooterContext({ - workflowName: "test-workflow", - runUrl: "https://github.com/owner/repo/actions/runs/123", - workflowSource: "owner/repo/workflows/test.md@v1", - workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", - }); - buffer.setFooterMode(false); - - mockGithub.rest.pulls.createReview.mockResolvedValue({ - data: { - id: 506, - html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-506", - }, - }); - - const result = await buffer.submitReview(); - - expect(result.success).toBe(true); - const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; - expect(callArgs.body).toBe("Review body"); - expect(callArgs.body).not.toContain("test-workflow"); - }); - it("should default to 'always' for invalid string mode", async () => { buffer.addComment({ path: "test.js", line: 1, body: "comment" }); buffer.setReviewMetadata("", "APPROVE");