Remove pull_request_review from on.pull_request_reviewer hybrid routing (#33461)

Copilot · pelikhan · web-flow · commit 28cf7963ef26 · 2026-05-19T22:49:53.000-07:00
* Plan pull_request_reviewer trigger update

Co-authored-by: pelikhan &lt;4175913+pelikhan@users.noreply.github.com&gt;

* Remove pull_request_review from pull_request_reviewer routing

Co-authored-by: pelikhan &lt;4175913+pelikhan@users.noreply.github.com&gt;

* Fix reviewer event merging and update reviewer lock triggers

Co-authored-by: pelikhan &lt;4175913+pelikhan@users.noreply.github.com&gt;

---------

Co-authored-by: copilot-swe-agent[bot] &lt;198982749+Copilot@users.noreply.github.com&gt;
Co-authored-by: pelikhan &lt;4175913+pelikhan@users.noreply.github.com&gt;
Co-authored-by: Peli de Halleux &lt;pelikhan@users.noreply.github.com&gt;
diff --git 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
diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml
diff --git a/.github/workflows/test-quality-sentinel.lock.yml b/.github/workflows/test-quality-sentinel.lock.yml
diff --git a/actions/setup/js/route_slash_command.cjs b/actions/setup/js/route_slash_command.cjs
@@ -2,7 +2,6 @@
 /// <reference types="@actions/github-script" />
 
 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
@@ -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<!-- gh-aw-workflow-id: pr-reviewer -->" },
-    };
-    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<!-- gh-aw-workflow-id: pr-reviewer -->" },
       };
       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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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")
 					}
 				}
 			}

Original file line number	Diff line number	Diff line change
`@@ -260,17 +260,13 @@ func collectPullRequestReviewerRoutes(workflowDataList []*WorkflowData, mergedEv`
`260`	`260`	`}`
`261`	`261`	`routes = append(routes, reviewerLifecycleRoute{`
`262`	`262`	`Workflow: wd.WorkflowID,`
`263`		`- Events: []string{"pull_request", "pull_request_review"},`
	`263`	`+ Events: []string{"pull_request"},`
`264`	`264`	`})`
`265`	`265`	`if mergedEvents["pull_request"] == nil {`
`266`	`266`	`mergedEvents["pull_request"] = make(map[string]bool)`
`267`	`267`	`}`
`268`	`268`	`mergedEvents["pull_request"]["ready_for_review"] = true`
`269`	`269`	`mergedEvents["pull_request"]["review_requested"] = true`
`270`		`- if mergedEvents["pull_request_review"] == nil {`
`271`		`- mergedEvents["pull_request_review"] = make(map[string]bool)`
`272`		`- }`
`273`		`- mergedEvents["pull_request_review"]["submitted"] = true`
`274`	`270`	`}`
`275`	`271`	`sort.Slice(routes, func(i, j int) bool {`
`276`	`272`	`return routes[i].Workflow < routes[j].Workflow`