feat(alert): allow line comments for PR alert type (#5118) by MahekPatel-2403 · Pull Request #6531 · mindersec/minder

MahekPatel-2403 · 2026-06-21T19:02:33Z

Summary

Adds support for line comments in PR alert reviews.

Changes include:

Added line comment support to the PR alert workflow.
Updated protobuf definitions and generated files.
Added validation and rendering logic for single-line and multi-line comments.
Refactored PR comment action handling into smaller helper functions to reduce cyclomatic complexity.
Added comprehensive tests covering line comment scenarios.

Testing

Added unit tests for PR comment handling.
Verified single-line and multi-line comment rendering.
Verified review creation and update flows.

Adds support for line-specific PR review comments in the pull_request_comment alert type, implementing the feature requested in issue mindersec#5118. Changes: - proto/minder/v1/minder.proto: add PullRequestLineComment message with filepath, line_start, line_end, comment fields; add repeated line_comments field to AlertTypePRComment. - pkg/api/protobuf/go/minder/v1/minder.pb.go: regenerated via 'buf generate' to include the new PullRequestLineComment type. - pkg/api/openapi/minder/v1/minder.swagger.json: regenerated. - docs/docs/ref/proto.mdx: regenerated. - pkg/api/protobuf/go/minder/v1/validators.go: validate each line_comment entry — filepath non-empty, line_start/line_end > 0, line_start <= line_end, comment non-empty and a valid Go template. - internal/.../pull_request_comment.go: render line comment templates and pass them as DraftReviewComment entries on CreateReview; single-line comments omit StartLine (GitHub requires start_line < line for multi-line ranges); CommitID only set when non-empty; warn when UpdateReview is called with line comments (GitHub API limitation). - internal/.../pull_request_comment_test.go: add TestPullRequestLineCommentAlert covering single-line, multi-line, multiple comments across files, and template rendering in line comment bodies. Fixes mindersec#5118

CLAassistant · 2026-06-21T19:02:40Z

All committers have signed the CLA.

coveralls · 2026-06-22T07:35:09Z

coverage: 60.761% (+0.04%) from 60.717% — MahekPatel-2403:feat/pr-line-comments-5118 into mindersec:main

krrish175-byte

A bunch of comments, but this is looking pretty good. Adding line-level comment support to our PR action is a great feature and the refactor of Alert.run() makes it much cleaner. Once you've made the adjustments from these comments, we can merge.

krrish175-byte · 2026-06-22T07:51:57Z

+		if lc.GetLineStart() <= 0 {
+			return fmt.Errorf("%w: line_comment %d: line_start must be greater than 0", ErrInvalidRuleTypeDefinition, i)
+		}
+		if lc.GetLineEnd() <= 0 {


Since line_start and line_end default to 0 in protobufs, requiring both to be > 0 means a user must specify both, even for a single-line comment. I see you handle line_start == line_end gracefully in getParamsForPRComment by omitting StartLine for the GitHub API.

This is me thinking out loud: Should we allow line_start to be omitted (i.e. 0) and default to line_end for single-line comments in the profile validation logic, so the user only has to specify line_end for a single-line comment? Pick an option. If we keep the current behavior, just make sure to document that both are required.

I kept both line_start and line_end required for consistency and explicitness in the validation logic. Since the GitHub API supports deriving a single-line comment when line_start == line_end, I thought requiring both fields would make the configuration less ambiguous. I'm happy to adjust the validation to allow omitting line_start if you think that provides a better user experience.

I think omitting line_end might be more natural.

That's a good suggestion. I agree that omitting line_end for single-line comments would be a more natural API. I'll keep that in mind while revisiting the design.

…t_comment.go Co-authored-by: Krrish Biswas <krrishbiswas175@gmail.com>

evankanderson

Sorry that I didn't look at the spec in #5118 closely enough when you started -- it's not clear that the line_comments as specified are particularly useful for PR comments, as (for example) the number and position of the comments are fixed. This might make sense if you wanted to comment on line 1 of a particular file that the contents should be a specific value, but a lot of common checks (e.g. the Chainguard "boilerplate" checks that we run to check for license headers) have at least some variance in terms of either filename or position in the file.

I think you may need to go back to #5118 and work out a few use cases more clearly to get a better spec for the API than was described in the issue. (To explain myself -- I'd read the title and said "yeah, we need line-level comments" and didn't actually read the implementation suggestion. 🤦 )

Sorry to have sent you in the wrong direction!

evankanderson · 2026-06-22T18:33:57Z

+                            gt: 0,
+                        },
+                        (google.api.field_behavior) = REQUIRED
+                    ];


Is line_end required to be greater than or equal to line_start?

For GitLab, I'm assuming you'd use the thread in merge request diff API, as that's the only one that seems to support -multi-line comments. You might look at the APIs for both GitLab and GitHub to see what kinds of parameters they accept. For example, both seem to have the ability comment on either the "old" or "new" side (LEFT or RIGHT in GitHub).

Thanks for taking another look at the spec. That makes sense . I implemented the API based on the requirements described in #5118, but I can see how fixed line positions may not be flexible enough for many real-world checks.

Regarding line_start and line_end, my assumption was:

Single-line comments: line_start == line_end

Multi-line comments: line_end >= line_start

I'll revisit the use cases in #5118 and review the GitHub and GitLab review comment APIs to better understand the positioning models before making further changes.
Thanks for the feedback and clarification.

evankanderson · 2026-06-22T18:36:59Z

+                        (google.api.field_behavior) = REQUIRED
+                    ];
+                }
+                repeated PullRequestLineComment line_comments = 3;


Practically, how does using line_comments like this work? I can see how a single PR comment with template expansion could address finding anywhere from 1-100 items (using a loop in the template to repeat contents), but I don't see how this format could work for something that might produce more than one finding.

That was my assumption as well — each entry in line_comments would map to a separate review comment tied to a specific line or line range.

I agree that this could become noisy for checks that produce a large number of findings. A single PR comment with template expansion would definitely scale better in those cases.

As a practical example, it would be great to be able to replace the boilerplate license checks with a Minder PR check / remediation.

This might be a good working example for some of the features needed.

That's a helpful example. I'll use the boilerplate license checks as a concrete use case and work through how the API would handle multiple findings before making further changes.

evankanderson · 2026-06-22T18:39:45Z

 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
+}


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.

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

evankanderson · 2026-06-22T20:19:32Z

+			// 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)")


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.)

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.

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

Here are a couple:

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.

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.

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.

evankanderson · 2026-06-22T20:23:18Z

+	action := alert.reviewCfg.GetAction()
+	if action == "" {
+		action = "comment"


What's going on here? Indentation seems off.

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

evankanderson · 2026-06-22T20:24:34Z

+			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)


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

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.

evankanderson

I've suggested a couple sample "test cases" you might be able to use while developing this feature, but feel free to come up with your own.

evankanderson · 2026-06-24T16:32:06Z

+		if lc.GetLineStart() <= 0 {
+			return fmt.Errorf("%w: line_comment %d: line_start must be greater than 0", ErrInvalidRuleTypeDefinition, i)
+		}
+		if lc.GetLineEnd() <= 0 {


I think omitting line_end might be more natural.

evankanderson · 2026-06-24T16:34:24Z

+                        (google.api.field_behavior) = REQUIRED
+                    ];
+                }
+                repeated PullRequestLineComment line_comments = 3;


As a practical example, it would be great to be able to replace the boilerplate license checks with a Minder PR check / remediation.

This might be a good working example for some of the features needed.

evankanderson · 2026-06-24T16:47:53Z

+			// 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)")


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

Here are a couple:

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.

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 · 2026-06-30T11:52:57Z

Hi! Just checking in on this PR. I've addressed the review comments and replied to the discussions. Let me know if there are any further changes you'd like me to make. Thanks!

evankanderson · 2026-07-01T04:08:17Z

I'm concerned about the API surface for PR line comments. The current design seems to be limited to fixed lines and files, which seems unlikely to be generally useful. In particular, I don't want to merge the current API because it will commit us to supporting that API even if we come up with a much better solution in the future (for example, specifying a rule output key pointing to either SARIF or a custom JSON format value).

MahekPatel-2403 · 2026-07-01T05:41:39Z

Thanks for the clarification. I understand the concern about committing to an API that may be too restrictive. I'll spend some time thinking about how a more flexible approach, such as driving review comments from structured rule output, could fit into Minder before making further changes.

MahekPatel-2403 added 2 commits June 21, 2026 15:33

refactor(alert): extract PR comment action handlers

fdf9f24

MahekPatel-2403 requested a review from a team as a code owner June 21, 2026 19:02

krrish175-byte reviewed Jun 22, 2026

View reviewed changes

MahekPatel-2403 and others added 3 commits June 22, 2026 13:30

Update internal/engine/actions/alert/pull_request_comment/pull_reques…

d9e7fc2

…t_comment.go Co-authored-by: Krrish Biswas <krrishbiswas175@gmail.com>

test(alert): add template rendering failure case

302dc52

fix(alert): address review feedback

de3baec

evankanderson requested changes Jun 22, 2026

View reviewed changes

evankanderson reviewed Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

MahekPatel-2403 commented Jun 21, 2026

Summary

Testing

Uh oh!

CLAassistant commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

krrish175-byte 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!

Uh oh!

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!

MahekPatel-2403 Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

MahekPatel-2403 commented Jun 30, 2026

Uh oh!

evankanderson commented Jul 1, 2026

Uh oh!

MahekPatel-2403 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CLAassistant commented Jun 21, 2026 •

edited

Loading

coveralls commented Jun 22, 2026 •

edited

Loading

MahekPatel-2403 Jun 22, 2026 •

edited

Loading

MahekPatel-2403 commented Jul 1, 2026 •

edited

Loading