| name | code-review |
| description | Première passe de revue sur le diff courant — signale bugs de correction, vecteurs sécurité, violations d'invariants et de mes conventions. Surcharge la skill bundled. Signale uniquement, n'applique jamais de correctif. Filet en amont de la revue humaine, jamais un quitus. |
| disable-model-invocation | true |
| allowed-tools | Read, Glob, Grep, Bash |
| effort | high |
Code Review — première passe surchargée
Revue du diff courant, calibrée sur mes conventions, ma structure de modules et ma
posture sécurité. Surcharge la skill bundled /code-review du même nom (une
skill user-scope l'emporte sur la bundled).
Décision de référence : adr/0010.
Cette skill en dérive — elle n'invente aucune règle. En cas de doute sur le rôle,
la frontière ou le mécanisme ledger→/adr, cet ADR fait foi.
Rôle — première passe, jamais un quitus. Filet de sécurité en amont de la revue
humaine réelle, qu'elle ne remplace jamais. Un /code-review vert ne dispense pas
de la revue humaine.
Invariant non négociable — signale, n'applique jamais. Cette skill SIGNALE ;
l'humain tranche chaque finding. Aucun auto-fix, même demandé via --fix. Le
nettoyage applicable relève de /simplify (voir frontière ci-dessous). L'absence
d'Edit/Write dans allowed-tools rend cette règle structurelle, pas seulement
déclarative.
Argument : $ARGUMENTS (optionnel) = niveau d'effort (low|medium|high|xhigh|max)
et/ou cible (target : chemin, ref de commit, PR). Sans niveau explicite, défaut
high (mes diffs sont petits mais denses en logique et sécurité — je préfère
une couverture large à des findings rares). Sans cible, revue du diff non
commité (working tree). Ignorer --fix s'il est passé : afficher un rappel que
cette skill ne fixe pas, et router vers /simplify.
Frontière — ce que la revue NE fait PAS (déjà couvert ailleurs)
Trois gestes, rôles disjoints, zéro overlap. Ne jamais redonder les deux autres.
| Geste | Couvre | Applique des fix ? |
|---|
hooks (ruff-check, garde-fous) | lint / style / format / imports inutilisés ; sécurité runtime (force-push, rm -rf, accès .env) | oui (ruff), automatique, bloquant |
/code-review (cette skill) | bugs de correction · sécurité · invariants métier · mes conventions | non — signale, l'humain tranche |
/simplify | nettoyage : reuse de helpers, simplification, efficacité, altitude | oui — applique |
Ne JAMAIS re-signaler ce que ruff couvre : style, formatage, longueur de
ligne, imports inutilisés, ordre d'imports, règles discoverables dans
pyproject.toml / pre-commit. Le hook ruff-check (PostToolUse sur Write|Edit de
.py) les a déjà attrapés. Les re-signaler est du bruit pur.
Ne JAMAIS appliquer un nettoyage : si un finding est un vrai cleanup applicable
(helper réutilisable, code mort réel, simplification), le signaler et router vers
/simplify — ne pas le corriger soi-même.
Périmètre propre — ce que la revue cible
Quatre catégories. C'est le cœur de la valeur de cette surcharge.
-
Bugs de correction — ce que le lint ne voit pas : logique fausse, off-by-one,
conditions inversées, états non gérés, races, ressources non libérées, valeurs de
retour ignorées, exceptions avalées qui masquent un échec réel, contrat de
fonction violé (le code ne fait pas ce que sa signature / son docstring promet).
-
Vecteurs sécurité — injection (SQL, commande, path traversal), secrets en
clair (clés, tokens, mots de passe, valeurs par défaut sensibles dans le code),
bypass d'authentification/autorisation, désérialisation non sûre, données
utilisateur non validées atteignant une sink dangereuse, fail-open là où il
faut fail-closed. Pour les fichiers sensibles, ne jamais lire un .env (le
hook protect_env.py le bloque de toute façon).
-
Invariants métier — violations des règles propres au domaine du projet :
contraintes que le code doit préserver et qu'aucun linter ne connaît. Les déduire
du CLAUDE.md projet et du code environnant, pas les inventer.
-
Mes conventions (cf. ~/dotfiles/claude/rules/) — uniquement ce que ruff
ne couvre PAS :
- Python :
structlog, jamais print() pour logger ou débugger ;
pathlib plutôt que os.path ; type hints sur toute signature publique ;
from __future__ import annotations en tête de module ; docstrings Google.
- SQL / dbt : modélisation Kimball (séparation fact/dimension explicite) ;
ref() plutôt que noms de tables en dur ; un modèle par fichier ; tout modèle
a une description et des tests de colonne sur les PK.
- Terraform : variables avec description, jamais de valeur en dur dans une
ressource ; state distant (pas de tfstate local versionné) ;
plan avant
apply.
Reconnaissance de la complexité délibérée — éviter les faux positifs structurels
Une partie de ma complexité est intentionnelle. La signaler comme « complexité
inutile », « code mort » ou « sur-ingénierie » est un faux positif à éviter
activement. Avant de signaler une complexité apparente, vérifier qu'elle ne relève
pas d'un de ces patterns assumés :
- Fail-closed explicite : un allowlist où tout cas inconnu est rejeté par défaut
(ex :
protect_env.py — un suffixe .env.* inconnu est bloqué). Ce n'est pas une
branche morte : c'est la posture sécurité voulue.
- Validation redondante à source unique : deux chemins d'entrée (fichier ET
commande bash) qui réutilisent la même fonction de décision. La redondance est
voulue (couvrir les deux surfaces), la source de vérité reste unique — ce n'est
pas de la duplication à factoriser.
- Vérifications imposées par mes contraintes : revalidation à une vraie
frontière (input utilisateur, API externe), garde-fou explicite contre un état
dangereux. À distinguer de la défense contre un scénario impossible (ça, c'est
bien un finding — cf. « Simplicity First »).
Règle de départage : la complexité protège-t-elle contre quelque chose de
réel (une entrée hostile, un état atteignable, une contrainte sécurité) ? Oui →
intentionnelle, ne pas signaler. Non (défense contre l'impossible, abstraction
spéculative pour un seul appelant) → finding légitime.
En cas de doute réel, signaler en sévérité basse avec la mention « peut être
délibéré » plutôt que d'affirmer « complexité inutile ». Le faux négatif (rater un
vrai défaut maquillé en intentionnel) est un risque assumé pour une première passe ;
le faux positif structurel, lui, érode la confiance dans l'outil.
Procédure
-
Résoudre la cible. $ARGUMENTS contient un chemin / une ref / une PR →
revue de cette cible. Sinon → git diff du working tree (changements non
commités). Si le diff est vide, vérifier git diff --staged ; si vide aussi,
s'arrêter et le dire — ne rien inventer.
-
Lire le diff en entier avant de juger quoi que ce soit. Lire aussi le
CLAUDE.md projet (conventions et invariants métier propres au projet) et, au
besoin, le code environnant pour comprendre le contexte d'un changement.
-
Identifier le langage de chaque fichier touché pour appliquer les conventions
pertinentes (rules/python.md, rules/dbt-sql.md, rules/terraform.md).
-
Parcourir les quatre catégories (bugs, sécurité, invariants, conventions) sur
chaque hunk. À chaque complexité apparente, passer le test de complexité
délibérée avant de la retenir comme finding.
Résoudre tout doute vérifiable AVANT d'émettre — jamais de finding spéculatif.
Si un finding dépend d'une hypothèse vérifiable dans le repo (« et si cette valeur
était None ? », « cette fonction est-elle appelée ailleurs ? », « l'invariant est-il
garanti en amont ? »), lire le fichier concerné et trancher avant d'émettre. Un
diff ne montre qu'un hunk ; l'invariant qui annule un faux positif vit souvent dans
le code non touché (ex : un _parse en amont qui droppe déjà les coords None rend
sans objet un finding « crash si None »). Un finding « à confirmer / à vérifier »
n'est légitime QUE si la confirmation dépend d'un élément hors du repo (valeur
runtime, réponse d'API externe, intention humaine). Sinon : vérifier, puis émettre
ou jeter. Cohérent avec « run the verifying command first » (CLAUDE.md).
-
Construire le ledger (format ci-dessous). Pour chaque finding : sévérité,
fichier:ligne, catégorie, description du problème, et impact concret (pourquoi
ça compte). Ne pas proposer le patch — décrire le problème, l'humain tranche.
-
Repérer les findings dignes d'un ADR : un finding qui révèle un trade-off
non-trivial (un secret en clair qui pourrait être un choix de mode dev assumé ;
un fail-open qui mériterait d'être acté comme tel ; un invariant relâché
volontairement) est tagué (ADR) et rendu autoportant (voir ci-dessous).
Output — le ledger de findings
Terminer par un ledger unique, trié par sévérité décroissante. Sévérités :
CRITICAL (faille exploitable, perte de données, corruption) · HIGH (bug de
correction, vecteur sécurité sérieux) · MEDIUM (violation de convention, bug
mineur) · LOW (amélioration, complexité peut-être délibérée).
Format de chaque finding :
[N] (SÉVÉRITÉ · catégorie · fichier:ligne) Titre court
Problème : ce qui ne va pas, en une à deux phrases.
Impact : conséquence concrète si non corrigé.
Piste : direction de correction (SANS l'appliquer). Si c'est un nettoyage
applicable → "relève de /simplify".
Catégories : correction · sécurité · invariant · convention.
Si aucun finding : le dire clairement, et rappeler que ça ne vaut pas quitus —
la revue humaine reste due.
Délégation vers /adr — par instruction, jamais par invocation
Un finding tagué (ADR) (trade-off non-trivial à acter) suit le pattern /grill
(cf. adr/0003) : cette
skill n'invoque jamais /adr (une slash-command ne pilote pas une autre ; une
skill est un prompt injecté, pas un programme). Elle produit du matériau
autoportant et instruit l'humain.
Rendre chaque item (ADR) autoportant — une session /adr est vierge, sans mémoire
de cette revue :
[N] (ADR) Titre de la décision à acter
Contexte : la tension révélée par le finding (1-2 lignes).
Options : les alternatives en jeu.
Décision : ce qui semble à trancher — l'humain confirmera dans /adr.
S'il existe au moins un item (ADR), clore par une invite isolée et visible
(séparateur dédié, jamais une parenthèse en fin de phrase) :
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
⚠️ FINDINGS TAGUÉS (ADR) — à formaliser toi-même.
Cette skill n'écrit aucun fichier et ne lance pas /adr. Pour chaque
item (ADR) ci-dessus, lance /adr --from-context et colle l'item —
la délibération est déjà dans ce fil.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Invariants de sortie
- N'éditer aucun fichier. N'appliquer aucun correctif. Remettre le ledger à
l'humain comme unique artefact de la session.
- Ne pas redonder les hooks : aucun finding de style / format / lint / import.
- Ne pas re-signaler la complexité délibérée comme un défaut.
- Aucun finding spéculatif vérifiable dans le repo : un doute levable par lecture
doit être levé avant émission (cf. procédure §4). « À confirmer » réservé au hors-repo.
- Première passe ≠ revue humaine : terminer en le rappelant si des findings de
sévérité ≥ HIGH subsistent.
