L'affirmation « Each sandbox is isolated » est plus forte que la réalité tant que les runArgs/containerEnv du dépôt analysé sont transmis tels quels à docker run (cf. container.rs). Soit durcir le code, soit nuancer la section en listant les limites de l'isolation.
Le job container-build compile l'image mais ne l'exécute jamais : il ne détectera donc pas l'absence de git/runtime conteneur dans l'image finale. Ajouter un smoke test (lancer l'image et vérifier la présence de git et docker/podman), et noter que le pipeline ne se déclenche que sur push, pas sur les pull requests.
La suppression du .dockerignore réintègre target/ et .env dans le contexte envoyé au builder (buildah bud ... .), ce qui alourdit fortement le build et expose potentiellement des secrets. Conserver un .dockerignore minimal (target, .env, .git).
L'absence de runtime conteneur ne produit qu'un warn : le service démarre normalement alors que toutes les revues échoueront ensuite. Le contrôle étant déjà fait au démarrage, un échec rapide (fail-fast) ou un mode dégradé explicite serait plus sûr et plus lisible.
SANDBOX_MAX_ITERATIONS retombe silencieusement sur 8 si la valeur est invalide et accepte 0 : avec 0, la boucle de l'agent (1..=max_iterations) ne s'exécute pas et la revue échoue immédiatement. Valider la valeur (>= 1) et journaliser en cas de repli sur la valeur par défaut.
Indentation par tabulation incohérente avec le reste du fichier ; --security-opt label=disable désactive l'étiquetage SELinux et --userns=keep-id est spécifique à Podman. À nettoyer/justifier, d'autant que ce devcontainer sert aussi de contexte d'exécution à la sandbox.
tempfile = "3" est déclaré en dur alors que toutes les autres dépendances passent par [workspace.dependencies] : le placer dans le workspace pour garder une version unique et cohérente (idem pour le dev-dependency de devcontainer-rs).
messages.clone() et tool_definitions.clone() à chaque itération recopient toute la conversation à chaque tour (coût O(n²)) alors qu'un passage par référence (&[Message]) suffirait. Par ailleurs rien ne borne la taille du contenu renvoyé par les outils : ajouter un plafond par appel pour maîtriser le contexte et le coût.
read_file exécute cat sans aucune borne : un gros fichier (ou binaire) injecte tout son contenu dans la conversation et peut saturer le contexte et faire exploser le coût. Plafonner le nombre de lignes/octets renvoyés avec une note de troncature (idem pour grep et find, potentiellement très verbeux).
L'étage final debian:trixie-slim ne contient que le binaire, or herald-server a besoin de git (clone de la PR) et d'un binaire de runtime conteneur (docker/podman) pour exécuter la sandbox. Avec cette image, chaque revue échouera à l'exécution. Ajouter ces paquets (plus ca-certificates) ou documenter explicitement le montage du socket/runtime requis.
Incohérence de toolchain : rust:1.97-trixie ici, alors que .devcontainer/Dockerfile et .woodpecker/tests.yml utilisent rust:1.98. Aligner les versions et écrire AS en majuscules (convention Dockerfile).
base_dir.join(schema.build.dockerfile) : un dockerfile absolu (/etc/passwd) ou contenant .. sort du dossier du devcontainer et devient le -f de docker build (lecture de fichier arbitraire côté hôte). Valider que le chemin reste bien sous base_dir (via normalize par exemple) avant de l'accepter.
Le token d’authentification est passé via GIT_CONFIG_VALUE_0. C’est correct car il n’apparaît pas dans les arguments. Toutefois, en cas d’erreur, git peut afficher la ligne de commande dans les logs, ce qui pourrait exposer le token indirectement via l’URL de clonage. Envisager d’utiliser un credential helper ou de filtrer les messages d’erreur.
Le chemin dest est converti en chaîne via display(), ce qui peut planter si le chemin contient des caractères non-UTF-8. Dans un contexte Linux, c’est très rare, mais il serait plus sûr d’utiliser to_string_lossy().
La fonction sanitize tronque les tirets et points en début et fin, mais elle pourrait laisser passer des séquences de deux tirets consécutifs qui sont autorisées par Docker. Ce n’est pas critique, mais une validation plus stricte (limiter la longueur, interdire les séquences interdites) améliorerait la robustesse.
La vérification est redondante : normalize() garantit déjà que le chemin ne peut pas sortir de la racine. Cependant, la double protection ne pose pas de problème de sécurité. On pourrait simplifier en supprimant cette condition si on est certain que normalize() ne retourne qu’un chemin bien formé.