diff --git a/.github/workflows/links.yml b/.github/workflows/links.yml new file mode 100644 index 0000000..7e007bb --- /dev/null +++ b/.github/workflows/links.yml @@ -0,0 +1,29 @@ +name: links + +# Every URL a skill, agent, rule, or doc cites must still resolve — a moved page is a hole in a +# rule's provenance. Needs the network, so it runs apart from `validate`: weekly, on demand, and on +# pull requests that touch the files that carry citations or the checker/workflow that resolves them. +on: + schedule: + - cron: '0 6 * * 1' + workflow_dispatch: + pull_request: + paths: + - 'skills/**' + - 'agents/**' + - 'rules/**' + - 'docs/**' + - 'README.md' + - 'AGENTS.md' + - 'scripts/validate.py' + - '.github/workflows/links.yml' + +jobs: + links: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.x' + - run: python3 scripts/validate.py --check-links diff --git a/AGENTS.md b/AGENTS.md index 2ce8cee..3a5f123 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,8 +17,9 @@ This plugin encodes **Go (golang) coding standards**. Guidance must be grounded - **Style references** (cite these when a rule depends on them): - **Effective Go** — - **Go Code Review Comments** — - - **Google Go Style Guide** — + - **Google Go Style Guide** — — cite the document a rule comes from: the *Guide* (normative and canonical; the ordered principles), *Style Decisions* (normative; the reviewer rulebook), *Best Practices* (advisory) - **Uber Go Style Guide** — + - **Linter rule catalogues** — name the rule when a skill says a tool catches something: `go vet` , staticcheck , revive - **Standard library & toolchain** — package docs at ; modules, `go test`, table-driven tests, and the race detector (`go test -race`) are the baseline testing conventions. When a recommendation derives from one of the above, attribute it explicitly and distinguish cited rules from inference. @@ -39,7 +40,7 @@ This repo supports **both Claude Code and Cursor**. Shared assets (skills, comma - **Cursor hooks**: `hooks/cursor-hooks.json` — object `{ "hooks": { "sessionStart": [...], "afterFileEdit": [...] } }`; the command runs from the plugin root (a **workspace-relative** path, **not** `${CLAUDE_PLUGIN_ROOT}`). Present — wires `session-start.sh` (`sessionStart`) and `format-on-save.sh` + `skill-nudge.sh` (`afterFileEdit`). - **Shared hook scripts**: `hooks/session-start.sh` — detects `go.mod` / `*.go`, prints one Go-standards context line, exits 0 always. `hooks/format-on-save.sh` — after a `*.go` Write/Edit, runs `gofumpt -w` (or `gofmt -w -s`) on that single file; resolves the path from `$CLAUDE_FILE_PATH` or the stdin tool-payload JSON, host-only, silent no-op if no formatter is installed, exits 0 always. `hooks/skill-nudge.sh` — after a `*.go` Write/Edit, names ONE matching go-coding skill for that edit, once per skill per session; delivered as a hook `systemMessage` under Claude Code, a plain line under Cursor; exits 0 always. All three host-agnostic so either manifest can invoke them. Present. - **MCP config** *(optional, not present)*: `.mcp.json` — only if the plugin later integrates an MCP server. There is no companion MCP server today; do not reference one. -- **Validation**: `scripts/validate.sh` wraps `scripts/validate.py` to check both manifests, dual-host parity, declared component paths, kebab-case names, hook-config JSON, skill/command/agent frontmatter (**agents must use `tools:` not `allowed-tools:`** — flagged as an error), hook parity (the same `hooks/*.sh` wired for the equivalent event on both hosts, each one existing and executable, none left unwired), doc component inventories (every shipped skill, agent and hook named in `README.md`, this file, and `docs/testing.md`; hooks alone in `docs/install.md`), and two *advice == tooling* invariants: every linter taught in a component is enabled in `references/golangci.v2.yml`, and (when a floor-minor Go toolchain is on PATH — CI's matrix installs `1.26.x` and `1.27.x`; the strict check runs on the 1.26.x (floor) leg, the 1.27.x leg soft-skips it) the `go-idioms` Fixer column matches `go tool fix help`. The Python is stdlib-only. `.github/workflows/validate.yml` pins Python + Go and runs the validator strictly. +- **Validation**: `scripts/validate.sh` wraps `scripts/validate.py` to check both manifests, dual-host parity, declared component paths, kebab-case names, hook-config JSON, skill/command/agent frontmatter (**agents must use `tools:` not `allowed-tools:`** — flagged as an error), hook parity (the same `hooks/*.sh` wired for the equivalent event on both hosts, each one existing and executable, none left unwired), doc component inventories (every shipped skill, agent and hook named in `README.md`, this file, and `docs/testing.md`; hooks alone in `docs/install.md`), tie-break parity (the Google readability tie-break sentence reads identically in the `go-coding` router, `rules/go-context.mdc`, and `go-reviewer`), and two *advice == tooling* invariants: every linter taught in a component is enabled in `references/golangci.v2.yml` (analyzers a skill teaches as opt-in — `shadow` in `go-idioms` — are the deliberate exception: each is taught with the config line that switches it on, and is not added to the reference config at a refresh), and (when a floor-minor Go toolchain is on PATH — CI's matrix installs `1.26.x` and `1.27.x`; the strict check runs on the 1.26.x (floor) leg, the 1.27.x leg soft-skips it) the `go-idioms` Fixer column matches `go tool fix help`. The Python is stdlib-only. `.github/workflows/validate.yml` pins Python + Go and runs the validator strictly. - **Contributor docs**: `docs/` for human-facing references — `install.md`, `testing.md`, `versioning.md`, `authoring.md`. `.github/` holds issue + PR templates, `copilot-instructions.md`, and the CI workflow. (Planning and research working notes are kept locally under `docs/`, **gitignored** — not part of the published plugin.) ### Component surface diff --git a/CHANGELOG.md b/CHANGELOG.md index 069b284..e52e146 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - Keep a Changelog: https://keepachangelog.com/en/1.1.0/ - Semantic Versioning: https://semver.org/spec/v2.0.0.html +## [Unreleased] + +### Added +- Skills: `go-layout` — imports in groups with the standard library first, blank imports only in `main` or a test (the `embed` package under `//go:embed` excepted; a justifying comment is revive's alternate in a library), no dot imports (`revive` `blank-imports`/`dot-imports`), and field names in struct literals of types from other packages (`go vet` `composites`). +- Skills: `go-errors` — `MustX` helpers are for package initialisation from constant inputs or a test helper that `t.Fatal`s, never for input that can fail. +- Skills: `go-testing` — `Example` functions with `// Output:` as runnable documentation, advised where feasible rather than one per export (`go vet` `tests`), field names in table-case literals, and comparing stable results rather than serialised bytes or map order. +- Skills: `go-idioms` — a nested `:=` that shadows `err` or `ctx` (the `shadow` analyzer, opt-in), and the redundant `break` at the end of a `switch` case (staticcheck S1023). +- Skills: `go-coding`, `rules/go-context.mdc` — the tie-break order for two valid forms: clarity, simplicity (with least mechanism), concision, maintainability, consistency (Google Go Style Guide). +- Agents: `go-reviewer` — import and literal hygiene and test-fragility dimensions, a shadowed `err` and a `Must` helper on a request path under error swallowing, and the same tie-break order for style findings. +- Scripts: `validate.py --check-links` verifies every cited URL resolves (HEAD, then GET; one retry on a transport error or a 429/503), and `--selftest` exercises that policy against a local server with no network; `.github/workflows/links.yml` runs the live check weekly and on pull requests touching skills, agents, rules, docs, the checker, or the workflow. +- Scripts: `validate.py` checks that the tie-break sentence is identical in the `go-coding` router, the Cursor rule, and `go-reviewer`. + +### Changed +- References: the source registry names Google's three documents by weight (Guide, Style Decisions, Best Practices), adds the linter rule catalogues to Tier 3, and records the revision read for each mutable source. +- Docs: `AGENTS.md` and `README.md` follow the registry. + ## [0.5.0] - 2026-09-04 Makes the `go-coding` router route. A usage analysis of local session transcripts found the router diff --git a/README.md b/README.md index 93abf40..0d57df9 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ Or load a local working copy for a single session: `claude --plugin-dir /path/to | Cursor rule `go-context.mdc` | shipped | `**/*.go`-scoped guidance mirroring the router for Cursor. | | Scripts `scripts/hooks-test.sh`, `scripts/usage-report.py` | shipped | Dev tooling, not part of the installed component surface: a bash test harness for the hooks, and a stdlib-only adoption-report generator over local session transcripts. | -Guidance is grounded in authoritative sources — [Effective Go](https://go.dev/doc/effective_go), [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments), the [Google](https://google.github.io/styleguide/go/) and [Uber](https://github.com/uber-go/guide) style guides — and the standard toolchain (`gofmt`/`gofumpt`, `go vet`, `staticcheck`, `golangci-lint`, `go test -race`). +Guidance is grounded in authoritative sources — [Effective Go](https://go.dev/doc/effective_go), [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments), the [Google Go Style Guide](https://google.github.io/styleguide/go/) (its *Guide*, *Style Decisions*, and *Best Practices*) and the [Uber Go Style Guide](https://github.com/uber-go/guide) — and the standard toolchain (`gofmt`/`gofumpt`, `go vet`, `staticcheck`, `golangci-lint`, `go test -race`). ## Using with subagent orchestrators diff --git a/agents/go-reviewer.md b/agents/go-reviewer.md index df781da..dfbf2cd 100644 --- a/agents/go-reviewer.md +++ b/agents/go-reviewer.md @@ -71,7 +71,9 @@ the judgment a linter cannot — the bugs and smells that survive `gofmt`, `go v - **Silent error swallowing** — `_ = f()` on an error that matters; empty `if err != nil {}`; `%v` where `%w` was needed (breaks downstream `errors.Is`/`errors.As`); returning `nil` after logging a - real failure. + real failure; a nested `:=` that shadows `err` so the outer check sees nil (`go-idioms`); + a `MustX` helper — panic on failure — called on a request path or on input the program does not + control (`go-errors`). - **Goroutine leaks / lifetime** — a goroutine with no exit path; a channel send/recv after the counterparty has returned; workers not tied to a `context` or done signal; `wg.Add`/`Done` mismatch (prefer `wg.Go`). @@ -115,11 +117,25 @@ the judgment a linter cannot — the bugs and smells that survive `gofmt`, `go v `go fix ./...` or `golangci-lint run --enable-only=modernize`. - **slog hot-path waste** — building a per-call logger instead of `logger.With(...)`; formatting or allocating before a level check; key-value variadic on a hot path instead of `slog.LogAttrs`. +- **Import and literal hygiene** — `import .`; `import _` in a library package with neither a + `//go:embed` use of `embed` nor a comment justifying the side effect; a positional struct literal + of a type from another package. `revive` (`blank-imports`, `dot-imports`) and `go vet` + (`composites`) catch all three — name the rule (`go-layout`). +- **Test fragility** — a test comparing serialised bytes or formatted text from a package the repo + does not own, or map-derived output without sorting; `t.Fatal` called from a goroutine the test + started; a table whose long positional case literals have to be decoded against the struct + (`go-testing`). For the *why* and citations behind any dimension, the `go-errors`, `go-concurrency`, `go-testing`, `go-idioms`, `go-lint-setup`, and `go-layout` skills carry the grounded rules — reference them rather than re-deriving from memory. +When a finding is about which of two valid forms to prefer and the tools accept both, rank by the +order Google's Go Style Guide gives — clarity, then simplicity (with its rule of least mechanism: +the most standard tool that expresses the idea), then concision, then maintainability, then +consistency — and say which attribute decided it (). A +style preference with no attribute behind it is not a finding. + ## Output format Lead with a one-line verdict, then findings highest-severity first: diff --git a/docs/authoring.md b/docs/authoring.md index 0cff9f8..ab8772f 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -64,10 +64,21 @@ citation. Everything the skills assert should be traceable to one of these. | Go Code Review Comments | | the review-rule catalogue (naming, errors, concurrency, API shape) | | Doc comment syntax | | `gofmt`-formatted doc comments, doc links | -**Tier 2 — style guides (attribute when a rule comes from one)** - -- Google Go Style Guide — (esp. `/best-practices`: naming, - error handling, panics, option structs, documentation, test structure) +**Tier 2 — style guides (attribute when a rule comes from one, and name the document)** + +- Google Go Style Guide — three documents of different weight, ranked by Google itself; a citation + names which one: + - the *Guide* — — **normative and canonical**: the five ordered readability principles + (clarity, simplicity, concision, maintainability, consistency) and, under simplicity, *least mechanism*. The + tie-break order the router, the Cursor rule and the reviewer use — one identical sentence in all + three, checked by `scripts/validate.py`. + - *Style Decisions* — — **normative, not canonical**: the reviewer rulebook — naming, + commentary, imports, errors, language, common libraries, useful test failures. The main Google + source for skill rules. + - *Best Practices* — — **advisory**: patterns with trade-offs (test doubles, option structs, + error structure, shadowing, table-test literals). + Google-internal guidance is not adopted: flag conventions, Google's own logging library and + verbosity levels, protocol-buffer stubs, and CLI library choices. - Uber Go Style Guide — **Tier 3 — the enforcing tools (this is what keeps "advice == tooling" true)** @@ -81,7 +92,24 @@ citation. Everything the skills assert should be traceable to one of these. - golangci-lint docs — · v1→v2 migration — · changelog (for the CI pin) — -- `go.dev/blog` for feature-specific posts (`synctest`, `testing-b-loop`, `slog`, `range-functions`) +- `go.dev/blog` for feature-specific posts (`synctest`, `testing-b-loop`, `slog`, `range-functions`, + `examples`) +- **Linter rule catalogues** — when a skill says a tool catches something, the rule id or name comes + from here, not from memory: `go vet` analyzers ; staticcheck checks + ; revive rules ; errorlint + ; gofumpt rules ; + the golangci-lint linters index + +**Revision record** — the mutable sources, as last read. A refresh diffs each against its recorded +revision first, so it reads what changed rather than everything, then updates this table. + +| Source | Revision read | Checked | +|---|---|---| +| Go Code Review Comments (`golang/wiki` mirror, `CodeReviewComments.md`) | `228ca0b` (2026-09-01) | 2026-09-09 | +| Google Go Style Guide (`google/styleguide`, `go/`) | `c098353` (2026-03-18) | 2026-09-09 | +| Uber Go Style Guide (`uber-go/guide`) | `1d60a91` (2026-04-15) | 2026-09-09 | +| revive rule descriptions (`mgechev/revive`, `RULES_DESCRIPTIONS.md`) | `803cd04` (2026-09-03) | 2026-09-09 | +| Effective Go, doc comment syntax, `cmd/vet`, package docs | versioned with the Go release — read at go1.27.1 | 2026-09-09 | **Procedure** @@ -105,16 +133,21 @@ citation. Everything the skills assert should be traceable to one of these. halves: every linter taught in components must be enabled in `references/golangci.v2.yml`, and — when a floor-minor Go toolchain is on PATH (CI's matrix installs both `1.26.x` and `1.27.x`; locally it soft-skips with a note) — the `go-idioms` Fixer column is verified against - `go tool fix help`: plain names must be registered, † names must not be. The floor minor - lives in `GO_FLOOR_MINOR` in the script and in the workflow's matrix floor entry (`1.26.x`) — move - all three (docs baseline included) together. + `go tool fix help`: plain names must be registered, † names must not be. An analyzer a skill + teaches as *opt-in* — `shadow` in `go-idioms` — is the deliberate exception to the first half: it + is taught together with the config line that switches it on, and is not added to the reference + config at a refresh. The floor minor lives in `GO_FLOOR_MINOR` in the script and in the + workflow's matrix floor entry (`1.26.x`) — move all three (docs baseline included) together. **Never hardcode a tool version in a component.** A named `golangci-lint` release rots within weeks and nobody remembers why it was chosen; the skills carry the *pin policy* (pin exactly, one source of truth, automated bump PR) plus the changelog URL, and let the consuming repo own the number. The same goes for `gopls`/`gofumpt` versions outside `docs/install.md`. 5. Keep the two copies of the reference lint config in sync: `references/golangci.v2.yml` and the scaffold block in `go-lint-setup`. -6. Record the refresh in **CHANGELOG.md** under `## [Unreleased]`. +6. Run `python3 scripts/validate.py --check-links` — every cited URL must still resolve; a moved + page is fixed in the same refresh. +7. Update the **Revision record** above, then record the refresh in **CHANGELOG.md** under + `## [Unreleased]`. ## Dual-host parity diff --git a/docs/testing.md b/docs/testing.md index 1c61483..1260f35 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -7,7 +7,8 @@ exercising the components. ## Validation - **Manifest / component validation** — `./scripts/validate.sh`: checks both `plugin.json` manifests, dual-host parity (name/version/description/author agree), declared component paths, kebab-case names, hook-config JSON, hook parity (the same `hooks/*.sh` wired for the equivalent event on both hosts, each existing and executable, none left unwired), doc component inventories (every shipped skill, agent and hook named where the docs claim to list them), and SKILL.md / agent / command frontmatter (including `name` == directory/filename, and that agents declare `tools:` not `allowed-tools:`). The wrapper runs `scripts/validate.py`; if Python 3 isn't installed it prints a warning and skips (exit 0) rather than failing — install `python3` for the full local check, or rely on `claude plugin validate .` and CI. CI pins Python and calls `python3 scripts/validate.py` directly, so the deep check can never silently skip there. -- **Validator self-test** — `python3 scripts/validate.py --selftest` (also run by CI): rebuilds each structural check's failure case in a temporary tree and requires the check to catch it, so a check that has quietly stopped checking cannot pass as green. +- **Validator self-test** — `python3 scripts/validate.py --selftest` (also run by CI): rebuilds each structural check's failure case in a temporary tree and requires the check to catch it, so a check that has quietly stopped checking cannot pass as green. It also exercises the link checker's fetch policy — HEAD then GET, one retry on a transport error or an HTTP 429/503, a 404 reported as broken — against a local HTTP server, with no network access. +- **Link check** — `python3 scripts/validate.py --check-links`: every URL cited in skills, agents, rules, and docs must resolve. Needs the network, so it is its own switch; CI runs it weekly and on pull requests that touch those files, the checker itself, or its workflow (`.github/workflows/links.yml`). - **Hook tests** — `./scripts/hooks-test.sh` (also run by CI on every PR): bash tests for all three hook scripts (`hooks/session-start.sh`, `hooks/format-on-save.sh`, `hooks/skill-nudge.sh`), including a can-fail self-test block that proves the negative-case helpers actually fail on bad input. - **Official validator** — `claude plugin validate .`: checks the manifest and component structure (no extra dependencies). - **Structural review** — run the `plugin-dev:plugin-validator` agent after creating or modifying components. diff --git a/rules/go-context.mdc b/rules/go-context.mdc index 32dbbde..68c19f6 100644 --- a/rules/go-context.mdc +++ b/rules/go-context.mdc @@ -21,7 +21,7 @@ This Cursor rule mirrors the `go-coding` router skill — apply it when editing | Errors | `golangci-lint run --enable-only=errorlint,exhaustive` | `go-errors` | | Concurrency | `go test -race ./...`, `go vet ./...` | `go-concurrency` | | Testing | `go test -race ./...`; `testing/synctest` for time/concurrency | `go-testing` | -| Layout, naming & API surface | `golangci-lint run --enable-only=revive`; rest is judgment | `go-layout` | +| Layout, naming & API surface | `golangci-lint run --enable-only=revive` (`blank-imports`, `dot-imports`), `go vet` (`composites`); rest is judgment | `go-layout` | ## Route, then load @@ -30,13 +30,13 @@ matching the change — it carries the cited rules and the judgment: - any `_test.go`, a benchmark, a fuzz target, a "verified by temporarily breaking it" claim → `go-testing` -- `fmt.Errorf`, `errors.*`, a sentinel, a typed error, a `switch` over an enum → `go-errors` -- a loop, map, slice, string split, `interface{}`, a struct literal that could be `new(expr)` → - `go-idioms` +- `fmt.Errorf`, `errors.*`, a sentinel, a typed error, a `Must` helper, a `switch` over an enum → `go-errors` +- a loop, map, slice, string split, `interface{}`, a struct literal that could be `new(expr)`, a + nested `:=` on `err` → `go-idioms` - `go func`, `chan`, `sync.`, `atomic.`, `errgroup`, `context.With*`, a `Close` on a goroutine-owned resource → `go-concurrency` -- a new package, an exported identifier, a `cmd/` or `internal/` decision, a doc comment on an API → - `go-layout` +- a new package, an exported identifier, a `cmd/` or `internal/` decision, an import block, a struct + literal of a foreign type, a doc comment on an API → `go-layout` - `.golangci.y*ml`, a linter complaint you do not understand → `go-lint-setup` ## Minimum checklist (when opening a skill is not affordable) @@ -58,8 +58,14 @@ first time it appears or leave it out. Keep it short: a few sentences per point, must make gets the options plus a recommendation. Identifiers, commands and linter names stay verbatim — it is the prose around them that must be plain. -Ground every judgment call in a cited source — Effective Go, Go Code Review Comments, the Google -and Uber Go style guides, `pkg.go.dev`. Don't invent rules. Adopt the shipped golangci-lint v2 +When two valid forms compete and the tools accept both, decide by the order Google's Go Style Guide +gives: clarity, then simplicity (with its rule of least mechanism: the most standard tool that +expresses the idea), then concision, then maintainability, then consistency. Say which attribute +decided it. + +Ground every judgment call in a cited source — Effective Go, Go Code Review Comments, the Google Go +Style Guide (cite the document: Guide, Style Decisions, or Best Practices), the Uber Go style guide, +`pkg.go.dev`, and the linter rule catalogues. Don't invent rules. Adopt the shipped golangci-lint v2 config (`references/golangci.v2.yml`, at the **plugin root** — search the installed plugin directory for it, not under this rule's own dir); run `golangci-lint run --fix` for auto-fixable findings, and read the rest. diff --git a/scripts/validate.py b/scripts/validate.py index fe255f0..26a6ecb 100644 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -24,15 +24,20 @@ * dual-host hook parity: the same ``hooks/*.sh`` wired for the equivalent event on both hosts, each wired script present and executable, and none left unwired; * doc component inventories: every shipped skill, agent and hook named in the docs that - claim to list them. One-directional, so tombstones for removed components stay legal. + claim to list them. One-directional, so tombstones for removed components stay legal; + * the Google tie-break sentence (clarity, then simplicity, then concision, then + maintainability, then consistency) is restated identically, verbatim, in every file + that carries it. Dependency-free (stdlib only) so the ``scripts/validate.sh`` soft-skip is the *only* reason it wouldn't run. Usage: - python3 scripts/validate.py # verify this tree - python3 scripts/validate.py --selftest # verify the checks themselves still catch things + python3 scripts/validate.py # verify this tree + python3 scripts/validate.py --selftest # verify the checks themselves still catch things + python3 scripts/validate.py --check-links # every URL cited in components and docs resolves (network) """ +import http.server import json import os import re @@ -40,6 +45,10 @@ import subprocess import sys import tempfile +import threading +import time +import urllib.error +import urllib.request from pathlib import Path ROOT = Path(__file__).resolve().parent.parent @@ -72,6 +81,14 @@ "docs/testing.md": ("skills", "agents", "hooks"), "docs/install.md": ("hooks",), } +# The Google Go Style Guide tie-break order, restated (not linked) in three independent files — +# it has already drifted once. Checked verbatim, after collapsing whitespace, since the sentence +# hard-wraps across lines at ~100 columns in each source file. +TIE_BREAK_SENTENCE = ( + "clarity, then simplicity (with its rule of least mechanism: the most standard tool that " + "expresses the idea), then concision, then maintainability, then consistency" +) +TIE_BREAK_FILES = ("skills/go-coding/SKILL.md", "rules/go-context.mdc", "agents/go-reviewer.md") def err(msg): @@ -381,6 +398,23 @@ def validate_doc_inventories(): f"its component inventory is stale") +def validate_tie_break_parity(): + """The Google tie-break sentence (clarity, then simplicity, then concision, then + maintainability, then consistency) is restated — not linked — in three independent files, so + an edit to one silently leaves the other two stating a different order. Collapse whitespace + runs to a single space (the sentence hard-wraps across lines at ~100 columns) and require + TIE_BREAK_SENTENCE verbatim; a paraphrase, a dropped clause, or a reordering fails the check.""" + for rel in TIE_BREAK_FILES: + path = ROOT / rel + if not path.is_file(): + err(f"{rel}: missing — cannot verify the Google tie-break sentence") + continue + collapsed = re.sub(r"\s+", " ", path.read_text()) + if TIE_BREAK_SENTENCE not in collapsed: + err(f"{rel}: does not carry the Google tie-break sentence verbatim — " + f"expected '{TIE_BREAK_SENTENCE}'") + + def main(): manifests = {} for subdir, label in ((".claude-plugin", "Claude manifest"), (".cursor-plugin", "Cursor manifest")): @@ -421,6 +455,196 @@ def main(): validate_linter_references() validate_fixer_column() validate_doc_inventories() + validate_tie_break_parity() + + +URL_RE = re.compile(r"https?://[^\s<>()\[\]`\"']+") +LINK_SOURCES = ("skills", "agents", "rules", "docs", "README.md", "AGENTS.md") + + +def collect_urls(): + """Every external URL cited in components and docs, with one file that cites it. Templated + URLs (a `go1.NN` placeholder, an angle-bracket slot) are skipped — they are patterns, not + links.""" + seen = {} + for src in LINK_SOURCES: + path = ROOT / src + files = [path] if path.is_file() else sorted(path.rglob("*.md")) + sorted(path.rglob("*.mdc")) + for f in files: + text = f.read_text() + for m in URL_RE.finditer(text): + url = m.group(0).rstrip(".,;:") + # A placeholder marks a pattern, not a link — skip it whole rather than checking a + # truncated prefix as if it were a citation. `{ver}` and `go1.NN` sit inside the + # match; `` does not, because the regex stops at `<`, so look at the character + # right after the match for that one. + if "NN" in url or "{" in url or text[m.end():m.end() + 1] == "<": + continue + seen.setdefault(url, f.relative_to(ROOT)) + return seen + + +# Delay before the one retry `resolve_url` grants a transport error or a 429/503. A module-level +# constant (rather than a literal) so `--selftest` can zero it for the fixture in run_selftest — +# the fetch-policy self-test would otherwise spend real seconds sleeping for no reason. +RETRY_DELAY = 1.0 + + +def _fetch(url: str, method: str) -> int: + req = urllib.request.Request(url, method=method, headers={"User-Agent": "go-coding-plugin-linkcheck/1"}) + with urllib.request.urlopen(req, timeout=20) as resp: + return resp.status + + +def resolve_url(url: str) -> tuple[int | None, str | None]: + """Resolve one URL: HEAD first, then GET whenever HEAD fails for any reason — some hosts + refuse or stall on HEAD, and a dead page fails both, so the retry costs nothing. Within a + method: a transport error (reset, timeout) is retried once after RETRY_DELAY, then falls + through to the next method; an HTTPError with code 429 or 503 is retried once after + RETRY_DELAY too — the server said "later", not "gone" — then falls through; any other + HTTPError is a definite answer for that method (no retry, straight to the next method). The + first status obtained, by either method, stops the search. Returns `(status, last_reason)`; + `status` is None if neither method ever returned one.""" + status, last = None, None + for method in ("HEAD", "GET"): + for attempt in (1, 2): + try: + status = _fetch(url, method) + break + except urllib.error.HTTPError as e: + last = f"HTTP {e.code}" + if e.code in (429, 503) and attempt == 1: + time.sleep(RETRY_DELAY) + continue + break # a definite answer for this method: no retry, try the other method + except Exception as e: # noqa: BLE001 — a reset or timeout is retried once, then GET + last = type(e).__name__ + if attempt == 1: + time.sleep(RETRY_DELAY) + continue + break + if status is not None: + break + return status, last + + +def check_links() -> int: + """Resolve every cited URL via resolve_url (HEAD then GET, retrying once on a transport error + or a 429/503 before falling through) and report the ones that never resolve. Needs the + network, so it is a separate switch and a separate CI job rather than part of the default + run — a moved page is a real defect in a rule's provenance, not a validation-time flake to + ignore.""" + urls = collect_urls() + broken = [] + for url, where in sorted(urls.items()): + status, last = resolve_url(url) + if status is None or status >= 400: + broken.append((url, where, last or f"HTTP {status}")) + for url, where, why in broken: + print(f"BROKEN {url} ({where}): {why}") + print(f"{'FAIL' if broken else 'OK'}: {len(urls) - len(broken)} of {len(urls)} cited URLs resolve") + return 1 if broken else 0 + + +class _FetchFixtureServer(http.server.ThreadingHTTPServer): + """A local, loopback-only HTTP server for the fetch-policy self-test. Threaded so a + connection deliberately left hanging (`/flaky`) cannot block a later request.""" + daemon_threads = True + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # First-hit counters for the two one-shot routes; every later hit on the same path + # behaves normally, so exactly one retry is what rescues each of them. + self.hits = {"flaky": 0, "busy": 0} + + def handle_error(self, request, client_address): + pass # /flaky deliberately drops the connection after closing wfile — the resulting + # "I/O operation on closed file" from the base handler's post-request flush is expected, + # not a real server error, so it must not print a traceback into --selftest output. + + +class _FetchFixtureHandler(http.server.BaseHTTPRequestHandler): + """Routes exercising resolve_url's policy without any network access: + /ok resolves on both methods; /head-refused fails HEAD but resolves on GET; /missing is a + hard 404 on both; /flaky closes the connection with no response on its first hit (any + method), then resolves — proving the one transport retry; /busy answers 503 on its first + hit, then resolves — proving the one 429/503 retry.""" + + def log_message(self, *_args): + pass # silent — --selftest output should not interleave with HTTP access logs + + def do_HEAD(self): + self._route("HEAD") + + def do_GET(self): + self._route("GET") + + def _route(self, method): + if self.path == "/ok": + self._reply(200) + elif self.path == "/head-refused": + self._reply(405 if method == "HEAD" else 200) + elif self.path == "/missing": + self._reply(404) + elif self.path == "/flaky": + if self.server.hits["flaky"] == 0: + self.server.hits["flaky"] += 1 + self.close_connection = True + self.wfile.close() + return + self._reply(200) + elif self.path == "/busy": + if self.server.hits["busy"] == 0: + self.server.hits["busy"] += 1 + self._reply(503) + else: + self._reply(200) + else: + self._reply(404) + + def _reply(self, code): + self.send_response(code) + self.end_headers() + + +def _selftest_fetch_policy() -> int: + """Exercise resolve_url's HEAD/GET fallback and retry policy against a local HTTP fixture — + `--selftest` must never touch the public network. Returns the number of mismatches (0 on + success), printed as one `ok` line or one `FAIL` line per mismatch.""" + global RETRY_DELAY + real_delay = RETRY_DELAY + RETRY_DELAY = 0 # the fixture's retries must not spend real seconds sleeping + server = _FetchFixtureServer(("127.0.0.1", 0), _FetchFixtureHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + base = f"http://127.0.0.1:{server.server_port}" + cases = ( + ("/ok", False), + ("/head-refused", False), + ("/flaky", False), + ("/busy", False), + ("/missing", True), + ) + mismatches = [] + for path, expect_broken in cases: + status, last = resolve_url(base + path) + broken = status is None or status >= 400 + if broken != expect_broken: + mismatches.append( + f"{path}: expected {'broken' if expect_broken else 'resolved'}, " + f"got status={status} last={last}") + if mismatches: + for mismatch in mismatches: + print(f"FAIL link fetch policy: {mismatch}") + else: + print("ok link fetch policy: HEAD/GET fallback, HEAD refusal, transport retry, " + "429/503 retry, and a hard 404 all resolve correctly against a local fixture") + return len(mismatches) + finally: + server.shutdown() + server.server_close() + RETRY_DELAY = real_delay SELFTEST_HOOKS_CLAUDE = {"hooks": {"PostToolUse": [{"matcher": "Write|Edit", "hooks": [ @@ -433,7 +657,9 @@ def _selftest_tree(root: Path, *, break_it=None): rather than copied from the repo so a self-test never passes because the real tree happens to be shaped a certain way.""" (root / "skills" / "go-thing").mkdir(parents=True) + (root / "skills" / "go-coding").mkdir() (root / "agents").mkdir() + (root / "rules").mkdir() (root / "hooks").mkdir() (root / "references").mkdir() (root / "docs").mkdir() @@ -443,6 +669,23 @@ def _selftest_tree(root: Path, *, break_it=None): f"---\nname: {name}\ndescription: {desc}\n---\n\nBody. " + ("Run `golangci-lint run --enable-only=nosuchlinter`.\n" if break_it == "taught_linter" else "\n")) + # A second, realistic skill/rule/agent trio carrying the Google tie-break sentence, so + # validate_tie_break_parity has the same three relative paths to read here as in the real + # tree. Independent of every other break_it variant: always clean unless break_it == + # "tie_break", and then only one of the three files drifts (concision/maintainability + # swapped) — proving the check catches a single-file drift, not just three blank files. + tie_break_ok = TIE_BREAK_SENTENCE + tie_break_bad = TIE_BREAK_SENTENCE.replace( + "then concision, then maintainability,", "then maintainability, then concision,") + (root / "skills" / "go-coding" / "SKILL.md").write_text( + f"---\nname: go-coding\ndescription: Go coding standards router\n---\n\n" + f"Tie-break order: {tie_break_ok}.\n") + (root / "rules" / "go-context.mdc").write_text( + f"---\ndescription: Go coding standards for Cursor\n---\n\n" + f"Tie-break order: {tie_break_ok}.\n") + (root / "agents" / "go-reviewer.md").write_text( + f"---\nname: go-reviewer\ndescription: reviews Go code\ntools: Read\n---\n\n" + f"Tie-break order: {tie_break_bad if break_it == 'tie_break' else tie_break_ok}.\n") tools = "allowed-tools:" if break_it == "agent_tools" else "tools:" (root / "agents" / "go-checker.md").write_text( f"---\nname: go-checker\ndescription: checks\n{tools} Read\n---\n\nBody.\n") @@ -459,9 +702,9 @@ def _selftest_tree(root: Path, *, break_it=None): (root / "references" / "golangci.v2.yml").write_text("linters:\n enable:\n - revive\n") inventory = "" if break_it == "doc_inventory" else "go-thing " for doc in ("README.md", "AGENTS.md"): - (root / doc).write_text(f"# Doc\n\n{inventory}go-checker a\n") + (root / doc).write_text(f"# Doc\n\n{inventory}go-coding go-reviewer go-checker a\n") for doc in ("testing.md", "install.md"): - (root / "docs" / doc).write_text(f"# Doc\n\n{inventory}go-checker a\n") + (root / "docs" / doc).write_text(f"# Doc\n\n{inventory}go-coding go-reviewer go-checker a\n") SELFTEST_CASES = ( @@ -474,6 +717,7 @@ def _selftest_tree(root: Path, *, break_it=None): ("agent declares allowed-tools", "agent_tools", (lambda: validate_md_components("agents", require_name=True, is_agent=True),)), ("taught linter not in the reference config", "taught_linter", (validate_linter_references,)), + ("Google tie-break sentence drifts between files", "tie_break", (validate_tie_break_parity,)), ) @@ -484,6 +728,21 @@ def run_selftest() -> int: same tree with the defect removed, so a check that always fires fails too.""" global ROOT, errors real_root, real_errors, failures = ROOT, errors, 0 + # The link collector: a templated URL is a pattern, not a link, and is skipped whole — never + # truncated at the placeholder and checked as a shorter, real-looking URL. + with tempfile.TemporaryDirectory() as tmp: + ROOT = Path(tmp) + (ROOT / "README.md").write_text( + "See . Docs at `https://pkg.go.dev/` and " + "`https://go.dev/doc/go1.NN`; config `https://example.com/{ver}/x`.\n") + got = set(collect_urls()) + ROOT = real_root + if got == {"https://example.com/a"}: + print("ok link collector skips templated URLs whole") + else: + print(f"FAIL link collector: collected {sorted(got)}") + failures += 1 + failures += _selftest_fetch_policy() for label, defect, checks in SELFTEST_CASES: outcomes = {} for variant, break_it in (("broken", defect), ("clean", None)): @@ -513,6 +772,8 @@ def run_selftest() -> int: if __name__ == "__main__": if "--selftest" in sys.argv[1:]: sys.exit(run_selftest()) + if "--check-links" in sys.argv[1:]: + sys.exit(check_links()) main() if errors: print(f"FAIL: {len(errors)} problem(s)") @@ -521,6 +782,7 @@ def run_selftest() -> int: sys.exit(1) print("OK: manifests, dual-host parity, component paths, kebab-case names, " "hook configs and hook parity, skills, agents, commands, rules, taught-linter " - "references, and doc component inventories are valid") + "references, doc component inventories, and Google tie-break sentence parity " + "are valid") for note in notes: print(f" note: {note}") diff --git a/skills/go-coding/SKILL.md b/skills/go-coding/SKILL.md index e24c9d7..538e163 100644 --- a/skills/go-coding/SKILL.md +++ b/skills/go-coding/SKILL.md @@ -23,7 +23,7 @@ Two principles from the project research drive it: | Errors (`%w`, `errors.Is`/`AsType`, `errors.Join`, sentinel/typed, enum dispatch) | `golangci-lint run --enable-only=errorlint,exhaustive` | `go-errors` | | Concurrency (goroutine leaks, ctx lifecycle, atomics) | `go test -race ./...`, `go vet ./...` | `go-concurrency` | | Testing (table-driven, `t.Parallel`, `t.Context`, `B.Loop`, `testing/synctest`) | `go test -race ./...`; use `testing/synctest` for time/concurrency tests | `go-testing` | -| Layout, naming & API surface (`internal/`, initialisms, receiver type, in-band errors, doc comments) | `golangci-lint run --enable-only=revive` (`var-naming`, `receiver-naming`, `exported`), `gofmt` for doc-comment layout; the rest is judgment | `go-layout` | +| Layout, naming & API surface (`internal/`, imports, initialisms, receiver type, in-band errors, struct literals, doc comments) | `golangci-lint run --enable-only=revive` (`var-naming`, `receiver-naming`, `exported`, `blank-imports`, `dot-imports`), `go vet` (`composites`), `gofmt` for doc-comment layout; the rest is judgment | `go-layout` | | Code intelligence (defs/refs/diagnostics/rename/vulncheck) | install the **`gopls-lsp`** plugin | — | Open the focused `go-*` skill for the topic — it carries the cited rules and the judgment; run the @@ -37,10 +37,10 @@ skill matching the change — with the Skill tool (`go-coding:go-errors`, …), | The diff touches… | Load | |---|---| | any `_test.go`, a benchmark, a fuzz target, a "verified by temporarily breaking it" claim | `go-testing` | -| `fmt.Errorf`, `errors.*`, a sentinel, a typed error, a `switch` over an enum | `go-errors` | -| a loop, map, slice, string split, `interface{}`, a struct literal that could be `new(expr)` | `go-idioms` | +| `fmt.Errorf`, `errors.*`, a sentinel, a typed error, a `Must` helper, a `switch` over an enum | `go-errors` | +| a loop, map, slice, string split, `interface{}`, a struct literal that could be `new(expr)`, a nested `:=` on `err` | `go-idioms` | | `go func`, `chan`, `sync.`, `atomic.`, `errgroup`, `context.With*`, a `Close` on a goroutine-owned resource | `go-concurrency` | -| a new package, an exported identifier, a `cmd/` or `internal/` decision, a doc comment on an API | `go-layout` | +| a new package, an exported identifier, a `cmd/` or `internal/` decision, an import block, a struct literal of a foreign type, a doc comment on an API | `go-layout` | | `.golangci.y*ml`, a linter complaint you do not understand | `go-lint-setup` | One load per skill per session is enough; the skill stays in context. Orchestrators dispatching @@ -59,6 +59,13 @@ Apply these even if you load nothing else; they are the rules the focused skills - `ctx` first; no goroutine without an owner that waits for it; `t.Context()` in tests. - Run `gofmt`/`gofumpt` and `golangci-lint run` — never reason out what a tool decides. +## Tie-breaks (when two valid forms compete) + +When both forms pass the tools, decide by the order Google's Go Style Guide gives for readable code: +clarity, then simplicity (with its rule of least mechanism: the most standard tool that expresses +the idea), then concision, then maintainability, then consistency. Say which attribute decided it; +"more idiomatic" on its own is not a reason. Source: (normative and canonical). + ## Writing for the human Anything a person reads — a PR description, a review comment, a question, a design choice put to @@ -72,9 +79,10 @@ names stay verbatim — it is the prose around them that must be plain. - Effective Go — - Go Code Review Comments — -- Google Go Style Guide — +- Google Go Style Guide — — three documents, cite the one a rule comes from: the *Guide* (, normative and canonical: the principles), *Style Decisions* (, normative: the reviewer rulebook), *Best Practices* (, advisory) - Uber Go Style Guide — - Package & toolchain docs — +- Linter rule catalogues (name the rule, not just the tool) — `go vet` ; staticcheck ; revive ; golangci-lint linters ## For a focused review diff --git a/skills/go-errors/SKILL.md b/skills/go-errors/SKILL.md index 5b29ceb..65c3d87 100644 --- a/skills/go-errors/SKILL.md +++ b/skills/go-errors/SKILL.md @@ -1,6 +1,6 @@ --- name: go-errors -description: Idiomatic Go error handling. This skill should be used when the user writes, reviews, or debugs Go error code — wrapping with `%w`, `errors.Is`/`errors.AsType`, sentinel vs typed errors, `errors.Join`, an unchecked `Close`, when panic is legitimate, enum-switch dispatch defaults, keeping payload values out of boundary errors and logs, or chasing a swallowed or context-losing error. Pair with the `errorlint` linter. Go only. +description: Idiomatic Go error handling. This skill should be used when the user writes, reviews, or debugs Go error code — wrapping with `%w`, `errors.Is`/`errors.AsType`, sentinel vs typed errors, `errors.Join`, an unchecked `Close`, when panic is legitimate, `Must` helpers, enum-switch dispatch defaults, keeping payload values out of boundary errors and logs, or chasing a swallowed or context-losing error. Pair with the `errorlint` linter. Go only. --- # go-errors — Go error handling @@ -41,6 +41,12 @@ Deterministic backstop: `golangci-lint run --enable-only=errorlint`, plus `errch plausibly hit; panic is for programmer error, API misuse, and genuinely unreachable states. If a package uses panic internally for unwinding, `recover` it inside that package and return an error — a panic must never escape into a caller. +- **`MustX` is for package initialisation and test helpers, not for input.** A helper that stops the + program on failure carries the `Must` prefix (`regexp.MustCompile`, `template.Must`) and is called + while setting up package-level values from constants the author controls; the same prefix fits a + test helper that stops only the current test with `t.Fatal` (`mustParse(t, s)`). Anything that can + fail on user input, a file, or the network returns an error instead — a `Must` on a request path + turns bad input into a crash. - **Fail loudly on impossible dispatch:** a `switch` over an internal enum/kind gets a `default` that returns an error (panic only for the genuinely unreachable) — never a silent pass-through that lets a later-added member ride the weakest arm. Pin exhaustiveness with the `exhaustive` @@ -58,7 +64,7 @@ Deterministic backstop: `golangci-lint run --enable-only=errorlint`, plus `errch ## Sources - Go 1.13 errors — ; `errors.AsType` (Go 1.26) — - Code Review Comments (Error Strings, Handle Errors, Indent Error Flow, Don't Panic) — -- Google Go Style Guide (Error Handling, Panics, `%w` placement) — +- Google Go Style Decisions (Must functions, Returning errors, Error strings, Handle errors, In-band errors, Don't panic) — ; Best Practices (Error handling, Panics, `%w` placement) — - Uber Go Style Guide (Errors) — - `os.File.Close` returns write errors — diff --git a/skills/go-idioms/SKILL.md b/skills/go-idioms/SKILL.md index 342b5f0..d3be8db 100644 --- a/skills/go-idioms/SKILL.md +++ b/skills/go-idioms/SKILL.md @@ -1,6 +1,6 @@ --- name: go-idioms -description: Modern idiomatic Go (the `modernize` analyzer set) — Go 1.26+, Go 1.27 additions noted. This skill should be used when a diff or question contains a rewritable construct, when the user asks to modernize Go or run `go fix`, or asks which fixer owns a rewrite — range-over-int, `min`/`max`, `slices`/`maps`, `strings.Cut`, `any` over `interface{}`, iterators, `omitzero` json tags, `os.Root`, `new(expr)`, `errors.AsType`, dropped loop-var copies, and the Go 1.27 additions (generic methods, json/v2-backed `encoding/json`, the `atomictypes`/`embedlit`/`slicesbackward`/`unsafefuncs` fixers). Advice equals tooling — `go fix ./...` or `golangci-lint --enable-only=modernize`. Not for linter configuration (go-lint-setup). Go only. +description: Modern idiomatic Go (`modernize`) — Go 1.26+, 1.27 additions noted. This skill should be used when a diff contains a rewritable construct, when the user asks to modernize Go or run `go fix`, or asks which fixer owns a rewrite — range-over-int, `min`/`max`, `slices`/`maps`, `strings.Cut`, `any`, iterators, `omitzero`, `os.Root`, `new(expr)`, `errors.AsType`, and Go 1.27's `atomictypes`, `embedlit`, `slicesbackward`, `unsafefuncs` — rewrites go to `go fix ./...` or `golangci-lint --enable-only=modernize`, a nested `:=` that shadows `err` to the opt-in `shadow` analyzer, a redundant `break` in a `switch` to staticcheck S1023. Not for linter configuration (go-lint-setup). Go only. --- # go-idioms — modern Go (modernize) @@ -76,6 +76,16 @@ something a modernizer rewrites. (e.g. marshalling `[]` vs `null`). - **`slices.Sorted(maps.Keys(m))`** (1.23) when iterating a map for output — map order is random, and unstable output is a flaky-test and noisy-diff source. +- **A nested `:=` can shadow `err` or `ctx`.** `if v, err := f(); err != nil { … }` inside a function + that already has an `err` declares a second one; the outer stays nil and a later `return err` + reports success. Use `=` to assign into the existing variable, or give the inner one another name. + Nothing in the standard set reports this; the `shadow` analyzer from `golang.org/x/tools` does + when enabled (in golangci-lint: `linters.settings.govet.enable: [shadow]`) — it is noisy on + legitimate reuse, so a repo enables it deliberately rather than by default. +- **No `break` at the end of a `switch` case.** Go cases do not fall through, so a `break` as the + last statement of a case is dead text; `staticcheck` S1023 (standard set) flags exactly that. + Earlier in a case a `break` still does work — it leaves the `switch` from that point — and a + labelled `break` leaves the enclosing loop instead; neither of those is dead text. *Go 1.27 (released 2026-08-19, ) graduates several † fixers into the toolchain's `go fix` (`atomictypes`, `slicesbackward`, plus new `embedlit` and `unsafefuncs`), renames `waitgroup` @@ -111,6 +121,7 @@ moves to 1.27, these are worth reaching for. - `slog` — ; Go 1.21–1.26 release notes (`new(expr)`, self-ref generics — ) - `os.Root` / `omitzero` / `rand.Text` — ; Code Review Comments (Declaring Empty Slices, Crypto Rand) — - Go 1.27 release notes (generic methods, `stdversion`, `encoding/json/v2`, new `go fix` modernizers) — +- Google Go Style Decisions (Switch and break, Nil slices) — ; Best Practices (Shadowing) — ; staticcheck S1023 — ; `shadow` analyzer — - `atomictypes` / `slicesbackward` / `embedlit` modernizer commits — , , --- diff --git a/skills/go-layout/SKILL.md b/skills/go-layout/SKILL.md index 8ae2b26..ddbf97a 100644 --- a/skills/go-layout/SKILL.md +++ b/skills/go-layout/SKILL.md @@ -1,6 +1,6 @@ --- name: go-layout -description: Go project layout, package design and API surface. This skill should be used when the user creates a new package or directory, adds or renames an exported identifier, decides between cmd/ and internal/, writes a doc comment on an exported API, or reviews a diff that adds a package or changes a public type or signature — also util/common grab-bags, start-flat-then-grow, receiver naming, initialisms, in-band error values, and hexagonal/DDD ceremony answered with the standard-library shape. Pair with revive (var-naming, receiver-naming, exported). Not for error handling (go-errors) or tests (go-testing). +description: Go project layout, package design and API surface. This skill should be used when the user creates a package, adds or renames an exported identifier, decides between cmd/ and internal/, writes a doc comment on an API, or reviews a diff that changes a public type or signature — also import grouping and blank or dot imports, struct literals of foreign types, receiver naming, initialisms, in-band errors, util/common grab-bags, and hexagonal/DDD ceremony. Pair with revive and `go vet` (composites). Not for error handling (go-errors) or tests (go-testing). --- # go-layout — layout, naming & API surface @@ -27,6 +27,14 @@ exported signature are part of the API — they are as reviewable as the code. explicit constructor called from `main`. - **Files:** one package per directory; `package foo` for `foo.go` + `foo_test.go`; use `package foo_test` for black-box tests that exercise only the exported API. +- **Imports in groups, standard library first,** then other modules, then side-effect imports — + `goimports`/`gofumpt` keep the groups. A blank import (`import _ "pkg"`) belongs in a `main` + package or a test that needs the side effect, not in a library, where it silently changes every + importer (Style Decisions, *Import "blank"*); the one library exception is `import _ "embed"` in a + file that uses the `//go:embed` directive. `revive` (`blank-imports`, default rule set) reports a + blank import outside `main` and test files unless a comment justifies it or it is that `embed` + case, so a deliberate library blank import carries a comment saying why. Never `import .` — it + hides where a name comes from; `revive` (`dot-imports`, default) flags it. ## Naming @@ -71,6 +79,11 @@ exported signature are part of the API — they are as reviewable as the code. - **Prefer synchronous signatures** — let the caller add concurrency (→ `go-concurrency`). - **Make the zero value useful where possible** (`bytes.Buffer`, `sync.Mutex` need no constructor). If a type genuinely requires a `New…`, the doc comment must say so. +- **Name the fields in a struct literal of a type from another package** — + `csv.Reader{Comma: ',', Comment: '#'}`, never positional. The owner may add or reorder fields; a + positional literal then breaks, or worse still compiles with the values in the wrong slots. `go vet` + (`composites`) flags it. Positional stays fine for a small type of the current package, whose + definition sits beside the use. ## Doc comments @@ -89,7 +102,8 @@ exported signature are part of the API — they are as reviewable as the code. ## Sources - Effective Go — - Code Review Comments (Package/Variable/Receiver Names, Initialisms, Mixed Caps, In-Band Errors, Named Result Parameters, Pass Values, Interfaces, Doc Comments) — -- Google Go Style Guide (naming, option structs, documentation, test doubles, program initialization) — +- Google Go Style Decisions (Import grouping, Import blank, Import dot, Field names, Getters, Receiver names, Initialisms) — ; Best Practices (naming, option structs, documentation, test doubles, program initialization) — +- `go vet` analyzers (`composites`, `copylocks`) — ; revive rules (`blank-imports`, `dot-imports`, `exported`, `var-naming`, `receiver-naming`) — - Uber Go Style Guide (Exit in Main, Avoid init()) — - Doc comment syntax — ; `internal/` — diff --git a/skills/go-testing/SKILL.md b/skills/go-testing/SKILL.md index 070ae5f..689e8e1 100644 --- a/skills/go-testing/SKILL.md +++ b/skills/go-testing/SKILL.md @@ -1,6 +1,6 @@ --- name: go-testing -description: Idiomatic Go testing. This skill should be used when the user writes or reviews Go tests, benchmarks or fuzz targets — any _test.go file, table-driven `t.Run`, a can-fail control proving a guard is mutation-detectable, a refusal test asserting the operation-specific facet not a shared sentinel, `t.Parallel` (and what cannot run under it), `t.Context`, `t.Chdir`/`t.Setenv`, `t.TempDir` vs `t.ArtifactDir`, `t.Output`, `testing.B.Loop`, the race detector, goroutine-leak checks, `testing/synctest` for time and concurrency, fuzzing, golden files, or failure messages that actually diagnose. Pair with `go test -race`. Go only; error wrapping belongs to go-errors. +description: Idiomatic Go testing. This skill should be used when the user writes or reviews Go tests, benchmarks or fuzz targets — any _test.go file, table-driven `t.Run`, a can-fail control proving a guard is mutation-detectable, `t.Parallel` and what cannot run under it, `t.Context`, `t.TempDir` vs `t.ArtifactDir`, `testing.B.Loop`, `Example` functions with `// Output:`, stable comparisons of serialised or map-derived output, `testing/synctest` for time and concurrency, the race detector, goroutine-leak checks, golden files, or failure messages that diagnose. Pair with `go test -race`. Go only; error wrapping belongs to go-errors. --- # go-testing — Go testing @@ -62,6 +62,19 @@ Deterministic backstop: `go test -race ./...` (always, in CI), `go test -bench`, - **Failure messages must diagnose without a debugger:** name the call, the input, the result, and the expectation — `t.Errorf("Parse(%q) = %v, want %v", in, got, want)` — never a bare `t.Error("failed")`. For structs and slices print a diff (`cmp.Diff(want, got)`), not two blobs. +- **Example functions are runnable documentation.** `func ExampleParse()` in a `_test.go` file + shows in `go doc` and on pkg.go.dev, and `go test` compiles it; end it with a `// Output:` comment + and `go test` also runs it and compares stdout, so the example cannot rot. Name them `ExampleT`, + `ExampleT_Method`, `ExampleF_suffix` (lowercase suffix) — `go vet` (`tests`) rejects a malformed + name. Style Decisions asks for one where feasible — for the entry points a reader meets first — as + advice, not a per-export rule. +- **Name the fields in table-case literals** when a case spans many lines, when adjacent fields share + a type, or when zero-value fields are left out — `{input: "a,b", sep: ",", want: 2}` reads on its + own; `{"a,b", ",", 2}` has to be decoded against the struct. +- **Compare stable results.** Output whose exact bytes belong to a package the repo does not own — + `json.Marshal`, a formatted string, map iteration order — can change under a dependency bump. Parse + it back and compare values; sort map-derived slices first (`slices.Sorted(maps.Keys(m))`); compare + structs with `cmp.Diff`, not `reflect.DeepEqual` on their text form. - **Helpers set up; the test body asserts.** Call `t.Helper()` so a failure points at the caller's line, and prefer a helper that *returns* a value or `error` over one that fails internally — assertion logic belongs where the case's context is visible. `t.Fatal` in a setup helper is fine; @@ -71,7 +84,8 @@ Deterministic backstop: `go test -race ./...` (always, in CI), `go test -bench`, ## Sources - synctest — ; `testing.B.Loop` — - `testing` package (`T.Context`, `T.Chdir`, `T.Output`, `T.Attr`, `T.ArtifactDir`) — ; Go 1.22/1.24/1.25/1.26 release notes — -- Code Review Comments (Useful Test Failures) — ; Google Go Style Guide (Tests) — +- Code Review Comments (Useful Test Failures) — ; Google Go Style Decisions (Examples, Compare stable results, Useful test failures) — ; Best Practices (Tests, Use field names in struct literals, `t.Error` vs `t.Fatal`) — +- Example functions — ; `go vet` `tests` analyzer — - `testing/cryptotest` (Go 1.26) — - `go.uber.org/goleak` —