Skip to content
Open
16 changes: 15 additions & 1 deletion .github/workflows/ponytail-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion .github/workflows/pr-code-quality-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
charm.land/bubbles/v2 v2.1.1 h1:7r55WzBxpo/R3z98hGmY7KKPd3ET6vsf0Fb9sDHOV60=
charm.land/bubbles/v2 v2.1.1/go.mod h1:GE6M31gaWZVXzGw73OeuTTgy4lX+OtkH0E5ymnNsHxo=
charm.land/bubbles/v2 v2.2.0 h1:9GEMewcejrNtVIxZ9Y2wSsWJamsWNsXa8MpMLnH4yI4=
charm.land/bubbles/v2 v2.2.0/go.mod h1:wdMgn+sje1KNXdwFizIWjbf328fIUBxqEmJ/vYPo8yc=
charm.land/bubbletea/v2 v2.0.8 h1:SxTJMhCAI3lbPmy4SgX5LWZ24AdINr4I6UEqzZvYJuY=
Expand Down Expand Up @@ -144,8 +142,6 @@ github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHP
github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8=
github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
github.com/mattn/go-runewidth v0.0.24 h1:cpokDiIn0MGnhdHwuWnJBITySJ20QyNGnY2kR/ay2DU=
github.com/mattn/go-runewidth v0.0.24/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs=
github.com/mattn/go-runewidth v0.0.27 h1:Feg/Oou5zI/wnpgDF6omIU0OokC9GxLC/WRknhVlIR0=
github.com/mattn/go-runewidth v0.0.27/go.mod h1:3qAiGCV4Koz/yuveO58qUefmUTRm8r0IGEXZ9jeHp/8=
github.com/mattn/go-shellwords v1.0.12 h1:M2zGm7EW6UQJvDeQxo4T51eKPurbeFbe8WtebGE2xrk=
Expand Down
23 changes: 23 additions & 0 deletions pkg/workflow/safe_outputs_tools_generation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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,
Comment on lines +355 to +360
"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 {
Expand Down
68 changes: 68 additions & 0 deletions pkg/workflow/safe_outputs_tools_generation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand Down Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The close_issue nil-config test (TestComputePropertyInjectionsNilCloseIssues) covers nil config explicitly — adding the same for submit_pull_request_review would mirror the pattern and document the nil contract.

💡 Suggested addition
func TestComputePropertyInjectionsNilSubmitPRReview(t *testing.T) {
    injections := computePropertyInjections(&SafeOutputsConfig{
        SubmitPullRequestReview: nil,
    })
    assert.NotContains(t, injections, "submit_pull_request_review")
}

Low-risk omission — the != nil guard already handles this — but the symmetry with close_issue tests makes the contract explicit.

@copilot please address this.

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{
Expand Down