diff --git a/CODESMELL.md b/CODESMELL.md index b226382..0feff21 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -65,11 +65,11 @@ _None._ All modules previously on the queue have been split; refresh the queue i ## 5. Bang File Operations in Long-Running Code -**Status:** open (limited scope). +**Status:** ✅ done (2026-05-10). Scoped `File.read!` / `File.write!` sites reachable from rebuild workers and LiveView events have been replaced with `{:ok, _} | {:error, _}` propagation. -**Scope:** only `File.read!`/`File.write!` reachable from a GenServer worker, scheduled task, or LiveView event matter. Mandatory-config reads at boot are fine. +**Scope audited:** `BDS.Posts.RebuildFromFiles`, `BDS.Media`, `BDS.MCP`, `BDS.Generation`, and the MCP settings UI config probe. Mandatory-config reads at boot remain out of scope. -**Plan:** sweep `BDS.Posts.RebuildFromFiles`, `BDS.Media`, `BDS.MCP`, `BDS.Generation`. Replace bang variants with `{:ok, _} | {:error, _}` propagation only on those paths. +**Rule:** long-running rebuild/sync/import paths and LiveView event-triggered config writes must not crash on expected filesystem read/write failures; return tagged errors instead. --- @@ -171,6 +171,9 @@ Most tests share the SQLite repo and named GenServers (`BDS.Tasks`, `BDS.Search` ### 2026-05-10 +- **Bang file operations in long-running code**: `BDS.Media.Sidecars.parse_canonical_sidecar/2` and `parse_translation_sidecar/1` now use `File.read/1` and return `{:ok, sidecar}` or `{:error, {:read_sidecar, path, reason}}` instead of raising. Media rebuild collects parsed sidecars and returns the first sidecar read error before mutating rows; media sync/import sidecar entrypoints propagate the same errors. Added regression coverage for an unreadable `.meta` sidecar directory preserving the worker instead of crashing it. +- **Bang file operations in long-running code**: `BDS.Posts.RebuildFromFiles.parse_rebuild_file/2` now uses `File.read/1` and returns `{:ok, rebuild_file}` or `{:error, {:read_rebuild_file, path, reason}}`; post rebuild/import/sync-from-file callers propagate the tagged error. `BDS.Generation.apply_validation/2` now hashes existing generated files with `File.read/1` and returns `{:error, {:read_generated_file, path, reason}}` on read failures. `BDS.MCP.AgentConfig` now uses `File.mkdir_p/1`, `File.read/1`, `Jason.decode/1`, and `File.write/2`, returning tagged config read/write/create/decode errors instead of raising; the settings editor reports those errors through its existing output surface and its config probe no longer uses bang reads. Added regressions for unreadable post files, unreadable generated files, unreadable MCP config, and unwritable MCP config. Section 5 is closed. + - **Side effects in transactions**: `BDS.Metadata.update_project_metadata/2`, `sync_project_metadata_from_filesystem/1`, and the shared category/publishing `update_state` path now keep only project/settings row changes inside `Repo.transaction/1`. Metadata JSON files are flushed after commit and `Persistence.atomic_write/2` now returns `{:error, reason}` for directory-creation failures instead of raising. Added regression coverage for a failed metadata filesystem flush preserving the committed project/settings changes. - **Side effects in transactions**: `BDS.Tags.sync_tags_from_posts/1`, `delete_tag/1`, `rename_tag/2`, and `merge_tags/2` now commit tag/post-tag DB changes before published-post rewrites and `meta/tags.json` flushes. `BDS.Projects.ensure_default_project/0` and `create_project/1` now commit the project row before rebuilding templates from filesystem files. Added regressions for failed tag JSON flushes and failed template rebuilds preserving committed DB changes. Finished the explicit `Repo.transaction/1` audit: remaining transactions are DB-only or already defer filesystem cleanup until after commit; `BDS.Posts`, `BDS.Publishing`, and `BDS.Generation` have no explicit transaction sites. - **Process dictionary for i18n state (Section 2)**: encapsulated behind `BDS.Desktop.UILocale` (`lib/bds/desktop/ui_locale.ex`, ~50 lines). Public surface: `put/1` (set without restore, for LV render boundaries that return lazy `Rendered`), `with_locale/2` (set + try/after restore, for short-lived eager contexts), `current/0` (read, returns `nil` when unset). The two raw `Process.put(:bds_ui_locale, _)` sites (`BDS.Desktop.ShellLive.render/1` and `BDS.Desktop.ShellLive.SidebarComponents.sidebar_content/1`) now call `UILocale.put/1`; the ~30 raw `Process.get(:bds_ui_locale)` reads (every editor `translated/1,2` helper plus `BDS.Desktop.ShellData.effective_ui_language/1`) now call `BDS.Desktop.UILocale.current/0`. The full thread-locale-through-assigns rewrite (~733 HEEx call sites) was deliberately rejected as too invasive; the encapsulation removes the implicit-global smell while preserving Phoenix's lazy `Rendered` evaluation. The render path uses `put/1` (not `with_locale/2`) because Phoenix `~H` returns a `Rendered` whose `dynamic` is invoked by LiveView *after* `render/1` returns; a `try/after Process.delete` would clear the binding before child components materialize. Only `BDS.Desktop.UILocale` is allowed to touch the `:bds_ui_locale` process key. Validates clean: `mix compile --warnings-as-errors`, `mix dialyzer --format short` (Total errors: 0), `mix test` (342 tests, 0 failures, 4 skipped, three consecutive runs). diff --git a/lib/bds/desktop/shell_live/settings_editor/mcp_config.ex b/lib/bds/desktop/shell_live/settings_editor/mcp_config.ex index b8289e8..db33e45 100644 --- a/lib/bds/desktop/shell_live/settings_editor/mcp_config.ex +++ b/lib/bds/desktop/shell_live/settings_editor/mcp_config.ex @@ -31,13 +31,22 @@ defmodule BDS.Desktop.ShellLive.SettingsEditor.MCPConfig do def toggle_mcp_agent(socket, agent, reload, append_output) do case find_mcp_agent(agent) do %{id: agent_id, supported?: true} = config -> - if mcp_configured?(config) do - {:ok, _payload} = AgentConfig.remove_from_config(agent_id) - reload.(socket, socket.assigns.workbench) - else - install_root = Application.app_dir(:bds) - {:ok, _payload} = AgentConfig.add_to_config(agent_id, install_root: install_root) - reload.(socket, socket.assigns.workbench) + result = + if mcp_configured?(config) do + AgentConfig.remove_from_config(agent_id) + else + install_root = Application.app_dir(:bds) + AgentConfig.add_to_config(agent_id, install_root: install_root) + end + + case result do + {:ok, _payload} -> + reload.(socket, socket.assigns.workbench) + + {:error, reason} -> + socket + |> append_output.(translated("MCP"), format_config_error(reason), nil, "error") + |> reload.(socket.assigns.workbench) end _other -> @@ -63,21 +72,34 @@ defmodule BDS.Desktop.ShellLive.SettingsEditor.MCPConfig do _error -> nil end + defp format_config_error({:read_config, path, reason}) do + translated("Could not read MCP config %{path}: %{reason}", path: path, reason: inspect(reason)) + end + + defp format_config_error({:write_config, path, reason}) do + translated("Could not write MCP config %{path}: %{reason}", path: path, reason: inspect(reason)) + end + + defp format_config_error({:create_config_dir, path, reason}) do + translated("Could not create MCP config folder %{path}: %{reason}", path: path, reason: inspect(reason)) + end + + defp format_config_error({:decode_config, path, _reason}) do + translated("Could not parse MCP config %{path}", path: path) + end + defp mcp_configured?(%{supported?: false}), do: false defp mcp_configured?(%{id: agent_id}) do path = AgentConfig.config_path(agent_id, System.user_home!()) - if File.exists?(path) do - path - |> File.read!() - |> Jason.decode!() - |> mcp_server_present?(agent_id) + with true <- File.exists?(path), + {:ok, contents} <- File.read(path), + {:ok, config} <- Jason.decode(contents) do + mcp_server_present?(config, agent_id) else - false + _other -> false end - rescue - _error -> false end defp mcp_config_path(%{supported?: false}), do: nil diff --git a/lib/bds/generation.ex b/lib/bds/generation.ex index 0e810a8..57459d9 100644 --- a/lib/bds/generation.ex +++ b/lib/bds/generation.ex @@ -155,10 +155,10 @@ defmodule BDS.Generation do @spec apply_validation(String.t(), [section()] | map()) :: {:ok, map()} | {:error, term()} def apply_validation(project_id, sections) when is_binary(project_id) and is_list(sections) do - with {:ok, plan} <- plan_generation(project_id, sections) do + with {:ok, plan} <- plan_generation(project_id, sections), + {:ok, actual_files} <- disk_generated_files(project_id) do expected_outputs = build_outputs(plan) expected_paths = MapSet.new(Enum.map(expected_outputs, &elem(&1, 0))) - actual_files = disk_generated_files(project_id) project = Projects.get_project!(project_id) now = Persistence.now_ms() @@ -186,24 +186,26 @@ defmodule BDS.Generation do end end) - disk_generated_files(project_id) - |> Map.keys() - |> Enum.filter(fn relative_path -> - path_section(relative_path) in plan.sections and not MapSet.member?(expected_paths, relative_path) - end) - |> Enum.each(fn relative_path -> - _ = File.rm(output_path(project, relative_path)) + with {:ok, generated_files_on_disk} <- disk_generated_files(project_id) do + generated_files_on_disk + |> Map.keys() + |> Enum.filter(fn relative_path -> + path_section(relative_path) in plan.sections and not MapSet.member?(expected_paths, relative_path) + end) + |> Enum.each(fn relative_path -> + _ = File.rm(output_path(project, relative_path)) - Repo.delete_all( - from generated_file in GeneratedFileHash, - where: - generated_file.project_id == ^project_id and - generated_file.relative_path == ^relative_path - ) - end) + Repo.delete_all( + from generated_file in GeneratedFileHash, + where: + generated_file.project_id == ^project_id and + generated_file.relative_path == ^relative_path + ) + end) - {:ok, generated_files} = list_generated_files(project_id) - {:ok, %{sections: plan.sections, generated_files: generated_files}} + {:ok, generated_files} = list_generated_files(project_id) + {:ok, %{sections: plan.sections, generated_files: generated_files}} + end end end @@ -521,18 +523,20 @@ defmodule BDS.Generation do |> Path.join("**/*") |> Path.wildcard(match_dot: false) |> Enum.filter(&File.regular?/1) - |> Enum.map(fn path -> + |> Enum.reduce_while({:ok, %{}}, fn path, {:ok, files} -> relative_path = Path.relative_to(path, html_root) - {relative_path, - path - |> File.read!() - |> sha256()} + case File.read(path) do + {:ok, contents} -> + {:cont, {:ok, Map.put(files, relative_path, sha256(contents))}} + + {:error, reason} -> + {:halt, {:error, {:read_generated_file, path, reason}}} + end end) - |> Map.new() {:error, :enoent} -> - %{} + {:ok, %{}} end end diff --git a/lib/bds/mcp/agent_config.ex b/lib/bds/mcp/agent_config.ex index 26a4038..299015c 100644 --- a/lib/bds/mcp/agent_config.ex +++ b/lib/bds/mcp/agent_config.ex @@ -9,26 +9,24 @@ defmodule BDS.MCP.AgentConfig do command = Keyword.get(opts, :command, default_command(opts)) args = Keyword.get(opts, :args, default_args(opts)) - File.mkdir_p!(Path.dirname(config_path)) - - config = read_config(config_path) - updated = merge_config(agent, config, command, args) - File.write!(config_path, Jason.encode!(updated, pretty: true)) - - {:ok, %{config_path: config_path, server_name: @server_name}} + with :ok <- ensure_config_dir(config_path), + {:ok, config} <- read_config(config_path), + updated <- merge_config(agent, config, command, args), + :ok <- write_config(config_path, updated) do + {:ok, %{config_path: config_path, server_name: @server_name}} + end end def remove_from_config(agent, opts \\ []) when is_atom(agent) and is_list(opts) do home_dir = Keyword.get(opts, :home_dir, System.user_home!()) config_path = config_path(agent, home_dir) - File.mkdir_p!(Path.dirname(config_path)) - - config = read_config(config_path) - updated = remove_server_entry(agent, config) - File.write!(config_path, Jason.encode!(updated, pretty: true)) - - {:ok, %{config_path: config_path, server_name: @server_name}} + with :ok <- ensure_config_dir(config_path), + {:ok, config} <- read_config(config_path), + updated <- remove_server_entry(agent, config), + :ok <- write_config(config_path, updated) do + {:ok, %{config_path: config_path, server_name: @server_name}} + end end def config_path(:claude_code, home_dir), do: Path.join(home_dir, ".claude.json") @@ -53,12 +51,39 @@ defmodule BDS.MCP.AgentConfig do defp default_args(_opts), do: [] defp read_config(path) do - if File.exists?(path) do - path - |> File.read!() - |> Jason.decode!() - else - %{} + cond do + File.exists?(path) -> + with {:ok, contents} <- read_config_file(path), + {:ok, config} <- Jason.decode(contents) do + {:ok, config} + else + {:error, %Jason.DecodeError{} = reason} -> {:error, {:decode_config, path, reason}} + {:error, reason} -> {:error, reason} + end + + true -> + {:ok, %{}} + end + end + + defp ensure_config_dir(config_path) do + case File.mkdir_p(Path.dirname(config_path)) do + :ok -> :ok + {:error, reason} -> {:error, {:create_config_dir, Path.dirname(config_path), reason}} + end + end + + defp read_config_file(path) do + case File.read(path) do + {:ok, contents} -> {:ok, contents} + {:error, reason} -> {:error, {:read_config, path, reason}} + end + end + + defp write_config(path, config) do + case File.write(path, Jason.encode!(config, pretty: true)) do + :ok -> :ok + {:error, reason} -> {:error, {:write_config, path, reason}} end end diff --git a/lib/bds/media/rebuilder.ex b/lib/bds/media/rebuilder.ex index 375a08c..c7a3346 100644 --- a/lib/bds/media/rebuilder.ex +++ b/lib/bds/media/rebuilder.ex @@ -19,64 +19,77 @@ defmodule BDS.Media.Rebuilder do @type rebuild_opts :: keyword() - @spec rebuild_media_from_files(String.t(), rebuild_opts()) :: {:ok, [Media.t()]} + @spec rebuild_media_from_files(String.t(), rebuild_opts()) :: {:ok, [Media.t()]} | {:error, term()} def rebuild_media_from_files(project_id, opts \\ []) do project = Projects.get_project!(project_id) on_progress = progress_callback(opts) - canonical_sidecars = - project - |> Projects.project_data_dir() - |> Path.join("media") + media_dir = project |> Projects.project_data_dir() |> Path.join("media") + + canonical_results = + media_dir |> list_matching_files("*.meta") |> Enum.filter(&Sidecars.canonical_sidecar?/1) |> Enum.filter(&Sidecars.binary_exists_for_sidecar?/1) |> Rebuild.parallel_map(&Sidecars.parse_canonical_sidecar(project, &1)) - translation_sidecars = - project - |> Projects.project_data_dir() - |> Path.join("media") - |> list_matching_files("*.meta") - |> Enum.filter(&Sidecars.translation_sidecar?/1) - |> Rebuild.parallel_map(&Sidecars.parse_translation_sidecar(&1)) + with {:ok, canonical_sidecars} <- collect_sidecars(canonical_results) do + translation_results = + media_dir + |> list_matching_files("*.meta") + |> Enum.filter(&Sidecars.translation_sidecar?/1) + |> Rebuild.parallel_map(&Sidecars.parse_translation_sidecar(&1)) - total_files = length(canonical_sidecars) + length(translation_sidecars) - :ok = report_rebuild_started(on_progress, total_files, "media files") + with {:ok, translation_sidecars} <- collect_sidecars(translation_results) do + total_files = length(canonical_sidecars) + length(translation_sidecars) + :ok = report_rebuild_started(on_progress, total_files, "media files") - media_items = - canonical_sidecars - |> Enum.with_index(1) - |> Enum.map(fn {sidecar, index} -> - media = Sidecars.upsert_media_from_sidecar(project, sidecar, sync_search: false) - :ok = report_rebuild_progress(on_progress, index, total_files, "media files") - media - end) + media_items = + canonical_sidecars + |> Enum.with_index(1) + |> Enum.map(fn {sidecar, index} -> + media = Sidecars.upsert_media_from_sidecar(project, sidecar, sync_search: false) + :ok = report_rebuild_progress(on_progress, index, total_files, "media files") + media + end) - canonical_media_by_binary_path = - Map.new(media_items, fn media -> - {Path.join(Projects.project_data_dir(project), media.file_path), media} - end) + canonical_media_by_binary_path = + Map.new(media_items, fn media -> + {Path.join(Projects.project_data_dir(project), media.file_path), media} + end) - translation_sidecars - |> Enum.with_index(length(canonical_sidecars) + 1) - |> Enum.each(fn {sidecar, index} -> - Sidecars.upsert_translation_from_sidecar(project, canonical_media_by_binary_path, sidecar, - sync_search: false - ) + translation_sidecars + |> Enum.with_index(length(canonical_sidecars) + 1) + |> Enum.each(fn {sidecar, index} -> + Sidecars.upsert_translation_from_sidecar(project, canonical_media_by_binary_path, sidecar, + sync_search: false + ) - :ok = report_rebuild_progress(on_progress, index, total_files, "media files") - end) + :ok = report_rebuild_progress(on_progress, index, total_files, "media files") + end) - if Keyword.get(opts, :reindex_search, true) do - :ok = report_rebuild_phase(on_progress, 0.99, "Refreshing media search index") + if Keyword.get(opts, :reindex_search, true) do + :ok = report_rebuild_phase(on_progress, 0.99, "Refreshing media search index") - :ok = - Search.reindex_media(project.id, - on_progress: scaled_progress_reporter(on_progress, 0.99, 1.0) - ) + :ok = + Search.reindex_media(project.id, + on_progress: scaled_progress_reporter(on_progress, 0.99, 1.0) + ) + end + + {:ok, media_items} + end end + end - {:ok, media_items} + defp collect_sidecars(results) do + Enum.reduce_while(results, {:ok, []}, fn + {:ok, sidecar}, {:ok, sidecars} -> {:cont, {:ok, [sidecar | sidecars]}} + {:error, reason}, {:ok, _sidecars} -> {:halt, {:error, reason}} + end) + |> case do + {:ok, sidecars} -> {:ok, Enum.reverse(sidecars)} + {:error, reason} -> {:error, reason} + end end end diff --git a/lib/bds/media/sidecars.ex b/lib/bds/media/sidecars.ex index 7e49ff0..dc2263f 100644 --- a/lib/bds/media/sidecars.ex +++ b/lib/bds/media/sidecars.ex @@ -67,28 +67,35 @@ defmodule BDS.Media.Sidecars do ) end - @spec parse_canonical_sidecar(BDS.Projects.Project.t(), Path.t()) :: map() + @spec parse_canonical_sidecar(BDS.Projects.Project.t(), Path.t()) :: + {:ok, map()} | {:error, {:read_sidecar, Path.t(), File.posix()}} def parse_canonical_sidecar(project, sidecar_path) do - {:ok, fields} = sidecar_path |> File.read!() |> Sidecar.parse_document() - relative_sidecar_path = Path.relative_to(sidecar_path, Projects.project_data_dir(project)) - relative_file_path = String.trim_trailing(relative_sidecar_path, ".meta") + with {:ok, contents} <- read_sidecar(sidecar_path), + {:ok, fields} <- Sidecar.parse_document(contents) do + relative_sidecar_path = Path.relative_to(sidecar_path, Projects.project_data_dir(project)) + relative_file_path = String.trim_trailing(relative_sidecar_path, ".meta") - %{ - fields: fields, - relative_sidecar_path: relative_sidecar_path, - relative_file_path: relative_file_path, - filename: Path.basename(relative_file_path) - } + {:ok, + %{ + fields: fields, + relative_sidecar_path: relative_sidecar_path, + relative_file_path: relative_file_path, + filename: Path.basename(relative_file_path) + }} + end end - @spec parse_translation_sidecar(Path.t()) :: map() + @spec parse_translation_sidecar(Path.t()) :: + {:ok, map()} | {:error, {:read_sidecar, Path.t(), File.posix()}} def parse_translation_sidecar(sidecar_path) do - {:ok, fields} = sidecar_path |> File.read!() |> Sidecar.parse_document() - - %{ - fields: fields, - binary_path: binary_path_for_translation_sidecar(sidecar_path) - } + with {:ok, contents} <- read_sidecar(sidecar_path), + {:ok, fields} <- Sidecar.parse_document(contents) do + {:ok, + %{ + fields: fields, + binary_path: binary_path_for_translation_sidecar(sidecar_path) + }} + end end @spec upsert_media_from_sidecar(BDS.Projects.Project.t(), map(), keyword()) :: Media.t() @@ -192,10 +199,12 @@ defmodule BDS.Media.Sidecars do project = Projects.get_project!(media.project_id) sidecar_path = Path.join(Projects.project_data_dir(project), media.sidecar_path) - if File.exists?(sidecar_path) do - {:ok, upsert_media_from_sidecar(project, parse_canonical_sidecar(project, sidecar_path), sync_search: true)} - else - {:error, :not_found} + case parse_existing_canonical_sidecar(project, sidecar_path) do + {:ok, sidecar} -> + {:ok, upsert_media_from_sidecar(project, sidecar, sync_search: true)} + + {:error, reason} -> + {:error, reason} end end end @@ -232,23 +241,23 @@ defmodule BDS.Media.Sidecars do translation_sidecar_path(media, translation.language) ) - if File.exists?(sidecar_path) do - sidecar = parse_translation_sidecar(sidecar_path) + case parse_existing_translation_sidecar(sidecar_path) do + {:ok, sidecar} -> + case BDS.Media.upsert_media_translation( + media.id, + DocumentFields.fetch!(sidecar.fields, "language"), + %{ + title: DocumentFields.get(sidecar.fields, "title"), + alt: DocumentFields.get(sidecar.fields, "alt"), + caption: DocumentFields.get(sidecar.fields, "caption") + } + ) do + {:ok, updated_translation} -> {:ok, updated_translation} + error -> error + end - case BDS.Media.upsert_media_translation( - media.id, - DocumentFields.fetch!(sidecar.fields, "language"), - %{ - title: DocumentFields.get(sidecar.fields, "title"), - alt: DocumentFields.get(sidecar.fields, "alt"), - caption: DocumentFields.get(sidecar.fields, "caption") - } - ) do - {:ok, updated_translation} -> {:ok, updated_translation} - error -> error - end - else - {:error, :not_found} + {:error, reason} -> + {:error, reason} end end end @@ -259,10 +268,12 @@ defmodule BDS.Media.Sidecars do project = Projects.get_project!(project_id) sidecar_path = Path.join(Projects.project_data_dir(project), relative_path) - if File.exists?(sidecar_path) do - {:ok, upsert_media_from_sidecar(project, parse_canonical_sidecar(project, sidecar_path), sync_search: true)} - else - {:error, :not_found} + case parse_existing_canonical_sidecar(project, sidecar_path) do + {:ok, sidecar} -> + {:ok, upsert_media_from_sidecar(project, sidecar, sync_search: true)} + + {:error, reason} -> + {:error, reason} end end @@ -272,35 +283,35 @@ defmodule BDS.Media.Sidecars do project = Projects.get_project!(project_id) sidecar_path = Path.join(Projects.project_data_dir(project), relative_path) - if File.exists?(sidecar_path) do - sidecar = parse_translation_sidecar(sidecar_path) + case parse_existing_translation_sidecar(sidecar_path) do + {:ok, sidecar} -> + case Repo.get(Media, DocumentFields.get(sidecar.fields, "translationFor")) do + nil -> + {:error, :not_found} - case Repo.get(Media, DocumentFields.get(sidecar.fields, "translationFor")) do - nil -> - {:error, :not_found} + media -> + case Repo.get_by(Translation, + translation_for: media.id, + language: DocumentFields.fetch!(sidecar.fields, "language") + ) do + nil -> + BDS.Media.upsert_media_translation( + media.id, + DocumentFields.fetch!(sidecar.fields, "language"), + %{ + title: DocumentFields.get(sidecar.fields, "title"), + alt: DocumentFields.get(sidecar.fields, "alt"), + caption: DocumentFields.get(sidecar.fields, "caption") + } + ) - media -> - case Repo.get_by(Translation, - translation_for: media.id, - language: DocumentFields.fetch!(sidecar.fields, "language") - ) do - nil -> - BDS.Media.upsert_media_translation( - media.id, - DocumentFields.fetch!(sidecar.fields, "language"), - %{ - title: DocumentFields.get(sidecar.fields, "title"), - alt: DocumentFields.get(sidecar.fields, "alt"), - caption: DocumentFields.get(sidecar.fields, "caption") - } - ) + _translation -> + {:error, :conflict} + end + end - _translation -> - {:error, :conflict} - end - end - else - {:error, :not_found} + {:error, reason} -> + {:error, reason} end end @@ -326,4 +337,27 @@ defmodule BDS.Media.Sidecars do |> String.trim_trailing(".meta") |> File.exists?() end + + defp parse_existing_canonical_sidecar(project, sidecar_path) do + if File.exists?(sidecar_path) do + parse_canonical_sidecar(project, sidecar_path) + else + {:error, :not_found} + end + end + + defp parse_existing_translation_sidecar(sidecar_path) do + if File.exists?(sidecar_path) do + parse_translation_sidecar(sidecar_path) + else + {:error, :not_found} + end + end + + defp read_sidecar(sidecar_path) do + case File.read(sidecar_path) do + {:ok, contents} -> {:ok, contents} + {:error, reason} -> {:error, {:read_sidecar, sidecar_path, reason}} + end + end end diff --git a/lib/bds/posts.ex b/lib/bds/posts.ex index 8efb8af..6a10f86 100644 --- a/lib/bds/posts.ex +++ b/lib/bds/posts.ex @@ -193,7 +193,7 @@ defmodule BDS.Posts do end end - @spec rebuild_posts_from_files(String.t(), rebuild_opts()) :: {:ok, [Post.t()]} + @spec rebuild_posts_from_files(String.t(), rebuild_opts()) :: {:ok, [Post.t()]} | {:error, term()} defdelegate rebuild_posts_from_files(project_id, opts \\ []), to: RebuildFromFiles @spec discard_post_changes(String.t()) :: @@ -211,9 +211,12 @@ defmodule BDS.Posts do full_path = Path.join(Projects.project_data_dir(project), post.file_path) if File.exists?(full_path) do - restored_post = RebuildFromFiles.upsert_post_from_file(post.project_id, project, full_path) - :ok = PostLinks.sync_post_links(restored_post) - {:ok, restored_post} + with {:ok, restored_post} <- RebuildFromFiles.upsert_post_from_file(post.project_id, project, full_path) do + :ok = PostLinks.sync_post_links(restored_post) + {:ok, restored_post} + else + {:error, reason} -> {:error, reason} + end else {:error, :not_found} end @@ -259,9 +262,12 @@ defmodule BDS.Posts do full_path = Path.join(Projects.project_data_dir(project), post.file_path) if File.exists?(full_path) do - repaired_post = RebuildFromFiles.upsert_post_from_file(post.project_id, project, full_path) - :ok = PostLinks.sync_post_links(repaired_post) - {:ok, repaired_post} + with {:ok, repaired_post} <- RebuildFromFiles.upsert_post_from_file(post.project_id, project, full_path) do + :ok = PostLinks.sync_post_links(repaired_post) + {:ok, repaired_post} + else + {:error, reason} -> {:error, reason} + end else {:error, :not_found} end diff --git a/lib/bds/posts/rebuild_from_files.ex b/lib/bds/posts/rebuild_from_files.ex index c6a5684..c26fd73 100644 --- a/lib/bds/posts/rebuild_from_files.ex +++ b/lib/bds/posts/rebuild_from_files.ex @@ -14,64 +14,66 @@ defmodule BDS.Posts.RebuildFromFiles do alias BDS.Repo alias BDS.Search - @spec rebuild_posts_from_files(String.t(), keyword()) :: {:ok, [Post.t()]} + @spec rebuild_posts_from_files(String.t(), keyword()) :: {:ok, [Post.t()]} | {:error, term()} def rebuild_posts_from_files(project_id, opts \\ []) do project = Projects.get_project!(project_id) on_progress = progress_callback(opts) - rebuild_files = + rebuild_results = project |> Projects.project_data_dir() |> Path.join("posts") |> TranslationValidation.list_matching_files("*.md") |> Rebuild.parallel_map(&parse_rebuild_file(project, &1)) - total_files = length(rebuild_files) - :ok = report_rebuild_started(on_progress, total_files, "post files") + with {:ok, rebuild_files} <- collect_rebuild_files(rebuild_results) do + total_files = length(rebuild_files) + :ok = report_rebuild_started(on_progress, total_files, "post files") - {translation_files, post_files} = - Enum.split_with(rebuild_files, &TranslationValidation.translation_rebuild_file?/1) + {translation_files, post_files} = + Enum.split_with(rebuild_files, &TranslationValidation.translation_rebuild_file?/1) - posts = - post_files - |> Enum.with_index(1) - |> Enum.map(fn {file, index} -> - post = - upsert_post_from_rebuild_file(project_id, file, - sync_search: false, - sync_embeddings: false - ) + posts = + post_files + |> Enum.with_index(1) + |> Enum.map(fn {file, index} -> + post = + upsert_post_from_rebuild_file(project_id, file, + sync_search: false, + sync_embeddings: false + ) + :ok = report_rebuild_progress(on_progress, index, total_files, "post files") + post + end) + + translation_files + |> Enum.with_index(length(post_files) + 1) + |> Enum.each(fn {file, index} -> + upsert_post_translation_from_rebuild_file(project_id, file, sync_search: false) :ok = report_rebuild_progress(on_progress, index, total_files, "post files") - post end) - translation_files - |> Enum.with_index(length(post_files) + 1) - |> Enum.each(fn {file, index} -> - upsert_post_translation_from_rebuild_file(project_id, file, sync_search: false) - :ok = report_rebuild_progress(on_progress, index, total_files, "post files") - end) + if Keyword.get(opts, :reindex_search, true) do + :ok = report_rebuild_phase(on_progress, 0.97, "Refreshing post search index") - if Keyword.get(opts, :reindex_search, true) do - :ok = report_rebuild_phase(on_progress, 0.97, "Refreshing post search index") + :ok = + Search.reindex_posts(project_id, + on_progress: scaled_progress_reporter(on_progress, 0.97, 0.99) + ) + end - :ok = - Search.reindex_posts(project_id, - on_progress: scaled_progress_reporter(on_progress, 0.97, 0.99) - ) + if Keyword.get(opts, :rebuild_embeddings, true) do + :ok = report_rebuild_phase(on_progress, 0.99, "Refreshing post embeddings") + + {:ok, _rebuilt_post_ids} = + Embeddings.rebuild_project(project_id, + on_progress: scaled_progress_reporter(on_progress, 0.99, 1.0) + ) + end + + {:ok, posts} end - - if Keyword.get(opts, :rebuild_embeddings, true) do - :ok = report_rebuild_phase(on_progress, 0.99, "Refreshing post embeddings") - - {:ok, _rebuilt_post_ids} = - Embeddings.rebuild_project(project_id, - on_progress: scaled_progress_reporter(on_progress, 0.99, 1.0) - ) - end - - {:ok, posts} end @spec import_orphan_post_file(String.t(), String.t()) :: @@ -81,20 +83,20 @@ defmodule BDS.Posts.RebuildFromFiles do full_path = Path.join(Projects.project_data_dir(project), relative_path) if File.exists?(full_path) do - rebuild_file = parse_rebuild_file(project, full_path) + with {:ok, rebuild_file} <- parse_rebuild_file(project, full_path) do + if TranslationValidation.translation_rebuild_file?(rebuild_file) do + {:error, :unsupported_file} + else + fields = + rebuild_file.fields + |> Map.put("id", unique_post_id(Map.get(rebuild_file.fields, "id"))) + |> Map.put( + "slug", + Slugs.unique_for_import(project_id, Map.fetch!(rebuild_file.fields, "slug")) + ) - if TranslationValidation.translation_rebuild_file?(rebuild_file) do - {:error, :unsupported_file} - else - fields = - rebuild_file.fields - |> Map.put("id", unique_post_id(Map.get(rebuild_file.fields, "id"))) - |> Map.put( - "slug", - Slugs.unique_for_import(project_id, Map.fetch!(rebuild_file.fields, "slug")) - ) - - {:ok, upsert_post_from_rebuild_file(project_id, %{rebuild_file | fields: fields})} + {:ok, upsert_post_from_rebuild_file(project_id, %{rebuild_file | fields: fields})} + end end else {:error, :not_found} @@ -108,33 +110,33 @@ defmodule BDS.Posts.RebuildFromFiles do full_path = Path.join(Projects.project_data_dir(project), relative_path) if File.exists?(full_path) do - rebuild_file = parse_rebuild_file(project, full_path) + with {:ok, rebuild_file} <- parse_rebuild_file(project, full_path) do + if TranslationValidation.translation_rebuild_file?(rebuild_file) do + source_post_id = Map.fetch!(rebuild_file.fields, "translationFor") + language = TranslationValidation.normalize_language(Map.fetch!(rebuild_file.fields, "language")) - if TranslationValidation.translation_rebuild_file?(rebuild_file) do - source_post_id = Map.fetch!(rebuild_file.fields, "translationFor") - language = TranslationValidation.normalize_language(Map.fetch!(rebuild_file.fields, "language")) + case Repo.get(Post, source_post_id) do + nil -> + {:error, :not_found} - case Repo.get(Post, source_post_id) do - nil -> - {:error, :not_found} + %Post{} = post -> + if TranslationValidation.normalize_language(post.language) == language or + Repo.get_by(Translation, translation_for: source_post_id, language: language) do + {:error, :conflict} + else + fields = Map.put(rebuild_file.fields, "id", Ecto.UUID.generate()) - %Post{} = post -> - if TranslationValidation.normalize_language(post.language) == language or - Repo.get_by(Translation, translation_for: source_post_id, language: language) do - {:error, :conflict} - else - fields = Map.put(rebuild_file.fields, "id", Ecto.UUID.generate()) - - {:ok, - upsert_post_translation_from_rebuild_file( - project_id, - %{rebuild_file | fields: fields}, - sync_search: true - )} + {:ok, + upsert_post_translation_from_rebuild_file( + project_id, + %{rebuild_file | fields: fields}, + sync_search: true + )} + end end + else + {:error, :unsupported_file} end - else - {:error, :unsupported_file} end else {:error, :not_found} @@ -143,8 +145,9 @@ defmodule BDS.Posts.RebuildFromFiles do @doc false def upsert_post_from_file(project_id, project, path) do - rebuild_file = parse_rebuild_file(project, path) - upsert_post_from_rebuild_file(project_id, rebuild_file) + with {:ok, rebuild_file} <- parse_rebuild_file(project, path) do + {:ok, upsert_post_from_rebuild_file(project_id, rebuild_file)} + end end @doc false @@ -244,14 +247,15 @@ defmodule BDS.Posts.RebuildFromFiles do @doc false def parse_rebuild_file(project, path) do - contents = File.read!(path) - {:ok, %{fields: fields}} = Frontmatter.parse_document(contents) - - %{ - path: path, - relative_path: Path.relative_to(path, Projects.project_data_dir(project)), - fields: fields - } + with {:ok, contents} <- read_rebuild_file(path), + {:ok, %{fields: fields}} <- Frontmatter.parse_document(contents) do + {:ok, + %{ + path: path, + relative_path: Path.relative_to(path, Projects.project_data_dir(project)), + fields: fields + }} + end end @doc false @@ -317,4 +321,22 @@ defmodule BDS.Posts.RebuildFromFiles do id end end + + defp collect_rebuild_files(results) do + Enum.reduce_while(results, {:ok, []}, fn + {:ok, rebuild_file}, {:ok, rebuild_files} -> {:cont, {:ok, [rebuild_file | rebuild_files]}} + {:error, reason}, {:ok, _rebuild_files} -> {:halt, {:error, reason}} + end) + |> case do + {:ok, rebuild_files} -> {:ok, Enum.reverse(rebuild_files)} + {:error, reason} -> {:error, reason} + end + end + + defp read_rebuild_file(path) do + case File.read(path) do + {:ok, contents} -> {:ok, contents} + {:error, reason} -> {:error, {:read_rebuild_file, path, reason}} + end + end end diff --git a/lib/bds/posts/translations.ex b/lib/bds/posts/translations.ex index ba48199..22a40b4 100644 --- a/lib/bds/posts/translations.ex +++ b/lib/bds/posts/translations.ex @@ -117,9 +117,8 @@ defmodule BDS.Posts.Translations do project = Projects.get_project!(translation.project_id) full_path = Path.join(Projects.project_data_dir(project), translation.file_path) - if File.exists?(full_path) do - rebuild_file = RebuildFromFiles.parse_rebuild_file(project, full_path) - + with true <- File.exists?(full_path), + {:ok, rebuild_file} <- RebuildFromFiles.parse_rebuild_file(project, full_path) do {:ok, RebuildFromFiles.upsert_post_translation_from_rebuild_file( translation.project_id, @@ -127,7 +126,8 @@ defmodule BDS.Posts.Translations do sync_search: true )} else - {:error, :not_found} + false -> {:error, :not_found} + {:error, reason} -> {:error, reason} end end end diff --git a/test/bds/generation_test.exs b/test/bds/generation_test.exs index 883c21a..7cb7e14 100644 --- a/test/bds/generation_test.exs +++ b/test/bds/generation_test.exs @@ -910,6 +910,20 @@ defmodule BDS.GenerationTest do assert clean_report.updated_post_url_paths == [] end + test "apply_validation returns an error for unreadable generated files", %{ + project: project, + temp_dir: temp_dir + } do + unreadable_path = Path.join([temp_dir, "html", "obsolete", "index.html"]) + File.mkdir_p!(Path.dirname(unreadable_path)) + File.write!(unreadable_path, "obsolete") + File.chmod!(unreadable_path, 0o000) + on_exit(fn -> File.chmod!(unreadable_path, 0o644) end) + + assert {:error, {:read_generated_file, ^unreadable_path, :eacces}} = + BDS.Generation.apply_validation(project.id, [:core]) + end + test "validate_site regenerates sitemap and reports missing, extra, and updated post url paths", %{project: project, temp_dir: temp_dir} do assert {:ok, _metadata} = diff --git a/test/bds/mcp_agent_config_test.exs b/test/bds/mcp_agent_config_test.exs index 655aa46..82681c8 100644 --- a/test/bds/mcp_agent_config_test.exs +++ b/test/bds/mcp_agent_config_test.exs @@ -99,6 +99,28 @@ defmodule BDS.MCPAgentConfigTest do assert written["mcpServers"]["other"] == %{"command" => "python", "args" => ["server.py"]} end + test "add_to_config returns an error for unreadable existing config", %{home_dir: home_dir} do + config_path = Path.join(home_dir, ".claude.json") + File.mkdir_p!(config_path) + + assert {:error, {:read_config, ^config_path, :eisdir}} = + AgentConfig.add_to_config(:claude_code, + home_dir: home_dir, + install_root: Path.join(home_dir, "dist"), + platform: :linux + ) + end + + test "remove_from_config returns an error for unwritable config path", %{home_dir: home_dir} do + config_path = Path.join(home_dir, ".claude.json") + + File.chmod!(home_dir, 0o555) + on_exit(fn -> File.chmod!(home_dir, 0o755) end) + + assert {:error, {:write_config, ^config_path, :eacces}} = + AgentConfig.remove_from_config(:claude_code, home_dir: home_dir) + end + test "packaged executable path resolves inside the distributable payload" do assert AgentConfig.packaged_executable_path("/Applications/bDS2.app/Contents/Resources", :macos) == "/Applications/bDS2.app/Contents/Resources/mcp/bin/bds-mcp" diff --git a/test/bds/media_test.exs b/test/bds/media_test.exs index 7af4c6c..a08c3fc 100644 --- a/test/bds/media_test.exs +++ b/test/bds/media_test.exs @@ -325,6 +325,23 @@ defmodule BDS.MediaTest do end) end + test "rebuild_media_from_files returns an error for unreadable sidecars", %{ + project: project, + temp_dir: temp_dir + } do + media_dir = Path.join([temp_dir, "media", "2026", "04"]) + File.mkdir_p!(media_dir) + + binary_path = Path.join(media_dir, "asset.jpg") + sidecar_path = binary_path <> ".meta" + + File.write!(binary_path, tiny_jpeg_binary()) + File.mkdir_p!(sidecar_path) + + assert {:error, {:read_sidecar, ^sidecar_path, :eisdir}} = + BDS.Media.rebuild_media_from_files(project.id) + end + test "import_media generates the four thumbnail files in bucketed thumbnail paths", %{ project: project, temp_dir: temp_dir diff --git a/test/bds/posts_test.exs b/test/bds/posts_test.exs index c3dcfee..b0cca69 100644 --- a/test/bds/posts_test.exs +++ b/test/bds/posts_test.exs @@ -458,6 +458,17 @@ defmodule BDS.PostsTest do assert post.published_at == 1_036_497_600_000 end + test "rebuild_posts_from_files returns an error for unreadable post files", %{project: project} do + posts_dir = Path.join([BDS.Projects.project_data_dir(project), "posts", "2026", "04"]) + File.mkdir_p!(posts_dir) + + file_path = Path.join(posts_dir, "unreadable.md") + File.mkdir_p!(file_path) + + assert {:error, {:read_rebuild_file, ^file_path, :eisdir}} = + BDS.Posts.rebuild_posts_from_files(project.id) + end + test "rebuild_posts_from_files parses folded multiline title and slug scalars alongside translations" do temp_dir = Path.join(System.tmp_dir!(), "bds-post-rebuild-folded-#{System.unique_integer([:positive])}")