Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 43 additions & 15 deletions internal/diff/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

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.

i++

for i < len(lines) && !strings.HasPrefix(lines[i], diffGitPrefix) && !strings.HasPrefix(lines[i], hunkPrefix) {
Expand Down Expand Up @@ -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.

}

// 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 {
Expand Down
7 changes: 5 additions & 2 deletions internal/diff/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.

StartLine int
Lines []Line
OldStartLine int
OldLineCount int
NewStartLine int
NewLineCount int
Lines []Line
}

type File struct {
Expand Down
8 changes: 7 additions & 1 deletion internal/github/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -21,7 +24,7 @@ type Client struct {
func New(token string) *Client {
return &Client{
token: token,
httpClient: &http.Client{},
httpClient: &http.Client{Timeout: httpTimeout},
}
}

Expand Down Expand Up @@ -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))
Expand Down
13 changes: 11 additions & 2 deletions internal/prompt/prompt.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 4 additions & 1 deletion internal/provider/gemini/gemini.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.

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)
}
Expand Down
Loading