feat: refresh Go standards to a hard 1.26.4+ baseline; widen go-layout; maintainable lint pin - #2
Conversation
…g guidelines - Added new review dimensions to `go-reviewer`: silent dispatch defaults, sensitive-value echo in errors/logs, and comment–code drift. - Introduced fail-loudly-on-impossible-dispatch and boundary-errors-carry-classification-not-payload rules in `go-errors`. - Updated `go-testing` to clarify that golden files pin shape, not behavior, and emphasized the need for live-execution tests alongside them. These updates improve the robustness of Go code reviews and error handling practices.
…ent stdlib Each of these skills predated stdlib APIs the Go 1.26 baseline already implies, so they taught the older form by omission. go-errors: prefer `errors.AsType[E]` over `errors.As` (generic target, no pointer to prepare, cannot panic on a mistyped target; `errorsastype` converts call sites); `%w` goes last unless the sentinel is the sentence; check `Close` on written files via `errors.Join` into a named result — a bare `defer f.Close()` hides a failed flush behind an apparently successful write; keep the happy path unindented; never let a panic cross a package boundary. go-testing: `t.Context`, `t.ArtifactDir` vs `t.TempDir`, `t.Output`/`t.Attr`, and the footgun that ties them together — `t.Setenv`, `t.Chdir` and `cryptotest.SetGlobalRandom` are process-global, so they fail under `t.Parallel` or a parallel ancestor. Plus failure messages that carry call/input/got/want, and helpers that set up while the test body asserts. go-concurrency: cancellation causes (`WithCancelCause` + `context.Cause`, `WithTimeoutCause`) so an expiring layer names itself instead of reporting a bare `context.DeadlineExceeded`; `context.WithoutCancel` for work outliving a request; `context.AfterFunc`; prefer-synchronous-APIs; and explicit cleanup over `runtime.AddCleanup`/`SetFinalizer`. Every rule cites go.dev, pkg.go.dev, Code Review Comments or the Google style guide; version annotations verified against the "added in go1.NN" markers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The skill claimed "advice == tooling" but never said which analyzer owns which rewrite, so a reader could not attribute a change, look up its behaviour, or tell the auto-fixed rules from the ones only review catches. Adds a Fixer column naming the `modernize` analyzer per row, sourced from the per-fixer docs in x/tools, with `—` where no fixer exists. `go tool fix help` is the way to see what the installed toolchain actually ships (in golangci-lint the whole set is the single `modernize` linter). New rows for idioms the table was missing: `any`, `errorsastype`, `omitzero` (which corrects a real trap — `omitempty` does nothing for a struct-typed field, so a zero `time.Time` still marshals), `testingcontext`, `stringsseq`, `slicesbackward`, `reflecttypefor`. Adds a "no fixer will do it for you" section for the modern-but-unfixable: `os.OpenRoot` for caller-supplied paths (replacing the `filepath.Join` plus manual `..` checks that traversal bugs keep coming from), `crypto/rand.Text` for tokens, nil slices over empty literals, and sorted map iteration for stable output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The set claimed "Go 1.26, works with 1.25+" and then hedged every modern rule against it: "on Go 1.26+ modules prefer errors.AsType", "check go.mod first — don't apply a Go 1.26 idiom to a repo pinned to 1.25 or older", "or golangci-lint on older toolchains". That fallback branch costs a clause in almost every rule and buys nothing for anyone actually on 1.26.4+. Drops the hedging from go-coding, go-idioms, go-errors (in the preceding commit), go-explain, the go-reviewer baseline and its modernization-debt dimension, rules/go-context.mdc, README.md and docs/install.md. Modern forms are now stated flat. Keeps the version annotations — the `Since` column and the inline "(Go 1.24)" tags. Those are provenance, not gates: they explain why older code looks different and what an older module would have to bump to. go-idioms says so explicitly where the go.mod-gating instruction used to be, and go-explain still names the version an idiom landed in without hedging the recommendation. Two single-line spillovers ride along because they share a line with a floor edit: the go-coding and go-context.mdc router tables gain their layout/naming/API-surface row here, which the next commit is about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in the set owned the bulk of Go Code Review Comments: naming, doc comments, and the shape of an exported signature. go-idioms is scoped to the `modernize` analyzers, go-layout to directory structure, and the reviewer had no dimension for it — so an initialism like `userId`, a `GetName()` accessor, or an in-band `-1` error passed without comment. go-layout widens from "project layout" to layout + naming + API surface: - Naming: initialism casing, MixedCaps over MAX_LENGTH, name length tracking scope, receiver names consistent across a type, no `Get` prefix, `<pkg>test` doubles named for behaviour. - Signatures: receiver type (pointer when it mutates, is large, or holds a sync field), pass small values directly, no in-band errors, named results only when they add information, option struct vs variadic options chosen by how often callers pass options, accept interfaces / return concrete types, useful zero value. - Doc comments: full sentence starting with the name, package comment placement, the gofmt-formatted syntax, documenting what the signature can't say (concurrency safety, ownership, cancellation), `Deprecated:` over deletion. go-reviewer gains the matching "exported-surface & naming slips" dimension and names `revive` for the mechanically-checkable half; the discarded `Close` on a written file joins the resource-leak dimension. The router and Cursor rule route there (their table rows landed with the preceding commit). Also drops two `1.26+ modules` hedges the hard-floor commit missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…version pin Two gaps. The v2 coverage stopped at the schema headline, so the mechanics a migration actually needs were missing; and the pin advice named `v2.11.4` as a bare fact with no rationale, which had already gone stale against the v2.12.x line and gave nobody a way to decide when to move. v2 mechanics: `golangci-lint migrate` (in-place, keeps a `.bck` backup, drops comments) instead of hand-porting; the `issues.exclude-rules` → `linters.exclusions.rules` and `linters-settings` → `linters.settings` / `formatters.settings` moves; `golangci-lint fmt` for the formatters section; and `//nolint:<linter> // reason` discipline over a bare `//nolint`. Version pin, replacing the hardcoded release with a policy: pin an exact version in exactly one source of truth (the action's `version:` input, which also caches), never `latest` — upstream's reasoning is that a release can retune linters and redden every build at once with nothing in the diff to blame — and let Renovate/Dependabot raise the bump as its own reviewable PR, so the pin stays current without drifting silently. On bump: `--fix` first, then land the leftovers or add an exclusion with a reason. Also records upstream's warning that `go install`/`go get` and `tool` directives "aren't guaranteed to work", since they compile against whatever local Go version is around. Enables `usetesting` (pushes `os.Setenv`/`os.Chdir`/`context.Background` in tests to the `t.*` forms, pairing with the go-testing rules) and `nolintlint` (enforces the suppression discipline above) across all three copies of the reference config. A build too old to know a linter name is a signal to bump the pin, not to delete the line — the config header says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-grounding the skills against current Go practice took a full pass over go.dev, pkg.go.dev, Code Review Comments, the Google style guide and the modernize/golangci-lint docs — none of which was written down, so the next refresh would have to rediscover both the sources and the traps. docs/authoring.md gains "Refreshing the standards baseline (source registry)": the sources in three tiers (normative → style guides → enforcing tools), each with what it settles, plus a procedure. The rules that cost the most to learn: - The released Go version is not whatever `go.dev/doc/go1.NN` renders — that page exists in draft for months. Check the release history first; unreleased guidance goes in as one italic, explicitly labelled sentence, never as a rule. - The baseline is a hard floor: state the modern form flat, keep version annotations as provenance, delete guidance below the floor when it moves. - Verify every version gate against pkg.go.dev's "added in go1.NN" marker before writing a `Since` cell. - Re-check tool names — a renamed fixer turns a rule into a wrong command (`waitgroup` → `waitgroupgo`). - Never hardcode a tool version in a component; carry the pin policy instead. - Keep the three copies of the reference lint config in sync. AGENTS.md points at it for refresh requests, so "refresh the Go skills" is enough to re-enter the procedure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ute slash skills Two grounded rules the set was missing, plus a routing gap: go-layout: `main` owns process exit — `os.Exit`/`log.Fatal` only in `main`, ideally once on the error from a `run() error` function; a deep `log.Fatal` skips deferred cleanup and makes the path untestable. `init()` is restricted to cheap, deterministic setup — no I/O, no environment reads, no global mutation; anything more is an explicit constructor called from `main`. (Uber: Exit in Main, Avoid init(); Google: program initialization.) go-errors: keep wrap context terse — `"new store: %w"`, not `"failed to create new store: %w"`; "failed to" states the obvious and piles up as the error climbs the stack. (Uber: Error Wrapping.) go-coding: the router now names `/go-explain` and `/go-lint-setup` — the two user-invoked skills were unreachable from the routing surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audit against the plugin-dev skill-development and agent-development
checklists; three classes of drift, none behavioural:
go-reviewer description: 313 words of <example> blocks living in always-on
context. Converted to the prose-trigger format — conditions, typical triggers,
"not for", pointer to the body — with the worked scenarios moved to a
"When to invoke" section that only loads when the agent is dispatched.
Always-on cost drops by more than half; no trigger scenario is lost.
Skill descriptions: go-coding had grown to 111 words and go-explain to 80
against the repo's ~50-75-word always-on budget (docs/authoring.md). Trimmed to
77 each — the router keeps its skill list, since that is the routing map the
harness dispatches on, and drops the tool enumeration the body already owns.
Imperative form: swept the second-person phrasing out of six skill bodies
("or you get sentinel breakage" → "or the result is sentinel breakage",
"where you can" → "where possible", "your API contract" → "the API contract",
heading "no fixer will do it for you" → "no fixer automates it"). Agent bodies
correctly remain second person — that is the system-prompt convention.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sebastian-iancu
left a comment
There was a problem hiding this comment.
Consolidated review (Go 1.26 digest + PR #2 tip)
Reviewed against the branch tip (d14b776), the PR summary, CI (validate ✅), and an earlier gap analysis vs Go 1.26 release notes / gofix blog under the Go 1.26.4+ hard floor policy. Verified fixer names locally with go version go1.26.4 → go tool fix help.
Verdict
Strong, shippable direction: hard floor, stdlib grounding (AsType, ArtifactDir, context causes, …), widened go-layout, lint pin policy, Fixer-column idea, authoring registry, and the plugin-dev description trim are all the right moves. No new components needed.
Before merge, please land the Must fix items — they break the plugin’s “advice == tooling” invariant on the declared minimum toolchain.
Must fix
-
go-idiomsFixer column disagrees with Go 1.26.4go tool fix help
On a 1.26.4 host the registered modernizers includewaitgroup,fmtappendf,newexpr,omitzero,testingcontext,stringsseq, … — and do not include names the table currently teaches as baseline:- table:
waitgroupgo→ toolchain:waitgroup(the skill’s own 1.27 footnote already says the rename is 1.27) - table:
atomictypes,slicesbackward→ not in 1.26.4go fix(again called out as 1.27 in the footnote) - table:
errorsastype,bloop→ also absent from 1.26.4go tool fix help
Recommendation: re-audit the Fixer column againstgo tool fix helpon 1.26.4; use—(review-only) or “golangci/x/tools ahead of toolchain” where appropriate; keep 1.27 renames/additions only in the italic forward note. Optionally add a Prefer row forfmt.Appendf/fmtappendf, which does ship on 1.26.4.
- table:
-
exhaustivetaught but not shipped
go-errors+go-reviewertell agents to pin enum exhaustiveness with theexhaustivelinter;references/golangci.v2.yml/go-linting//go-lint-setupnever enable it. Either enable it in all three YAML copies, or stop naming it (completeness-test only). Prefer enable — advice == tooling.
Should fix (hard-floor consistency)
-
go-explainstill hedges on oldergo.mod
Body still says tailor down “if the repo'sgo.modactually says older,” which contradicts the hard-floor commit / CHANGELOG claim that per-idiom hedging was removed from this skill. Keep “name the landing version as provenance”; drop the tailor-down branch. -
Router / teaching order still lead with
errors.Asgo-codingerrors cell:errors.Is/As— should surfaceAsTypego-errorsstill teaches “Inspect with …errors.As” then “PreferAsType” — under a 1.26.4 floor, lead withAsTypefor typed extraction; keeperrors.Asonly for the narrow cases it still uniquely covers
-
CHANGELOG Unreleased hygiene
- Two consecutive
### Addedsections — merge into one Keep a Changelog group - “preferred … on 1.26+ modules” wording is pre–hard-floor; say preferred flat
- Changed bullet claiming
go-lintingnotes Cadasto pinv2.11.4vs upstreamv2.12.x— that note is not in the skill (pin policy replaced it); delete or rewrite the bullet - Section title still says “no fixer will do it for you” in one Added bullet while the skill heading is now “no fixer automates it”
- Two consecutive
-
references/golangci.v2.ymlheader still says keep in sync with/go-lint-setuponly; authoring procedure requires three copies including thego-lintinginline block.
Nice to have (non-blocking)
- Short
go fixrecipe ingo-idioms(from the gofix blog): clean tree →go fix -diff ./...→ apply; selective-name/-name=false; multi-GOOS/GOARCHwhen build tags matter.go tool fix helpis already cited. - Drop the
synctest.Run/ pre-1.25 GOEXPERIMENT digression ingo-testing— unsupported-toolchain trivia under a 1.26.4 floor; keepsynctest.Testonly. - Optional polish if you want a follow-up:
slog.NewMultiHandler; one-line cryptotest note that many crypto APIs ignore customio.Readerrand; concrete early-return channel-leak example ingo-concurrency. - Description SDO is much better after
d14b776; further slimming the router description is optional.
Explicit non-asks
- Do not reintroduce 1.25/1.24 compatibility tracks or per-idiom
go.modgates. - Do not add skills for Green Tea GC / SIMD /
runtime/secret/ HPKE — out of coding-standards scope. - Architecture (router + focused skills + slash skills +
go-reviewer+ rule + hooks) looks right; no new component.
Happy to re-review once the Fixer column and exhaustive mismatch are aligned.
|
Review addressed in Must fix — both valid, both landed:
Should fix — all landed: go-explain hedge dropped (3); Nice to have: 7 done (recipe, verified flags only — the blog's multi- Validators green on both commits; the three lint-config copies verified identical. 🤖 Generated with Claude Code |
Addresses the two must-fix items from the PR #2 review — both verified against the actual go1.26.4 toolchain on this host, not the review text alone. Fixer column vs `go tool fix help` (go1.26.4): the table taught five names the declared minimum toolchain does not register — `waitgroupgo` (1.26 name is `waitgroup`; the rename is 1.27), `errorsastype`, `bloop`, `atomictypes`, `slicesbackward` — plus `appendclipped`, which the review did not flag. New notation: plain = registered in 1.26.4 `go fix`; † = only in the newer x/tools modernize suite (golangci-lint/gopls) so far; `—` = review-only. The 1.27 forward note now reads as "† fixers graduate into the toolchain". Added the `fmt.Appendf`/`fmtappendf` row (ships in 1.26.4, fixer dropped in 1.27) and a verified `go fix` recipe: `-diff` preview on a clean tree, then apply, with per-fixer `-<name>` / `-<name>=false` selection (flags confirmed via `go help fix` — the review's `-name` spelling was close but not exact). exhaustive: taught by go-errors and go-reviewer, shipped by none of the three config copies — enabled in references/golangci.v2.yml, the go-linting block, and /go-lint-setup, restoring advice == tooling. Also from the review: typed-error teaching now leads with `errors.AsType` (single merged bullet in go-errors; `errors.As` kept as not-deprecated with the † tooling caveat), the router/Cursor-rule errors cell says `Is`/`AsType` and runs `--enable-only=errorlint,exhaustive`, and the yml header names both inlined copies it must stay in sync with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…view
go-explain: drop the "tailor down if go.mod says older" branch — the last
per-idiom hedge, contradicting the hard-floor commit's own claim. Landing
versions stay as provenance for the reader, not a gate on the recommendation.
go-testing: trim the synctest.Run digression to a one-line guard ("always
synctest.Test — the pre-stable synctest.Run no longer exists"); the guard stays
because models trained on pre-1.25 code still emit the removed API.
CHANGELOG [Unreleased]: merge the duplicate "### Added" groups; delete the
stale bullet claiming go-linting notes the v2.11.4-vs-v2.12.x pin (the pin
policy replaced that note before it ever shipped); "preferred on 1.26+
modules" → preferred flat; section title updated to "no fixer automates it";
the go-idioms bullet now describes the †-audited Fixer column and recipe, and
the lint-config bullet includes exhaustive.
Not taken from the review (explicitly): slog.NewMultiHandler / cryptotest
io.Reader note / channel-leak worked example (nice-to-have 9) and further
router-description slimming (10) — left for a follow-up if wanted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sebastian-iancu
left a comment
There was a problem hiding this comment.
Re-review (4d4316a)
Checked commits 2384684 + 4d4316a against the prior Must/Should list, re-verified plain Fixer names with go1.26.4 go tool fix help, and confirmed CI validate is green.
Prior findings — cleared
| Item | Status |
|---|---|
Fixer column vs 1.26.4 go tool fix help |
Fixed — waitgroup, fmtappendf, † notation for errorsastype/bloop/atomictypes/slicesbackward, recipe uses -<fixer> |
exhaustive in all three YAML copies |
Fixed |
go-explain hard-floor hedge |
Fixed |
Router / go-errors lead with AsType |
Fixed |
| CHANGELOG Unreleased hygiene | Fixed (single Added; stale pin bullet gone; wording updated) |
| YAML header names both inlined copies | Fixed |
go fix -diff recipe |
Fixed |
synctest.Run digression |
Trimmed appropriately |
The † distinction (toolchain go fix vs newer x/tools/modernize) is the right framing for a 1.26.4 hard floor. Nice-to-haves (NewMultiHandler / cryptotest rand-arg / channel-leak example / further SDO trim) correctly deferred.
Optional nits (non-blocking)
go-errorsdeterministic backstop still says--enable-only=errorlintonly; the router/Cursor rule now useerrorlint,exhaustive. Worth aligning the skill’s opening backstop (and optionally the description’s “Pair with …”) so agents don’t missexhaustivewhen they opengo-errorsdirectly.- Prefer row still lists
slices.Collectalongside fixersslicescontains/slicessort/mapsloop— Collect itself has no 1.26.4 fixer; harmless, or drop Collect from that cell for pedantic accuracy.
LGTM — ready to merge from my side (prior Must/Should all addressed).
…ols by running them Two guardrails so the PR #2 review findings become structural checks instead of reviewer vigilance. scripts/validate.py gains an advice == tooling cross-check: every linter a component teaches — named in a `--enable-only=...` command or in the phrase "the `<name>` linter" (patterns kept deliberately narrow so a passing mention is not flagged; the phrase regex tolerates hard-wrapped lines) — must be enabled in references/golangci.v2.yml or belong to the v2 `standard` set. Verified both ways: the current tree passes, and re-creating the exact `exhaustive` drift the review caught fails with all four teaching sites named. docs/authoring.md: the refresh procedure gains "run the tool, don't read about it" — on a floor-version toolchain, `go tool fix help` settles fixer names in one command, and `golangci-lint help linters` on the pinned build does the same for linters. The Tier 3 registry row that called pkg.go.dev's modernize page "the authority for the Fixer column" is corrected to what the review proved: that page tracks x/tools tip, so it sources the † rows and is never evidence that a fixer ships in `go fix`; the floor toolchain's own listing is the authority for plain rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n CI Completes the guardrail pair from the PR #2 review response: the linter half was text-vs-text and already enforced; the fixer half needed the toolchain itself, so validate.py now runs `go tool fix help` when a Go toolchain at the floor minor (GO_FLOOR_MINOR = 1.26) is on PATH and checks the go-idioms Fixer column both ways — plain names must be registered in `go fix`, † names must NOT be (a registered † fixer means the marker went stale after a toolchain bump). Any other Go minor soft-skips with a note: another minor's fixer list proves nothing about the floor. Locally the check is opportunistic (no Go → note + skip, keeping the stdlib-only soft-skip philosophy); CI installs Go 1.26.x via setup-go so it runs strictly there. The floor minor now lives in three pinned places — the script constant, the workflow's setup-go version, and the documented baseline — and docs/authoring.md says to move them together. Tested all four paths on go1.26.4: clean pass (21 fixer cells verified), plain-name-not-registered fails (re-created the original `waitgroupgo` bug), stale-† fails (marked shipped `newexpr` as †), and the no-Go skip path notes itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both manifests to 0.4.0, AGENTS.md status line synced, and the accumulated [Unreleased] notes folded into `## [0.4.0] - 2026-08-05`. The v0.4.0 tag goes on the merge commit once PR #2 lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
A grounded refresh of the standards set against current Go practice (verified against go.dev release notes, pkg.go.dev "added in" markers, Code Review Comments, and the Google/Uber style guides — never from memory), plus an authoring-convention pass driven by the plugin-dev skill/agent checklists.
Standards content
go-errors,go-testing,go-concurrencygain the APIs that landed after they were written:errors.AsType[E],%w-last placement,Close-into-named-result on write paths, panic-never-crosses-a-package-boundary;t.Context/t.ArtifactDir/t.Output/t.Attrand the process-global helpers that cannot run undert.Parallel; cancellation causes (WithCancelCause+context.Cause),context.WithoutCancel,AfterFunc,runtime.AddCleanup.go-idiomsFixer column — every row names themodernizeanalyzer that owns the rewrite (—where review has to catch it); new rows (any,errorsastype,omitzero,testingcontext,stringsseq, …) and a "no fixer automates it" section (os.OpenRoot,crypto/rand.Text, nil slices, sorted map iteration).go-layoutwidened to layout + naming + API surface: initialism casing, receiver rules, no in-band errors, option struct vs variadic options, doc comments, exit-in-main /init()discipline.go-reviewergains the matching review dimension (revivenamed for the mechanical half).goroutineleakprofile going on-by-default).Policy changes (release-noteworthy)
go.modgating removed; version annotations stay as provenance. User-facing for marketplace installs → suggests a 0.4.0 minor bump at release.v2.11.4(already stale vs v2.12.x) is replaced with a pin policy: exact version, one source of truth (actionversion:input), automated bump PRs,--fix-then-triage on bump; plus upstream's warning againstgo install/tool-directive installs. Reference config gainsusetesting+nolintlintin all three copies.Maintainability
docs/authoring.mdgains a refresh source registry — three tiers of sources with what each settles, and the procedure (released-version check, hard-floor rule, pkg.go.dev version-gate verification, no hardcoded tool versions).AGENTS.mdpoints at it, so "refresh the Go skills" is a repeatable request.go-reviewer's 313-word<example>-block description converted to prose triggers + a "When to invoke" body section (always-on cost more than halved); over-budget skill descriptions trimmed; second-person phrasing swept out of skill bodies.Every commit validates independently (
./scripts/validate.sh+claude plugin validate .), so the series is bisectable.Test plan
./scripts/validate.shgreen on every commitclaude plugin validate .green on every commitreferences/golangci.v2.yml,go-linting,/go-lint-setup)claude plugin add .) and spot-check triggering ofgo-layouton a naming question🤖 Generated with Claude Code