fix: address review findings
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m17s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m22s
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m17s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m22s
- findOwnReview: skip superseded reviews, pick highest ID (most recent) - findOwnReviewStrict: verify authorship before superseding (defense-in-depth) - buildSupersededBody: handle empty commitSHA gracefully - Tests: add cases for superseded skip, highest-ID selection
This commit is contained in:
+41
-7
@@ -327,7 +327,7 @@ func main() {
|
|||||||
// In shared-token mode, skip superseding to avoid clobbering sibling reviews.
|
// In shared-token mode, skip superseding to avoid clobbering sibling reviews.
|
||||||
sharedToken := hasSharedToken(existingReviews, sentinel)
|
sharedToken := hasSharedToken(existingReviews, sentinel)
|
||||||
if !sharedToken {
|
if !sharedToken {
|
||||||
existing := findOwnReview(existingReviews, sentinel)
|
existing := findOwnReviewStrict(existingReviews, sentinel, *reviewerName)
|
||||||
if existing != nil {
|
if existing != nil {
|
||||||
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
|
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -522,9 +522,13 @@ func buildSupersededBody(originalBody, commitSHA, sentinel string) string {
|
|||||||
var sb strings.Builder
|
var sb strings.Builder
|
||||||
sb.WriteString("~~Original review~~\n\n")
|
sb.WriteString("~~Original review~~\n\n")
|
||||||
sb.WriteString("**Superseded** \u2014 see current review for up-to-date findings.\n\n")
|
sb.WriteString("**Superseded** \u2014 see current review for up-to-date findings.\n\n")
|
||||||
sb.WriteString("<details><summary>Previous findings (commit ")
|
if shortSHA != "" {
|
||||||
sb.WriteString(shortSHA)
|
sb.WriteString("<details><summary>Previous findings (commit ")
|
||||||
sb.WriteString(")</summary>\n\n")
|
sb.WriteString(shortSHA)
|
||||||
|
sb.WriteString(")</summary>\n\n")
|
||||||
|
} else {
|
||||||
|
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
||||||
|
}
|
||||||
sb.WriteString(originalBody)
|
sb.WriteString(originalBody)
|
||||||
sb.WriteString("\n\n</details>\n\n")
|
sb.WriteString("\n\n</details>\n\n")
|
||||||
sb.WriteString(sentinel)
|
sb.WriteString(sentinel)
|
||||||
@@ -574,10 +578,40 @@ func extractSentinelName(body string) string {
|
|||||||
|
|
||||||
// findOwnReview locates a review matching the given sentinel in its body.
|
// findOwnReview locates a review matching the given sentinel in its body.
|
||||||
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
||||||
|
var best *gitea.Review
|
||||||
for i := range reviews {
|
for i := range reviews {
|
||||||
if strings.Contains(reviews[i].Body, sentinel) {
|
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||||
return &reviews[i]
|
continue
|
||||||
|
}
|
||||||
|
// Skip superseded reviews (they contain our sentinel in the collapsed body)
|
||||||
|
if strings.Contains(reviews[i].Body, "~~Original review~~") {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
// Take the highest ID (most recent)
|
||||||
|
if best == nil || reviews[i].ID > best.ID {
|
||||||
|
best = &reviews[i]
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return nil
|
return best
|
||||||
|
}
|
||||||
|
|
||||||
|
// findOwnReviewStrict is like findOwnReview but also verifies the review
|
||||||
|
// was posted by the expected user (defense-in-depth against sentinel injection).
|
||||||
|
func findOwnReviewStrict(reviews []gitea.Review, sentinel, expectedLogin string) *gitea.Review {
|
||||||
|
var best *gitea.Review
|
||||||
|
for i := range reviews {
|
||||||
|
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if strings.Contains(reviews[i].Body, "~~Original review~~") {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if expectedLogin != "" && reviews[i].User.Login != expectedLogin {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if best == nil || reviews[i].ID > best.ID {
|
||||||
|
best = &reviews[i]
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return best
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -140,6 +140,32 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
wantID: 20,
|
wantID: 20,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "skips superseded review",
|
||||||
|
reviews: []gitea.Review{
|
||||||
|
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
|
||||||
|
makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
|
||||||
|
},
|
||||||
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
|
wantID: 20,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "only superseded reviews exist",
|
||||||
|
reviews: []gitea.Review{
|
||||||
|
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
|
||||||
|
},
|
||||||
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
|
wantNil: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "picks highest ID among matches",
|
||||||
|
reviews: []gitea.Review{
|
||||||
|
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
|
||||||
|
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
|
||||||
|
},
|
||||||
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
|
wantID: 50,
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tc := range tests {
|
for _, tc := range tests {
|
||||||
|
|||||||
Reference in New Issue
Block a user