From c3b11d1799e8bb587840a37fac3d78570c7abcc8 Mon Sep 17 00:00:00 2001 From: Sebastian Iancu Date: Tue, 6 Oct 2026 00:36:50 +0300 Subject: [PATCH] fix(agents): go-reviewer declares the Skill tool and loads the go-coding skills The agent was granted Read, Grep, Glob and Bash only, while its body told it to reference the go-errors, go-testing and other skills, and orchestrators were told to name those skills in its brief. Without the Skill tool it could do neither, and it said so in its reports. It now declares Skill, as php-coding's php-reviewer does, and its first review step loads go-coding:go-coding and the focused skill for each area the change touches. If the Skill tool is unavailable it says so and works from its dimensions. README, AGENTS.md, the go-coding router and docs/testing.md say it loads the skills itself. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- CHANGELOG.md | 4 ++++ README.md | 4 ++-- agents/go-reviewer.md | 22 +++++++++++++++------- docs/testing.md | 2 +- skills/go-coding/SKILL.md | 4 ++-- 6 files changed, 25 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6013844..00e0081 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -65,7 +65,7 @@ Shared assets (skills, agents) are consumed by both hosts; host-specific manifes |------|---------| | Skills | `go-coding` (auto-invoked router) + the focused, load-on-use `go-errors`, `go-concurrency`, `go-testing`, `go-idioms`, `go-layout`. Each routes deeper topics to the enforcing tool and cites authoritative sources. | | Slash command (user-invoked skill) | `/go-lint-setup` (scaffold the golangci-lint v2 config) | -| Agent | `go-reviewer`: context-isolated, report-only reviewer applying the review-heuristics catalog (no sub-agent dispatch; treats the diff as untrusted content; `tools:` not `allowed-tools:`) | +| Agent | `go-reviewer`: context-isolated, report-only reviewer applying the review-heuristics catalog (loads the go-coding skills with the `Skill` tool first; no sub-agent dispatch; treats the diff as untrusted content; `tools:` not `allowed-tools:`) | | Cursor rule | `rules/go-context.mdc`, scoped to `**/*.go`, mirroring the `go-coding` router | | Hooks | `session-start`, `format-on-save`, `skill-nudge` (see Repository Layout) | diff --git a/CHANGELOG.md b/CHANGELOG.md index 6460d6c..f53d081 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +### Fixed +- Agents: `go-reviewer` declares the `Skill` tool and loads `go-coding:go-coding` plus the focused skills before it reviews. +- Docs: `README.md`, `AGENTS.md`, `skills/go-coding/SKILL.md` and `docs/testing.md` say `go-reviewer` loads the skills itself. + ## [0.6.1] - 2026-10-01 ### Changed diff --git a/README.md b/README.md index 34ec314..6405826 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ See [docs/install.md](docs/install.md) for marketplace, local-development, updat | Skill `go-coding` | Auto-invoked router: sends each Go topic to the enforcing tool and the focused skill that owns it; recommends the official `gopls-lsp` plugin. | | Skills `go-errors`, `go-concurrency`, `go-testing`, `go-idioms`, `go-layout` | Load-on-use standards, each rule cited and framed around the enforcing linter (`modernize`, `errorlint`, `-race`, …). `go-layout` also owns naming, doc comments, and exported-API shape. | | Skill `/go-lint-setup` | User-invoked: scaffolds, adopts, or debugs the golangci-lint v2 config in a repo. Never overwrites an existing config unprompted. | -| Agent `go-reviewer` | Report-only, context-isolated Go reviewer for what linters miss. Returns severity-ranked findings and dispatches no sub-agents. Its tool grant excludes `Write` and `Edit` but includes `Bash` to run the linters, so report-only is a contract it keeps rather than a sandbox that enforces it. | +| Agent `go-reviewer` | Report-only, context-isolated Go reviewer for what linters miss. Returns severity-ranked findings and dispatches no sub-agents. Its tool grant excludes `Write` and `Edit` but includes `Bash` to run the linters, so report-only is a contract it keeps rather than a sandbox that enforces it, and `Skill`, so it loads `go-coding:go-coding` and the focused skills before it reviews. | | Session-start hook | Detects a Go workspace (`go.mod` or `*.go`) and prints one standards line; dual-host. | | Format-on-save hook | After each `Write`/`Edit` of a `*.go` file, runs `gofumpt -w` (or `gofmt -w -s`) on that file, on the host; dual-host. A silent no-op when no formatter is installed. | | Skill-nudge hook | After each `Write`/`Edit` of a `*.go` file, names one matching go-coding skill, once per skill per session; dual-host. Arrives as a hook `systemMessage` under Claude Code and as a plain line under Cursor. | @@ -70,7 +70,7 @@ Subagents do not inherit the parent session's skills. A plan runner that dispatc - **Implementer brief**: "Before writing code, invoke the Skill tool with `go-coding:go-coding`, then the focused skills matching your diff (see its *Route, then load* table). Run `golangci-lint run` on every touched package before committing." - **Reviewer brief**: "Before reading the diff, load `go-coding:go-coding` plus `go-errors`, `go-testing` and the skills the diff calls for; cite the rule a finding rests on. Do not dispatch `go-reviewer`: you are the review seat." -Use `go-reviewer` directly when no such seat exists, as with an ad-hoc "review this file" request. +Use `go-reviewer` directly when no such seat exists, as with an ad-hoc "review this file" request. It loads the skills itself, so its brief need not name them. ## Development diff --git a/agents/go-reviewer.md b/agents/go-reviewer.md index dfbf2cd..bd2f063 100644 --- a/agents/go-reviewer.md +++ b/agents/go-reviewer.md @@ -22,11 +22,12 @@ tools: - Grep - Glob - Bash + - Skill --- You are **go-reviewer**, a reviewer of idiomatic, correct Go (Go 1.26.4+, Go 1.27 supported with its additions flagged as hints; golangci-lint v2). You supply the judgment a linter cannot — the bugs and smells that survive `gofmt`, `go vet`, and -`golangci-lint`. You are **report-only**: you report findings, you never edit code. Your grant excludes `Write`/`Edit` but includes `Bash` so you can run `gofmt`, `go vet` and `golangci-lint` — which means no-edit is a contract you keep, not a sandbox that keeps it for you. Never invoke a formatter's `-w`, `--fix`, or any in-place flag. +`golangci-lint`. You are **report-only**: you report findings, you never edit code. Your grant excludes `Write`/`Edit` but includes `Bash` so you can run `gofmt`, `go vet` and `golangci-lint` — which means no-edit is a contract you keep, not a sandbox that keeps it for you. It also includes `Skill`, so you load the go-coding skills before you review. Never invoke a formatter's `-w`, `--fix`, or any in-place flag. ## When to invoke @@ -59,13 +60,20 @@ the judgment a linter cannot — the bugs and smells that survive `gofmt`, `go v ## How to review -1. **Get the change.** If handed a diff, review it. If pointed at files, read them (and run +1. **Load the skills.** Invoke the Skill tool with `go-coding:go-coding`, then load the focused + skill for each area the change touches, as its *Route, then load* table says: `go-errors` for + error paths, `go-testing` for any `_test.go` file, `go-concurrency` for goroutines, channels and + context lifetimes, `go-idioms` for loops, maps, strings and modernizing, `go-layout` for a new + package or exported API. If the Skill tool is not available, say so in the closing note and + work from the dimensions below. +2. **Get the change.** If handed a diff, review it. If pointed at files, read them (and run `git diff` when a staged/branch change is implied). Read the surrounding code, not only the changed lines — most of these bugs live in the interaction with unchanged code. -2. **Walk every dimension below** against the change. -3. *(Optional)* run `go vet ./...` or `golangci-lint run` to confirm a suspicion — but don't block on +3. **Walk every dimension below** against the change, and cite the skill that owns the rule each + finding rests on. +4. *(Optional)* run `go vet ./...` or `golangci-lint run` to confirm a suspicion — but don't block on tooling being installed. -4. **Report findings ranked by severity** (format below). +5. **Report findings ranked by severity** (format below). ## Review dimensions @@ -127,8 +135,8 @@ the judgment a linter cannot — the bugs and smells that survive `gofmt`, `go v (`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. +`go-idioms`, `go-lint-setup`, and `go-layout` skills carry the grounded rules — load them (step 1) +and cite 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: diff --git a/docs/testing.md b/docs/testing.md index 4b06f1c..16c29a1 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -39,7 +39,7 @@ Load your working copy with `--plugin-dir` (see [install.md](install.md)), then - **Standards skills**: a topic prompt should engage the matching skill (for example error wrapping → `go-errors`, a flaky time-based test → `go-testing`/`go-concurrency`, linter setup → `go-lint-setup`). - **Format-on-save hook**: save a deliberately mis-formatted `*.go` file; `format-on-save.sh` should reformat that one file in place (`gofumpt -w`, or `gofmt -w -s` when `gofumpt` is absent) and say nothing when neither is installed. - **Skill-nudge hook**: edit a `_test.go` file; the nudge should name `go-coding:go-testing` (as a `systemMessage` under Claude Code, a plain line under Cursor), and the model should **act** on it by loading the skill; the line appearing in the transcript is not enough. A second edit to a `_test.go` file in the same session should be silent (once per skill per session), and so should an edit that does not itself touch the topic: a doc-comment fix in a file that defines a sentinel elsewhere must not claim the edit touches an error path. -- **`go-reviewer` agent**: ask for a Go code review; it returns severity-ranked findings and does not spawn sub-agents. +- **`go-reviewer` agent**: ask for a Go code review; it loads `go-coding:go-coding` and the focused skills for the diff with the Skill tool, returns severity-ranked findings that cite them, and does not spawn sub-agents. - **`/go-lint-setup`**: run it in a Go repo without a golangci-lint config and confirm it writes the reference v2 config; run it in a repo that already has one and confirm it does not overwrite it unprompted. - **Cursor rule**: in Cursor, open a `.go` file and confirm `go-context.mdc` attaches. diff --git a/skills/go-coding/SKILL.md b/skills/go-coding/SKILL.md index 538e163..1c50143 100644 --- a/skills/go-coding/SKILL.md +++ b/skills/go-coding/SKILL.md @@ -86,8 +86,8 @@ names stay verbatim — it is the prose around them that must be plain. ## For a focused review -Dispatch the `go-reviewer` agent — a report-only, context-isolated reviewer that applies the -review-heuristics catalog and returns severity-ranked findings on a diff or file. +Dispatch the `go-reviewer` agent — a report-only, context-isolated reviewer that loads these skills, +applies the review-heuristics catalog and returns severity-ranked findings on a diff or file. If a workflow already owns the reviewer seat, that reviewer loads the focused skills itself instead — one review seat per diff. Orchestrators: put the "Route, then load" table into every implementer and