Skip to content

Commit 8c0c918

Browse files
committed
fix: prevent multiple conflicting review comments
Root cause of PR #6 comment chaos: 1. Review posted '❌ Issues found' comment 2. Auto-fix posted '✅ LGTM' BEFORE re-review (wrong!) 3. Re-review posted another comment Fixed by: - Only post comment for FINAL review result - Don't post 'LGTM' until after re-review confirms - Track re-review state with isReReview parameter - Combine auto-fix status with original review in single comment - Re-review gets marked as 'RE-REVIEW after auto-fix' Now posts maximum 2 comments instead of 3+ conflicting ones.
1 parent 424f799 commit 8c0c918

1 file changed

Lines changed: 20 additions & 13 deletions

File tree

internal/evolution/git.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,8 @@ If the changes are good, reply: "LGTM"
193193
`, e.prNumber, util.Truncate(prDiff, 8000), e.branchName)
194194
}
195195

196-
func (e *Engine) reviewPR(ctx context.Context, p iteragent.Provider, tools []iteragent.Tool, systemPrompt string, skills *iteragent.SkillSet) error {
196+
// reviewPR runs the AI self-review. If isReReview is true, this is a second review after auto-fix.
197+
func (e *Engine) reviewPR(ctx context.Context, p iteragent.Provider, tools []iteragent.Tool, systemPrompt string, skills *iteragent.SkillSet, isReReview ...bool) error {
197198
if e.prNumber == 0 {
198199
return fmt.Errorf("no PR to review")
199200
}
@@ -206,6 +207,9 @@ func (e *Engine) reviewPR(ctx context.Context, p iteragent.Provider, tools []ite
206207
}
207208

208209
userMsg := e.buildPRReviewMessage(prDiff)
210+
if len(isReReview) > 0 && isReReview[0] {
211+
userMsg = "This is a RE-REVIEW after auto-fixes were applied.\n\n" + userMsg
212+
}
209213

210214
a := e.newAgent(p, tools, systemPrompt, skills)
211215
var reviewOutput string
@@ -225,32 +229,39 @@ func (e *Engine) reviewPR(ctx context.Context, p iteragent.Provider, tools []ite
225229
low := strings.ToLower(reviewOutput)
226230
passed := strings.Contains(low, "lgtm") || strings.Contains(low, "looks good")
227231

228-
// Always post the review output as a PR comment for visibility.
229-
e.postReviewComment(ctx, reviewOutput, passed)
230-
231232
if passed {
233+
// Only post comment for final successful review
234+
e.postReviewComment(ctx, reviewOutput, true)
232235
e.logger.Info("PR self-review passed")
233236
return nil
234237
}
235238

236-
// Reviewer found issues — try to auto-fix them
239+
// Reviewer found issues — try to auto-fix them (only on first review)
240+
if len(isReReview) > 0 && isReReview[0] {
241+
// Already tried auto-fix, still failed
242+
e.postReviewComment(ctx, reviewOutput, false)
243+
return fmt.Errorf("review blocked merge: issues remain after auto-fix")
244+
}
245+
237246
e.logger.Warn("PR self-review found issues — attempting auto-fix")
238247

239248
fixed, fixErr := e.autoFixIssues(ctx, p, tools, systemPrompt, skills, reviewOutput)
240249
if fixErr != nil {
241250
e.logger.Error("auto-fix failed", "err", fixErr)
242-
e.postReviewComment(ctx, "Auto-fix attempt failed: "+fixErr.Error(), false)
251+
// Post the original review + auto-fix failure
252+
e.postReviewComment(ctx, reviewOutput+"\n\n---\n**Auto-fix failed:** "+fixErr.Error(), false)
243253
return fmt.Errorf("review blocked merge: %w", fixErr)
244254
}
245255

246256
if !fixed {
247257
e.logger.Warn("auto-fix could not resolve all issues — blocking merge")
258+
e.postReviewComment(ctx, reviewOutput+"\n\n---\n**Auto-fix:** Could not resolve all issues automatically.", false)
248259
return fmt.Errorf("review blocked merge: auto-fix could not resolve all issues")
249260
}
250261

251262
// Re-review after fixes
252263
e.logger.Info("auto-fix applied — re-reviewing PR")
253-
return e.reviewPR(ctx, p, tools, systemPrompt, skills)
264+
return e.reviewPR(ctx, p, tools, systemPrompt, skills, true)
254265
}
255266

256267
// autoFixIssues attempts to fix issues identified during review.
@@ -267,17 +278,14 @@ Fix these issues in the codebase. Run tests to verify the fixes work.
267278
If you cannot fix an issue, explain why. Only commit if tests pass.`, reviewOutput)
268279

269280
a := e.newAgent(p, tools, systemPrompt, skills)
270-
var fixResult string
271281
for ev := range a.Prompt(ctx, fixPrompt) {
272282
if e.eventSink != nil {
273283
select {
274284
case e.eventSink <- ev:
275285
default:
276286
}
277287
}
278-
if ev.Type == string(iteragent.EventMessageEnd) {
279-
fixResult = ev.Content
280-
}
288+
281289
}
282290
a.Finish()
283291

@@ -323,8 +331,7 @@ If you cannot fix an issue, explain why. Only commit if tests pass.`, reviewOutp
323331
return false, fmt.Errorf("failed to push auto-fix: %w", err)
324332
}
325333

326-
e.logger.Info("auto-fix applied successfully")
327-
e.postReviewComment(ctx, fmt.Sprintf("Auto-fix applied:\n\n%s\n\nChanges committed and pushed.", fixResult), true)
334+
e.logger.Info("auto-fix applied successfully - will re-review")
328335
return true, nil
329336
}
330337

0 commit comments

Comments
 (0)