From b9c5644b16416efc1f618ddcdbf781e0107c82be Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 25 Aug 2026 15:54:44 +0000 Subject: [PATCH 1/2] Add code-review-calibration skill (non-default) Adds skills/code-review-calibration, a skill that mines a repository's own commit history, bug fixes, reverts, conventions, testing strategy, CI coverage, and past PR reviews to generate a calibrated .agents/skills/code-review.md with cited failure patterns. Registered in marketplaces/openhands-extensions.json without defaultEnabled, so it stays opt-in from the catalog UI. Co-authored-by: openhands --- README.md | 5 +- marketplaces/openhands-extensions.json | 14 + skills/code-review-calibration/SKILL.md | 707 ++++++++++++++++++++++++ skills/index.js | 7 + 4 files changed, 731 insertions(+), 2 deletions(-) create mode 100644 skills/code-review-calibration/SKILL.md diff --git a/README.md b/README.md index 2b883705..5dbdfa52 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ The JS and Python versions are kept in lock-step by `release-please` and guarded ## Extensions Catalog -This repository contains **2 marketplace(s)** with **68 extensions** (58 skills, 10 plugins). +This repository contains **2 marketplace(s)** with **69 extensions** (59 skills, 10 plugins). ### large-codebase @@ -115,7 +115,7 @@ OpenHands skills for interacting, improving, and refactoring large codebases Official skills and plugins for OpenHands — the open-source AI software engineer. -**64 extensions** (56 skills, 8 plugins) +**65 extensions** (57 skills, 8 plugins) | Name | Type | Description | Commands | |------|------|-------------|----------| @@ -130,6 +130,7 @@ Official skills and plugins for OpenHands — the open-source AI software engine | bitbucket-data-center | skill | Bitbucket Data Center (self-hosted Bitbucket Server) specifics — authenticate with BITBUCKET_DATA_CENTER_TOKEN, use t... | — | | city-weather | plugin | Get current weather, time, and precipitation forecast for any city using the free Open-Meteo API. Provides slash comm... | — | | code-review | skill | Rigorous code review focusing on data structures, simplicity, security, pragmatism, and risk/safety evaluation. Provi... | `/codereview`, `/codereview-roasted` | +| code-review-calibration | skill | Investigate a repository's commit history, bug fixes, reverts, house conventions, testing strategy, CI coverage, and ... | — | | code-simplifier | skill | Simplifies and refines code across three dimensions - code reuse, code quality, and efficiency - while preserving all... | `/simplify` | | datadog | skill | Query and analyze Datadog logs, metrics, APM traces, and monitors using the Datadog API. Use when debugging productio... | — | | deno | skill | Common project operations using Deno (tasks, run/test/lint/fmt, and dependency management). | — | diff --git a/marketplaces/openhands-extensions.json b/marketplaces/openhands-extensions.json index afa26ff8..81865ae2 100644 --- a/marketplaces/openhands-extensions.json +++ b/marketplaces/openhands-extensions.json @@ -197,6 +197,20 @@ "risk" ] }, + { + "name": "code-review-calibration", + "source": "./skills/code-review-calibration", + "description": "Investigate a repository's commit history, bug fixes, reverts, house conventions, testing strategy, CI coverage, and past PR reviews, then write a calibrated code-review skill whose failure patterns, conventions, and hotspots are all cited from that repository.", + "category": "code-quality", + "keywords": [ + "code-review", + "calibration", + "git-history", + "failure-patterns", + "conventions", + "hotspots" + ] + }, { "name": "datadog", "source": "./skills/datadog", diff --git a/skills/code-review-calibration/SKILL.md b/skills/code-review-calibration/SKILL.md new file mode 100644 index 00000000..6c3d4373 --- /dev/null +++ b/skills/code-review-calibration/SKILL.md @@ -0,0 +1,707 @@ +--- +name: code-review-calibration +description: >- + Investigates a repository end to end - commit history, bug fixes, reverts, + house conventions, testing strategy, CI coverage, and past PR review + comments - to learn how that specific codebase works and how it actually + breaks, then writes or updates `.agents/skills/code-review.md` with the + resulting failure patterns, conventions, and review rules. Use this + whenever the user wants a code review skill for their repo, wants the + reviewer calibrated or recalibrated, asks what this repo's recurring bugs, + conventions, or problem areas are, asks where bugs cluster, asks what the + testing norms are, or says the current review skill gives generic + feedback. Also use when onboarding to an unfamiliar codebase and needing + to know its idioms and dangerous areas. Prefer this over writing review + guidelines from general principles - the entire point is that every rule + comes from evidence in this repository. +--- + +# Code Review Calibration + +Investigate this repository twice over: **how it breaks** (history) and +**how it works** (conventions). Encode both as a review skill at +`.agents/skills/code-review.md`. + +**The output is a file, not a report.** Analysis that ends in the chat +window is wasted. Finish by writing the file. + +## Why both halves matter + +Defect history tells a reviewer what to fear. Conventions tell a reviewer +what "correct" looks like here, which is what lets them say "use +`scopedQuery()`" instead of "consider filtering by tenant." A reviewer +armed only with bug patterns produces suspicion; one that also knows the +house style produces fixes. + +## Ground rules + +**Statistics narrow the search; reading is the work.** The commands below +tell you which twenty commits and which six files to read out of thousands. +They do not tell you what the patterns are. A generated file full of +rankings and no named failure modes is a failed run. + +**Two occurrences minimum for a failure pattern.** One is an anecdote. +Encoding it produces false positives forever and teaches the author to +ignore the reviewer. Conventions are different - an idiom followed +consistently across the codebase counts even without a bug behind it. + +**Cite everything.** Failure patterns cite commit SHAs. Conventions cite +file paths where the idiom is visible. "This matches the bug in `9f2a1bc`" +and "the codebase does this in `db/query.ts:44`" both survive a skeptical +author. "Consider adding a null check" does not. + +**Six to twelve failure patterns.** Past fifteen the review loses focus. +Keep the ones that were most *expensive* - reached production, caused +reverts, took several attempts - not the most frequent. + +**Non-interactive gotcha:** several git commands read stdin when not +attached to a terminal and silently return nothing. Always pass an explicit +rev: `git shortlog -sn HEAD -- path`, never `git shortlog -sn -- path`. + +--- + +# PART I - Orientation + +## Phase 0 - Get your bearings + +```bash +git rev-parse --show-toplevel && git log --oneline | wc -l +git log -1 --format=%ad --date=short && git log --reverse -1 --format=%ad --date=short +ls .agents/skills/ 2>/dev/null +``` + +If `.agents/skills/code-review.md` exists, read it now. You are updating +it, not replacing it - see **Update mode**. + +Read whatever the project already says about itself. Anything documented +here is a convention you can confirm rather than infer: + +```bash +cat CLAUDE.md AGENTS.md CONTRIBUTING.md ARCHITECTURE.md 2>/dev/null | head -120 +ls .github/PULL_REQUEST_TEMPLATE* .github/pull_request_template* 2>/dev/null +``` + +Note the default branch; commands below assume `main`. + +Get the shape of the codebase - languages, size, layout: + +```bash +git ls-files | sed -n 's/.*\.\([a-zA-Z0-9]\+\)$/\1/p' | sort | uniq -c | sort -rn | head -12 +git ls-files | awk -F/ 'NF>1{print $1"/"$2}' | sort | uniq -c | sort -rn | head -20 +``` + +## Phase 1 - Calibrate fix detection (do not skip) + +Everything in Part II depends on correctly identifying bug fixes. Get this +wrong and the run is noise. Find out how the team writes commit messages: + +```bash +git log --no-merges --pretty=format:%s -800 \ + | grep -oiE '^[a-z]+(\([a-z0-9 _-]+\))?:' | sort | uniq -c | sort -rn | head +``` + +- **Shows `fix:`, `feat:`, `chore:`** → conventional commits. Use + `--grep='^fix'` alone. Precise. +- **Empty or scattered** → prose messages. Use the broad pattern, then + check the rate below. +- **Ticket refs** (`JIRA-123`, `#456`) → prefer matching the ticket + pattern; if `gh` is available, cross-reference which issues were labeled + as bugs. + +```bash +TOT=$(git log --no-merges --oneline | wc -l) +FIX=$(git log --no-merges --oneline -i --grep='fix' --grep='bug' --grep='revert' | wc -l) +echo "total=$TOT fix=$FIX rate=$(( 100 * FIX / TOT ))%" +``` + +**A plausible fix rate is 10-25%.** Over 30% means over-matching - probably +catching "fix up", "prefix", "suffix". Tighten and re-run. Under 5% means +under-matching; widen. Record the pattern you settle on; the generated file +must document it so the next run is reproducible. + +Set the exclusion list once and reuse it throughout. Add repo-specific +noise you spot (generated clients, docs sites, fixtures): + +```bash +EXCL='(^|/)(vendor|node_modules|third_party|dist|build|target)/|\.(lock|sum|min\.js|min\.css|map|svg|png|jpg|woff2?)$|(package-lock|yarn\.lock|Cargo\.lock|poetry\.lock)' +``` + +--- + +# PART II - How this codebase breaks + +## Phase 2 - Hotspots + +Rank by *bug-fix* touches, not raw churn. Churn surfaces your router and +your config file; fix-density surfaces code people keep getting wrong. + +```bash +git log --no-merges -i --grep='fix' --grep='bug' --grep='revert' \ + --since="18 months ago" --pretty=format: --name-only \ + | grep -vE "^$|$EXCL" | sort | uniq -c | awk '{print $2" "$1}' | sort > /tmp/fix.txt + +git log --no-merges --since="18 months ago" --pretty=format: --name-only \ + | grep -vE "^$|$EXCL" | sort | uniq -c | awk '{print $2" "$1}' | sort > /tmp/all.txt + +join /tmp/fix.txt /tmp/all.txt \ + | awk '$3>=5 {printf "%-4d %3d%% %-5d %s\n", $2, 100*$2/$3, $3, $1}' \ + | sort -rn | head -25 +``` + +Columns: fixes, fix-ratio, total commits, path. + +Read with judgment. 30 fixes out of 200 commits is a *busy* file. 12 out of +20 is a *broken* file - more interesting despite ranking lower. Weight +high-ratio files when choosing what to investigate. Drop files that no +longer exist (`git ls-files` to check). + +## Phase 3 - Reverts and fix-inducing commits + +### Reverts + +Small sample, highest signal per item: something shipped, got through +review, and had to be pulled. + +```bash +git log --no-merges -i --grep='^revert' --pretty=format:'%h %ad %s' --date=short | head -20 +``` + +`git show` every one. Ask what a reviewer could plausibly have caught. If +the answer is "nothing, it was an infra failure," discard it. + +### Fix-inducing commits (SZZ) + +For each bug fix, blame the lines it *deleted* against the parent. That +points at what introduced the defect, so you read the change that caused +the problem rather than the one that cleaned it up. + +```bash +git log --no-merges -i --grep='fix' --grep='bug' --since="18 months ago" \ + --pretty=format:%H | head -150 > /tmp/fixshas.txt +: > /tmp/introducers.txt + +while read sha; do + nf=$(git show --format= --name-only "$sha" | grep -c .) + [ "$nf" -gt 25 ] && continue # skip sweeps: reformats, mass renames + git show -U0 --format= --no-renames --diff-filter=M "$sha" 2>/dev/null | awk ' + /^--- a\// { f=substr($0,7); next } + /^@@ / && f { split($2,h,","); s=substr(h[1],2); c=(h[2]==""?1:h[2]); + if (c>0) print f" "s" "c } + ' | while read f s c; do + git blame -w --porcelain -L "$s,+$c" "$sha^" -- "$f" 2>/dev/null \ + | grep -oE '^[0-9a-f]{40}' + done +done < /tmp/fixshas.txt >> /tmp/introducers.txt + +sort /tmp/introducers.txt | uniq -c | sort -rn | head -20 | while read n s; do + echo "$n $(git show -s --format='%h %ad %s' --date=short $s | cut -c1-90)" +done +``` + +**Expect about half to be noise.** Blame credits whoever last rewrote a +line, so refactors float up without having caused anything. Published SZZ +benchmarks put precision near 0.6 - fine for "give me twenty commits to +read," useless as a verdict. Treat as leads. + +Then do the actual work: + +```bash +git show +git log --oneline --since= -- +``` + +Read each introducing commit next to the fix that followed. **This is where +patterns come from.** Look for the shape of the mistake, not the mistake. + +## Phase 4 - Temporal coupling + +Files that keep changing together encode an invariant the codebase does not +enforce. Cross-module pairs are the useful ones. + +```bash +git log --no-merges --since="18 months ago" --pretty=format:'@%H' --name-only | awk ' + /^@/ { if (n>1 && n<=25) for(i=1;ib){t=a;a=b;b=t} print a" "b } + n=1; next } + NF { f[n++]=$0 } +' | grep -vE "$EXCL" | sort | uniq -c | sort -rn | head -30 +``` + +Discard the uninformative - a file and its own test, a file and its +snapshot. Keep pairs crossing a module boundary, and pairs where one side +is generated from the other. These become "you touched X, did you update +Y?" rules. + +--- + +# PART III - How this codebase works + +This half is what lets the reviewer prescribe rather than merely suspect. + +## Phase 5 - Conventions and idioms + +Grepping for conventions across arbitrary languages does not generalize. +**Reading exemplars does.** Pick changes the team evidently considered +good - substantial, merged, not reverted, not later bug-fixed - and read +them as style specimens. + +```bash +git log --no-merges --since="6 months ago" --pretty=format:'%h %ad %s' --date=short \ + --shortstat | grep -B1 -E '[0-9]+ files? changed' | head -60 +``` + +Pick 5-8 with meaningful size (roughly 3-15 files) that are *not* in your +fix list, and read them in full: `git show `. Extract: + +- **Error handling.** Exceptions or result types? Wrapped with context or + bubbled raw? Is there a house error type? What happens at the boundary? +- **Logging and observability.** Which logger, what structure, what gets a + trace or metric. Any rule about logging PII? +- **Config and secrets.** Env vars, a config object, injection? +- **Async and concurrency.** Which primitives are blessed, which avoided. +- **Boundaries.** Do handlers hit the DB directly or go through a + repository layer? Where does validation live? + +Then find the **blessed abstractions** - the internal helpers everything +imports. Adapt to the language, and filter to first-party prefixes or +stdlib imports will dominate: + +```bash +# adjust glob + regex per language; filter to your own module prefix +git ls-files '*.ts' '*.tsx' | head -400 | tr '\n' '\0' \ + | xargs -0 grep -hoE "from ['\"][@~./][^'\"]*['\"]" 2>/dev/null \ + | sort | uniq -c | sort -rn | head -20 +``` + +High-gravity internal modules are what a reviewer should redirect people +toward. If `lib/db/scoped.ts` has 90 importers and a new file queries the +ORM directly, that is a finding. + +### Direction of travel + +The most useful convention data is what the codebase is moving *away* +from. New code using a deprecated approach is a real, checkable defect - +and invisible without this step. + +```bash +git log --no-merges -i --grep='migrat' --grep='refactor' --grep='deprecat' \ + --grep='replace' --grep='rewrite' --grep='switch to' --grep='move to' \ + --pretty='%h %ad %s' --date=short | head -25 +``` + +Read the promising ones. Subjects like "consumer API is now deprecated" or +"use open_doc instead" hand you the rule directly. Also check for an +*unfinished* migration - if both old and new patterns exist in the tree, +the reviewer needs to know which side is which and that new code belongs on +the new side. + +## Phase 6 - Testing strategy + +Locate tests and identify the framework: + +```bash +TESTPAT='(^|/)(tests?|spec|specs|__tests__)/|[._-](test|spec)\.[a-z]+$|(^|/)test_[^/]*$|_test\.[a-z]+$' +git ls-files | grep -E "$TESTPAT" | head -20 +git ls-files | grep -cE "$TESTPAT" +cat package.json pyproject.toml Cargo.toml go.mod 2>/dev/null | grep -iE 'test|jest|vitest|pytest|mocha|rspec' | head +``` + +Read two or three test files near your hotspots. Note the idioms: fixtures +vs factories vs inline setup, what gets mocked and what runs for real, how +the database is handled, whether integration or e2e tiers exist and what +distinguishes them. + +### Regression-test discipline + +Measurable, and it tells you whether "add a test" is a real norm here or an +aspiration nobody enforces: + +```bash +tot=0; wt=0 +for sha in $(git log --no-merges -i --grep='fix' --grep='bug' --pretty=%H | head -150); do + tot=$((tot+1)) + if git show --format= --name-only "$sha" | grep -qE "$TESTPAT"; then wt=$((wt+1)); fi +done +echo "fix commits=$tot shipped with test changes=$wt ($(( 100*wt/tot ))%)" +``` + +Interpretation: +- **60%+** → strong norm. A fix without a regression test is a legitimate, + well-supported finding. +- **25-60%** → inconsistent. Worth raising, framed as a suggestion. +- **under 25%** → not a norm. Do *not* have the reviewer demand tests on + every fix; it will be ignored and make the whole skill feel officious. + Record the number honestly so the team can decide. + +### Untested hotspots + +The intersection of "breaks often" and "has no test" is the single +highest-value thing a reviewer can know. For each top hotspot, check +whether anything test-shaped ever changes alongside it: + +```bash +for f in ; do + n=$(git log --no-merges --pretty=%H HEAD -- "$f" | head -60 | while read s; do + git show --format= --name-only "$s" | grep -qE "$TESTPAT" && echo x + done | wc -l) + echo "$f -> co-changed with tests in $n of last 60 commits" +done +``` + +A hotspot that never co-changes with a test is a file where every change is +unverified. Say so explicitly. + +## Phase 7 - What CI already enforces (the boundary) + +**A reviewer that duplicates the linter is noise.** Establish what is +already automated so the generated skill can explicitly stay off it. + +```bash +ls .github/workflows/ 2>/dev/null && cat .github/workflows/*.y*ml 2>/dev/null \ + | grep -E '^\s*-?\s*(run|uses|name):' | head -50 +cat .pre-commit-config.yaml 2>/dev/null | grep -E 'repo:|id:' | head -20 +ls .eslintrc* eslint.config.* .ruff.toml ruff.toml .rubocop.yml tsconfig.json 2>/dev/null +``` + +Check strictness too, because it changes what is worth flagging. A repo +with `strict: true` in tsconfig does not need a human watching nullability; +one without it very much does. + +Write the list down. The generated file gets an explicit "already covered, +do not comment on" section. This is as important as anything in Part II - +it is what keeps reviews short enough to be read. + +## Phase 8 - Ownership, stability, and blast radius + +### Bus factor on hotspots + +```bash +for f in ; do + echo "== $f" + git log --no-merges --pretty=%an HEAD -- "$f" | sort | uniq -c | sort -rn | head -3 +done +``` + +A single-author file with a high fix rate is dangerous in a specific way: +the person who understands it may not be the person reviewing the change. +Flag it so the reviewer can pull in the right human. + +### Stability + +```bash +git ls-files | head -500 | while read f; do + echo "$(git log -1 --format=%ad --date=short HEAD -- "$f") $f" +done | sort | head -20 +``` + +Code untouched for years is load-bearing and under-understood. A diff +touching it deserves more scrutiny than its size suggests, not less. + +### Blast radius + +Frequency-based analysis has a blind spot: code that rarely changes but is +catastrophic when wrong. Hotspots will never surface it. Find these by +inspection - auth and sessions, permission checks, payments and billing, +migrations, cryptography, anything touching PII, deletion paths, public API +contracts. + +```bash +git ls-files | grep -iE 'auth|session|permission|billing|payment|migrat|crypto|secret|token|delete|purge' | head -30 +``` + +These get their own list: **low frequency, high severity - scrutinize +regardless of diff size.** + +--- + +# PART IV - What reviewers already say + +## Phase 9 - GitHub PR reviews (optional) + +Skip if `gh auth status` fails. When available it is valuable: the commit +log shows what broke, review comments show what the team *already knows* is +easy to get wrong. Patterns confirmed by both are the strongest you'll get. + +```bash +REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) + +gh api "repos/$REPO/pulls/comments?per_page=100&sort=created&direction=desc" \ + --jq '.[] | [(.pull_request_url|split("/")|last), .user.login, .path, (.body|gsub("\n";" ")|.[0:180])] | @tsv' \ + > /tmp/review-comments.tsv +wc -l /tmp/review-comments.tsv +``` + +Find the **10 most recent substantial reviews** - where a reviewer engaged +rather than stamped. Group by PR, keep those with 3+ comments: + +```bash +cut -f1 /tmp/review-comments.tsv | uniq -c | sort -rn | awk '$1>=3' | head -10 +``` + +If that yields fewer than 10, paginate (`&page=2`) or fall back to PRs that +were sent back: + +```bash +gh pr list --state merged --limit 40 --json number,title,reviewDecision \ + --jq '.[] | select(.reviewDecision=="CHANGES_REQUESTED") | "\(.number)\t\(.title)"' +``` + +Read each thread with its surrounding diff: + +```bash +gh api "repos/$REPO/pulls//comments" \ + --jq '.[] | "--- \(.path):\(.line // .original_line) [\(.user.login)]\n\(.diff_hunk)\n>> \(.body)\n"' +``` + +Sort what you find into three buckets: + +- **Substantive** - "needs the tenant filter", "this migration will lock", + "you're swallowing the error". These become review patterns. +- **Conventional** - "use the `X` helper", "validation goes in the schema". + These confirm or extend Phase 5, often more reliably than inference did. +- **Procedural** - nits, naming, changelog reminders. These belong in a + linter or PR template, *not* a review skill. List them separately as + automation candidates; that turns noise into a useful byproduct. + +Note **who** raises what. A reviewer who consistently catches one class of +issue is institutional knowledge currently dependent on their availability. + +--- + +# PART V - Synthesize and write + +## Phase 10 - Synthesize + +For each **failure pattern**: + +1. **Trigger** - what must be in a diff to make this worth checking. + Specific paths or constructs, not "when reviewing backend code." +2. **Failure** - what goes wrong, and *why it survived review last time*. + If tests and review both passed, say what made it invisible. +3. **Tell** - what a reviewer can literally look for in the diff. +4. **Citations** - two or more SHAs. Non-negotiable. + +For each **convention**: the rule, the blessed helper, and a file path +where it is visible. + +Be specific to the point of discomfort. "Null pointer bugs" is not a +pattern. "New handlers under `api/` query the ORM directly instead of +through `scopedQuery()`, losing the tenant filter, and single-tenant test +fixtures never catch it" is - and note how it fuses a defect pattern with a +convention and a testing gap. Those fusions are the best output this +process produces; look for them deliberately. + +Discard candidates whose cause was designed away. If a type change or new +helper made a bug class unrepresentable, that pattern is dead - record it +in Retired so nobody re-adds it. + +Cross-check the halves against each other: + +- A convention nobody follows *in the hotspot files* is worth flagging + hard - that gap is likely causing the bugs. +- A failure pattern that CI now catches should be deleted, not documented. +- A hotspot that is also blast-radius and also untested is your headline + finding. Lead with it. + +## Phase 11 - Write the file + +Write `.agents/skills/code-review.md`, creating `.agents/skills/` if +needed. The HTML comment markers matter - they let future runs regenerate +evidence sections without destroying human edits. + +````markdown +--- +name: code-review +description: >- + Reviews changes against 's own history and conventions - its + documented failure patterns, hotspot files, house idioms, testing norms, + and coupling rules, all derived from mining this repository. Use whenever + the user asks for a code review, asks you to look over a diff, branch, PR, + or staged changes, asks "does this look right" or "what could break", or + is about to commit or open a pull request. Also use proactively after + writing or modifying code in this repo, before reporting work as done. +--- + +# Code Review - + +Review against what has actually broken here and how this codebase is +actually written. Every finding cites a commit or a file path; findings +that cite nothing are nits and belong in the linter. + +## Scope + +1. User named a scope → use it. +2. Staged changes exist (`git diff --cached --stat`) → review those. +3. On a feature branch → `git diff $(git merge-base HEAD main)...HEAD` +4. Otherwise → `git show HEAD` + +## Before reviewing + +Check whether the diff touches a hotspot or blast-radius file below. If so, +say which, and raise scrutiny. Then per modified file: + +```bash +git log --oneline -12 HEAD -- +``` + +A hunk sitting on lines a recent fix touched is worth flagging on its own - +code just fixed and being changed again is code not well understood. + +## Already covered by CI - do not comment on + + + + +Raise these only when the diff would *disable* or *bypass* a check. + + +## Failure patterns + + +### P1 - + +**Check when:** +**Failure:** +**Tell:** +**Instead:** +**Seen in:** `` (, ), `` (...) + + +## House conventions + +Deviations are findings - cite the convention, not personal preference. + + +**Error handling:** - see `` +**Logging:** - see `` +**Data access:** - see `` +**Validation:** - see `` +**Blessed helpers:** `` ( importers) for + +**Direction of travel** - new code belongs on the new side: + +| Deprecated | Current | Since | +|---|---|---| +| `` | `` | `` | + + +## Testing expectations + + +**Framework:** - tests in ``, named `` +**Idioms:** - see `` +**Regression discipline:** % of bug fixes ship with test changes. + +**Untested hotspots:** `` - changes here are effectively unverified; +flag any non-trivial modification. + + +## Coupling rules + +Touching one side without the other → ask why. + + +| If the diff touches | It probably also needs | Co-changed | +|---|---|---:| + + +## Hotspot files + + +| File | Bug fixes | Commits | Fix ratio | Primary author | +|---|---:|---:|---:|---| + + +## Blast radius + +Rarely changed, expensive when wrong. Scrutinize regardless of diff size. + + +- `` - + + +## Reviewer conventions + + +Recurring substantive concerns from past reviews: +- - raised on #, # + +Recurring procedural nits - should be automated, do not raise manually: +- - candidate for lint rule / PR template + + +## Output format + +Group by confidence, highest first. Each finding: location, pattern or +convention matched, the evidence, the concrete fix. + +``` +### High confidence + +**`src/billing/invoice.ts:142` - missing tenant scope on new query** +Matches P1. Same omission caused the leak fixed in `9f2a1bc` and again in +`44de0a1`. Use `scopedQuery()` (see `src/lib/db/scoped.ts`). +``` + +Close with which hotspot or blast-radius files the diff touched. + +## Rules + +- **Cite or drop it.** No historical evidence, no convention, no concrete + failure mode → it is a nit. A short review that is all signal beats a + long one people learn to skim. +- **Do not duplicate CI.** See the covered list above. +- **Do not restyle.** Formatting and naming are the formatter's job. +- **Say when it's clean.** If the diff avoids every known trap, say so and + stop. Manufacturing findings trains people to ignore this skill. +- **Absence of a pattern is not absence of a bug.** The catalog covers what + has broken before, not everything that can break. Still raise things that + look wrong - mark them as judgment rather than documented pattern, and + flag them as candidates for the catalog. + +## Retired patterns + + +Kept so nobody re-adds a rule a design change made moot. +- - retired , + + +--- + +Generated by `code-review-calibration` on . +Window: . Commits: . Fix commits: (%). +Fix-detection pattern: `` +Exemplar changes read: . PR reviews sampled: . +Regenerate quarterly, or after any significant incident. + +```` + +## Update mode + +If the file already existed: + +- **Regenerate only marked sections.** Everything outside + `` is human-authored. Leave it. +- **Never silently drop a pattern.** A pattern absent from new evidence may + have been fixed - or your fix-regex may have changed. Move it to Retired + with a reason; do not delete. +- **Preserve hand-written entries.** A pattern with no SHAs may come from + an incident predating your window. Keep it, mark `(manually added)`. +- **Report the delta.** What changed between runs: new and retired + patterns, hotspots that entered or left, whether regression-test + discipline moved, conventions that shifted. That delta is often more + interesting than the file. + +## Finally + +Report briefly: commits analyzed, the fix-detection pattern you settled on +and why, patterns kept versus discarded, conventions found, and your honest +confidence in each half. + +The two halves fail independently. A young repo may have too little history +for good failure patterns while still having perfectly legible conventions +- in that case say the conventions half is solid and the history half thin, +and write the file with a strong conventions section and few patterns. +**Name the weak parts rather than padding with speculation.** Four +well-evidenced patterns beat twelve invented ones. diff --git a/skills/index.js b/skills/index.js index c7bc3a61..06dace23 100644 --- a/skills/index.js +++ b/skills/index.js @@ -112,6 +112,13 @@ export const SKILLS_CATALOG = [ "category": "code-quality", "defaultEnabled": true }, + { + "name": "code-review-calibration", + "description": "Investigates a repository end to end - commit history, bug fixes, reverts, house conventions, testing strategy, CI coverage, and past PR review comments - to learn how that specific codebase works and how it actually breaks, then writes or updates `.agents/skills/code-review.md` with the resulting failure patterns, conventions, and review rules. Use this whenever the user wants a code review skill for their repo, wants the reviewer calibrated or recalibrated, asks what this repo's recurring bugs, conventions, or problem areas are, asks where bugs cluster, asks what the testing norms are, or says the current review skill gives generic feedback. Also use when onboarding to an unfamiliar codebase and needing to know its idioms and dangerous areas. Prefer this over writing review guidelines from general principles - the entire point is that every rule comes from evidence in this repository.", + "triggers": [], + "content": "# Code Review Calibration\n\nInvestigate this repository twice over: **how it breaks** (history) and\n**how it works** (conventions). Encode both as a review skill at\n`.agents/skills/code-review.md`.\n\n**The output is a file, not a report.** Analysis that ends in the chat\nwindow is wasted. Finish by writing the file.\n\n## Why both halves matter\n\nDefect history tells a reviewer what to fear. Conventions tell a reviewer\nwhat \"correct\" looks like here, which is what lets them say \"use\n`scopedQuery()`\" instead of \"consider filtering by tenant.\" A reviewer\narmed only with bug patterns produces suspicion; one that also knows the\nhouse style produces fixes.\n\n## Ground rules\n\n**Statistics narrow the search; reading is the work.** The commands below\ntell you which twenty commits and which six files to read out of thousands.\nThey do not tell you what the patterns are. A generated file full of\nrankings and no named failure modes is a failed run.\n\n**Two occurrences minimum for a failure pattern.** One is an anecdote.\nEncoding it produces false positives forever and teaches the author to\nignore the reviewer. Conventions are different - an idiom followed\nconsistently across the codebase counts even without a bug behind it.\n\n**Cite everything.** Failure patterns cite commit SHAs. Conventions cite\nfile paths where the idiom is visible. \"This matches the bug in `9f2a1bc`\"\nand \"the codebase does this in `db/query.ts:44`\" both survive a skeptical\nauthor. \"Consider adding a null check\" does not.\n\n**Six to twelve failure patterns.** Past fifteen the review loses focus.\nKeep the ones that were most *expensive* - reached production, caused\nreverts, took several attempts - not the most frequent.\n\n**Non-interactive gotcha:** several git commands read stdin when not\nattached to a terminal and silently return nothing. Always pass an explicit\nrev: `git shortlog -sn HEAD -- path`, never `git shortlog -sn -- path`.\n\n---\n\n# PART I - Orientation\n\n## Phase 0 - Get your bearings\n\n```bash\ngit rev-parse --show-toplevel && git log --oneline | wc -l\ngit log -1 --format=%ad --date=short && git log --reverse -1 --format=%ad --date=short\nls .agents/skills/ 2>/dev/null\n```\n\nIf `.agents/skills/code-review.md` exists, read it now. You are updating\nit, not replacing it - see **Update mode**.\n\nRead whatever the project already says about itself. Anything documented\nhere is a convention you can confirm rather than infer:\n\n```bash\ncat CLAUDE.md AGENTS.md CONTRIBUTING.md ARCHITECTURE.md 2>/dev/null | head -120\nls .github/PULL_REQUEST_TEMPLATE* .github/pull_request_template* 2>/dev/null\n```\n\nNote the default branch; commands below assume `main`.\n\nGet the shape of the codebase - languages, size, layout:\n\n```bash\ngit ls-files | sed -n 's/.*\\.\\([a-zA-Z0-9]\\+\\)$/\\1/p' | sort | uniq -c | sort -rn | head -12\ngit ls-files | awk -F/ 'NF>1{print $1\"/\"$2}' | sort | uniq -c | sort -rn | head -20\n```\n\n## Phase 1 - Calibrate fix detection (do not skip)\n\nEverything in Part II depends on correctly identifying bug fixes. Get this\nwrong and the run is noise. Find out how the team writes commit messages:\n\n```bash\ngit log --no-merges --pretty=format:%s -800 \\\n | grep -oiE '^[a-z]+(\\([a-z0-9 _-]+\\))?:' | sort | uniq -c | sort -rn | head\n```\n\n- **Shows `fix:`, `feat:`, `chore:`** → conventional commits. Use\n `--grep='^fix'` alone. Precise.\n- **Empty or scattered** → prose messages. Use the broad pattern, then\n check the rate below.\n- **Ticket refs** (`JIRA-123`, `#456`) → prefer matching the ticket\n pattern; if `gh` is available, cross-reference which issues were labeled\n as bugs.\n\n```bash\nTOT=$(git log --no-merges --oneline | wc -l)\nFIX=$(git log --no-merges --oneline -i --grep='fix' --grep='bug' --grep='revert' | wc -l)\necho \"total=$TOT fix=$FIX rate=$(( 100 * FIX / TOT ))%\"\n```\n\n**A plausible fix rate is 10-25%.** Over 30% means over-matching - probably\ncatching \"fix up\", \"prefix\", \"suffix\". Tighten and re-run. Under 5% means\nunder-matching; widen. Record the pattern you settle on; the generated file\nmust document it so the next run is reproducible.\n\nSet the exclusion list once and reuse it throughout. Add repo-specific\nnoise you spot (generated clients, docs sites, fixtures):\n\n```bash\nEXCL='(^|/)(vendor|node_modules|third_party|dist|build|target)/|\\.(lock|sum|min\\.js|min\\.css|map|svg|png|jpg|woff2?)$|(package-lock|yarn\\.lock|Cargo\\.lock|poetry\\.lock)'\n```\n\n---\n\n# PART II - How this codebase breaks\n\n## Phase 2 - Hotspots\n\nRank by *bug-fix* touches, not raw churn. Churn surfaces your router and\nyour config file; fix-density surfaces code people keep getting wrong.\n\n```bash\ngit log --no-merges -i --grep='fix' --grep='bug' --grep='revert' \\\n --since=\"18 months ago\" --pretty=format: --name-only \\\n | grep -vE \"^$|$EXCL\" | sort | uniq -c | awk '{print $2\" \"$1}' | sort > /tmp/fix.txt\n\ngit log --no-merges --since=\"18 months ago\" --pretty=format: --name-only \\\n | grep -vE \"^$|$EXCL\" | sort | uniq -c | awk '{print $2\" \"$1}' | sort > /tmp/all.txt\n\njoin /tmp/fix.txt /tmp/all.txt \\\n | awk '$3>=5 {printf \"%-4d %3d%% %-5d %s\\n\", $2, 100*$2/$3, $3, $1}' \\\n | sort -rn | head -25\n```\n\nColumns: fixes, fix-ratio, total commits, path.\n\nRead with judgment. 30 fixes out of 200 commits is a *busy* file. 12 out of\n20 is a *broken* file - more interesting despite ranking lower. Weight\nhigh-ratio files when choosing what to investigate. Drop files that no\nlonger exist (`git ls-files` to check).\n\n## Phase 3 - Reverts and fix-inducing commits\n\n### Reverts\n\nSmall sample, highest signal per item: something shipped, got through\nreview, and had to be pulled.\n\n```bash\ngit log --no-merges -i --grep='^revert' --pretty=format:'%h %ad %s' --date=short | head -20\n```\n\n`git show` every one. Ask what a reviewer could plausibly have caught. If\nthe answer is \"nothing, it was an infra failure,\" discard it.\n\n### Fix-inducing commits (SZZ)\n\nFor each bug fix, blame the lines it *deleted* against the parent. That\npoints at what introduced the defect, so you read the change that caused\nthe problem rather than the one that cleaned it up.\n\n```bash\ngit log --no-merges -i --grep='fix' --grep='bug' --since=\"18 months ago\" \\\n --pretty=format:%H | head -150 > /tmp/fixshas.txt\n: > /tmp/introducers.txt\n\nwhile read sha; do\n nf=$(git show --format= --name-only \"$sha\" | grep -c .)\n [ \"$nf\" -gt 25 ] && continue # skip sweeps: reformats, mass renames\n git show -U0 --format= --no-renames --diff-filter=M \"$sha\" 2>/dev/null | awk '\n /^--- a\\// { f=substr($0,7); next }\n /^@@ / && f { split($2,h,\",\"); s=substr(h[1],2); c=(h[2]==\"\"?1:h[2]);\n if (c>0) print f\" \"s\" \"c }\n ' | while read f s c; do\n git blame -w --porcelain -L \"$s,+$c\" \"$sha^\" -- \"$f\" 2>/dev/null \\\n | grep -oE '^[0-9a-f]{40}'\n done\ndone < /tmp/fixshas.txt >> /tmp/introducers.txt\n\nsort /tmp/introducers.txt | uniq -c | sort -rn | head -20 | while read n s; do\n echo \"$n $(git show -s --format='%h %ad %s' --date=short $s | cut -c1-90)\"\ndone\n```\n\n**Expect about half to be noise.** Blame credits whoever last rewrote a\nline, so refactors float up without having caused anything. Published SZZ\nbenchmarks put precision near 0.6 - fine for \"give me twenty commits to\nread,\" useless as a verdict. Treat as leads.\n\nThen do the actual work:\n\n```bash\ngit show \ngit log --oneline --since= -- \n```\n\nRead each introducing commit next to the fix that followed. **This is where\npatterns come from.** Look for the shape of the mistake, not the mistake.\n\n## Phase 4 - Temporal coupling\n\nFiles that keep changing together encode an invariant the codebase does not\nenforce. Cross-module pairs are the useful ones.\n\n```bash\ngit log --no-merges --since=\"18 months ago\" --pretty=format:'@%H' --name-only | awk '\n /^@/ { if (n>1 && n<=25) for(i=1;ib){t=a;a=b;b=t} print a\" \"b }\n n=1; next }\n NF { f[n++]=$0 }\n' | grep -vE \"$EXCL\" | sort | uniq -c | sort -rn | head -30\n```\n\nDiscard the uninformative - a file and its own test, a file and its\nsnapshot. Keep pairs crossing a module boundary, and pairs where one side\nis generated from the other. These become \"you touched X, did you update\nY?\" rules.\n\n---\n\n# PART III - How this codebase works\n\nThis half is what lets the reviewer prescribe rather than merely suspect.\n\n## Phase 5 - Conventions and idioms\n\nGrepping for conventions across arbitrary languages does not generalize.\n**Reading exemplars does.** Pick changes the team evidently considered\ngood - substantial, merged, not reverted, not later bug-fixed - and read\nthem as style specimens.\n\n```bash\ngit log --no-merges --since=\"6 months ago\" --pretty=format:'%h %ad %s' --date=short \\\n --shortstat | grep -B1 -E '[0-9]+ files? changed' | head -60\n```\n\nPick 5-8 with meaningful size (roughly 3-15 files) that are *not* in your\nfix list, and read them in full: `git show `. Extract:\n\n- **Error handling.** Exceptions or result types? Wrapped with context or\n bubbled raw? Is there a house error type? What happens at the boundary?\n- **Logging and observability.** Which logger, what structure, what gets a\n trace or metric. Any rule about logging PII?\n- **Config and secrets.** Env vars, a config object, injection?\n- **Async and concurrency.** Which primitives are blessed, which avoided.\n- **Boundaries.** Do handlers hit the DB directly or go through a\n repository layer? Where does validation live?\n\nThen find the **blessed abstractions** - the internal helpers everything\nimports. Adapt to the language, and filter to first-party prefixes or\nstdlib imports will dominate:\n\n```bash\n# adjust glob + regex per language; filter to your own module prefix\ngit ls-files '*.ts' '*.tsx' | head -400 | tr '\\n' '\\0' \\\n | xargs -0 grep -hoE \"from ['\\\"][@~./][^'\\\"]*['\\\"]\" 2>/dev/null \\\n | sort | uniq -c | sort -rn | head -20\n```\n\nHigh-gravity internal modules are what a reviewer should redirect people\ntoward. If `lib/db/scoped.ts` has 90 importers and a new file queries the\nORM directly, that is a finding.\n\n### Direction of travel\n\nThe most useful convention data is what the codebase is moving *away*\nfrom. New code using a deprecated approach is a real, checkable defect -\nand invisible without this step.\n\n```bash\ngit log --no-merges -i --grep='migrat' --grep='refactor' --grep='deprecat' \\\n --grep='replace' --grep='rewrite' --grep='switch to' --grep='move to' \\\n --pretty='%h %ad %s' --date=short | head -25\n```\n\nRead the promising ones. Subjects like \"consumer API is now deprecated\" or\n\"use open_doc instead\" hand you the rule directly. Also check for an\n*unfinished* migration - if both old and new patterns exist in the tree,\nthe reviewer needs to know which side is which and that new code belongs on\nthe new side.\n\n## Phase 6 - Testing strategy\n\nLocate tests and identify the framework:\n\n```bash\nTESTPAT='(^|/)(tests?|spec|specs|__tests__)/|[._-](test|spec)\\.[a-z]+$|(^|/)test_[^/]*$|_test\\.[a-z]+$'\ngit ls-files | grep -E \"$TESTPAT\" | head -20\ngit ls-files | grep -cE \"$TESTPAT\"\ncat package.json pyproject.toml Cargo.toml go.mod 2>/dev/null | grep -iE 'test|jest|vitest|pytest|mocha|rspec' | head\n```\n\nRead two or three test files near your hotspots. Note the idioms: fixtures\nvs factories vs inline setup, what gets mocked and what runs for real, how\nthe database is handled, whether integration or e2e tiers exist and what\ndistinguishes them.\n\n### Regression-test discipline\n\nMeasurable, and it tells you whether \"add a test\" is a real norm here or an\naspiration nobody enforces:\n\n```bash\ntot=0; wt=0\nfor sha in $(git log --no-merges -i --grep='fix' --grep='bug' --pretty=%H | head -150); do\n tot=$((tot+1))\n if git show --format= --name-only \"$sha\" | grep -qE \"$TESTPAT\"; then wt=$((wt+1)); fi\ndone\necho \"fix commits=$tot shipped with test changes=$wt ($(( 100*wt/tot ))%)\"\n```\n\nInterpretation:\n- **60%+** → strong norm. A fix without a regression test is a legitimate,\n well-supported finding.\n- **25-60%** → inconsistent. Worth raising, framed as a suggestion.\n- **under 25%** → not a norm. Do *not* have the reviewer demand tests on\n every fix; it will be ignored and make the whole skill feel officious.\n Record the number honestly so the team can decide.\n\n### Untested hotspots\n\nThe intersection of \"breaks often\" and \"has no test\" is the single\nhighest-value thing a reviewer can know. For each top hotspot, check\nwhether anything test-shaped ever changes alongside it:\n\n```bash\nfor f in ; do\n n=$(git log --no-merges --pretty=%H HEAD -- \"$f\" | head -60 | while read s; do\n git show --format= --name-only \"$s\" | grep -qE \"$TESTPAT\" && echo x\n done | wc -l)\n echo \"$f -> co-changed with tests in $n of last 60 commits\"\ndone\n```\n\nA hotspot that never co-changes with a test is a file where every change is\nunverified. Say so explicitly.\n\n## Phase 7 - What CI already enforces (the boundary)\n\n**A reviewer that duplicates the linter is noise.** Establish what is\nalready automated so the generated skill can explicitly stay off it.\n\n```bash\nls .github/workflows/ 2>/dev/null && cat .github/workflows/*.y*ml 2>/dev/null \\\n | grep -E '^\\s*-?\\s*(run|uses|name):' | head -50\ncat .pre-commit-config.yaml 2>/dev/null | grep -E 'repo:|id:' | head -20\nls .eslintrc* eslint.config.* .ruff.toml ruff.toml .rubocop.yml tsconfig.json 2>/dev/null\n```\n\nCheck strictness too, because it changes what is worth flagging. A repo\nwith `strict: true` in tsconfig does not need a human watching nullability;\none without it very much does.\n\nWrite the list down. The generated file gets an explicit \"already covered,\ndo not comment on\" section. This is as important as anything in Part II -\nit is what keeps reviews short enough to be read.\n\n## Phase 8 - Ownership, stability, and blast radius\n\n### Bus factor on hotspots\n\n```bash\nfor f in ; do\n echo \"== $f\"\n git log --no-merges --pretty=%an HEAD -- \"$f\" | sort | uniq -c | sort -rn | head -3\ndone\n```\n\nA single-author file with a high fix rate is dangerous in a specific way:\nthe person who understands it may not be the person reviewing the change.\nFlag it so the reviewer can pull in the right human.\n\n### Stability\n\n```bash\ngit ls-files | head -500 | while read f; do\n echo \"$(git log -1 --format=%ad --date=short HEAD -- \"$f\") $f\"\ndone | sort | head -20\n```\n\nCode untouched for years is load-bearing and under-understood. A diff\ntouching it deserves more scrutiny than its size suggests, not less.\n\n### Blast radius\n\nFrequency-based analysis has a blind spot: code that rarely changes but is\ncatastrophic when wrong. Hotspots will never surface it. Find these by\ninspection - auth and sessions, permission checks, payments and billing,\nmigrations, cryptography, anything touching PII, deletion paths, public API\ncontracts.\n\n```bash\ngit ls-files | grep -iE 'auth|session|permission|billing|payment|migrat|crypto|secret|token|delete|purge' | head -30\n```\n\nThese get their own list: **low frequency, high severity - scrutinize\nregardless of diff size.**\n\n---\n\n# PART IV - What reviewers already say\n\n## Phase 9 - GitHub PR reviews (optional)\n\nSkip if `gh auth status` fails. When available it is valuable: the commit\nlog shows what broke, review comments show what the team *already knows* is\neasy to get wrong. Patterns confirmed by both are the strongest you'll get.\n\n```bash\nREPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)\n\ngh api \"repos/$REPO/pulls/comments?per_page=100&sort=created&direction=desc\" \\\n --jq '.[] | [(.pull_request_url|split(\"/\")|last), .user.login, .path, (.body|gsub(\"\\n\";\" \")|.[0:180])] | @tsv' \\\n > /tmp/review-comments.tsv\nwc -l /tmp/review-comments.tsv\n```\n\nFind the **10 most recent substantial reviews** - where a reviewer engaged\nrather than stamped. Group by PR, keep those with 3+ comments:\n\n```bash\ncut -f1 /tmp/review-comments.tsv | uniq -c | sort -rn | awk '$1>=3' | head -10\n```\n\nIf that yields fewer than 10, paginate (`&page=2`) or fall back to PRs that\nwere sent back:\n\n```bash\ngh pr list --state merged --limit 40 --json number,title,reviewDecision \\\n --jq '.[] | select(.reviewDecision==\"CHANGES_REQUESTED\") | \"\\(.number)\\t\\(.title)\"'\n```\n\nRead each thread with its surrounding diff:\n\n```bash\ngh api \"repos/$REPO/pulls//comments\" \\\n --jq '.[] | \"--- \\(.path):\\(.line // .original_line) [\\(.user.login)]\\n\\(.diff_hunk)\\n>> \\(.body)\\n\"'\n```\n\nSort what you find into three buckets:\n\n- **Substantive** - \"needs the tenant filter\", \"this migration will lock\",\n \"you're swallowing the error\". These become review patterns.\n- **Conventional** - \"use the `X` helper\", \"validation goes in the schema\".\n These confirm or extend Phase 5, often more reliably than inference did.\n- **Procedural** - nits, naming, changelog reminders. These belong in a\n linter or PR template, *not* a review skill. List them separately as\n automation candidates; that turns noise into a useful byproduct.\n\nNote **who** raises what. A reviewer who consistently catches one class of\nissue is institutional knowledge currently dependent on their availability.\n\n---\n\n# PART V - Synthesize and write\n\n## Phase 10 - Synthesize\n\nFor each **failure pattern**:\n\n1. **Trigger** - what must be in a diff to make this worth checking.\n Specific paths or constructs, not \"when reviewing backend code.\"\n2. **Failure** - what goes wrong, and *why it survived review last time*.\n If tests and review both passed, say what made it invisible.\n3. **Tell** - what a reviewer can literally look for in the diff.\n4. **Citations** - two or more SHAs. Non-negotiable.\n\nFor each **convention**: the rule, the blessed helper, and a file path\nwhere it is visible.\n\nBe specific to the point of discomfort. \"Null pointer bugs\" is not a\npattern. \"New handlers under `api/` query the ORM directly instead of\nthrough `scopedQuery()`, losing the tenant filter, and single-tenant test\nfixtures never catch it\" is - and note how it fuses a defect pattern with a\nconvention and a testing gap. Those fusions are the best output this\nprocess produces; look for them deliberately.\n\nDiscard candidates whose cause was designed away. If a type change or new\nhelper made a bug class unrepresentable, that pattern is dead - record it\nin Retired so nobody re-adds it.\n\nCross-check the halves against each other:\n\n- A convention nobody follows *in the hotspot files* is worth flagging\n hard - that gap is likely causing the bugs.\n- A failure pattern that CI now catches should be deleted, not documented.\n- A hotspot that is also blast-radius and also untested is your headline\n finding. Lead with it.\n\n## Phase 11 - Write the file\n\nWrite `.agents/skills/code-review.md`, creating `.agents/skills/` if\nneeded. The HTML comment markers matter - they let future runs regenerate\nevidence sections without destroying human edits.\n\n````markdown\n---\nname: code-review\ndescription: >-\n Reviews changes against 's own history and conventions - its\n documented failure patterns, hotspot files, house idioms, testing norms,\n and coupling rules, all derived from mining this repository. Use whenever\n the user asks for a code review, asks you to look over a diff, branch, PR,\n or staged changes, asks \"does this look right\" or \"what could break\", or\n is about to commit or open a pull request. Also use proactively after\n writing or modifying code in this repo, before reporting work as done.\n---\n\n# Code Review - \n\nReview against what has actually broken here and how this codebase is\nactually written. Every finding cites a commit or a file path; findings\nthat cite nothing are nits and belong in the linter.\n\n## Scope\n\n1. User named a scope → use it.\n2. Staged changes exist (`git diff --cached --stat`) → review those.\n3. On a feature branch → `git diff $(git merge-base HEAD main)...HEAD`\n4. Otherwise → `git show HEAD`\n\n## Before reviewing\n\nCheck whether the diff touches a hotspot or blast-radius file below. If so,\nsay which, and raise scrutiny. Then per modified file:\n\n```bash\ngit log --oneline -12 HEAD -- \n```\n\nA hunk sitting on lines a recent fix touched is worth flagging on its own -\ncode just fixed and being changed again is code not well understood.\n\n## Already covered by CI - do not comment on\n\n\n\n\nRaise these only when the diff would *disable* or *bypass* a check.\n\n\n## Failure patterns\n\n\n### P1 - \n\n**Check when:** \n**Failure:** \n**Tell:** \n**Instead:** \n**Seen in:** `` (, ), `` (...)\n\n\n## House conventions\n\nDeviations are findings - cite the convention, not personal preference.\n\n\n**Error handling:** - see ``\n**Logging:** - see ``\n**Data access:** - see ``\n**Validation:** - see ``\n**Blessed helpers:** `` ( importers) for \n\n**Direction of travel** - new code belongs on the new side:\n\n| Deprecated | Current | Since |\n|---|---|---|\n| `` | `` | `` |\n\n\n## Testing expectations\n\n\n**Framework:** - tests in ``, named ``\n**Idioms:** - see ``\n**Regression discipline:** % of bug fixes ship with test changes.\n\n**Untested hotspots:** `` - changes here are effectively unverified;\nflag any non-trivial modification.\n\n\n## Coupling rules\n\nTouching one side without the other → ask why.\n\n\n| If the diff touches | It probably also needs | Co-changed |\n|---|---|---:|\n\n\n## Hotspot files\n\n\n| File | Bug fixes | Commits | Fix ratio | Primary author |\n|---|---:|---:|---:|---|\n\n\n## Blast radius\n\nRarely changed, expensive when wrong. Scrutinize regardless of diff size.\n\n\n- `` - \n\n\n## Reviewer conventions\n\n\nRecurring substantive concerns from past reviews:\n- - raised on #, #\n\nRecurring procedural nits - should be automated, do not raise manually:\n- - candidate for lint rule / PR template\n\n\n## Output format\n\nGroup by confidence, highest first. Each finding: location, pattern or\nconvention matched, the evidence, the concrete fix.\n\n```\n### High confidence\n\n**`src/billing/invoice.ts:142` - missing tenant scope on new query**\nMatches P1. Same omission caused the leak fixed in `9f2a1bc` and again in\n`44de0a1`. Use `scopedQuery()` (see `src/lib/db/scoped.ts`).\n```\n\nClose with which hotspot or blast-radius files the diff touched.\n\n## Rules\n\n- **Cite or drop it.** No historical evidence, no convention, no concrete\n failure mode → it is a nit. A short review that is all signal beats a\n long one people learn to skim.\n- **Do not duplicate CI.** See the covered list above.\n- **Do not restyle.** Formatting and naming are the formatter's job.\n- **Say when it's clean.** If the diff avoids every known trap, say so and\n stop. Manufacturing findings trains people to ignore this skill.\n- **Absence of a pattern is not absence of a bug.** The catalog covers what\n has broken before, not everything that can break. Still raise things that\n look wrong - mark them as judgment rather than documented pattern, and\n flag them as candidates for the catalog.\n\n## Retired patterns\n\n\nKept so nobody re-adds a rule a design change made moot.\n- - retired , \n\n\n---\n\nGenerated by `code-review-calibration` on .\nWindow: . Commits: . Fix commits: (%).\nFix-detection pattern: ``\nExemplar changes read: . PR reviews sampled: .\nRegenerate quarterly, or after any significant incident.\n\n````\n\n## Update mode\n\nIf the file already existed:\n\n- **Regenerate only marked sections.** Everything outside\n `` is human-authored. Leave it.\n- **Never silently drop a pattern.** A pattern absent from new evidence may\n have been fixed - or your fix-regex may have changed. Move it to Retired\n with a reason; do not delete.\n- **Preserve hand-written entries.** A pattern with no SHAs may come from\n an incident predating your window. Keep it, mark `(manually added)`.\n- **Report the delta.** What changed between runs: new and retired\n patterns, hotspots that entered or left, whether regression-test\n discipline moved, conventions that shifted. That delta is often more\n interesting than the file.\n\n## Finally\n\nReport briefly: commits analyzed, the fix-detection pattern you settled on\nand why, patterns kept versus discarded, conventions found, and your honest\nconfidence in each half.\n\nThe two halves fail independently. A young repo may have too little history\nfor good failure patterns while still having perfectly legible conventions\n- in that case say the conventions half is solid and the history half thin,\nand write the file with a strong conventions section and few patterns.\n**Name the weak parts rather than padding with speculation.** Four\nwell-evidenced patterns beat twelve invented ones.", + "category": "code-quality" + }, { "name": "code-simplifier", "description": "Simplifies and refines code for clarity, consistency, and maintainability while preserving all functionality. Analyzes recently modified code across three dimensions - code reuse, code quality, and efficiency - and provides actionable improvement suggestions. This skill should be used when the user asks to \"simplify code\", \"refine code\", \"clean up code\", \"improve code quality\", or requests a simplification review of recent changes.", From 960f02f90fecf624ee224dfafdd097e07997af5a Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 25 Aug 2026 16:00:27 +0000 Subject: [PATCH 2/2] Add README and plugin manifest for code-review-calibration CI requires every skill directory to ship a README.md and a .plugin/plugin.json with vendor symlinks so Codex and Claude Code can load it. Co-authored-by: openhands --- skills/code-review-calibration/.claude-plugin | 1 + skills/code-review-calibration/.codex-plugin | 1 + .../.plugin/plugin.json | 20 +++++++++ skills/code-review-calibration/README.md | 42 +++++++++++++++++++ 4 files changed, 64 insertions(+) create mode 120000 skills/code-review-calibration/.claude-plugin create mode 120000 skills/code-review-calibration/.codex-plugin create mode 100644 skills/code-review-calibration/.plugin/plugin.json create mode 100644 skills/code-review-calibration/README.md diff --git a/skills/code-review-calibration/.claude-plugin b/skills/code-review-calibration/.claude-plugin new file mode 120000 index 00000000..665797f0 --- /dev/null +++ b/skills/code-review-calibration/.claude-plugin @@ -0,0 +1 @@ +.plugin \ No newline at end of file diff --git a/skills/code-review-calibration/.codex-plugin b/skills/code-review-calibration/.codex-plugin new file mode 120000 index 00000000..665797f0 --- /dev/null +++ b/skills/code-review-calibration/.codex-plugin @@ -0,0 +1 @@ +.plugin \ No newline at end of file diff --git a/skills/code-review-calibration/.plugin/plugin.json b/skills/code-review-calibration/.plugin/plugin.json new file mode 100644 index 00000000..2c174af9 --- /dev/null +++ b/skills/code-review-calibration/.plugin/plugin.json @@ -0,0 +1,20 @@ +{ + "name": "code-review-calibration", + "version": "1.0.0", + "description": "Investigate a repository's commit history, bug fixes, reverts, house conventions, testing strategy, CI coverage, and past PR reviews, then write a calibrated code-review skill whose failure patterns, conventions, and hotspots are all cited from that repository.", + "author": { + "name": "OpenHands", + "email": "contact@all-hands.dev" + }, + "homepage": "https://github.com/OpenHands/extensions", + "repository": "https://github.com/OpenHands/extensions", + "license": "MIT", + "keywords": [ + "code-review", + "calibration", + "git-history", + "failure-patterns", + "conventions", + "hotspots" + ] +} diff --git a/skills/code-review-calibration/README.md b/skills/code-review-calibration/README.md new file mode 100644 index 00000000..cb520a15 --- /dev/null +++ b/skills/code-review-calibration/README.md @@ -0,0 +1,42 @@ +# Code Review Calibration + +Generate a code review skill that is calibrated to one specific repository, by investigating that repository's own history and conventions. + +## What It Does + +Most review guidance is generic, so the author has already heard it. This skill investigates a repo in two halves and writes what it learns to `.agents/skills/code-review.md`: + +- **How the codebase breaks** - fix-density hotspots, reverts, fix-inducing commits (SZZ), and temporal coupling between files. +- **How the codebase works** - house conventions and idioms, testing strategy and regression-test discipline, what CI already enforces, and ownership/blast-radius signals. + +The result is a reviewer that can say "use `scopedQuery()`, the same omission caused `9f2a1bc`" instead of "consider filtering by tenant." + +## When to Use + +- "Write a code review skill for this repo" +- "Recalibrate the reviewer to our codebase" +- "Where do bugs cluster here?" +- "What are our testing conventions?" +- The current review skill is producing generic feedback +- Onboarding to an unfamiliar codebase and needing to know its idioms and dangerous areas + +## Requirements + +- A git repository with real history. Very young or fully squash-merged repos yield weak patterns, and the skill is instructed to say so rather than pad the output. +- Optional: GitHub CLI (`gh`), authenticated. Enables the PR review-comment phase, which shows what the team already knows is easy to get wrong. + +## Output + +Writes or updates `.agents/skills/code-review.md`. Generated sections are wrapped in `` / `` markers so later runs can refresh the evidence without discarding hand-written content. + +Ground rules the generated file must satisfy: + +- Every failure pattern cites two or more commit SHAs. Uncitable observations are nits and get dropped. +- Six to twelve patterns. Beyond roughly fifteen the review loses focus. +- Patterns that a design change made impossible move to a Retired section rather than being deleted. + +## Related Skills + +- `code-review` - perform a code review +- `learn-from-code-review` - distill PR review feedback into guidelines +- `agent-memory` - persist repository knowledge in AGENTS.md