diff --git a/actions/setup/js/submit_pr_review.cjs b/actions/setup/js/submit_pr_review.cjs index 7ed72702608..a68bcaad687 100644 --- a/actions/setup/js/submit_pr_review.cjs +++ b/actions/setup/js/submit_pr_review.cjs @@ -5,6 +5,9 @@ * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction */ +const { resolveTarget } = require("./safe_output_helpers.cjs"); +const { getErrorMessage } = require("./error_helpers.cjs"); + /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "submit_pull_request_review"; @@ -23,6 +26,7 @@ const VALID_EVENTS = new Set(["APPROVE", "REQUEST_CHANGES", "COMMENT"]); */ async function main(config = {}) { const maxCount = config.max || 1; + const targetConfig = config.target || "triggering"; const buffer = config._prReviewBuffer; if (!buffer) { @@ -32,7 +36,7 @@ async function main(config = {}) { }; } - core.info(`Submit PR review handler initialized: max=${maxCount}`); + core.info(`Submit PR review handler initialized: max=${maxCount}, target=${targetConfig}`); let processedCount = 0; @@ -82,17 +86,59 @@ async function main(config = {}) { // Ensure review context is set for body-only reviews (no inline comments). // If create_pull_request_review_comment already set context, this is a no-op. - if (!buffer.getReviewContext() && typeof context !== "undefined" && context && context.payload) { - const pr = context.payload.pull_request; - if (pr && pr.head && pr.head.sha) { - const repo = `${context.repo.owner}/${context.repo.repo}`; - buffer.setReviewContext({ - repo, - repoParts: { owner: context.repo.owner, repo: context.repo.repo }, - pullRequestNumber: pr.number, - pullRequest: pr, - }); - core.info(`Set review context from triggering PR: ${repo}#${pr.number}`); + // Use target config as single source of truth (same as add_comment): resolveTarget first, then use payload PR only when it matches. + if (!buffer.getReviewContext()) { + const repo = `${context.repo.owner}/${context.repo.repo}`; + const repoParts = { owner: context.repo.owner, repo: context.repo.repo }; + + const targetResult = resolveTarget({ + targetConfig, + item: message, + context, + itemType: "PR review", + supportsPR: false, + supportsIssue: false, + }); + + if (!targetResult.success) { + if (targetResult.shouldFail) { + core.warning(`Could not resolve PR for review context: ${targetResult.error}`); + } + } else if (targetResult.number) { + const prNum = targetResult.number; + const payloadPR = context.payload?.pull_request; + const usePayloadPR = payloadPR && payloadPR.number === prNum && payloadPR.head?.sha; + + if (usePayloadPR) { + buffer.setReviewContext({ + repo, + repoParts, + pullRequestNumber: payloadPR.number, + pullRequest: payloadPR, + }); + core.info(`Set review context from triggering PR: ${repo}#${payloadPR.number}`); + } else { + try { + const { data: fetchedPR } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNum, + }); + if (fetchedPR?.head?.sha) { + buffer.setReviewContext({ + repo, + repoParts, + pullRequestNumber: fetchedPR.number, + pullRequest: fetchedPR, + }); + core.info(`Set review context from target: ${repo}#${fetchedPR.number}`); + } else { + core.warning("Fetched PR missing head.sha - cannot set review context"); + } + } catch (fetchErr) { + core.warning(`Could not fetch PR #${prNum} for review context: ${getErrorMessage(fetchErr)}`); + } + } } } diff --git a/actions/setup/js/submit_pr_review.test.cjs b/actions/setup/js/submit_pr_review.test.cjs index 41d46c2d453..44c7c0e71de 100644 --- a/actions/setup/js/submit_pr_review.test.cjs +++ b/actions/setup/js/submit_pr_review.test.cjs @@ -245,8 +245,9 @@ describe("submit_pr_review (Handler Factory Architecture)", () => { }); it("should set review context from triggering PR for body-only reviews", async () => { - // Simulate a PR trigger context + // Simulate a PR trigger context (resolveTarget uses eventName) global.context = { + eventName: "pull_request", repo: { owner: "test-owner", repo: "test-repo" }, payload: { pull_request: { number: 42, head: { sha: "abc123" } }, @@ -274,10 +275,129 @@ describe("submit_pr_review (Handler Factory Architecture)", () => { expect(ctx.repo).toBe("test-owner/test-repo"); expect(ctx.pullRequestNumber).toBe(42); - // Clean up delete global.context; }); + it("should set review context from target config when explicit PR number (e.g. workflow_dispatch)", async () => { + const fetchedPR = { + number: 99, + head: { sha: "fetched-sha" }, + user: { login: "author" }, + }; + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "my-owner", repo: "my-repo" }, + payload: {}, + }; + global.github = { + rest: { + pulls: { + get: vi.fn().mockResolvedValue({ data: fetchedPR }), + }, + }, + }; + + const localBuffer = createReviewBuffer(); + const { main } = require("./submit_pr_review.cjs"); + const localHandler = await main({ max: 1, target: "99", _prReviewBuffer: localBuffer }); + + const message = { + type: "submit_pull_request_review", + body: "Approved via target", + event: "APPROVE", + }; + + const result = await localHandler(message, {}); + + expect(result.success).toBe(true); + expect(localBuffer.hasReviewMetadata()).toBe(true); + const ctx = localBuffer.getReviewContext(); + expect(ctx).not.toBeNull(); + expect(ctx.repo).toBe("my-owner/my-repo"); + expect(ctx.pullRequestNumber).toBe(99); + expect(ctx.pullRequest.head.sha).toBe("fetched-sha"); + expect(global.github.rest.pulls.get).toHaveBeenCalledWith({ + owner: "my-owner", + repo: "my-repo", + pull_number: 99, + }); + + delete global.context; + delete global.github; + }); + + it("should not set review context when target is triggering and not in PR context", async () => { + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "o", repo: "r" }, + payload: {}, + }; + + const localBuffer = createReviewBuffer(); + const { main } = require("./submit_pr_review.cjs"); + const localHandler = await main({ max: 1, _prReviewBuffer: localBuffer }); + + const message = { + type: "submit_pull_request_review", + body: "Review", + event: "COMMENT", + }; + + const result = await localHandler(message, {}); + + expect(result.success).toBe(true); + expect(localBuffer.hasReviewMetadata()).toBe(true); + expect(localBuffer.getReviewContext()).toBeNull(); + + delete global.context; + }); + + it("should set review context from target '*' with message.pull_request_number", async () => { + const fetchedPR = { + number: 5, + head: { sha: "sha5" }, + user: { login: "u" }, + }; + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "o", repo: "r" }, + payload: {}, + }; + global.github = { + rest: { + pulls: { + get: vi.fn().mockResolvedValue({ data: fetchedPR }), + }, + }, + }; + + const localBuffer = createReviewBuffer(); + const { main } = require("./submit_pr_review.cjs"); + const localHandler = await main({ max: 1, target: "*", _prReviewBuffer: localBuffer }); + + const message = { + type: "submit_pull_request_review", + body: "Review", + event: "APPROVE", + pull_request_number: 5, + }; + + const result = await localHandler(message, {}); + + expect(result.success).toBe(true); + const ctx = localBuffer.getReviewContext(); + expect(ctx).not.toBeNull(); + expect(ctx.pullRequestNumber).toBe(5); + expect(global.github.rest.pulls.get).toHaveBeenCalledWith({ + owner: "o", + repo: "r", + pull_number: 5, + }); + + delete global.context; + delete global.github; + }); + it("should not override existing review context from comments", async () => { // Pre-set context as if a comment handler already set it buffer.setReviewContext({ diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 2a8f0e9ab6a..2c2301da99b 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -2700,6 +2700,11 @@ safe-outputs: # (optional) max: 1 + # Target PR for the review: 'triggering' (default), '*' (any PR), or explicit PR number. + # Required when workflow is not triggered by a pull request (e.g. workflow_dispatch). + # (optional) + target: "triggering" + # Controls whether AI-generated footer is added to the review body. When false, # the footer is omitted. Defaults to true. # (optional) diff --git a/docs/src/content/docs/reference/safe-outputs-specification.md b/docs/src/content/docs/reference/safe-outputs-specification.md index 87891cda4b5..ce9559e6e04 100644 --- a/docs/src/content/docs/reference/safe-outputs-specification.md +++ b/docs/src/content/docs/reference/safe-outputs-specification.md @@ -1422,6 +1422,13 @@ add-comment: allowed-repos: [...] ``` +**Submit PR Review Extensions**: +```yaml +submit-pull-request-review: + target: "triggering" | "*" | # Required when not in pull_request trigger + footer: "always" | "none" | "if-body" # Footer on review body +``` + **Pull Request Extensions**: ```yaml create-pull-request: @@ -2317,6 +2324,7 @@ This section provides complete definitions for all remaining safe output types. **Notes**: - Submits all buffered review comments from `create_pull_request_review_comment` - Review status affects PR merge requirements +- **Target**: `target` accepts `"triggering"` (default), `"*"` (use `pull_request_number` from message), or an explicit PR number (e.g. `${{ github.event.inputs.pr_number }}`). Required when the workflow is not triggered by a pull request (e.g. `workflow_dispatch`). - Footer control: `footer` accepts `"always"` (default), `"none"`, or `"if-body"` (only when review body has text); boolean `true`/`false` maps to `"always"`/`"none"` --- diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 50c47d05d5d..69610b5049c 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -736,12 +736,15 @@ If the agent calls `submit_pull_request_review`, it can specify a review `body` If the agent does not call `submit_pull_request_review` at all, buffered comments are still submitted as a COMMENT review automatically. +When the workflow is not triggered by a pull request (e.g. `workflow_dispatch`), set `target` to the PR number (e.g. `${{ github.event.inputs.pr_number }}`) so the review can be submitted. Same semantics as [add-comment](#comment-creation-add-comment) `target`: `"triggering"` (default), `"*"` (use `pull_request_number` from the message), or an explicit number. + ```yaml wrap safe-outputs: create-pull-request-review-comment: max: 10 submit-pull-request-review: max: 1 # max reviews to submit (default: 1) + target: "triggering" # or "*", or e.g. ${{ github.event.inputs.pr_number }} when not in pull_request trigger footer: false # omit AI-generated footer from review body (default: true) ``` diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index ffc1f010928..5d4b5020b6f 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5039,6 +5039,10 @@ ], "description": "Controls when AI-generated footer is added to the review body. Accepts boolean (true/false) or string ('always', 'none', 'if-body'). The 'if-body' mode is useful for clean approval reviews without body text. Defaults to 'always'." }, + "target": { + "type": "string", + "description": "Target PR for the review: 'triggering' (default, current PR), '*' (any PR, requires pull_request_number in agent output), or explicit PR number (e.g. ${{ github.event.inputs.pr_number }}). Required when workflow is not triggered by a pull request (e.g. workflow_dispatch)." + }, "github-token": { "$ref": "#/$defs/github_token", "description": "GitHub token to use for this specific output type. Overrides global github-token if specified." diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index 10301718ab0..bcf3bbed2b1 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -331,6 +331,7 @@ var handlerRegistry = map[string]handlerBuilder{ c := cfg.SubmitPullRequestReview return newHandlerConfigBuilder(). AddIfPositive("max", c.Max). + AddIfNotEmpty("target", c.Target). AddStringPtr("footer", getEffectiveFooterString(c.Footer, cfg.Footer)). Build() }, diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index 542a8d436aa..686245b339c 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -84,6 +84,9 @@ func validateSafeOutputsTarget(config *SafeOutputsConfig) error { if config.CreatePullRequestReviewComments != nil { configs = append(configs, targetConfig{"create-pull-request-review-comment", config.CreatePullRequestReviewComments.Target}) } + if config.SubmitPullRequestReview != nil { + configs = append(configs, targetConfig{"submit-pull-request-review", config.SubmitPullRequestReview.Target}) + } if config.ReplyToPullRequestReviewComment != nil { configs = append(configs, targetConfig{"reply-to-pull-request-review-comment", config.ReplyToPullRequestReviewComment.Target}) } diff --git a/pkg/workflow/submit_pr_review.go b/pkg/workflow/submit_pr_review.go index 4ce76038570..8c8116a6d40 100644 --- a/pkg/workflow/submit_pr_review.go +++ b/pkg/workflow/submit_pr_review.go @@ -12,6 +12,7 @@ var submitPRReviewLog = logger.New("workflow:submit_pr_review") // If this safe output type is not configured, review comments default to event: "COMMENT". type SubmitPullRequestReviewConfig struct { BaseSafeOutputConfig `yaml:",inline"` + Target string `yaml:"target,omitempty"` // Target PR: "triggering" (default), "*" (use message.pull_request_number), or explicit number e.g. ${{ github.event.inputs.pr_number }} Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review body: "always" (default), "none", or "if-body" (only when review has body text) } @@ -31,6 +32,14 @@ func (c *Compiler) parseSubmitPullRequestReviewConfig(outputMap map[string]any) // Parse common base fields with default max of 1 c.parseBaseSafeOutputConfig(configMap, &config.BaseSafeOutputConfig, 1) + // Parse target (same semantics as add-comment / create-pull-request-review-comment) + if target, exists := configMap["target"]; exists { + if targetStr, ok := target.(string); ok && targetStr != "" { + config.Target = targetStr + submitPRReviewLog.Printf("Target: %s", config.Target) + } + } + // Parse footer configuration (string: "always"/"none"/"if-body", or bool for backward compat) if footer, exists := configMap["footer"]; exists { switch f := footer.(type) { diff --git a/pkg/workflow/submit_pr_review_footer_test.go b/pkg/workflow/submit_pr_review_footer_test.go index 17aa4bb2d6b..69fcd261965 100644 --- a/pkg/workflow/submit_pr_review_footer_test.go +++ b/pkg/workflow/submit_pr_review_footer_test.go @@ -128,6 +128,33 @@ func TestSubmitPRReviewFooterConfig(t *testing.T) { require.NotNil(t, config, "Config should be parsed") assert.Nil(t, config.Footer, "Footer should be nil when not configured") }) + + t.Run("parses target field", func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "submit-pull-request-review": map[string]any{ + "max": 1, + "target": "42", + }, + } + + config := compiler.parseSubmitPullRequestReviewConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + assert.Equal(t, "42", config.Target, "Target should be parsed") + }) + + t.Run("target empty when omitted", func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "submit-pull-request-review": map[string]any{ + "max": 1, + }, + } + + config := compiler.parseSubmitPullRequestReviewConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + assert.Empty(t, config.Target, "Target should be empty when not configured") + }) } func TestCreatePRReviewCommentNoFooter(t *testing.T) { @@ -233,4 +260,43 @@ func TestSubmitPRReviewFooterInHandlerConfig(t *testing.T) { } } }) + + t.Run("target included in submit_pull_request_review handler config when set", func(t *testing.T) { + compiler := NewCompiler() + targetValue := "123" + workflowData := &WorkflowData{ + Name: "Test", + SafeOutputs: &SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: 1}, + Target: targetValue, + }, + }, + } + + 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") + + submitConfig, ok := handlerConfig["submit_pull_request_review"].(map[string]any) + require.True(t, ok, "submit_pull_request_review config should exist") + assert.Equal(t, "123", submitConfig["target"], "Target should be in submit handler config") + } + } + } + }) }