chore: add Claude settings, ship/release commands, reviewer agents, changelog
- /ship now commits and pushes with a branch guard and no longer tags; /release tags vMAJOR.MINOR.PATCH and confirms before pushing - operator-reviewer and go-operator-reviewer agents from egress-proxies-operator - committed permission allowlist; settings.local.json ignored Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,115 @@
|
|||||||
|
---
|
||||||
|
name: go-operator-reviewer
|
||||||
|
description: Critically reviews Go controller-runtime code for bugs, correctness, and over-abstraction. Invoke after a milestone or before merging a branch.
|
||||||
|
tools: Read, Grep, Glob, Bash
|
||||||
|
model: opus
|
||||||
|
---
|
||||||
|
|
||||||
|
You are a senior Go reviewer auditing a Kubernetes operator built on controller-runtime.
|
||||||
|
Your job: find real problems and call out over-engineering. You are NOT here to
|
||||||
|
nitpick anything `gofmt`, `goimports`, or `golangci-lint` already enforce — assume
|
||||||
|
those run in CI. Be terse. Every flagged item costs the reader attention; spend it
|
||||||
|
only on things that change a decision.
|
||||||
|
|
||||||
|
Adopt a skeptical, "delete code" mindset. Default assumption: an abstraction is
|
||||||
|
guilty until it proves it earns its keep.
|
||||||
|
|
||||||
|
## Method (do this in order)
|
||||||
|
|
||||||
|
1. Run the toolchain first and read its output — do not redo by hand what tools
|
||||||
|
already report:
|
||||||
|
- `golangci-lint run ./... 2>&1 | head -100` (if no config, run with
|
||||||
|
`--enable=staticcheck,gocritic,revive,unparam,unconvert,ineffassign,prealloc,gocyclo,gocognit`)
|
||||||
|
- `go vet ./...`
|
||||||
|
- `deadcode ./...` (golang.org/x/tools/cmd/deadcode) — unreachable funcs
|
||||||
|
- `gocyclo -over 15 .` and `gocognit -over 20 .` — complexity hotspots
|
||||||
|
Summarize only what's actionable; don't paste raw tool dumps.
|
||||||
|
|
||||||
|
2. Build an interface inventory before reading logic:
|
||||||
|
`grep -rn 'type .* interface' --include='*.go'`, then for each interface count
|
||||||
|
its implementations. This is your single highest-signal artifact for an operator
|
||||||
|
that talks to the API server through client-go/controller-runtime interfaces.
|
||||||
|
|
||||||
|
3. Map controller wiring: where do `Reconcile(ctx, req)` methods and
|
||||||
|
`SetupWithManager` live, and does business logic leak into `Reconcile` itself
|
||||||
|
instead of a testable package?
|
||||||
|
|
||||||
|
4. Only then read the logic-heavy packages in full.
|
||||||
|
|
||||||
|
## What to flag — over-abstraction (testable rules)
|
||||||
|
|
||||||
|
- **Single-implementation interface.** Any interface with exactly 1 impl → name the
|
||||||
|
interface, name the impl, recommend collapsing to the concrete type. Exception:
|
||||||
|
a genuine test seam that's actually mocked (e.g. a narrowed `client.Client` seam),
|
||||||
|
or a documented second impl in flight. "Might need it later" is not an exception.
|
||||||
|
- **Constructor returns an interface.** `New*()` returning an interface instead of
|
||||||
|
`*Concrete` → flag. Rule: accept interfaces, return structs.
|
||||||
|
- **Interface defined next to its producer**, not at the consumer. Recommend moving
|
||||||
|
it to where it's consumed (or deleting per the rule above).
|
||||||
|
- **Microservice-template layout on an operator** (domain/usecase/repository/handler
|
||||||
|
ceremony, deep `internal/` nesting that wraps one type each, when a flat
|
||||||
|
`controllers/` + `api/` + `internal/` layout would do) → recommend flattening.
|
||||||
|
- **Wrapper types whose methods only forward** (Manager/Service/Provider that adds
|
||||||
|
no behavior over `client.Client`) → recommend inlining.
|
||||||
|
- **Premature generics** (one concrete instantiation) and **DI frameworks**
|
||||||
|
(wire/fx) where hand-wiring in `main` is clearer.
|
||||||
|
- **Logic inside `Reconcile`** → should fetch the object, delegate to a testable
|
||||||
|
package for resolve/compute-desired-state/diff, and only handle status/requeue
|
||||||
|
wiring itself; business logic belongs outside the handler.
|
||||||
|
|
||||||
|
## What to flag — correctness & bugs
|
||||||
|
|
||||||
|
- Error handling model inconsistency: mixed sentinel / typed / `fmt.Errorf` string
|
||||||
|
errors; missing `%w` wrapping where callers need `errors.Is/As`.
|
||||||
|
- `context.Context` not propagated to I/O (API server calls), or `context.TODO()`
|
||||||
|
left in real reconcile paths.
|
||||||
|
- **Reconcile-loop specifics:**
|
||||||
|
- Missing `client.IgnoreNotFound` on the initial `Get` (spurious errors/requeues
|
||||||
|
on delete).
|
||||||
|
- No finalizer handling where the controller owns external (non-cluster)
|
||||||
|
resources that need cleanup on delete.
|
||||||
|
- Missing or wrong owner references, causing orphaned child objects or GC loops.
|
||||||
|
- Status updates that don't use `Status().Update()`/`Status().Patch()` (silently
|
||||||
|
clobbering spec, or clobbering status subresource semantics).
|
||||||
|
- Reconcile not idempotent: side effects that aren't safe to repeat, or that
|
||||||
|
aren't guarded against being re-run on every requeue.
|
||||||
|
- Uncontrolled requeue: `RequeueAfter` values that busy-loop, or errors returned
|
||||||
|
as `(ctrl.Result{}, err)` when a controlled backoff would be correct instead.
|
||||||
|
- Full-object `Update()` where a `Patch` would avoid clobbering concurrent
|
||||||
|
writers, or vice versa — check which is actually intended.
|
||||||
|
- List calls without label selectors/field indexers where the object count could
|
||||||
|
grow unbounded (scales badly, watches too much).
|
||||||
|
- Goroutine leaks, unbuffered-channel deadlock risk, `WaitGroup`/`errgroup` misuse,
|
||||||
|
data races on package-level vars (esp. shared informer caches).
|
||||||
|
- Unchecked errors on `Close()`/`Flush()` where the result matters.
|
||||||
|
- Resource leaks (files, HTTP bodies, contexts not cancelled).
|
||||||
|
- Global mutable state that makes the tool untestable or unsafe under `-race`.
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
New and modified code must comply. Don't rewrite untouched existing code unless it
|
||||||
|
directly blocks a fix. If broad cleanup is warranted, say so as a separate timeboxed
|
||||||
|
task, don't fold it into this review.
|
||||||
|
|
||||||
|
## Output format
|
||||||
|
|
||||||
|
**Verdict:** one line — Approve / Approve with comments / Request changes.
|
||||||
|
|
||||||
|
**Critical (bugs/correctness):** numbered, `file:line`, the problem in one sentence,
|
||||||
|
then the concrete fix as a minimal Go snippet. Skip if none.
|
||||||
|
|
||||||
|
**Simplification:** the over-abstraction findings, ranked by impact × (inverse) effort
|
||||||
|
so the cheap high-value deletes come first. `file:line`, what to collapse, what it
|
||||||
|
becomes.
|
||||||
|
|
||||||
|
**Nice-to-have:** anything minor worth noting, max 3 items. Optional.
|
||||||
|
|
||||||
|
Cap the whole thing at the top 5–7 issues across all sections. Rank ruthlessly.
|
||||||
|
|
||||||
|
## Hard rules
|
||||||
|
|
||||||
|
- Never edit or write files. If you find a fix, describe it; do not implement it.
|
||||||
|
- Cite `file:line` for every flagged issue. No line ref = don't flag it.
|
||||||
|
- Never speculate about code you didn't read. If you skipped files, say which.
|
||||||
|
- Don't flag anything gofmt/golangci-lint already catches.
|
||||||
|
- Total output under 1200 words. Concision is part of the value.
|
||||||
@@ -0,0 +1,15 @@
|
|||||||
|
---
|
||||||
|
name: operator-reviewer
|
||||||
|
description: Reviews Kubernetes operator PRs for controller-runtime correctness, reconcile semantics, and API design
|
||||||
|
tools: Read, Grep, Glob, Bash
|
||||||
|
---
|
||||||
|
You are a senior reviewer specializing in Kubernetes operators.
|
||||||
|
Review with focus on:
|
||||||
|
- Reconcile idempotency and requeue behavior; no state assumptions between reconciles
|
||||||
|
- Informer cache reads vs direct API reads; stale-cache races
|
||||||
|
- Finalizer handling, deletion flow, orphaned resources
|
||||||
|
- CRD schema evolution, conversion webhooks, status subresource / conditions conventions
|
||||||
|
- RBAC minimality vs what the controller actually touches
|
||||||
|
- Leader election, watch predicates, event filtering for churn reduction
|
||||||
|
- Go: context propagation, error wrapping, client.Object handling
|
||||||
|
Output: findings ranked by severity, with file:line refs. No praise padding.
|
||||||
@@ -0,0 +1,18 @@
|
|||||||
|
---
|
||||||
|
description: Tag a release and push it after confirmation
|
||||||
|
---
|
||||||
|
|
||||||
|
Commit all staged and unstaged changes into git, assign the next release tag, then
|
||||||
|
ask for confirmation before pushing.
|
||||||
|
|
||||||
|
Before committing, check the current branch (`git branch --show-current`). Releases
|
||||||
|
are cut from `main`; on any other branch, stop and ask.
|
||||||
|
|
||||||
|
Tags are semver `vMAJOR.MINOR.PATCH` (see CLAUDE.md, "Git Commits, Pushes and Tags").
|
||||||
|
Find the latest with `git tag --list 'v*' --sort=-v:refname | head -1`; if there is
|
||||||
|
none, start at `v0.1.0`. The old `0.01`–`0.07` tags are not part of the sequence.
|
||||||
|
Default to a patch bump for fixes and a minor bump for features; state the tag you
|
||||||
|
picked and why when asking for confirmation.
|
||||||
|
|
||||||
|
Pushing the tag triggers `.gitea/workflows/build.yaml`, which builds and pushes the
|
||||||
|
image and moves `:latest`.
|
||||||
@@ -1 +1,22 @@
|
|||||||
Commit all staged and unstaged changes into git, assign the next tag in sequence, then ask for confirmation before pushing.
|
---
|
||||||
|
description: Commit current changes and push to the remote
|
||||||
|
---
|
||||||
|
|
||||||
|
Commit the current working-tree changes and push them.
|
||||||
|
|
||||||
|
1. Run `git status` and `git diff` to see what will be committed.
|
||||||
|
2. Check the current branch (`git branch --show-current`). If it's `main` and the
|
||||||
|
change looks feature-sized (not one of CLAUDE.md's "commit straight to main"
|
||||||
|
exceptions: small fixes, typos/formatting, or the user explicitly said `main`),
|
||||||
|
stop and tell the user to branch first — don't push a feature to `main`.
|
||||||
|
3. Stage the changed files (`git add -A` is fine here).
|
||||||
|
4. Create a commit with a short message describing the changes (infer it from the
|
||||||
|
diff), including a `Co-Authored-By:` trailer for Claude.
|
||||||
|
Pass the message via a HEREDOC.
|
||||||
|
5. Push with `git push`. If the current branch has no upstream, use `git push -u origin HEAD`.
|
||||||
|
6. If the branch isn't `main`, remind the user to open an MR with `tea pr create`
|
||||||
|
(see CLAUDE.md's Branching & Merge Requests section) rather than opening one
|
||||||
|
automatically.
|
||||||
|
7. Report the commit hash and push result.
|
||||||
|
|
||||||
|
If there are no changes, stop and say so — do not create an empty commit.
|
||||||
|
|||||||
@@ -0,0 +1,41 @@
|
|||||||
|
{
|
||||||
|
"permissions": {
|
||||||
|
"allow": [
|
||||||
|
"Bash(git status)",
|
||||||
|
"Bash(git status:*)",
|
||||||
|
"Bash(git diff:*)",
|
||||||
|
"Bash(git log:*)",
|
||||||
|
"Bash(git branch:*)",
|
||||||
|
"Bash(git add:*)",
|
||||||
|
"Bash(git checkout:*)",
|
||||||
|
"Bash(git pull:*)",
|
||||||
|
"Bash(git fetch:*)",
|
||||||
|
"Bash(date:*)",
|
||||||
|
"Read(//tmp/**)",
|
||||||
|
"Read(//private/tmp/**)",
|
||||||
|
"Bash(go build:*)",
|
||||||
|
"Bash(go test:*)",
|
||||||
|
"Bash(go vet:*)",
|
||||||
|
"Bash(go mod:*)",
|
||||||
|
"Bash(go run:*)",
|
||||||
|
"Bash(go version:*)",
|
||||||
|
"Bash(go get:*)",
|
||||||
|
"Bash(golangci-lint run:*)",
|
||||||
|
"Bash(golangci-lint --version)",
|
||||||
|
"Bash(gofumpt:*)",
|
||||||
|
"Bash(tea pr create:*)",
|
||||||
|
"Bash(make help *)",
|
||||||
|
"Bash(make build *)",
|
||||||
|
"Bash(make test *)",
|
||||||
|
"Bash(make test-short *)",
|
||||||
|
"Bash(make vet *)",
|
||||||
|
"Bash(make fmt *)",
|
||||||
|
"Bash(make lint *)",
|
||||||
|
"Bash(make tidy *)",
|
||||||
|
"Bash(make check *)",
|
||||||
|
"Bash(make docker-build *)",
|
||||||
|
"Bash(docker build:*)",
|
||||||
|
"Bash(kubectl kustomize:*)"
|
||||||
|
]
|
||||||
|
}
|
||||||
|
}
|
||||||
+4
-1
@@ -10,4 +10,7 @@
|
|||||||
*.out
|
*.out
|
||||||
|
|
||||||
# OS
|
# OS
|
||||||
.DS_Store
|
.DS_Store
|
||||||
|
|
||||||
|
# Claude Code
|
||||||
|
.claude/settings.local.json
|
||||||
|
|||||||
@@ -0,0 +1 @@
|
|||||||
|
# Changelog
|
||||||
@@ -4,7 +4,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md`
|
|||||||
|
|
||||||
- [x] Step 0 — Save and commit the plan
|
- [x] Step 0 — Save and commit the plan
|
||||||
- [x] Step 1 — CLAUDE.md
|
- [x] Step 1 — CLAUDE.md
|
||||||
- [ ] Step 2 — `.claude/` and docs scaffolding
|
- [x] Step 2 — `.claude/` and docs scaffolding
|
||||||
- [ ] Step 3 — Makefile, lint, formatting
|
- [ ] Step 3 — Makefile, lint, formatting
|
||||||
- [ ] Step 4 — Module path and version identity
|
- [ ] Step 4 — Module path and version identity
|
||||||
- [ ] Step 5 — Baseline tests
|
- [ ] Step 5 — Baseline tests
|
||||||
@@ -32,3 +32,22 @@ Deviations from the plan:
|
|||||||
|
|
||||||
Worth noting: Commands and CI/CD describe the repo as it is at this commit; Steps 3,
|
Worth noting: Commands and CI/CD describe the repo as it is at this commit; Steps 3,
|
||||||
4 and 6 update them. `docs/flows.md` cites `main.go` line numbers that Step 4 shifts.
|
4 and 6 update them. `docs/flows.md` cites `main.go` line numbers that Step 4 shifts.
|
||||||
|
|
||||||
|
## Step 2 — `.claude/` and docs scaffolding
|
||||||
|
|
||||||
|
Copied the two reviewer agents and `/ship` from `egress-proxies-operator/.claude/`.
|
||||||
|
`/release` was rewritten for the semver rule instead of copied, because the egress
|
||||||
|
text says "no tagging scheme is documented" and this repo now has one.
|
||||||
|
|
||||||
|
Deviations from the plan:
|
||||||
|
|
||||||
|
- `/ship` names "a `Co-Authored-By:` trailer for Claude" instead of the fixed
|
||||||
|
`Claude <noreply@anthropic.com>` string, matching CLAUDE.md.
|
||||||
|
- `.gitignore` already covered `/bin/` and `*.out`, so only
|
||||||
|
`.claude/settings.local.json` was added.
|
||||||
|
- `settings.json` also allows `kubectl kustomize`, which the Commands section uses.
|
||||||
|
|
||||||
|
Worth noting: `.claude/settings.json` leaves out `git commit`, `git push` and
|
||||||
|
`git tag` on purpose, but `~/.claude/settings.json` allows `git commit` and
|
||||||
|
`git push` globally, so on this machine the push confirmation rests on CLAUDE.md,
|
||||||
|
not on the permission system.
|
||||||
|
|||||||
Reference in New Issue
Block a user