qpismont/herald
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Fix missing env var error #5

Merged
qpismont merged 4 commits from 1.0.1 into main 2026-06-16 22:05:25 +02:00
Conversation 5 Commits 4 Files Changed 7
+32 -22
7 changed files with 32 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.env.example
+1
View File
@@ -16,3 +16,4 @@ GITEA_TIMEOUT=30
# Optional
SENTRY_DSN=
RUST_LOG=info
RUST_BACKTRACE=1
Cargo.lock
Generated
+1 -1
View File
@@ -786,7 +786,7 @@ checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a"
[[package]]
name = "herald"
version = "1.0.0"
version = "1.0.1"
dependencies = [
"anyhow",
"axum",
Cargo.toml
+6 -3
View File
@@ -1,8 +1,11 @@
[package]
name = "herald"
version = "1.0.0"
version = "1.0.1"
edition = "2024"
[profile.release]
debug = 1
qpismont marked this conversation as resolved
Herald commented 2026-06-16 20:54:23 +02:00
Review
Copy Link
Copy Source

L'ajout d'informations de débogage (debug = 1) dans le profil release augmente la taille du binaire. Si cela est nécessaire pour le diagnostic en production (par exemple pour les backtraces), c'est acceptable. Sinon, il serait préférable de ne l'activer que pour un profil release-with-debug dédié ou d'ajouter un commentaire expliquant la décision.

L'ajout d'informations de débogage (`debug = 1`) dans le profil `release` augmente la taille du binaire. Si cela est nécessaire pour le diagnostic en production (par exemple pour les backtraces), c'est acceptable. Sinon, il serait préférable de ne l'activer que pour un profil `release-with-debug` dédié ou d'ajouter un commentaire expliquant la décision.
[dependencies]
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
tokio = { version = "1.52", features = ["full"] }
@@ -12,7 +15,7 @@ futures-util = "0.3"
serde_json = "1.0"
serde = { version = "1.0", features = ["derive"] }
sentry = { version = "0.48", features = ["tower-axum-matched-path"] }
sentry-anyhow = "0.48"
sentry-anyhow = { version = "0.48", features = ["backtrace"] }
openrouter-rs = "0.10"
dotenvy = "0.15"
tower = "0.5"
@@ -20,7 +23,7 @@ tower-http = {version = "0.6", features = ["trace"] }
tracing = "0.1"
tracing-subscriber = { version = "0.3", features=["env-filter"] }
axum = "0.8"
anyhow = "1.0"
anyhow = { version = "1.0", features = ["backtrace"] }
thiserror = "2.0"
hmac = "0.13"
sha2 = "0.11"
src/bot.rs
+8 -2
View File
@@ -97,9 +97,15 @@ impl Bot {
Ok(())
}
#[instrument(skip(self))]
#[instrument(skip(self, webhook), fields(repo, pr))]
pub async fn exec(&self, webhook: WebhookType) {
tracing::Span::current().record("webhook_type", tracing::field::debug(&webhook));
match &webhook {
WebhookType::Review(p) => {
tracing::Span::current().record("repo", &p.repository.full_name);
tracing::Span::current().record("pr", p.pull_request.number);
}
};
let exec_result = match webhook {
WebhookType::Review(review_payload) => crate::bot_actions::review::exec_review(
&self.gitea_api,
src/bot_actions/review.rs
+1 -1
View File
@@ -10,7 +10,7 @@ use crate::{
open_router::OpenRouterClient,
};
#[instrument(skip(gitea_api, open_router_client, http_client, review_payload), err)]
#[instrument(skip(gitea_api, open_router_client, http_client, review_payload))]
pub async fn exec_review(
gitea_api: &GiteaAPI,
open_router_client: &OpenRouterClient,
src/env.rs
+3 -3
View File
@@ -41,13 +41,13 @@ pub fn load_config() -> anyhow::Result<EnvConfig> {
}
pub fn try_get_env(key: &str) -> anyhow::Result<String> {
let env = std::env::var(key)?;
let env_value = std::env::var(key).map_err(|e| anyhow::anyhow!("{}: {}", key, e))?;
qpismont marked this conversation as resolved Outdated
Herald commented 2026-06-12 23:45:03 +02:00
Outdated
Review
Copy Link
Copy Source

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.

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 2026-06-12 23:45:03 +02:00
Outdated
Review
Copy Link
Copy Source

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.

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 2026-06-16 20:54:23 +02:00
Review
Copy Link
Copy Source

Cette transformation en anyhow::Error est claire et appropriée. Bonne pratique pour la propagation d'erreur.

Cette transformation en `anyhow::Error` est claire et appropriée. Bonne pratique pour la propagation d'erreur.
if env.trim().is_empty() {
if env_value.trim().is_empty() {
qpismont marked this conversation as resolved
Herald commented 2026-06-16 19:11:21 +02:00
Review
Copy Link
Copy Source

Ce bloc if ne fait rien si la valeur est vide. Il semble que vous souhaitiez traiter une variable d'environnement vide comme une erreur, mais le code continue et retourne Ok(env_value) à la ligne 50. Vous devriez ajouter une instruction de retour d'erreur (par exemple avec bail! ou return Err(...)) à l'intérieur du bloc if, ou bien utiliser une structure différente pour gérer le cas vide.

Ce bloc `if` ne fait rien si la valeur est vide. Il semble que vous souhaitiez traiter une variable d'environnement vide comme une erreur, mais le code continue et retourne `Ok(env_value)` à la ligne 50. Vous devriez ajouter une instruction de retour d'erreur (par exemple avec `bail!` ou `return Err(...)`) à l'intérieur du bloc `if`, ou bien utiliser une structure différente pour gérer le cas vide.
Herald commented 2026-06-16 20:54:23 +02:00
Review
Copy Link
Copy Source

Après avoir vérifié que la variable d'environnement est vide ou ne contient que des espaces, il n'y a pas de traitement visible (pas de return Err(...) ni de panic!). Si l'intention est de rejeter les valeurs vides, il faut ajouter une gestion d'erreur explicite, par exemple return Err(anyhow::anyhow!("{}: variable vide", key)). Sinon, la fonction continue avec une valeur vide et échouera peut-être plus tard de manière obscure.

Après avoir vérifié que la variable d'environnement est vide ou ne contient que des espaces, il n'y a pas de traitement visible (pas de `return Err(...)` ni de `panic!`). Si l'intention est de rejeter les valeurs vides, il faut ajouter une gestion d'erreur explicite, par exemple `return Err(anyhow::anyhow!("{}: variable vide", key))`. Sinon, la fonction continue avec une valeur vide et échouera peut-être plus tard de manière obscure.
return Err(anyhow!(format!("env var {} is empty", key)));
}
Ok(env)
Ok(env_value)
}
#[cfg(test)]
src/gitea.rs
+12 -12
View File
@@ -29,7 +29,7 @@ impl GiteaAPI {
})
}
#[instrument(skip(self), err)]
#[instrument(skip(self))]
pub async fn comment(
&self,
body: &str,
@@ -57,7 +57,7 @@ impl GiteaAPI {
res.json::<Comment>().await.map_err(anyhow::Error::from)
}
#[instrument(skip(self), err)]
#[instrument(skip(self))]
pub async fn edit_comment(
&self,
body: &str,
@@ -85,7 +85,7 @@ impl GiteaAPI {
Ok(())
}
#[instrument(skip(self), err)]
#[instrument(skip(self))]
pub async fn delete_comment(&self, full_name: &str, comment_id: u64) -> anyhow::Result<()> {
let url = format!(
"{}/api/v1/repos/{}/issues/comments/{}",
@@ -104,7 +104,7 @@ impl GiteaAPI {
Ok(())
}
#[instrument(skip(self, review_result), err)]
#[instrument(skip(self, review_result))]
pub async fn post_pull_request_review(
&self,
review_result: &ReviewResult,
@@ -206,14 +206,6 @@ impl WebhookType {
_ => Err(AppError::UnknownEventErr),
}?;
let pr_body = match &wb {
WebhookType::Review(review_payload) => &review_payload.comment.body,
};
if !pr_body.starts_with(&format!("@{}", bot_name)) {
return Err(AppError::UnauthorizedUserErr);
}
let action = match &wb {
WebhookType::Review(review_payload) => &review_payload.action,
};
@@ -222,6 +214,14 @@ impl WebhookType {
return Err(AppError::InvalidActionErr);
}
let pr_body = match &wb {
qpismont marked this conversation as resolved
Herald commented 2026-06-16 20:54:23 +02:00
Review
Copy Link
Copy Source

Le motif match ne gère que la variante WebhookType::Review. Si une autre variante (par exemple WebhookType::PullRequest) est reçue, cela provoquera une panique. Il faut soit étendre le match à toutes les variantes, soit s'assurer que cette fonction n'est appelée que pour des événements Review (par exemple en déplaçant la logique dans le bloc if let).

Le motif `match` ne gère que la variante `WebhookType::Review`. Si une autre variante (par exemple `WebhookType::PullRequest`) est reçue, cela provoquera une panique. Il faut soit étendre le match à toutes les variantes, soit s'assurer que cette fonction n'est appelée que pour des événements `Review` (par exemple en déplaçant la logique dans le bloc `if let`).
WebhookType::Review(review_payload) => &review_payload.comment.body,
qpismont marked this conversation as resolved
Herald commented 2026-06-16 19:11:21 +02:00
Review
Copy Link
Copy Source

L'extraction du corps du commentaire via &review_payload.comment.body suppose que comment et body existent toujours. Si comment est optionnel ou que body peut être None, cela pourrait causer une panique. Envisagez d'utiliser une approche plus sécurisée avec des if let ou pattern matching pour gérer les valeurs manquantes.

L'extraction du corps du commentaire via `&review_payload.comment.body` suppose que `comment` et `body` existent toujours. Si `comment` est optionnel ou que `body` peut être `None`, cela pourrait causer une panique. Envisagez d'utiliser une approche plus sécurisée avec des `if let` ou pattern matching pour gérer les valeurs manquantes.
};
if !pr_body.starts_with(&format!("@{}", bot_name)) {
qpismont marked this conversation as resolved
Herald commented 2026-06-16 20:54:23 +02:00
Review
Copy Link
Copy Source

La variable bot_name n'est pas définie dans le contexte visible. Assurez-vous qu'elle est passée en paramètre ou accessible dans la portée, sinon cela causera une erreur de compilation.

La variable `bot_name` n'est pas définie dans le contexte visible. Assurez-vous qu'elle est passée en paramètre ou accessible dans la portée, sinon cela causera une erreur de compilation.
return Err(AppError::UnauthorizedUserErr);
}
Ok(wb)
}
}