Skip to content

feat(alert): allow line comments for PR alert type (#5118) #6531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MahekPatel-2403 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(alert): allow line comments for PR alert type (#5118) #6531

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
634f6f3
feat: allow line comments for PR alert type (#5118)
MahekPatel-2403 Jun 21, 2026
fdf9f24
refactor(alert): extract PR comment action handlers
MahekPatel-2403 Jun 21, 2026
d9e7fc2
Update internal/engine/actions/alert/pull_request_comment/pull_reques…
MahekPatel-2403 Jun 22, 2026
302dc52
test(alert): add template rendering failure case
MahekPatel-2403 Jun 22, 2026
de3baec
fix(alert): address review feedback
MahekPatel-2403 Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/docs/ref/proto.mdx
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

209 changes: 137 additions & 72 deletions internal/engine/actions/alert/pull_request_comment/pull_request_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package pull_request_comment

import (
"cmp"
"context"
"encoding/json"
"errors"
Expand Down Expand Up @@ -57,15 +56,16 @@ type PrCommentTemplateParams struct {
}

type paramsPR struct {
Owner string
Repo string
CommitSha string
Number int
Comment string
RuleName string
Event string
Metadata *alertMetadata
prevStatus *db.ListRuleEvaluationsByProfileIdRow
Owner string
Repo string
CommitSha string
Number int
Comment string
LineComments []*github.DraftReviewComment
RuleName string
Event string
Metadata *alertMetadata
prevStatus *db.ListRuleEvaluationsByProfileIdRow
}

type alertMetadata struct {
Expand Down Expand Up @@ -139,83 +139,117 @@ func (alert *Alert) Do(
}

func (alert *Alert) run(ctx context.Context, params *paramsPR, cmd interfaces.ActionCmd) (json.RawMessage, error) {
// Process the command
switch cmd {
// Create or update a review
case interfaces.ActionCmdOn:
return alert.handleActionOn(ctx, params)
// Dismiss the review
case interfaces.ActionCmdOff:
return alert.handleActionOff(ctx, params)
case interfaces.ActionCmdDoNothing:
// Return the previous alert status.
return alert.runDoNothing(ctx, params)
}
return nil, enginerr.ErrActionSkipped
Comment thread
MahekPatel-2403 marked this conversation as resolved.
}
Comment on lines 142 to +156

@evankanderson evankanderson Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's helpful to do this kind of refactoring in a separate PR as it makes it easier to check that existing behavior has been preserved. You don't need to do that this time, but consider it a note for next time.

@MahekPatel-2403 MahekPatel-2403 Jun 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good point. I'll keep feature changes and refactors separate in future PRs where possible.


func (alert *Alert) handleActionOn(ctx context.Context, params *paramsPR) (json.RawMessage, error) {
logger := zerolog.Ctx(ctx).With().
Str("owner", params.Owner).
Str("repo", params.Repo).
Int("pr", params.Number).
Logger()

// Process the command
switch cmd {
// Create or update a review
case interfaces.ActionCmdOn:
existingReview, err := alert.findExistingReview(ctx, params)
if err != nil {
return nil, fmt.Errorf("error searching for existing PR review: %w", err)
}
existingReview, err := alert.findExistingReview(ctx, params)
if err != nil {
return nil, fmt.Errorf("error searching for existing PR review: %w", err)
}

var reviewID int64
if existingReview != nil {
reviewID = existingReview.GetID()
if _, err := alert.gh.UpdateReview(ctx, params.Owner, params.Repo, params.Number, reviewID, params.Comment); err != nil {
return nil, fmt.Errorf("error updating PR review: %w, %w", err, enginerr.ErrActionFailed)
}
logger.Info().Int64("review_id", reviewID).Msg("PR review updated")
var reviewID int64
if existingReview != nil {
reviewID = existingReview.GetID()
if _, err := alert.gh.UpdateReview(ctx, params.Owner, params.Repo, params.Number, reviewID, params.Comment); err != nil {
return nil, fmt.Errorf("error updating PR review: %w, %w", err, enginerr.ErrActionFailed)
}
if len(params.LineComments) > 0 {
// The GitHub UpdateReview API only accepts a new body; it cannot add new
// line-level comments to an already-submitted review. Line comments are
// only posted when the review is first created.
logger.Warn().Int64("review_id", reviewID).
Int("line_comments", len(params.LineComments)).
Msg("PR review updated but line comments were not re-posted (GitHub API limitation)")

@evankanderson evankanderson Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the action proposes line comments that are different from the previous, should we leave a new comment with the new line comments?

(I'm wondering what would be useful to users, not what's simplest to implement.)

@MahekPatel-2403 MahekPatel-2403 Jun 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial assumption was that line comments would only be posted when the review is first created, and subsequent updates would continue updating the review body rather than creating additional line comments.

That said, I haven't thought through the user experience deeply yet. If findings change between runs, creating new line comments might make the updated findings more visible, but it could also lead to noise and duplicate discussions.

@evankanderson evankanderson Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #5118 doesn't really provide a lot of motivation, so you may want to lay out some use cases.

Here are a couple:

  1. Minder currently has a check for license boilerplate at the top of many file types. This burns a lot of actions time due to the implementation (it needs to build a go binary and then run it once for each file type), and adds a bunch of dependencies to the CI process.
  2. Minder has static (built-in) checks for various unicode attacks from the Trojan Source paper. It would be great if Minder were flexible enough that these could instead be encoded as standard alert rules.

I suspect that we'll need to do some design work to figure out how to pass the data from the rule evaluation to the remediation to get these to work -- our typical mechanism for doing this is the output parameter from the rule evaluation, but you may need to define some mechanism to take structured output and convert it into a set of line comments, rather than defining the line comments statically in the rule definition.

@MahekPatel-2403 MahekPatel-2403 Jun 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples help clarify the problem space a lot. I can see how checks like the license boilerplate and Trojan Source detection would need a more dynamic way of generating review comments from evaluation results. I'll spend some time working through these use cases and see how they fit with the current design.

} else {
req := &github.PullRequestReviewRequest{
Body: github.String(params.Comment),
Event: github.String(params.Event),
}
review, err := alert.gh.CreateReview(ctx, params.Owner, params.Repo, params.Number, req)
if err != nil {
return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed)
}
reviewID = review.GetID()
existingReview = review
logger.Info().Int64("review_id", reviewID).Msg("PR review created")
logger.Info().Int64("review_id", reviewID).Msg("PR review updated")
}

now := time.Now()
newMeta, err := json.Marshal(alertMetadata{
ReviewID: strconv.FormatInt(reviewID, 10),
SubmittedAt: &now,
PullRequestUrl: existingReview.HTMLURL,
})
if err != nil {
return nil, fmt.Errorf("error marshalling alert metadata json: %w", err)
} else {
req := &github.PullRequestReviewRequest{
Body: github.String(params.Comment),
Event: github.String(params.Event),
}

return newMeta, nil
// Dismiss the review
case interfaces.ActionCmdOff:
existingReview, err := alert.findExistingReview(ctx, params)
// CommitID is required by GitHub for reviews that include line comments;
// it tells GitHub which commit's diff to annotate.
if params.CommitSha != "" {
req.CommitID = github.String(params.CommitSha)
}
// Attach line-specific draft comments if any were configured
if len(params.LineComments) > 0 {
req.Comments = params.LineComments
}
review, err := alert.gh.CreateReview(ctx, params.Owner, params.Repo, params.Number, req)
if err != nil {
return nil, fmt.Errorf("error searching for existing PR review: %w", err)
return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed)
}
reviewID = review.GetID()
existingReview = review
logger.Info().Int64("review_id", reviewID).
Int("line_comments", len(params.LineComments)).
Msg("PR review created")
}

if existingReview == nil {
logger.Debug().Msg("No PR review to dismiss")
return nil, enginerr.ErrActionTurnedOff
}
now := time.Now()
newMeta, err := json.Marshal(alertMetadata{
ReviewID: strconv.FormatInt(reviewID, 10),
SubmittedAt: &now,
PullRequestUrl: existingReview.HTMLURL,
})
if err != nil {
return nil, fmt.Errorf("error marshalling alert metadata json: %w", err)
}

reviewID := existingReview.GetID()
if _, err := alert.gh.DismissReview(
ctx, params.Owner, params.Repo, params.Number, reviewID, &github.PullRequestReviewDismissalRequest{
Message: github.String("Dismissed due to alert being turned off"),
},
); err != nil {
if errors.Is(err, enginerr.ErrNotFound) {
return nil, fmt.Errorf("PR review already dismissed: %w, %w", err, enginerr.ErrActionTurnedOff)
}
return nil, fmt.Errorf("error dismissing PR review: %w, %w", err, enginerr.ErrActionFailed)
}
logger.Info().Int64("review_id", reviewID).Msg("PR review dismissed")
return newMeta, nil
}

func (alert *Alert) handleActionOff(ctx context.Context, params *paramsPR) (json.RawMessage, error) {
logger := zerolog.Ctx(ctx).With().
Str("owner", params.Owner).
Str("repo", params.Repo).
Int("pr", params.Number).
Logger()

existingReview, err := alert.findExistingReview(ctx, params)
if err != nil {
return nil, fmt.Errorf("error searching for existing PR review: %w", err)
}

if existingReview == nil {
logger.Debug().Msg("No PR review to dismiss")
return nil, enginerr.ErrActionTurnedOff
case interfaces.ActionCmdDoNothing:
// Return the previous alert status.
return alert.runDoNothing(ctx, params)
}
return nil, enginerr.ErrActionSkipped

reviewID := existingReview.GetID()
if _, err := alert.gh.DismissReview(
ctx, params.Owner, params.Repo, params.Number, reviewID, &github.PullRequestReviewDismissalRequest{
Message: github.String("Dismissed due to alert being turned off"),
},
); err != nil {
if errors.Is(err, enginerr.ErrNotFound) {
return nil, fmt.Errorf("PR review already dismissed: %w, %w", err, enginerr.ErrActionTurnedOff)
}
return nil, fmt.Errorf("error dismissing PR review: %w, %w", err, enginerr.ErrActionFailed)
}
logger.Info().Int64("review_id", reviewID).Msg("PR review dismissed")
return nil, enginerr.ErrActionTurnedOff
}

func (alert *Alert) findExistingReview(ctx context.Context, params *paramsPR) (*github.PullRequestReview, error) {
Expand Down Expand Up @@ -244,6 +278,9 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsPR, cmd interfaces
body := github.String(params.Comment)
logger.Info().Msgf("dry run: create a PR comment on PR %d in repo %s/%s with the following body: %s",
params.Number, params.Owner, params.Repo, *body)
if len(params.LineComments) > 0 {
logger.Info().Msgf("dry run: would also post %d line comment(s)", len(params.LineComments))
}
return nil, nil
case interfaces.ActionCmdOff:
if params.Metadata == nil || params.Metadata.ReviewID == "" {
Expand Down Expand Up @@ -318,7 +355,10 @@ func (alert *Alert) getParamsForPRComment(

result.RuleName = params.GetRule().Name

action := cmp.Or(alert.reviewCfg.GetAction(), "comment")
action := alert.reviewCfg.GetAction()
if action == "" {
action = "comment"

@evankanderson evankanderson Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? Indentation seems off.

@MahekPatel-2403 MahekPatel-2403 Jun 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran gofmt locally and it didn't make any changes. Is there a specific indentation issue you're seeing?

}
Comment thread
MahekPatel-2403 marked this conversation as resolved.
Outdated
if strings.ToLower(action) == "request_changes" {
result.Event = "REQUEST_CHANGES"
} else {
Expand All @@ -328,6 +368,31 @@ func (alert *Alert) getParamsForPRComment(
// Add magic comment to identify Minder reviews for this rule
result.Comment = fmt.Sprintf("%s\n\n<!-- minder-rule: %s -->", comment, result.RuleName)

// Render and collect line-specific draft comments
for i, lc := range alert.reviewCfg.GetLineComments() {
commentBody := lc.GetComment()
lineTmpl, err := util.NewSafeHTMLTemplate(&commentBody, fmt.Sprintf("line_comment_%d", i))
if err != nil {
return nil, fmt.Errorf("cannot parse line comment %d template: %w", i, err)
}
rendered, err := lineTmpl.Render(ctx, tmplParams, PrCommentMaxLength)
if err != nil {
return nil, fmt.Errorf("cannot render line comment %d: %w", i, err)
}

draftComment := &github.DraftReviewComment{
Path: github.String(lc.GetFilepath()),
Body: github.String(rendered),
Line: github.Int(int(lc.GetLineEnd())),
StartLine: github.Int(int(lc.GetLineStart())),
}
// If the comment spans only one line, omit StartLine (GitHub requires start_line < line)

@evankanderson evankanderson Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation doesn't allow this to happen, does it?

@MahekPatel-2403 MahekPatel-2403 Jun 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. With the current validation, single-line comments would still have both line_start and line_end set, so this path should only be reached when line_start == line_end. I'll double-check whether this special handling is still needed.

if lc.GetLineStart() == lc.GetLineEnd() {
draftComment.StartLine = nil
}
result.LineComments = append(result.LineComments, draftComment)
}

// Unmarshal the existing alert metadata, if any
if metadata != nil {
meta := &alertMetadata{}
Expand Down
Loading