review-pr

name	review-pr
description	Review a pull request and post structured feedback as a comment.
argument-hint	<PR-number>
allowed-tools	Read, Glob, Bash(grep:*), Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr comment:*)

Review the pull request and provide structured feedback.

How to fetch the PR

If given a PR number or URL, use gh pr diff <number> to get the diff and gh pr view <number> to get the description. Read any referenced issues for context.

Review structure

Return feedback in this order:

1. Summary — one paragraph: what the PR does, why it exists, whether the approach is sound.
2. Must fix — blocking issues; the PR should not merge until these are resolved.
3. Should fix — non-blocking but important (correctness risks, API design concerns, missing tests).
4. Nits — style, naming, minor clarity improvements. Prefix each with nit:.
5. Verdict — one of: Approve, Request changes, or Needs discussion.

If a section has no items, omit it.

---

Checklist

Work through each category before writing the review.

Correctness

• Does the logic handle error cases? Are errors returned (not swallowed with _)?
• Are all Rows, Batch, and Conn values closed on every code path, including error paths?
• Are there data races? Does any shared state lack synchronisation?
• Does the Batch lifecycle (Append → Send or Abort) hold on every path?
• Is context.Context propagated correctly (first param, never stored in a struct)?

API design — easy to use correctly, hard to use incorrectly

• Can a caller misuse the new API without the compiler or runtime catching it?
• Does the change silently break any existing callers? Prefer deprecation over removal.
• Are new exported types/functions/methods necessary? Could this be done with existing primitives?
• Do new interfaces belong at the point of consumption, not implementation?
• Is the zero value of any new struct useful or at least safe?

Go idioms

• Error strings: lowercase, no trailing punctuation.
• Acronyms in names: URL, HTTP, ID — not Url, Http, Id.
• Receiver names: short abbreviation, never self or this.
• No context.Context stored in struct fields.
• No panic outside of init()-style invariant checks.
• Imports: stdlib → external → internal, blank line between groups.
• New types that contain a mutex are not copied by value.
• mixedCaps for unexported constants, not ALL_CAPS.

Tests

• Is there a test that would have caught the bug being fixed?
• Bug fixes: is there a regression test in tests/issues/issue_<N>_test.go?
• New ClickHouse type support: column implementation + round-trip test + example?
• Do test failure messages say what was wrong, what input triggered it, and what was expected vs. got?
• Are tests table-driven where there are multiple cases?
• No test mocking of driver.Conn or driver.Rows for integrations tests inside root /tests/ — use a real ClickHouse via testcontainers.
• Make sure tests are covered for both TCP and HTTP protocol cases.
• Make sure tests are covered for both clickhouse_native (Open() api returns) api and std api (OpenDB() api returns).

Performance & protocol

• Does the change avoid unnecessary allocations in hot paths (encoding/decoding columns)?
• Are new Options fields safe to ignore when unset (i.e., the zero value is the sensible default)?
• Does the change affect both the native TCP and HTTP protocol paths? If so, are both covered?

Documentation

• Are new exported symbols documented with a full-sentence doc comment beginning with the symbol name?
• ClickHouse SQL types and function names wrapped in backticks in any prose.
• Make sure existing docs and examples are updated.
• Make sure new docs and examples are added if needed for new features or bug fixes.
• Make sure the docs are covered for both TCP and HTTP protocol cases.
• Make sure the docs are covered for both clickhouse_native (Open() api returns) api and std api (OpenDB() api returns).
• If a comment or doc comment includes usage examples (e.g. inline code blocks), it is acceptable to omit error checking. Do not flag missing error handling in example code.