From 9e6d93a4b31298906f94f38ac24f88c636c6c927 Mon Sep 17 00:00:00 2001
From: Chili Palmer <hugoms@me.com>
Date: Wed, 27 May 2026 19:10:13 +0200
Subject: [PATCH] fix(safety): replace File.read! with File.read and
 error-tuple handling in preview_assets and templates (CSM-034)

---
 CODESMELL.md                            |  11 ++-
 lib/bds/preview_assets.ex               |   7 +-
 lib/bds/templates.ex                    |  74 +++++++++-------
 test/bds/csm034_file_read_bang_test.exs | 107 ++++++++++++++++++++++++
 4 files changed, 162 insertions(+), 37 deletions(-)
 create mode 100644 test/bds/csm034_file_read_bang_test.exs

diff --git a/CODESMELL.md b/CODESMELL.md
index a022c90..30e31e7 100644
--- a/CODESMELL.md
+++ b/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.
 
 ---
 
diff --git a/lib/bds/preview_assets.ex b/lib/bds/preview_assets.ex
index 8246085..975b7f6 100644
--- a/lib/bds/preview_assets.ex
+++ b/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
 
diff --git a/lib/bds/templates.ex b/lib/bds/templates.ex
index 790622c..d04d4cc 100644
--- a/lib/bds/templates.ex
+++ b/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,31 +501,33 @@ 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))
-    now = Persistence.now_ms()
 
-    attrs = %{
-      id: DocumentFields.get(fields, "id") || Ecto.UUID.generate(),
-      project_id: project_id,
-      slug: DocumentFields.fetch!(fields, "slug"),
-      title: DocumentFields.get(fields, "title") || "",
-      kind: parse_template_kind(DocumentFields.fetch!(fields, "kind")),
-      enabled: Map.get(fields, "enabled", true),
-      version: Map.get(fields, "version", 1),
-      file_path: relative_path,
-      status: :published,
-      content: nil,
-      created_at: DocumentFields.get(fields, "createdAt", now),
-      updated_at: DocumentFields.get(fields, "updatedAt", now)
-    }
+    with {:ok, contents} <- File.read(path),
+         {:ok, %{fields: fields}} <- Frontmatter.parse_document(contents) do
+      now = Persistence.now_ms()
 
-    template = Repo.get_by(Template, project_id: project_id, slug: attrs.slug) || %Template{}
+      attrs = %{
+        id: DocumentFields.get(fields, "id") || Ecto.UUID.generate(),
+        project_id: project_id,
+        slug: DocumentFields.fetch!(fields, "slug"),
+        title: DocumentFields.get(fields, "title") || "",
+        kind: parse_template_kind(DocumentFields.fetch!(fields, "kind")),
+        enabled: Map.get(fields, "enabled", true),
+        version: Map.get(fields, "version", 1),
+        file_path: relative_path,
+        status: :published,
+        content: nil,
+        created_at: DocumentFields.get(fields, "createdAt", now),
+        updated_at: DocumentFields.get(fields, "updatedAt", now)
+      }
 
-    template
-    |> Template.changeset(attrs)
-    |> Repo.insert_or_update!()
+      template = Repo.get_by(Template, project_id: project_id, slug: attrs.slug) || %Template{}
+
+      template
+      |> Template.changeset(attrs)
+      |> Repo.insert_or_update()
+    end
   end
 
   defp remove_stale_published_templates(project_id, project, template_paths) do
diff --git a/test/bds/csm034_file_read_bang_test.exs b/test/bds/csm034_file_read_bang_test.exs
new file mode 100644
index 0000000..abb41af
--- /dev/null
+++ b/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