COL-BOT-05: stop /weather panicking on missing token or error payloads

Codex review: a missing/blank WEATHER_TOKEN and invalid locations produced an API
error body that then hit as_str().unwrap() calls. Store the token as
Option<String> (None when missing or blank) and disable /weather with a friendly
ephemeral reply instead of running with an empty key. get_weather now returns the
HTTP status with the parsed body; a non-2xx surfaces the API's error message, and
every success field is read via JSON pointers with no unwrap() on external JSON.
The thumbnail is only set when an icon URL is actually present.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

src/commands/weather.rs

@@ -2,17 +2,25 @@ use crate::{Context, Error};
 use poise::CreateReply;
 use serenity::{all::CreateEmbed, json::Value};
 
-async fn get_weather(location: String, weather_api_key: &str) -> Result<String, Error> {
+/// Fetch the raw weather response for a location. Returns the HTTP status
+/// alongside the parsed JSON body so the caller can tell a success payload from
+/// an API error payload (e.g. an unknown location) without unwrapping anything
+/// on external JSON. A body that is not valid JSON parses to `Value::Null`.
+async fn get_weather(
+    location: &str,
+    weather_api_key: &str,
+) -> Result<(reqwest::StatusCode, Value), Error> {
     let request_url = format!(
         "https://api.weatherapi.com/v1/current.json?key={key}&q={location}",
         key = weather_api_key,
         location = location
     );
 
-    // retrieve weather data
-    let response = reqwest::get(request_url).await?.text().await?;
-
-    Ok(response)
+    let response = reqwest::get(request_url).await?;
+    let status = response.status();
+    let body = response.text().await?;
+    let value: Value = serde_json::from_str(&body).unwrap_or(Value::Null);
+    Ok((status, value))
 }
 
 /// Shows weather information
@@ -21,33 +29,83 @@ pub async fn weather(
     ctx: Context<'_>,
     #[description = "Location (City or Country)"] location: String,
 ) -> Result<(), Error> {
-    let weather_response = get_weather(location, &ctx.data().state.weather_token).await?;
-    let value: Value = serde_json::from_str(&weather_response)?;
+    // Weather is optional. Without a configured token the command is disabled
+    // rather than panicking on a missing key.
+    let Some(weather_api_key) = ctx.data().state.weather_token.as_deref() else {
+        ctx.send(
+            CreateReply::default()
+                .content("The weather command is not configured on this bot.")
+                .ephemeral(true),
+        )
+        .await?;
+        return Ok(());
+    };
 
-    let location_name = value["location"]["name"].as_str().unwrap();
-    let location_region = value["location"]["region"].as_str().unwrap();
-    let location_country = value["location"]["country"].as_str().unwrap();
+    let (status, value) = get_weather(&location, weather_api_key).await?;
 
-    let weather_condition = value["current"]["condition"]["text"].as_str().unwrap();
-    let weather_temp = &value["current"]["temp_c"];
-    let weather_feels_like = &value["current"]["feelslike_c"];
-    let weather_wind_kph = &value["current"]["wind_kph"];
-    let weather_humidity = &value["current"]["humidity"];
-    let weather_cloud = &value["current"]["cloud"];
+    // weatherapi.com returns a JSON error body (unknown location, bad key, quota)
+    // with a non-2xx status. Surface a friendly message instead of unwrapping
+    // fields that are not present in an error payload.
+    if !status.is_success() {
+        let message = value
+            .pointer("/error/message")
+            .and_then(Value::as_str)
+            .unwrap_or("Could not fetch the weather for that location.");
+        ctx.send(
+            CreateReply::default()
+                .content(format!("Weather lookup failed: {message}"))
+                .ephemeral(true),
+        )
+        .await?;
+        return Ok(());
+    }
 
-    let weather_icon = value["current"]["condition"]["icon"].as_str().unwrap();
+    // Read every field defensively — external JSON is never unwrapped.
+    let text = |ptr: &str| -> String {
+        value
+            .pointer(ptr)
+            .and_then(Value::as_str)
+            .unwrap_or("Unknown")
+            .to_string()
+    };
+    let number = |ptr: &str| -> String {
+        match value.pointer(ptr) {
+            Some(v) if !v.is_null() => v.to_string(),
+            _ => "?".to_string(),
+        }
+    };
 
-    let embed = CreateEmbed::new()
-        .field("Name", location_name.to_string(), true)
-        .field("Region", location_region.to_string(), true)
-        .field("Country", location_country.to_string(), true)
-        .field("Condition", weather_condition.to_string(), true)
-        .field("Temperature", format!("{} °C", weather_temp), true)
-        .field("Feels like", format!("{} °C", weather_feels_like), true)
-        .field("Wind", format!("{} kph", weather_wind_kph), true)
-        .field("Humidity", format!("{}%", weather_humidity), true)
-        .field("Cloud", format!("{}%", weather_cloud), true)
-        .thumbnail(format!("https:{}", weather_icon));
+    let mut embed = CreateEmbed::new()
+        .field("Name", text("/location/name"), true)
+        .field("Region", text("/location/region"), true)
+        .field("Country", text("/location/country"), true)
+        .field("Condition", text("/current/condition/text"), true)
+        .field(
+            "Temperature",
+            format!("{} °C", number("/current/temp_c")),
+            true,
+        )
+        .field(
+            "Feels like",
+            format!("{} °C", number("/current/feelslike_c")),
+            true,
+        )
+        .field("Wind", format!("{} kph", number("/current/wind_kph")), true)
+        .field(
+            "Humidity",
+            format!("{}%", number("/current/humidity")),
+            true,
+        )
+        .field("Cloud", format!("{}%", number("/current/cloud")), true);
+
+    // Only set the thumbnail when the API actually returned an icon URL — a
+    // placeholder would produce an invalid "https:Unknown" URL that Discord rejects.
+    if let Some(icon) = value
+        .pointer("/current/condition/icon")
+        .and_then(Value::as_str)
+    {
+        embed = embed.thumbnail(format!("https:{icon}"));
+    }
 
     ctx.send(CreateReply::default().embed(embed)).await?;
     Ok(())

src/main.rs

@@ -27,8 +27,9 @@ pub struct AppState {
     pub verified_role_id: serenity::RoleId,
     pub logs_channel_id: serenity::ChannelId,
     // Optional: only /weather uses it. Read once here so no handler re-reads the
-    // process environment, but a missing value must not stop the bot from booting.
-    pub weather_token: String,
+    // process environment; `None` (missing or blank) simply disables /weather
+    // rather than stopping the bot from booting.
+    pub weather_token: Option<String>,
 }
 
 /// Read a required `u64` snowflake from the environment, recording the variable
@@ -67,8 +68,11 @@ impl AppState {
             std::process::exit(1);
         }
 
-        // Non-fatal: the bot boots without a weather key, /weather just fails.
-        let weather_token = std::env::var("WEATHER_TOKEN").unwrap_or_default();
+        // Non-fatal: the bot boots without a weather key; a missing or blank
+        // value becomes None and disables /weather rather than failing.
+        let weather_token = std::env::var("WEATHER_TOKEN")
+            .ok()
+            .filter(|t| !t.trim().is_empty());
 
         let supabase_url = std::env::var("SUPABASE_URL").expect("missing SUPABASE_URL");
         let supabase_key = std::env::var("SUPABASE_KEY").expect("missing SUPABASE_KEY");