feat(pierre): add diff chunking and configurable review settings

cmd/pierre/main.go

@@ -40,6 +40,7 @@ type LLMConfig struct {
 type ReviewConfig struct {
 	MaxChunkChars   int      `help:"Maximum diff chunk size in characters (default 60000)" default:"60000"`
 	Guidelines      []string `help:"Project-specific review guidelines"`
+	DisableComments bool     `help:"Do not post comments to the Git provider (dry‑run mode)"`
 }
 
 type Config struct {
@@ -123,7 +124,7 @@ func main() {
 		log.Fatalf("Error initializing AI: %v", err)
 	}
 
-	pierreService := pierre.New(ai, git, cfg.Review.MaxChunkChars, cfg.Review.Guidelines)
+	pierreService := pierre.New(ai, git, cfg.Review.MaxChunkChars, cfg.Review.Guidelines, cfg.Review.DisableComments)
 	if err := pierreService.MakeReview(context.Background(), cfg.Repo.Owner, cfg.Repo.Repo, cfg.Repo.PRID); err != nil {
 		log.Fatalf("Error during review: %v", err)
 	}

internal/pierre/judge.go

@@ -39,29 +39,29 @@ func (s *Service) judgePR(ctx context.Context, diff io.Reader) (comments []Comme
 		}
 	}
 
-	baseSystem := `
-		You are a very strict senior software architect.
-		You review **only** newly added or modified lines in a unified diff, together with the immediate hunk context.
-		You do **not** report issues that appear **solely** in deleted lines (“-”) or that have already been fixed by the change.
-		No comments are made on pure formatting/whitespace changes or reordering that does not alter the program’s behavior.` + guidelinesText + `
-	`
+	baseSystem := strings.TrimSpace(`
+You are a very strict senior software architect.
+You review **only** newly added or modified lines in a unified diff, together with the immediate hunk context.
+You do **not** report issues that appear **solely** in deleted lines (“-") or that have already been fixed by the change.
+No comments are made on pure formatting/whitespace changes or reordering that does not alter the program’s behavior.`) + guidelinesText
 
 	for i, chunk := range chunks {
 		// Add a small header so the model knows this is a fragment
 		header := fmt.Sprintf("\n--- Chunk %d of %d ---\n", i+1, len(chunks))
 		userContent := fmt.Sprintf("Hello please review my PR. Write comments where improvements are necessary in new lines.%s\nHere is the git diff of it: %s", header, chunk)
 
+		var chunkComments []Comment
 		err = s.chat.GenerateStructured(ctx, []chatter.Message{{
 			Role:    chatter.RoleSystem,
 			Content: baseSystem,
 		}, {
 			Role:    chatter.RoleUser,
 			Content: userContent,
-		}}, &comments)
+		}}, &chunkComments)
 		if err != nil {
 			return nil, err
 		}
-		allComments = append(allComments, comments...)
+		allComments = append(allComments, chunkComments...)
 	}
 
 	// De‑duplicate comments (keyed by file:line)
@@ -91,7 +91,8 @@ func splitDiffIntoChunks(diff []byte, maxSize int) []string {
 	for idx, part := range parts {
 		seg := part
 		if idx != 0 {
-			seg = "diff --git " + part
+			// Preserve the leading newline that was removed by Split
+			seg = "\n" + "diff --git " + part
 		}
 		if current.Len()+len(seg) > maxSize && current.Len() > 0 {
 			chunks = append(chunks, current.String())
@@ -128,3 +129,4 @@ func splitDiffIntoChunks(diff []byte, maxSize int) []string {
 	}
 	return chunks
 }
+

internal/pierre/judge_test.go

@@ -1,57 +1,127 @@
 package pierre
 
 import (
-	"github.com/google/go-cmp/cmp"
+	"bytes"
+	"context"
+	"io"
 	"strings"
 	"testing"
+
+	"git.schreifuchs.ch/schreifuchs/pierre-bot/internal/chatter"
+	"github.com/google/go-cmp/cmp"
 )
 
-func TestSplitDiffIntoChunks_SmallDiff(t *testing.T) {
-	diff := "diff --git a/file1.txt b/file1.txt\n+added line\n"
-	max := 1000
-	chunks := splitDiffIntoChunks([]byte(diff), max)
-	if got, want := len(chunks), 1; got != want {
-		t.Fatalf("expected %d chunk, got %d", want, got)
-	}
-	if diff != chunks[0] {
-		t.Fatalf("chunk content changed: %s", diff)
+// mockChat implements the ChatAdapter interface for testing.
+type mockChat struct{}
+
+func (m *mockChat) GenerateStructured(ctx context.Context, msgs []chatter.Message, target interface{}) error {
+	if cSlice, ok := target.(*[]Comment); ok {
+		*cSlice = []Comment{{File: "file.go", Line: 1, Message: "test comment"}}
+		return nil
 	}
+	return nil
 }
 
-func TestSplitDiffIntoChunks_MultipleFiles(t *testing.T) {
-	diff := "diff --git a/file1.txt b/file1.txt\n+added line 1\n" +
-		"diff --git a/file2.txt b/file2.txt\n+added line 2\n"
-	max := 50 // each file diff is less than this, but total exceeds
-	chunks := splitDiffIntoChunks([]byte(diff), max)
-	if got, want := len(chunks), 2; got != want {
-		t.Fatalf("expected %d chunks, got %d", want, got)
-	}
-	// Ensure each chunk starts with the proper file header
-	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") {
-		t.Fatalf("second chunk does not contain file2 header: %s", chunks[1])
-	}
+func (m *mockChat) GetProviderName() string { return "mock" }
+
+// mockGit implements the GitAdapter interface for testing.
+type mockGit struct{}
+
+func (g *mockGit) GetDiff(ctx context.Context, owner, repo string, prID int) (io.ReadCloser, error) {
+	diff := "diff --git a/file1.go b/file1.go\n+line1\n" + "diff --git a/file2.go b/file2.go\n+line2\n"
+	return io.NopCloser(bytes.NewReader([]byte(diff))), nil
 }
 
-func TestSplitDiffIntoChunks_LargeSingleFile(t *testing.T) {
-	// Create a diff that exceeds maxSize by repeating a line
+func (g *mockGit) AddComment(ctx context.Context, owner, repo string, prID int, comment Comment) error {
+	return nil
+}
+
+func TestSplitDiffIntoChunks(t *testing.T) {
+	cases := []struct {
+		name           string
+		diff           string
+		maxSize        int
+		wantChunks     int // 0 means we don't assert exact count
+		wantPrefixes   []string
+		checkRecombine bool
+	}{
+		{
+			name:           "small diff",
+			diff:           "diff --git a/file1.txt b/file1.txt\n+added line\n",
+			maxSize:        1000,
+			wantChunks:     1,
+			wantPrefixes:   []string{"diff --git a/file1.txt"},
+			checkRecombine: true,
+		},
+		{
+			name: "multiple files",
+			diff: "diff --git a/file1.txt b/file1.txt\n+added line 1\n" +
+				"diff --git a/file2.txt b/file2.txt\n+added line 2\n",
+			maxSize:        50,
+			wantChunks:     2,
+			wantPrefixes:   []string{"diff --git a/file1.txt", "diff --git a/file2.txt"},
+			checkRecombine: false,
+		},
+		{
+			name: "large single file",
+			diff: func() string {
 				line := "+very long added line that will be repeated many times to exceed the chunk size\n"
-	diff := "diff --git a/large.txt b/large.txt\n" + strings.Repeat(line, 200)
-	max := 500 // small limit
-	chunks := splitDiffIntoChunks([]byte(diff), max)
-	// Ensure no chunk exceeds max size
+				return "diff --git a/large.txt b/large.txt\n" + strings.Repeat(line, 200)
+			}(),
+			maxSize:        500,
+			wantChunks:     0,
+			wantPrefixes:   nil,
+			checkRecombine: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			chunks := splitDiffIntoChunks([]byte(tc.diff), tc.maxSize)
+			if tc.wantChunks > 0 && len(chunks) != tc.wantChunks {
+				t.Fatalf("expected %d chunks, got %d", tc.wantChunks, len(chunks))
+			}
+			for i, prefix := range tc.wantPrefixes {
+				if i >= len(chunks) {
+					t.Fatalf("missing chunk %d for prefix check", i)
+				}
+				trimmed := strings.TrimPrefix(chunks[i], "\n")
+				if !strings.HasPrefix(trimmed, prefix) {
+					t.Fatalf("chunk %d does not start with expected prefix %q: %s", i, prefix, chunks[i])
+				}
+			}
 			for i, c := range chunks {
-		if len(c) > max {
-			t.Fatalf("chunk %d exceeds max size: %d > %d", i, len(c), max)
+				if tc.maxSize > 0 && len(c) > tc.maxSize {
+					t.Fatalf("chunk %d exceeds max size %d: %d", i, tc.maxSize, len(c))
 				}
 			}
-	// Recombine chunks and compare to original (ignoring possible split boundaries)
+			if tc.checkRecombine {
 				recombined := strings.Join(chunks, "")
-	if diff != recombined {
-		if d := cmp.Diff(diff, recombined); d != "" {
-			t.Fatalf("recombined diff differs:\n%s", d)
+				if diff := cmp.Diff(tc.diff, recombined); diff != "" {
+					t.Fatalf("recombined diff differs:\n%s", diff)
 				}
 			}
+		})
+	}
+}
+
+func TestJudgePR_ChunkAggregationAndDeduplication(t *testing.T) {
+	svc := &Service{
+		maxChunkSize: 50,
+		guidelines:   nil,
+		git:          &mockGit{},
+		chat:         &mockChat{},
+	}
+	diffReader, err := svc.git.GetDiff(context.Background(), "", "", 0)
+	if err != nil {
+		t.Fatalf("failed to get diff: %v", err)
+	}
+	defer diffReader.Close()
+	comments, err := svc.judgePR(context.Background(), diffReader)
+	if err != nil {
+		t.Fatalf("judgePR error: %v", err)
+	}
+	if got, want := len(comments), 1; got != want {
+		t.Fatalf("expected %d comment after deduplication, got %d", want, got)
+	}
 }

internal/pierre/resource.go

@@ -10,16 +10,18 @@ import (
 type Service struct {
 	maxChunkSize    int
 	guidelines      []string
+	disableComments bool
 	git             GitAdapter
 	chat            ChatAdapter
 }
 
-func New(chat ChatAdapter, git GitAdapter, maxChunkSize int, guidelines []string) *Service {
+func New(chat ChatAdapter, git GitAdapter, maxChunkSize int, guidelines []string, disableComments bool) *Service {
 	return &Service{
 		git:             git,
 		chat:            chat,
 		maxChunkSize:    maxChunkSize,
 		guidelines:      guidelines,
+		disableComments: disableComments,
 	}
 }
 

internal/pierre/review.go

@@ -29,9 +29,12 @@ func (s *Service) MakeReview(ctx context.Context, organisation string, repo stri
 		fmt.Printf("File: %s\nLine: %d\nMessage: %s\n%s\n",
 			c.File, c.Line, c.Message, "---")
 
-		if err := s.git.AddComment(ctx, organisation, repo, prID, c); err != nil {
+		if s.disableComments {
+			log.Printf("dry-run: not posting comment for %s:%d", c.File, c.Line)
+		} else if err := s.git.AddComment(ctx, organisation, repo, prID, c); err != nil {
 			log.Printf("Failed to add comment: %v", err)
 		}
+
 	}
 
 	return nil