Support reporting status checks and comments from PR rules by krrish175-byte · Pull Request #6218 · mindersec/minder

krrish175-byte · 2026-03-31T11:50:31Z

Summary

This pull request implements the enhancement requested in Issue #5977, allowing generic PR rules (e.g., Rego rules) to report status checks and leave PR comments natively without relying on custom Go evaluation engines like vulncheck.

Changes included:

Protobuf (minder.proto):
- Added commit_status as an available Alert type.
- Added pull_request_comment as an available Remediate type.
Alert Engine: Created a new commit_status action under internal/engine/actions/alert/commit_status which interfaces with the GitHub SetCommitStatus API to surface a success/failure check run directly on the PR.
Remediate Engine: Linked the existing pull_request_comment alert module into the Remediate registry so it can be utilized in the remediation phase without duplicating logic.

Fixes #5977

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.

Implemented full unit test coverage for the new commit_status action (commit_status_test.go) utilizing mockghclient.
Verified that pull_request_comment correctly registers inside the Remediator engine.
Ran make buf-generate to ensure all generated protobuf Go bindings are clean and up to date.
Ran go test ./... across the entire internal/engine/actions/... package, with all tests passing successfully.

Issue: mindersec#5977

coveralls · 2026-03-31T12:03:47Z

coverage: 58.45% (+0.2%) from 58.298% — krrish175-byte:fix/issue-5977 into mindersec:main

…ency upgrades

evankanderson · 2026-03-31T16:33:24Z

You'll need to sync your proto changes with main and re-run make gen -- there were some fields added to the proto in another PR.

evankanderson

Very nice work, though we may want to consider adding either target_url or description as fields from the proto message, possibly with Go templating for the latter, and RFC6570 templates for the URL.

With respect to the target_url, I'm also trying to figure out whether we should offer some sort of "Rule Status URL" endpoint that could be auto-filled based on server-known parameters. We'd also need to figure out permissions on that endpoint, which is a separate work track. For now, I'd simply allow users to template their own URL, and we can add a separate automagic_url option (bool) in the future that's exclusive with the detail_url on the commit status message.

evankanderson · 2026-03-31T16:39:11Z

            optional RestType rest = 2;
            optional GhBranchProtectionType gh_branch_protection = 3;
            optional PullRequestRemediation pull_request = 4;
+            optional Alert.AlertTypePRComment pull_request_comment = 5;


Try pulling the AlertTypePRComment message out of the Alert message, and see whether buf flags that as a breaking change. We had some early use of nested messages that went a bit overboard for these use cases.

Also, if we are going to add PR comments as a remediation type, it would be good to understand (document) in an issue or the PR description when people should use the "remediate" vs "alert" type. A few options:

We'll automatically convert "alert: pr_comment" to "remediate: pr_comment"

What happens to people who have policies remediation enabled but not alerting, or vice-versa?

We'll support all options on both types

how do we explain the differences to people?

We'll have different options on the two types -- e.g. only remediate can block PR merges

Should this be a separate type then?

I'm not trying to be a pain, but it's a lot easier to jump into a mess than to get ourselves back out. 😁

evankanderson · 2026-03-31T16:39:42Z

        Remediate remediate = 6;

        message Alert {
            string type = 1 [


We probably should have included a comment here about which types correspond to which entities as well.

evankanderson · 2026-03-31T16:40:26Z

+            message AlertTypeCommitStatus {}
+            optional AlertTypeCommitStatus commit_status = 4;


What is this / what is it used for?

evankanderson · 2026-03-31T16:43:59Z

+			zerolog.Ctx(ctx).Debug().Str("rule-type", ruletype.GetName()).
+				Msg("provider is not a GitHub provider. Silently skipping alerts.")
+			return noop.NewNoopAlert(ActionType)


Ooof, this is a cleanup opportunity. The Security Alerts feature is GitHub specific, but it should be implemented as a Provider API such that we can do the equivalent GitLab, Forgejo, etc operation. Commit Statuses should be handled similarly, though probably as a different "facet", so a provider could implement one but not the other.

Given the other set of changes here, I think filing an issue to track this in a future PR is the right step.

Let's file this issue before we submit the PR, so we don't forget about it.

evankanderson · 2026-03-31T16:56:41Z

+	// Context string for the commit status check (e.g. "minder/rule_name")
+	// If rule_name isn't available, we fallback to "minder"
+	contextStr := "minder"
+	if rule := actionParams.GetRule(); rule != nil {
+		contextStr = fmt.Sprintf("minder/%s", rule.Name)
+	}


Oh, this is interesting! I would have expected the name of the status check to be in the rule definition. You may also want to distinguish between rule type and rule name (our nomenclature here isn't always consistent, you basically have several "name-like" things:

The name field from the profile, if set. If not set, I think there's some defaulting here from either 3 or 2, and I don't recall the exact rules

The type field from the profile, which matches the name from the ruletype

The display_name from the ruletype

evankanderson · 2026-04-02T00:20:24Z

While setting up testing for another PR, I realized that we may also need to change the GitHub App recommended permissions to allow updating status checks.

You also have some lint errors that might be fixed by make lint-fix, or might need to be handled by hand.

krrish175-byte · 2026-04-02T17:03:19Z

Good catch on both fronts.

Lint errors: I will run make lint-fix and address any remaining issues manually in an upcoming push.

GitHub App permissions: That makes sense, we would need statuses: write for the commit status API calls. I will file a separate issue to track the permissions update so it doesn't block this PR.

evankanderson

Looking a little bit more deeply, I'm wondering if we should be using "Check Runs" instead of "Status Checks":

https://stackoverflow.com/questions/67919168/github-checks-api-vs-check-runs-vs-check-suites

Pros of Status Check

Simpler GitHub API
Works with other types of tokens than GitHub App

Pros of Check Runs

Automatic grouping under the GitHub App (e.g. "minder")
Ability to add line-by-line annotations
Detail page with markdown, and summary images in addition to URL

evankanderson · 2026-04-02T23:27:32Z

+            // type is the type of the remediation.
+            // * 'rest' can be used with any entity type.
+            // * 'gh_branch_protection' can only be used with the 'repository' entity type.
+            // * 'pull_request' and 'pull_request_comment' can only be used with the 'pull_request' entity type.


I think pull_request opens a new PR on the repo, so it applies to the "repository" type, not the "pull_request" type.

evankanderson · 2026-04-02T23:41:54Z


 replace github.com/mindersec/minder => ../.
+
+replace github.com/docker/docker => github.com/docker/docker v28.0.0+incompatible


This is probably not right -- I'm assuming this was in hopes of fixing some of the security warnings?

evankanderson · 2026-04-02T23:42:24Z

+			zerolog.Ctx(ctx).Debug().Str("rule-type", ruletype.GetName()).
+				Msg("provider is not a GitHub provider. Silently skipping alerts.")
+			return noop.NewNoopAlert(ActionType)


Let's file this issue before we submit the PR, so we don't forget about it.

evankanderson · 2026-04-02T23:43:55Z

+
+	// Create template params
+	tmplParams := &CommitStatusTemplateParams{
+		EvalErrorDetails: enginerr.ErrorAsEvalDetails(params.GetEvalErr()),


How would this be used / what does it show up as?

evankanderson · 2026-04-02T23:47:44Z

+	contextStr := "minder"
+	contextNameTmpl := alert.alertCfg.GetStatusName()
+	if contextNameTmpl != "" {
+		// No templating specified for status_name, use as-is
+		contextStr = contextNameTmpl
+	} else if tmplParams.RuleName != "" {
+		contextStr = fmt.Sprintf("minder/%s", tmplParams.RuleName)
+	}
+	result.Context = contextStr


With Go 1.22 and later, you can use cmp.Or for some of this:

Suggested change

contextStr := "minder"

contextNameTmpl := alert.alertCfg.GetStatusName()

if contextNameTmpl != "" {

// No templating specified for status_name, use as-is

contextStr = contextNameTmpl

} else if tmplParams.RuleName != "" {

contextStr = fmt.Sprintf("minder/%s", tmplParams.RuleName)

}

result.Context = contextStr

contextStr := "minder"

if tmplParams.RuleName != "" {

contextStr = fmt.Sprintf("%s/%s", contextStr, tmplParams.RuleName)

}

result.Context = cmp.Or(alert.alertCfg.GetStatusName(), contextStr)

evankanderson · 2026-04-02T23:50:31Z

+	// 2. Evaluate description, using text/template
+	descTmplStr := alert.alertCfg.GetDescription()
+	if descTmplStr != "" {
+		descTmpl, err := util.NewSafeHTMLTemplate(&descTmplStr, "description")


Can "description" be HTML content? (I don't know, but the GitHub API doesn't say anything about HTML: https://docs.github.com/en/rest/commits/statuses?apiVersion=2026-03-10#create-a-commit-status)

evankanderson · 2026-04-02T23:50:50Z

+		if err != nil {
+			return nil, fmt.Errorf("cannot parse description template: %w", err)
+		}
+		descStr, err := descTmpl.Render(ctx, tmplParams, 1024)


Extract the 1024 to a const parameter

evankanderson · 2026-04-02T23:51:36Z

+		argsMap := map[string]any{
+			"owner":      result.Owner,
+			"repo":       result.Repo,
+			"commit_sha": result.CommitSha,
+			"rule_name":  tmplParams.RuleName,
+		}


This feels like it should include output, so you could have a rule that pokes some external system, gets back a URL for the status check, and then loads that URL into the status check.

evankanderson · 2026-04-03T00:15:26Z

Sorry for jumping around a bit on the status checks; I threw it out there as something which had been previously discussed, but I hadn't actually researched it until after you sent your PR, so I didn't really have any sort of design in mind.

evankanderson · 2026-04-03T00:16:32Z

Also, it would be good to update https://github.com/mindersec/minder/blob/main/docs/docs/run_minder_server/config_provider.md alongside any changes to necessary GitHub app permissions, since they are in the same repo.

…r substitution

evankanderson

I'm marking this PR as "request changes" (needs merge conflicts resolved) to help track which outstanding PRs need maintainer action vs contributor action.

github-actions · 2026-05-30T05:02:36Z

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

krrish175-byte added 2 commits March 31, 2026 16:24

feat: support reporting status checks and comments from PR rules

800f901

Issue: mindersec#5977

Merge main and resolve protobuf conflict

5d3fa3a

krrish175-byte requested a review from a team as a code owner March 31, 2026 11:50

krrish175-byte added 6 commits March 31, 2026 18:09

fix: resolve gosec and error wrapping lint warnings

a3c82f8

fix: resolve CI linting and security scan issues

497bdc1

fix: sync generated protobuf bindings with project tools

da18251

fix: resolve unused-receiver lint issue

ee83775

fix: minimize PR diff and resolve docker security vulnerability

e2e4feb

fix: stabilize build and minimize PR diff by reverting harmful depend…

320cdab

…ency upgrades

evankanderson reviewed Mar 31, 2026

View reviewed changes

krrish175-byte added 2 commits April 1, 2026 22:41

feat: address maintainer feedback on commit status and remediation

b3ad69a

fix: apply trusted replace directive for docker CVE-2026-34040

bed25ae

fix: resolve lint errors in commit_status action

2ee19d5

evankanderson reviewed Apr 3, 2026

View reviewed changes

krrish175-byte added 3 commits April 3, 2026 10:47

fix: address latest maintainer feedback on templates, docs, and docke…

ff6335e

…r substitution

Merge branch 'main' into fix/issue-5977

61961db

fix: resolve compile errs resulting from error pkg rename

5995f2e

evankanderson requested changes Apr 28, 2026

View reviewed changes

github-actions Bot added the Stale label May 30, 2026

		message AlertTypeCommitStatus {}
		optional AlertTypeCommitStatus commit_status = 4;


		replace github.com/mindersec/minder => ../.

		replace github.com/docker/docker => github.com/docker/docker v28.0.0+incompatible

Conversation

krrish175-byte commented Mar 31, 2026

Summary

Testing

Uh oh!

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

Uh oh!

Uh oh!

evankanderson commented Mar 31, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

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 2, 2026

Uh oh!

krrish175-byte commented Apr 2, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Pros of Status Check

Pros of Check Runs

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 3, 2026

Uh oh!

evankanderson commented Apr 3, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Mar 31, 2026 •

edited

Loading