From 01216c0cfabf9f84125992ede567de037db71968 Mon Sep 17 00:00:00 2001 From: Sebastian Iancu Date: Wed, 9 Sep 2026 16:06:29 +0300 Subject: [PATCH 1/3] feat(skills): Google Style Decisions coverage, tie-break order, linter rule catalogues, link check Eight rules the Google Go style documents rule on and the skills did not, each tied to its enforcing tool and cited to the document it comes from: - go-layout: import grouping, blank imports only in main or a test with a comment, no dot imports (revive blank-imports/dot-imports); field names in struct literals of foreign types (go vet composites). - go-errors: MustX helpers are for package initialisation from constant inputs. - go-testing: Example functions with // Output: (go vet tests); field names in table-case literals; compare stable results, not serialised bytes or map order. - go-idioms: a nested := that shadows err/ctx (the shadow analyzer, opt-in); no redundant break ending a switch case (staticcheck S1023). The router, the Cursor rule, and go-reviewer carry the Guide's order for choosing between two forms the tools both accept: clarity, simplicity, concision, maintainability, consistency, and least mechanism. go-reviewer gains import and literal hygiene and test-fragility dimensions. The source registry names Google's three documents by the weight Google assigns them, adds the linter rule catalogues to Tier 3, and records the revision read for each mutable source. validate.py gains --check-links (58 URLs, all resolving) and a weekly links workflow runs it. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/links.yml | 21 ++++++++++++ AGENTS.md | 3 +- CHANGELOG.md | 15 +++++++++ README.md | 2 +- agents/go-reviewer.md | 16 ++++++++- docs/authoring.md | 42 ++++++++++++++++++++---- docs/testing.md | 1 + rules/go-context.mdc | 19 +++++++---- scripts/validate.py | 65 +++++++++++++++++++++++++++++++++++-- skills/go-coding/SKILL.md | 18 +++++++--- skills/go-errors/SKILL.md | 9 +++-- skills/go-idioms/SKILL.md | 12 ++++++- skills/go-layout/SKILL.md | 15 +++++++-- skills/go-testing/SKILL.md | 17 ++++++++-- 14 files changed, 225 insertions(+), 30 deletions(-) create mode 100644 .github/workflows/links.yml diff --git a/.github/workflows/links.yml b/.github/workflows/links.yml new file mode 100644 index 0000000..f07a420 --- /dev/null +++ b/.github/workflows/links.yml @@ -0,0 +1,21 @@ +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. +on: + schedule: + - cron: '0 6 * * 1' + workflow_dispatch: + pull_request: + paths: ['skills/**', 'agents/**', 'rules/**', 'docs/**', 'README.md', 'AGENTS.md'] + +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..ea024f3 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. diff --git a/CHANGELOG.md b/CHANGELOG.md index 069b284..d073bea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,21 @@ 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 and with a comment, 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, never for input that can fail. +- Skills: `go-testing` — `Example` functions with `// Output:` as runnable documentation (`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, concision, maintainability, consistency, and least mechanism (Google Go Style Guide). +- Agents: `go-reviewer` — import and literal hygiene and test-fragility dimensions, a shadowed `err` under error swallowing, and the same tie-break order for style findings. +- Scripts: `validate.py --check-links` verifies every cited URL resolves; `.github/workflows/links.yml` runs it weekly and on pull requests touching skills, agents, rules, or docs. + +### 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..53cf297 100644 --- a/agents/go-reviewer.md +++ b/agents/go-reviewer.md @@ -71,7 +71,7 @@ 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`). - **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 +115,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 _` outside a `main` package or a test, or + without a comment naming the side effect; a positional struct literal of a type from another + package; a `MustX` helper called on a request path. `revive` (`blank-imports`, `dot-imports`) and + `go vet` (`composites`) catch the first three — name the rule; `Must` placement is judgment + (`go-layout`, `go-errors`). +- **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; a new + exported entry point in a library with no `Example` function (`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, then concision, then maintainability, +then consistency, plus least mechanism — 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..c3f85db 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -64,10 +64,20 @@ 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 *least mechanism*. The + tie-break order the router and the reviewer use. + - *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 +91,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** @@ -114,7 +141,10 @@ citation. Everything the skills assert should be traceable to one of these. 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..986cda7 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -8,6 +8,7 @@ exercising the components. - **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. +- **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 (`.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..3e3f5bc 100644 --- a/rules/go-context.mdc +++ b/rules/go-context.mdc @@ -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,13 @@ 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, then concision, then maintainability, then consistency — and least +mechanism (the most standard tool that expresses the idea). 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..52a1fa4 100644 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -30,8 +30,9 @@ 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 json import os @@ -40,6 +41,8 @@ import subprocess import sys import tempfile +import urllib.error +import urllib.request from pathlib import Path ROOT = Path(__file__).resolve().parent.parent @@ -423,6 +426,62 @@ def main(): validate_doc_inventories() +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: + for url in URL_RE.findall(f.read_text()): + url = url.rstrip(".,;:") + if "NN" in url or "<" in url or "{" in url: + continue + seen.setdefault(url, f.relative_to(ROOT)) + return seen + + +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 check_links() -> int: + """Resolve every cited URL (HEAD, falling back to GET for hosts that refuse HEAD). 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 = None, None + for method in ("HEAD", "GET"): + try: + status = _fetch(url, method) + break + except urllib.error.HTTPError as e: + last = f"HTTP {e.code}" + if e.code in (403, 405) and method == "HEAD": + continue + break + except Exception as e: # noqa: BLE001 — any transport failure is a broken link here + last = type(e).__name__ + break + 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 + + SELFTEST_HOOKS_CLAUDE = {"hooks": {"PostToolUse": [{"matcher": "Write|Edit", "hooks": [ {"type": "command", "command": "bash ${CLAUDE_PLUGIN_ROOT}/hooks/a.sh"}]}]}} SELFTEST_HOOKS_CURSOR = {"hooks": {"afterFileEdit": [{"command": "bash hooks/a.sh"}]}} @@ -513,6 +572,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)") diff --git a/skills/go-coding/SKILL.md b/skills/go-coding/SKILL.md index e24c9d7..ce219e2 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`), `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, then concision, then maintainability, then consistency** — and *least +mechanism*: prefer the most standard tool that expresses the idea. 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..c6ef57f 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,11 @@ 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, not for input.** A helper that panics on failure carries + the `Must` prefix (`regexp.MustCompile`, `template.Must`) and is called only while setting up + package-level values from constants the author controls. 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 +63,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..9c39aa0 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 (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, a nested `:=` that shadows `err` or `ctx`, a redundant `break` ending a `switch` case, 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. --- # go-idioms — modern Go (modernize) @@ -76,6 +76,15 @@ 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 the `break` is dead + text; `staticcheck` S1023 (standard set) flags it. `break` inside a `switch` means something only + with a label, to leave an enclosing loop. *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 +120,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..3ae15f5 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 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, import grouping and blank or dot imports, unkeyed struct literals of foreign types, and hexagonal/DDD ceremony answered with the standard-library shape. Pair with revive (var-naming, receiver-naming, exported, blank-imports, dot-imports) and `go vet` (composites). Not for error handling (go-errors) or tests (go-testing). --- # go-layout — layout, naming & API surface @@ -27,6 +27,11 @@ 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 only in a `main` + package or a test that needs the side effect, with a comment naming it; never in a library, where + it silently changes every importer. Never `import .` — it hides where a name comes from. `revive` + (`blank-imports`, `dot-imports`, both in its default rule set) catches both. ## Naming @@ -71,6 +76,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 +99,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..73a2e4e 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, 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`, `Example` functions with `// Output:`, field names in table cases, stable comparisons of serialised or map-derived output, 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. --- # go-testing — Go testing @@ -62,6 +62,18 @@ 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. A library gets one for every exported entry point a user reaches for first. +- **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 +83,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` — From 1737a7fb49979c56c40d1678462d1cbfb950eab1 Mon Sep 17 00:00:00 2001 From: Sebastian Iancu Date: Wed, 9 Sep 2026 16:27:07 +0300 Subject: [PATCH 2/3] fix(review): link checker skips templated URLs whole, GET after any HEAD failure; Must covers test helpers; descriptions trimmed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an independent review of the previous commit against the primary sources: - validate.py --check-links: the regex stopped at `<`, so a placeholder URL such as `https://pkg.go.dev/` was checked as its real-looking prefix instead of skipped; the collector now tests for `{`/`NN` inside the match and `<` right after it, and a self-test pins all three placeholder forms. GET is tried after any HEAD failure, as the docstring said, and a transport error is retried once so a single reset does not fail a CI run. - go-errors: the Must rule also covers a test helper that stops only the current test with t.Fatal, which Style Decisions § Must functions sanctions explicitly. - go-coding and the Cursor rule: the layout row's backstop cell names blank-imports, dot-imports and go vet composites, matching go-layout. - Least mechanism is stated under simplicity, where the Guide places it, in the router, the Cursor rule, the reviewer, the registry and the changelog. - go-layout, go-testing, go-idioms descriptions trimmed from 107-108 words to the low 80s by dropping triggers whose rules the bodies still carry. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 4 +-- agents/go-reviewer.md | 4 +-- docs/authoring.md | 2 +- rules/go-context.mdc | 7 ++--- scripts/validate.py | 52 ++++++++++++++++++++++++++++---------- skills/go-coding/SKILL.md | 6 ++--- skills/go-errors/SKILL.md | 11 ++++---- skills/go-idioms/SKILL.md | 2 +- skills/go-layout/SKILL.md | 2 +- skills/go-testing/SKILL.md | 2 +- 10 files changed, 59 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d073bea..65552b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,10 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve ### Added - Skills: `go-layout` — imports in groups with the standard library first, blank imports only in `main` or a test and with a comment, 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, never for input that can fail. +- 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 (`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, concision, maintainability, consistency, and least mechanism (Google Go Style Guide). +- 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` under error swallowing, and the same tie-break order for style findings. - Scripts: `validate.py --check-links` verifies every cited URL resolves; `.github/workflows/links.yml` runs it weekly and on pull requests touching skills, agents, rules, or docs. diff --git a/agents/go-reviewer.md b/agents/go-reviewer.md index 53cf297..7d0b7ce 100644 --- a/agents/go-reviewer.md +++ b/agents/go-reviewer.md @@ -130,8 +130,8 @@ For the *why* and citations behind any dimension, the `go-errors`, `go-concurren 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, then concision, then maintainability, -then consistency, plus least mechanism — and say which attribute decided it (). A style +order Google's Go Style Guide gives — clarity, then simplicity (with its rule of least mechanism), +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 diff --git a/docs/authoring.md b/docs/authoring.md index c3f85db..a6f4c81 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -69,7 +69,7 @@ citation. Everything the skills assert should be traceable to one of these. - 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 *least mechanism*. The + (clarity, simplicity, concision, maintainability, consistency) and, under simplicity, *least mechanism*. The tie-break order the router and the reviewer use. - *Style Decisions* — — **normative, not canonical**: the reviewer rulebook — naming, commentary, imports, errors, language, common libraries, useful test failures. The main Google diff --git a/rules/go-context.mdc b/rules/go-context.mdc index 3e3f5bc..70e889f 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`, `go vet` (`composites`); rest is judgment | `go-layout` | ## Route, then load @@ -59,8 +59,9 @@ must make gets the options plus a recommendation. Identifiers, commands and lint verbatim — it is the prose around them that must be plain. When two valid forms compete and the tools accept both, decide by the order Google's Go Style Guide -gives: clarity, then simplicity, then concision, then maintainability, then consistency — and least -mechanism (the most standard tool that expresses the idea). Say which attribute decided it. +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, diff --git a/scripts/validate.py b/scripts/validate.py index 52a1fa4..a309ada 100644 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -41,6 +41,7 @@ import subprocess import sys import tempfile +import time import urllib.error import urllib.request from pathlib import Path @@ -439,9 +440,14 @@ def collect_urls(): path = ROOT / src files = [path] if path.is_file() else sorted(path.rglob("*.md")) + sorted(path.rglob("*.mdc")) for f in files: - for url in URL_RE.findall(f.read_text()): - url = url.rstrip(".,;:") - if "NN" in url or "<" in url or "{" in url: + 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 @@ -454,7 +460,9 @@ def _fetch(url: str, method: str) -> int: def check_links() -> int: - """Resolve every cited URL (HEAD, falling back to GET for hosts that refuse HEAD). Needs the + """Resolve every cited 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. A transport + error (reset, timeout) gets one retry per method, so a single hiccup does not fail the run. 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.""" @@ -463,16 +471,18 @@ def check_links() -> int: for url, where in sorted(urls.items()): status, last = None, None for method in ("HEAD", "GET"): - try: - status = _fetch(url, method) - break - except urllib.error.HTTPError as e: - last = f"HTTP {e.code}" - if e.code in (403, 405) and method == "HEAD": - continue - break - except Exception as e: # noqa: BLE001 — any transport failure is a broken link here - last = type(e).__name__ + for attempt in (1, 2): + try: + status = _fetch(url, method) + break + except urllib.error.HTTPError as e: + last = f"HTTP {e.code}" # a definite answer: no retry, try the other method + break + except Exception as e: # noqa: BLE001 — a reset or timeout is retried once, then GET + last = type(e).__name__ + if attempt == 1: + time.sleep(1) + if status is not None: break if status is None or status >= 400: broken.append((url, where, last or f"HTTP {status}")) @@ -543,6 +553,20 @@ 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 for label, defect, checks in SELFTEST_CASES: outcomes = {} for variant, break_it in (("broken", defect), ("clean", None)): diff --git a/skills/go-coding/SKILL.md b/skills/go-coding/SKILL.md index ce219e2..20b594d 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/`, imports, initialisms, receiver type, in-band errors, struct literals, 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 @@ -62,8 +62,8 @@ Apply these even if you load nothing else; they are the rules the focused skills ## 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, then concision, then maintainability, then consistency** — and *least -mechanism*: prefer the most standard tool that expresses the idea. Say which attribute decided it; +**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 diff --git a/skills/go-errors/SKILL.md b/skills/go-errors/SKILL.md index c6ef57f..65c3d87 100644 --- a/skills/go-errors/SKILL.md +++ b/skills/go-errors/SKILL.md @@ -41,11 +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, not for input.** A helper that panics on failure carries - the `Must` prefix (`regexp.MustCompile`, `template.Must`) and is called only while setting up - package-level values from constants the author controls. 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. +- **`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` diff --git a/skills/go-idioms/SKILL.md b/skills/go-idioms/SKILL.md index 9c39aa0..a0742b3 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, a nested `:=` that shadows `err` or `ctx`, a redundant `break` ending a `switch` case, 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 (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`, iterators, `omitzero`, `os.Root`, `new(expr)`, `errors.AsType`, a nested `:=` that shadows `err`, a redundant `break` in a `switch`, and the Go 1.27 fixers (`atomictypes`, `embedlit`, `slicesbackward`, `unsafefuncs`). Advice equals tooling — `go fix ./...` or `golangci-lint --enable-only=modernize`. Not for linter configuration (go-lint-setup). Go only. --- # go-idioms — modern Go (modernize) diff --git a/skills/go-layout/SKILL.md b/skills/go-layout/SKILL.md index 3ae15f5..6a6a250 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, import grouping and blank or dot imports, unkeyed struct literals of foreign types, and hexagonal/DDD ceremony answered with the standard-library shape. Pair with revive (var-naming, receiver-naming, exported, blank-imports, dot-imports) and `go vet` (composites). 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 diff --git a/skills/go-testing/SKILL.md b/skills/go-testing/SKILL.md index 73a2e4e..13efaec 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`, `Example` functions with `// Output:`, field names in table cases, stable comparisons of serialised or map-derived output, 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 From 9563b1c1b9d9603eb5f57bd86978f913458d031a Mon Sep 17 00:00:00 2001 From: Sebastian Iancu Date: Wed, 9 Sep 2026 17:02:14 +0300 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20apply=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20embed=20exception,=20advisory=20Example,=20shadow/S?= =?UTF-8?q?1023=20triggers,=20fetch-policy=20self-test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule text, checked against Style Decisions and revive's rule source: - go-layout: blank imports allowed in main or a test; the embed package under //go:embed is the one library exception; a justifying comment is revive's alternate, not a second requirement. - go-testing: Example coverage is advice ("try to provide"), not one per exported identifier; the reviewer's "new export without Example" clause is dropped. - go-idioms: the description routes rewrites to go fix / modernize and the shadowed err and end-of-case break triggers to the opt-in shadow analyzer and staticcheck S1023; the break bullet no longer claims an unlabeled break is meaningless mid-case. - go-reviewer: MustX on a request path moves from import hygiene to error swallowing. - Tie-break sentence made identical in the router, the Cursor rule and go-reviewer; the Cursor rule's layout row names blank-imports and dot-imports like the router's. - AGENTS.md and docs/authoring.md: analyzers taught as opt-in (shadow) are the deliberate exception to advice == tooling. Checker: - validate.py: resolve_url extracted; 429/503 get one retry before the other method; --selftest exercises HEAD-refused, transport-reset, 503-then-200 and 404 against a loopback http.server, with no network; a tie-break sentence parity check, self-tested with a swapped-order drift. - links.yml also runs on changes to scripts/validate.py and to the workflow itself. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/links.yml | 12 +- AGENTS.md | 2 +- CHANGELOG.md | 9 +- agents/go-reviewer.md | 24 ++-- docs/authoring.md | 11 +- docs/testing.md | 4 +- rules/go-context.mdc | 2 +- scripts/validate.py | 221 ++++++++++++++++++++++++++++++++---- skills/go-coding/SKILL.md | 4 +- skills/go-idioms/SKILL.md | 9 +- skills/go-layout/SKILL.md | 11 +- skills/go-testing/SKILL.md | 3 +- 12 files changed, 254 insertions(+), 58 deletions(-) diff --git a/.github/workflows/links.yml b/.github/workflows/links.yml index f07a420..7e007bb 100644 --- a/.github/workflows/links.yml +++ b/.github/workflows/links.yml @@ -2,13 +2,21 @@ 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. +# 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'] + paths: + - 'skills/**' + - 'agents/**' + - 'rules/**' + - 'docs/**' + - 'README.md' + - 'AGENTS.md' + - 'scripts/validate.py' + - '.github/workflows/links.yml' jobs: links: diff --git a/AGENTS.md b/AGENTS.md index ea024f3..3a5f123 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,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 65552b6..e52e146 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,14 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve ## [Unreleased] ### Added -- Skills: `go-layout` — imports in groups with the standard library first, blank imports only in `main` or a test and with a comment, no dot imports (`revive` `blank-imports`/`dot-imports`), and field names in struct literals of types from other packages (`go vet` `composites`). +- 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 (`go vet` `tests`), field names in table-case literals, and comparing stable results rather than serialised bytes or map order. +- 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` under error swallowing, and the same tie-break order for style findings. -- Scripts: `validate.py --check-links` verifies every cited URL resolves; `.github/workflows/links.yml` runs it weekly and on pull requests touching skills, agents, rules, or docs. +- 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. diff --git a/agents/go-reviewer.md b/agents/go-reviewer.md index 7d0b7ce..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; a nested `:=` that shadows `err` so the outer check sees nil (`go-idioms`). + 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,24 +117,24 @@ 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 _` outside a `main` package or a test, or - without a comment naming the side effect; a positional struct literal of a type from another - package; a `MustX` helper called on a request path. `revive` (`blank-imports`, `dot-imports`) and - `go vet` (`composites`) catch the first three — name the rule; `Must` placement is judgment - (`go-layout`, `go-errors`). +- **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; a new - exported entry point in a library with no `Example` function (`go-testing`). + 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), -then concision, then maintainability, then consistency — and say which attribute decided it (). A style -preference with no attribute behind it is not a finding. +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 diff --git a/docs/authoring.md b/docs/authoring.md index a6f4c81..ab8772f 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -70,7 +70,8 @@ citation. Everything the skills assert should be traceable to one of these. 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 and the reviewer use. + 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. @@ -132,9 +133,11 @@ revision first, so it reads what changed rather than everything, then updates th 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 diff --git a/docs/testing.md b/docs/testing.md index 986cda7..1260f35 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -7,8 +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. -- **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 (`.github/workflows/links.yml`). +- **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 70e889f..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`, `go vet` (`composites`); 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 diff --git a/scripts/validate.py b/scripts/validate.py index a309ada..26a6ecb 100644 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -24,7 +24,10 @@ * 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. @@ -34,6 +37,7 @@ 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 @@ -41,6 +45,7 @@ import subprocess import sys import tempfile +import threading import time import urllib.error import urllib.request @@ -76,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): @@ -385,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")): @@ -425,6 +455,7 @@ def main(): validate_linter_references() validate_fixer_column() validate_doc_inventories() + validate_tie_break_parity() URL_RE = re.compile(r"https?://[^\s<>()\[\]`\"']+") @@ -453,37 +484,60 @@ def collect_urls(): 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: 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. A transport - error (reset, timeout) gets one retry per method, so a single hiccup does not fail the run. Needs the + """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 = 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}" # a definite answer: no retry, try the other method - break - except Exception as e: # noqa: BLE001 — a reset or timeout is retried once, then GET - last = type(e).__name__ - if attempt == 1: - time.sleep(1) - if status is not None: - break + 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: @@ -492,6 +546,107 @@ def check_links() -> int: 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": [ {"type": "command", "command": "bash ${CLAUDE_PLUGIN_ROOT}/hooks/a.sh"}]}]}} SELFTEST_HOOKS_CURSOR = {"hooks": {"afterFileEdit": [{"command": "bash hooks/a.sh"}]}} @@ -502,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() @@ -512,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") @@ -528,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 = ( @@ -543,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,)), ) @@ -567,6 +742,7 @@ def run_selftest() -> int: 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)): @@ -606,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 20b594d..538e163 100644 --- a/skills/go-coding/SKILL.md +++ b/skills/go-coding/SKILL.md @@ -62,8 +62,8 @@ Apply these even if you load nothing else; they are the rules the focused skills ## 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; +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 diff --git a/skills/go-idioms/SKILL.md b/skills/go-idioms/SKILL.md index a0742b3..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`, iterators, `omitzero`, `os.Root`, `new(expr)`, `errors.AsType`, a nested `:=` that shadows `err`, a redundant `break` in a `switch`, and the Go 1.27 fixers (`atomictypes`, `embedlit`, `slicesbackward`, `unsafefuncs`). 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) @@ -82,9 +82,10 @@ something a modernizer rewrites. 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 the `break` is dead - text; `staticcheck` S1023 (standard set) flags it. `break` inside a `switch` means something only - with a label, to leave an enclosing loop. +- **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` diff --git a/skills/go-layout/SKILL.md b/skills/go-layout/SKILL.md index 6a6a250..ddbf97a 100644 --- a/skills/go-layout/SKILL.md +++ b/skills/go-layout/SKILL.md @@ -28,10 +28,13 @@ exported signature are part of the API — they are as reviewable as the code. - **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 only in a `main` - package or a test that needs the side effect, with a comment naming it; never in a library, where - it silently changes every importer. Never `import .` — it hides where a name comes from. `revive` - (`blank-imports`, `dot-imports`, both in its default rule set) catches both. + `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 diff --git a/skills/go-testing/SKILL.md b/skills/go-testing/SKILL.md index 13efaec..689e8e1 100644 --- a/skills/go-testing/SKILL.md +++ b/skills/go-testing/SKILL.md @@ -66,7 +66,8 @@ Deterministic backstop: `go test -race ./...` (always, in CI), `go test -bench`, 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. A library gets one for every exported entry point a user reaches for first. + 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.