# Review Pr > Review a pull request and post structured feedback as a comment. Use when this capability is needed. - Skill: `tomevault-io/review-pr-21` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/review-pr-21` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/review-pr-21/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/review-pr-21 --- Review the pull request and provide structured feedback. ## How to fetch the PR If given a PR number or URL, use `gh pr diff ` to get the diff and `gh pr view ` 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__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. --- > Source: [ClickHouse/clickhouse-go](https://github.com/ClickHouse/clickhouse-go) — distributed by [TomeVault](https://tomevault.io).