qpismont
changed title from Project website + use Apalis crate for task queue (with redis backend) to Postgres persistance + job queue + api with sse2026-10-13 11:41:53 +02:00
La persistance PostgreSQL, les migrations versionnées, la limite du corps webhook et le bornage de la concurrence sont de bonnes évolutions, et la validation des positions de review ainsi que le nettoyage des sandboxes améliorent nettement la robustesse. Les points à corriger concernent surtout la réservation concurrente des jobs, l'accumulation sans limite, l'arrêt avec une concurrence nulle et la perte définitive des reviews en échec.
Cost: $0.06907231999999999
## Review Feedback
### 6 issues found.
- 5 bug
- 1 performance
---
### Summary
La persistance PostgreSQL, les migrations versionnées, la limite du corps webhook et le bornage de la concurrence sont de bonnes évolutions, et la validation des positions de review ainsi que le nettoyage des sandboxes améliorent nettement la robustesse. Les points à corriger concernent surtout la réservation concurrente des jobs, l'accumulation sans limite, l'arrêt avec une concurrence nulle et la perte définitive des reviews en échec.
---
### Cost: $0.06907231999999999
[bug]BOT_MAX_CONCURRENT=0 est accepté par le parsing de configuration, mais acquire_owned() attendra alors indéfiniment un permis. Comme cette attente n'est pas incluse dans un select! avec l'annulation, l'arrêt peut rester bloqué jusqu'au délai forcé de 60 secondes. Il faut rejeter zéro au chargement de configuration ou rendre l'attente annulable.
**[bug]** `BOT_MAX_CONCURRENT=0` est accepté par le parsing de configuration, mais `acquire_owned()` attendra alors indéfiniment un permis. Comme cette attente n'est pas incluse dans un `select!` avec l'annulation, l'arrêt peut rester bloqué jusqu'au délai forcé de 60 secondes. Il faut rejeter zéro au chargement de configuration ou rendre l'attente annulable.
[bug] En cas d'échec de mark_processing, le code journalise l'erreur mais lance quand même le traitement. Le job reste alors Pending et peut être repris par le poller pendant que la review est déjà en cours, ce qui crée des traitements et commentaires en double. Il faut abandonner ce job (ou le remettre explicitement dans un état contrôlé) lorsque la réservation échoue.
**[bug]** En cas d'échec de `mark_processing`, le code journalise l'erreur mais lance quand même le traitement. Le job reste alors `Pending` et peut être repris par le poller pendant que la review est déjà en cours, ce qui crée des traitements et commentaires en double. Il faut abandonner ce job (ou le remettre explicitement dans un état contrôlé) lorsque la réservation échoue.
[bug] Même quand exec échoue, le code enregistre une Review avec ok = false, puis supprime le job. Un webhook de retry sera ensuite considéré comme déjà traité par is_reviewed et ignoré : une panne temporaire de Gitea, du sandbox ou d'OpenRouter devient donc un échec définitif sans possibilité de retry. Les jobs échoués devraient rester en file (avec backoff/retry) ou être distingués des reviews effectivement publiées.
**[bug]** Même quand `exec` échoue, le code enregistre une `Review` avec `ok = false`, puis supprime le job. Un webhook de retry sera ensuite considéré comme déjà traité par `is_reviewed` et ignoré : une panne temporaire de Gitea, du sandbox ou d'OpenRouter devient donc un échec définitif sans possibilité de retry. Les jobs échoués devraient rester en file (avec backoff/retry) ou être distingués des reviews effectivement publiées.
[bug] Au démarrage, tous les jobs Processing sont remis à Pending sans notion de propriétaire ou de lease. Si deux instances tournent simultanément, le démarrage de l'une peut réenfiler les jobs activement traités par l'autre, en plus de la course déjà présente dans le poller. La récupération après crash doit être limitée aux jobs dont le lease a expiré, et le traitement doit être atomiquement réservé.
**[bug]** Au démarrage, tous les jobs `Processing` sont remis à `Pending` sans notion de propriétaire ou de lease. Si deux instances tournent simultanément, le démarrage de l'une peut réenfiler les jobs activement traités par l'autre, en plus de la course déjà présente dans le poller. La récupération après crash doit être limitée aux jobs dont le lease a expiré, et le traitement doit être atomiquement réservé.
[bug] La lecture des jobs Pending et leur passage à Processing ne sont pas atomiques. Avec plusieurs instances du serveur (ou plusieurs consommateurs), elles peuvent toutes récupérer le même job avant l'appel à mark_processing, puis exécuter plusieurs fois la review et publier des commentaires dupliqués. Il faut effectuer une réservation conditionnelle en base (UPDATE ... WHERE status = 'pending' ... RETURNING) plutôt que seulement filtrer puis mettre à jour.
**[bug]** La lecture des jobs `Pending` et leur passage à `Processing` ne sont pas atomiques. Avec plusieurs instances du serveur (ou plusieurs consommateurs), elles peuvent toutes récupérer le même job avant l'appel à `mark_processing`, puis exécuter plusieurs fois la review et publier des commentaires dupliqués. Il faut effectuer une réservation conditionnelle en base (`UPDATE ... WHERE status = 'pending' ... RETURNING`) plutôt que seulement filtrer puis mettre à jour.
[performance] Cette requête récupère tous les jobs en attente sans limite, puis les accumule dans le VecDeque à la ligne suivante. Une file PostgreSQL qui reçoit plus de jobs qu'elle n'en traite peut donc faire croître la mémoire du processus sans borne pratique. Il faudrait paginer ou limiter le nombre de jobs réservés par poll.
**[performance]** Cette requête récupère tous les jobs en attente sans limite, puis les accumule dans le `VecDeque` à la ligne suivante. Une file PostgreSQL qui reçoit plus de jobs qu'elle n'en traite peut donc faire croître la mémoire du processus sans borne pratique. Il faudrait paginer ou limiter le nombre de jobs réservés par poll.
La file PostgreSQL apporte une bonne base pour la reprise après redémarrage, et les erreurs d’enqueue sont correctement renvoyées à l’appelant. À corriger : le chargement non borné des jobs en attente, la poursuite du traitement après échec du marquage, et la suppression du job lorsque l’enregistrement du résultat échoue.
Cost: $0.022133020000000003
## Review Feedback
### 3 issues found.
- 2 bug
- 1 performance
---
### Summary
La file PostgreSQL apporte une bonne base pour la reprise après redémarrage, et les erreurs d’enqueue sont correctement renvoyées à l’appelant. À corriger : le chargement non borné des jobs en attente, la poursuite du traitement après échec du marquage, et la suppression du job lorsque l’enregistrement du résultat échoue.
---
### Cost: $0.022133020000000003
[bug] Si la mise à jour du statut échoue, le job est quand même traité. Il reste alors Pending et sera récupéré au prochain sondage, ce qui peut lancer plusieurs reviews concurrentes pour le même événement. Ne lancez pas le traitement si cette transition échoue, ou garantissez une prise en charge atomique.
**[bug]** Si la mise à jour du statut échoue, le job est quand même traité. Il reste alors `Pending` et sera récupéré au prochain sondage, ce qui peut lancer plusieurs reviews concurrentes pour le même événement. Ne lancez pas le traitement si cette transition échoue, ou garantissez une prise en charge atomique.
[bug] Si mark_reviewed échoue, l’erreur est seulement journalisée, puis le job est supprimé. Une panne temporaire de la base peut donc faire perdre définitivement l’événement sans enregistrer son résultat. Ne supprimez le job qu’après l’enregistrement réussi, ou conservez-le pour reprise.
**[bug]** Si `mark_reviewed` échoue, l’erreur est seulement journalisée, puis le job est supprimé. Une panne temporaire de la base peut donc faire perdre définitivement l’événement sans enregistrer son résultat. Ne supprimez le job qu’après l’enregistrement réussi, ou conservez-le pour reprise.
[performance] Cette requête récupère tous les jobs en attente, puis les met en mémoire dans le buffer. Si la file grossit ou si le service redémarre avec un grand retard à rattraper, chaque worker peut charger un volume non borné de payloads JSON. Limitez la taille des lots et ne chargez que les jobs nécessaires à la capacité disponible.
**[performance]** Cette requête récupère tous les jobs en attente, puis les met en mémoire dans le buffer. Si la file grossit ou si le service redémarre avec un grand retard à rattraper, chaque worker peut charger un volume non borné de payloads JSON. Limitez la taille des lots et ne chargez que les jobs nécessaires à la capacité disponible.
La PR apporte une base solide pour la persistance PostgreSQL, une file durable, des schémas explicites et des tests d’intégration, et la documentation comme les métriques sont bien étoffées. Les points à corriger concernent le traitement concurrent des jobs, leur récupération au démarrage, l’arrêt gracieux et les instructions de mise en route.
Cost: $0.021829394999999998
## Review Feedback
### 4 issues found.
- 4 bug
---
### Summary
La PR apporte une base solide pour la persistance PostgreSQL, une file durable, des schémas explicites et des tests d’intégration, et la documentation comme les métriques sont bien étoffées. Les points à corriger concernent le traitement concurrent des jobs, leur récupération au démarrage, l’arrêt gracieux et les instructions de mise en route.
---
### Cost: $0.021829394999999998
[bug] L’attente d’un permis n’écoute pas shutdown. Si tous les slots sont occupés au moment de l’arrêt, le bot attend qu’un slot se libère puis lance quand même le job en attente, et peut continuer ainsi à vider la file. Cela contredit le comportement annoncé où seuls les jobs déjà en cours terminent ; rendez aussi cette attente interruptible.
**[bug]** L’attente d’un permis n’écoute pas `shutdown`. Si tous les slots sont occupés au moment de l’arrêt, le bot attend qu’un slot se libère puis lance quand même le job en attente, et peut continuer ainsi à vider la file. Cela contredit le comportement annoncé où seuls les jobs déjà en cours terminent ; rendez aussi cette attente interruptible.
[bug] Au démarrage, chaque instance remet à Pending tous les jobs Processing, y compris ceux qu’une autre instance est encore en train de traiter. Un redémarrage ou déploiement pendant une review peut donc la faire exécuter en double. La récupération doit distinguer les jobs réellement abandonnés de ceux détenus par des workers actifs.
**[bug]** Au démarrage, chaque instance remet à `Pending` tous les jobs `Processing`, y compris ceux qu’une autre instance est encore en train de traiter. Un redémarrage ou déploiement pendant une review peut donc la faire exécuter en double. La récupération doit distinguer les jobs réellement abandonnés de ceux détenus par des workers actifs.
[bug] La lecture des jobs en attente n’en réserve aucun : deux instances peuvent récupérer le même job avant que l’une d’elles n’appelle mark_processing, puis lancer deux reviews et poster des résultats en double. Il faut réclamer les jobs de façon atomique, par exemple avec une mise à jour conditionnelle ou un verrouillage SKIP LOCKED.
**[bug]** La lecture des jobs en attente n’en réserve aucun : deux instances peuvent récupérer le même job avant que l’une d’elles n’appelle `mark_processing`, puis lancer deux reviews et poster des résultats en double. Il faut réclamer les jobs de façon atomique, par exemple avec une mise à jour conditionnelle ou un verrouillage `SKIP LOCKED`.
[bug] Le guide demande de configurer neuf variables, mais omet DATABASE_URL, désormais obligatoire, puis passe directement au lancement sans expliquer comment appliquer les migrations. Un nouvel utilisateur suivant ces étapes ne peut pas démarrer le service. Ajoutez la configuration de PostgreSQL et l’étape migration apply.
**[bug]** Le guide demande de configurer neuf variables, mais omet `DATABASE_URL`, désormais obligatoire, puis passe directement au lancement sans expliquer comment appliquer les migrations. Un nouvel utilisateur suivant ces étapes ne peut pas démarrer le service. Ajoutez la configuration de PostgreSQL et l’étape `migration apply`.
Expose /jobs, /dedup, and /events routes backed by a new EventBus that
broadcasts job lifecycle and per-job progress over SSE. Serve a static
dashboard from front/ that lists pending and in-progress jobs, streams
agent activity, and shows completed reviews.
Also add migrate-apply/migrate-generate scripts that load .env, update
the Containerfile to ship the front assets and use the apply script, and
refresh the roadmap.
La PR apporte une vraie file durable en PostgreSQL, des événements SSE avec historique de progression, et un dashboard utile pour suivre les jobs. Les principaux points à corriger concernent l’exposition sans authentification des données de review, la réservation non atomique des jobs et plusieurs cas de concurrence ou de reconnexion qui peuvent produire des traitements en double ou désynchroniser l’interface.
Cost: $0.030983235
## Review Feedback
### 7 issues found.
- 5 bug
- 1 security
- 1 performance
---
### Summary
La PR apporte une vraie file durable en PostgreSQL, des événements SSE avec historique de progression, et un dashboard utile pour suivre les jobs. Les principaux points à corriger concernent l’exposition sans authentification des données de review, la réservation non atomique des jobs et plusieurs cas de concurrence ou de reconnexion qui peuvent produire des traitements en double ou désynchroniser l’interface.
---
### Cost: $0.030983235
[security] Ces routes de consultation et les flux SSE sont accessibles sans authentification. Elles exposent notamment les noms de dépôts, les reviews et les sorties des outils exécutés sur le code, qui peuvent contenir des données de dépôts privés. Protégez ces endpoints ou limitez explicitement leur accès.
**[security]** Ces routes de consultation et les flux SSE sont accessibles sans authentification. Elles exposent notamment les noms de dépôts, les reviews et les sorties des outils exécutés sur le code, qui peuvent contenir des données de dépôts privés. Protégez ces endpoints ou limitez explicitement leur accès.
[bug] Le test is_known et l’insertion dans la file ne sont pas atomiques. Deux livraisons simultanées du même webhook peuvent toutes deux passer le test ; la contrainte unique fait ensuite échouer l’une et renvoie 503 au lieu de traiter ce doublon comme déjà accepté. Gérez explicitement ce conflit d’insertion.
**[bug]** Le test `is_known` et l’insertion dans la file ne sont pas atomiques. Deux livraisons simultanées du même webhook peuvent toutes deux passer le test ; la contrainte unique fait ensuite échouer l’une et renvoie 503 au lieu de traiter ce doublon comme déjà accepté. Gérez explicitement ce conflit d’insertion.
[bug] En cas d’échec de mark_processing, le job est tout de même traité. Il reste alors Pending en base et pourra être repris après redémarrage — ou par une autre instance — pendant que cette exécution peut déjà publier la review. N’exécutez pas le job si sa transition d’état échoue.
**[bug]** En cas d’échec de `mark_processing`, le job est tout de même traité. Il reste alors Pending en base et pourra être repris après redémarrage — ou par une autre instance — pendant que cette exécution peut déjà publier la review. N’exécutez pas le job si sa transition d’état échoue.
[performance]Review::all() charge toutes les reviews et leurs champs, notamment le Markdown, à chaque rafraîchissement du dashboard. Cette requête et la réponse JSON grossissent sans limite avec l’historique. Ajoutez une pagination ou une limite et ne chargez que les champs nécessaires à la liste.
**[performance]** `Review::all()` charge toutes les reviews et leurs champs, notamment le Markdown, à chaque rafraîchissement du dashboard. Cette requête et la réponse JSON grossissent sans limite avec l’historique. Ajoutez une pagination ou une limite et ne chargez que les champs nécessaires à la liste.
[bug] La sélection des jobs Pending et leur passage à Processing sont deux opérations distinctes, sans réservation atomique. Avec plusieurs instances, chacune peut récupérer le même job et lancer la même review avant que l’autre ne le marque Processing. Il faut réclamer les jobs de façon atomique.
**[bug]** La sélection des jobs Pending et leur passage à Processing sont deux opérations distinctes, sans réservation atomique. Avec plusieurs instances, chacune peut récupérer le même job et lancer la même review avant que l’autre ne le marque Processing. Il faut réclamer les jobs de façon atomique.
[bug]refresh() est lancé sans attendre avant l’abonnement SSE. Si un événement arrive pendant le chargement, la réponse REST peut ensuite écraser l’état plus récent fourni par SSE ; aucun nouvel événement n’est garanti pour corriger la liste. Abonnez-vous avant de charger l’état, ou réconciliez la réponse avec les événements reçus entre-temps.
**[bug]** `refresh()` est lancé sans attendre avant l’abonnement SSE. Si un événement arrive pendant le chargement, la réponse REST peut ensuite écraser l’état plus récent fourni par SSE ; aucun nouvel événement n’est garanti pour corriger la liste. Abonnez-vous avant de charger l’état, ou réconciliez la réponse avec les événements reçus entre-temps.
[bug] Le gestionnaire ferme l’EventSource à la première erreur, y compris lors d’une coupure réseau temporaire pour laquelle EventSource aurait normalement réessayé. L’utilisateur perd alors les mises à jour du job. Fermez-le seulement quand la fin du job est confirmée ou quand le serveur indique que le job n’existe plus.
**[bug]** Le gestionnaire ferme l’EventSource à la première erreur, y compris lors d’une coupure réseau temporaire pour laquelle EventSource aurait normalement réessayé. L’utilisateur perd alors les mises à jour du job. Fermez-le seulement quand la fin du job est confirmée ou quand le serveur indique que le job n’existe plus.
Cette PR apporte une persistance Postgres propre avec migrations Toasty, une file de jobs durable et un dashboard SSE. Elle est bien structurée et bien testée.
Ce qui est bien fait :
Le claim_next atomique (UPDATE avec sous-requête et RETURNING).
Le requeue_interrupted au démarrage.
Le réveil du worker par Notify plutôt que par polling.
Le sémaphore pris avant le claim.
Le bus d'événements avec historique rejoué et abonnement sans trou.
Le délai maximal d'arrêt, avec fermeture des flux SSE.
Le passage à rustls/ring, verrouillé par cargo deny.
Le découpage du module review en sous-modules.
L'échappement HTML côté front.
La documentation des migrations et de la file de jobs.
Problèmes à traiter :
Sécurité : les routes du dashboard et les flux SSE sont publics sans authentification. Ils exposent des dépôts privés, des payloads et des extraits de code.
Webhooks : la course entre is_known et push provoque des 503 au lieu d'un 200 « déjà traité ».
Ressources :/reviews n'est pas paginé, et les sorties d'outils conservées dans l'historique SSE ne sont pas tronquées.
Robustesse du worker : une panique de tâche laisse le job bloqué en processing et fait dériver la jauge.
Front : le message « vide » reste affiché dans job-list, il n'y a pas de resynchronisation à la reconnexion SSE, et le bouton « Watch » mène à un 404 pour les jobs en attente.
Outillage : le socket podman est codé en dur dans le devcontainer, cargo-deny est recompilé à chaque run de CI, et les versions de Rust diffèrent entre le Containerfile et la CI.
Cost: $1.4524780000000002
## Review Feedback
### 9 issues found.
- 4 bug
- 1 security
- 3 performance
- 1 maintainability
---
### Summary
Cette PR apporte une persistance Postgres propre avec migrations Toasty, une file de jobs durable et un dashboard SSE. Elle est bien structurée et bien testée.
Ce qui est bien fait :
- Le `claim_next` atomique (UPDATE avec sous-requête et RETURNING).
- Le `requeue_interrupted` au démarrage.
- Le réveil du worker par `Notify` plutôt que par polling.
- Le sémaphore pris avant le claim.
- Le bus d'événements avec historique rejoué et abonnement sans trou.
- Le délai maximal d'arrêt, avec fermeture des flux SSE.
- Le passage à rustls/ring, verrouillé par `cargo deny`.
- Le découpage du module `review` en sous-modules.
- L'échappement HTML côté front.
- La documentation des migrations et de la file de jobs.
Problèmes à traiter :
- **Sécurité :** les routes du dashboard et les flux SSE sont publics sans authentification. Ils exposent des dépôts privés, des payloads et des extraits de code.
- **Webhooks :** la course entre `is_known` et `push` provoque des 503 au lieu d'un 200 « déjà traité ».
- **Ressources :** `/reviews` n'est pas paginé, et les sorties d'outils conservées dans l'historique SSE ne sont pas tronquées.
- **Robustesse du worker :** une panique de tâche laisse le job bloqué en `processing` et fait dériver la jauge.
- **Front :** le message « vide » reste affiché dans `job-list`, il n'y a pas de resynchronisation à la reconnexion SSE, et le bouton « Watch » mène à un 404 pour les jobs en attente.
- **Outillage :** le socket podman est codé en dur dans le devcontainer, `cargo-deny` est recompilé à chaque run de CI, et les versions de Rust diffèrent entre le `Containerfile` et la CI.
---
### Cost: $1.4524780000000002
[maintainability] Le socket podman est monté avec le chemin en dur /run/user/1000/podman/podman.sock. Le devcontainer ne démarre pas pour un utilisateur dont l'UID n'est pas 1000, alors que le build utilise ${localEnv:UID}. De plus, si le socket n'existe pas, le montage fait échouer l'ouverture du conteneur. Il faut utiliser ${localEnv:XDG_RUNTIME_DIR}/podman/podman.sock.
**[maintainability]** Le socket podman est monté avec le chemin en dur `/run/user/1000/podman/podman.sock`. Le devcontainer ne démarre pas pour un utilisateur dont l'UID n'est pas 1000, alors que le build utilise `${localEnv:UID}`. De plus, si le socket n'existe pas, le montage fait échouer l'ouverture du conteneur. Il faut utiliser `${localEnv:XDG_RUNTIME_DIR}/podman/podman.sock`.
[performance]cargo install cargo-deny --locked recompile l'outil à chaque pipeline, ce qui ajoute plusieurs minutes de CI. Il vaut mieux utiliser une image préconstruite (ghcr.io/embarkstudios/cargo-deny) ou un binaire précompilé. Par ailleurs les étapes CI utilisent rust:1.99 alors que le Containerfile build avec rust:1.98-trixie : les versions de toolchain divergent.
**[performance]** `cargo install cargo-deny --locked` recompile l'outil à chaque pipeline, ce qui ajoute plusieurs minutes de CI. Il vaut mieux utiliser une image préconstruite (`ghcr.io/embarkstudios/cargo-deny`) ou un binaire précompilé. Par ailleurs les étapes CI utilisent `rust:1.99` alors que le `Containerfile` build avec `rust:1.98-trixie` : les versions de toolchain divergent.
[security] Les routes /reviews, /jobs, /events et /jobs/{id}/events sont publiques, sans authentification. Elles exposent les noms de dépôts privés, les numéros de PR, le payload complet des webhooks (corps des commentaires, clone_url), le markdown des reviews et le flux des appels d'outils de l'agent. Ce flux contient des extraits du code des dépôts, ce qui est sensible. Le service tourne sur 0.0.0.0 et le dashboard est servi par le même processus. Il faut protéger ces routes (token ou basic auth, ou reverse proxy authentifié), ou au moins les placer sur un port interne distinct du /webhook. Il n'y a aussi aucune limite sur le nombre de connexions SSE ouvertes.
**[security]** Les routes `/reviews`, `/jobs`, `/events` et `/jobs/{id}/events` sont publiques, sans authentification. Elles exposent les noms de dépôts privés, les numéros de PR, le payload complet des webhooks (corps des commentaires, clone_url), le markdown des reviews et le flux des appels d'outils de l'agent. Ce flux contient des extraits du code des dépôts, ce qui est sensible. Le service tourne sur `0.0.0.0` et le dashboard est servi par le même processus. Il faut protéger ces routes (token ou basic auth, ou reverse proxy authentifié), ou au moins les placer sur un port interne distinct du `/webhook`. Il n'y a aussi aucune limite sur le nombre de connexions SSE ouvertes.
[bug] Le contrôle is_known puis queue.push n'est pas atomique. Deux livraisons concurrentes du même événement (retry Gitea) passent toutes les deux le contrôle. La seconde échoue sur la contrainte UNIQUE(event_id) et renvoie 503 Could not enqueue task, avec un faux événement Sentry et le compteur enqueue_failed. Gitea risque alors de re-livrer en boucle. Il faut traiter la violation d'unicité comme AlreadyProcessedErr (200), ou faire un insert avec ON CONFLICT DO NOTHING.
**[bug]** Le contrôle `is_known` puis `queue.push` n'est pas atomique. Deux livraisons concurrentes du même événement (retry Gitea) passent toutes les deux le contrôle. La seconde échoue sur la contrainte `UNIQUE(event_id)` et renvoie 503 `Could not enqueue task`, avec un faux événement Sentry et le compteur `enqueue_failed`. Gitea risque alors de re-livrer en boucle. Il faut traiter la violation d'unicité comme `AlreadyProcessedErr` (200), ou faire un insert avec `ON CONFLICT DO NOTHING`.
[performance] Contrairement aux arguments (tronqués à 200 caractères), output est conservé en entier, jusqu'à 32 Ko par appel d'outil. L'historique garde 256 événements par job, soit environ 8 Mo par job actif. Chaque nouvel abonné SSE reçoit ensuite tout cet historique. Il faut tronquer l'aperçu envoyé au dashboard (comme pour MAX_EVENT_ARGUMENTS), ce qui limite aussi ce qui fuit sur une route publique.
**[performance]** Contrairement aux arguments (tronqués à 200 caractères), `output` est conservé en entier, jusqu'à 32 Ko par appel d'outil. L'historique garde 256 événements par job, soit environ 8 Mo par job actif. Chaque nouvel abonné SSE reçoit ensuite tout cet historique. Il faut tronquer l'aperçu envoyé au dashboard (comme pour `MAX_EVENT_ARGUMENTS`), ce qui limite aussi ce qui fuit sur une route publique.
[performance]list() charge toutes les reviews, y compris la colonne markdown, sans pagination ni tri, et /reviews les renvoie toutes d'un coup. La table ne fait que grossir et chaque ouverture ou rafraîchissement du dashboard relit tout l'historique. Il faut une limite (par exemple les N dernières, triées par completed_at DESC), ou omettre markdown dans la liste.
**[performance]** `list()` charge toutes les reviews, y compris la colonne `markdown`, sans pagination ni tri, et `/reviews` les renvoie toutes d'un coup. La table ne fait que grossir et chaque ouverture ou rafraîchissement du dashboard relit tout l'historique. Il faut une limite (par exemple les N dernières, triées par `completed_at DESC`), ou omettre `markdown` dans la liste.
[bug] Le flux /events reconnecte automatiquement (EventSource), mais les événements émis pendant la coupure (redémarrage serveur, réseau) sont perdus, sans événement gap. Le dashboard garde alors un état périmé (jobs fantômes dans Pending/In progress). Il faut brancher onOpen pour appeler refresh() à chaque (re)connexion, y compris la première.
**[bug]** Le flux `/events` reconnecte automatiquement (EventSource), mais les événements émis pendant la coupure (redémarrage serveur, réseau) sont perdus, sans événement `gap`. Le dashboard garde alors un état périmé (jobs fantômes dans Pending/In progress). Il faut brancher `onOpen` pour appeler `refresh()` à chaque (re)connexion, y compris la première.
[bug] Le bouton « Watch activity » est affiché aussi pour les jobs pending. Or /jobs/{id}/events renvoie 404 tant que le job n'a pas démarré (le canal n'est créé que par EventBus::start). Le panneau affiche alors « stream closed » sans explication. Il faut masquer ou désactiver le bouton pour les jobs en attente, ou ré-ouvrir le flux au passage en started.
**[bug]** Le bouton « Watch activity » est affiché aussi pour les jobs `pending`. Or `/jobs/{id}/events` renvoie 404 tant que le job n'a pas démarré (le canal n'est créé que par `EventBus::start`). Le panneau affiche alors « stream closed » sans explication. Il faut masquer ou désactiver le bouton pour les jobs en attente, ou ré-ouvrir le flux au passage en `started`.
[bug] Dans la branche « liste vide », render() ajoute un <p class="empty">, mais la branche non vide ne le retire jamais. Après la première apparition d'un job (liste vide puis job), le message « No pending job. » reste affiché au-dessus ou au-dessous des cartes. Il faut supprimer l'élément .empty dans la branche non vide, ou faire un replaceChildren avant de ré-attacher les cartes.
**[bug]** Dans la branche « liste vide », `render()` ajoute un `<p class="empty">`, mais la branche non vide ne le retire jamais. Après la première apparition d'un job (liste vide puis job), le message « No pending job. » reste affiché au-dessus ou au-dessous des cartes. Il faut supprimer l'élément `.empty` dans la branche non vide, ou faire un `replaceChildren` avant de ré-attacher les cartes.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Postgres persistance + job queue + api with sse
Project website + use Apalis crate for task queue (with redis backend)to Postgres persistance + job queue + api with sseReview Feedback
6 issues found.
Summary
La persistance PostgreSQL, les migrations versionnées, la limite du corps webhook et le bornage de la concurrence sont de bonnes évolutions, et la validation des positions de review ainsi que le nettoyage des sandboxes améliorent nettement la robustesse. Les points à corriger concernent surtout la réservation concurrente des jobs, l'accumulation sans limite, l'arrêt avec une concurrence nulle et la perte définitive des reviews en échec.
Cost: $0.06907231999999999
@@ -182,3 +188,1 @@info!(queued = rx.len(), active = tasks.len(), "Webhook received");let permit = sem.clone().acquire_owned().await?;let self_clone = self.clone();let permit = semaphore.clone().acquire_owned().await?;[bug]
BOT_MAX_CONCURRENT=0est accepté par le parsing de configuration, maisacquire_owned()attendra alors indéfiniment un permis. Comme cette attente n'est pas incluse dans unselect!avec l'annulation, l'arrêt peut rester bloqué jusqu'au délai forcé de 60 secondes. Il faut rejeter zéro au chargement de configuration ou rendre l'attente annulable.@@ -184,1 +188,3 @@let self_clone = self.clone();let permit = semaphore.clone().acquire_owned().await?;if let Err(err) = self.queue.mark_processing(job.id).await {[bug] En cas d'échec de
mark_processing, le code journalise l'erreur mais lance quand même le traitement. Le job reste alorsPendinget peut être repris par le poller pendant que la review est déjà en cours, ce qui crée des traitements et commentaires en double. Il faut abandonner ce job (ou le remettre explicitement dans un état contrôlé) lorsque la réservation échoue.@@ -204,0 +255,4 @@duration_ms,};if let Err(err) = self.dedup.mark_reviewed(record).await {[bug] Même quand
execéchoue, le code enregistre uneReviewavecok = false, puis supprime le job. Un webhook de retry sera ensuite considéré comme déjà traité paris_reviewedet ignoré : une panne temporaire de Gitea, du sandbox ou d'OpenRouter devient donc un échec définitif sans possibilité de retry. Les jobs échoués devraient rester en file (avec backoff/retry) ou être distingués des reviews effectivement publiées.@@ -0,0 +12,4 @@pub async fn connect(url: &str) -> toasty::Result<Db> {let mut db = open(url).await?;Job::update_by_status(JobStatus::Processing)[bug] Au démarrage, tous les jobs
Processingsont remis àPendingsans notion de propriétaire ou de lease. Si deux instances tournent simultanément, le démarrage de l'une peut réenfiler les jobs activement traités par l'autre, en plus de la course déjà présente dans le poller. La récupération après crash doit être limitée aux jobs dont le lease a expiré, et le traitement doit être atomiquement réservé.@@ -0,0 +65,4 @@return Some((job, buffer));}match Job::filter_by_status(JobStatus::Pending)[bug] La lecture des jobs
Pendinget leur passage àProcessingne sont pas atomiques. Avec plusieurs instances du serveur (ou plusieurs consommateurs), elles peuvent toutes récupérer le même job avant l'appel àmark_processing, puis exécuter plusieurs fois la review et publier des commentaires dupliqués. Il faut effectuer une réservation conditionnelle en base (UPDATE ... WHERE status = 'pending' ... RETURNING) plutôt que seulement filtrer puis mettre à jour.[performance] Cette requête récupère tous les jobs en attente sans limite, puis les accumule dans le
VecDequeà la ligne suivante. Une file PostgreSQL qui reçoit plus de jobs qu'elle n'en traite peut donc faire croître la mémoire du processus sans borne pratique. Il faudrait paginer ou limiter le nombre de jobs réservés par poll.Review Feedback
3 issues found.
Summary
La file PostgreSQL apporte une bonne base pour la reprise après redémarrage, et les erreurs d’enqueue sont correctement renvoyées à l’appelant. À corriger : le chargement non borné des jobs en attente, la poursuite du traitement après échec du marquage, et la suppression du job lorsque l’enregistrement du résultat échoue.
Cost: $0.022133020000000003
@@ -184,1 +188,3 @@let self_clone = self.clone();let permit = semaphore.clone().acquire_owned().await?;if let Err(err) = self.queue.mark_processing(job.id).await {[bug] Si la mise à jour du statut échoue, le job est quand même traité. Il reste alors
Pendinget sera récupéré au prochain sondage, ce qui peut lancer plusieurs reviews concurrentes pour le même événement. Ne lancez pas le traitement si cette transition échoue, ou garantissez une prise en charge atomique.@@ -204,0 +259,4 @@error!(%err, "failed to record review");}if let Err(err) = self.queue.remove(job.id).await {[bug] Si
mark_reviewedéchoue, l’erreur est seulement journalisée, puis le job est supprimé. Une panne temporaire de la base peut donc faire perdre définitivement l’événement sans enregistrer son résultat. Ne supprimez le job qu’après l’enregistrement réussi, ou conservez-le pour reprise.@@ -0,0 +65,4 @@return Some((job, buffer));}match Job::filter_by_status(JobStatus::Pending)[performance] Cette requête récupère tous les jobs en attente, puis les met en mémoire dans le buffer. Si la file grossit ou si le service redémarre avec un grand retard à rattraper, chaque worker peut charger un volume non borné de payloads JSON. Limitez la taille des lots et ne chargez que les jobs nécessaires à la capacité disponible.
Review Feedback
4 issues found.
Summary
La PR apporte une base solide pour la persistance PostgreSQL, une file durable, des schémas explicites et des tests d’intégration, et la documentation comme les métriques sont bien étoffées. Les points à corriger concernent le traitement concurrent des jobs, leur récupération au démarrage, l’arrêt gracieux et les instructions de mise en route.
Cost: $0.021829394999999998
@@ -182,3 +188,1 @@info!(queued = rx.len(), active = tasks.len(), "Webhook received");let permit = sem.clone().acquire_owned().await?;let self_clone = self.clone();let permit = semaphore.clone().acquire_owned().await?;[bug] L’attente d’un permis n’écoute pas
shutdown. Si tous les slots sont occupés au moment de l’arrêt, le bot attend qu’un slot se libère puis lance quand même le job en attente, et peut continuer ainsi à vider la file. Cela contredit le comportement annoncé où seuls les jobs déjà en cours terminent ; rendez aussi cette attente interruptible.@@ -0,0 +12,4 @@pub async fn connect(url: &str) -> toasty::Result<Db> {let mut db = open(url).await?;Job::update_by_status(JobStatus::Processing)[bug] Au démarrage, chaque instance remet à
Pendingtous les jobsProcessing, y compris ceux qu’une autre instance est encore en train de traiter. Un redémarrage ou déploiement pendant une review peut donc la faire exécuter en double. La récupération doit distinguer les jobs réellement abandonnés de ceux détenus par des workers actifs.@@ -0,0 +65,4 @@return Some((job, buffer));}match Job::filter_by_status(JobStatus::Pending)[bug] La lecture des jobs en attente n’en réserve aucun : deux instances peuvent récupérer le même job avant que l’une d’elles n’appelle
mark_processing, puis lancer deux reviews et poster des résultats en double. Il faut réclamer les jobs de façon atomique, par exemple avec une mise à jour conditionnelle ou un verrouillageSKIP LOCKED.@@ -0,0 +551,4 @@<strong>Configure every required setting</strong><pre>cp .env.example .env$EDITOR .env</pre><p>Set all nine required variables:</p>[bug] Le guide demande de configurer neuf variables, mais omet
DATABASE_URL, désormais obligatoire, puis passe directement au lancement sans expliquer comment appliquer les migrations. Un nouvel utilisateur suivant ces étapes ne peut pas démarrer le service. Ajoutez la configuration de PostgreSQL et l’étapemigration apply.Review Feedback
7 issues found.
Summary
La PR apporte une vraie file durable en PostgreSQL, des événements SSE avec historique de progression, et un dashboard utile pour suivre les jobs. Les principaux points à corriger concernent l’exposition sans authentification des données de review, la réservation non atomique des jobs et plusieurs cas de concurrence ou de reconnexion qui peuvent produire des traitements en double ou désynchroniser l’interface.
Cost: $0.030983235
@@ -27,2 +40,3 @@.route("/", get(root)).route("/webhook", post(webhook)).route("/dedup", get(dedup_list)).route("/jobs", get(jobs_list))[security] Ces routes de consultation et les flux SSE sont accessibles sans authentification. Elles exposent notamment les noms de dépôts, les reviews et les sorties des outils exécutés sur le code, qui peuvent contenir des données de dépôts privés. Protégez ces endpoints ou limitez explicitement leur accès.
@@ -64,3 +146,1 @@if app_state.bot.check_and_mark(event_id).await {metrics::webhook_duplicate(event_type);return Err(AppError::AlreadyProcessedErr);match app_state.dedup.is_known(event_id).await {[bug] Le test
is_knownet l’insertion dans la file ne sont pas atomiques. Deux livraisons simultanées du même webhook peuvent toutes deux passer le test ; la contrainte unique fait ensuite échouer l’une et renvoie 503 au lieu de traiter ce doublon comme déjà accepté. Gérez explicitement ce conflit d’insertion.@@ -184,1 +193,3 @@let self_clone = self.clone();let permit = semaphore.clone().acquire_owned().await?;if let Err(err) = self.queue.mark_processing(job.id).await {[bug] En cas d’échec de
mark_processing, le job est tout de même traité. Il reste alors Pending en base et pourra être repris après redémarrage — ou par une autre instance — pendant que cette exécution peut déjà publier la review. N’exécutez pas le job si sa transition d’état échoue.@@ -0,0 +73,4 @@pub async fn list_reviews(&self) -> toasty::Result<Vec<Review>> {let mut db = self.db.clone();let reviews = Review::all().exec(&mut db).await?;[performance]
Review::all()charge toutes les reviews et leurs champs, notamment le Markdown, à chaque rafraîchissement du dashboard. Cette requête et la réponse JSON grossissent sans limite avec l’historique. Ajoutez une pagination ou une limite et ne chargez que les champs nécessaires à la liste.@@ -0,0 +57,4 @@return Some((job, buffer));}match Job::filter_by_status(JobStatus::Pending)[bug] La sélection des jobs Pending et leur passage à Processing sont deux opérations distinctes, sans réservation atomique. Avec plusieurs instances, chacune peut récupérer le même job et lancer la même review avant que l’autre ne le marque Processing. Il faut réclamer les jobs de façon atomique.
@@ -0,0 +23,4 @@this._selectedId = null;this.render();this.refresh();[bug]
refresh()est lancé sans attendre avant l’abonnement SSE. Si un événement arrive pendant le chargement, la réponse REST peut ensuite écraser l’état plus récent fourni par SSE ; aucun nouvel événement n’est garanti pour corriger la liste. Abonnez-vous avant de charger l’état, ou réconciliez la réponse avec les événements reçus entre-temps.@@ -0,0 +52,4 @@// Un `error` survient aussi quand le serveur ferme le flux (job terminé).// On ferme alors la source pour éviter la boucle de reconnexion (le job// n'est plus en cours, la route renverrait 404).onError: () => {[bug] Le gestionnaire ferme l’EventSource à la première erreur, y compris lors d’une coupure réseau temporaire pour laquelle EventSource aurait normalement réessayé. L’utilisateur perd alors les mises à jour du job. Fermez-le seulement quand la fin du job est confirmée ou quand le serveur indique que le job n’existe plus.
Review Feedback
9 issues found.
Summary
Cette PR apporte une persistance Postgres propre avec migrations Toasty, une file de jobs durable et un dashboard SSE. Elle est bien structurée et bien testée.
Ce qui est bien fait :
claim_nextatomique (UPDATE avec sous-requête et RETURNING).requeue_interruptedau démarrage.Notifyplutôt que par polling.cargo deny.reviewen sous-modules.Problèmes à traiter :
is_knownetpushprovoque des 503 au lieu d'un 200 « déjà traité »./reviewsn'est pas paginé, et les sorties d'outils conservées dans l'historique SSE ne sont pas tronquées.processinget fait dériver la jauge.job-list, il n'y a pas de resynchronisation à la reconnexion SSE, et le bouton « Watch » mène à un 404 pour les jobs en attente.cargo-denyest recompilé à chaque run de CI, et les versions de Rust diffèrent entre leContainerfileet la CI.Cost: $1.4524780000000002
@@ -20,2 +20,4 @@"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/steep,type=bind","workspaceFolder": "/workspaces/steep","mounts": ["source=/run/user/1000/podman/podman.sock,target=/var/run/docker.sock,type=bind"[maintainability] Le socket podman est monté avec le chemin en dur
/run/user/1000/podman/podman.sock. Le devcontainer ne démarre pas pour un utilisateur dont l'UID n'est pas 1000, alors que le build utilise${localEnv:UID}. De plus, si le socket n'existe pas, le montage fait échouer l'ouverture du conteneur. Il faut utiliser${localEnv:XDG_RUNTIME_DIR}/podman/podman.sock.@@ -20,2 +27,3 @@image: rust:1.99commands:- cargo test --workspace --all-targets- cargo install cargo-deny --locked --version 0.18.9[performance]
cargo install cargo-deny --lockedrecompile l'outil à chaque pipeline, ce qui ajoute plusieurs minutes de CI. Il vaut mieux utiliser une image préconstruite (ghcr.io/embarkstudios/cargo-deny) ou un binaire précompilé. Par ailleurs les étapes CI utilisentrust:1.99alors que leContainerfilebuild avecrust:1.98-trixie: les versions de toolchain divergent.@@ -26,3 +39,3 @@let app = Router::new().route("/", get(root)).route("/webhook", post(webhook)).route("/reviews", get(reviews_list))[security] Les routes
/reviews,/jobs,/eventset/jobs/{id}/eventssont publiques, sans authentification. Elles exposent les noms de dépôts privés, les numéros de PR, le payload complet des webhooks (corps des commentaires, clone_url), le markdown des reviews et le flux des appels d'outils de l'agent. Ce flux contient des extraits du code des dépôts, ce qui est sensible. Le service tourne sur0.0.0.0et le dashboard est servi par le même processus. Il faut protéger ces routes (token ou basic auth, ou reverse proxy authentifié), ou au moins les placer sur un port interne distinct du/webhook. Il n'y a aussi aucune limite sur le nombre de connexions SSE ouvertes.@@ -71,3 +158,1 @@metrics::webhook_channel_full(event_type);return Err(AppError::ChannelFullErr);}let job = match app_state.queue.push(wb).await {[bug] Le contrôle
is_knownpuisqueue.pushn'est pas atomique. Deux livraisons concurrentes du même événement (retry Gitea) passent toutes les deux le contrôle. La seconde échoue sur la contrainteUNIQUE(event_id)et renvoie 503Could not enqueue task, avec un faux événement Sentry et le compteurenqueue_failed. Gitea risque alors de re-livrer en boucle. Il faut traiter la violation d'unicité commeAlreadyProcessedErr(200), ou faire un insert avecON CONFLICT DO NOTHING.@@ -0,0 +218,4 @@iteration,name: name.to_string(),truncated,output: output.to_string(),[performance] Contrairement aux arguments (tronqués à 200 caractères),
outputest conservé en entier, jusqu'à 32 Ko par appel d'outil. L'historique garde 256 événements par job, soit environ 8 Mo par job actif. Chaque nouvel abonné SSE reçoit ensuite tout cet historique. Il faut tronquer l'aperçu envoyé au dashboard (comme pourMAX_EVENT_ARGUMENTS), ce qui limite aussi ce qui fuit sur une route publique.@@ -0,0 +59,4 @@pub async fn list(&self) -> toasty::Result<Vec<Review>> {let mut db = self.db.clone();let reviews = Review::all().exec(&mut db).await?;[performance]
list()charge toutes les reviews, y compris la colonnemarkdown, sans pagination ni tri, et/reviewsles renvoie toutes d'un coup. La table ne fait que grossir et chaque ouverture ou rafraîchissement du dashboard relit tout l'historique. Il faut une limite (par exemple les N dernières, triées parcompleted_at DESC), ou omettremarkdowndans la liste.@@ -0,0 +52,4 @@this._source = subscribe(endpoints.lifecycle, {onLifecycle: (event) => this.applyLifecycle(event),// En cas de gap (évènements ratés), on recharge l'état depuis le REST.onGap: () => this.refresh(),[bug] Le flux
/eventsreconnecte automatiquement (EventSource), mais les événements émis pendant la coupure (redémarrage serveur, réseau) sont perdus, sans événementgap. Le dashboard garde alors un état périmé (jobs fantômes dans Pending/In progress). Il faut brancheronOpenpour appelerrefresh()à chaque (re)connexion, y compris la première.@@ -0,0 +43,4 @@${job.event_id != null ? `<div><dt>Event</dt><dd>${job.event_id}</dd></div>` : ""}<div><dt>Created</dt><dd>${escapeHtml(relativeTime(job.created_at))}</dd></div></dl><button class="button button-secondary job-watch" type="button">Watch activity</button>[bug] Le bouton « Watch activity » est affiché aussi pour les jobs
pending. Or/jobs/{id}/eventsrenvoie 404 tant que le job n'a pas démarré (le canal n'est créé que parEventBus::start). Le panneau affiche alors « stream closed » sans explication. Il faut masquer ou désactiver le bouton pour les jobs en attente, ou ré-ouvrir le flux au passage enstarted.@@ -0,0 +51,4 @@this.append(card);}for (const card of known.values()) card.remove();[bug] Dans la branche « liste vide »,
render()ajoute un<p class="empty">, mais la branche non vide ne le retire jamais. Après la première apparition d'un job (liste vide puis job), le message « No pending job. » reste affiché au-dessus ou au-dessous des cartes. Il faut supprimer l'élément.emptydans la branche non vide, ou faire unreplaceChildrenavant de ré-attacher les cartes.