feat: inline review comments on specific lines #26

Merged
rodin merged 6 commits from feat/inline-review-comments into main 2026-05-02 06:13:02 +00:00
2 changed files with 108 additions and 0 deletions
Showing only changes of commit 1c2292265b - Show all commits
+29
View File
@@ -257,6 +257,15 @@ func main() {
log.Printf("Attaching %d inline comments", len(inlineComments))
}
// Check if existing review is unchanged — skip to preserve conversation threads
if *updateExisting && *reviewerName != "" {
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err == nil && reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
log.Printf("Review unchanged from previous run; skipping to preserve threads")
return
}
}
Review

[MAJOR] Escalation now happens before posting and depends on deriving ownLogin from an existing review containing the sentinel. On the first run (no existing review with this sentinel), ownLogin is empty and escalation is skipped, so an APPROVED review may be posted even if a sibling bot (same token) already has REQUEST_CHANGES. Previously this was handled by escalating after posting using the posted user login.

**[MAJOR]** Escalation now happens before posting and depends on deriving ownLogin from an existing review containing the sentinel. On the first run (no existing review with this sentinel), ownLogin is empty and escalation is skipped, so an APPROVED review may be posted even if a sibling bot (same token) already has REQUEST_CHANGES. Previously this was handled by escalating after posting using the posted user login.
log.Printf("Posting review (event=%s)...", event)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
if err != nil {
@@ -465,3 +474,23 @@ func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSent
}
return false
}
Outdated
Review

[MINOR] shouldEscalate relies on detecting a 'sentinel' substring in review bodies to identify sibling bot reviews. An untrusted user could spoof the sentinel in their own review, potentially causing unintended escalation. Consider additionally verifying the matched review's user login matches the bot's account (derived independently from the API) before treating it as a sibling.

**[MINOR]** shouldEscalate relies on detecting a 'sentinel' substring in review bodies to identify sibling bot reviews. An untrusted user could spoof the sentinel in their own review, potentially causing unintended escalation. Consider additionally verifying the matched review's user login matches the bot's account (derived independently from the API) before treating it as a sibling.
// reviewUnchanged checks if an existing review with the same sentinel
// already has identical body and state. Returns true if a re-post would
// produce the same result (skip to preserve conversation threads).
func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string) bool {
for _, r := range reviews {
if r.Stale {
continue
}
Outdated
Review

[MINOR] old finding Superseded by newer review

~~**[MINOR]** old finding~~ *Superseded by newer review*
if !strings.Contains(r.Body, sentinel) {
continue
}
// Compare state (map APPROVED back from Gitea's representation)
existingEvent := r.State
if existingEvent == r.State && existingEvent == newEvent && r.Body == newBody {
return true
}
}
return false
}
+79
View File
@@ -148,3 +148,82 @@ func TestShouldEscalate(t *testing.T) {
})
}
}
func TestReviewUnchanged(t *testing.T) {
tests := []struct {
name string
existing []gitea.Review
newBody string
newEvent string
sentinel string
want bool
}{
{
name: "no existing review",
existing: nil,
newBody: "new review",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "identical body and state",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "same body\n<!-- review-bot:sonnet -->"),
},
newBody: "same body\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "same body but different state",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:sonnet -->"),
},
newBody: "body\n<!-- review-bot:sonnet -->",
newEvent: "REQUEST_CHANGES",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "different body same state",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "old body\n<!-- review-bot:sonnet -->"),
},
newBody: "new body\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "stale review with same body (should still post)",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", true, "same\n<!-- review-bot:sonnet -->"),
},
newBody: "same\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "different sentinel (not our review)",
existing: []gitea.Review{
makeReview(100, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
},
newBody: "body\n<!-- review-bot:sonnet -->",
newEvent: "APPROVED",
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := reviewUnchanged(tc.existing, tc.newBody, tc.newEvent, tc.sentinel)
if got != tc.want {
t.Errorf("reviewUnchanged() = %v, want %v", got, tc.want)
}
})
}
}