fix(safety): replace File.read! with File.read and error-tuple handling in preview_assets and templates (CSM-034)

CODESMELL.md

@@ -517,9 +517,14 @@
 
 ---
 
-### CSM-034 — `File.read!` / `File.write!` Without Error Handling
-- **Files:** `lib/bds/preview_assets.ex:32`, `lib/bds/release_packaging.ex:105`, `lib/bds/templates.ex:488-489`
-- **Fix:** Use `File.read/1`, `File.write/2`, and handle `{:error, reason}`.
+### ~~CSM-034 — `File.read!` / `File.write!` Without Error Handling~~ ✅ FIXED
+- **Fixed:** 2026-05-27
+- **What was done:**
+  - **`lib/bds/preview_assets.ex`** — `generated_outputs/0`: Replaced `File.read!` with `File.read` inside `Enum.flat_map`, silently skipping files that become unreadable between `Path.wildcard` and read (TOCTOU race).
+  - **`lib/bds/templates.ex`** — `upsert_template_from_file/3`: Replaced `File.read!` and pattern-matched `Frontmatter.parse_document` with a `with` chain returning `{:ok, template} | {:error, reason}`. Replaced `Repo.insert_or_update!` with `Repo.insert_or_update` to propagate changeset errors.
+  - **`lib/bds/templates.ex`** — Updated all three callers: `rebuild_templates_from_files` logs a warning and skips bad files, `sync_template_from_file` and `import_orphan_template_file` map errors to `{:error, :not_found}`.
+  - **`lib/bds/release_packaging.ex`** — Already fixed by CSM-030 (`File.write!` → `File.write`).
+  - Added 8 tests in `test/bds/csm034_file_read_bang_test.exs`: source-level assertions for no bang file ops in all three files, functional tests for rebuild skipping bad templates, sync returning error on deleted file, import returning error on missing file, and preview_assets returning valid output tuples.
 
 ---
 

lib/bds/preview_assets.ex

@@ -28,8 +28,11 @@ defmodule BDS.PreviewAssets do
     end)
     |> Enum.filter(&File.regular?/1)
     |> Enum.sort()
-    |> Enum.map(fn path ->
-      {Path.relative_to(path, @preview_root), File.read!(path)}
+    |> Enum.flat_map(fn path ->
+      case File.read(path) do
+        {:ok, contents} -> [{Path.relative_to(path, @preview_root), contents}]
+        {:error, _reason} -> []
+      end
     end)
   end
 

lib/bds/templates.ex

@@ -4,6 +4,8 @@ defmodule BDS.Templates do
   including slug derivation, status transitions, and filesystem synchronization.
   """
 
+  require Logger
+
   import Ecto.Query
   import BDS.MapUtils, only: [attr: 2, maybe_put: 3]
 
@@ -184,10 +186,18 @@ defmodule BDS.Templates do
     templates =
       template_paths
       |> Enum.with_index(1)
-      |> Enum.map(fn {path, index} ->
-        template = upsert_template_from_file(project_id, project, path)
+      |> Enum.flat_map(fn {path, index} ->
+        result = upsert_template_from_file(project_id, project, path)
         :ok = report_rebuild_progress(on_progress, index, total_files, "template files")
-        template
+
+        case result do
+          {:ok, template} ->
+            [template]
+
+          {:error, reason} ->
+            Logger.warning("Skipping template #{path}: #{inspect(reason)}")
+            []
+        end
       end)
 
     remove_stale_published_templates(project_id, project, template_paths)
@@ -241,10 +251,9 @@ defmodule BDS.Templates do
         project = Projects.get_project!(template.project_id)
         full_path = Path.join(Projects.project_data_dir(project), template.file_path)
 
-        if File.exists?(full_path) do
-          {:ok, upsert_template_from_file(template.project_id, project, full_path)}
-        else
-          {:error, :not_found}
+        case upsert_template_from_file(template.project_id, project, full_path) do
+          {:ok, _template} = ok -> ok
+          {:error, _reason} -> {:error, :not_found}
         end
     end
   end
@@ -278,10 +287,9 @@ defmodule BDS.Templates do
     project = Projects.get_project!(project_id)
     full_path = Path.join(Projects.project_data_dir(project), relative_path)
 
-    if File.exists?(full_path) do
-      {:ok, upsert_template_from_file(project_id, project, full_path)}
-    else
-      {:error, :not_found}
+    case upsert_template_from_file(project_id, project, full_path) do
+      {:ok, _template} = ok -> ok
+      {:error, _reason} -> {:error, :not_found}
     end
   end
 
@@ -493,9 +501,10 @@ defmodule BDS.Templates do
   end
 
   defp upsert_template_from_file(project_id, project, path) do
-    contents = File.read!(path)
-    {:ok, %{fields: fields}} = Frontmatter.parse_document(contents)
     relative_path = Path.relative_to(path, Projects.project_data_dir(project))
+
+    with {:ok, contents} <- File.read(path),
+         {:ok, %{fields: fields}} <- Frontmatter.parse_document(contents) do
       now = Persistence.now_ms()
 
       attrs = %{
@@ -517,7 +526,8 @@ defmodule BDS.Templates do
 
       template
       |> Template.changeset(attrs)
-    |> Repo.insert_or_update!()
+      |> Repo.insert_or_update()
+    end
   end
 
   defp remove_stale_published_templates(project_id, project, template_paths) do

test/bds/csm034_file_read_bang_test.exs

@@ -0,0 +1,107 @@
+defmodule BDS.CSM034FileReadBangTest do
+  use ExUnit.Case, async: false
+  import Ecto.Query
+
+  describe "source-level: no File.read! or File.write! in affected files" do
+    test "preview_assets.ex has no File.read!" do
+      source = File.read!("lib/bds/preview_assets.ex")
+      refute source =~ "File.read!", "preview_assets.ex should use File.read, not File.read!"
+    end
+
+    test "templates.ex has no File.read!" do
+      source = File.read!("lib/bds/templates.ex")
+      refute source =~ "File.read!", "templates.ex should use File.read, not File.read!"
+    end
+
+    test "templates.ex has no File.write!" do
+      source = File.read!("lib/bds/templates.ex")
+      refute source =~ "File.write!", "templates.ex should use File.write, not File.write!"
+    end
+
+    test "release_packaging.ex has no File.read! or File.write!" do
+      source = File.read!("lib/bds/release_packaging.ex")
+      refute source =~ "File.read!", "release_packaging.ex should use File.read, not File.read!"
+      refute source =~ "File.write!", "release_packaging.ex should use File.write, not File.write!"
+    end
+  end
+
+  describe "preview_assets.generated_outputs/0 handles read errors" do
+    test "returns results for readable files, skips unreadable ones" do
+      outputs = BDS.PreviewAssets.generated_outputs()
+      assert is_list(outputs)
+      assert Enum.all?(outputs, fn {path, body} -> is_binary(path) and is_binary(body) end)
+    end
+  end
+
+  describe "templates file error handling" do
+    setup do
+      :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo)
+      temp_dir = Path.join(System.tmp_dir!(), "bds-csm034-#{System.unique_integer([:positive])}")
+      File.mkdir_p!(temp_dir)
+      on_exit(fn -> File.rm_rf(temp_dir) end)
+
+      {:ok, project} = BDS.Projects.create_project(%{name: "CSM034", data_path: temp_dir})
+      %{project: project, temp_dir: temp_dir}
+    end
+
+    test "rebuild skips unreadable template files without crashing", %{
+      project: project,
+      temp_dir: temp_dir
+    } do
+      templates_dir = Path.join(temp_dir, "templates")
+      File.mkdir_p!(templates_dir)
+
+      File.write!(Path.join(templates_dir, "good.liquid"), """
+      ---
+      slug: good
+      kind: post
+      title: Good Template
+      ---
+      <p>{{ content }}</p>
+      """)
+
+      File.write!(Path.join(templates_dir, "bad.liquid"), "no frontmatter here")
+
+      assert {:ok, templates} = BDS.Templates.rebuild_templates_from_files(project.id)
+      assert length(templates) == 1
+      assert hd(templates).slug == "good"
+    end
+
+    test "sync_template_from_file returns error when file is deleted", %{
+      project: project,
+      temp_dir: temp_dir
+    } do
+      templates_dir = Path.join(temp_dir, "templates")
+      File.mkdir_p!(templates_dir)
+
+      path = Path.join(templates_dir, "ephemeral.liquid")
+
+      File.write!(path, """
+      ---
+      slug: ephemeral
+      kind: post
+      title: Ephemeral
+      ---
+      <p>{{ content }}</p>
+      """)
+
+      {:ok, _templates} = BDS.Templates.rebuild_templates_from_files(project.id)
+
+      template =
+        BDS.Repo.one(
+          from(t in BDS.Templates.Template,
+            where: t.project_id == ^project.id and t.slug == "ephemeral"
+          )
+        )
+
+      File.rm!(path)
+
+      assert {:error, :not_found} = BDS.Templates.sync_template_from_file(template.id)
+    end
+
+    test "import_orphan_template_file returns error for missing file", %{project: project} do
+      assert {:error, :not_found} =
+               BDS.Templates.import_orphan_template_file(project.id, "templates/ghost.liquid")
+    end
+  end
+end