diff --git a/.github/workflows/agentic_commands.yml b/.github/workflows/agentic_commands.yml index f1ffd58c552..9711c64bc32 100644 --- a/.github/workflows/agentic_commands.yml +++ b/.github/workflows/agentic_commands.yml @@ -29,10 +29,10 @@ # necromancer -> necromancer [pull_request] reaction=eyes # needs-design -> approach-validator [issues,pull_request] reaction=eyes # pull-request reviewers: -# design-decision-gate [pull_request,pull_request_review] -# mattpocock-skills-reviewer [pull_request,pull_request_review] -# pr-code-quality-reviewer [pull_request,pull_request_review] -# test-quality-sentinel [pull_request,pull_request_review] +# design-decision-gate [pull_request] +# mattpocock-skills-reviewer [pull_request] +# pr-code-quality-reviewer [pull_request] +# test-quality-sentinel [pull_request] # ___ _ _ # / _ \ | | (_) # | |_| | __ _ ___ _ __ | |_ _ ___ @@ -64,8 +64,6 @@ on: types: [created, edited] pull_request: types: [edited, labeled, opened, ready_for_review, reopened, review_requested] - pull_request_review: - types: [submitted] pull_request_review_comment: types: [created, edited] discussion: @@ -99,7 +97,7 @@ jobs: env: GH_AW_SLASH_ROUTING: '{"ace":[{"workflow":"ace-editor","events":["pull_request_comment"],"ai_reaction":"eyes"}],"approach-validator":[{"workflow":"approach-validator","events":["issue_comment","pull_request_comment"],"ai_reaction":"eyes"}],"archie":[{"workflow":"archie","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes"}],"brave":[{"workflow":"brave","events":["issue_comment"],"ai_reaction":"eyes"}],"cloclo":[{"workflow":"cloclo","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"craft":[{"workflow":"craft","events":["issues"],"ai_reaction":"eyes"}],"design-decision-gate":[{"workflow":"design-decision-gate","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"grumpy":[{"workflow":"grumpy-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"matt":[{"workflow":"mattpocock-skills-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"mergefest":[{"workflow":"mergefest","events":["pull_request_comment"],"ai_reaction":"eyes"}],"nit":[{"workflow":"pr-nitpick-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"plan":[{"workflow":"plan","events":["discussion_comment","issue_comment"],"ai_reaction":"eyes"}],"poem-bot":[{"workflow":"poem-bot","events":["issues"],"ai_reaction":"eyes"}],"review":[{"workflow":"pr-code-quality-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"scout":[{"workflow":"scout","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"security-review":[{"workflow":"security-review","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"summarize":[{"workflow":"pdf-summary","events":["issue_comment","issues"],"ai_reaction":"eyes"}],"test-quality-sentinel":[{"workflow":"test-quality-sentinel","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"}],"tidy":[{"workflow":"tidy","events":["pull_request_comment"],"ai_reaction":"eyes"}],"unbloat":[{"workflow":"unbloat-docs","events":["pull_request_comment"],"ai_reaction":"eyes"}]}' GH_AW_LABEL_ROUTING: '{"approach-proposal":[{"workflow":"approach-validator","events":["issues","pull_request"],"ai_reaction":"eyes"}],"ci-doctor":[{"workflow":"ci-doctor","events":["pull_request"],"ai_reaction":"eyes"}],"cloclo":[{"workflow":"cloclo","events":["discussion","issues","pull_request"],"ai_reaction":"eyes"}],"dev":[{"workflow":"dev","events":["discussion","issues","pull_request"],"ai_reaction":"eyes"}],"necromancer":[{"workflow":"necromancer","events":["pull_request"],"ai_reaction":"eyes"}],"needs-design":[{"workflow":"approach-validator","events":["issues","pull_request"],"ai_reaction":"eyes"}]}' - GH_AW_REVIEWER_ROUTING: '[{"workflow":"design-decision-gate","events":["pull_request","pull_request_review"]},{"workflow":"mattpocock-skills-reviewer","events":["pull_request","pull_request_review"]},{"workflow":"pr-code-quality-reviewer","events":["pull_request","pull_request_review"]},{"workflow":"test-quality-sentinel","events":["pull_request","pull_request_review"]}]' + GH_AW_REVIEWER_ROUTING: '[{"workflow":"design-decision-gate","events":["pull_request"]},{"workflow":"mattpocock-skills-reviewer","events":["pull_request"]},{"workflow":"pr-code-quality-reviewer","events":["pull_request"]},{"workflow":"test-quality-sentinel","events":["pull_request"]}]' with: script: | const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs'); diff --git a/.github/workflows/mattpocock-skills-reviewer.lock.yml b/.github/workflows/mattpocock-skills-reviewer.lock.yml index 22fc9208276..71458fcfa22 100644 --- a/.github/workflows/mattpocock-skills-reviewer.lock.yml +++ b/.github/workflows/mattpocock-skills-reviewer.lock.yml @@ -62,9 +62,6 @@ on: types: - ready_for_review - review_requested - pull_request_review: - types: - - submitted workflow_dispatch: inputs: aw_context: diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml index 13a98748f65..50ea141163e 100644 --- a/.github/workflows/pr-code-quality-reviewer.lock.yml +++ b/.github/workflows/pr-code-quality-reviewer.lock.yml @@ -63,9 +63,6 @@ on: types: - ready_for_review - review_requested - pull_request_review: - types: - - submitted workflow_dispatch: inputs: aw_context: diff --git a/.github/workflows/test-quality-sentinel.lock.yml b/.github/workflows/test-quality-sentinel.lock.yml index 88af6399522..41e6638e00b 100644 --- a/.github/workflows/test-quality-sentinel.lock.yml +++ b/.github/workflows/test-quality-sentinel.lock.yml @@ -61,9 +61,6 @@ on: types: - ready_for_review - review_requested - pull_request_review: - types: - - submitted workflow_dispatch: inputs: aw_context: diff --git a/actions/setup/js/route_slash_command.cjs b/actions/setup/js/route_slash_command.cjs index b51df1bd72a..d063a3de555 100644 --- a/actions/setup/js/route_slash_command.cjs +++ b/actions/setup/js/route_slash_command.cjs @@ -2,7 +2,6 @@ /// const { REACTION_MAP } = require("./add_reaction.cjs"); -const { extractWorkflowId } = require("./generate_footer.cjs"); // Keep this aligned with the current default stable GitHub REST API version used by workflows. // Update when GitHub advances the recommended version to avoid sunset/deprecation warnings. const GITHUB_API_VERSION = "2022-11-28"; @@ -272,27 +271,12 @@ async function main() { return; } - if ((context.eventName === "pull_request" || context.eventName === "pull_request_review") && Array.isArray(reviewerRoutes) && reviewerRoutes.length > 0) { + if (context.eventName === "pull_request" && Array.isArray(reviewerRoutes) && reviewerRoutes.length > 0) { const matches = reviewerRoutes.filter(route => Array.isArray(route.events) && route.events.includes(context.eventName)); if (matches.length > 0) { let selected = matches; - if (context.eventName === "pull_request") { - if (!["ready_for_review", "review_requested"].includes(context.payload?.action ?? "")) { - selected = []; - } - } else if (context.eventName === "pull_request_review") { - const action = context.payload?.action ?? ""; - if (action !== "submitted") { - selected = []; - } else { - const workflowId = extractWorkflowId(context.payload?.review?.body ?? ""); - if (workflowId) { - selected = selected.filter(route => route.workflow === workflowId); - } else { - selected = []; - core.info("No workflow marker found in pull request review body; skipping reviewer dispatch."); - } - } + if (!["ready_for_review", "review_requested"].includes(context.payload?.action ?? "")) { + selected = []; } if (selected.length > 0) { core.info(`Matched reviewer routes on '${context.eventName}': ${selected.map(route => route.workflow).join(", ")}.`); diff --git a/actions/setup/js/route_slash_command.test.cjs b/actions/setup/js/route_slash_command.test.cjs index 102de77e508..491dc5d4605 100644 --- a/actions/setup/js/route_slash_command.test.cjs +++ b/actions/setup/js/route_slash_command.test.cjs @@ -228,7 +228,7 @@ describe("route_slash_command", () => { it("dispatches reviewer lifecycle routes on pull_request ready_for_review", async () => { globals.context.eventName = "pull_request"; globals.context.payload = { action: "ready_for_review", pull_request: { number: 12, state: "open" } }; - process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request", "pull_request_review"] }]); + process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request"] }]); await main(); @@ -242,7 +242,7 @@ describe("route_slash_command", () => { it("dispatches reviewer lifecycle routes on pull_request review_requested", async () => { globals.context.eventName = "pull_request"; globals.context.payload = { action: "review_requested", pull_request: { number: 12, state: "open" } }; - process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request", "pull_request_review"] }]); + process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request"] }]); await main(); @@ -264,35 +264,14 @@ describe("route_slash_command", () => { expect(globals.core.info).toHaveBeenCalledWith(expect.stringContaining("Pull request is closed at workflow start")); }); - it("routes pull_request_review using workflow marker in review body", async () => { - globals.context.eventName = "pull_request_review"; - globals.context.payload = { - action: "submitted", - pull_request: { number: 9, state: "open" }, - review: { body: "Looks good\n" }, - }; - process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([ - { workflow: "pr-reviewer", events: ["pull_request_review"] }, - { workflow: "other-reviewer", events: ["pull_request_review"] }, - ]); - - await main(); - - expect(dispatchCalls).toHaveLength(1); - expect(dispatchCalls[0].workflow_id).toBe("pr-reviewer.lock.yml"); - const awContext = JSON.parse(dispatchCalls[0].inputs.aw_context); - expect(awContext.reviewer_lifecycle_event).toBe("pull_request_review"); - }); - - it("ignores pull_request_review edited and dismissed actions", async () => { - process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request_review"] }]); - for (const action of ["edited", "dismissed"]) { + it("ignores pull_request actions outside reviewer lifecycle", async () => { + process.env.GH_AW_REVIEWER_ROUTING = JSON.stringify([{ workflow: "pr-reviewer", events: ["pull_request"] }]); + for (const action of ["opened", "synchronize"]) { dispatchCalls.length = 0; - globals.context.eventName = "pull_request_review"; + globals.context.eventName = "pull_request"; globals.context.payload = { action, pull_request: { number: 9, state: "open" }, - review: { body: "Looks good\n" }, }; await main(); expect(dispatchCalls).toHaveLength(0); diff --git a/docs/src/content/docs/reference/safe-outputs-specification.md b/docs/src/content/docs/reference/safe-outputs-specification.md index c2a41b5b4be..379e40d187a 100644 --- a/docs/src/content/docs/reference/safe-outputs-specification.md +++ b/docs/src/content/docs/reference/safe-outputs-specification.md @@ -1696,8 +1696,7 @@ on: - **PRR3**: Built-in slash command behavior for `on.pull_request_reviewer` MUST always be enabled and MUST NOT be replaced by a separate `on.slash_command` trigger definition. If both fields are present, reviewer-trigger command name and reviewer-trigger event set MUST take precedence. - **PRR4**: Implementations MUST subscribe reviewer lifecycle routing to: - `pull_request` actions `ready_for_review` and `review_requested` - - `pull_request_review` action `submitted` only -- **PRR5**: Implementations MUST ignore `pull_request_review` actions other than `submitted` (including `edited` and `dismissed`) for reviewer lifecycle routing. +- **PRR5**: Implementations MUST ignore `pull_request` actions outside reviewer lifecycle routing (for example `opened`, `edited`, `synchronize`, and `closed`). - **PRR6**: When multiple `pull_request_reviewer` workflows are configured, slash command names MUST be unique case-insensitively. - **PRR7**: Duplicate reviewer slash command names MUST fail compilation with an explicit validation error. diff --git a/pkg/workflow/central_slash_command_workflow.go b/pkg/workflow/central_slash_command_workflow.go index 17955a565e5..467a71aa5ec 100644 --- a/pkg/workflow/central_slash_command_workflow.go +++ b/pkg/workflow/central_slash_command_workflow.go @@ -260,17 +260,13 @@ func collectPullRequestReviewerRoutes(workflowDataList []*WorkflowData, mergedEv } routes = append(routes, reviewerLifecycleRoute{ Workflow: wd.WorkflowID, - Events: []string{"pull_request", "pull_request_review"}, + Events: []string{"pull_request"}, }) if mergedEvents["pull_request"] == nil { mergedEvents["pull_request"] = make(map[string]bool) } mergedEvents["pull_request"]["ready_for_review"] = true mergedEvents["pull_request"]["review_requested"] = true - if mergedEvents["pull_request_review"] == nil { - mergedEvents["pull_request_review"] = make(map[string]bool) - } - mergedEvents["pull_request_review"]["submitted"] = true } sort.Slice(routes, func(i, j int) bool { return routes[i].Workflow < routes[j].Workflow diff --git a/pkg/workflow/central_slash_command_workflow_test.go b/pkg/workflow/central_slash_command_workflow_test.go index 47abd9dc975..5946a16a3e6 100644 --- a/pkg/workflow/central_slash_command_workflow_test.go +++ b/pkg/workflow/central_slash_command_workflow_test.go @@ -169,13 +169,12 @@ func TestGenerateCentralSlashCommandWorkflow_IncludesPullRequestReviewerRoutes(t require.NoError(t, err) text := string(content) require.Contains(t, text, "GH_AW_REVIEWER_ROUTING") - require.Contains(t, text, `"workflow":"pr-reviewer","events":["pull_request","pull_request_review"]`) - require.Contains(t, text, "pull_request_review:") - require.Contains(t, text, "types: [submitted]") + require.Contains(t, text, `"workflow":"pr-reviewer","events":["pull_request"]`) + require.NotContains(t, text, "pull_request_review:") require.Contains(t, text, "ready_for_review") require.Contains(t, text, "review_requested") require.Contains(t, text, "# pull-request reviewers:") - require.Contains(t, text, "# pr-reviewer [pull_request,pull_request_review]") + require.Contains(t, text, "# pr-reviewer [pull_request]") } func TestGenerateCentralSlashCommandWorkflow_InfersReviewerCommandFromWorkflowIDWhenMissing(t *testing.T) { diff --git a/pkg/workflow/compiler_safe_outputs.go b/pkg/workflow/compiler_safe_outputs.go index 8bb17191df7..d58da791a05 100644 --- a/pkg/workflow/compiler_safe_outputs.go +++ b/pkg/workflow/compiler_safe_outputs.go @@ -70,6 +70,75 @@ func resolvePullRequestReviewerCommandName(reviewerTriggerValue any, workflowDat return strings.TrimSuffix(filepath.Base(markdownPath), ".md") } +func mergeCommandOtherEvents(existing map[string]any, incoming map[string]any) map[string]any { + if len(existing) == 0 { + return incoming + } + if len(incoming) == 0 { + return existing + } + merged := maps.Clone(existing) + for eventName, incomingValue := range incoming { + if existingValue, hasExisting := merged[eventName]; hasExisting { + merged[eventName] = mergeEventConfig(existingValue, incomingValue) + continue + } + merged[eventName] = incomingValue + } + return merged +} + +func mergeEventConfig(existing any, incoming any) any { + existingMap, existingOK := existing.(map[string]any) + incomingMap, incomingOK := incoming.(map[string]any) + if !existingOK || !incomingOK { + return incoming + } + merged := maps.Clone(existingMap) + maps.Copy(merged, incomingMap) + + existingTypes, existingTypesOK := parseEventTypes(existingMap["types"]) + incomingTypes, incomingTypesOK := parseEventTypes(incomingMap["types"]) + if existingTypesOK && incomingTypesOK { + seen := make(map[string]bool, len(existingTypes)+len(incomingTypes)) + combined := make([]string, 0, len(existingTypes)+len(incomingTypes)) + for _, eventType := range existingTypes { + if !seen[eventType] { + seen[eventType] = true + combined = append(combined, eventType) + } + } + for _, eventType := range incomingTypes { + if !seen[eventType] { + seen[eventType] = true + combined = append(combined, eventType) + } + } + merged["types"] = combined + } + + return merged +} + +func parseEventTypes(value any) ([]string, bool) { + switch typed := value.(type) { + case []string: + return typed, true + case []any: + out := make([]string, 0, len(typed)) + for _, entry := range typed { + entryStr, ok := entry.(string) + if !ok { + return nil, false + } + out = append(out, entryStr) + } + return out, true + default: + return nil, false + } +} + // parseOnSection handles parsing of the "on" section from frontmatter, extracting command triggers, // reactions, and stop-after configurations while detecting conflicts with other event types. func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *WorkflowData, markdownPath string) error { @@ -213,9 +282,6 @@ func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *Work "pull_request": map[string]any{ "types": []string{"ready_for_review", "review_requested"}, }, - "pull_request_review": map[string]any{ - "types": []string{"submitted"}, - }, } if _, hasConcurrency := frontmatter["concurrency"]; !hasConcurrency && workflowData.Concurrency == "" { workflowData.Concurrency = "concurrency:\n group: \"gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }}-all-reviewers\"\n queue: max" @@ -337,7 +403,7 @@ func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *Work if hasCommand && len(otherEvents) > 0 { // We'll store this and handle it in applyDefaults workflowData.On = "" // This will trigger command handling in applyDefaults - workflowData.CommandOtherEvents = otherEvents + workflowData.CommandOtherEvents = mergeCommandOtherEvents(workflowData.CommandOtherEvents, otherEvents) } else if hasLabelCommand && len(otherEvents) > 0 { // Store other events for label-command merging in applyDefaults workflowData.On = "" // This will trigger label-command handling in applyDefaults diff --git a/pkg/workflow/compiler_safe_outputs_test.go b/pkg/workflow/compiler_safe_outputs_test.go index 324bdfbdced..c430d5021a6 100644 --- a/pkg/workflow/compiler_safe_outputs_test.go +++ b/pkg/workflow/compiler_safe_outputs_test.go @@ -188,6 +188,28 @@ func TestParseOnSection(t *testing.T) { expectedConcurrencyContains: "queue: max", checkCommandEvents: true, }, + { + name: "pull_request_reviewer preserves reviewer lifecycle when on has other events", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request_reviewer": nil, + "workflow_dispatch": map[string]any{}, + }, + }, + workflowData: &WorkflowData{}, + markdownPath: "/path/to/reviewer.md", + expectedError: false, + expectedReaction: "eyes", + expectedCentralized: true, + expectedPullRequestReviewer: true, + checkCommandEvents: true, + expectedOtherEvents: map[string]any{ + "pull_request": map[string]any{ + "types": []string{"ready_for_review", "review_requested"}, + }, + "workflow_dispatch": map[string]any{}, + }, + }, { name: "slash_command conflicts with issues", frontmatter: map[string]any{ @@ -346,8 +368,7 @@ func TestParseOnSection(t *testing.T) { if tt.checkCommandEvents { assert.NotNil(t, tt.workflowData.CommandOtherEvents, "CommandOtherEvents should be set") if tt.expectedOtherEvents != nil { - // Basic check that other events were extracted - assert.NotEmpty(t, tt.workflowData.CommandOtherEvents, "CommandOtherEvents should not be empty") + assert.Equal(t, tt.expectedOtherEvents, tt.workflowData.CommandOtherEvents, "CommandOtherEvents mismatch") } } }