Skip to content

fix: address legitimate review feedback#2

Merged
monokrome merged 2 commits intomainfrom
fix/review-feedback
Mar 14, 2026
Merged

fix: address legitimate review feedback#2
monokrome merged 2 commits intomainfrom
fix/review-feedback

Conversation

@monokrome
Copy link
Member

Summary

Fixes from the bot's own review of the initial PR:

  • Hunk header parsing: Hunk struct now captures all four components (OldStartLine, OldLineCount, NewStartLine, NewLineCount), producing correct @@ -old,count +new,count @@ headers in the prompt
  • Base reviewer instructions: System prompt now guides the LLM to focus on code changes (not project decisions), avoid uncertain factual claims, not repeat prior comments, and be sparing
  • HTTP timeouts: GitHub client (30s) and Gemini client (120s)
  • Error handling: Check io.ReadAll error before unmarshalling in comment fetcher

Test plan

  • Verify the review workflow triggers and produces fewer noisy comments
  • Confirm hunk headers in LLM prompt are accurate
  • Check that repeated suggestions (like the Ubuntu one) don't recur

Add base reviewer instructions to prevent common LLM missteps:
focus on code changes not project decisions, don't make uncertain
factual claims, don't repeat prior comments, be sparing.

Fix hunk header parsing to capture all four components
(old_start, old_count, new_start, new_count) so the LLM sees
accurate diff context.

Add HTTP timeouts to GitHub (30s) and Gemini (120s) clients.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The pull request significantly improves diff parsing logic and enhances HTTP client robustness by adding timeouts. However, there's a duplicate error check in internal/github/client.go that needs to be addressed.

}

lineNum := hunk.StartLine
lineNum := hunk.NewStartLine

Choose a reason for hiding this comment

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

praise: Changing hunk.StartLine to hunk.NewStartLine is correct. This ensures line numbers for added and context lines are tracked relative to the new file, which is essential for accurate commenting.

@@ -109,29 +109,57 @@ func resolvePathFromHeaders(lines []string, start int, fallback string) string {
return fallback

Choose a reason for hiding this comment

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

praise: The refactor of parseHunkHeader and the introduction of parseRange significantly improve the accuracy and robustness of diff hunk header parsing. This is a critical correctness fix for handling the full @@ -old_start,old_count +new_start,new_count @@ format.

@@ -15,8 +15,11 @@ type Line struct {
}

type Hunk struct {

Choose a reason for hiding this comment

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

praise: Updating the Hunk struct to include OldStartLine, OldLineCount, NewStartLine, and NewLineCount is a good design choice, allowing for a more complete and accurate representation of diff hunks.

@@ -70,7 +72,8 @@ func New(apiKey string, model string) provider.ReviewFunc {

Choose a reason for hiding this comment

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

praise: Similar to the GitHub client, adding an httpTimeout to the Gemini HTTP client is a good improvement for robustness.

@monokrome monokrome merged commit 3e16a5b into main Mar 14, 2026
2 checks passed
@monokrome monokrome deleted the fix/review-feedback branch March 14, 2026 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant