Code Review with GitHub PRs

Q: Should reviewers run code locally?

For risky changes, yes: gh pr checkout then go test ./.... CI green is necessary but not always sufficient.

Q: What labels help Go repos?

breaking, module-deps, needs-changelog, security. Filter release notes and review priority.

Q: Do comments on generated files matter?

Comment on generator inputs or Makefile targets, not *.pb.go lines directly. Regenerate in follow-up commit.

Pull requests are the integration gate for Go module repos.

Effective review keeps changes small, calls out go.mod impact, and verifies tests - not just style opinions.

Summary

A good Go PR states behavior change, module/API impact, and how tests were run.

Reviewers scan exported API deltas, error handling, concurrency, and dependency diffs before approving.

Large PRs stall trunk; slice by package or feature flag when possible.

Recipe

Quick-reference recipe card - copy-paste ready.

PR title: internal/parse: reject empty input in Decode

PR body template:

## Summary
- Reject empty input in `Decode` with `ErrEmptyInput`
 
## Module impact
- No new dependencies
- Exported error var is a compatible addition
 
## Test plan
- [ ] go test ./...
- [ ] go vet ./...
- [ ] go mod tidy (no diff)
 
## Screenshots / logs
N/A

When to reach for this:

Establishing team review norms for a new Go service or library.
Training reviewers to spot breaking module changes.
Reducing review latency on monorepos with noisy diffs.
Onboarding contributors who only know language-level review.

Working Example

Contributor opens a PR adding gRPC health checks:

// internal/health/grpc.go (excerpt)
package health
 
import "google.golang.org/grpc/health/grpc_health_v1"
 
func Register(s *grpc.Server) {
    srv := grpc_health_v1.NewHealthServer()
    grpc_health_v1.RegisterHealthServer(s, srv)
}

# go.mod excerpt
+ google.golang.org/grpc v1.68.0

Reviewer checklist:

Confirm go test ./... green in CI.
Ask whether health should be optional via config.
Verify gRPC version aligns with existing server setup.
Check import path does not leak internal/ across module boundary.
Request changelog note if public operator-facing behavior changed.

Approval after author documents opt-out flag and tidy check passes.

What this demonstrates:

API and dependency sections in description guide review focus.
Reviewers treat go.mod diff as first-class, not noise.
Behavior questions precede bike-shedding on naming.

Deep Dive

How It Works

Author pushes branch and opens PR against main.
CI runs tests, vet, tidy, lint.
Reviewers leave comments on lines, packages, or module files.
Author pushes fixes; GitHub re-runs checks.
Maintainer merges when approvals and required checks satisfy branch protection.

GitHub's merge queue (optional) serializes merges to keep trunk green under load.

PR Size Guidance

Size	Lines (approx)	Review outcome
Small	under 200	Same-day merge likely
Medium	200-400	Needs focused review time
Large	400+	Split unless mechanical rename

Mechanical changes (go fix, generated protobuf refresh) may exceed limits if isolated and labeled.

Module-Aware Review Points

Area	Question
Exported symbols	Does this promise compatibility for module consumers?
`go.mod`	New transitive weight? Version justified?
`internal/`	Any illegal cross-module internal import?
Errors	Wrapped with `%w`? Sentinel vs dynamic errors documented?
Concurrency	Goroutine lifecycle, context cancellation, data races?
Tests	Table-driven? Covers error paths? `-race` worth it?

Description Fields That Help

User-visible behavior - CLI flags, HTTP codes, config keys.
Breaking changes - Requires /v2 path or major bump?
Rollout - Feature flag, migration steps.
Benchmarks - If hot path changed, paste benchstat summary.

Reviewer Etiquette

Request changes for correctness, module safety, or missing tests.

Nit comments should be optional (nit: prefix).

Approve when issues are minor and trust author follow-up issues for debt.

Gotchas

Rubber-stamping green CI - Tests may miss packages without cases. Fix: skim diff for untested exported funcs.
Ignoring go.sum bulk - Could hide supply-chain swap. Fix: read go mod why -m for new modules.
Style-only review on hot paths - Misses alloc regressions. Fix: ask for benchmark or profiling note.
Giant generated diff - Hides manual edits. Fix: separate PRs for regen vs logic.
Reviewing fork PRs without maintainer checkout - Hard to run locally. Fix: use gh pr checkout or trust CI matrix.

Alternatives

Alternative	Use When	Don't Use When
Pair programming	Complex design spike	Async distributed team
Stacked PRs	Sequential dependent changes	Reviewers lack stack tooling
CODEOWNERS auto-request	Large monorepo ownership	Tiny two-person library
Maintainer merge without PR	Emergency hotfix	Normal feature flow

FAQs

How small should a Go PR be?

Target one logical change reviewers can hold in working memory.

Split refactors from behavior changes across PRs.

Should reviewers run code locally?

For risky changes, yes: gh pr checkout <n> then go test ./....

CI green is necessary but not always sufficient.

How review dependency-only PRs?

Check changelog and security advisories for bumped modules.

Run tests and inspect go mod why for unexpected paths.

What labels help Go repos?

breaking, module-deps, needs-changelog, security.

Filter release notes and review priority.

Should API renames go in one PR?

Prefer mechanical rename PR separate from behavior changes.

Easier review and cleaner bisect.

How handle WIP PRs?

Use draft PRs.

Convert to ready only when description, tests, and tidy are complete.

Do comments on generated files matter?

Comment on generator inputs or Makefile targets, not *.pb.go lines directly.

Regenerate in follow-up commit.

How review context cancellation changes?

Trace call paths from HTTP/gRPC handlers to background work.

Missing ctx.Done() checks are release blockers.

Should libraries require changelog entries?

Yes for exported API or behavior consumers rely on.

Apps may use release notes only at tag time.

How fast should first review happen?

Within one business day keeps trunk flow healthy.

Set team SLA and rotate review duty.

What about auto-merge?

Fine when required checks are strong and PR size limits enforced.

Avoid for dependency bumps without human glance.

How review monorepo cross-module PRs?

List every touched go.mod.

Confirm workspace or replace directives are not accidentally committed.

Branching Strategies & Trunk-Based Flow - Short branches make review tractable
GitHub Actions for Go Repos - CI reviewers rely on
Git for Go Teams - Why merged PRs become module releases
Signed Tags & Go Module Releases - Post-merge tagging discipline
Git & GitHub Best Practices - Portable review and Git habits

Stack versions: This page was written for Go 1.26.x (Green Tea GC default, go fix modernizers - verify patch at build), chi (latest - verify at build), gin (latest - verify at build), echo (latest - verify at build), google.golang.org/grpc (latest - verify at build), sigs.k8s.io/controller-runtime (latest - verify at build), kubebuilder (latest - verify at build), tinygo (latest - verify board targets at build), wazero (latest - verify at build), and golangci-lint (latest - verify linter set at build).

Effective review keeps changes small, calls out go.mod impact, and verifies tests - not just style opinions.

Summary

A good Go PR states behavior change, module/API impact, and how tests were run.

Reviewers scan exported API deltas, error handling, concurrency, and dependency diffs before approving.

Large PRs stall trunk; slice by package or feature flag when possible.

Recipe

Quick-reference recipe card - copy-paste ready.

PR title: internal/parse: reject empty input in Decode

PR body template:

## Summary
- Reject empty input in `Decode` with `ErrEmptyInput`
 
## Module impact
- No new dependencies
- Exported error var is a compatible addition
 
## Test plan
- [ ] go test ./...
- [ ] go vet ./...
- [ ] go mod tidy (no diff)
 
## Screenshots / logs
N/A

When to reach for this:

Establishing team review norms for a new Go service or library.
Training reviewers to spot breaking module changes.
Reducing review latency on monorepos with noisy diffs.
Onboarding contributors who only know language-level review.

Working Example

Contributor opens a PR adding gRPC health checks:

// internal/health/grpc.go (excerpt)
package health
 
import "google.golang.org/grpc/health/grpc_health_v1"
 
func Register(s *grpc.Server) {
    srv := grpc_health_v1.NewHealthServer()
    grpc_health_v1.RegisterHealthServer(s, srv)
}

# go.mod excerpt
+ google.golang.org/grpc v1.68.0

Reviewer checklist:

Confirm go test ./... green in CI.
Ask whether health should be optional via config.
Verify gRPC version aligns with existing server setup.
Check import path does not leak internal/ across module boundary.
Request changelog note if public operator-facing behavior changed.

Approval after author documents opt-out flag and tidy check passes.

What this demonstrates:

API and dependency sections in description guide review focus.
Reviewers treat go.mod diff as first-class, not noise.
Behavior questions precede bike-shedding on naming.

Deep Dive

How It Works

Author pushes branch and opens PR against main.
CI runs tests, vet, tidy, lint.
Reviewers leave comments on lines, packages, or module files.
Author pushes fixes; GitHub re-runs checks.
Maintainer merges when approvals and required checks satisfy branch protection.

GitHub's merge queue (optional) serializes merges to keep trunk green under load.

PR Size Guidance

Size	Lines (approx)	Review outcome
Small	under 200	Same-day merge likely
Medium	200-400	Needs focused review time
Large	400+	Split unless mechanical rename

Mechanical changes (go fix, generated protobuf refresh) may exceed limits if isolated and labeled.

Module-Aware Review Points

Area	Question
Exported symbols	Does this promise compatibility for module consumers?
`go.mod`	New transitive weight? Version justified?
`internal/`	Any illegal cross-module internal import?
Errors	Wrapped with `%w`? Sentinel vs dynamic errors documented?
Concurrency	Goroutine lifecycle, context cancellation, data races?
Tests	Table-driven? Covers error paths? `-race` worth it?

Description Fields That Help

User-visible behavior - CLI flags, HTTP codes, config keys.
Breaking changes - Requires /v2 path or major bump?
Rollout - Feature flag, migration steps.
Benchmarks - If hot path changed, paste benchstat summary.

Reviewer Etiquette

Request changes for correctness, module safety, or missing tests.

Nit comments should be optional (nit: prefix).

Approve when issues are minor and trust author follow-up issues for debt.

Gotchas

Rubber-stamping green CI - Tests may miss packages without cases. Fix: skim diff for untested exported funcs.
Ignoring go.sum bulk - Could hide supply-chain swap. Fix: read go mod why -m for new modules.
Style-only review on hot paths - Misses alloc regressions. Fix: ask for benchmark or profiling note.
Giant generated diff - Hides manual edits. Fix: separate PRs for regen vs logic.
Reviewing fork PRs without maintainer checkout - Hard to run locally. Fix: use gh pr checkout or trust CI matrix.

Alternatives

Alternative	Use When	Don't Use When
Pair programming	Complex design spike	Async distributed team
Stacked PRs	Sequential dependent changes	Reviewers lack stack tooling
CODEOWNERS auto-request	Large monorepo ownership	Tiny two-person library
Maintainer merge without PR	Emergency hotfix	Normal feature flow

FAQs

How small should a Go PR be?

Target one logical change reviewers can hold in working memory.

Split refactors from behavior changes across PRs.

Should reviewers run code locally?

For risky changes, yes: gh pr checkout <n> then go test ./....

CI green is necessary but not always sufficient.

How review dependency-only PRs?

Check changelog and security advisories for bumped modules.

Run tests and inspect go mod why for unexpected paths.

What labels help Go repos?

breaking, module-deps, needs-changelog, security.

Filter release notes and review priority.

Should API renames go in one PR?

Prefer mechanical rename PR separate from behavior changes.

Easier review and cleaner bisect.

How handle WIP PRs?

Use draft PRs.

Convert to ready only when description, tests, and tidy are complete.

Do comments on generated files matter?

Comment on generator inputs or Makefile targets, not *.pb.go lines directly.

Regenerate in follow-up commit.

How review context cancellation changes?

Trace call paths from HTTP/gRPC handlers to background work.

Missing ctx.Done() checks are release blockers.

Should libraries require changelog entries?

Yes for exported API or behavior consumers rely on.

Apps may use release notes only at tag time.

How fast should first review happen?

Within one business day keeps trunk flow healthy.

Set team SLA and rotate review duty.

What about auto-merge?

Fine when required checks are strong and PR size limits enforced.

Avoid for dependency bumps without human glance.

How review monorepo cross-module PRs?

List every touched go.mod.

Confirm workspace or replace directives are not accidentally committed.

Branching Strategies & Trunk-Based Flow - Short branches make review tractable
GitHub Actions for Go Repos - CI reviewers rely on
Git for Go Teams - Why merged PRs become module releases
Signed Tags & Go Module Releases - Post-merge tagging discipline
Git & GitHub Best Practices - Portable review and Git habits

Stack versions: This page was written for Go 1.26.x (Green Tea GC default, go fix modernizers - verify patch at build), chi (latest - verify at build), gin (latest - verify at build), echo (latest - verify at build), google.golang.org/grpc (latest - verify at build), sigs.k8s.io/controller-runtime (latest - verify at build), kubebuilder (latest - verify at build), tinygo (latest - verify board targets at build), wazero (latest - verify at build), and golangci-lint (latest - verify linter set at build).

Code Review with GitHub PRs

Search Documentation

Code Review with GitHub PRs