From 37eab229fcf118da646d110a50c4965b3ddc9fc7 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 28 Apr 2026 09:02:21 +0200 Subject: [PATCH] docs: mandate `/kbagent:review` self-review before tagging a human reviewer Now that the kbagent-pr-reviewer subagent + /kbagent:review slash command have shipped (#228), describe the self-review workflow in CONTRIBUTING.md so authors run the agent against their open PR before pinging a maintainer. Two touchpoints: - New top-level section "## Self-review before tagging a human reviewer" between "Commit & PR Conventions" and "Testing Guidelines". Covers what the reviewer does, how to invoke it (with no args, with explicit PR number, with focus hint), how to triage findings by severity, and why it is a per-author courtesy rather than a CI gate (depends on Claude Code + plugin install + gh auth, not portable). - New bullet inside "Commit & PR Conventions" cross-linking to the new section -- ensures authors see it in the PR-flow conventions list, not only when they happen to scroll to the new section. The new section explicitly tells authors: address BLOCKING findings or push back in a PR comment; mention NON-BLOCKING in the PR description; NIT is optional. It also documents the escape hatch ("self-review skipped: " in the PR description) for contributors who genuinely cannot run the agent. --- CONTRIBUTING.md | 74 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 014e8471..4c0ac4fc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,9 +284,83 @@ to catch this before the change ships. - **Pre-commit hook must pass** -- `ruff check` + `ruff format --check`. Install via `make hooks` - **Never skip hooks** (`--no-verify`) -- fix the lint issue instead - **Protected main branch** -- always work on a feature branch, create PR, merge via GitHub +- **Self-review with `/kbagent:review` before tagging a human** -- see + [Self-review before tagging a human reviewer](#self-review-before-tagging-a-human-reviewer) + for what it does and how to run it. CI does not catch the silent-drift + surfaces (Plugin synchronization map); the self-review does. For reference on commit style: https://github.com/padak/claude-code-kit/blob/main/CLAUDE.md +## Self-review before tagging a human reviewer + +Before you ping a maintainer, **run the `/kbagent:review` slash command +against your open PR**. It is a read-only specialist subagent +(`kbagent-pr-reviewer`, shipped with the kbagent Claude Code plugin) that +walks the same playbook a careful human reviewer would: 3-layer compliance, +[Plugin synchronization map](#plugin-synchronization-map) silent-drift hunt, +test coverage, behavior verification, backward compatibility, security and +token discipline. It posts ONE structured comment review on the PR with +findings rated BLOCKING / NON-BLOCKING / NIT, each carrying a `file:line` +citation. + +### How to run it + +1. Push your branch and open the PR (`gh pr create ...`). +2. Stay checked out on the PR's branch with a clean working tree. +3. Confirm `gh auth status` is authenticated to the same fork as the PR. +4. In a Claude Code session in the repo root, type: + + ``` + /kbagent:review + ``` + + The slash command auto-detects the PR for the current branch. To target a + different PR explicitly: + + ``` + /kbagent:review 234 + /kbagent:review https://github.com/padak/keboola_agent_cli/pull/234 + /kbagent:review 234 focus on the new cache semantics + ``` + +5. The reviewer reads `CONTRIBUTING.md`, walks the diff, runs `make check`, + attempts to reproduce the PR's claimed behavior, and posts a single + `gh pr review --comment` to the PR. It NEVER approves, requests changes, + merges, or pushes -- the verdict in the comment body is advice; you and + the human reviewer retain every veto. + +### What to do with the findings + +- **BLOCKING** -- address before tagging a human, OR push back in a PR + comment explaining why you disagree. Some BLOCKING findings are + calibration mistakes; the reviewer defaults conservative, and a + ~30-second human disposition is faster than a re-run. +- **NON-BLOCKING** -- address if quick; otherwise mention them in the PR + description so the human reviewer knows they are not regressions hiding + in the diff. +- **NIT** -- optional. Address if you agree. + +### This is a courtesy, not a CI gate + +The reviewer is intentionally NOT wired into CI. It depends on Claude Code +with the kbagent plugin installed and an authenticated `gh`, which is not +portable across all contributor setups. Running it remains a per-author +courtesy that: + +- catches the silent-drift gaps (`OPERATION_REGISTRY`, `gotchas.md` + version tags, `keboola-expert.md` matrix, `commands/context.py` + `AGENT_CONTEXT`, `commands-reference.md`, `--hint` definitions) that + CI does not check; +- demonstrates to the human reviewer that you have walked the + [Plugin synchronization map](#plugin-synchronization-map); +- saves a review round-trip when the reviewer would otherwise catch the + same issues. + +If you genuinely cannot run it (offline, no `gh` auth, plugin not +installed), say so explicitly in the PR description (`self-review +skipped: `) -- the human reviewer may run it on your behalf, or +ask you to address it before merge. + ## Testing Guidelines - Use `typer.testing.CliRunner` for CLI tests