diff --git a/.github/workflows/ponytail-reviewer.lock.yml b/.github/workflows/ponytail-reviewer.lock.yml index 1e358d3522e..28be194c113 100644 --- a/.github/workflows/ponytail-reviewer.lock.yml +++ b/.github/workflows/ponytail-reviewer.lock.yml @@ -691,7 +691,21 @@ jobs: "submit_pull_request_review": " CONSTRAINTS: Maximum 1 review(s) can be submitted." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [], + "property_injections": { + "submit_pull_request_review": { + "event": { + "description": "Review decision. Restricted by allowed-events configuration to: COMMENT.", + "enum": [ + "COMMENT" + ], + "type": "string", + "x-synonyms": [ + "action" + ] + } + } + } } GH_AW_VALIDATION_JSON: | { diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml index b2c8d9ba96b..6a031f4a1ef 100644 --- a/.github/workflows/pr-code-quality-reviewer.lock.yml +++ b/.github/workflows/pr-code-quality-reviewer.lock.yml @@ -665,7 +665,22 @@ jobs: "submit_pull_request_review": " CONSTRAINTS: Maximum 1 review(s) can be submitted." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [], + "property_injections": { + "submit_pull_request_review": { + "event": { + "description": "Review decision. Restricted by allowed-events configuration to: COMMENT, REQUEST_CHANGES.", + "enum": [ + "COMMENT", + "REQUEST_CHANGES" + ], + "type": "string", + "x-synonyms": [ + "action" + ] + } + } + } } GH_AW_VALIDATION_JSON: | { diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index 0025c793fc1..3754812017d 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -4,6 +4,8 @@ import ( "encoding/json" "fmt" "path/filepath" + "slices" + "strings" "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/stringutil" @@ -277,6 +279,11 @@ func computeRequiredFieldAdditions(safeOutputs *SafeOutputsConfig) map[string][] if safeOutputs.AssignToAgent != nil && issueIntentRequired(safeOutputs.AssignToAgent.IssueIntent) { additions["assign_to_agent"] = issueIntentRequiredFields } + if safeOutputs.SubmitPullRequestReview != nil && len(safeOutputs.SubmitPullRequestReview.AllowedEvents) > 0 { + if !slices.Contains(safeOutputs.SubmitPullRequestReview.AllowedEvents, "COMMENT") { + additions["submit_pull_request_review"] = []string{"event"} + } + } return additions } @@ -341,6 +348,22 @@ func computePropertyInjections(safeOutputs *SafeOutputsConfig) map[string]map[st } } + // submit_pull_request_review event: when allowed-events restricts the set of review + // decisions, narrow the tool schema's event enum to match so the agent cannot select + // an event that runtime policy will reject. This retains runtime enforcement as + // defense in depth while preventing the doomed call in the first place. + if safeOutputs.SubmitPullRequestReview != nil && len(safeOutputs.SubmitPullRequestReview.AllowedEvents) > 0 { + allowedEvents := safeOutputs.SubmitPullRequestReview.AllowedEvents + injections["submit_pull_request_review"] = map[string]any{ + "event": map[string]any{ + "type": "string", + "enum": allowedEvents, + "description": "Review decision. Restricted by allowed-events configuration to: " + strings.Join(allowedEvents, ", ") + ".", + "x-synonyms": []string{"action"}, + }, + } + } + if safeOutputs.DataEnabled { dataProperty := map[string]any{"$ref": "#/0/inputSchema/$defs/structured_data"} if safeOutputs.NormalizedDataSchema != nil { diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index d5cf62a6a46..987c295645a 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -509,6 +509,26 @@ func TestComputeRequiredFieldAdditionsDisabledByDefault(t *testing.T) { assert.Empty(t, additions) } +func TestComputeRequiredFieldAdditionsSubmitPRReviewEventRequiredWhenCommentDisallowed(t *testing.T) { + additions := computeRequiredFieldAdditions(&SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + AllowedEvents: []string{"APPROVE"}, + }, + }) + + assert.Equal(t, []string{"event"}, additions["submit_pull_request_review"]) +} + +func TestComputeRequiredFieldAdditionsSubmitPRReviewEventOptionalWhenCommentAllowed(t *testing.T) { + additions := computeRequiredFieldAdditions(&SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + AllowedEvents: []string{"COMMENT", "REQUEST_CHANGES"}, + }, + }) + + assert.NotContains(t, additions, "submit_pull_request_review") +} + func TestComputeRequiredFieldAdditionsIssueIntentDefaultDisabled(t *testing.T) { additions := computeRequiredFieldAdditions(&SafeOutputsConfig{ CloseIssues: &CloseIssuesConfig{}, @@ -588,6 +608,54 @@ func TestComputePropertyInjectionsNilCloseIssues(t *testing.T) { assert.Empty(t, injections) } +// TestComputePropertyInjectionsNilSubmitPRReview verifies that nil submit-pull-request-review +// does not add submit_pull_request_review property injections. +func TestComputePropertyInjectionsNilSubmitPRReview(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + SubmitPullRequestReview: nil, + }) + assert.NotContains(t, injections, "submit_pull_request_review") +} + +// TestComputePropertyInjectionsAllowedEventsSubmitPRReview verifies that a configured +// allowed-events list narrows the submit_pull_request_review event enum in the tool schema. +func TestComputePropertyInjectionsAllowedEventsSubmitPRReview(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + AllowedEvents: []string{"COMMENT"}, + }, + }) + + require.Contains(t, injections, "submit_pull_request_review") + prop, ok := injections["submit_pull_request_review"]["event"].(map[string]any) + require.True(t, ok, "event should be a property map") + assert.Equal(t, []string{"COMMENT"}, prop["enum"]) +} + +// TestComputePropertyInjectionsAllowedEventsMultipleSubmitPRReview verifies multiple allowed +// events are all present in the narrowed enum. +func TestComputePropertyInjectionsAllowedEventsMultipleSubmitPRReview(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + AllowedEvents: []string{"COMMENT", "REQUEST_CHANGES"}, + }, + }) + + prop, ok := injections["submit_pull_request_review"]["event"].(map[string]any) + require.True(t, ok, "event should be a property map") + assert.Equal(t, []string{"COMMENT", "REQUEST_CHANGES"}, prop["enum"]) +} + +// TestComputePropertyInjectionsNoAllowedEventsSubmitPRReview verifies that no injection +// happens when allowed-events is not configured, so the static schema's full enum applies. +func TestComputePropertyInjectionsNoAllowedEventsSubmitPRReview(t *testing.T) { + injections := computePropertyInjections(&SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{}, + }) + + assert.NotContains(t, injections, "submit_pull_request_review", "no allowed-events should not inject an event enum") +} + // TestPreprocessStateReasonListSlice verifies that a []any slice is converted to allowed-state-reason. func TestPreprocessStateReasonListSlice(t *testing.T) { configData := map[string]any{ diff --git a/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden b/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden index 77630551569..74c7e1a1a4c 100644 --- a/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden +++ b/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden @@ -511,10 +511,10 @@ jobs: gh aw version env: GH_TOKEN: ${{ github.token }} - - name: Copy gh-aw binary for MCP server + - name: Copy gh-aw binary for MCP Server run: | gh aw --version - # Copy the gh-aw binary to ${RUNNER_TEMP}/gh-aw for MCP server containerization + # Copy the gh-aw binary to ${RUNNER_TEMP}/gh-aw for MCP Server containerization mkdir -p "${RUNNER_TEMP}/gh-aw" GH_AW_BIN="" GH_AW_BIN=$(command -v gh-aw 2>/dev/null) || true @@ -532,7 +532,7 @@ jobs: chmod +x "${RUNNER_TEMP}/gh-aw/gh-aw" echo "Copied gh-aw binary to ${RUNNER_TEMP}/gh-aw/gh-aw" else - echo "::error::Failed to find gh-aw binary for MCP server" + echo "::error::Failed to find gh-aw binary for MCP Server" exit 1 fi - name: Start MCP Gateway