go/.claude/skills/review-pr/SKILL.md at main - go - Hanzo Git

hanzo-ds/go

mirror of https://github.com/hanzo-ds/go.git synced 2026-10-30 19:45:01 +00:00

Files

T

zeekay 0c90ebe02e debrand: finish the rename across source, docs and dev infra

Source: the unexported clickhouse struct becomes client, the ch receiver and
chproto import alias follow, error message prefixes read datastore, and the
JSON serializer interface drops the vendor infix. resources' metadata key and
its accessor are renamed together, with a test proving the embedded YAML still
unmarshals — a silent drift there would have made every connection take the
unsupported-server-version branch.

Codegen: all three templates still emitted the upstream module paths, so
regenerating would have quietly undone the rename. Note the templates have
drifted from the files they produce (regenerating today drops QBit and
scanTypeDuration), so the generated files are left as tracked and only the
templates are corrected.

Dead links: an earlier mechanical pass invented URLs that do not resolve —
github.com/ClickHouse/datastore-go, hanzo-ds/driver issue numbers that belong to
upstream, and mangled names of independent projects (mailru, uptrace, nikepan,
EpicStep). Third-party names are not ours to rewrite, so those directories are
removed rather than repaired, and provenance comments now point at real files.

Dev infra: compose runs our own server image at the tag production runs, since
the vendor's alpine tag does not exist in our registry. Cluster hostnames,
nginx upstreams and server configs are renamed as one set.

CHANGELOG.md is upstream's release history for versions this module never
released; rewriting it would fabricate a lineage, so it is dropped and the
upstream base recorded in README instead.

The two long-standing vet findings are fixed by keying the struct literals
rather than suppressed. LICENSE and contributors/list keep the vendor name on
purpose: one is the copyright notice Apache-2.0 requires, the other is real
people's names. CI now enforces build, vet, test, gofmt and a grep gate.

2026-07-26 11:46:39 -07:00

4.1 KiB

Raw Permalink Blame History

name, description, argument-hint, allowed-tools

name	description	argument-hint	allowed-tools
review-pr	Review a pull request and post structured feedback as a comment.	<PR-number>	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:

Summary — one paragraph: what the PR does, why it exists, whether the approach is sound.
Must fix — blocking issues; the PR should not merge until these are resolved.
Should fix — non-blocking but important (correctness risks, API design concerns, missing tests).
Nits — style, naming, minor clarity improvements. Prefix each with nit:.
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 Datastore 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 Datastore via testcontainers.
Make sure tests are covered for both TCP and HTTP protocol cases.
Make sure tests are covered for both datastore_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?
Datastore 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 datastore_native (Open() api returns) api and std api (OpenDB() api returns).