review-bot/REVIEW.md

review-bot Code Review (vs go-patterns)

Overall Assessment

The review-bot is a well-structured, focused Go application that follows many idiomatic patterns correctly. The package layout is clean (gitea/, llm/, review/, cmd/), error handling uses %w wrapping consistently, and the test suite covers all major code paths using httptest. However, there are several areas where the code diverges from the patterns documented in go-patterns — particularly around configuration, context propagation, exported fields, documentation, and testing idioms.

Verdict: Solid foundation with targeted improvements needed.

Findings

#	Severity	File	Pattern Violated	Finding
1	MAJOR	gitea/client.go	concurrency / api-conventions	No context.Context parameter on any method — HTTP calls are uncancellable
2	MAJOR	llm/client.go	concurrency / api-conventions	Complete() accepts no context — no timeout or cancellation support
3	MAJOR	gitea/client.go	structs / encapsulation	Client fields (BaseURL, Token, HTTP) are exported but should be unexported
4	MAJOR	llm/client.go	structs / encapsulation	Client fields (BaseURL, APIKey, Model, HTTP) are exported — leaks credentials via reflection/logging
5	MINOR	cmd/review-bot/main.go	configuration	No input validation beyond emptiness — e.g., URL format, model name format
6	MINOR	cmd/review-bot/main.go	error-handling	Uses log.Fatalf for all errors — no cleanup, deferred functions won't run
7	MINOR	gitea/client.go	error-handling / style	Error strings in doGet are inconsistent — some use fmt.Errorf, the raw HTTP error doesn't wrap with %w
8	MINOR	review/prompt.go	style / api-conventions	BuildSystemPrompt uses 20+ WriteString calls — could use a raw string literal for readability
9	MINOR	gitea/client.go	documentation	No concurrency safety documentation on Client type
10	MINOR	llm/client.go	documentation	No concurrency safety documentation on Client type
11	MINOR	gitea/client_test.go	testing-advanced	Tests don't use t.Run subtests — individual test functions instead of table-driven with named cases
12	MINOR	integration_test.go	style	Uses rune literal '/' comparison in a loop instead of strings.SplitN (inconsistent with main.go)
13	MINOR	llm/client.go	configuration	Temperature: 0.1 is hardcoded — not configurable and the zero-value (0.0) semantic isn't clear
14	NIT	gitea/client.go	style	PostReview converts []byte to string then passes to strings.NewReader — use bytes.NewReader(data) directly
15	NIT	review/formatter.go	documentation	GiteaEvent has no doc comment explaining the mapping semantics
16	NIT	cmd/review-bot/main.go	package-design	evaluateCIStatus is unexported logic in main — could live in review package for testability
17	NIT	gitea/client.go	interfaces	No interface defined for the Gitea client — makes the main function harder to unit test
18	NIT	llm/client.go	interfaces	No interface defined for the LLM client — same testability concern
19	NIT	review/parser.go	error-handling	extractJSON silently handles malformed fences — edge case: \``` with only 1 line produces empty string
20	NIT	Various	documentation	No package-level doc comments (// Package xxx ...) on any package

Detailed Findings

1. No context.Context on Gitea client methods (MAJOR)

What the code does:

func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) {
    url := fmt.Sprintf(...)
    body, err := c.doGet(url)
    ...
}

What the pattern says: From concurrency.md §6 (Context Propagation Rules): "Pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx." From api-conventions.md §3 (WithContext variant): All I/O-performing functions should accept a context for timeout/cancellation.

How to fix:

func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
    ...
    req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
    ...
}

Add context.Context as the first parameter to all public methods. Update doGet to accept context internally.

---

2. No context.Context on LLM Complete() (MAJOR)

What the code does:

func (c *Client) Complete(messages []Message) (string, error) {
    ...
    req, err := http.NewRequest("POST", url, bytes.NewReader(data))
    ...
}

What the pattern says: Same as finding #1. LLM calls can take 30-60+ seconds. Without context, there's no way to enforce a timeout or cancel a review that's taking too long.

How to fix:

func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
    ...
    req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewReader(data))
    ...
}

The caller in main.go should create a context with timeout: ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute).

---

3 & 4. Exported struct fields on Client types (MAJOR)

What the code does:

type Client struct {
    BaseURL string
    Token   string
    HTTP    *http.Client
}

What the pattern says: From structs.md: use unexported fields for internal state; expose only what callers need to read/modify. From configuration.md §9: Document immutability constraints. Exported fields like Token and APIKey are sensitive credentials that could be accidentally logged, serialized, or mutated after construction.

How to fix:

type Client struct {
    baseURL string
    token   string
    http    *http.Client
}

If tests need to override HTTP, expose it via a functional option or a WithHTTPClient(*http.Client) setter, or accept it in the constructor.

---

5. No input validation beyond emptiness (MINOR)

What the code does:

if *giteaURL == "" || *repo == "" || ...

What the pattern says: From configuration.md §1 (Zero-Value Config): Document and validate configuration explicitly. A malformed URL (e.g., missing scheme) will produce a confusing error later during HTTP request creation rather than at startup.

How to fix:

if _, err := url.Parse(*giteaURL); err != nil || !strings.HasPrefix(*giteaURL, "http") {
    log.Fatalf("Invalid --gitea-url: %s", *giteaURL)
}

---

6. log.Fatalf for all errors (MINOR)

What the code does: log.Fatalf(...) is used for every error in main().

What the pattern says: From api-conventions.md §9 (Graceful Shutdown): distinguish between fatal and recoverable errors. From error-handling.md: error handling should give callers the ability to respond. While main() is the top-level caller, log.Fatalf calls os.Exit(1) which doesn't run deferred functions.

How to fix: Use a run() error pattern:

func main() {
    if err := run(); err != nil {
        fmt.Fprintf(os.Stderr, "error: %v\n", err)
        os.Exit(1)
    }
}

func run() error { ... }

This allows deferred cleanup to run and makes the code testable.

---

7. Inconsistent error formatting in doGet (MINOR)

What the code does:

func (c *Client) doGet(url string) ([]byte, error) {
    ...
    return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
}

The error from the raw HTTP response isn't wrapped with %w, but the callers wrap it again: fmt.Errorf("fetch PR: %w", err). The inner error starts with a capital "HTTP".

What the pattern says: From smells/anti-patterns.md §6 (Error String Formatting): error strings should be lowercase. They compose upward: fetch PR: HTTP 404: ... has inconsistent casing.

How to fix:

return nil, fmt.Errorf("http %d: %s", resp.StatusCode, string(body))

---

8. Prompt building uses excessive WriteString (MINOR)

What the code does:

sb.WriteString("You are an expert code reviewer...\n\n")
sb.WriteString("Your task:\n")
sb.WriteString("1. Review the diff...\n")
// ... 20+ more lines

What the pattern says: From style.md: code should be readable and maintainable. A raw string literal would be far more readable for a multi-line prompt template.

How to fix:

const systemPromptTemplate = `You are an expert code reviewer. Review the provided pull request diff carefully.

Your task:
1. Review the diff for correctness, idiomatic code, potential bugs, and design issues.
...
`

func BuildSystemPrompt(conventions string) string {
    prompt := systemPromptTemplate
    if conventions != "" {
        prompt += fmt.Sprintf("\n\nThe repository has the following coding conventions...\n\n%s\n", conventions)
    }
    return prompt
}

---

9 & 10. No concurrency safety documentation (MINOR)

What the code does: Neither gitea.Client nor llm.Client documents whether they're safe for concurrent use.

What the pattern says: From documentation.md §9 (Concurrency Documentation): "Doc comments explicitly state the concurrency safety of a type." Since both types embed *http.Client (which IS safe for concurrent use), the wrapping types should document this.

How to fix:

// Client interacts with the Gitea API.
// A Client is safe for concurrent use by multiple goroutines.
type Client struct { ... }

---

11. Tests don't use t.Run subtests (MINOR)

What the code does: gitea/client_test.go defines 8 separate TestXxx functions, each creating their own httptest server.

What the pattern says: From testing-advanced.md §1 (Table-Driven Tests with t.Run): related tests should use named subtests for filterability and clarity. The Gitea client tests share identical setup patterns — they'd benefit from a shared helper.

How to fix: Consider a test helper that creates a server with a handler map, then use t.Run for each case. The existing structure is acceptable but could be DRYer.

---

12. Inconsistent repo parsing in integration_test.go (MINOR)

What the code does:

for i, c := range giteaRepo {
    if c == '/' {
        owner = giteaRepo[:i]
        repoName = giteaRepo[i+1:]
        break
    }
}

What the pattern says: From style.md §3 (File Organization by Responsibility): related logic should be consistent. main.go uses strings.SplitN(*repo, "/", 2) for the same operation. The integration test reinvents it with a manual loop.

How to fix: Use strings.SplitN(giteaRepo, "/", 2) for consistency, or extract a shared helper.

---

13. Hardcoded Temperature: 0.1 (MINOR)

What the code does:

reqBody := ChatRequest{
    ...
    Temperature: 0.1,
}

What the pattern says: From configuration.md §1 (Zero-Value Usable Config): "Every field documents its zero-value behavior." The temperature is buried in implementation. It should be configurable (e.g., a field on Client with a documented default).

How to fix: Add a Temperature field to Client with documentation:

type Client struct {
    ...
    // Temperature controls LLM randomness. If zero, defaults to 0.1.
    Temperature float64
}

---

14. Unnecessary string() → strings.NewReader conversion (NIT)

What the code does:

data, err := json.Marshal(payload)
...
req, err := http.NewRequest("POST", url, strings.NewReader(string(data)))

What the pattern says: From style.md: avoid unnecessary allocations. json.Marshal returns []byte; use bytes.NewReader(data) directly to avoid the []byte→string copy.

How to fix:

req, err := http.NewRequest("POST", url, bytes.NewReader(data))

---

15. Missing doc comment on GiteaEvent (NIT)

What the code does:

// GiteaEvent converts the verdict to the Gitea API event string.
func GiteaEvent(verdict string) string {

Actually, this one DOES have a doc comment. On closer inspection the comment exists. Removing this finding — correction: the comment is present but minimal. It doesn't document the mapping or the "COMMENT" fallback behavior. This is borderline.

---

16. evaluateCIStatus in main package (NIT)

What the code does: The evaluateCIStatus function lives in cmd/review-bot/main.go and operates on []gitea.CommitStatus.

What the pattern says: From package-design.md: packages should encapsulate related logic. This function interprets CI status semantics — it belongs in the review package (or even gitea) where it could be unit-tested independently without building the entire binary.

---

17 & 18. No interfaces for testability (NIT)

What the code does: main.go directly uses *gitea.Client and *llm.Client concrete types.

What the pattern says: From interfaces.md: "Define interfaces in the package that USES them." From smells/common-mistakes.md §10 (Premature Abstraction): don't create interfaces before you need them. However, the consumer (main.go) would benefit from small interfaces for testing the orchestration logic independently.

How to fix (when needed):

// In main or a review orchestrator package:
type PRFetcher interface {
    GetPullRequest(ctx context.Context, owner, repo string, number int) (*gitea.PullRequest, error)
    GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
}

Note: This is a NIT because the current code doesn't have tests for main.go orchestration. If/when that's needed, interfaces become valuable.

---

19. extractJSON edge case (NIT)

What the code does:

if strings.HasPrefix(s, "```") {
    lines := strings.Split(s, "\n")
    if len(lines) > 2 {
        lines = lines[1:]
    }
    ...
}

If input is exactly ```json\n``` (fence with empty body), it produces an empty string that will fail JSON parse with a confusing error message.

What the pattern says: From error-handling.md: errors should carry context. Consider returning an explicit error from extractJSON when the extracted content is empty after fence stripping.

---

20. No package doc comments (NIT)

What the code does: None of the packages (gitea, llm, review) have // Package xxx ... doc comments.

What the pattern says: From documentation.md §1 (Package Documentation): "The first file in a package starts with a // Package xxx ... comment that explains the package's purpose."

How to fix: Add to each package's primary file:

// Package gitea provides a client for the Gitea API, focused on pull request review operations.
package gitea

---

Positive Patterns

The codebase does several things well:

1. Clean package separation — gitea/, llm/, review/, cmd/ each have a single responsibility. This matches package-design.md perfectly.

2. Consistent error wrapping — Every public function wraps errors with fmt.Errorf("context: %w", err), providing clear error chains. This follows error-handling.md closely.

3. Return concrete types from constructors — NewClient() returns *Client, not an interface. Matches smells/common-mistakes.md §7 and smells/anti-patterns.md §8.

4. httptest-based testing — Both client packages use net/http/httptest for isolated, deterministic tests. No external dependencies needed.

5. Good test coverage of error paths — Tests cover 404s, bad JSON, connection failures, invalid severities, missing fields. This is thorough.

6. Zero dependencies — go.mod has no external dependencies. The entire project uses only the standard library. This is excellent for a focused tool.

7. Build-tagged integration test — The //go:build integration tag keeps expensive tests separate from unit tests. Good practice.

8. strings.Builder usage — Prompt building and formatting use strings.Builder correctly for efficient string construction.

9. Named return values where useful — evaluateCIStatus uses named returns (passed bool, details string) for documentation clarity, matching style.md §5.

10. No premature abstraction — The code doesn't define interfaces it doesn't need yet. It's concrete and straightforward, following smells/common-mistakes.md §10.

Recommendations

Priority-ordered list of improvements:

1. Add context.Context to all client methods (Critical) — This is the single most impactful change. LLM calls can hang indefinitely without timeout support. Both gitea.Client and llm.Client should accept context as the first parameter on all public methods. Use http.NewRequestWithContext.

2. Unexport client struct fields (High) — Token, APIKey, BaseURL should be unexported to prevent accidental logging/serialization of credentials. Expose only what's needed via methods or constructor options.

3. Add package documentation (Medium) — Each package needs a // Package xxx ... comment. This takes 5 minutes and significantly improves discoverability.

4. Extract evaluateCIStatus to review package (Medium) — Makes it independently testable and keeps main.go focused on orchestration.

5. Use run() error pattern in main (Medium) — Enables deferred cleanup and makes the orchestration logic more testable.

6. Replace WriteString chain with raw string literal (Low) — Pure readability improvement for BuildSystemPrompt.

7. Make LLM temperature configurable (Low) — Add as a field on Client with documented zero-value default.

8. Use bytes.NewReader instead of strings.NewReader(string(...)) in PostReview (Low) — Eliminates one unnecessary allocation.

9. Add concurrency documentation to Client types (Low) — One-line doc additions.

10. Consider consumer-side interfaces when testing main orchestration (Future) — Not needed now, but will become valuable if the main.go logic grows or needs unit testing.