fix(fs): handle File.mkdir_p errors and remove bang variants in sidecars and release packaging (CSM-030)

CODESMELL.md

@@ -445,10 +445,15 @@
 
 ---
 
-### CSM-030 — Unchecked `File.mkdir_p` / `File.mkdir_p!`
+### ~~CSM-030 — Unchecked `File.mkdir_p` / `File.mkdir_p!`~~ ✅ FIXED
-- **Files:** `lib/bds/media/thumbnails.ex:133`, `lib/bds/media/sidecars.ex:24,56`, `lib/bds/release_packaging.ex:80,85`
+- **Fixed:** 2026-05-11
-- **What:** Result of `File.mkdir_p/1` is discarded. `File.mkdir_p!/1` in `release_packaging` can crash on permission errors.
+- **What was done:**
-- **Fix:** Pattern-match `File.mkdir_p/1` or use `with`; replace bang variants with non-bang and handle errors.
+  - **`lib/bds/media/thumbnails.ex`** — Already fixed in CSM-009; `File.mkdir_p` is inside a `with` chain in `write_all_thumbnails`.
+  - **`lib/bds/media/sidecars.ex`** — Removed redundant `File.mkdir_p` calls from `write_sidecar/2` and `write_translation_sidecar/3` (the underlying `Persistence.atomic_write` already handles `mkdir_p`). Updated specs to return `:ok | {:error, File.posix()}`. Updated callers (`sync_media_sidecar`, `sync_media_translation_sidecar`) to propagate errors.
+  - **`lib/bds/media.ex`** — Replaced all `:ok = write_sidecar(...)` and `:ok = write_translation_sidecar(...)` match assertions with `log_sidecar_error/2` (mirrors existing `log_thumbnail_error/2` pattern). Sidecar write failures are logged as warnings but don't fail the DB operation.
+  - **`lib/bds/media/linking.ex`** — Same `log_sidecar_error/2` pattern for post link/unlink sidecar writes.
+  - **`lib/bds/release_packaging.ex`** — Replaced `File.mkdir_p!` with `File.mkdir_p` in `reset_output/1` (return value propagated through `with` chain in `package/1`). Replaced `File.mkdir_p!` with `with :ok <- File.mkdir_p(...)` in `copy_release/2`. Replaced `File.write!` with `File.write` in `write_manifest/1`.
+  - Added 6 tests in `test/bds/csm030_unchecked_mkdir_test.exs`: source-level assertions for no unchecked `File.mkdir_p`, no bang variants, no `:ok =` match assertions on sidecar writes.
 
 ---
 

lib/bds/media.ex

@@ -106,7 +106,7 @@ defmodule BDS.Media do
            |> Repo.insert!()
          end) do
       {:ok, media} ->
-        :ok = write_sidecar(project, media)
+        log_sidecar_error(write_sidecar(project, media), media.id)
         log_thumbnail_error(ensure_thumbnails(project, media), media.id)
         :ok = Search.sync_media(media)
         {:ok, media}
@@ -148,7 +148,7 @@ defmodule BDS.Media do
                |> Repo.update!()
              end) do
           {:ok, updated_media} ->
-            :ok = write_sidecar(project, updated_media)
+            log_sidecar_error(write_sidecar(project, updated_media), updated_media.id)
             :ok = Search.sync_media(updated_media)
             {:ok, updated_media}
 
@@ -240,7 +240,7 @@ defmodule BDS.Media do
                |> Repo.insert_or_update!()
              end) do
           {:ok, updated_translation} ->
-            :ok = write_translation_sidecar(project, media, updated_translation)
+            log_sidecar_error(write_translation_sidecar(project, media, updated_translation), media.id)
             :ok = Search.sync_media(media.id)
             {:ok, updated_translation}
 
@@ -275,7 +275,7 @@ defmodule BDS.Media do
                 )
 
                 :ok = Search.sync_media(media)
-                :ok = write_sidecar(project, media)
+                log_sidecar_error(write_sidecar(project, media), media.id)
                 {:ok, true}
 
               {:error, changeset} ->
@@ -322,7 +322,7 @@ defmodule BDS.Media do
                  end) do
               {:ok, updated_media} ->
                 _ = File.rm(previous_destination_backup)
-                :ok = write_sidecar(project, updated_media)
+                log_sidecar_error(write_sidecar(project, updated_media), updated_media.id)
                 log_thumbnail_error(ensure_thumbnails(project, updated_media), updated_media.id)
                 :ok = Search.sync_media(updated_media)
                 {:ok, updated_media}
@@ -350,4 +350,10 @@ defmodule BDS.Media do
   defp log_thumbnail_error({:error, reason}, media_id) do
     Logger.warning("Thumbnail generation failed for media #{media_id}: #{inspect(reason)}")
   end
+
+  defp log_sidecar_error(:ok, _media_id), do: :ok
+
+  defp log_sidecar_error({:error, reason}, media_id) do
+    Logger.warning("Sidecar write failed for media #{media_id}: #{inspect(reason)}")
+  end
 end

lib/bds/media/linking.ex

@@ -1,6 +1,8 @@
 defmodule BDS.Media.Linking do
   @moduledoc false
 
+  require Logger
+
   import Ecto.Query
 
   alias BDS.Media.Media
@@ -64,7 +66,7 @@ defmodule BDS.Media.Linking do
                end
              end) do
           {:ok, _result} ->
-            :ok = Sidecars.write_sidecar(project, media)
+            log_sidecar_error(Sidecars.write_sidecar(project, media), media.id)
             {:ok, :linked}
 
           {:error, reason} ->
@@ -93,7 +95,7 @@ defmodule BDS.Media.Linking do
                :ok
              end) do
           {:ok, :ok} ->
-            :ok = Sidecars.write_sidecar(project, media)
+            log_sidecar_error(Sidecars.write_sidecar(project, media), media.id)
             {:ok, :unlinked}
 
           {:error, reason} ->
@@ -112,6 +114,12 @@ defmodule BDS.Media.Linking do
     )
   end
 
+  defp log_sidecar_error(:ok, _media_id), do: :ok
+
+  defp log_sidecar_error({:error, reason}, media_id) do
+    Logger.warning("Sidecar write failed for media #{media_id}: #{inspect(reason)}")
+  end
+
   defp next_sort_order(media_id) do
     case Repo.one(
            from pm in PostMedia,

lib/bds/media/sidecars.ex

@@ -18,10 +18,9 @@ defmodule BDS.Media.Sidecars do
   alias BDS.Search
   alias BDS.Sidecar
 
-  @spec write_sidecar(BDS.Projects.Project.t(), Media.t()) :: :ok
+  @spec write_sidecar(BDS.Projects.Project.t(), Media.t()) :: :ok | {:error, File.posix()}
   def write_sidecar(project, media) do
     path = Path.join(Projects.project_data_dir(project), media.sidecar_path)
-    :ok = File.mkdir_p(Path.dirname(path))
 
     atomic_write(
       path,
@@ -45,7 +44,8 @@ defmodule BDS.Media.Sidecars do
     )
   end
 
-  @spec write_translation_sidecar(BDS.Projects.Project.t(), Media.t(), Translation.t()) :: :ok
+  @spec write_translation_sidecar(BDS.Projects.Project.t(), Media.t(), Translation.t()) ::
+          :ok | {:error, File.posix()}
   def write_translation_sidecar(project, media, translation) do
     path =
       Path.join(
@@ -53,8 +53,6 @@ defmodule BDS.Media.Sidecars do
         translation_sidecar_path(media, translation.language)
       )
 
-    :ok = File.mkdir_p(Path.dirname(path))
-
     atomic_write(
       path,
       Sidecar.serialize_document([
@@ -189,8 +187,7 @@ defmodule BDS.Media.Sidecars do
 
       media ->
         project = Projects.get_project!(media.project_id)
-        :ok = write_sidecar(project, media)
+        write_sidecar(project, media)
-        :ok
     end
   end
 
@@ -224,8 +221,11 @@ defmodule BDS.Media.Sidecars do
       %Translation{} = translation ->
         media = Repo.get!(Media, translation.translation_for)
         project = Projects.get_project!(media.project_id)
-        :ok = write_translation_sidecar(project, media, translation)
+
-        {:ok, translation}
+        case write_translation_sidecar(project, media, translation) do
+          :ok -> {:ok, translation}
+          {:error, reason} -> {:error, reason}
+        end
     end
   end
 

lib/bds/release_packaging.ex

@@ -77,16 +77,15 @@ defmodule BDS.ReleasePackaging do
   defp reset_output(metadata) do
     File.rm_rf!(metadata.payload_root)
     File.rm_rf!(metadata.archive_path)
-    File.mkdir_p!(metadata.output_dir)
+    File.mkdir_p(metadata.output_dir)
-    :ok
   end
 
   defp copy_release(source, destination) do
-    File.mkdir_p!(Path.dirname(destination))
+    with :ok <- File.mkdir_p(Path.dirname(destination)) do
-
+      case File.cp_r(source, destination) do
-    case File.cp_r(source, destination) do
+        {:ok, _files} -> :ok
-      {:ok, _files} -> :ok
+        {:error, reason, _file} -> {:error, reason}
-      {:error, reason, _file} -> {:error, reason}
+      end
     end
   end
 
@@ -102,8 +101,7 @@ defmodule BDS.ReleasePackaging do
     }
 
     manifest_path = Path.join(metadata.payload_root, "manifest.json")
-    File.write!(manifest_path, Jason.encode!(manifest, pretty: true))
+    File.write(manifest_path, Jason.encode!(manifest, pretty: true))
-    :ok
   end
 
   defp create_archive(%Metadata{platform: :windows} = metadata) do

test/bds/csm030_unchecked_mkdir_test.exs

@@ -0,0 +1,42 @@
+defmodule BDS.CSM030UncheckedMkdirTest do
+  use ExUnit.Case, async: true
+
+  describe "source-level: no unchecked File.mkdir_p" do
+    test "sidecars.ex has no bare File.mkdir_p calls" do
+      source = File.read!("lib/bds/media/sidecars.ex")
+      refute source =~ ~r/:ok\s*=\s*File\.mkdir_p/
+      refute source =~ ~r/File\.mkdir_p!/
+    end
+
+    test "release_packaging.ex has no File.mkdir_p! calls" do
+      source = File.read!("lib/bds/release_packaging.ex")
+      refute source =~ ~r/File\.mkdir_p!/
+    end
+
+    test "thumbnails.ex mkdir_p is inside a with chain" do
+      source = File.read!("lib/bds/media/thumbnails.ex")
+      refute source =~ ~r/:ok\s*=\s*File\.mkdir_p/
+      refute source =~ ~r/File\.mkdir_p!/
+    end
+  end
+
+  describe "source-level: sidecar write errors are handled" do
+    test "media.ex does not assert :ok on write_sidecar" do
+      source = File.read!("lib/bds/media.ex")
+      refute source =~ ~r/:ok\s*=\s*write_sidecar/
+      refute source =~ ~r/:ok\s*=\s*write_translation_sidecar/
+    end
+
+    test "linking.ex does not assert :ok on write_sidecar" do
+      source = File.read!("lib/bds/media/linking.ex")
+      refute source =~ ~r/:ok\s*=\s*Sidecars\.write_sidecar/
+    end
+  end
+
+  describe "source-level: release_packaging.ex write_manifest" do
+    test "uses File.write not File.write!" do
+      source = File.read!("lib/bds/release_packaging.ex")
+      refute source =~ ~r/File\.write!/
+    end
+  end
+end