SEC-01: pull-request CI on a hosted runner, and stop the deploy leaving DOT_ENV on the VPS (#5)

* SEC-01: add pull-request CI and stop the deploy leaving DOT_ENV on the VPS

A merge to main runs the merged commit as root on the club VPS through the
self-hosted runner, and this repo has no PR CI, no required review and no
status check.

- .github/workflows/ci.yml: fmt / clippy / build --locked / test on
  ubuntu-latest, contents: read, both actions pinned by commit SHA. Never
  the VPS.
- deploy.yml: keep push to main, add workflow_dispatch, permissions
  contents: read, environment: production, and an if: always() step that
  removes the .env the deploy writes into the workspace.
- .dockerignore: keep that .env, target/ and .git out of the build context.
- Dockerfile: --locked on both cargo build --release lines.

environment: production gates nothing until required reviewers are configured
on the environment itself, and the ruleset still requires zero approving
reviews. Both are owner-only and listed in the pull request.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* SEC-01: make cargo fmt and clippy clean so the CI job can go green

The CI workflow added in #5 fails on code that predates it: three files were
unformatted and `cargo clippy --all-targets -- -D warnings` reported 26 errors.
A red check cannot be made a required status check on the protect-main ruleset,
which is what this unblocks.

`cargo fmt --all` over three files, and 26 clippy errors resolved: 18 via
`cargo clippy --all-targets --fix`, the rest by hand. Two fixes uncovered lints
that had been masked (an `unnecessary_unwrap` in info.rs behind the needless
borrow on the line above, and two `unnecessary_to_owned` at the call sites of a
signature that moved from `&String` to `&str`), so 28 fixes for 26 warnings.

Three `#[allow]`s where the only real fix would change a signature or a public
API: `result_large_err` on `AppState::new` (the large variant is
`supabase::Error`, owned by supabase-lib-rs) and `too_many_arguments` on
`log_embed` and on the `embed` slash command.

No behaviour change. src/ only; .github/, Dockerfile, .dockerignore, Cargo.toml
and Cargo.lock are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* SEC-01: pin actions/checkout in the deploy workflow to a commit SHA

deploy.yml runs on the self-hosted VPS runner with passwordless sudo, so a
mutable tag on this action is a code-execution path onto that host if the tag
is ever moved. Pinned to the commit v4 currently resolves to
(11d5960a326750d5838078e36cf38b85af677262), verified against upstream — this is
the same code the deploy already runs today, not a version bump. ci.yml is on
v7.0.1; the deploy path is deliberately left on v4 so that pinning does not
smuggle a major-version change into a workflow whose only test is a live deploy.

Raised by Codex review of PR #5 as the one unpinned `uses:` in either workflow.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

.dockerignore

@@ -0,0 +1,9 @@
+# Keep the docker build context to what the Dockerfile actually COPYs
+# (Cargo.toml, Cargo.lock, src). deploy.yml writes the DOT_ENV secret to .env in
+# the workspace before running `docker compose up --build`; without this file
+# that secret is tarred into the build context and handed to the docker daemon
+# on every deploy.
+.env
+.env.*
+target/
+.git

.github/workflows/ci.yml

@@ -0,0 +1,82 @@
+# Format, lint, build and test on every pull request.
+#
+# This must run on a GitHub-hosted runner and never on the club VPS. The runner
+# deploy.yml uses lives on the club's own server and executes whatever a workflow
+# tells it to, as root, so nothing triggered by a pull request may be pointed at
+# it. Read that as a rule for this file, not a property of the repository: this
+# repo is public, forking is on, and fork pull requests only need approval from a
+# first-time contributor, so a fork that brings its own workflow can still reach
+# that runner. See SEC-01.
+#
+# SEC-01: point the protect-main ruleset's required status check at this job's
+# name ("fmt / clippy / build / test") so a red build blocks the merge that
+# deploys.
+
+name: CI
+
+on:
+  pull_request:
+
+# A newer push to the same pull request makes an in-flight run irrelevant.
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  check:
+    name: fmt / clippy / build / test
+    runs-on: ubuntu-latest
+    # A cold build of this dependency tree (serenity, reqwest, image, exr) in
+    # release, then clippy, then test, is slow on a 4-vCPU hosted runner. This is
+    # a guard against a hung job, not a target.
+    timeout-minutes: 45
+
+    steps:
+      # Third-party and first-party actions alike are pinned to a full commit
+      # SHA: a tag is mutable and can be repointed at new code by whoever owns
+      # the action.
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+
+      # Caches written by a pull_request run are scoped to that pull request, so
+      # this pays off across pushes to the same branch rather than across branches.
+      - name: Cache cargo registry and build artifacts
+        uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+        with:
+          path: |
+            ~/.cargo/registry
+            ~/.cargo/git
+            target
+          key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-
+
+      # actions/runner-images documents rustfmt for ubuntu-24.04 but not clippy (it
+      # lists clippy only under ubuntu-22.04), while the image actually in use does
+      # ship it. Adding both components is a no-op when they are present and keeps
+      # this job working if that undocumented extra ever goes away. Print the
+      # versions too: clippy's lint set moves between releases, so knowing which one
+      # ran is what explains a lint that appeared from nowhere.
+      - name: Toolchain
+        run: |
+          rustup component add clippy rustfmt
+          cargo --version
+          cargo fmt --version
+          cargo clippy --version
+
+      - name: Format
+        run: cargo fmt --all -- --check
+
+      # Kept ahead of clippy and test: those resolve dependencies and would
+      # refresh Cargo.lock in place, so a stale lockfile would slip past
+      # --locked if they ran first.
+      - name: Build
+        run: cargo build --locked --release
+
+      - name: Clippy
+        run: cargo clippy --all-targets -- -D warnings
+
+      - name: Test
+        run: cargo test

.github/workflows/deploy.yml

@@ -4,14 +4,23 @@ on:
   push:
     branches:
       - main
+  # Manual re-run, for redeploying after a rollback or re-creating the
+  # workspace .env that the cleanup step below now removes.
+  workflow_dispatch:
+
+permissions:
+  contents: read
 
 jobs:
   build_and_deploy:
     runs-on: [self-hosted, linux]
+    # SEC-01: this gates nothing until required reviewers are configured on the
+    # "production" environment itself. See the pull request description.
+    environment: production
 
     steps:
       - name: Checkout code
-        uses: actions/checkout@v4
+        uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
         with:
           clean: true
           fetch-depth: 1
@@ -35,5 +44,10 @@ jobs:
           sudo docker compose up -d --build --force-recreate
           echo "Deployment complete."
 
+      - name: Remove environment file
+        if: always()
+        working-directory: ${{ github.workspace }}
+        run: rm -f .env
+
       - name: Clean up old Docker images
         run: sudo docker image prune -f

Dockerfile

@@ -17,14 +17,14 @@ RUN mkdir src && \
     echo 'fn main() { println!("Dummy main for dependency caching"); }' > src/main.rs
 
 # Build dependencies (this layer will be cached unless Cargo.toml/Cargo.lock changes)
-RUN cargo build --release && \
+RUN cargo build --locked --release && \
     rm -rf src target/release/deps/dsec_bot*
 
 # Copy the actual source code
 COPY src ./src
 
 # Build the actual application
-RUN cargo build --release
+RUN cargo build --locked --release
 
 # Runtime stage - use a minimal image
 FROM debian:bookworm-slim

src/commands/info.rs

@@ -151,10 +151,10 @@ pub async fn serverinfo(ctx: Context<'_>) -> Result<(), Error> {
     let server_description = server_description_option.as_deref().unwrap_or("N/A");
 
     // rules channel, if empty N/A
-    let rules_channel = if (&partial_guild.rules_channel_id).is_none() {
-        "N/A"
+    let rules_channel = if let Some(rules_channel_id) = partial_guild.rules_channel_id {
+        &format!("<#{}>", rules_channel_id)
     } else {
-        &format!("<#{}>", &partial_guild.rules_channel_id.unwrap())
+        "N/A"
     };
 
     let embed_footer = CreateEmbedFooter::new(format!("ID: {}", server_id));

src/commands/member_info.rs

@@ -18,7 +18,7 @@ struct DiscordLinkRow {
     student_id: String,
 }
 
-async fn member_data(database: &Database, user_id: &String) -> Result<Option<MemberRow>, Error> {
+async fn member_data(database: &Database, user_id: &str) -> Result<Option<MemberRow>, Error> {
     let rows: Vec<MemberRow> = database
         .from("active_members")
         .select("student_id, full_name, campus, membership_status, end_date")
@@ -30,7 +30,7 @@ async fn member_data(database: &Database, user_id: &String) -> Result<Option<Mem
 }
 
 /// Look up the student id linked to a given discord user, if any.
-async fn recorded_member(database: &Database, user_id: &String) -> Result<Option<String>, Error> {
+async fn recorded_member(database: &Database, user_id: &str) -> Result<Option<String>, Error> {
     let rows: Vec<DiscordLinkRow> = database
         .from("dsec_discord_members")
         .select("student_id")
@@ -66,15 +66,7 @@ pub async fn member_info(ctx: ApplicationContext<'_>) -> Result<(), Error> {
 
     let user_data_option = member_data(&database, &student_id).await?;
 
-    if user_data_option.is_none() {
-        ctx.send(CreateReply::default().embed(
-            CreateEmbed::new().title("Couldn't find info").description(
-                "Your membership may have expired. Contact a club executive to be sure.",
-            ),
-        ))
-        .await?;
-    } else {
-        let user_data = user_data_option.expect("Member not found");
+    if let Some(user_data) = user_data_option {
         let member_name = user_data.full_name;
         let member_campus = user_data.campus;
         let membership_status = user_data.membership_status;
@@ -97,6 +89,13 @@ pub async fn member_info(ctx: ApplicationContext<'_>) -> Result<(), Error> {
                 .ephemeral(true),
         )
         .await?;
+    } else {
+        ctx.send(CreateReply::default().embed(
+            CreateEmbed::new().title("Couldn't find info").description(
+                "Your membership may have expired. Contact a club executive to be sure.",
+            ),
+        ))
+        .await?;
     }
 
     // let student_data = state

src/commands/mods_only.rs

@@ -1,8 +1,11 @@
 use crate::{Context, Error};
 use dotenv::dotenv;
-use poise::{serenity_prelude as serenity, CreateReply};
+use poise::{CreateReply, serenity_prelude as serenity};
 
 /// Send message to logs channel
+// Each argument is one optional embed field; collapsing them into a struct would
+// change this function's signature and every call site, which this PR does not do.
+#[allow(clippy::too_many_arguments)]
 pub async fn log_embed(
     ctx: &serenity::Context,
     title: Option<String>,
@@ -36,10 +39,10 @@ pub async fn log_embed(
     }
 
     // Set color (parse hex color)
-    if let Some(color_str) = colour {
-        if let Ok(color_value) = u32::from_str_radix(color_str.trim_start_matches('#'), 16) {
-            embed = embed.color(color_value);
-        }
+    if let Some(color_str) = colour
+        && let Ok(color_value) = u32::from_str_radix(color_str.trim_start_matches('#'), 16)
+    {
+        embed = embed.color(color_value);
     }
 
     // Set thumbnail
@@ -77,6 +80,9 @@ pub async fn log_embed(
     slash_command,
     required_permissions = "MANAGE_MESSAGES | MANAGE_THREADS"
 )]
+// These arguments are the slash command's options as Discord presents them;
+// bundling them into a struct would change the command's public interface.
+#[allow(clippy::too_many_arguments)]
 pub async fn embed(
     ctx: Context<'_>,
     #[description = "Title of embed"] title: Option<String>,
@@ -109,10 +115,10 @@ pub async fn embed(
     }
 
     // Set color (parse hex color)
-    if let Some(color_str) = colour {
-        if let Ok(color_value) = u32::from_str_radix(color_str.trim_start_matches('#'), 16) {
-            embed = embed.color(color_value);
-        }
+    if let Some(color_str) = colour
+        && let Ok(color_value) = u32::from_str_radix(color_str.trim_start_matches('#'), 16)
+    {
+        embed = embed.color(color_value);
     }
 
     // Set thumbnail

src/commands/weather.rs

@@ -26,24 +26,24 @@ pub async fn weather(
     let weather_response = get_weather(location).await?;
     let value: Value = serde_json::from_str(&weather_response)?;
 
-    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 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 weather_condition = (&value["current"]["condition"]["text"]).as_str().unwrap();
+    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"];
 
-    let weather_icon = (&value["current"]["condition"]["icon"]).as_str().unwrap();
+    let weather_icon = value["current"]["condition"]["icon"].as_str().unwrap();
 
     let embed = CreateEmbed::new()
-        .field("Name", format!("{}", location_name), true)
-        .field("Region", format!("{}", location_region), true)
-        .field("Country", format!("{}", location_country), true)
-        .field("Condition", format!("{}", weather_condition), true)
+        .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)

src/events.rs

@@ -1,3 +1,3 @@
-pub mod ready;
 pub mod interaction_create;
 pub mod message;
+pub mod ready;

src/events/interaction_create.rs

@@ -115,7 +115,7 @@ async fn grant_verified_role(
     data: &Data,
     modal_submit: &ModalInteraction,
     discord_member: &Member,
-    student_id: &String,
+    student_id: &str,
     verified_role_id: RoleId,
     via_cache: bool,
 ) -> Result<(), Error> {
@@ -170,7 +170,7 @@ async fn fetch_student(data: &Data, student_id: &str) -> Result<Option<StudentRo
     Ok(student_data.into_iter().next())
 }
 
-async fn member_recorded(data: &Data, user_id: &String) -> Result<bool, Error> {
+async fn member_recorded(data: &Data, user_id: &str) -> Result<bool, Error> {
     let rows: Vec<serde_json::Value> = data
         .state
         .supabase
@@ -210,18 +210,18 @@ async fn handle_verify(
     // already attached to the button interaction. Anything slower than this
     // (a DB query, a member fetch) must NOT run before the modal is shown, or
     // Discord's ~3s acknowledgement window elapses and the click fails.
-    if let Some(member) = &component_interaction.member {
-        if member.roles.contains(&verified_role_id) {
-            component_interaction
-                .create_response(
-                    ctx,
-                    ephemeral_embed(CreateEmbed::new().title("Already Verified ✅").description(
-                        format!("You already have the <@&{}> role!", verified_role_id),
-                    )),
-                )
-                .await?;
-            return Ok(());
-        }
+    if let Some(member) = &component_interaction.member
+        && member.roles.contains(&verified_role_id)
+    {
+        component_interaction
+            .create_response(
+                ctx,
+                ephemeral_embed(CreateEmbed::new().title("Already Verified ✅").description(
+                    format!("You already have the <@&{}> role!", verified_role_id),
+                )),
+            )
+            .await?;
+        return Ok(());
     }
 
     // Respond to the click with the modal immediately.
@@ -247,7 +247,7 @@ async fn handle_verify(
             data,
             &modal_submit,
             &discord_member,
-            &student_id.to_string(),
+            student_id,
             verified_role_id,
             true,
         )
@@ -277,7 +277,7 @@ async fn handle_verify(
             data,
             &modal_submit,
             &discord_member,
-            &student_id.to_string(),
+            student_id,
             verified_role_id,
             false,
         )

src/events/message.rs

@@ -65,7 +65,7 @@ async fn create_leetcode_thread(
         fn create_title_from_message(message: impl Into<String>) -> String {
             let message_string: String = message.into();
 
-            let title = message_string
+            message_string
                 .lines()
                 .next()
                 .map(|line| {
@@ -77,8 +77,7 @@ async fn create_leetcode_thread(
                     &line[start..]
                 })
                 .unwrap_or("")
-                .to_string();
-            title
+                .to_string()
         }
 
         let new_thread =
@@ -95,7 +94,7 @@ pub async fn on_message(
     ctx: &serenity::Context,
     new_message: &serenity::Message,
 ) -> Result<(), Error> {
-    let _ = honeypot(ctx, new_message).await?;
-    let _ = create_leetcode_thread(ctx, new_message).await?;
+    honeypot(ctx, new_message).await?;
+    create_leetcode_thread(ctx, new_message).await?;
     Ok(())
 }

src/main.rs

@@ -22,6 +22,9 @@ pub struct AppState {
 }
 
 impl AppState {
+    // The large `Err` variant is `supabase::Error` from the `supabase-lib-rs` crate;
+    // boxing it would change this public signature rather than shrink their type.
+    #[allow(clippy::result_large_err)]
     pub async fn new() -> supabase::Result<Self> {
         dotenv().ok();
         let supabase_url = std::env::var("SUPABASE_URL").expect("missing SUPABASE_URL");
@@ -42,9 +45,7 @@ impl AppState {
                 None => println!("User not found"),
             },
             Err(err) => {
-                eprintln!(
-                    "Failed to connect/sign in to Supabase, continuing setup anyways: {err}"
-                );
+                eprintln!("Failed to connect/sign in to Supabase, continuing setup anyways: {err}");
             }
         }
 
@@ -66,7 +67,7 @@ async fn event_handler(
             events::ready::on_ready(ctx, data_about_bot).await?;
         }
         serenity::FullEvent::InteractionCreate { interaction } => {
-            events::interaction_create::on_interaction_create(ctx, interaction, &data).await?;
+            events::interaction_create::on_interaction_create(ctx, interaction, data).await?;
         }
         serenity::FullEvent::Message { new_message } => {
             events::message::on_message(ctx, new_message).await?;