feat: guidelines

AGENTS.md

@@ -6,20 +6,20 @@ layout: default
 
 > This document is consulted by autonomous coding agents (including the
 > Open‑Source `opencode` agent) when they read, modify, test, or lint the
-> code‑base.  It contains the canonical commands for building, testing, and
+> code‑base. It contains the canonical commands for building, testing, and
 > lint‑ing the project as well as a concise style guide that all Go code must
-> follow.  The file is deliberately kept around **150 lines** to be readable for
+> follow. The file is deliberately kept around **150 lines** to be readable for
 > both humans and LLMs.
 
 ---
 
 ## 1️⃣ Build / Run Commands
 
-| Command | Description |
-|--------|-------------|
-| `go build -o bin/pierre ./cmd/pierre/main.go` | Build the binary into `./bin/pierre`. |
-| `go run ./cmd/pierre/main.go --help` | Run the CLI with the help flag (fast sanity check). |
-| `./bin/pierre --version` | Verify the built binary reports its version. |
+| Command                                       | Description                                         |
+| --------------------------------------------- | --------------------------------------------------- |
+| `go build -o bin/pierre ./cmd/pierre/main.go` | Build the binary into `./bin/pierre`.               |
+| `go run ./cmd/pierre/main.go --help`          | Run the CLI with the help flag (fast sanity check). |
+| `./bin/pierre --version`                      | Verify the built binary reports its version.        |
 
 > **Tip for agents** – always prefer the `./bin/pierre` binary over invoking `go run` in CI or tests; it guarantees the same compilation flags.
 
@@ -29,13 +29,13 @@ layout: default
 
 The project uses the standard Go testing framework.
 
-| Command | Use case |
-|---------|----------|
-| `go test ./...` | Run **all** unit and integration tests. |
-| `go test ./... -run ^TestJudgePR$` | Run a **single** test (replace `TestJudgePR` with the desired name). |
-| `go test -cover ./...` | Run tests and emit a coverage report. |
-| `go test -run ^TestName$ -count=1 -v` | Verbose, non‑cached run for debugging a single test. |
-| `go test ./... -bench .` | Execute benchmarks (if any). |
+| Command                               | Use case                                                             |
+| ------------------------------------- | -------------------------------------------------------------------- |
+| `go test ./...`                       | Run **all** unit and integration tests.                              |
+| `go test ./... -run ^TestJudgePR$`    | Run a **single** test (replace `TestJudgePR` with the desired name). |
+| `go test -cover ./...`                | Run tests and emit a coverage report.                                |
+| `go test -run ^TestName$ -count=1 -v` | Verbose, non‑cached run for debugging a single test.                 |
+| `go test ./... -bench .`              | Execute benchmarks (if any).                                         |
 
 > **Agents should never** invoke `go test` with `-parallel` flags; the default parallelism is sufficient and ensures deterministic ordering for our table‑driven tests.
 
@@ -46,10 +46,10 @@ The project uses the standard Go testing framework.
 We rely on two lightweight built‑in tools plus optional `golangci-lint` when
 available.
 
-| Tool | Command | Description |
-|------|---------|-------------|
-| `go vet ./...` | `go vet ./...` | Detect obvious bugs, misuse of `fmt.Printf`, etc. |
-| `staticcheck ./...` | `staticcheck ./...` | Deeper static analysis (must be installed via `go install honnef.co/go/tools/cmd/staticcheck@latest`). |
+| Tool                       | Command             | Description                                                                                                                                                 |
+| -------------------------- | ------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `go vet ./...`             | `go vet ./...`      | Detect obvious bugs, misuse of `fmt.Printf`, etc.                                                                                                           |
+| `staticcheck ./...`        | `staticcheck ./...` | Deeper static analysis (must be installed via `go install honnef.co/go/tools/cmd/staticcheck@latest`).                                                      |
 | `golangci-lint` (optional) | `golangci-lint run` | Runs a configurable suite of linters. Install with `brew install golangci-lint` or `go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest`. |
 
 **Agent tip** – when `golangci-lint` is present, run it after `go vet` to catch style
@@ -60,66 +60,74 @@ use the same commands.
 
 ## 4️⃣ Code‑Style Guidelines (Go)
 
-All changes must satisfy the following conventions.  They are enforced by
-`go fmt`, `go vet`, and the optional `golangci-lint` config.
+All changes must satisfy the following conventions. They are enforced by
+`go fmt`, `go vet`, and the optional `golangci-lint` config. The code does not have
+to be backwards compatible.
 
 ### 4.1 Formatting & Imports
-* Run `go fmt ./...` before committing.  The repository uses **tab‑indented** Go code.
-* Imports are grouped in three blocks, separated by a blank line:
+
+- Run `go fmt ./...` before committing. The repository uses **tab‑indented** Go code.
+- Imports are grouped in three blocks, separated by a blank line:
   1. **Standard library**
   2. **External dependencies** (e.g., `github.com/alecthomas/kong`)
   3. **Internal packages** (e.g., `github.com/yourorg/pierre/internal/...`)
-* Within each block, imports are sorted alphabetically.
-* Use `goimports` (or `golangci‑lint --fast`) to automatically fix import ordering.
+- Within each block, imports are sorted alphabetically.
+- Use `goimports` (or `golangci‑lint --fast`) to automatically fix import ordering.
 
 ### 4.2 Naming Conventions
-| Entity | Rule |
-|--------|------|
-| Packages | lower‑case, no underscores or hyphens. |
-| Files | snake_case (`*_test.go` for tests). |
-| Types / Structs | PascalCase, descriptive (e.g., `ReviewConfig`). |
-| Interfaces | End with `er` when appropriate (e.g., `ChatAdapter`). |
-| Variables / Constants | camelCase for locals, PascalCase for exported. |
-| Functions | PascalCase if exported, camelCase if private. |
-| Constants | Use `CamelCase` for groups, `CONST_NAME` only for truly global values. |
-| Test Functions | `TestXxx` and optionally `BenchmarkXxx`. |
+
+| Entity                | Rule                                                                   |
+| --------------------- | ---------------------------------------------------------------------- |
+| Packages              | lower‑case, no underscores or hyphens.                                 |
+| Files                 | snake_case (`*_test.go` for tests).                                    |
+| Types / Structs       | PascalCase, descriptive (e.g., `ReviewConfig`).                        |
+| Interfaces            | End with `er` when appropriate (e.g., `ChatAdapter`).                  |
+| Variables / Constants | camelCase for locals, PascalCase for exported.                         |
+| Functions             | PascalCase if exported, camelCase if private.                          |
+| Constants             | Use `CamelCase` for groups, `CONST_NAME` only for truly global values. |
+| Test Functions        | `TestXxx` and optionally `BenchmarkXxx`.                               |
 
 ### 4.3 Error Handling
-* Errors are **never** ignored.  Use the blank identifier only when the value is truly irrelevant.
-* Wrap contextual information using `fmt.Errorf("context: %w", err)` or `errors.Wrap` if the project imports `github.com/pkg/errors` (currently not used).
-* Return errors **as soon as** they are detected – "guard clause" style.
-* In public APIs, prefer error values over panics.  Panics are limited to unrecoverable
+
+- Errors are **never** ignored. Use the blank identifier only when the value is truly irrelevant.
+- Wrap contextual information using `fmt.Errorf("context: %w", err)` or `errors.Wrap` if the project imports `github.com/pkg/errors` (currently not used).
+- Return errors **as soon as** they are detected – "guard clause" style.
+- In public APIs, prefer error values over panics. Panics are limited to unrecoverable
   programming errors (e.g., nil‑pointer dereference in `init`).
 
 ### 4.4 Documentation & Comments
-* Exported identifiers **must** have a preceding doc comment beginning with the name (e.g., `// JudgePR reviews a PR and returns comments.`).
-* Inline comments should be sentence‑case and end with a period.
-* Use `//go:generate` directives sparingly; they must be accompanied by a test that ensures the generated file is up‑to‑date.
+
+- Exported identifiers **must** have a preceding doc comment beginning with the name (e.g., `// JudgePR reviews a PR and returns comments.`).
+- Inline comments should be sentence‑case and end with a period.
+- Use `//go:generate` directives sparingly; they must be accompanied by a test that ensures the generated file is up‑to‑date.
 
 ### 4.5 Testing Practices
-* Keep tests **table‑driven** where possible; this yields concise, readable test suites.
-* Use the `testing` package only; avoid third‑party test frameworks.
-* Prefer `t.Fatalf` for fatal errors and `t.Errorf` for non‑fatal assertions.
-* When comparing complex structs, use `github.com/google/go-cmp/cmp` (already in `go.mod`).
-* Test coverage should be **≥ 80 %** for new code.
-* All tests must be **deterministic** – no reliance on external services; use mocks or fakes.
+
+- Keep tests **table‑driven** where possible; this yields concise, readable test suites.
+- Use the `testing` package only; avoid third‑party test frameworks.
+- Prefer `t.Fatalf` for fatal errors and `t.Errorf` for non‑fatal assertions.
+- When comparing complex structs, use `github.com/google/go-cmp/cmp` (already in `go.mod`).
+- Test coverage should be **≥ 80 %** for new code.
+- All tests must be **deterministic** – no reliance on external services; use mocks or fakes.
 
 ### 4.6 Dependency Management
-* All dependencies are managed via Go modules (`go.mod`).
-* Run `go mod tidy` after adding/removing imports.
-* Do not commit `vendor/` unless a vendoring strategy is explicitly adopted.
+
+- All dependencies are managed via Go modules (`go.mod`).
+- Run `go mod tidy` after adding/removing imports.
+- Do not commit `vendor/` unless a vendoring strategy is explicitly adopted.
 
 ### 4.7 Logging & Output
-* Use the standard library `log` package for user‑visible output.
-* For structured logs (debug mode), wrap with a small helper that respects the `--debug` flag.
-* CLI output must be **machine‑parsable** when `--json-output` is set (future feature).
+
+- Use the standard library `log` package for user‑visible output.
+- For structured logs (debug mode), wrap with a small helper that respects the `--debug` flag.
+- CLI output must be **machine‑parsable** when `--json-output` is set (future feature).
 
 ---
 
 ## 5️⃣ Git & Pull‑Request Workflow (for agents)
 
 1. **Branch naming** – `feature/<short‑desc>` or `bugfix/<short‑desc>`.
-2. **Commits** – One logical change per commit, with a concise subject line (<50 chars) and an optional body explaining *why* the change was needed.
+2. **Commits** – One logical change per commit, with a concise subject line (<50 chars) and an optional body explaining _why_ the change was needed.
 3. **CI** – Every PR must pass `go test ./...`, `go vet ./...`, and `golangci‑lint run` (if configured).
 4. **Review** – Agents should add comments only via the LLM; do not edit code generated by the LLM unless a test fails.
 5. **Rebasing** – Keep a linear history; use `git rebase -i` locally before merging.
@@ -129,7 +137,7 @@ All changes must satisfy the following conventions.  They are enforced by
 ## 6️⃣ Cursor / Copilot Rules (None Present)
 
 The repository does not contain a `.cursor` directory or a `.github/copilot‑instructions.md`
-file.  If such files are added in the future, agents should read them and incorporate
+file. If such files are added in the future, agents should read them and incorporate
 any additional constraints into this document.
 
 ---
@@ -159,4 +167,5 @@ go mod tidy
 
 ---
 
-*End of AGENTS.md*
+_End of AGENTS.md_
+

internal/pierre/guidelines_test.go

@@ -0,0 +1,70 @@
+package pierre
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+)
+
+func TestParseGuidelinesFromStringValid(t *testing.T) {
+	md := "# Rule One\n\n  - Item A  \n\n# Rule Two\n"
+	lines, err := parseGuidelinesFromString(md)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	expected := []string{"# Rule One", "- Item A", "# Rule Two"}
+	if got, want := fmt.Sprint(lines), fmt.Sprint(expected); got != want {
+		t.Fatalf("expected %v, got %v", expected, lines)
+	}
+}
+
+func TestParseGuidelinesFromStringEmpty(t *testing.T) {
+	md := "\n   \n"
+	lines, err := parseGuidelinesFromString(md)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(lines) != 0 {
+		t.Fatalf("expected empty slice, got %d elements", len(lines))
+	}
+}
+
+func TestParseGuidelinesFromStringTooManyLines(t *testing.T) {
+	// generate 1001 non-empty lines
+	var sb strings.Builder
+	for i := 0; i < 1001; i++ {
+		sb.WriteString(fmt.Sprintf("Line %d\n", i))
+	}
+	_, err := parseGuidelinesFromString(sb.String())
+	if err == nil {
+		t.Fatalf("expected error for exceeding line limit, got nil")
+	}
+}
+
+func TestWithGuidelinesSuccess(t *testing.T) {
+	svc := &Service{}
+	md := "First line\nSecond line\n"
+	if err := svc.WithGuidelines(md); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	expected := []string{"First line", "Second line"}
+	if got, want := fmt.Sprint(svc.guidelines), fmt.Sprint(expected); got != want {
+		t.Fatalf("expected guidelines %v, got %v", expected, svc.guidelines)
+	}
+}
+
+func TestWithGuidelinesError(t *testing.T) {
+	svc := &Service{guidelines: []string{"old"}}
+	var sb strings.Builder
+	for i := 0; i < 1001; i++ {
+		sb.WriteString("x\n")
+	}
+	err := svc.WithGuidelines(sb.String())
+	if err == nil {
+		t.Fatalf("expected error, got nil")
+	}
+	// ensure old guidelines unchanged
+	if len(svc.guidelines) != 1 || svc.guidelines[0] != "old" {
+		t.Fatalf("guidelines should remain unchanged on error")
+	}
+}

internal/pierre/resource.go

@@ -2,7 +2,9 @@ package pierre
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"strings"
 
 	"git.schreifuchs.ch/schreifuchs/pierre-bot/internal/chatter"
 )
@@ -14,13 +16,14 @@ import (
 // elsewhere in the future.
 type Service struct {
 	maxChunkSize    int
-	guidelines      []string
+	guidelines      []string // stored as slice of lines; legacy, see WithGuidelines
 	disableComments bool
 	git             GitAdapter
 	chat            ChatAdapter
 }
 
 func New(chat ChatAdapter, git GitAdapter, maxChunkSize int, guidelines []string, disableComments bool) *Service {
+	// Existing constructor retains slice based guidelines for backward compatibility.
 	return &Service{
 		git:             git,
 		chat:            chat,
@@ -30,6 +33,39 @@ func New(chat ChatAdapter, git GitAdapter, maxChunkSize int, guidelines []string
 	}
 }
 
+// WithGuidelines parses a raw Markdown string (or any multiline string) into
+// individual guideline lines, validates the line‑count (max 1000 non‑empty lines),
+// and stores the result in the Service. It returns an error if validation fails.
+// This is a convenience mutator for callers that have the guidelines as a
+// single string.
+func (s *Service) WithGuidelines(md string) error {
+	lines, err := parseGuidelinesFromString(md)
+	if err != nil {
+		return err
+	}
+	s.guidelines = lines
+	return nil
+}
+
+// parseGuidelinesFromString splits a markdown string into trimmed, non‑empty
+// lines and ensures the total number of lines does not exceed 1000.
+func parseGuidelinesFromString(md string) ([]string, error) {
+	var result []string
+	// Split on newline. Handles both \n and \r\n because TrimSpace removes \r.
+	rawLines := strings.Split(md, "\n")
+	for _, l := range rawLines {
+		trimmed := strings.TrimSpace(l)
+		if trimmed == "" {
+			continue
+		}
+		result = append(result, trimmed)
+	}
+	if len(result) > 1000 {
+		return nil, fmt.Errorf("guidelines exceed 1000 lines (found %d)", len(result))
+	}
+	return result, nil
+}
+
 type GitAdapter interface {
 	GetDiff(ctx context.Context, owner, repo string, prID int) (io.ReadCloser, error)
 	AddComment(ctx context.Context, owner, repo string, prID int, comment Comment) error