diff --git a/.github/workflows/agentic_commands.yml b/.github/workflows/agentic_commands.yml index 7bfa5b60725..2e677d5b7a2 100644 --- a/.github/workflows/agentic_commands.yml +++ b/.github/workflows/agentic_commands.yml @@ -7,20 +7,20 @@ # /brave -> brave [issue_comment] reaction=eyes # /cloclo -> cloclo [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /craft -> craft [issues] reaction=eyes -# /design-decision-gate -> design-decision-gate [pull_request_comment,pull_request_review_comment] reaction=eyes +# /design-decision-gate -> design-decision-gate [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /grumpy -> grumpy-reviewer [pull_request_comment,pull_request_review_comment] reaction=eyes -# /matt -> mattpocock-skills-reviewer [pull_request_comment,pull_request_review_comment] reaction=eyes +# /matt -> mattpocock-skills-reviewer [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /mergefest -> mergefest [pull_request_comment] reaction=eyes # /nit -> pr-nitpick-reviewer [pull_request_comment,pull_request_review_comment] reaction=eyes # /plan -> plan [discussion_comment,issue_comment] reaction=eyes # /poem-bot -> poem-bot [issues] reaction=eyes -# /review -> design-decision-gate [pull_request_comment,pull_request_review_comment] reaction=eyes -# /review -> pr-code-quality-reviewer [pull_request_comment,pull_request_review_comment] reaction=eyes -# /review -> test-quality-sentinel [pull_request_comment,pull_request_review_comment] reaction=eyes +# /review -> design-decision-gate [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes +# /review -> pr-code-quality-reviewer [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes +# /review -> test-quality-sentinel [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /scout -> scout [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /security-review -> security-review [pull_request_comment,pull_request_review_comment] reaction=eyes # /summarize -> pdf-summary [issue_comment,issues] reaction=eyes -# /test-quality-sentinel -> test-quality-sentinel [pull_request_comment,pull_request_review_comment] reaction=eyes +# /test-quality-sentinel -> test-quality-sentinel [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes # /tidy -> tidy [pull_request_comment] reaction=eyes # /unbloat -> unbloat-docs [pull_request_comment] reaction=eyes # labels: @@ -92,7 +92,7 @@ jobs: - name: Route slash command uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 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":"design-decision-gate","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"},{"workflow":"pr-code-quality-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"},{"workflow":"test-quality-sentinel","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_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":["discussion","discussion_comment","issue_comment","issues","pull_request","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":["discussion","discussion_comment","issue_comment","issues","pull_request","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":"design-decision-gate","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"},{"workflow":"pr-code-quality-reviewer","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes"},{"workflow":"test-quality-sentinel","events":["discussion","discussion_comment","issue_comment","issues","pull_request","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":["discussion","discussion_comment","issue_comment","issues","pull_request","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"}]}' with: script: | diff --git a/.github/workflows/design-decision-gate.lock.yml b/.github/workflows/design-decision-gate.lock.yml index 3be188aa3e7..932503bc893 100644 --- a/.github/workflows/design-decision-gate.lock.yml +++ b/.github/workflows/design-decision-gate.lock.yml @@ -97,13 +97,15 @@ jobs: activation: needs: pre_activation if: > - needs.pre_activation.outputs.activated == 'true' && ((((github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed') && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.repository_id)) && - (github.event_name != 'pull_request' || github.event.action != 'labeled' || github.event.label.name == 'implementation')) + needs.pre_activation.outputs.activated == 'true' && (((github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || + github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')) && (github.event_name != 'pull_request' || + github.event.pull_request.head.repo.id == github.repository_id)) && (github.event_name != 'pull_request' || + github.event.action != 'labeled' || github.event.label.name == 'implementation')) runs-on: ubuntu-slim permissions: actions: read contents: read + discussions: write issues: write pull-requests: write outputs: @@ -1552,8 +1554,8 @@ jobs: pre_activation: if: > - (((github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || github.event.pull_request.state != 'closed') && - (github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.repository_id)) && + ((github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && + github.event.pull_request.state != 'closed')) && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.repository_id)) && (github.event_name != 'pull_request' || github.event.action != 'labeled' || github.event.label.name == 'implementation') runs-on: ubuntu-slim permissions: diff --git a/.github/workflows/mattpocock-skills-reviewer.lock.yml b/.github/workflows/mattpocock-skills-reviewer.lock.yml index 52e425f58d0..c7cbd002eeb 100644 --- a/.github/workflows/mattpocock-skills-reviewer.lock.yml +++ b/.github/workflows/mattpocock-skills-reviewer.lock.yml @@ -89,12 +89,13 @@ jobs: activation: needs: pre_activation if: > - needs.pre_activation.outputs.activated == 'true' && ((github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed') + needs.pre_activation.outputs.activated == 'true' && (github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || + github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')) runs-on: ubuntu-slim permissions: actions: read contents: read + discussions: write issues: write pull-requests: write outputs: @@ -1478,8 +1479,8 @@ jobs: pre_activation: if: > - (github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed' + github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && + github.event.pull_request.state != 'closed') runs-on: ubuntu-slim permissions: contents: read diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml index 5a6fd1bdf8a..aa4e663a75b 100644 --- a/.github/workflows/pr-code-quality-reviewer.lock.yml +++ b/.github/workflows/pr-code-quality-reviewer.lock.yml @@ -90,12 +90,13 @@ jobs: activation: needs: pre_activation if: > - needs.pre_activation.outputs.activated == 'true' && ((github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed') + needs.pre_activation.outputs.activated == 'true' && (github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || + github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')) runs-on: ubuntu-slim permissions: actions: read contents: read + discussions: write issues: write pull-requests: write outputs: @@ -1438,8 +1439,8 @@ jobs: pre_activation: if: > - (github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed' + github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && + github.event.pull_request.state != 'closed') runs-on: ubuntu-slim permissions: contents: read diff --git a/.github/workflows/test-quality-sentinel.lock.yml b/.github/workflows/test-quality-sentinel.lock.yml index 0b5a58999df..4c5fce9c21d 100644 --- a/.github/workflows/test-quality-sentinel.lock.yml +++ b/.github/workflows/test-quality-sentinel.lock.yml @@ -88,12 +88,13 @@ jobs: activation: needs: pre_activation if: > - needs.pre_activation.outputs.activated == 'true' && ((github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed') + needs.pre_activation.outputs.activated == 'true' && (github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || + github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')) runs-on: ubuntu-slim permissions: actions: read contents: read + discussions: write issues: write pull-requests: write outputs: @@ -1450,8 +1451,8 @@ jobs: pre_activation: if: > - (github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || - github.event.pull_request.state != 'closed' + github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && + github.event.pull_request.state != 'closed') runs-on: ubuntu-slim permissions: contents: read diff --git a/pkg/workflow/central_slash_command_workflow.go b/pkg/workflow/central_slash_command_workflow.go index 76ed2eb5021..ba75ad2c965 100644 --- a/pkg/workflow/central_slash_command_workflow.go +++ b/pkg/workflow/central_slash_command_workflow.go @@ -115,6 +115,13 @@ func collectCentralSlashCommandRoutes(workflowDataList []*WorkflowData) (map[str continue } + // PullRequestReviewer workflows are driven by PR lifecycle events only. + // They must not be enrolled in comment-based slash-command routing to + // prevent accidental double-triggers when comments are posted. + if wd.PullRequestReviewer { + continue + } + filteredEvents := FilterCommentEvents(wd.CommandEvents) if len(filteredEvents) == 0 { continue diff --git a/pkg/workflow/central_slash_command_workflow_test.go b/pkg/workflow/central_slash_command_workflow_test.go index 26ce468b36a..ecca1dd567b 100644 --- a/pkg/workflow/central_slash_command_workflow_test.go +++ b/pkg/workflow/central_slash_command_workflow_test.go @@ -271,6 +271,41 @@ func TestCollectCentralSlashCommandRoutes_RespectsReactionEventTargets(t *testin require.Equal(t, []string{"|issue_comment"}, routeReactions["none-reaction"]) } +func TestCollectCentralSlashCommandRoutes_ExcludesPullRequestReviewerWorkflows(t *testing.T) { + data := []*WorkflowData{ + { + WorkflowID: "regular-slash-command", + Command: []string{"review"}, + CommandEvents: []string{"issue_comment"}, + CommandCentralized: true, + PullRequestReviewer: false, + }, + { + WorkflowID: "reviewer-workflow", + Command: []string{"review"}, + CommandEvents: nil, // nil CommandEvents would normally mean all events in FilterCommentEvents, + // but this workflow is excluded from routing by PullRequestReviewer=true + // before FilterCommentEvents is called. + CommandCentralized: true, + PullRequestReviewer: true, + }, + } + + routesByCommand, mergedEvents := collectCentralSlashCommandRoutes(data) + + // Only the regular slash-command workflow should be registered; the reviewer + // workflow must be absent even though CommandEvents is nil (which FilterCommentEvents + // would normally treat as "all events"). + require.Len(t, routesByCommand["review"], 1) + require.Equal(t, "regular-slash-command", routesByCommand["review"][0].Workflow) + for _, route := range routesByCommand["review"] { + require.NotEqual(t, "reviewer-workflow", route.Workflow, + "reviewer-workflow must not appear in centralized slash-command routes") + } + // The reviewer workflow must not contribute any events to the merged event set. + require.NotContains(t, mergedEvents, "pull_request_review") +} + func TestGenerateCentralSlashCommandWorkflow_UsesCentralizedRunsOnResolution(t *testing.T) { tmpDir := testutil.TempDir(t, "central-slash-workflow-runs-on-test") data := []*WorkflowData{ diff --git a/pkg/workflow/compiler_safe_outputs.go b/pkg/workflow/compiler_safe_outputs.go index 858601a0775..cb7b7961453 100644 --- a/pkg/workflow/compiler_safe_outputs.go +++ b/pkg/workflow/compiler_safe_outputs.go @@ -283,7 +283,11 @@ func (c *Compiler) parseOnSection(frontmatter map[string]any, workflowData *Work workflowData.PullRequestReviewer = true workflowData.CommandCentralized = true workflowData.Command = resolvePullRequestReviewerCommandNames(reviewerValue, workflowData, markdownPath) - workflowData.CommandEvents = []string{"pull_request_comment", "pull_request_review_comment"} + // Reviewer workflows are driven by PR lifecycle events only. + // CommandEvents is left nil here; the actual exclusion from + // comment-based slash-command routing is enforced in + // collectCentralSlashCommandRoutes via the PullRequestReviewer flag. + workflowData.CommandEvents = nil // Populate CommandOtherEvents with reviewer lifecycle events workflowData.CommandOtherEvents = map[string]any{ "pull_request": map[string]any{ diff --git a/pkg/workflow/compiler_safe_outputs_test.go b/pkg/workflow/compiler_safe_outputs_test.go index a82399d3777..fa8ea0d5964 100644 --- a/pkg/workflow/compiler_safe_outputs_test.go +++ b/pkg/workflow/compiler_safe_outputs_test.go @@ -495,8 +495,12 @@ func TestParseOnSection_PullRequestReviewerDefaultsToReviewAndWorkflowID(t *test err := c.parseOnSection(frontmatter, workflowData, "/path/to/reviewer.md") require.NoError(t, err) assert.Equal(t, []string{"review", "reviewer-workflow-id"}, workflowData.Command) - assert.Equal(t, []string{"pull_request_comment", "pull_request_review_comment"}, workflowData.CommandEvents) + // CommandEvents is nil for reviewer workflows; the actual exclusion from + // comment-based routing is enforced via the PullRequestReviewer flag in + // collectCentralSlashCommandRoutes. + assert.Nil(t, workflowData.CommandEvents) assert.True(t, workflowData.CommandCentralized) + assert.True(t, workflowData.PullRequestReviewer) } func TestParseOnSection_PullRequestReviewerOwnsCommandAndEvents(t *testing.T) { @@ -520,8 +524,12 @@ func TestParseOnSection_PullRequestReviewerOwnsCommandAndEvents(t *testing.T) { err := c.parseOnSection(frontmatter, workflowData, "/path/to/reviewer.md") require.NoError(t, err) assert.Equal(t, []string{"review", "reviewer-workflow-id"}, workflowData.Command) - assert.Equal(t, []string{"pull_request_comment", "pull_request_review_comment"}, workflowData.CommandEvents) + // CommandEvents is nil for reviewer workflows; the actual exclusion from + // comment-based routing is enforced via the PullRequestReviewer flag in + // collectCentralSlashCommandRoutes. + assert.Nil(t, workflowData.CommandEvents) assert.True(t, workflowData.CommandCentralized) + assert.True(t, workflowData.PullRequestReviewer) } func TestParseOnSection_PullRequestReviewerDoesNotInjectLifecycleTriggers(t *testing.T) { diff --git a/pkg/workflow/pull_request_reviewer_if_guard_test.go b/pkg/workflow/pull_request_reviewer_if_guard_test.go new file mode 100644 index 00000000000..73bced3f1bf --- /dev/null +++ b/pkg/workflow/pull_request_reviewer_if_guard_test.go @@ -0,0 +1,107 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/testutil" +) + +// TestPullRequestReviewerIfGuard verifies that the compiled if: condition for +// pull_request_reviewer workflows uses an allowlist (not a denylist) so that +// only pull_request, pull_request_review, and workflow_dispatch events can +// trigger the reviewer job. Comment events are excluded from triggering via +// the central router by the PullRequestReviewer check in +// collectCentralSlashCommandRoutes, not by this if: guard. +func TestPullRequestReviewerIfGuard(t *testing.T) { + tmpDir := testutil.TempDir(t, "reviewer-if-guard-test") + + tests := []struct { + name string + content string + expectedIfContains string + notExpectedIfParts []string + }{ + { + name: "reviewer workflow produces allowlist if guard", + content: `--- +on: + pull_request_reviewer: +permissions: + contents: read +engine: copilot +--- + +# Reviewer Workflow +`, + expectedIfContains: "github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')", + notExpectedIfParts: []string{ + // The old denylist guard must be absent – it allowed comment events through. + "github.event_name != 'pull_request' && github.event_name != 'pull_request_review'", + }, + }, + { + name: "reviewer workflow if guard does not allow comment events", + content: `--- +on: + pull_request_reviewer: custom-reviewer +permissions: + contents: read +engine: copilot +--- + +# Custom Reviewer Workflow +`, + // The allowlist guard must be present and include workflow_dispatch. + // Verifies the guard does not use the old denylist form that admitted comments. + expectedIfContains: "github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')", + notExpectedIfParts: []string{ + // The old denylist form must not appear in the compiled reviewer guard. + "github.event_name != 'pull_request' && github.event_name != 'pull_request_review'", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testFile := filepath.Join(tmpDir, tt.name+".md") + if err := os.WriteFile(testFile, []byte(tt.content), 0600); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + compiler.SetStrictMode(false) + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("CompileWorkflow failed: %v", err) + } + + lockFile := stringutil.MarkdownToLockFile(testFile) + raw, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockContent := string(raw) + // Normalize whitespace for substring matching across line-wrapped YAML. + normalised := strings.Join(strings.Fields(lockContent), " ") + + if tt.expectedIfContains != "" { + normExpected := strings.Join(strings.Fields(tt.expectedIfContains), " ") + if !strings.Contains(normalised, normExpected) { + t.Errorf("Expected lock file to contain if guard:\n %s\nbut it was not found.\nLock content:\n%s", tt.expectedIfContains, lockContent) + } + } + + for _, bad := range tt.notExpectedIfParts { + normBad := strings.Join(strings.Fields(bad), " ") + if strings.Contains(normalised, normBad) { + t.Errorf("Lock file must NOT contain:\n %s\nbut it was found.\nLock content:\n%s", bad, lockContent) + } + } + }) + } +} diff --git a/pkg/workflow/tools.go b/pkg/workflow/tools.go index 490a40cd2d1..4f8f124e4db 100644 --- a/pkg/workflow/tools.go +++ b/pkg/workflow/tools.go @@ -307,7 +307,16 @@ func (c *Compiler) applyDefaults(data *WorkflowData, markdownPath string) error } if data.PullRequestReviewer { - reviewerGuard := "(github.event_name != 'pull_request' && github.event_name != 'pull_request_review') || github.event.pull_request.state != 'closed'" + // Use an allowlist so that only the events this reviewer workflow is designed + // to handle can trigger the job. The previous denylist form + // (event != 'pull_request' && event != 'pull_request_review') || PR.state != 'closed' + // was logically inverted: the first clause evaluated to TRUE for every event + // that is neither pull_request nor pull_request_review (e.g. + // pull_request_review_comment), causing the job to run on comment events. + // The allowlist below permits only: + // • workflow_dispatch – used by the central router for lifecycle dispatch + // • pull_request / pull_request_review – only when the PR is still open + reviewerGuard := "github.event_name == 'workflow_dispatch' || ((github.event_name == 'pull_request' || github.event_name == 'pull_request_review') && github.event.pull_request.state != 'closed')" if data.If == "" { data.If = reviewerGuard } else {