From a25505ebe71ea80c3ccf5d19c1c4865b198a6a84 Mon Sep 17 00:00:00 2001 From: Bailey Stoner Date: Sat, 14 Mar 2026 15:24:31 -0700 Subject: [PATCH 1/2] fix: improve review quality and fix hunk header parsing 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. --- internal/diff/parse.go | 58 ++++++++++++++++++++++-------- internal/diff/types.go | 7 ++-- internal/github/client.go | 5 ++- internal/prompt/prompt.go | 13 +++++-- internal/provider/gemini/gemini.go | 5 ++- 5 files changed, 67 insertions(+), 21 deletions(-) diff --git a/internal/diff/parse.go b/internal/diff/parse.go index a2f1f41..8c9f169 100644 --- a/internal/diff/parse.go +++ b/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 { diff --git a/internal/diff/types.go b/internal/diff/types.go index 223bb24..192b8ee 100644 --- a/internal/diff/types.go +++ b/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 { diff --git a/internal/github/client.go b/internal/github/client.go index 9cd8a11..1f87733 100644 --- a/internal/github/client.go +++ b/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}, } } diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 16e6589..cd4038b 100644 --- a/internal/prompt/prompt.go +++ b/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 { diff --git a/internal/provider/gemini/gemini.go b/internal/provider/gemini/gemini.go index f3d86c9..918f639 100644 --- a/internal/provider/gemini/gemini.go +++ b/internal/provider/gemini/gemini.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "strings" + "time" "github.com/monokrome/codereview/internal/provider" ) @@ -14,6 +15,7 @@ import ( 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) } From 306c426b7b0ea99d529a9532b0e1ce1ec1f02d6a Mon Sep 17 00:00:00 2001 From: Bailey Stoner Date: Sat, 14 Mar 2026 15:32:14 -0700 Subject: [PATCH 2/2] fix: check io.ReadAll error before unmarshalling in comment fetcher --- internal/github/client.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/github/client.go b/internal/github/client.go index 1f87733..6b3e880 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -63,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))