• Joined on 2026-05-18
Herald commented on pull request qpismont/herald#5 2026-06-12 23:45:03 +02:00
Fix missing env var error

Review Feedback

Herald commented on pull request qpismont/herald#5 2026-06-12 23:45:03 +02:00
Fix missing env var error

Le nom de variable env est trop générique. Envisagez un nom plus explicite comme value ou env_value pour indiquer qu'il s'agit de la valeur de la variable d'environnement, et non de l'environnement lui-même.

Herald commented on pull request qpismont/herald#5 2026-06-12 23:45:03 +02:00
Fix missing env var error

L'utilisation de format! à l'intérieur de anyhow! est redondante. anyhow! accepte directement une chaîne formatée. Préférez anyhow!("{}: {}", key, e) pour plus de clarté et de concision.

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

Review Feedback

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

Le code utilise tokio::signal::unix qui ne fonctionne que sous Linux/Unix. Assurez-vous que l'application ne cible que ces systèmes, ou ajoutez une gestion alternative pour Windows (ex: tokio::signal::ctrl_c). Pour une première release, cela peut être acceptable si le déploiement est Linux uniquement.

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

L'image Rust 1.96 n'est pas une version stable connue. Privilégiez une version précise et stable (ex: rust:1.85.0) pour garantir la reproductibilité des builds. Si vous utilisez une nightly, indiquez-le clairement avec une date ou un digest.

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

Debian trixie est actuellement une version Testing, pas encore stable. Utilisez une version stable comme debian:bookworm-slim (Debian 12) ou debian:bullseye-slim pour éviter des surprises lors de mises à jour non contrôlées.

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

La commande push ne précise pas le registre complet. Comme l'image a été taguée avec docker.io/..., cela fonctionnera, mais il peut être plus explicite d'utiliser buildah push "docker.io/${IMAGE}:${TAG}" pour éviter toute ambiguïté. Ajoutez également un tag latest si cela est souhaité pour les releases.

Herald commented on pull request qpismont/herald#4 2026-06-10 21:33:03 +02:00
prepare first release with graceful shutdown + containerfile + push to

L'import Bytes semble inutilisé dans ce fichier (le nom est importé mais jamais référencé). Supprimez-le pour éviter un avertissement du compilateur. Vérifiez la suite du fichier, mais si non utilisé, il est préférable de le retirer.

Herald commented on pull request qpismont/herald#3 2026-06-10 19:54:13 +02:00
Starting impl Sentry and tracing

Review Feedback

Herald commented on pull request qpismont/herald#3 2026-06-10 19:54:13 +02:00
Starting impl Sentry and tracing

La dépendance sentry est configurée avec la feature tower-axum-matched-path, mais le fichier Cargo.lock inclut également sentry-actix, ce qui suggère que les features par défaut ne sont pas désactivées. Pour éviter de tirer des dépendances inutiles (comme actix), il est recommandé d'ajouter default-features = false. Exemple : sentry = { version = "0.48", default-features = false, features = ["tower-axum-matched-path"] }.

Herald commented on pull request qpismont/herald#3 2026-06-10 19:54:13 +02:00
Starting impl Sentry and tracing

Les commentaires sont en français, alors que le reste du code est en anglais. Pour la cohérence du projet, il est conseillé de rédiger tous les commentaires en anglais (ou dans une seule langue).

Herald commented on pull request qpismont/herald#3 2026-06-10 19:54:13 +02:00
Starting impl Sentry and tracing

Ajouter un event processor à chaque requête est problématique : les processors s'accumulent et ne sont jamais retirés, ce qui peut entraîner une fuite mémoire et une dégradation des performances. De plus, Sentry peut déjà capturer le corps de la requête si l'option attach_request_body est activée dans ClientOptions (ou via l'intégration tower). Il est préférable de configurer cela globalement plutôt que manuellement à chaque appel.

Herald commented on pull request qpismont/herald#3 2026-06-10 19:54:13 +02:00
Starting impl Sentry and tracing

Le fichier importe des éléments d'Axum, alors que le projet semble utiliser Actix-Web (présent dans Cargo.lock). Cette incohérence peut causer des erreurs de compilation ou des comportements inattendus. Vérifiez si le projet est en cours de migration vers Axum ou s'il s'agit d'une erreur. Dans tous les cas, un seul framework HTTP doit être utilisé.

Herald commented on pull request qpismont/herald#2 2026-06-06 19:38:36 +02:00
started gitea api impl

Review Feedback

Herald commented on pull request qpismont/herald#2 2026-06-06 19:28:46 +02:00
started gitea api impl

La pull request implémente une API Gitea pour un bot de revue de code. Les principaux problèmes de sécurité relevés sont : 1) Absence de validation de l'URL de téléchargement du diff (risque SSRF). 2) Désactivation de la sécurité du conteneur dans le devcontainer. 3) Stockage du token Gitea en mémoire sans mécanisme de protection avancé. Des améliorations de fiabilité sont également nécessaires (canal de taille 1, parsing de diff non robuste). Il est recommandé de corriger ces points avant de finaliser l'implémentation.

Herald commented on pull request qpismont/herald#2 2026-06-06 19:19:01 +02:00
started gitea api impl

Les principaux problèmes de sécurité identifiés sont : l'absence de vérification de signature des webhooks (permettant à quiconque de déclencher des revues), un risque de SSRF via l'URL de diff non validée, et l'utilisation d'un client HTTP séparé sans restriction. Il est recommandé d'ajouter la validation HMAC, de restreindre l'URL de diff au domaine Gitea configuré, et de réutiliser le client de GiteaAPI. Les autres modifications (Cargo.lock, etc.) sont correctes.

Herald commented on pull request qpismont/herald#2 2026-06-06 18:53:34 +02:00
started gitea api impl

La PR introduit une architecture plus complète avec un canal asynchrone entre l'API et le bot, une gestion robuste des erreurs et le passage à rustls pour le TLS, ce qui améliore la sécurité en éliminant les dépendances OpenSSL. Plusieurs améliorations de sécurité sont bienvenues : limitation de taille du diff, timeouts configurés, et utilisation d'en-têtes Authorization par défaut. Toutefois, quelques points pourraient être renforcés : validation de l'origine des URLs récupérées via le webhook, protection contre les valeurs extrêmes de timeout, et gestion correcte des noms de fichiers sans extension pour la coloration syntaxique. Dans l'ensemble, la qualité du code est bonne pour un travail en cours, avec une attention particulière à la sécurité via le choix des bibliothèques et la limitation des ressources.