From 2be69b9aace7a259327dd5312317fccd90c4e900 Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Thu, 8 Oct 2026 20:31:26 +0200 Subject: [PATCH] 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 --- .claude/agents/go-operator-reviewer.md | 115 ++++++++++++++++++ .claude/agents/operator-reviewer.md | 15 +++ .claude/commands/release.md | 18 +++ .claude/commands/ship.md | 23 +++- .claude/settings.json | 41 +++++++ .gitignore | 5 +- CHANGELOG.md | 1 + .../2026-10-08-2028-house-conventions.md | 21 +++- docs/reviews/.gitkeep | 0 9 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 .claude/agents/go-operator-reviewer.md create mode 100644 .claude/agents/operator-reviewer.md create mode 100644 .claude/commands/release.md create mode 100644 .claude/settings.json create mode 100644 CHANGELOG.md create mode 100644 docs/reviews/.gitkeep diff --git a/.claude/agents/go-operator-reviewer.md b/.claude/agents/go-operator-reviewer.md new file mode 100644 index 0000000..0d3ba04 --- /dev/null +++ b/.claude/agents/go-operator-reviewer.md @@ -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. diff --git a/.claude/agents/operator-reviewer.md b/.claude/agents/operator-reviewer.md new file mode 100644 index 0000000..326097a --- /dev/null +++ b/.claude/agents/operator-reviewer.md @@ -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. \ No newline at end of file diff --git a/.claude/commands/release.md b/.claude/commands/release.md new file mode 100644 index 0000000..caadc6f --- /dev/null +++ b/.claude/commands/release.md @@ -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`. diff --git a/.claude/commands/ship.md b/.claude/commands/ship.md index acfdbc6..f67a8e1 100644 --- a/.claude/commands/ship.md +++ b/.claude/commands/ship.md @@ -1 +1,22 @@ -Commit all staged and unstaged changes into git, assign the next tag in sequence, then ask for confirmation before pushing. \ No newline at end of file +--- +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. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..6a0c283 --- /dev/null +++ b/.claude/settings.json @@ -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:*)" + ] + } +} diff --git a/.gitignore b/.gitignore index 907f0fc..634a238 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,7 @@ *.out # OS -.DS_Store \ No newline at end of file +.DS_Store + +# Claude Code +.claude/settings.local.json diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..825c32f --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1 @@ +# Changelog diff --git a/docs/plans-executions/2026-10-08-2028-house-conventions.md b/docs/plans-executions/2026-10-08-2028-house-conventions.md index d07411f..d2344fb 100644 --- a/docs/plans-executions/2026-10-08-2028-house-conventions.md +++ b/docs/plans-executions/2026-10-08-2028-house-conventions.md @@ -4,7 +4,7 @@ Plan: `docs/plans/2026-10-08-2028-house-conventions.md` - [x] Step 0 — Save and commit the plan - [x] Step 1 — CLAUDE.md -- [ ] Step 2 — `.claude/` and docs scaffolding +- [x] Step 2 — `.claude/` and docs scaffolding - [ ] Step 3 — Makefile, lint, formatting - [ ] Step 4 — Module path and version identity - [ ] 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, 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 ` 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. diff --git a/docs/reviews/.gitkeep b/docs/reviews/.gitkeep new file mode 100644 index 0000000..e69de29