Conversation
Composite GitHub Action that reviews pull requests using configurable LLM providers. Posts inline review comments using conventional comment format (nit:, issue:, suggestion:, etc.) and submits a verdict. Starts with Gemini support, designed to add more providers easily. Includes dogfooding workflow to review its own PRs.
LLMs sometimes reference lines outside the diff. Filter those out with warnings instead of aborting the entire review. Also fixes cache-dependency-path for setup-go.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall, the project is well-structured with good separation of concerns and robust error handling, especially in the filterValidComments function. However, an issue was found with the diff hunk header reconstruction for the LLM prompt, and a few minor suggestions were identified.
|
|
||
| jobs: | ||
| review: | ||
| runs-on: macos-latest |
There was a problem hiding this comment.
suggestion: Consider using ubuntu-latest instead of macos-latest for the runs-on property. Ubuntu runners are generally more cost-effective and often faster for Go applications, unless there's a specific macOS requirement not apparent here.
|
|
||
| if cfg.Provider == "" { | ||
| cfg.Provider = "gemini" | ||
| } |
There was a problem hiding this comment.
suggestion: The default for provider is already set to gemini in action.yml. This line (if cfg.Provider == "" { cfg.Provider = "gemini" }) duplicates that logic. It's generally better to have such defaults in a single, clear place, like action.yml.
| newFile := File{Path: path} | ||
| files = append(files, newFile) | ||
| current = &files[len(files)-1] | ||
| current.Path = resolvePathFromHeaders(lines, i, path) |
There was a problem hiding this comment.
question: Is resolvePathFromHeaders strictly necessary? extractPath already appears to correctly derive the new file path from the diff --git line, even in cases of renames (e.g., diff --git a/old b/new). If extractPath is always reliable for the b/ path, this might be redundant, or perhaps it's a defensive fallback for specific diff formats not covered by extractPath?
| token: token, | ||
| httpClient: &http.Client{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: It's good practice to add a timeout to the http.Client for robustness, especially when making external network requests. E.g., Timeout: 30 * time.Second.
| func Build(files []diff.File, instructions string) (string, string) { | ||
| var user strings.Builder | ||
|
|
||
| if instructions != "" { |
There was a problem hiding this comment.
praise: The systemTemplate is exceptionally well-defined. It clearly outlines the rules, expected JSON structure, comment labels, and output format, which is crucial for guiding the LLM's responses effectively.
| fmt.Fprintf(&user, "@@ -%d +%d @@\n", h.StartLine, h.StartLine) | ||
|
|
||
| for _, l := range h.Lines { | ||
| switch l.Kind { |
There was a problem hiding this comment.
issue: The diff hunk header reconstruction is inaccurate. fmt.Fprintf(&user, "@@ -%d +%d @@\n", h.StartLine, h.StartLine) generates @@ -X +X @@. A correct unified diff hunk header should be @@ -old_start_line,old_num_lines +new_start_line,new_num_lines @@. The diff.Hunk struct currently only stores StartLine (which is the new start line). To accurately reconstruct the diff for the LLM, the diff.Hunk struct needs to be extended to capture the old_start_line, old_num_lines, new_start_line, and new_num_lines from the hunk header during parsing. Providing the LLM with an accurate diff is critical for its ability to understand the context and generate precise comments.
| return provider.Response{}, fmt.Errorf("calling Gemini API: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
suggestion: Similar to the GitHub client, consider using a custom http.Client with a timeout for Gemini API calls to prevent requests from hanging indefinitely.
|
|
||
| var valid []Comment | ||
| for _, c := range comments { | ||
| if !IsValidLabel(c.Label) { |
There was a problem hiding this comment.
praise: The filterValidComments function is excellent. It performs robust validation, ensuring that comments have valid labels, positive line numbers, and correctly map to lines in the new file within the provided diff. This is crucial for successful interaction with the GitHub API and preventing invalid comments.
The action now responds when someone replies to a review comment. Fetches the conversation thread, sends it to the LLM with the relevant diff hunk, and posts a reply. Skips its own comments to prevent infinite loops.
There was a problem hiding this comment.
The overall architecture and implementation are well-structured and logical. The action successfully sets up the Go environment, builds the application, and orchestrates the review process. The diff parsing, GitHub API interaction, and LLM prompting are handled robustly. Several minor issues and suggestions are provided for improvement, primarily around cost optimization, error handling, and prompt clarity.
| review: | ||
| runs-on: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
suggestion: Using macos-latest for runs-on is quite expensive. For a Go build and a simple action execution, ubuntu-latest should be sufficient and significantly cheaper. Consider changing this unless there's a specific macOS dependency.
| - name: Setup Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.24' |
There was a problem hiding this comment.
nit: While 1.24 works, it's generally recommended to pin to a specific patch version (e.g., 1.22.x) or just use 1 to avoid unexpected breaking changes if the upstream action updates 1.24 to a future 1.24.x that has issues, though for Go 1.24 is not a strict release yet.
|
|
||
| - name: Build | ||
| shell: bash | ||
| run: cd ${{ github.action_path }} && go build -o $RUNNER_TEMP/codereview ./cmd/codereview |
There was a problem hiding this comment.
suggestion: Building into $RUNNER_TEMP/codereview is fine, but it might be slightly cleaner to build directly into the action's path (e.g., cd ${{ github.action_path }} && go build -o codereview ./cmd/codereview) and then run $(github.action_path)/codereview. This avoids potential issues if $RUNNER_TEMP has unexpected contents or permissions, although less likely with GitHub-managed runners.
| Repository struct { | ||
| Owner struct { | ||
| Login string `json:"login"` | ||
| } `json:"owner"` |
There was a problem hiding this comment.
thought: The githubEvent struct is quite large and includes fields that are directly mapped to Config or CommentContext. This duplication is handled manually. It's functional, but worth noting the manual mapping might become verbose if event structures grow significantly.
|
|
||
| i++ | ||
| } | ||
|
|
There was a problem hiding this comment.
thought: The loop j <= start+4 in resolvePathFromHeaders assumes the relevant headers (like +++) will always appear within the first 4 lines after the diff --git line. This is typically true for standard diff formats, but in very unusual cases, it might miss a +++ line if there are many other headers first. It's a pragmatic choice, not necessarily an 'issue'.
internal/github/client.go
Outdated
|
|
||
| page := 1 | ||
| for { | ||
| url := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/comments?per_page=100&page=%d", apiBase, owner, repo, prNumber, page) |
There was a problem hiding this comment.
issue: In FetchCommentThread, the resp.Body.Close() is called after io.ReadAll(resp.Body). If io.ReadAll returns an error, resp.Body.Close() will still be called (deferred). However, the body variable ([]byte) might not contain valid data for the subsequent json.Unmarshal. If io.ReadAll fails, body would be nil or partial, and json.Unmarshal would fail, but the error message would be misleading. It's safer to check err after io.ReadAll before attempting to unmarshal.
internal/prompt/prompt.go
Outdated
| - If the developer's response resolves your concern, acknowledge it | ||
| - If you still have concerns, explain why clearly | ||
| - Be collaborative, not adversarial | ||
| - Output plain text only, no JSON, no markdown fences` |
There was a problem hiding this comment.
nit: The prompt rule Output ONLY the JSON object, no markdown fences, no extra text is given. However, review.stripMarkdownFences exists, implying the LLM might still output fences. While the stripping acts as a safeguard, it might be worth refining the prompt to reinforce strict adherence, or acknowledge that the stripping is a necessary robustness measure.
| for _, f := range files { | ||
| lines := make(map[int]bool) | ||
| for _, h := range f.Hunks { | ||
| for _, l := range h.Lines { |
There was a problem hiding this comment.
suggestion: Logging warnings to os.Stderr is appropriate, but consider using a structured logger (e.g., log/slog or an external library) for more consistent and potentially more useful output, especially in CI environments where these warnings might be parsed by other tools. For a simple action, fmt.Fprintf is acceptable.
On synchronize events, fetch the bot's existing review comments and pass them to the LLM as context so it doesn't repeat itself. Also refactors duplicated struct definitions in the GitHub client.
There was a problem hiding this comment.
The PR introduces a new 'reply' mode and significant refactoring to support it, which is well-structured. However, several previously identified issues and suggestions remain unaddressed, most notably the critical bug related to inaccurate diff hunk header reconstruction for the LLM.
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: ./ |
There was a problem hiding this comment.
suggestion: The runs-on: macos-latest is still present. As noted previously, ubuntu-latest is generally more cost-effective and often faster for Go applications unless there's a specific macOS requirement not apparent here.
| types: [opened, synchronize] | ||
| pull_request_review_comment: | ||
| types: [created] | ||
|
|
There was a problem hiding this comment.
praise: The addition of pull_request_review_comment: types: [created] to the on trigger is crucial for the new reply functionality and is well-implemented.
| @@ -4,7 +4,7 @@ | |||
| *.dll | |||
| *.so | |||
There was a problem hiding this comment.
nit: Changing codereview to /codereview makes the exclusion more specific to the root binary, which is a good minor improvement.
| @@ -0,0 +1,21 @@ | |||
| { | |||
There was a problem hiding this comment.
praise: This new .projections.json file provides useful configuration for development environments and tooling, improving the developer experience.
| go-version: '1.24' | ||
| cache-dependency-path: go.sum | ||
|
|
||
| - name: Build |
There was a problem hiding this comment.
nit: The go-version: '1.24' remains. While 1.24 is fine, it's generally safer to pin to a specific patch version (e.g., 1.22.x) or just use 1 for stability, although 1.24 isn't a stable release yet, so it's less critical here.
|
|
||
| Rules: | ||
| - Be concise and directly address the question or comment | ||
| - Reference the code when relevant |
There was a problem hiding this comment.
praise: The inclusion of prior comments in the user prompt is a significant improvement, helping the LLM avoid repetition and focus on new feedback or changes addressing previous comments.
| switch l.Kind { | ||
| case diff.KindAdded: | ||
| fmt.Fprintf(&user, "+%s\n", l.Content) | ||
| case diff.KindRemoved: |
There was a problem hiding this comment.
praise: The new replySystemTemplate is clear and concise, providing excellent guidance for the LLM when generating replies within a conversation thread.
| return provider.Response{}, fmt.Errorf("calling Gemini API: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
suggestion: Similar to the GitHub client, using http.DefaultClient.Do(httpReq) means there's no timeout configured for Gemini API calls. Consider using a custom http.Client with a timeout to prevent requests from hanging indefinitely, improving the action's robustness.
| @@ -0,0 +1,14 @@ | |||
| package provider | |||
There was a problem hiding this comment.
praise: Introducing the provider interface and associated types (Request, Response, ReviewFunc) is a great abstraction, making the codebase more modular and easier to extend with other LLM providers in the future.
|
|
||
| result, err := ParseResponse(resp.Content) | ||
| if err != nil { | ||
| return Result{}, fmt.Errorf("parsing response: %w", err) |
There was a problem hiding this comment.
praise: The RunReply function correctly orchestrates the reply workflow, building the prompt, calling the provider, and processing the response.
On review, fetch the complete contents of each changed file so the LLM can understand surrounding code, not just the diff. Files over 256KB are skipped silently. Deleted files return empty and are also skipped.
| review: | ||
| runs-on: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
suggestion: The runs-on: macos-latest is still present. As noted previously, ubuntu-latest is generally more cost-effective and often faster for Go applications unless there's a specific macOS requirement not apparent here.
| *.so | ||
| *.dylib | ||
| codereview | ||
| /codereview |
There was a problem hiding this comment.
nit: Changing codereview to /codereview makes the exclusion more specific to the root binary, which is a good minor improvement.
| with: | ||
| go-version: '1.24' | ||
| cache-dependency-path: go.sum | ||
|
|
There was a problem hiding this comment.
nit: The go-version: '1.24' remains. While 1.24 is fine, it's generally safer to pin to a specific patch version (e.g., 1.22.x) or just use 1 for stability. As 1.24 is not a stable release yet, this is less critical, but still a general best practice.
|
|
||
| var event githubEvent | ||
| if err := json.Unmarshal(data, &event); err != nil { | ||
| return Config{}, fmt.Errorf("parsing event JSON: %w", err) |
There was a problem hiding this comment.
suggestion: This line if cfg.Provider == "" { cfg.Provider = "gemini" } still duplicates the default provider: 'gemini' already set in action.yml. It's generally better to maintain defaults in a single, authoritative place to avoid inconsistencies.
| } | ||
|
|
||
| type File struct { | ||
| Path string |
There was a problem hiding this comment.
issue: The diff.Hunk struct still only contains StartLine int, which represents the new start line. To accurately reconstruct a unified diff hunk header for the LLM (e.g., @@ -old_start_line,old_num_lines +new_start_line,new_num_lines @@), the Hunk struct must be extended to capture OldStartLine, OldNumLines, NewStartLine (which is current StartLine), and NewNumLines during parsing. This is critical for the LLM to precisely understand the diff context.
|
|
||
| func New(token string) *Client { | ||
| return &Client{ | ||
| token: token, |
There was a problem hiding this comment.
suggestion: The http.Client created in New still doesn't have a timeout. It's good practice to add a timeout (e.g., Timeout: 30 * time.Second) for robustness when making external network requests.
| req.Header.Set("Authorization", "Bearer "+c.token) | ||
| req.Header.Set("Accept", "application/vnd.github.v3+json") | ||
|
|
||
| resp, err := c.httpClient.Do(req) |
There was a problem hiding this comment.
issue: In fetchAllPRComments, resp.Body.Close() is called after io.ReadAll(resp.Body). If io.ReadAll returns an error, body might be partial or invalid, and json.Unmarshal would receive bad data. It's safer to defer resp.Body.Close() immediately after c.httpClient.Do(req) to ensure it's called reliably, and then check the error from io.ReadAll before attempting to unmarshal the body or check resp.StatusCode.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: This is directly related to the internal/diff/types.go issue. The hunk header is still being reconstructed incorrectly as @@ -%d +%d @@. A correct unified diff hunk header requires old/new start lines and number of lines (e.g., @@ -old_start,old_num +new_start,new_num @@). This inaccurate format will hinder the LLM's ability to understand the exact context of the changes, leading to less precise comments. This needs to be corrected by using the full hunk header information, which requires updates to the diff.Hunk struct and its parsing logic.
| return provider.Response{}, fmt.Errorf("calling Gemini API: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
suggestion: Similar to the GitHub client, using http.DefaultClient.Do(httpReq) means there's no timeout configured for Gemini API calls. Consider using a custom http.Client with a timeout to prevent requests from hanging indefinitely, improving the action's robustness.
The LLM now returns structured replies indicating whether the concern is resolved. If resolved and the thread was started by the bot, it calls the GitHub GraphQL API to resolve the thread automatically.
| review: | ||
| runs-on: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
suggestion: The runs-on: macos-latest is still present. As noted previously, ubuntu-latest is generally more cost-effective and often faster for Go applications unless there's a specific macOS requirement not apparent here.
| with: | ||
| go-version: '1.24' | ||
| cache-dependency-path: go.sum | ||
|
|
There was a problem hiding this comment.
nit: The go-version: '1.24' remains. While 1.24 is fine, it's generally safer to pin to a specific patch version (e.g., 1.22.x) or just use 1 for stability. As 1.24 is not a stable release yet, this is less critical, but still a general best practice.
|
|
||
| if cfg.Provider == "" { | ||
| cfg.Provider = "gemini" | ||
| } |
There was a problem hiding this comment.
suggestion: This line if cfg.Provider == "" { cfg.Provider = "gemini" } still duplicates the default provider: 'gemini' already set in action.yml. It's generally better to maintain defaults in a single, authoritative place to avoid inconsistencies.
| StartLine int | ||
| Lines []Line | ||
| } | ||
|
|
There was a problem hiding this comment.
issue: This is a re-highlight of a critical, unaddressed issue. The diff.Hunk struct still only contains StartLine int, which represents the new start line. To accurately reconstruct a unified diff hunk header for the LLM (e.g., @@ -old_start_line,old_num_lines +new_start_line,new_num_lines @@), the Hunk struct must be extended to capture OldStartLine, OldNumLines, NewStartLine (which is current StartLine), and NewNumLines during parsing. This is critical for the LLM to precisely understand the diff context.
| return &Client{ | ||
| token: token, | ||
| httpClient: &http.Client{}, | ||
| } |
There was a problem hiding this comment.
suggestion: The http.Client created in New still doesn't have a timeout. It's good practice to add a timeout (e.g., Timeout: 30 * time.Second) for robustness when making external network requests.
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
issue: This is a re-highlight of a previously identified problem. In fetchAllPRComments, resp.Body.Close() is called after io.ReadAll(resp.Body). If io.ReadAll returns an error (e.g., due to network issues while reading a large body), body might be partial or invalid, and json.Unmarshal would receive bad data. It's safer to defer resp.Body.Close() immediately after c.httpClient.Do(req) to ensure it's called reliably, and then check the error from io.ReadAll before attempting to unmarshal the body or check resp.StatusCode.
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return provider.Response{}, fmt.Errorf("Gemini API returned status %d: %s", resp.StatusCode, string(respBody)) | ||
| } |
There was a problem hiding this comment.
suggestion: Similar to the GitHub client, using http.DefaultClient.Do(httpReq) means there's no timeout configured for Gemini API calls. Consider using a custom http.Client with a timeout to prevent requests from hanging indefinitely, improving the action's robustness.
1 participant
Summary
Test plan