Decouple PR feedback actions from GitHub-specific provider by krrish175-byte · Pull Request #6262 · mindersec/minder

krrish175-byte · 2026-04-03T07:08:58Z

Summary

This PR introduces provider-agnostic CommitStatusPublisher and ReviewPublisher interfaces in pkg/providers/v1, decoupling the commit_status alert action and pull_request_comment alert/remediate action from the hardcoded provinfv1.GitHub interface.

Previously, both actions had a silent fallback to NoopAlert/NoopRemediate with the misleading log message "provider is not a GitHub provider" when used with any non-GitHub provider. This change replaces the GitHub-specific type assertions with generic interface casts, improving extensibility for future VCS providers (e.g. GitLab).

Key changes:

Added CommitStatus and Review provider-agnostic DTOs to pkg/providers/v1
Added CommitStatusPublisher and ReviewPublisher interfaces
Implemented PublishCommitStatus, PublishReview, and DismissPublishedReview adapter methods on the GitHub client
Refactored commit_status and pull_request_comment actions to cast to the new generic interfaces
Updated alert.go and remediate.go to use the new interfaces with accurate error messages
Regenerated mocks and updated unit tests to use MockCommitStatusPublisher/MockReviewPublisher

Testing

Ran go build ./... — builds cleanly with no errors
Ran go test ./internal/engine/actions/... — all 8 packages pass:
- alert/commit_status
- alert/pull_request_comment
- alert/security_advisory
- remediate and all sub-packages
Verified compile-time assertions (var _ provifv1.CommitStatusPublisher = (*GitHub)(nil)) fail at compile time if the methods are missing — they pass, confirming correct implementation
Existing engines (vulncheck, homoglyphs, trusty) that use GitHub.CreateReview / GitHub.SetCommitStatus directly are fully unaffected

coveralls · 2026-04-03T07:20:17Z

coverage: 58.493% (+0.2%) from 58.298% — krrish175-byte:feat/generic-pr-feedback into mindersec:main

Add generic CommitStatusPublisher and ReviewPublisher interfaces to pkg/providers/v1 so that commit_status alert and pull_request_comment alert/remediate actions are no longer hardcoded to provinfv1.GitHub. Previously both actions fell back silently to NoopAlert/NoopRemediate with a misleading log message when used with any non-GitHub provider. This change replaces the GitHub-specific type assertions with generic interface casts, improving extensibility for future VCS providers. Changes: - Define CommitStatus and Review DTOs in pkg/providers/v1 - Define CommitStatusPublisher and ReviewPublisher interfaces - Implement PublishCommitStatus, PublishReview, DismissPublishedReview on the GitHub client as adapter methods - Add compile-time interface assertions on the GitHub client struct - Refactor commit_status action to cast to CommitStatusPublisher - Refactor pull_request_comment action to cast to ReviewPublisher - Update alert.go and remediate.go to use the new generic interfaces - Regenerate mocks and update unit tests to MockCommitStatusPublisher and MockReviewPublisher

evankanderson

The commit_status isn't used yet, correct? I thought in #6261 we were trying to decide between "commit status" and "check runs" APIs.

evankanderson

Thanks for starting this! Can we have this PR just focus on pull_request_comment? I've made a few suggestions that may be scope creep (templating in body and event); feel free to separate them out into their own PR.

evankanderson · 2026-04-03T21:42:45Z

            string type = 1 [
                (buf.validate.field).string = {
-                    in: ["rest", "gh_branch_protection", "pull_request"],
+                    in: ["rest", "gh_branch_protection", "pull_request", "pull_request_comment"],


This is part of #6261 , right?

evankanderson · 2026-04-03T21:55:00Z

-			review,
-		)
-		if err != nil {
+		if err := alert.gh.PublishReview(ctx, params.Owner, params.Repo, params.Number,


It would be great to have this action work like the Trusty / OSV reviews:

If the rule hasn't commented on the PR before, publish a comment.

If the rule has commented on the PR before, update the existing comment.

If the rule is passing, delete the previous comment on the PR for that rule

(Yes, this is a bit more complex than the current logic -- feel free to defer that to another issue, but I think the ReviewPublisher interface should support that.)

We should also allow the remediation to use at least the REQUEST_CHANGES or COMMENT events (defaulting to COMMENT). There should be some way to wire in output from the rule to both body and event. In the future, we should figure out how to populate the comments array from outputs as well, but I think we can hold that for another PR.

Narrow the scope of PR feedback to pull request comments by removing commit statuses. Refactor provider interfaces to use standard GitHub methods and implement create-or-update/dismiss logic for Minder reviews. Co-Authored-By: Antigravity <noreply@google.com>

krrish175-byte · 2026-04-04T05:39:23Z

Thanks for the detailed feedback. I have updated the PR to address your suggestions:

Narrowed Scope: I have removed the commit_status alert type from the engine and protobuf definitions. The focus is now exclusively on enhancing pull_request_comment as the primary standard feedback mechanism.

Refactored Provider Interfaces: Hoisted standard GitHub API methods (ListReviews, CreateReview, UpdateReview, DismissReview, and GetPullRequest) directly into the ReviewPublisher interface in pkg/providers/v1/providers.go.

Refactored CommitStatusPublisher to reuse the existing SetCommitStatus method from the GitHub trait.

Removed redundant DTO-based wrapper methods (PublishReview, DismissPublishedReview) to align with your suggestion of reusing the standard GitHub API patterns.

Advanced PR Review Lifecycle:

Implemented a "create-or-update" mechanism in pull_request_comment.go. Minder now uses a magic comment () to uniquely identify reviews it has created for a specific rule.

If a review for a rule already exists, Minder will now update it rather than posting a new comment.
When a rule evaluation passes (alert turned off), Minder now automatically dismisses the previously created review for that rule.
Configurable Review Events: Updated the protobuf definition to include a review_event field in

AlertTypePRComment, supporting both COMMENT (default) and REQUEST_CHANGES.

Verification:

Updated unit tests in pull_request_comment_test.go to cover the new lifecycle and event configurations.
Verified that all tests for the pull_request_comment package are passing locally.

Looking forward to your thoughts!

…deps

Ran 'make bootstrap' to install buf, sqlc, mockgen, then 'make gen' to regenerate all files from source. This ensures generated artifacts match exactly what CI produces, resolving uncommitted changes checks.

evankanderson

Will review more tomorrow, this is about +1500 LOC, and that's not all auto-generated.

evankanderson · 2026-04-06T05:45:35Z

This is an accidental include, right? I generally try to put this type of output to /tmp or ~/tmp to avoid accidentally including it with e.g. git add .

Ahh, my bad. Yep, I accidentally added this txt file. Thanks for the solution

evankanderson

Sorry for the delay. This looks promising; there's one test error due to an empty _test.go file, and then a few API comments, but this looks like a nice addition!

evankanderson · 2026-04-08T16:05:52Z

 	github.com/alexdrl/zerowater v0.0.3
 	github.com/aws/aws-sdk-go-v2 v1.41.2
-	github.com/aws/aws-sdk-go-v2/config v1.32.7
+	github.com/aws/aws-sdk-go-v2/config v1.32.5


It looks like you tried to do a rebase here and ended up losing changes to go mod. I'd suggest simply reverting these files via git checkout upstream/main go.* and then running go mod tidy.

evankanderson · 2026-04-08T16:12:09Z

                    },
                    (google.api.field_behavior) = REQUIRED
                ];
+                // review_event is the event to use for the PR review (COMMENT or REQUEST_CHANGES).


I feel like "review event" is a bit of a weird name. What about block_commit or something along those lines?

Also, our other string-as-enum values are lowercase. Rather than directly mirroring the GitHub API (which might be different for e.g. GitLab in the future), let's name things based on the intent.

pull_request_comment: review_message: "..." block_commit: "advise-only" # or "require-changes"

I'm not attached to the exact wording, but it should be clear looking at the YAML what the result of the alert / remediation will be.

evankanderson · 2026-04-08T16:15:43Z

+	case pull_request_comment.AlertType:
+		client, err := provinfv1.As[provinfv1.ReviewPublisher](provider)
+		if err != nil {
+			return nil, errors.New("provider does not support publishing pull request reviews")


Given the regularity of the other error messages, let's call this the "pull_request_comment trait":

Suggested change

return nil, errors.New("provider does not support publishing pull request reviews")

return nil, errors.New("provider does not implement pull_request_comment trait")

(Alternatively, we could call it the pr-review trait; I'd simply say "review", but that feels a bit too broad as a name to be clear.)

evankanderson · 2026-04-08T16:18:40Z

This empty file is producing an error (which screws up the test reporting):

# github.com/mindersec/minder/internal/engine/actions/alert/commit_status internal/engine/actions/alert/commit_status/commit_status_test.go:1:1: expected 'package', found 'EOF' FAIL github.com/mindersec/minder/internal/engine/actions/alert/commit_status [setup failed]

evankanderson · 2026-04-08T16:36:27Z

+		} else {
+			req := &github.PullRequestReviewRequest{
+				Body:  github.String(params.Comment),
+				Event: github.String(params.Event),


IMO, we should translate from our API to the GitHub constants here, rather than leaking GitHub's constants into the API.

(It's interesting that you can't change the event from REQUEST_CHANGES to COMMENT after posting.)

evankanderson · 2026-04-08T16:38:06Z

 		newMeta, err := json.Marshal(alertMetadata{
-			ReviewID:       strconv.FormatInt(r.GetID(), 10),
-			SubmittedAt:    r.SubmittedAt.GetTime(),
-			PullRequestUrl: r.PullRequestURL,


Why did we remove PullRequestUrl?

evankanderson · 2026-04-08T16:42:25Z

 - Repository Permissions:

  - Administration (read and write)
+  - Commit statuses (read and write)


We don't need this until we add commit status or check runs, right?

evankanderson · 2026-04-20T17:58:10Z

(This looks like it needs to be merged up to date and a few comments addressed.)

krrish175-byte requested a review from a team as a code owner April 3, 2026 07:08

krrish175-byte force-pushed the feat/generic-pr-feedback branch from b8f287f to 1f1c470 Compare April 3, 2026 07:10

krrish175-byte force-pushed the feat/generic-pr-feedback branch from 1f1c470 to 134f096 Compare April 3, 2026 18:06

security: upgrade vulnerable dependencies

35d3b8e

evankanderson reviewed Apr 3, 2026

View reviewed changes

krrish175-byte and others added 2 commits April 4, 2026 10:49

Merge branch 'main' into feat/generic-pr-feedback

a282dab

krrish175-byte added 5 commits April 4, 2026 11:22

fix: stabilize CI, resolve proto breaking changes and regenerate mocks

92c13fa

fix: restore binary compatibility in proto, sync generation and tidy …

a8c625a

…deps

fix: synchronize all provider mocks after interface refactoring

1ed61a0

fix: regenerate all proto, mock, and swagger files with proper toolchain

1cb7d48

Ran 'make bootstrap' to install buf, sqlc, mockgen, then 'make gen' to regenerate all files from source. This ensures generated artifacts match exactly what CI produces, resolving uncommitted changes checks.

fix: resolve final linting issues and synchronize mocks

414e86b

evankanderson reviewed Apr 6, 2026

View reviewed changes

krrish175-byte added 2 commits April 7, 2026 13:00

fix: resolve CI failures for linting, tests, and proto checks

e9af11f

fix: resolve residual CI errors (gci, trivy)

d213631

evankanderson reviewed Apr 8, 2026

View reviewed changes

evankanderson mentioned this pull request Apr 10, 2026

feat: add request_changes action to PR comment alerts #6304

Open

	return nil, errors.New("provider does not support publishing pull request reviews")
	return nil, errors.New("provider does not implement pull_request_comment trait")

Conversation

krrish175-byte commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

coveralls commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krrish175-byte commented Apr 4, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krrish175-byte commented Apr 3, 2026 •

edited

Loading

coveralls commented Apr 3, 2026 •

edited

Loading