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
16 changes: 16 additions & 0 deletions actions/setup/js/submit_pr_review.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ async function main(config = {}) {
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);

// Build the allowed events set from config (empty set means all events are allowed)
const allowedEvents = new Set(Array.isArray(config.allowed_events) && config.allowed_events.length > 0 ? config.allowed_events.map(e => String(e).toUpperCase()) : []);

if (!buffer) {
core.warning("submit_pull_request_review: No PR review buffer provided in config");
return async function handleSubmitPRReview() {
Expand All @@ -45,6 +48,9 @@ async function main(config = {}) {
if (allowedRepos.size > 0) {
core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`);
}
if (allowedEvents.size > 0) {
core.info(`Allowed review events: ${Array.from(allowedEvents).join(", ")}`);
}

// Propagate per-handler staged flag to the shared PR review buffer
if (config.staged === true) {
Expand Down Expand Up @@ -81,6 +87,16 @@ async function main(config = {}) {
};
}

// Enforce allowed-events filter (infrastructure-level enforcement)
if (allowedEvents.size > 0 && !allowedEvents.has(event)) {
const allowedList = Array.from(allowedEvents).join(", ");
core.warning(`Review event '${event}' is not allowed. Allowed events: ${allowedList}`);
return {
success: false,
error: `Review event '${event}' is not allowed by safe-outputs configuration. Allowed events: ${allowedList}`,
};
}

// Body is required for REQUEST_CHANGES per GitHub API docs;
// optional for APPROVE and COMMENT
const body = message.body || "";
Expand Down
77 changes: 77 additions & 0 deletions actions/setup/js/submit_pr_review.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,83 @@ describe("submit_pr_review (Handler Factory Architecture)", () => {
expect(result.error).toContain("Invalid review event");
});

it("should allow all events when allowed_events not configured", async () => {
const { main } = require("./submit_pr_review.cjs");

for (const event of ["APPROVE", "COMMENT", "REQUEST_CHANGES"]) {
const localBuffer = createReviewBuffer();
const h = await main({ max: 5, _prReviewBuffer: localBuffer });
const result = await h({ event, body: event === "REQUEST_CHANGES" ? "body" : "" }, {});
expect(result.success).toBe(true);
expect(result.event).toBe(event);
}
});

it("should block APPROVE when allowed_events is [COMMENT, REQUEST_CHANGES]", async () => {
const { main } = require("./submit_pr_review.cjs");
const localBuffer = createReviewBuffer();
const localHandler = await main({
max: 5,
_prReviewBuffer: localBuffer,
allowed_events: ["COMMENT", "REQUEST_CHANGES"],
});

const result = await localHandler({ event: "APPROVE", body: "" }, {});

expect(result.success).toBe(false);
expect(result.error).toContain("not allowed");
expect(result.error).toContain("APPROVE");
});

it("should allow COMMENT when allowed_events is [COMMENT, REQUEST_CHANGES]", async () => {
const { main } = require("./submit_pr_review.cjs");
const localBuffer = createReviewBuffer();
const localHandler = await main({
max: 5,
_prReviewBuffer: localBuffer,
allowed_events: ["COMMENT", "REQUEST_CHANGES"],
});

const result = await localHandler({ event: "COMMENT", body: "some feedback" }, {});

expect(result.success).toBe(true);
expect(result.event).toBe("COMMENT");
});

it("should normalize event case before checking allowed_events", async () => {
const { main } = require("./submit_pr_review.cjs");
const localBuffer = createReviewBuffer();
const localHandler = await main({
max: 5,
_prReviewBuffer: localBuffer,
allowed_events: ["COMMENT"],
});

const result = await localHandler({ event: "comment", body: "feedback" }, {});

expect(result.success).toBe(true);
expect(result.event).toBe("COMMENT");
});

it("should not consume max count slot when event is blocked by allowed_events", async () => {
const { main } = require("./submit_pr_review.cjs");
const localBuffer = createReviewBuffer();
const localHandler = await main({
max: 1,
_prReviewBuffer: localBuffer,
allowed_events: ["COMMENT"],
});

// Blocked event should not consume a slot
const blocked = await localHandler({ event: "APPROVE", body: "" }, {});
expect(blocked.success).toBe(false);

// Allowed event should still succeed since blocked one didn't count
const allowed = await localHandler({ event: "COMMENT", body: "feedback" }, {});
expect(allowed.success).toBe(true);
expect(allowed.event).toBe("COMMENT");
});

it("should allow empty body for APPROVE event", async () => {
const message = {
type: "submit_pull_request_review",
Expand Down
1 change: 1 addition & 0 deletions actions/setup/js/types/safe-outputs-config.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ interface SubmitPullRequestReviewConfig extends SafeOutputConfig {
target?: string;
"target-repo"?: string;
"allowed-repos"?: string[];
"allowed-events"?: Array<"APPROVE" | "COMMENT" | "REQUEST_CHANGES">;
footer?: boolean | "always" | "none" | "if-body";
}

Expand Down
7 changes: 7 additions & 0 deletions docs/src/content/docs/reference/frontmatter-full.md
Original file line number Diff line number Diff line change
Expand Up @@ -4039,6 +4039,13 @@ safe-outputs:
allowed-repos: []
# Array of strings

# Optional list of allowed review event types. If omitted, all event types
# (APPROVE, COMMENT, REQUEST_CHANGES) are allowed. Use this to restrict the agent
# to specific event types at the infrastructure level.
# (optional)
allowed-events: []
# Array of strings (APPROVE, COMMENT, REQUEST_CHANGES)

# GitHub token to use for this specific output type. Overrides global github-token
# if specified.
# (optional)
Expand Down
3 changes: 3 additions & 0 deletions docs/src/content/docs/reference/safe-outputs.md
Original file line number Diff line number Diff line change
Expand Up @@ -734,9 +734,12 @@ safe-outputs:
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
allowed-events: [COMMENT, REQUEST_CHANGES] # restrict allowed review event types (default: all allowed)
footer: false # omit AI-generated footer from review body (default: true)
```

Use `allowed-events` to restrict which review event types the agent can submit. This provides infrastructure-level enforcement — for example, `allowed-events: [COMMENT, REQUEST_CHANGES]` prevents the agent from submitting APPROVE reviews regardless of what the agent attempts to output. If omitted, all event types (APPROVE, COMMENT, REQUEST_CHANGES) are allowed.

### Resolve PR Review Thread (`resolve-pull-request-review-thread:`)

Resolves review threads on pull requests. Allows AI agents to mark review conversations as resolved after addressing the feedback. Uses the GitHub GraphQL API with the `resolveReviewThread` mutation.
Expand Down
19 changes: 16 additions & 3 deletions pkg/parser/schemas/main_workflow_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1887,7 +1887,7 @@
},
"status-comment": {
"type": "boolean",
"description": "Whether to post status comments (started/completed) on the triggering item. When true, adds a comment with workflow run link and updates it on completion. When false or not specified, no status comments are posted. Automatically enabled for slash_command and label_command triggers manual configuration is only needed for other trigger types.",
"description": "Whether to post status comments (started/completed) on the triggering item. When true, adds a comment with workflow run link and updates it on completion. When false or not specified, no status comments are posted. Automatically enabled for slash_command and label_command triggers \u2014 manual configuration is only needed for other trigger types.",
"examples": [true, false]
},
"github-token": {
Expand Down Expand Up @@ -5907,6 +5907,15 @@
},
"description": "List of additional repositories in format 'owner/repo' that PR reviews can be submitted in. When specified, the agent can use a 'repo' field in the output to specify which repository to submit the review in. The target repository (current or target-repo) is always implicitly allowed."
},
"allowed-events": {
"type": "array",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot add constraint for minimum 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added minItems: 1 to the allowed-events schema in commit 5b475a7. Also addressed the security reviewer's fail-closed feedback in the same commit: the parser now returns nil if allowed-events is present but not a list, or if all provided values are invalid (rather than silently falling back to allowing all events).

"items": {
"type": "string",
"enum": ["APPROVE", "COMMENT", "REQUEST_CHANGES"]
},
"description": "Optional list of allowed review event types. If omitted, all event types (APPROVE, COMMENT, REQUEST_CHANGES) are allowed. Use this to restrict the agent to specific event types, e.g. [COMMENT, REQUEST_CHANGES] to prevent approvals.",
"minItems": 1
},
"github-token": {
"$ref": "#/$defs/github_token",
"description": "GitHub token to use for this specific output type. Overrides global github-token if specified."
Expand Down Expand Up @@ -7553,12 +7562,16 @@
"properties": {
"include": {
"type": "array",
"items": { "type": "string" },
"items": {
"type": "string"
},
"description": "Glob patterns for files to include"
},
"exclude": {
"type": "array",
"items": { "type": "string" },
"items": {
"type": "string"
},
"description": "Glob patterns for files to exclude"
}
},
Expand Down
1 change: 1 addition & 0 deletions pkg/workflow/compiler_safe_outputs_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ var handlerRegistry = map[string]handlerBuilder{
AddIfNotEmpty("target", c.Target).
AddIfNotEmpty("target-repo", c.TargetRepoSlug).
AddStringSlice("allowed_repos", c.AllowedRepos).
AddStringSlice("allowed_events", c.AllowedEvents).
AddIfNotEmpty("github-token", c.GitHubToken).
AddStringPtr("footer", getEffectiveFooterString(c.Footer, cfg.Footer)).
Comment on lines 461 to 466
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

There are JS + Go parse unit tests for allowed-events, but there’s no test asserting the compiler actually emits allowed_events into GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG for submit_pull_request_review. Add a focused test that sets SubmitPullRequestReview.AllowedEvents and verifies the generated handler config JSON contains the expected allowed_events array, to prevent regressions in the Go→JS config wiring.

Copilot uses AI. Check for mistakes.
AddIfTrue("staged", c.Staged).
Expand Down
33 changes: 31 additions & 2 deletions pkg/workflow/submit_pr_review.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package workflow

import (
"strings"

"github.com/github/gh-aw/pkg/logger"
)

Expand All @@ -13,7 +15,8 @@ var submitPRReviewLog = logger.New("workflow:submit_pr_review")
type SubmitPullRequestReviewConfig struct {
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)
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)
AllowedEvents []string `yaml:"allowed-events,omitempty"` // Optional list of allowed review event types: APPROVE, COMMENT, REQUEST_CHANGES. If omitted, all event types are allowed.
}

// parseSubmitPullRequestReviewConfig handles submit-pull-request-review configuration
Expand Down Expand Up @@ -71,7 +74,33 @@ func (c *Compiler) parseSubmitPullRequestReviewConfig(outputMap map[string]any)
}
}

submitPRReviewLog.Printf("Parsed submit-pull-request-review config: max=%d, target=%s, target_repo=%s", templatableIntValue(config.Max), config.Target, config.TargetRepoSlug)
// Parse allowed-events configuration
if allowedEvents, exists := configMap["allowed-events"]; exists {
eventsSlice, ok := allowedEvents.([]any)
if !ok {
submitPRReviewLog.Printf("Invalid allowed-events configuration: must be a list of review event types")
return nil
}

validEvents := map[string]bool{"APPROVE": true, "COMMENT": true, "REQUEST_CHANGES": true}
for _, e := range eventsSlice {
if eventStr, ok := e.(string); ok {
upper := strings.ToUpper(eventStr)
if validEvents[upper] {
config.AllowedEvents = append(config.AllowedEvents, upper)
} else {
submitPRReviewLog.Printf("Ignoring invalid allowed-events value: %s", eventStr)
}
}
}

if len(config.AllowedEvents) == 0 {
submitPRReviewLog.Printf("Invalid allowed-events configuration: at least one valid event type is required when allowed-events is specified")
return nil
}
}

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