Repository navigation
feat(skills): Google Style Decisions coverage, tie-break order, linter rule catalogues, link check - #6
Conversation
…r 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 <noreply@anthropic.com>
…EAD failure; Must covers test helpers; descriptions trimmed 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/<pkg>` 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 <noreply@anthropic.com>
Review — Google Style Decisions coverage (
|
| Section | Result |
|---|---|
Manifests (go-coding / 0.5.0, author object, parity) |
Pass — bump deferred to the 0.6.0 release, as the PR says |
| Discoverability (7 skills, 1 agent, both hook stacks, Cursor rule) | Pass — no component added or renamed |
| Frontmatter / YAML trap / description trim (87–95 words) | Pass |
| Inventories | Pass |
validate.py, --selftest, hooks-test.sh, claude plugin validate . |
Pass |
--check-links |
Pass — 57/57 |
| Marketplace pin | N/A until the tag |
Keeping the eight new rules inside the existing skills is the right cut. A go-decisions skill would be source-shaped, not task-shaped, and would undo the 0.5.0 shrink.
Critical
None.
Important
-
Blank-import rule drops Style Decisions’
embedexception.skills/go-layout/SKILL.md:31-34andagents/go-reviewer.md:118-121say a blank import belongs only inmainor a test, “never in a library.” Style Decisions Import “blank” is that rule plus:You may use a blank import of the embed package in a source file which uses the
//go:embedcompiler directive.A normal library
import _ "embed"will be flagged. Do not also copy thenogoexception — that is Google-internal, and the registry already excludes it.The skill also requires main/test and a comment. revive
blank-importsis or (main/test or a justifying comment). Location from Style Decisions; comment as revive’s alternate, not a second requirement. -
“One
Exampleper exported entry point” is stronger than the cited sources. Style Decisions Examples: “Try to provide a runnable example.”go.dev/blog/examplesdocuments naming and// Output:.go vettestsonly rejects a malformed name.skills/go-testing/SKILL.md:69(“A library gets one for every exported entry point a user reaches for first”) andagents/go-reviewer.md:125-126(“a new exported entry point in a library with noExample”) turn an advisory into a walk-every-diff finding. Keep the naming/// Output:mechanics; drop or mark the per-export line advisory, and soften the reviewer dimension to match. -
go-idiomsdescription attributes the new rules togo fix/modernize. The body correctly names theshadowanalyzer and staticcheck S1023. The description still says “Advice equals tooling —go fix ./...orgolangci-lint --enable-only=modernize” while listing nested:=onerrand a redundantswitchbreakas triggers. Neither is a modernizer. Keep the triggers; point them atgovetshadow(opt-in) and staticcheck. -
--selftestdoes not cover the fetch policy the second commit added. Placeholder skip (<pkg>,go1.NN,{ver}) is tested and correct. HEAD→GET, one transport retry, and “404 is broken” are implemented only. A later revert of the first-commit fallback would still pass--selftest. Add a localhttptestfixture; do not hit the network in--selftest.
Suggestions
breakwording ingo-idioms(SKILL.md:85-87): “breakinside aswitchmeans something only with a label” overstates. An unlabeledbreakin the middle of a case still leaves the switch. Ban the redundant end-of-casebreak(S1023); keep labeledbreakfor leaving a surrounding loop.links.ymlpath filter omitsscripts/validate.pyand the workflow file. A checker-only follow-up never runs the live fetch.- 429/503 is treated as a definite answer (no retry). If
linksbecomes a required check, retry those or keep GitHub HTML for the weekly run. - Tie-break clone-drift is already visible: the router names
blank-imports/dot-imports; the Cursor rule does not; the reviewer names least mechanism only. One identical sentence in all three, or a cheap string check — not a new reference file. MustXunder “Import and literal hygiene” ingo-revieweris a catalog smell. Move it next to error swallowing.- Advice == tooling cannot see
shadow. Intentional opt-in; one sentence in AGENTS.md that named opt-in analyzers are the exception, so the next refresh does not “fix” it by enablingshadowingolangci.v2.yml.
Leaving references/golangci.v2.yml and the go-lint-setup scaffold untouched is the right product call. blank-imports / dot-imports, composites, tests, and S1023 already fire in the shipped standard + revive set.
Strengths
- Most new rules match the fetched pages: tie-break order (Guide), Must including
t.Fatalhelpers (Style Decisions), import groups / noimport ./ keyed foreign literals (go vetcomposites), table-case field names (Best Practices), compare-stable-results, S1023,shadowas noisy opt-in. - Second commit actually fixed the first review: templated URLs skipped whole, GET after any HEAD failure, Must covers test helpers, least mechanism under simplicity, descriptions trimmed.
- Router hop table gained
Must, nested:=, import block, and foreign literal in lockstep with the Cursor rule. ## [Unreleased]at version0.5.0is the correct in-flight state.- Public-safety / disclosure is clean on this diff.
Recommended action
- Add the
embed+//go:embedexception togo-layoutandgo-reviewer; treat the comment as revive’s alternate, not a second requirement. - Soften the Example line in
go-testingand drop or advisory-mark the reviewer “new export withoutExample” dimension. - Point the
go-idiomsdescription triggers atshadow/ S1023, notgo fix. - Optional in the same pass:
httptestfor HEAD→GET;links.ymlpaths for the checker; thebreakwording nit. - Then merge. Bump both manifests to
0.6.0in the release commit. Cursor smoke (open a.gofile, confirmgo-context.mdcattaches) can land just after merge — it is process, not a structural hole.
sebastian-iancu
left a comment
There was a problem hiding this comment.
Review — Google Style Decisions coverage (1737a7f)
Re-review of current head against main (881ae3c / v0.5.0): plugin structure, dual-host submission checklist, architecture, source-accuracy of the new rules, link-checker tests. Two commits; the second already landed a first-pass review. CI validate (1.26.x and 1.27.x) and links are green. Cursor install is still unchecked.
New rules were checked against the fetched Style Decisions page (blank-import / Examples sections quoted below).
Verdict: with fixes. Plugin structure is mergeable. Do not merge until the blank-import embed exception and the Example-per-export overclaim are corrected — both will produce false reviewer findings.
Plugin-submission checklist
| Section | Result |
|---|---|
Manifests (go-coding / 0.5.0, author object, parity) |
Pass — bump deferred to the 0.6.0 release, as the PR says |
| Discoverability (7 skills, 1 agent, both hook stacks, Cursor rule) | Pass — no component added or renamed |
| Frontmatter / YAML trap / description trim (87–95 words) | Pass |
| Inventories | Pass |
validate.py, --selftest, hooks-test.sh, claude plugin validate . |
Pass |
--check-links |
Pass — 57/57 |
| Marketplace pin | N/A until the tag |
Keeping the eight new rules inside the existing skills is the right cut. A go-decisions skill would be source-shaped, not task-shaped, and would undo the 0.5.0 shrink.
Critical
None.
Important
-
Blank-import rule drops Style Decisions’
embedexception.skills/go-layout/SKILL.md:31-34andagents/go-reviewer.md:118-121say a blank import belongs only inmainor a test, “never in a library.” Style Decisions Import “blank” is that rule plus:You may use a blank import of the embed package in a source file which uses the
//go:embedcompiler directive.A normal library
import _ "embed"will be flagged. Do not also copy thenogoexception — that is Google-internal, and the registry already excludes it.The skill also requires main/test and a comment. revive
blank-importsis or (main/test or a justifying comment). Location from Style Decisions; comment as revive’s alternate, not a second requirement. -
“One
Exampleper exported entry point” is stronger than the cited sources. Style Decisions Examples: “Try to provide a runnable example.”go.dev/blog/examplesdocuments naming and// Output:.go vettestsonly rejects a malformed name.skills/go-testing/SKILL.md:69(“A library gets one for every exported entry point a user reaches for first”) andagents/go-reviewer.md:125-126(“a new exported entry point in a library with noExample”) turn an advisory into a walk-every-diff finding. Keep the naming/// Output:mechanics; drop or mark the per-export line advisory, and soften the reviewer dimension to match. -
go-idiomsdescription attributes the new rules togo fix/modernize. The body correctly names theshadowanalyzer and staticcheck S1023. The description still says “Advice equals tooling —go fix ./...orgolangci-lint --enable-only=modernize” while listing nested:=onerrand a redundantswitchbreakas triggers. Neither is a modernizer. Keep the triggers; point them atgovetshadow(opt-in) and staticcheck. -
--selftestdoes not cover the fetch policy the second commit added. Placeholder skip (<pkg>,go1.NN,{ver}) is tested and correct. HEAD→GET, one transport retry, and “404 is broken” are implemented only. A later revert of the first-commit fallback would still pass--selftest. Add a localhttptestfixture; do not hit the network in--selftest.
Suggestions
breakwording ingo-idioms(SKILL.md:85-87): “breakinside aswitchmeans something only with a label” overstates. An unlabeledbreakin the middle of a case still leaves the switch. Ban the redundant end-of-casebreak(S1023); keep labeledbreakfor leaving a surrounding loop.links.ymlpath filter omitsscripts/validate.pyand the workflow file. A checker-only follow-up never runs the live fetch.- 429/503 is treated as a definite answer (no retry). If
linksbecomes a required check, retry those or keep GitHub HTML for the weekly run. - Tie-break clone-drift is already visible: the router names
blank-imports/dot-imports; the Cursor rule does not; the reviewer names least mechanism only. One identical sentence in all three, or a cheap string check — not a new reference file. MustXunder “Import and literal hygiene” ingo-revieweris a catalog smell. Move it next to error swallowing.- Advice == tooling cannot see
shadow. Intentional opt-in; one sentence in AGENTS.md that named opt-in analyzers are the exception, so the next refresh does not “fix” it by enablingshadowingolangci.v2.yml.
Leaving references/golangci.v2.yml and the go-lint-setup scaffold untouched is the right product call. blank-imports / dot-imports, composites, tests, and S1023 already fire in the shipped standard + revive set.
Strengths
- Most new rules match the fetched pages: tie-break order (Guide), Must including
t.Fatalhelpers (Style Decisions), import groups / noimport ./ keyed foreign literals (go vetcomposites), table-case field names (Best Practices), compare-stable-results, S1023,shadowas noisy opt-in. - Second commit actually fixed the first review: templated URLs skipped whole, GET after any HEAD failure, Must covers test helpers, least mechanism under simplicity, descriptions trimmed.
- Router hop table gained
Must, nested:=, import block, and foreign literal in lockstep with the Cursor rule. ## [Unreleased]at version0.5.0is the correct in-flight state.- Public-safety / disclosure is clean on this diff.
Recommended action
- Add the
embed+//go:embedexception togo-layoutandgo-reviewer; treat the comment as revive’s alternate, not a second requirement. - Soften the Example line in
go-testingand drop or advisory-mark the reviewer “new export withoutExample” dimension. - Point the
go-idiomsdescription triggers atshadow/ S1023, notgo fix. - Optional in the same pass:
httptestfor HEAD→GET;links.ymlpaths for the checker; thebreakwording nit. - Then merge. Bump both manifests to
0.6.0in the release commit. Cursor smoke (open a.gofile, confirmgo-context.mdcattaches) can land just after merge — it is process, not a structural hole.
Round 2 — review of
|
…/S1023 triggers, fetch-policy self-test
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 <noreply@anthropic.com>
Summary
Closes the gap between what the skills teach and what Google's Go style documents actually rule on, and tightens the source registry so every rule stays traceable.
Coverage added, each rule tied to the tool that enforces it and the document it comes from:
go-layoutmainor a test, with a comment; neverimport .reviveblank-imports,dot-imports(default set);goimports/gofumptgo-layoutgo vetcompositesgo-errorsMustXhelpers only for package initialisation from constant inputsgo-testingExamplefunctions with// Output:as runnable documentationgo vettestsgo-testinggo-testinggo-idioms:=that shadowserr/ctxshadowanalyzer, opt-in undergovetgo-idiomsbreakending aswitchcaseTie-breaks. The router, the Cursor rule, and the reviewer agent now carry the order the Google Guide gives for readable code — clarity, simplicity (with least mechanism), concision, maintainability, consistency — as the rule for choosing between two forms the tools both accept. Until now no component stated a priority order for judgment calls.
Sources. The registry in
docs/authoring.mdnow names Google's three documents by the weight Google itself assigns (Guide: normative and canonical; Style Decisions: normative; Best Practices: advisory), adds the linter rule catalogues to Tier 3 so "revive catches this" can name the rule, and records the revision read for each mutable source (Code Review Comments wiki, Google styleguide, Uber guide, revive rules) so the next refresh diffs against a known point.AGENTS.mdandREADME.mdfollow.Link check.
python3 scripts/validate.py --check-linksresolves every URL cited in skills, agents, rules, and docs (57 today, all resolve). A separatelinksworkflow runs it weekly, on demand, and on pull requests that touch those files — separate fromvalidateso a moved external page is visible without blocking unrelated work.Every added rule was checked against the source text before it was written: the three Google documents were fetched raw and grepped, and the tool claims against
cmd/vet, staticcheck's check list, revive's rule descriptions, and golangci-lint's configuration reference. An independent review on a second model then re-checked every claim against the same sources and confirmed all ten. It found one real defect and four things to tighten, fixed in the second commit: the link checker's placeholder skip could never fire (a templatedhttps://pkg.go.dev/<pkg>was checked as its real-looking prefix; now skipped whole, with a self-test); GET is tried after any HEAD failure and a transport error is retried once; theMustrule now also covers a test helper that stops only the current test witht.Fatal, which Style Decisions sanctions; the router's layout row names the rules its shown commands catch; least mechanism is stated under simplicity, where the Guide places it; and the three descriptions that had grown to 107–108 words are trimmed to 87–95.Verification
python3 scripts/validate.py,--selftest,./scripts/hooks-test.sh,claude plugin validate .all pass.python3 scripts/validate.py --check-links: 57 of 57 URLs resolve (58 before the fix, because the truncated placeholder prefix was being counted as a citation).:(the YAML trap); longest body isgo-idiomsat ~1,600 words.references/golangci.v2.ymland thego-lint-setupscaffold block are untouched — no consumer's lint results change.Checklist
./scripts/validate.shpassesclaude plugin validate .passesrules/go-context.mdcchanged (mirror of the router).cursor-plugin/plugin.jsonpath map kept in stepCHANGELOG.mdupdated under[Unreleased]; version bumps to 0.6.0 in the release commit (coverage expansion = minor)🤖 Generated with Claude Code