feat: add layered config loading (ToolKit.Config.Layers)

[toshi0806]

概要

registry-manager の Config から設定の読み込み・マージの機構を純関数として抽出し、ToolKit.Config.Layers を追加します。thesis-monitor の Config も参照し、両ツール(rm#74 / tm#50)が採用できる上位集合として設計しています。保持方式(struct / Agent / Application env)と設定スキーマはツール側の責務のため持ち込んでいません。

Resolves #4
親 epic: smkwlab/latex-ecosystem#146

変更内容

• resolve/2 — defaults ⊕ YAML ファイル ⊕ 環境変数 ⊕ CLI オーバーライドの 4 層を後勝ちでマージ
  • ファイル不存在は defaults にフォールバック、パース失敗は {:error, {:parse_error, path}}
  • ファイル内容は defaults をテンプレートに normalize_keys/2 で正規化(string キー → atom、未知キーは落とす。未知キーからの atom 生成なし)
• merge/1 — nil は既存値を上書きしない(未設定扱い)、入れ子 map は再帰マージ(env の 1 変数がファイルの入れ子設定を潰さない = rm issue #38 の挙動)
• read_env/2 — ツール prefix + 型付き spec(string / integer / boolean / string_list)。入れ子 spec と末端セグメントの派生名差し替え(REGISTRY_MANAGER_API_TIMEOUT 型の変数名)に対応。変換失敗は {:error, message}
• 規約ヘルパ — owner_from_repo/1、derive_github_org/2(org 未設定時に repo owner を導出、rm#45 / tm#28 の規約)、valid_owner_repo?/1、conventional_csv_path/2、find_conventional_csv/2(存在チェック込み、rm#16 の規約)、expand_home/2、default_config_path/2
• first_existing/1 — 設定ファイル探索順の解決(tm の「--config → ./config/.yml → ~/.config//config.yml」パターン)

確認済み

• mix format --check-formatted ✔
• mix credo --strict 0 issues ✔
• mix dialyzer passed ✔
• mix test --cover 16 doctests + 121 tests, 0 failures / Total 96.63%(Layers 98.55%)✔
• rm / tm の config テストから優先順位・規約導出・env 変換のケースを移植し、@tag :tmp_dir で YAML 読み込みを検証。System.put_env を使うテストは async: false

https://claude.ai/code/session_015FdaeD1BANgCMUcNTUjKVV

[github-actions]

全体的に設計が明確で、純関数として責務が適切に分離されています。テストカバレッジも充実しており、エッジケースも網羅されています。いくつか指摘事項を挙げます。

[github-actions]

⚠️ [MEDIUM] load_file/1 のTOCTOU競合状態

File.exists?(path) でチェックしてから YamlElixir.read_from_file(path) で読み込むまでの間にファイルが削除される可能性があります（Time-of-check to time-of-use）。

以下のように直接読み込んでエラーをハンドリングする方が安全です：

def load_file(path) do
  case YamlElixir.read_from_file(path) do
    {:ok, config} when is_map(config) -> {:ok, config}
    {:ok, _} -> {:error, {:parse_error, path}}
    {:error, %YamlElixir.FileNotFoundError{}} -> {:ok, %{}}
    {:error, _} -> {:error, {:parse_error, path}}
  end
end

YamlElixir が返すエラー型を確認した上で適切にパターンマッチしてください。

[toshi0806]

commit 35ec43d で修正しました。File.exists? を廃止して直接読み、不存在は {:error, %YamlElixir.FileNotFoundError{}} のパターンマッチで判別するようにしました(deps の yaml_elixir 実装でエラー型を確認済み)。

[github-actions]

ℹ️ [LOW] read_env/2 での throw の使用

throw / catch は Elixir では非慣用的なフロー制御です。Enum.reduce_while/3 や Enum.reduce + with を使った明示的なエラー伝播の方が、Elixir のイディオムに沿っており可読性も高くなります。

例：

defp read_env_map(prefix, spec) do
  Enum.reduce_while(spec, {:ok, %{}}, fn {key, entry}, {:ok, acc} ->
    case put_env_entry(acc, key, entry, prefix) do
      {:ok, new_acc} -> {:cont, {:ok, new_acc}}
      {:error, _} = err -> {:halt, err}
    end
  end)
end

ただし、この変更は put_env_entry 系の関数シグネチャも変更する必要があるため、影響範囲が広い点に注意してください。

[toshi0806]

据え置きます。throw は本モジュール内の private 関数群に閉じており、公開 API の read_env/2 の境界で必ず {:error, message} に変換されます(呼び出し側に漏れません)。入れ子 spec の再帰(read_env_map ⇄ put_env_entry)全段に {:ok, _}/{:error, _} を通す reduce_while 化はシグネチャの複雑化に対して得るものが小さく、抽出元の registry-manager Config.load_env_config も同じ throw/catch パターンです(挙動互換の維持も兼ねています)。

[github-actions]

ℹ️ [LOW] merge_entry/2 における nil の扱いの非対称性

defp merge_entry(acc, {key, nil}), do: Map.put_new(acc, key, nil)

この実装では「nil は既存値を上書きしない」という仕様ですが、defaults 層で %{a: 1} を設定した後に明示的に nil でリセットしたい場合（例：CLIで設定を無効化したい）に手段がありません。

現在の設計判断として意図的であれば、モジュールドキュメントに「nil で既存値をリセットする手段は提供しない」と明記することを推奨します。将来の利用者が混乱する可能性があります。

[toshi0806]

commit 35ec43d で merge/1 の doc に「nil で既存値をリセットする手段は意図的に提供しない(無効化は false / 0 など明示的な値で表現するのがツール側の規約)」と明記しました。

[github-actions]

ℹ️ [LOW] setup での環境変数クリーンアップの堅牢性

setup ブロックで on_exit を登録していますが、@env_vars に列挙されていない環境変数がテスト中に設定された場合（将来のテスト追加時など）はクリーンアップされません。

現状は @env_vars リストが完全に管理されているため問題ありませんが、テストを追加する際に @env_vars の更新を忘れやすいという保守上のリスクがあります。テストごとに使用する変数を明示するか、コメントで注意を促すことを検討してください。

[toshi0806]

commit 35ec43d で @env_vars の直前に「新しい環境変数を使うテストを追加したら必ずここにも追加する」旨の保守コメントを追加しました。

[github-actions]

✨ [POSITIVE] fetch_raw/2 でのアトム優先の挙動がドキュメントと実装で一致しているか確認

defp fetch_raw(raw, key) do
  case Map.fetch(raw, key) do
    {:ok, value} -> {:ok, value}
    :error -> Map.fetch(raw, Atom.to_string(key))
  end
end

key は常に atom（テンプレートのキー）なので、まず atom キーで検索し、なければ string キーで検索するという実装は正しいです。ただし、%{"a" => 1, a: 2} のような混在マップでは atom が優先されます。この挙動はテスト（"an atom key wins when both atom and string keys are present"）でカバーされており問題ありませんが、ドキュメントの「atom が優先」という記述と一致しています。POSITIVE評価です。

[toshi0806]

確認ありがとうございます。ご指摘のとおり doc・実装・テスト(atom 優先)の三者は一致しています。

[github-actions]

全体的に設計が明確で、テストカバレッジも十分です。以下にいくつかの指摘事項を挙げます。

[github-actions]

⚠️ [MEDIUM] エラー伝播に throw/catch を使用していますが、Elixir では with や再帰的な Enum.reduce_while を使った明示的なエラー伝播が慣用的です。throw はスタックを跨ぐ制御フローとして機能しますが、Elixir コードベースでは例外的な用途であり、可読性・保守性の観点から Enum.reduce_while を使って {:error, reason} を明示的に伝播させる方が望ましいです。

defp read_env_map(prefix, spec) do
  Enum.reduce_while(spec, {:ok, %{}}, fn {key, entry}, {:ok, acc} ->
    case put_env_entry(acc, key, entry, prefix) do
      {:ok, new_acc} -> {:cont, {:ok, new_acc}}
      {:error, _} = err -> {:halt, err}
    end
  end)
end

[toshi0806]

据え置きます(同趣旨の指摘 #15 (comment) への返信と同じ根拠です)。throw はモジュール内 private に閉じ、公開 API read_env/2 の境界で {:error, message} に変換されます。入れ子 spec の再帰全段への {:ok,}/{:error,} スレッディングは複雑化に見合わず、抽出元 registry-manager の実装とも挙動互換を保っています。

[github-actions]

⚠️ [MEDIUM] conventional_csv_path/2 と default_config_path/2 はデフォルト引数に System.user_home!/0 を使っており、HOME 環境変数が未設定の場合に関数定義時ではなく呼び出し時に例外が発生します。一方 find_conventional_csv/2 と expand_home/2 は System.user_home/0(nil を返す)を使っており、挙動が非対称です。conventional_csv_path/1 と default_config_path/1 も nil を返す設計に統一するか、ドキュメントで例外が発生しうることを明記することを推奨します。

[toshi0806]

据え置きます。この非対称は意図的な設計です: conventional_csv_path/2・default_config_path/2 は「必ずパスを返す constructor」なので HOME 不在時は fail-fast(user_home!)にし、nil を返して離れた場所の Path.join で落ちるより原因に近い位置で失敗させます。一方 find_conventional_csv/2・expand_home/2 は「規約解決の resolver」で、HOME 不在時は規約導出をスキップ(nil / 素通し)するのが正しい挙動です。この分担は抽出元 registry-manager の conventional_csv_path(user_home!)と apply_csv_convention(user_home)の分担をそのまま踏襲しています。

[github-actions]

ℹ️ [LOW] file_layer/2 の実装は {:error, reason} -> {:error, reason} というパターンマッチが恒等変換になっています。load_file/1 の結果をそのまま with に渡せるため、以下のように簡略化できます。

defp file_layer(path, defaults) do
  with {:ok, raw} <- load_file(path) do
    {:ok, normalize_keys(raw, defaults)}
  end
end

[toshi0806]

据え置きます。提案の単一節 with は本リポジトリの credo --strict で有効な Credo.Check.Readability.WithSingleClause に抵触するため採用できません(case での明示分岐が credo green を保つ書き方です)。

[github-actions]

ℹ️ [LOW] load_file/1 の _ ワイルドカード節は、YamlElixir が将来新しいエラー型を追加した場合に、パース失敗と区別できない形で捕捉してしまいます。少なくとも {:error, _} と {:ok, _} (非マップ) を明示的に分けることを検討してください。

case YamlElixir.read_from_file(path) do
  {:ok, config} when is_map(config) -> {:ok, config}
  {:ok, _non_map} -> {:error, {:parse_error, path}}
  {:error, %YamlElixir.FileNotFoundError{}} -> {:ok, %{}}
  {:error, _} -> {:error, {:parse_error, path}}
end

[toshi0806]

据え置きます。提案の {:ok, _non_map} と {:error, _} はどちらも同一の {:error, {:parse_error, path}} を返すため、分けても挙動は変わりません。将来 YamlElixir が新しいエラー型を追加した場合も「mapping として読めなかった」ことに変わりはなく、parse_error への縮退が正しい扱いです。

[github-actions]

ℹ️ [LOW] テストファイル冒頭のコメント「新しい環境変数を使うテストを追加したら、必ずここにも追加すること」は、手動管理が必要なリストへの依存を示しています。setup 内で @env_spec から動的に変数名を導出してクリーンアップする方法を検討すると、追加漏れのリスクを排除できます。

[toshi0806]

据え置きます。@env_spec には末端セグメント差し替え({:integer, "TIMEOUT"})が含まれ、spec から変数名を動的導出するにはテスト対象そのものの派生ロジックを再実装することになり、名前導出のバグをテスト側が隠してしまいます。クリーンアップ対象は実装から独立した明示リストで管理し、保守コメントで追加漏れを防ぐ方針です。

[toshi0806]

AI レビュー対応まとめ

指摘 10 件(MEDIUM 3 / LOW 6 / POSITIVE 1)に対応しました。

修正(commit 35ec43d)

• [MEDIUM] load_file/1 の TOCTOU — File.exists? を廃止し、直接読み込んで YamlElixir.FileNotFoundError のパターンマッチで不存在を判別
• [LOW] nil の非対称性 — merge/1 doc に「nil でのリセット手段は意図的に提供しない」を明記
• [LOW] env テストのクリーンアップ — @env_vars の保守コメントを追加

据え置き(各スレッドに根拠を返信)

• [MEDIUM/LOW] throw/catch(2 件) — モジュール内に閉じ read_env/2 境界で {:error, _} に変換。抽出元 registry-manager と同パターンで挙動互換を維持
• [MEDIUM] user_home! と user_home の非対称 — constructor は fail-fast、resolver は nil スキップという意図的な分担(registry-manager の分担を踏襲)
• [LOW] 単一節 with 化 — credo --strict の WithSingleClause に抵触するため採用不可
• [LOW] ワイルドカード節の分割 — 分けても同一値を返し挙動不変。将来のエラー型も parse_error への縮退が正しい
• [LOW] @env_vars の動的導出 — テスト対象の名前導出ロジックの再実装になりバグを隠すため、明示リストで管理

最新 commit 35ec43d で Elixir CI / AI Code Review とも success です。