Merge pull request #2 from LimpidTech/fix/review-feedback

fix: address legitimate review feedback

Author: monokrome

internal/diff/parse.go

@@ -44,7 +44,7 @@ func Parse(diffText string) ([]File, error) {
 			return nil, fmt.Errorf("parsing hunk header %q: %w", line, err)
 		}
 
-		lineNum := hunk.StartLine
+		lineNum := hunk.NewStartLine
 		i++
 
 		for i < len(lines) && !strings.HasPrefix(lines[i], diffGitPrefix) && !strings.HasPrefix(lines[i], hunkPrefix) {
@@ -109,29 +109,57 @@ func resolvePathFromHeaders(lines []string, start int, fallback string) string {
 	return fallback
 }
 
+// parseHunkHeader parses "@@ -old,count +new,count @@" into a Hunk.
 func parseHunkHeader(line string) (Hunk, error) {
-	atIdx := strings.Index(line, "+")
-	if atIdx < 0 {
-		return Hunk{}, fmt.Errorf("no + range in hunk header")
+	// Find the range portion between the @@ markers
+	// Format: @@ -old_start[,old_count] +new_start[,new_count] @@
+	trimmed := strings.TrimPrefix(line, "@@ ")
+	if end := strings.Index(trimmed, " @@"); end >= 0 {
+		trimmed = trimmed[:end]
 	}
 
-	rest := line[atIdx+1:]
-	endIdx := strings.Index(rest, " ")
-	if endIdx < 0 {
-		endIdx = strings.Index(rest, hunkPrefix)
+	parts := strings.SplitN(trimmed, " ", 2)
+	if len(parts) < 2 {
+		return Hunk{}, fmt.Errorf("invalid hunk header: %q", line)
 	}
-	if endIdx < 0 {
-		endIdx = len(rest)
+
+	oldRange := strings.TrimPrefix(parts[0], "-")
+	newRange := strings.TrimPrefix(parts[1], "+")
+
+	oldStart, oldCount, err := parseRange(oldRange)
+	if err != nil {
+		return Hunk{}, fmt.Errorf("parsing old range %q: %w", oldRange, err)
+	}
+
+	newStart, newCount, err := parseRange(newRange)
+	if err != nil {
+		return Hunk{}, fmt.Errorf("parsing new range %q: %w", newRange, err)
 	}
 
-	rangeStr := rest[:endIdx]
-	parts := strings.SplitN(rangeStr, ",", 2)
-	startLine, err := strconv.Atoi(parts[0])
+	return Hunk{
+		OldStartLine: oldStart,
+		OldLineCount: oldCount,
+		NewStartLine: newStart,
+		NewLineCount: newCount,
+	}, nil
+}
+
+func parseRange(s string) (int, int, error) {
+	parts := strings.SplitN(s, ",", 2)
+	start, err := strconv.Atoi(parts[0])
 	if err != nil {
-		return Hunk{}, fmt.Errorf("parsing start line %q: %w", parts[0], err)
+		return 0, 0, fmt.Errorf("parsing start: %w", err)
+	}
+
+	count := 1
+	if len(parts) == 2 {
+		count, err = strconv.Atoi(parts[1])
+		if err != nil {
+			return 0, 0, fmt.Errorf("parsing count: %w", err)
+		}
 	}
 
-	return Hunk{StartLine: startLine}, nil
+	return start, count, nil
 }
 
 func trimLeadingSpace(s string) string {

internal/diff/types.go

@@ -15,8 +15,11 @@ type Line struct {
 }
 
 type Hunk struct {
-	StartLine int
-	Lines     []Line
+	OldStartLine int
+	OldLineCount int
+	NewStartLine int
+	NewLineCount int
+	Lines        []Line
 }
 
 type File struct {

internal/github/client.go

@@ -7,10 +7,13 @@ import (
 	"io"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/monokrome/codereview/internal/review"
 )
 
+const httpTimeout = 30 * time.Second
+
 const apiBase = "https://api.github.com"
 
 type Client struct {
@@ -21,7 +24,7 @@ type Client struct {
 func New(token string) *Client {
 	return &Client{
 		token:      token,
-		httpClient: &http.Client{},
+		httpClient: &http.Client{Timeout: httpTimeout},
 	}
 }
 
@@ -60,6 +63,9 @@ func (c *Client) fetchAllPRComments(ctx context.Context, owner, repo string, prN
 
 		body, err := io.ReadAll(resp.Body)
 		resp.Body.Close()
+		if err != nil {
+			return nil, fmt.Errorf("reading response: %w", err)
+		}
 
 		if resp.StatusCode != http.StatusOK {
 			return nil, fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body))

internal/prompt/prompt.go

@@ -9,6 +9,15 @@ import (
 
 const systemTemplate = `You are a senior code reviewer. Review the provided unified diff and produce a JSON response.
 
+Reviewer principles:
+- Review only the code changes in the diff. Do not critique project-level decisions like CI configuration, tech stack choices, runner OS, or deployment strategy.
+- Accept existing project conventions as intentional. If the codebase uses a particular pattern, do not suggest changing it unless the diff introduces an inconsistency with that pattern.
+- Do not make factual claims unless you are certain. If you are unsure whether something is correct (e.g., whether a version exists, whether an API behaves a certain way), say so rather than asserting.
+- Do not suggest adding dependencies, frameworks, or libraries unless the code has a clear bug that requires one.
+- Never repeat a comment you have already made. If your prior comments are listed below, do not raise the same point again. You may note whether prior feedback was addressed.
+- Focus on correctness, bugs, security issues, and logic errors over style preferences.
+- Be sparing with comments. A review with zero comments is perfectly valid if the code is correct.
+
 Use these conventional comment labels:
 - "nit": style or trivial improvements that don't affect correctness
 - "suggestion": a better approach or alternative worth considering
@@ -37,7 +46,7 @@ Your response must be valid JSON matching this exact structure:
   ]
 }
 
-Rules:
+Output rules:
 - The "line" field must reference a valid line number from the diff (a line that was added or exists as context in the new file)
 - The "body" field must start with the label followed by a colon and space (e.g. "nit: unused import")
 - The "path" field must match a file path from the diff exactly
@@ -97,7 +106,7 @@ func Build(files []diff.File, instructions string, priorComments []PriorComment,
 		fmt.Fprintf(&user, "--- a/%s\n+++ b/%s\n", f.Path, f.Path)
 
 		for _, h := range f.Hunks {
-			fmt.Fprintf(&user, "@@ -%d +%d @@\n", h.StartLine, h.StartLine)
+			fmt.Fprintf(&user, "@@ -%d,%d +%d,%d @@\n", h.OldStartLine, h.OldLineCount, h.NewStartLine, h.NewLineCount)
 
 			for _, l := range h.Lines {
 				switch l.Kind {

internal/provider/gemini/gemini.go

@@ -7,13 +7,15 @@ import (
 	"io"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/monokrome/codereview/internal/provider"
 )
 
 const (
 	DefaultModel = "gemini-2.5-flash"
 	apiBaseURL   = "https://generativelanguage.googleapis.com/v1beta/models"
+	httpTimeout  = 120 * time.Second
 )
 
 type part struct {
@@ -70,7 +72,8 @@ func New(apiKey string, model string) provider.ReviewFunc {
 
 		httpReq.Header.Set("Content-Type", "application/json")
 
-		resp, err := http.DefaultClient.Do(httpReq)
+		client := &http.Client{Timeout: httpTimeout}
+		resp, err := client.Do(httpReq)
 		if err != nil {
 			return provider.Response{}, fmt.Errorf("calling Gemini API: %w", err)
 		}