94 lines
3.5 KiB
Markdown
94 lines
3.5 KiB
Markdown
---
|
|
name: review-and-commit
|
|
description: Review and Commit
|
|
disable-model-invocation: true
|
|
---
|
|
|
|
# Review and Commit
|
|
|
|
Perform the following steps in order. Stop and report if any step fails.
|
|
|
|
## 1. Run Code Checks
|
|
|
|
Run these in parallel where possible:
|
|
|
|
- `task lint` (buf format + buf lint + gofmt + goimports + go vet)
|
|
- `task generate` then verify no diff in generated files (`git diff --exit-code gen/ api/openapi.yaml`)
|
|
|
|
Fix any issues found. Re-run checks after fixes.
|
|
|
|
## 2. Run Tests with Coverage
|
|
|
|
Run tests uncached with verbose output and coverage:
|
|
|
|
```
|
|
go test -count=1 -v -coverprofile=coverage.out ./...
|
|
```
|
|
|
|
Then analyze coverage:
|
|
|
|
```
|
|
go tool cover -func=coverage.out
|
|
```
|
|
|
|
### Coverage analysis
|
|
|
|
1. **Baseline**: before the review, capture coverage on the base branch (or use the most recent known coverage if available). If no baseline exists, use the current run as the initial baseline.
|
|
2. **Compare**: check total coverage and per-package coverage for packages that contain changed files.
|
|
3. **Thresholds**:
|
|
- If total coverage drops by **2 or more percentage points**, suggest adding tests and ask for confirmation before committing. Do not block the commit.
|
|
- If a changed package has **0% coverage**, mention it in the report unless the package is purely generated code or contains only types/constants.
|
|
4. **Report**: include a short coverage summary in the findings (total %, per-changed-package %, and delta if baseline is available).
|
|
|
|
Clean up `coverage.out` after analysis.
|
|
|
|
## 3. Review Changed Go Files
|
|
|
|
For each `.go` file in the diff (excluding `gen/`), review against:
|
|
|
|
### Go Code Review Comments (go.dev/wiki/CodeReviewComments)
|
|
|
|
- Comment sentences: doc comments are full sentences starting with the name, ending with a period.
|
|
- Contexts: `context.Context` as first param, never stored in structs.
|
|
- Error strings: lowercase, no trailing punctuation.
|
|
- Handle errors: no discarded errors with `_`.
|
|
- Imports: grouped (stdlib, external, internal), no unnecessary renames.
|
|
- Indent error flow: normal path at minimal indentation, error handling first.
|
|
- Initialisms: `ID`, `URL`, `HTTP` — consistent casing.
|
|
- Interfaces: defined where used, not where implemented.
|
|
- Named result parameters: only when they clarify meaning.
|
|
- Receiver names: short, consistent, never `this`/`self`.
|
|
- Variable names: short for narrow scope, descriptive for wide scope.
|
|
|
|
### Effective Go (go.dev/doc/effective_go)
|
|
|
|
- Zero value useful: structs should be usable without explicit init.
|
|
- Don't panic: use error returns, not panic, for normal error handling.
|
|
- Goroutine lifetimes: clear when/whether goroutines exit.
|
|
- Embedding: prefer embedding over forwarding methods when appropriate.
|
|
- Concurrency: share memory by communicating, not vice versa.
|
|
|
|
### Go Proverbs (go-proverbs.github.io)
|
|
|
|
- The bigger the interface, the weaker the abstraction.
|
|
- A little copying is better than a little dependency.
|
|
- Clear is better than clever.
|
|
- Errors are values — handle them, don't just check them.
|
|
- Don't panic.
|
|
- Make the zero value useful.
|
|
- Design the architecture, name the components, document the details.
|
|
|
|
## 4. Report Findings
|
|
|
|
Present findings grouped by severity:
|
|
|
|
- **Must fix**: violations that would break conventions, correctness, or backwards compatibility.
|
|
- **Should fix**: style, clarity, or maintainability issues.
|
|
- **Nit**: minor suggestions, optional.
|
|
|
|
Apply must-fix and should-fix changes (ask if unsure about any). Re-run checks.
|
|
|
|
## 5. Commit
|
|
|
|
After all checks pass and review issues are resolved, create a commit following the repository's commit conventions. Do not push unless asked.
|