mattac21/skills

Revue de qualité

Cible : $ARGUMENTS

Examiner la cible pour la qualité de conception et de test. Signaler les résultats ; ne pas modifier le code.

1. Charger le référentiel

Lire les deux avant d'examiner le code :

• ~/.claude/CODING_STANDARDS.md est le référentiel. Chaque résultat cite l'un de ses titres de section.
• design-vocabulary.md contient les termes pour formuler les résultats, et les tests utilisés pour les vérifier ci-dessous.

Lire également le fichier AGENTS.md / CLAUDE.md du dépôt s'il est présent. Si ces fichiers indiquent que le dépôt fait délibérément quelque chose que les normes déconseillent, le mentionner une fois dans le rapport plutôt que de signaler chaque instance. Une habitude que vous observez uniquement dans le code (chaque module lit l'environnement, chaque test utilise monkeypatch) n'est pas un choix documenté : un nouveau code qui la suit est toujours un résultat.

2. Résoudre la cible

• Pas d'argument : la branche actuelle par rapport à sa base. git merge-base HEAD origin/main (ou master), puis git diff <base>...HEAD plus les modifications non commitées de git diff HEAD.
• Numéro ou URL de PR : gh pr diff <n> et gh pr view <n> --json baseRefName,headRefName,files.
• Branche : git diff <base>...<branch>.
• Chemin, ou . pour l'ensemble du dépôt : examiner le code existant sous ce chemin tel quel, sans diff. C'est le mode base de code, ci-dessous.

3. Lire suffisamment pour juger la conception

Mode modification (diff, branche, PR)

Un diff seul ne peut pas montrer si un module est profond. Pour chaque module modifié, lire l'intégralité du fichier, et chercher ses appelants et ses tests. La profondeur, la propriété et la duplication sont des jugements sur les sites d'appel, il faut donc les examiner.

Limiter les résultats à ce que la modification introduit ou aggrave. Un problème préexistant dans le code que le diff touche est dans le périmètre si la modification était le moment naturel pour le corriger ; un problème dans du code non touché ne l'est pas.

Les nouveaux modules bénéficient d'une revue à l'échelle du module, comme dans le mode base de code : parcourir les sections Types et Lisibilité aussi complètement que les autres sections. Pour chaque module nouveau ou substantiellement réécrit, lister chaque fonction avec ses types de paramètres et de retour avant de la juger, et noter chaque map, dict ou tuple non typé qu'elle accepte, retourne ou lit des clés, et chaque nom à une seule lettre.

Mesurer la complexité cyclomatique pour chaque fichier source modifié avant de juger la Lisibilité, avec l'outil habituel du langage : par exemple radon cc -s pour Python, gocyclo pour Go, la règle complexity d'ESLint pour JavaScript/TypeScript, cognitive_complexity de Clippy pour Rust, PMD pour Java. L'exécuter via les outils du dépôt lui-même s'il en a, ou un exécuteur unique (uvx, npx, go run) sinon. Préférer un outil qui compte les opérandes booléens en court-circuit ; certains (C901 de Ruff, par exemple) les ignorent et sous-estiment les pires fonctions. Si aucun outil n'est disponible, compter les points de décision manuellement comme le référentiel les définit. Une fonction que le diff ajoute avec un score supérieur à 10, ou une fonction existante que le diff pousse au-delà de 10 ou augmente davantage, est candidate. Pour une fonction existante, noter la version de base aussi (écrire git show <base>:<file> dans un fichier temporaire et le mesurer) afin que le résultat puisse dire ce que la modification a ajouté.

Lorsque le diff ajoute plus d'environ 150 lignes de source, lancer deux sous-agents general-purpose en parallèle pendant que vous examinez les sections de conception, chacun recevant le diff, la liste des fichiers modifiés et les règles de vérification de l'étape 4 pour ses sections : un pour les Types, la Lisibilité et « Pencher vers les données immuables là où c'est rentable », l'autre pour les Tests, en appliquant les vérifications d'assertion à chaque nouveau test. Vérifier ce que chacun retourne avant de le conserver.

Mode base de code (chemin ou .)

Tout ce qui se trouve sous le chemin est dans le périmètre, mais le seuil pour un résultat s'élève avec la taille du périmètre. Un seul module ou un petit répertoire (environ moins de 1 500 lignes de source) reçoit des résultats au niveau du module, de la fonction et du nommage : parcourir les sections Types et Lisibilité aussi complètement que les autres sections. Un paquet ou l'ensemble du dépôt ne reçoit que les problèmes majeurs : les décisions de conception et les motifs architecturaux qui freinent la base de code. Il en va de même lorsque l'utilisateur nomme une fonctionnalité ou un outil (par exemple « toute la CLI ») qui se révèle être un seul module : le juger à l'échelle du module. Se demander pour chaque candidat à l'échelle du paquet ou du dépôt :

• Combien de modules, de tests ou de modifications futures paient pour cela ? Un coût confiné à une seule fonction est un détail à cette échelle.
• Le corriger changerait-il la manière dont le nouveau code est écrit dans ce dépôt ? Si le correctif est quelques lignes locales sans effet sur les appelants, l'abandonner.
• Est-ce un motif ou un cas isolé ? Les instances individuelles ne comptent que comme preuve d'un motif.

Exemples de problèmes majeurs : un système externe sans module propriétaire, de sorte que chaque appelant refait l'auth et la pagination ; la politique et l'orchestration fusionnées dans les points d'entrée, de sorte que les décisions ne peuvent être testées que de bout en bout ; la propriété de l'état répartie entre des modules qui connaissent chacun la disposition de stockage ; les clients et la configuration construits au moment de l'importation, de sorte que rien ne peut être testé sans l'environnement réel ; une suite de tests qui patche les internes partout car il n'existe pas de jointures. Une longue fonction, une variable mal nommée, une seule exception avalée ou une assertion faible ne sont pas des problèmes majeurs en eux-mêmes.

Ces problèmes s'étendent sur plusieurs modules, il faut donc construire une carte avant de lire les fichiers en profondeur :

• les modules (fichiers ou paquets) et approximativement ce que chacun fait, à partir de git ls-files <path> et d'une lecture rapide du haut de chaque module
• comment les principales pièces se connectent : ce qui appelle quoi, et comment l'état et les données se déplacent entre les points d'entrée, les modules et les systèmes externes
• chaque système externe (API, base de données, file d'attente, format de fichier, modèle) et chaque module qui lui parle directement
• chaque endroit où l'environnement ou la configuration est lu (os.environ, getenv, process.env, analyse des drapeaux)
• l'état mutable au niveau du module et les effets au moment de l'importation (clients, connexions, caches, verrous construits à la portée du module)
• les points d'entrée, et le répertoire de tests avec les modules que chaque fichier de test exerce

grep suffit pour la plupart de cela. Garder la carte courte ; c'est un index de travail, pas une partie du rapport.

Si le périmètre est petit (environ moins de 3 000 lignes de source), l'examiner directement à partir de la carte, et lancer en parallèle deux sous-agents general-purpose, chacun recevant la liste des fichiers et les règles de vérification de l'étape 4 pour ses sections. L'un reçoit uniquement les sections Types, Lisibilité et « Pencher vers les données immuables là où c'est rentable » du référentiel. L'autre reçoit uniquement la section Tests et doit appliquer les vérifications d'assertion à chaque test. Ainsi, ces sections reçoivent toute l'attention plutôt que ce qui reste après les précédentes. Vérifier ce que chacun retourne avant de le conserver. Sinon, le diviser en zones de modules connexes, chacun avec ses tests, et donner chaque zone à un sous-agent general-purpose, lancé en parallèle. Donner à chacun le chemin du référentiel, le chemin du vocabulaire, la carte, sa liste de fichiers, les règles de vérification de l'étape 4 et le seuil des problèmes majeurs ci-dessus, et lui dire de ne rien modifier. Un sous-agent qui ne voit que sa propre zone dérive vers des détails locaux, donc lui demander les motifs de conception qu'il voit dans sa zone et les preuves pour chacun (fichier:ligne), pas une liste de résultats au niveau de la ligne. Les problèmes inter-modules proviennent de la carte, donc les résoudre vous-même, puis les combiner avec les motifs des sous-agents, fusionner ceux qui sont la même décision sous-jacente vue de différentes zones, et revérifier les preuves pour chacun avant de les signaler.

4. Examiner chaque section

Parcourir les normes dans l'ordre : Modules, État, Types, Dépendances et configuration, Erreurs, Lisibilité, Tests. Pour chaque résultat candidat, le vérifier avant de le conserver :

• Module superficiel ou de passage : appliquer le test de suppression. Nommer les appelants et dire quelle complexité réapparaîtrait chez eux, ou montrer que rien ne disparaîtrait.
• Abstraction qui n'a pas gagné sa place : compter les adaptateurs à la jointure. Un faux de test compte. Un adaptateur sans rien caché derrière est un résultat ; deux est une véritable jointure.
• Abstraction manquante : vérifier la direction opposée tout aussi rigoureusement. Chercher une préoccupation traitée en ligne à trois endroits ou plus, à l'intérieur d'un module comme entre des modules.
• Propriétaire manquant / préoccupation dupliquée : lister chaque endroit où la préoccupation est écrite (fichier:ligne), pas « cela apparaît à plusieurs endroits ».
• Concept sans propriétaire : lister chaque endroit où il est représenté, analysé ou construit (fichier:ligne), et toute signature où des paramètres de même type avec des significations différentes côtoient les uns les autres. Les identifiants comptent comme des concepts, pas seulement les données structurées. Les noms et ID de différents types passés comme la même primitive (un nom de dépôt source et cible, un nom de branche et une référence, deux types d'ID) sont le principal cas à attraper. Ensuite, dire quel type de propriétaire convient. Si les appelants détiennent et passent le concept, recommander un type pour celui-ci. S'ils ont seulement besoin que quelque chose soit fait avec lui, recommander de le cacher à l'intérieur du module qui fait le travail. Ne pas par défaut à un type de domaine.
• Contraintes d'ordre ou clés/chemins/ID construits à la main : montrer l'appelant qui effectue le séquencement ou la construction.
• État mutable partagé, effets au moment du chargement, configuration lue en profondeur : nommer la variable globale ou l'appel au moment de l'importation, et le test ou la seconde configuration qu'il bloque.
• Écritures instantanées : montrer la lecture, l'étape de longue durée, et la réécriture.
• Données mutables qui seraient mieux immuables : ne les signaler que lorsque la mutabilité a un coût concret. Montrer le second détenteur qui voit un changement inattendu, le cache ou la clé qu'elle rend dangereux, ou le changement effectué loin de l'endroit où la valeur a été créée. Une structure mutable avec un seul propriétaire qui la modifie via son propre code n'est pas un résultat. Si la version immuable coûterait en performance, dire quoi mémoriser ou partager, et où ce cache se trouve.
• État invalide qui peut être représenté : nommer l'état qui ne doit jamais se produire et montrer le type qui le permet (champs optionnels qui doivent être définis ensemble, combinaisons de booléens, une chaîne avec quelques valeurs autorisées). Lister les gardes ou les vérifications None qui n'existent qu'à cause de cela (fichier:ligne). Proposer la forme qui le rend impossible, et dire quelles gardes cette modification supprime.
• Dict ou tuple comme concept : lister les clés lues et chaque fonction qui les lit (fichier:ligne), et chaque endroit où le dict est construit. Proposer le type d'enregistrement dans le style existant du dépôt (un modèle pydantic ou dataclass, une interface TypeScript, une structure Go, peu importe ce que le dépôt utilise déjà), contenant uniquement les champs que le code utilise, et dire où la charge utile est analysée dans celui-ci.
• Module qui possède un système externe : vérifier si ses fonctions partagent un identifiant, une URL de base ou un cache via des globales de module ou des lectures d'environnement par appel, et si les tests patchent plusieurs de ses fonctions. Si c'est le cas, recommander une classe client avec des méthodes nommées par domaine, construite au démarrage et passée en paramètre, et nommer les correctifs que le faux remplacerait.
• Commentaires de documentation manquants : lister chaque fonction non triviale sans documentation (fichier:ligne) et ce que son commentaire de documentation devrait dire que la signature ne dit pas (une solution de repli, un mode d'erreur, un état qu'elle touche, une forme d'entrée). Les regrouper en un résultat de Lisibilité par module.
• Noms à une seule lettre : les signaler comme un résultat de Lisibilité groupé listant chacun (fichier:ligne) avec un nom proposé. Ils comptent vers le plafond de Lisibilité comme un résultat, jamais comme une raison de les abandonner.
• Erreurs avalées : montrer le chemin de l'échec à un résultat qui change alors que seule une ligne de log l'enregistre.
• Nom avec plusieurs significations : le chercher avec grep et lister ce qu'il contient à chaque site.
• Nom qui cache son niveau ou son objet : montrer la chaîne d'appels ou les sites d'appel où il se lit de manière ambiguë, et proposer le remplacement.
• Fonction qui fait plus d'une chose : essayer d'écrire son résumé en une phrase. Si vous n'y arrivez pas, citer la branche, le bloc try ou le paramètre de mode qui vous en empêche.
• Fonction trop complexe : pour chaque candidat issu de la mesure de complexité, donner son score, puis lister les phases qu'il exécute dans l'ordre (filtrer l'entrée, récupérer, réclamer, valider, construire, créer, enregistrer, notifier, ...) avec leurs plages de lignes et la règle d'échec de chaque phase. Proposer la division en fonctions nommées, une par phase ou décision, avec des signatures, et dire ce que chaque élément extrait rend testable sans correctif ; généralement, les décisions deviennent des fonctions pures et l'original se lit comme une courte séquence d'étapes nommées. Un résultat par fonction. Au-dessus de 20, il se classe avec les résultats de conception ; de 11 à 20, c'est un résultat de Lisibilité qui ne compte pas vers le plafond de Lisibilité. Ne pas abandonner une division parce que la fonction est un point d'entrée ou parce que le dépôt en a de plus grandes.
• Assertions de test : nommer le bug du produit qui ferait échouer l'assertion. Si vous ne pouvez pas, c'est le résultat. Pour les cas paramétrés ou pilotés par table, tracer chaque entrée à travers le code et dire quelle partie de celle-ci atteint le comportement pour lequel le cas est nommé. Un cas dont l'entrée distinctive est consommée par une branche antérieure ne couvre pas ce comportement, même s'il existe un test pour cela. De même, vérifier que le fixture ou le faux peut effectivement produire l'entrée qui déclencherait chaque garde ou chemin de refus. Si ce n'est jamais le cas, la garde n'est pas testée.
• Doubles de test : dire de quoi parle le test, puis si le double se trouve en dehors de cette frontière ou réimplémente la logique du produit.
• Tests qui patchent les internes : pointer vers la décision qui devrait être extraite ou la dépendance qui devrait être passée en paramètre, car c'est le véritable correctif.
• Jointure ou port proposé, dans le correctif de tout résultat : lui donner des opérations nommées dans le domaine (close_ticket(id)), pas des verbes de transport (get/post/put). Un port qui ne fait que transmettre des requêtes échoue au test de suppression, et son faux devrait réimplémenter le routage. Vérifier chaque jointure proposée par rapport au test de suppression avant de la signaler.

Donner aux Tests la même profondeur que les sections précédentes, même lorsque les sections précédentes produisent de nombreux résultats. Exécuter chaque vérification de Test ci-dessus sur chaque fichier de test : pour chaque assertion, nommer le bug du produit qui le ferait échouer. Un comportement lié à la sécurité sans assertion échouant (rédaction, vérification d'auth, porte d'approbation) se classe avec les résultats de conception.

Abandonner tout ce que vous ne pouvez pas rendre concret. Abandonner les détails de style que les normes ne couvrent pas. Ne pas signaler les bugs d'exactitude ici à moins qu'ils ne découlent directement d'un problème de qualité (par exemple, une écriture instantanée qui perd des données) ; les mentionner en une ligne et pointer vers /code-review.