The splitDiffIntoChunks implementation drops the newline that separates two files when it splits on "\ndiff --git ". After the split the code re‑adds the prefix with "diff --git " + part, which loses the leading line‑break. When the chunks are concatenated the original diff is no longer identical, causing the TestSplitDiffIntoChunks_LargeSingleFile test to fail. Fix by adding the missing newline, e.g. seg = "\n" + "diff --git " + part (or keep the delimiter when splitting with strings.SplitAfter). (Generated by: OpenAI (openai/gpt-oss-120b))
The `splitDiffIntoChunks` implementation drops the newline that separates two files when it splits on `"\ndiff --git "`. After the split the code re‑adds the prefix with `"diff --git " + part`, which loses the leading line‑break. When the chunks are concatenated the original diff is no longer identical, causing the `TestSplitDiffIntoChunks_LargeSingleFile` test to fail. Fix by adding the missing newline, e.g. `seg = "\n" + "diff --git " + part` (or keep the delimiter when splitting with `strings.SplitAfter`). (Generated by: OpenAI (openai/gpt-oss-120b))
The test TestSplitDiffIntoChunks_LargeSingleFile relies on an exact string equality after re‑joining the chunks. Because of the newline‑loss bug described above the test will currently fail. Once the split function preserves delimiters, the test will pass; otherwise consider comparing the recombined diff with the original using cmp.Diff only (as you already do) rather than a strict equality check. (Generated by: OpenAI (openai/gpt-oss-120b))
The test `TestSplitDiffIntoChunks_LargeSingleFile` relies on an exact string equality after re‑joining the chunks. Because of the newline‑loss bug described above the test will currently fail. Once the split function preserves delimiters, the test will pass; otherwise consider comparing the recombined diff with the original using `cmp.Diff` only (as you already do) rather than a strict equality check. (Generated by: OpenAI (openai/gpt-oss-120b))
You added github.com/google/go-cmp v0.7.0 as a direct dependency – good for the new tests. Remember to run go mod tidy so that the go.sum is updated accordingly. (Generated by: OpenAI (openai/gpt-oss-120b))
You added `github.com/google/go-cmp v0.7.0` as a direct dependency – good for the new tests. Remember to run `go mod tidy` so that the `go.sum` is updated accordingly. (Generated by: OpenAI (openai/gpt-oss-120b))
New now takes the extra parameters maxChunkSize and guidelines. Ensure all call‑sites (currently only cmd/pierre/main.go) are updated accordingly. Consider adding a comment that documents the meaning of maxChunkSize (bytes) and that a non‑positive value triggers the built‑in default. (Generated by: OpenAI (openai/gpt-oss-120b))
`New` now takes the extra parameters `maxChunkSize` and `guidelines`. Ensure all call‑sites (currently only `cmd/pierre/main.go`) are updated accordingly. Consider adding a comment that documents the meaning of `maxChunkSize` (bytes) and that a non‑positive value triggers the built‑in default. (Generated by: OpenAI (openai/gpt-oss-120b))
The comment that builds baseSystem concatenates the dynamically created guidelinesText directly inside a raw string literal. This makes the final prompt contain a leading tab on every line, which the LLM will treat as part of the instruction. Consider using a plain string without the leading indentation or run strings.TrimSpace on the final prompt. (Generated by: OpenAI (openai/gpt-oss-120b))
The comment that builds `baseSystem` concatenates the dynamically created `guidelinesText` directly inside a raw string literal. This makes the final prompt contain a leading tab on every line, which the LLM will treat as part of the instruction. Consider using a plain string without the leading indentation or run `strings.TrimSpace` on the final prompt. (Generated by: OpenAI (openai/gpt-oss-120b))
splitDiffIntoChunks may split a diff in the middle of a line (e.g. when a single line exceeds maxSize). Breaking a diff line arbitrarily can corrupt the unified‑diff syntax and confuse the model. Prefer to split only on line boundaries – e.g. split the input with strings.SplitAfter(diff, "\n") and then build chunks while staying under the size limit. (Generated by: OpenAI (openai/gpt-oss-120b))
`splitDiffIntoChunks` may split a diff in the middle of a line (e.g. when a single line exceeds `maxSize`). Breaking a diff line arbitrarily can corrupt the unified‑diff syntax and confuse the model. Prefer to split only on line boundaries – e.g. split the input with `strings.SplitAfter(diff, "\n")` and then build chunks while staying under the size limit. (Generated by: OpenAI (openai/gpt-oss-120b))
cfg.Review.MaxChunkChars is an int that may be zero if the user omits the flag. The service correctly falls back to the default (60000) inside judgePR, but you could also enforce the default earlier (e.g. in the config struct default tag or after flag parsing) to avoid passing a zero value downstream. (Generated by: OpenAI (openai/gpt-oss-120b))
`cfg.Review.MaxChunkChars` is an `int` that may be zero if the user omits the flag. The service correctly falls back to the default (60000) inside `judgePR`, but you could also enforce the default earlier (e.g. in the config struct default tag or after flag parsing) to avoid passing a zero value downstream. (Generated by: OpenAI (openai/gpt-oss-120b))
The condition for posting comments is inverted; it adds comments when disableComments is true. Change if s.disableComments { to if !s.disableComments {. (Reason: The code adds VCS comments only when s.disableComments is true, which contradicts the flag's purpose; flipping the condition fixes the logic.) (Generated by: OpenAI (openai/gpt-oss-120b))
The condition for posting comments is inverted; it adds comments when `disableComments` is true. Change `if s.disableComments {` to `if !s.disableComments {`. (Reason: The code adds VCS comments only when `s.disableComments` is true, which contradicts the flag's purpose; flipping the condition fixes the logic.) (Generated by: OpenAI (openai/gpt-oss-120b))
Update the comment above the block to reflect that comments are posted only when not in dry‑run mode. (Reason: The comment correctly identifies that the existing comment is misleading—the code posts comments only when disableComments (dry‑run) is true, so the comment should be updated to reflect posting occurs when not in dry‑run mode.) (Generated by: OpenAI (openai/gpt-oss-120b))
Update the comment above the block to reflect that comments are posted only when not in dry‑run mode. (Reason: The comment correctly identifies that the existing comment is misleading—the code posts comments only when `disableComments` (dry‑run) is true, so the comment should be updated to reflect posting occurs when not in dry‑run mode.) (Generated by: OpenAI (openai/gpt-oss-120b))
File: cmd/pierre/main.go, Line: 102
The logic block initiating var ai pierre.ChatAdapter uses a hardcoded boolean return in the error handling if 'err' is nil (lines 116-118). While safe, this pattern can be clearer by using guard clauses (e.g., if err != nil { log.Fatalf(...) }) throughout the initialization phase.
File: cmd/pierre/main.go, Line: 102
The logic block initiating `var ai pierre.ChatAdapter` uses a hardcoded boolean return in the error handling if 'err' is nil (lines 116-118). While safe, this pattern can be clearer by using guard clauses (e.g., `if err != nil { log.Fatalf(...) }`) throughout the initialization phase.
File: internal/pierre/resource.go, Line: 23
The signature for GetDiff should ideally also include a way to limit the size or scope of the diff content passed (e.g., file paths filtered by modified files) to prevent overwhelming memory if PR diffs are huge.
File: internal/pierre/resource.go, Line: 23
The signature for `GetDiff` should ideally also include a way to limit the size or scope of the diff content passed (e.g., file paths filtered by modified files) to prevent overwhelming memory if PR diffs are huge.
File: internal/pierre/judge.go, Line: 21
The function reads the entire diff into memory (io.ReadAll(diff)). Consider using a configurable limit or streaming approach if reviewing extremely large pulls (e.g., multi-megabyte diffs) to avoid potential OOM errors and timeouts.
File: internal/pierre/judge.go, Line: 21
The function reads the entire diff into memory (`io.ReadAll(diff)`). Consider using a configurable limit or streaming approach if reviewing extremely large pulls (e.g., multi-megabyte diffs) to avoid potential OOM errors and timeouts.
File: internal/chatter/gemini.go, Line: 156
In mapRoleToGemini, System role mapping should handle the fact that Gemini's chat history primarily uses 'user' and 'model', not a distinct system role for context injection.
File: internal/chatter/gemini.go, Line: 156
In `mapRoleToGemini`, System role mapping should handle the fact that Gemini's chat history primarily uses 'user' and 'model', not a distinct system role for context injection.
File: internal/chatter/gemini.go, Line: 143
The schemaFromType function heavily relies on reflection (reflect) and is complex; adding documentation or a dedicated unit test suite for this schema generation logic would greatly improve maintainability.
File: internal/chatter/gemini.go, Line: 143
The `schemaFromType` function heavily relies on reflection (`reflect`) and is complex; adding documentation or a dedicated unit test suite for this schema generation logic would greatly improve maintainability.
File: internal/chatter/ollama.go, Line: 32
The initialization function NewOllamaAdapter uses api.ClientFromEnvironment() which assumes a default local endpoint (typically http://localhost:11434). Making the base URL configurable here would improve deployment flexibility.
File: internal/chatter/ollama.go, Line: 32
The initialization function `NewOllamaAdapter` uses `api.ClientFromEnvironment()` which assumes a default local endpoint (typically http://localhost:11434). Making the base URL configurable here would improve deployment flexibility.
File: internal/chatter/ollama.go, Line: 90
The dependency on fmt.Print within the streaming callback function is side-effect based and makes testing difficult. If this were a library component, it would be better to buffer or collect the output stream explicitly.
File: internal/chatter/ollama.go, Line: 90
The dependency on `fmt.Print` within the streaming callback function is side-effect based and makes testing difficult. If this were a library component, it would be better to buffer or collect the output stream explicitly.
File: internal/chatter/openai.go, Line: 32
The configuration block has repeated logic for setting the BaseURL (lines 32-33 and 34-35). Consolidating this or adding a single check would clean up the adapter initialization.
File: internal/chatter/openai.go, Line: 32
The configuration block has repeated logic for setting the BaseURL (lines 32-33 and 34-35). Consolidating this or adding a single check would clean up the adapter initialization.
File: internal/chatter/openai.go, Line: 125
The manual unwrapping of slice JSON responses due to OpenAI's structured output format is highly fragile (lines 124-132). This pattern should be isolated and robustly tested for all possible data types that might appear wrapped in items.
File: internal/chatter/openai.go, Line: 125
The manual unwrapping of slice JSON responses due to OpenAI's structured output format is highly fragile (lines 124-132). This pattern should be isolated and robustly tested for all possible data types that might appear wrapped in items.
File: /tmp/pr-review-2253999896/internal/chatter/openai.go, Line: 25
Using InsecureSkipVerify: true bypasses certificate validation and introduces a security risk; this should only be used for development and replaced with proper certificate handling in production.
File: /tmp/pr-review-2253999896/internal/chatter/openai.go, Line: 25
Using `InsecureSkipVerify: true` bypasses certificate validation and introduces a security risk; this should only be used for development and replaced with proper certificate handling in production.
File: /tmp/pr-review-2253999896/internal/pierre/review.go, Line: 34
The logic for adding comments uses log.Printf and continues execution even if an API call to add the comment fails, potentially leading to missed review feedback without raising a fatal error.
File: /tmp/pr-review-2253999896/internal/pierre/review.go, Line: 34
The logic for adding comments uses `log.Printf` and continues execution even if an API call to add the comment fails, potentially leading to missed review feedback without raising a fatal error.
File: /tmp/pr-review-2253999896/internal/chatter/gemini.go, Line: 150
The role mapping function mapRoleToGemini defaults to 'user' for any custom or unhandled chat role, which could hide logical bugs if the conversation history includes non-standard roles.
File: /tmp/pr-review-2253999896/internal/chatter/gemini.go, Line: 150
The role mapping function `mapRoleToGemini` defaults to 'user' for any custom or unhandled chat role, which could hide logical bugs if the conversation history includes non-standard roles.
File: /tmp/pr-review-2253999896/internal/gitadapters/bitbucket/controller.go, Line: 10
The function GetDiff should ensure that response.Body is closed regardless of whether the status code check passes or fails to prevent resource leaks.
File: /tmp/pr-review-2253999896/internal/gitadapters/bitbucket/controller.go, Line: 10
The function `GetDiff` should ensure that `response.Body` is closed regardless of whether the status code check passes or fails to prevent resource leaks.
File: /tmp/pr-review-2253999896/internal/pierre/resource.go, Line: 11
The signature for GetDiff returns io.ReadCloser, which forces the caller to explicitly close it (as seen in review.go:12). Consider accepting a function (e.g., func(ctx) (io.ReadCloser, error)) if resource management across packages is complex.
File: /tmp/pr-review-2253999896/internal/pierre/resource.go, Line: 11
The signature for `GetDiff` returns `io.ReadCloser`, which forces the caller to explicitly close it (as seen in `review.go:12`). Consider accepting a function (e.g., `func(ctx) (io.ReadCloser, error)`) if resource management across packages is complex.
Reading the entire diff into memory using io.ReadAll(diff) can cause excessive memory usage if a pull request has a very large diff; consider streaming or limiting the read size.
Reading the entire diff into memory using io.ReadAll(diff) can cause excessive memory usage if a pull request has a very large diff; consider streaming or limiting the read size.
Using InsecureSkipVerify: true bypasses crucial SSL certificate validation, opening a potential security vulnerability (Man-in-the-Middle attacks) if the baseURL is not fully trusted.
Using InsecureSkipVerify: true bypasses crucial SSL certificate validation, opening a potential security vulnerability (Man-in-the-Middle attacks) if the baseURL is not fully trusted.
Ignoring the potential error return from strings.CutSuffix might mask issues if the provided bitbucket BaseURL is malformed or does not match expected suffix patterns.
Ignoring the potential error return from strings.CutSuffix might mask issues if the provided bitbucket BaseURL is malformed or does not match expected suffix patterns.
Using fmt.Print inside the chat response function pollutes standard output with side effects and mixes concerns between data processing and logging, making the code harder to maintain.
Using fmt.Print inside the chat response function pollutes standard output with side effects and mixes concerns between data processing and logging, making the code harder to maintain.
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
@@ -40,0 +103,4 @@for j, h := range hunks {hseg := hif j != 0 {hseg = "@@ " + hThe
splitDiffIntoChunksimplementation drops the newline that separates two files when it splits on"\ndiff --git ". After the split the code re‑adds the prefix with"diff --git " + part, which loses the leading line‑break. When the chunks are concatenated the original diff is no longer identical, causing theTestSplitDiffIntoChunks_LargeSingleFiletest to fail. Fix by adding the missing newline, e.g.seg = "\n" + "diff --git " + part(or keep the delimiter when splitting withstrings.SplitAfter). (Generated by: OpenAI (openai/gpt-oss-120b))@@ -0,0 +30,4 @@if !strings.HasPrefix(chunks[0], "diff --git a/file1.txt") {t.Fatalf("first chunk does not contain file1 header: %s", chunks[0])}if !strings.HasPrefix(chunks[1], "diff --git a/file2.txt") {The test
TestSplitDiffIntoChunks_LargeSingleFilerelies on an exact string equality after re‑joining the chunks. Because of the newline‑loss bug described above the test will currently fail. Once the split function preserves delimiters, the test will pass; otherwise consider comparing the recombined diff with the original usingcmp.Diffonly (as you already do) rather than a strict equality check. (Generated by: OpenAI (openai/gpt-oss-120b))@@ -7,7 +7,9 @@ require (github.com/alecthomas/kong v1.14.0github.com/alecthomas/kong-yaml v0.2.0github.com/google/generative-ai-go v0.20.1You added
github.com/google/go-cmp v0.7.0as a direct dependency – good for the new tests. Remember to rungo mod tidyso that thego.sumis updated accordingly. (Generated by: OpenAI (openai/gpt-oss-120b))The error message now correctly uses
response.StatusCode. No further changes needed. (Generated by: OpenAI (openai/gpt-oss-120b))@@ -13,0 +11,4 @@maxChunkSize intguidelines []stringgit GitAdapterchat ChatAdapterNewnow takes the extra parametersmaxChunkSizeandguidelines. Ensure all call‑sites (currently onlycmd/pierre/main.go) are updated accordingly. Consider adding a comment that documents the meaning ofmaxChunkSize(bytes) and that a non‑positive value triggers the built‑in default. (Generated by: OpenAI (openai/gpt-oss-120b))@@ -40,0 +80,4 @@// It tries to split on file boundaries ("diff --git") first, then on hunk boundaries (@@),// and finally on a hard byte limit.func splitDiffIntoChunks(diff []byte, maxSize int) []string {if len(diff) <= maxSize {The comment that builds
baseSystemconcatenates the dynamically createdguidelinesTextdirectly inside a raw string literal. This makes the final prompt contain a leading tab on every line, which the LLM will treat as part of the instruction. Consider using a plain string without the leading indentation or runstrings.TrimSpaceon the final prompt. (Generated by: OpenAI (openai/gpt-oss-120b))@@ -40,0 +121,4 @@}} else {current.WriteString(seg)}splitDiffIntoChunksmay split a diff in the middle of a line (e.g. when a single line exceedsmaxSize). Breaking a diff line arbitrarily can corrupt the unified‑diff syntax and confuse the model. Prefer to split only on line boundaries – e.g. split the input withstrings.SplitAfter(diff, "\n")and then build chunks while staying under the size limit. (Generated by: OpenAI (openai/gpt-oss-120b))cfg.Review.MaxChunkCharsis anintthat may be zero if the user omits the flag. The service correctly falls back to the default (60000) insidejudgePR, but you could also enforce the default earlier (e.g. in the config struct default tag or after flag parsing) to avoid passing a zero value downstream. (Generated by: OpenAI (openai/gpt-oss-120b))985fac347cto61d538d4a561d538d4a5toac5ff7aeebac5ff7aeebtocc321be6582bde5dff47tob5912d0725b5912d0725to3b709bc48e@@ -23,0 +26,4 @@headSHA, err := s.git.GetPRHeadSHA(ctx, organisation, repo, prID)if err != nil {log.Printf("warning: could not fetch PR head SHA (%v); skipping sanity check", err)} else {The condition for posting comments is inverted; it adds comments when
disableCommentsis true. Changeif s.disableComments {toif !s.disableComments {. (Reason: The code adds VCS comments only whens.disableCommentsis true, which contradicts the flag's purpose; flipping the condition fixes the logic.) (Generated by: OpenAI (openai/gpt-oss-120b))@@ -23,0 +30,4 @@filtered := []Comment{}for _, c := range comments {// Retrieve full file content at the PR headfileContent, fErr := s.git.GetFileContent(ctx, organisation, repo, c.File, headSHA)Update the comment above the block to reflect that comments are posted only when not in dry‑run mode. (Reason: The comment correctly identifies that the existing comment is misleading—the code posts comments only when
disableComments(dry‑run) is true, so the comment should be updated to reflect posting occurs when not in dry‑run mode.) (Generated by: OpenAI (openai/gpt-oss-120b))3b709bc48eto1ba0e879f91ba0e879f9to2e12c397869f6c6830a6toef55295c4def55295c4dto75ef8da1a4File: README.md, Line: 1
Test comment from worker
File: cmd/pierre/main.go, Line: 102
The logic block initiating
var ai pierre.ChatAdapteruses a hardcoded boolean return in the error handling if 'err' is nil (lines 116-118). While safe, this pattern can be clearer by using guard clauses (e.g.,if err != nil { log.Fatalf(...) }) throughout the initialization phase.File: internal/pierre/resource.go, Line: 23
The signature for
GetDiffshould ideally also include a way to limit the size or scope of the diff content passed (e.g., file paths filtered by modified files) to prevent overwhelming memory if PR diffs are huge.File: internal/pierre/judge.go, Line: 21
The function reads the entire diff into memory (
io.ReadAll(diff)). Consider using a configurable limit or streaming approach if reviewing extremely large pulls (e.g., multi-megabyte diffs) to avoid potential OOM errors and timeouts.File: internal/chatter/gemini.go, Line: 156
In
mapRoleToGemini, System role mapping should handle the fact that Gemini's chat history primarily uses 'user' and 'model', not a distinct system role for context injection.File: internal/chatter/gemini.go, Line: 143
The
schemaFromTypefunction heavily relies on reflection (reflect) and is complex; adding documentation or a dedicated unit test suite for this schema generation logic would greatly improve maintainability.File: internal/chatter/ollama.go, Line: 32
The initialization function
NewOllamaAdapterusesapi.ClientFromEnvironment()which assumes a default local endpoint (typically http://localhost:11434). Making the base URL configurable here would improve deployment flexibility.File: internal/chatter/ollama.go, Line: 90
The dependency on
fmt.Printwithin the streaming callback function is side-effect based and makes testing difficult. If this were a library component, it would be better to buffer or collect the output stream explicitly.File: internal/chatter/openai.go, Line: 32
The configuration block has repeated logic for setting the BaseURL (lines 32-33 and 34-35). Consolidating this or adding a single check would clean up the adapter initialization.
File: internal/chatter/openai.go, Line: 125
The manual unwrapping of slice JSON responses due to OpenAI's structured output format is highly fragile (lines 124-132). This pattern should be isolated and robustly tested for all possible data types that might appear wrapped in items.
Test review from pierre-bot test
File: /tmp/pr-review-2253999896/internal/chatter/openai.go, Line: 25
Using
InsecureSkipVerify: truebypasses certificate validation and introduces a security risk; this should only be used for development and replaced with proper certificate handling in production.File: /tmp/pr-review-2253999896/internal/pierre/review.go, Line: 34
The logic for adding comments uses
log.Printfand continues execution even if an API call to add the comment fails, potentially leading to missed review feedback without raising a fatal error.File: /tmp/pr-review-2253999896/internal/chatter/gemini.go, Line: 150
The role mapping function
mapRoleToGeminidefaults to 'user' for any custom or unhandled chat role, which could hide logical bugs if the conversation history includes non-standard roles.File: /tmp/pr-review-2253999896/internal/gitadapters/bitbucket/controller.go, Line: 10
The function
GetDiffshould ensure thatresponse.Bodyis closed regardless of whether the status code check passes or fails to prevent resource leaks.File: /tmp/pr-review-2253999896/internal/pierre/resource.go, Line: 11
The signature for
GetDiffreturnsio.ReadCloser, which forces the caller to explicitly close it (as seen inreview.go:12). Consider accepting a function (e.g.,func(ctx) (io.ReadCloser, error)) if resource management across packages is complex.Test comment from worker
Test comment from worker
Reading the entire diff into memory using io.ReadAll(diff) can cause excessive memory usage if a pull request has a very large diff; consider streaming or limiting the read size.
Test comment from worker
Test comment from worker
Using InsecureSkipVerify: true bypasses crucial SSL certificate validation, opening a potential security vulnerability (Man-in-the-Middle attacks) if the baseURL is not fully trusted.
Ignoring the potential error return from strings.CutSuffix might mask issues if the provided bitbucket BaseURL is malformed or does not match expected suffix patterns.
Using fmt.Print inside the chat response function pollutes standard output with side effects and mixes concerns between data processing and logging, making the code harder to maintain.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.