Review Commenter

Guardrails

• max_comments: 15
• timeout: 5min
• no_approve_without_tests

Context

Consumes: git_diff, test_results, lint_results Produces: review_comments Memory: conversation

Output Format

Output: Review_comments

Provide your review as a structured list. For each finding:

1. Severity: critical | warning | suggestion
2. Location:

   file:line

3. Issue: One-sentence description
4. Fix: Concrete code suggestion or action
5. Rationale: Why this matters

Group findings by severity (critical first). End with a summary count.

---

Review Comments — feat/scanner-enricher-bench → main

Branch :

feat/scanner-enricher-bench

Tests : 706/706 pass Lint : 103 mineurs, 0 critiques

---

Critiques

Aucun problème bloquant identifié.

---

Avertissements

1

Severity: warning Location:

internal/generator/claude/agent.go:66

Issue: Quand un skill figure dans

agent.Skills

mais pas dans

resolvedSkills

, le générateur émet l'en-tête

### Step N: X

suivi d'une ligne vide sans bloc

Launch a subagent

, produisant une étape vide dans le markdown final. Fix: Déplacer l'ajout de l'en-tête à l'intérieur de la branche

if s.Skill == skillName

afin qu'il ne soit émis que si le skill est effectivement résolu, ou ajouter un commentaire

(skill not resolved)

explicite.

// Avant (ligne 66-88)
lines = append(lines, fmt.Sprintf("### Step %d: %s", i+1, generator.ToTitle(skillName)))
effort := model.EffortMedium
for _, s := range resolvedSkills {
    if s.Skill == skillName {
        lines = append(lines, "Launch a subagent:")
        ...
        break
    }
}
lines = append(lines, "")

// Après — déplacer l'en-tête à l'intérieur du if
for _, s := range resolvedSkills {
    if s.Skill == skillName {
        lines = append(lines, fmt.Sprintf("### Step %d: %s", i+1, generator.ToTitle(skillName)))
        lines = append(lines, "Launch a subagent:")
        ...
        break
    }
}

Rationale: Le même problème existe dans

internal/generator/copilot/agent.go:66

. Un skill non résolu produit du markdown silencieusement cassé sans que le build échoue ni n'avertisse au niveau du générateur.

---

2

Severity: warning Location:

internal/generator/copilot/effort.go:6

Issue:

EffortToModel

du package

copilot

retourne

"haiku"

,

"sonnet"

,

"opus"

— des noms de modèles Anthropic — alors que GitHub Copilot utilise des modèles GPT/OpenAI. Fix: Mapper vers les identifiants corrects pour Copilot (ex.

"gpt-4o-mini"

,

"gpt-4o"

,

"o1"

) ou documenter explicitement que ces noms sont intentionnellement génériques et laissés à l'utilisateur. Rationale: Un agent Copilot généré avec

Model: haiku

sera incompréhensible pour un utilisateur GitHub Copilot ; cela peut induire des erreurs de configuration silencieuses.

---

3

Severity: warning Location:

internal/generator/copilot/effort.go

(fichier entier) Issue:

EffortToModel

est dupliqué à l'identique dans

internal/generator/claude/effort.go

et

internal/generator/copilot/effort.go

sans factorisation. Fix: Extraire la logique commune dans

internal/generator/effort.go

(package

generator

) et appeler

generator.EffortToModel

depuis chaque package, ou au moins ajouter un commentaire signalant la divergence intentionnelle (noms de modèles différents par cible). Rationale: Toute évolution future (ajout d'un niveau

EffortUltra

) devra être dupliquée manuellement, avec risque de désynchronisation.

---

4

Severity: warning Location:

internal/generator/claude/agent.go:44

Issue: La section

## Execution

et le texte d'orchestration sont toujours émis, même quand

resolvedSkills

est vide (ex. aucun skill chargé), ce qui produit un agent avec

Execute 2 skills sequentially...

mais aucun step. Fix: Conditionner l'émission du bloc

## Execution

à

len(resolvedSkills) > 0

, ou au moins à

len(agent.Skills) > 0

. Rationale: Le test

TestGenerateAgentMd_NoSkills

passe

nil

comme

resolvedSkills

mais

agent.Skills

reste

["code-review","security-scan"]

— l'output contient le texte d'orchestration sans aucune étape concrète.

---

5

Severity: warning Location:

pkg/model/skill.go:143–148

(suppression de

DependencyFacet

/

DependsOn

) Issue: Le champ

DependsOn DependencyFacet

a été supprimé du modèle

SkillBehavior

sans versioning ni note de migration : les fichiers

.skill.yaml

existants contenant

depends_on:

seront silencieusement ignorés par le loader YAML. Fix: Ajouter un avertissement dans le loader YAML ou le linter quand un champ

depends_on

est détecté dans le fichier source (strict mode YAML ou vérification explicite). Rationale: Un breaking change silencieux sur le format YAML de définition des skills peut casser des projets utilisateurs sans message d'erreur.

---

Suggestions

6

Severity: suggestion Location:

internal/generator/copilot/agent.go:35

Issue: La valeur

tools: ["task"]

est un littéral de chaîne Go brut non construit programmatiquement, contrairement à

GenerateCompactCopilotAgentMd

(ligne 138) qui génère le format

tools: ["read", ...]

via

fmt.Sprintf

. Fix: Utiliser une constante ou une construction cohérente :

const copilotTaskTool = `tools: ["task"]`
lines = append(lines, copilotTaskTool)

Rationale: Incohérence de style entre les deux fonctions du même fichier ; risque d'erreur typographique lors d'une future modification.

---

7

Severity: suggestion Location:

internal/generator/claude/agent.go:41

Issue: Le corps de l'agent en mode dispatch ne contient plus la

description

de l'agent (remplacée par le template fixe

An orchestrator that coordinates N specialized subagents

), alors que

GenerateCompactAgentMd

(ligne 150) conserve la description dans le corps. Fix: Soit inclure la description dans les deux modes de façon cohérente :

lines = append(lines, fmt.Sprintf("You are %s. %s Orchestrates %d specialized subagents.",
    generator.ToTitle(agent.Agent), agent.Description, len(agent.Skills)))

soit documenter que le mode dispatch n'expose pas la description dans le corps. Rationale: Incohérence comportementale entre

--compact

et mode standard ; peut surprendre les utilisateurs qui personnalisent la description.

---

8

Severity: suggestion Location:

internal/generator/copilot/effort.go

(absence de fichier

effort_test.go

) Issue:

EffortToModel

dans le package

copilot

n'a aucun test unitaire dédié, alors que le package

claude

possède

effort_test.go

. Fix: Créer

internal/generator/copilot/effort_test.go

avec les quatre cas (

light

,

medium

,

heavy

,

""

). Rationale: Couverture asymétrique entre les deux packages pour une fonction identique.

---

9

Severity: suggestion Location:

internal/generator/claude/agent.go:114

Issue:

formatIOPointers

est une fonction privée triviale (2 lignes) qui duplique partiellement la logique de formatage d'I/O déjà présente dans

internal/generator/util.go

. Fix: Vérifier si

generator.FormatIO

ou une fonction similaire existe dans

util.go

et consolider, ou au moins ajouter un commentaire de justification. Rationale: Fragmentation des helpers de formatage entre

util.go

(package

generator

) et les sous-packages.

---

10

Severity: suggestion Location:

internal/generator/claude/agent_test.go:112

Issue:

TestGenerateAgentMd_NoSkills

passe

resolvedSkills=nil

mais l'agent a toujours

Skills: ["code-review","security-scan"]

— la condition testée est donc

resolvedSkills vide

et non

agent.Skills vide

; le cas

agent.Skills=[]

+

resolvedSkills=[]

n'est pas couvert. Fix: Ajouter un test explicite :

func TestGenerateAgentMd_EmptySkillsList(t *testing.T) {
    agent := testAgent()
    agent.Skills = nil
    md := claude.GenerateAgentMd(agent, nil, ".claude")
    assert.NotContains(t, md, "## Execution")
}

Rationale: La validation

ValidateAgent

rejette

len(Skills)==0

, mais ce chemin de génération reste non couvert.

---

11

Severity: suggestion Location:

internal/generator/claude/agent_test.go

(absence de test d'étape orpheline) Issue: Aucun test ne vérifie le comportement quand

agent.Skills

référence un skill absent de

resolvedSkills

(ex. skill déclaré mais non chargé). Fix: Ajouter :

func TestGenerateAgentMd_UnresolvedSkill_NoOrphanBlock(t *testing.T) {
    agent := testAgent() // Skills: ["code-review", "security-scan"]
    // Ne passer qu'un seul des deux skills
    md := claude.GenerateAgentMd(agent, testResolvedSkills()[:1], ".claude")
    // Vérifier que l'étape sans skill résolu ne produit pas de header vide
    assert.NotContains(t, md, "### Step 2: Security Scan\n\n")
}

Rationale: Le comportement actuel (header + ligne vide sans contenu) est silencieux et non spécifié.

---

12

Severity: suggestion Location:

pkg/model/skill.go:169-178

Issue:

WhenToUseFacet.IsEmpty()

ne vérifie que

len == 0

pour trois champs, ce qui est correct, mais la méthode n'est pas testée isolément — elle est uniquement exercée indirectement via les règles du linter et le score. Fix: Ajouter un test unitaire minimal dans

pkg/model/skill_test.go

pour

IsEmpty()

avec un cas vide et un cas non-vide. Rationale: Méthode publique sur un type de modèle central, sans test direct.

---

Résumé

Sévérité	Nombre
Critique	0
Avertissement	5
Suggestion	7
Total	12

Les tests passent tous (706/706). Les points les plus risqués sont l'étape orpheline en cas de skill non résolu (#1, #11) et le breaking change silencieux sur

depends_on

(#5). Le mapping de modèles Copilot (#2) est fonctionnellement incorrect pour les utilisateurs GitHub Copilot.

Strategy

Approach: diff-first Tools: read_file, grep, search

Steps

1. read the git diff
2. cross-reference with test results
3. cross-reference with lint results
4. identify risky changes
5. write actionable review comments

Security

• Filesystem: read-only
• Network: none