Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
76 changes: 43 additions & 33 deletions actions/setup/js/submit_pr_review.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,13 @@
*/

const { resolveTarget } = require("./safe_output_helpers.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");

/** @type {string} Safe output type handled by this module */
const HANDLER_TYPE = "submit_pull_request_review";

// allowedRepos: this handler operates exclusively on the triggering repository.
// Cross-repository PR review submission is not supported; no target-repo allowlist
// check is required.

/** @type {Set<string>} Valid review event types */
const VALID_EVENTS = new Set(["APPROVE", "REQUEST_CHANGES", "COMMENT"]);

Expand All @@ -33,6 +30,7 @@ async function main(config = {}) {
const maxCount = config.max || 1;
const targetConfig = config.target || "triggering";
const buffer = config._prReviewBuffer;
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);

if (!buffer) {
Expand All @@ -43,6 +41,10 @@ async function main(config = {}) {
}

core.info(`Submit PR review handler initialized: max=${maxCount}, target=${targetConfig}`);
core.info(`Default target repo: ${defaultTargetRepo}`);
if (allowedRepos.size > 0) {
core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`);
}

let processedCount = 0;

Expand Down Expand Up @@ -109,39 +111,47 @@ async function main(config = {}) {
}
} else if (targetResult.number) {
const prNum = targetResult.number;
const repo = `${context.repo.owner}/${context.repo.repo}`;
const repoParts = { owner: context.repo.owner, repo: context.repo.repo };
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}`);

// Resolve and validate the target repository (supports cross-repo via target-repo config)
const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "PR review");
if (!repoResult.success) {
// Warn and leave context unset; submitReview() will subsequently fail
// with "No review context available" — this is not a silent failure.
core.warning(`Could not resolve repository for PR review context: ${repoResult.error}`);
} else {
try {
const { data: fetchedPR } = await githubClient.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: prNum,
const { repo, repoParts } = repoResult;
const payloadPR = context.payload?.pull_request;
const usePayloadPR = payloadPR && payloadPR.number === prNum && payloadPR.head?.sha && repo === `${context.repo.owner}/${context.repo.repo}`;

if (usePayloadPR) {
buffer.setReviewContext({
repo,
repoParts,
pullRequestNumber: payloadPR.number,
pullRequest: payloadPR,
});
if (fetchedPR?.head?.sha) {
buffer.setReviewContext({
repo,
repoParts,
pullRequestNumber: fetchedPR.number,
pullRequest: fetchedPR,
core.info(`Set review context from triggering PR: ${repo}#${payloadPR.number}`);
} else {
try {
const { data: fetchedPR } = await githubClient.rest.pulls.get({
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: prNum,
});
core.info(`Set review context from target: ${repo}#${fetchedPR.number}`);
} else {
core.warning("Fetched PR missing head.sha - cannot set review context");
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)}`);
}
} catch (fetchErr) {
core.warning(`Could not fetch PR #${prNum} for review context: ${getErrorMessage(fetchErr)}`);
}
}
}
Expand Down
143 changes: 143 additions & 0 deletions actions/setup/js/submit_pr_review.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,149 @@ describe("submit_pr_review (Handler Factory Architecture)", () => {
delete global.github;
});

it("should use target-repo for cross-repo PR review submission", async () => {
const fetchedPR = {
number: 1,
head: { sha: "consumer-sha" },
user: { login: "author" },
};
global.context = {
eventName: "workflow_dispatch",
repo: { owner: "provider-org", repo: "provider-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: "1",
"target-repo": "consumer-org/consumer-repo",
_prReviewBuffer: localBuffer,
});

const message = {
type: "submit_pull_request_review",
body: "LGTM",
event: "APPROVE",
};

const result = await localHandler(message, {});

expect(result.success).toBe(true);
const ctx = localBuffer.getReviewContext();
expect(ctx).not.toBeNull();
expect(ctx.repo).toBe("consumer-org/consumer-repo");
expect(ctx.pullRequestNumber).toBe(1);
expect(ctx.pullRequest.head.sha).toBe("consumer-sha");
// Ensure API was called on consumer-repo, not provider-repo
expect(global.github.rest.pulls.get).toHaveBeenCalledWith({
owner: "consumer-org",
repo: "consumer-repo",
pull_number: 1,
});

delete global.context;
delete global.github;
});

it("should use target-repo with allowed-repos for cross-repo review", async () => {
const fetchedPR = {
number: 7,
head: { sha: "allowed-sha" },
user: { login: "author" },
};
global.context = {
eventName: "workflow_dispatch",
repo: { owner: "provider-org", repo: "provider-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: "7",
"target-repo": "consumer-org/consumer-repo",
allowed_repos: ["consumer-org/other-repo"],
_prReviewBuffer: localBuffer,
});

// Message with explicit repo matching an allowed repo
const message = {
type: "submit_pull_request_review",
body: "Looks good",
event: "APPROVE",
repo: "consumer-org/other-repo",
};

const result = await localHandler(message, {});

expect(result.success).toBe(true);
const ctx = localBuffer.getReviewContext();
expect(ctx).not.toBeNull();
expect(ctx.repo).toBe("consumer-org/other-repo");
expect(global.github.rest.pulls.get).toHaveBeenCalledWith({
owner: "consumer-org",
repo: "other-repo",
pull_number: 7,
});

delete global.context;
delete global.github;
});

it("should reject cross-repo review when repo is not in allowed list", async () => {
global.context = {
eventName: "workflow_dispatch",
repo: { owner: "provider-org", repo: "provider-repo" },
payload: {},
};

const localBuffer = createReviewBuffer();
const { main } = require("./submit_pr_review.cjs");
const localHandler = await main({
max: 1,
target: "1",
"target-repo": "consumer-org/consumer-repo",
_prReviewBuffer: localBuffer,
});

// Message with explicit repo that is not allowed
const message = {
type: "submit_pull_request_review",
body: "Attempting to review disallowed repo",
event: "APPROVE",
repo: "attacker-org/attacker-repo",
};

const result = await localHandler(message, {});

// Metadata is stored successfully (success refers to buffering, not final submission)
expect(result.success).toBe(true);
expect(localBuffer.hasReviewMetadata()).toBe(true);

// Review context should NOT be set because the disallowed repo was rejected
// submitReview() will subsequently fail with "No review context available"
expect(localBuffer.getReviewContext()).toBeNull();

delete global.context;
});

it("should not override existing review context from comments", async () => {
// Pre-set context as if a comment handler already set it
buffer.setReviewContext({
Expand Down
3 changes: 3 additions & 0 deletions actions/setup/js/types/safe-outputs-config.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ interface CreatePullRequestReviewCommentConfig extends SafeOutputConfig {
* Boolean values are also supported: true maps to "always", false maps to "none".
*/
interface SubmitPullRequestReviewConfig extends SafeOutputConfig {
target?: string;
"target-repo"?: string;
"allowed-repos"?: string[];
footer?: boolean | "always" | "none" | "if-body";
}

Expand Down
4 changes: 4 additions & 0 deletions docs/src/content/docs/reference/safe-outputs.md
Original file line number Diff line number Diff line change
Expand Up @@ -758,13 +758,17 @@ If the agent does not call `submit_pull_request_review` at all, buffered comment

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.

For cross-repository scenarios, use `target-repo` to specify the repository where the PR lives. This mirrors the behavior of `create-pull-request-review-comment` and `add-comment`.

```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
target-repo: "owner/repo" # cross-repository: submit review on PR in another repo
allowed-repos: ["org/repo1", "org/repo2"] # additional allowed repositories
footer: false # omit AI-generated footer from review body (default: true)
```

Expand Down
2 changes: 2 additions & 0 deletions pkg/workflow/compiler_safe_outputs_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ var handlerRegistry = map[string]handlerBuilder{
return newHandlerConfigBuilder().
AddTemplatableInt("max", c.Max).
AddIfNotEmpty("target", c.Target).
AddIfNotEmpty("target-repo", c.TargetRepoSlug).
AddStringSlice("allowed_repos", c.AllowedRepos).
AddIfNotEmpty("github-token", c.GitHubToken).
AddStringPtr("footer", getEffectiveFooterString(c.Footer, cfg.Footer)).
Build()
Expand Down
18 changes: 14 additions & 4 deletions pkg/workflow/submit_pr_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ var submitPRReviewLog = logger.New("workflow:submit_pr_review")
// are collected and submitted as a single PR review with the configured event type.
// 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)
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
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)
}

// parseSubmitPullRequestReviewConfig handles submit-pull-request-review configuration
Expand All @@ -32,13 +32,21 @@ 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)
// Parse target config (target, target-repo, allowed-repos)
// Uses parseTargetRepoWithValidation to disallow wildcard "*" for target-repo
if target, exists := configMap["target"]; exists {
if targetStr, ok := target.(string); ok {
config.Target = targetStr
}
}

targetRepoSlug, isInvalid := parseTargetRepoWithValidation(configMap)
if isInvalid {
return nil // Invalid configuration, return nil to cause validation error
}
config.TargetRepoSlug = targetRepoSlug
config.AllowedRepos = parseAllowedReposFromConfig(configMap)

// Parse footer configuration (string: "always"/"none"/"if-body", or bool for backward compat)
if footer, exists := configMap["footer"]; exists {
switch f := footer.(type) {
Expand All @@ -62,6 +70,8 @@ func (c *Compiler) parseSubmitPullRequestReviewConfig(outputMap map[string]any)
submitPRReviewLog.Printf("Footer control (mapped from bool): %s", footerStr)
}
}

submitPRReviewLog.Printf("Parsed submit-pull-request-review config: max=%d, target=%s, target_repo=%s", templatableIntValue(config.Max), config.Target, config.TargetRepoSlug)
} else {
// If configData is nil or not a map, set the default max
config.Max = defaultIntStr(1)
Expand Down
Loading
Loading