From 2bce5799dce8c3fdeebc81a88240061c7930397c Mon Sep 17 00:00:00 2001 From: Markus Graf Date: Mon, 28 Sep 2026 21:42:01 +0200 Subject: [PATCH] Replace honest-reviewer with code-review skill - Remove honest-reviewer (Socratic/devil's-advocate approach) - Add code-review skill: high-precision review based on current best practices (confidence scoring >= 80, validate-before-report, context gathering beyond the diff, strict output contract, GLM/open-weight guardrails) - dev-workflow: point review step at code-review, findings instead of questions; also adopt issue-based plan storage with explicit user approval gate --- dev/code-review/SKILL.md | 121 +++++++++++++++++++++++++++++++++++ dev/dev-workflow/SKILL.md | 8 +-- dev/honest-reviewer/SKILL.md | 100 ----------------------------- 3 files changed, 125 insertions(+), 104 deletions(-) create mode 100644 dev/code-review/SKILL.md delete mode 100644 dev/honest-reviewer/SKILL.md diff --git a/dev/code-review/SKILL.md b/dev/code-review/SKILL.md new file mode 100644 index 0000000..901547e --- /dev/null +++ b/dev/code-review/SKILL.md @@ -0,0 +1,121 @@ +--- +name: code-review +description: Reviews a branch, diff, or merge request like a skeptical senior engineer, reporting only high-confidence findings. Gathers context beyond the diff (intent, callers, project guidelines), validates every finding before reporting, and scores confidence (reports only >= 80/100). Covers correctness, contracts, security, and test coverage. Use before merging — or when the user says "review", "code review", "review my branch", or during the dev-workflow review step. +--- + +# Code Review + +You review code changes like a senior engineer whose only currency is trust: one +false positive costs more than ten missed nits. In the wild, developers reject +over half of all automated review comments — your job is to be the exception. + +## Prime directive + +Report only findings you have **validated in the code**. If you cannot name the +concrete input, state, or code path that triggers a problem, do not report it. +"No issues found" is a valid, respected outcome — never invent findings to seem +thorough, never pad with style opinions. + +This skill is designed to run with **fresh context** (e.g. in a subagent). If the +current session authored the changes it is about to review, say so and recommend +a fresh-context review instead: the assumptions made while writing code carry +over into its review. + +## Workflow + +### Phase 1 — Scope + +1. Identify the base branch (`main`/`master`/`develop`). Range: + `git diff ...HEAD`, `git log ..HEAD`. + For an MR/PR on request: fetch the diff (`glab mr view --raw`, + `gh pr diff `) and apply the same workflow. +2. If there are no changes to review, say so and stop. +3. If the diff is large (roughly >600 changed lines): review in passes, one + group of related files at a time, prioritizing source code over tests, docs, + and generated files. State explicitly which files you did not fully review. + +### Phase 2 — Context (the diff alone lies) + +1. Read the **intent**: branch name, commit messages, MR/issue description if + available. State the intent in one sentence before judging the code. +2. Read project guidelines: `AGENTS.md`, `AGENT.md`, `CLAUDE.md`, + `CONTRIBUTING.md` at the repo root and in changed directories. Only enforce + rules that are actually written there — and quote the exact rule. +3. Trace the change into the codebase: open the full files around the hunks, + grep callers of changed functions, follow the data flow end to end. Do not + judge code you have not seen in context. + +### Phase 3 — Review passes + +Run the passes in this order; spend effort where the real defects live: + +1. **Correctness and logic** — wrong conditions, off-by-one, inverted logic, + unhandled error paths (null, empty, timeout, retry), resource leaks, race + conditions, wrong state transitions. +2. **Contract breaks** — changed signatures or behavior vs. callers; API, DB, + or schema mismatches; serialization changes. +3. **Security at trust boundaries** — user input, files, network, env vars, + secrets in logs or history, auth checks, injection, unsafe deserialization. +4. **Tests** — which changed behavior has no test? Do new tests assert + outcomes, or merely execute code? +5. **Cheap checks** — if quick, run tests, linter, typecheck on the branch. + Report pass/fail in one line. + +### Phase 4 — Validate and score + +For every candidate finding, before it may be reported: + +1. **Validate**: re-read the actual code and confirm the trigger path exists. + If the issue is handled elsewhere, drop the finding. +2. **Score confidence** 0–100: 0 = false positive · 50 = real but minor · + 75 = real and important · 100 = certain. +3. **Report only findings >= 80.** Exception: potential data loss or security + impact with lower confidence — report it with an explicit uncertainty note. + +## Never report + +- Pre-existing issues the branch does not make worse +- Code that only looks wrong but is actually correct +- Nitpicks a senior engineer would not flag +- Issues a linter or formatter will catch +- Style or quality subjectives; hypothetical "might be a problem" without a trigger +- Issues in code the diff does not touch +- Anything already silenced in code (lint-ignore comments) + +## Output contract + +Output exactly this structure — no preamble, no praise, no summary of what the +code does: + +``` +# Code Review: + +Scope: ...HEAD, files (+/-). Checks: . + +## Findings + +- **[blocker|should-fix]** `path:line` — . Trigger: . Confidence: /100. + + +## Verdict +One line: safe to merge, or what must change first. +``` + +Severity definitions: + +- **blocker** — data loss, security hole, certain crash, broken contract. Merge + must not happen. +- **should-fix** — real defect or unhandled failure mode that will bite. + +There is no "nit" category — nitpicks are excluded entirely. + +## Notes for open-weight models (GLM, Qwen, DeepSeek) + +- Work through the phases strictly in order. Reason inside a phase (thinking + mode), but keep the final output to the contract above. +- Resist severity inflation: a finding without a concrete trigger scenario is + noise, not signal. +- Hard cap: at most 10 findings. If you found more, report the 10 highest- + severity ones and state how many you dropped. +- If context is missing to validate a candidate, drop the candidate — do not + report guesses. diff --git a/dev/dev-workflow/SKILL.md b/dev/dev-workflow/SKILL.md index bed3255..1b388ac 100644 --- a/dev/dev-workflow/SKILL.md +++ b/dev/dev-workflow/SKILL.md @@ -22,19 +22,19 @@ Discuss the task, idea, or bug with the coding agent. The outcome is an **implem - Summary of research and discussion - Implementation plan broken into discrete steps -Save the plan in the feature branch as `{feature-slug}-plan.md`. The file is deleted when the workflow completes. +Create a GitLab/Git issue for the plan. The issue title starts with the feature slug, and the description contains the full plan. **WAIT for explicit user approval before creating the issue.** ### 2. Implementation -Before starting, the agent reads `AGENT.md` (project guidelines). Implementation proceeds step-by-step according to the plan. Each step is committed individually. +**Only proceed if the user explicitly approves the plan** (e.g., `go`, `approved`, `start`). Do not start implementing on your own. Before starting, the agent reads `AGENT.md` (project guidelines). Implementation proceeds step-by-step according to the plan. Each step is committed individually. ### 3. Review -After implementation is complete, a subagent with **fresh context** reviews the code and implementation using `honest-reviewer`. +After implementation is complete, a subagent with **fresh context** reviews the code and implementation using `code-review`. ### 4. Iteration -The agent answers the reviewer's questions and fixes any issues. This cycle continues until the reviewer subagent approves. +The agent fixes any findings the reviewer reported. This cycle continues until the reviewer subagent approves. ### 5. Merge Request diff --git a/dev/honest-reviewer/SKILL.md b/dev/honest-reviewer/SKILL.md deleted file mode 100644 index bcd1280..0000000 --- a/dev/honest-reviewer/SKILL.md +++ /dev/null @@ -1,100 +0,0 @@ ---- -name: honest-reviewer -description: Reviews a feature branch as a devil's advocate and Socratic questioner. Covers code quality, security, test coverage, and minimalism. Produces ranked findings plus open questions that surface hidden assumptions instead of lecturing. Use for reviewing branches, MRs, PRs, or diffs before merge — or whenever the user says "honest review", "devil's advocate", "review my branch", "play socratic", or asks what could be wrong with a change. ---- - -# Honest Reviewer - -You review a feature branch the way a skeptical senior reviewer should: you -hunt for what's wrong — not to score points, but to make the merge safe. You -are the devil's advocate AND the reader's advocate: the code must survive -you, because it will survive worse. - -## Socratic questioning - -Every question you ask must pass all five tests: - -1. **Open** — it cannot be answered with yes or no. -2. **Forces thinking** — the answer is not immediately obvious. -3. **Surfaces assumptions** — it exposes hidden beliefs behind the change. -4. **Does not lecture** — you never smuggle the answer into the question. -5. **Respectful** — it serves understanding, not showing off. - -Ask questions you genuinely cannot answer from the code alone. If the diff -answers it, it is not a question — it is a finding. Never use a question as -a veiled accusation ("Don't you think this is over-engineered?" fails tests -1, 4, 5). - -## Workflow - -1. **Identify the base.** Ask or infer the default branch (`main`/`master`/`develop`). Review range: `git diff ...HEAD`, plus `git log ..HEAD`. -2. **Read the diff fully.** Every hunk. Skip nothing. -3. **Trace in context.** The diff lies without context: grep callers of changed functions, read the surrounding file, follow the actual data flow end to end before judging. -4. **Check the tests.** Which new tests exist, what do they actually assert (not just cover), and what behavior changed with no test at all? -5. **Run the cheap checks.** If quick: tests, linter, type check on the branch. Report what passed/failed. -6. **Write the review.** Findings, then Socratic questions. No placeholder praise, no filler. - -## Review dimensions - -### Code quality -Correctness and error handling first: what happens on the empty input, the -null, the timeout, the retry? Then readability: is the next reader's -boring-but-correct reading the right one? Duplication, dead code, copy-paste -drift, swallowed errors, misleading names. - -### Security -Anything that crosses a trust boundary: user input, files, network, env -vars, secrets in logs or git history, auth checks, injection, unsafe -deserialization, default-deny vs default-allow. Missing validation at a -boundary is a finding, not a question. - -### Test coverage -Not line coverage — behavior coverage. Is the happy path the only path? -Are edge cases and failure modes tested? Does the test assert the outcome or -just execute the code? Is the test coupled to implementation details so it -breaks on refactor but not on regression? A missing test for changed -behavior is a finding. - -### Minimalism -YAGNI and the standard library: does this change need to exist at all? Is -there an existing helper, stdlib, or platform feature that covers it? New -dependencies, new abstractions, config for values that never change, -scaffolding "for later" — flag them. Deletion is a feature. - -## Output format - -``` -# Honest Review: - -## Scope -..., files changed, additions, deletions. - -## Findings -Ranked by severity: **blocker** / **should-fix** / **nit**. -- **should-fix** `path:line` — what is wrong and why it matters. (state, evidence, impact; no fixes unless asked) -- ... - -## Socratic questions -Open questions that surface assumptions. Numbered, no answers attached. -1. When happens, what should the user see? -2. ... - -## Verdict -One line: what must be fixed before merge, what is safe to ship. -``` - -Severity rules: **blocker** = data loss, security hole, definite crash, -broken contract. **should-fix** = real risk or debt that will bite. **nit** -= style, naming, trivia — name them once, do not belabor. - -## Guardrails - -- Findings and questions are separated and never blurred. A finding states - facts; a question asks for reasoning the code cannot show. -- You question decisions, never people. No snark, no rhetorical gotchas. -- If something is good, say so in one line — a review that only attacks - teaches the author to hide things. -- You review the branch, not the whole codebase. Pre-existing issues only - count when the branch makes them worse or touches them. -- Never demand fixes you cannot justify with a concrete failure. "Could this - be more elegant?" is not a finding.