diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index 8a5cfd5..a8c0620 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -2,6 +2,8 @@ # Mix.Task behaviour info not available in dialyzer context {"lib/mix/tasks/jido_shell.ex", :callback_info_missing}, {"lib/mix/tasks/jido_shell.ex", :unknown_function}, + {"lib/mix/tasks/jido_shell.guardrails.ex", :callback_info_missing}, + {"lib/mix/tasks/jido_shell.guardrails.ex", :unknown_function}, {"lib/mix/tasks/jido_shell.install.ex", :callback_info_missing}, {"lib/mix/tasks/jido_shell.install.ex", :unknown_function} ] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a7d901a..fef136c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,6 +11,14 @@ concurrency: cancel-in-progress: true jobs: + guardrails: + name: Guardrails + uses: agentjido/github-actions/.github/workflows/elixir-test.yml@main + with: + otp_versions: '["27"]' + elixir_versions: '["1.18"]' + test_command: mix jido_shell.guardrails + lint: name: Lint uses: agentjido/github-actions/.github/workflows/elixir-lint.yml@main diff --git a/AGENTS.md b/AGENTS.md index 19b1c4f..50d4e69 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,7 @@ mix compile --warnings-as-errors mix test mix test --include flaky mix coveralls +mix jido_shell.guardrails mix quality mix docs mix jido_shell diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 179bcf3..b00bbd1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,12 +16,15 @@ mix test Run before opening a PR: ```bash +mix jido_shell.guardrails mix quality mix test mix test --include flaky mix coveralls ``` +`mix quality` runs `mix jido_shell.guardrails` first, so namespace/layout regressions fail early in scripted workflows. + ## Common Commands ```bash @@ -38,6 +41,7 @@ mix docs 2. Add tests for behavior changes. 3. Keep docs and examples in sync. 4. Use conventional commits. +5. If module namespace/layout conventions change, update guardrails and guardrail tests in the same PR. Examples: diff --git a/lib/jido/shell/guardrails.ex b/lib/jido/shell/guardrails.ex new file mode 100644 index 0000000..63350a1 --- /dev/null +++ b/lib/jido/shell/guardrails.ex @@ -0,0 +1,70 @@ +defmodule Jido.Shell.Guardrails do + @moduledoc """ + Runs namespace and layout guardrails for the `jido_shell` codebase. + """ + + alias Jido.Shell.Guardrails.Violation + + @default_rules [ + Jido.Shell.Guardrails.Rules.ForbiddenPaths, + Jido.Shell.Guardrails.Rules.RequiredFiles, + Jido.Shell.Guardrails.Rules.NamespacePrefixes + ] + + @type options :: [ + root: String.t(), + rules: [module()] + ] + + @spec check(options()) :: :ok | {:error, [Violation.t()]} + def check(opts \\ []) do + context = %{root: normalize_root(opts)} + + violations = + opts + |> rules() + |> Enum.flat_map(&run_rule(&1, context)) + |> Enum.sort_by(fn %Violation{file: file, message: message} -> + {file || "", message} + end) + + case violations do + [] -> :ok + _ -> {:error, violations} + end + end + + @spec default_rules() :: [module()] + def default_rules, do: @default_rules + + @spec format_violations([Violation.t()]) :: String.t() + def format_violations(violations) do + Enum.map_join(violations, "\n", fn %Violation{rule: rule, file: file, message: message} -> + location = if file, do: " (#{file})", else: "" + "[#{inspect(rule)}]#{location} #{message}" + end) + end + + defp normalize_root(opts) do + opts + |> Keyword.get(:root, File.cwd!()) + |> Path.expand() + end + + defp rules(opts) do + Keyword.get(opts, :rules, @default_rules) + end + + defp run_rule(rule, context) when is_atom(rule) do + Code.ensure_loaded(rule) + + unless function_exported?(rule, :check, 1) do + raise ArgumentError, "guardrail rule #{inspect(rule)} must define check/1" + end + + case rule.check(context) do + :ok -> [] + violations when is_list(violations) -> violations + end + end +end diff --git a/lib/jido/shell/guardrails/rule.ex b/lib/jido/shell/guardrails/rule.ex new file mode 100644 index 0000000..04b103a --- /dev/null +++ b/lib/jido/shell/guardrails/rule.ex @@ -0,0 +1,13 @@ +defmodule Jido.Shell.Guardrails.Rule do + @moduledoc """ + Behaviour for guardrail rules. + """ + + alias Jido.Shell.Guardrails.Violation + + @type context :: %{ + root: String.t() + } + + @callback check(context()) :: :ok | [Violation.t()] +end diff --git a/lib/jido/shell/guardrails/rules/forbidden_paths.ex b/lib/jido/shell/guardrails/rules/forbidden_paths.ex new file mode 100644 index 0000000..400f2d6 --- /dev/null +++ b/lib/jido/shell/guardrails/rules/forbidden_paths.ex @@ -0,0 +1,25 @@ +defmodule Jido.Shell.Guardrails.Rules.ForbiddenPaths do + @moduledoc false + @behaviour Jido.Shell.Guardrails.Rule + + alias Jido.Shell.Guardrails.Violation + + @forbidden_paths [ + "lib/kodo", + "test/kodo", + "lib/mix/tasks/kodo.ui.ex" + ] + + @impl true + def check(%{root: root}) do + @forbidden_paths + |> Enum.filter(&(root |> Path.join(&1) |> File.exists?())) + |> Enum.map(fn path -> + %Violation{ + rule: __MODULE__, + file: path, + message: "legacy namespace path exists: #{path}" + } + end) + end +end diff --git a/lib/jido/shell/guardrails/rules/namespace_prefixes.ex b/lib/jido/shell/guardrails/rules/namespace_prefixes.ex new file mode 100644 index 0000000..4c81046 --- /dev/null +++ b/lib/jido/shell/guardrails/rules/namespace_prefixes.ex @@ -0,0 +1,61 @@ +defmodule Jido.Shell.Guardrails.Rules.NamespacePrefixes do + @moduledoc false + @behaviour Jido.Shell.Guardrails.Rule + + alias Jido.Shell.Guardrails.Violation + + @module_pattern ~r/^\s*defmodule\s+([A-Za-z0-9_.]+)\s+do/m + + @file_prefix_rules [ + {"lib/jido/shell/**/*.ex", "Jido.Shell"}, + {"lib/mix/tasks/jido_shell*.ex", "Mix.Tasks.JidoShell"} + ] + + @impl true + def check(%{root: root}) do + Enum.flat_map(@file_prefix_rules, fn {glob, prefix} -> + root + |> Path.join(glob) + |> Path.wildcard() + |> Enum.flat_map(fn file -> + relative = Path.relative_to(file, root) + file_namespace_violations(file, relative, prefix) + end) + end) + end + + defp file_namespace_violations(file, relative, prefix) do + modules = + file + |> File.read!() + |> modules_in_file() + + case modules do + [] -> + [ + %Violation{ + rule: __MODULE__, + file: relative, + message: "no module definition found for namespace check" + } + ] + + _ -> + modules + |> Enum.reject(&String.starts_with?(&1, prefix)) + |> Enum.map(fn module_name -> + %Violation{ + rule: __MODULE__, + file: relative, + message: "module #{module_name} does not use expected prefix #{prefix}" + } + end) + end + end + + defp modules_in_file(contents) do + @module_pattern + |> Regex.scan(contents, capture: :all_but_first) + |> Enum.map(fn [module_name] -> module_name end) + end +end diff --git a/lib/jido/shell/guardrails/rules/required_files.ex b/lib/jido/shell/guardrails/rules/required_files.ex new file mode 100644 index 0000000..46ae51d --- /dev/null +++ b/lib/jido/shell/guardrails/rules/required_files.ex @@ -0,0 +1,59 @@ +defmodule Jido.Shell.Guardrails.Rules.RequiredFiles do + @moduledoc false + @behaviour Jido.Shell.Guardrails.Rule + + alias Jido.Shell.Guardrails.Violation + + @required_content %{ + "lib/jido/shell/shell_session.ex" => [ + "defmodule Jido.Shell.ShellSession do" + ], + "lib/jido/shell/shell_session_server.ex" => [ + "defmodule Jido.Shell.ShellSessionServer do" + ], + "lib/jido/shell/shell_session/state.ex" => [ + "defmodule Jido.Shell.ShellSession.State do" + ], + "lib/jido/shell/session.ex" => [ + "defmodule Jido.Shell.Session do", + "@moduledoc deprecated:" + ], + "lib/jido/shell/session_server.ex" => [ + "defmodule Jido.Shell.SessionServer do", + "@moduledoc deprecated:" + ], + "lib/jido/shell/session/state.ex" => [ + "defmodule Jido.Shell.Session.State do", + "@moduledoc deprecated:" + ] + } + + @impl true + def check(%{root: root}) do + Enum.flat_map(@required_content, fn {relative_path, snippets} -> + full_path = Path.join(root, relative_path) + + case File.read(full_path) do + {:ok, contents} -> + snippets + |> Enum.reject(&String.contains?(contents, &1)) + |> Enum.map(fn snippet -> + %Violation{ + rule: __MODULE__, + file: relative_path, + message: "missing expected content: #{snippet}" + } + end) + + {:error, _reason} -> + [ + %Violation{ + rule: __MODULE__, + file: relative_path, + message: "missing required file" + } + ] + end + end) + end +end diff --git a/lib/jido/shell/guardrails/violation.ex b/lib/jido/shell/guardrails/violation.ex new file mode 100644 index 0000000..09ad197 --- /dev/null +++ b/lib/jido/shell/guardrails/violation.ex @@ -0,0 +1,14 @@ +defmodule Jido.Shell.Guardrails.Violation do + @moduledoc """ + Represents a single guardrail violation. + """ + + @enforce_keys [:rule, :message] + defstruct [:rule, :message, :file] + + @type t :: %__MODULE__{ + rule: module(), + message: String.t(), + file: String.t() | nil + } +end diff --git a/lib/mix/tasks/jido_shell.guardrails.ex b/lib/mix/tasks/jido_shell.guardrails.ex new file mode 100644 index 0000000..9568750 --- /dev/null +++ b/lib/mix/tasks/jido_shell.guardrails.ex @@ -0,0 +1,39 @@ +defmodule Mix.Tasks.JidoShell.Guardrails do + @moduledoc """ + Verifies namespace and file layout guardrails for this repository. + + ## Usage + + mix jido_shell.guardrails + mix jido_shell.guardrails --root /path/to/repo + """ + + use Mix.Task + + @shortdoc "Check namespace/layout guardrails" + + @impl Mix.Task + def run(args) do + {opts, _argv, _invalid} = + OptionParser.parse(args, + strict: [root: :string] + ) + + root = Keyword.get(opts, :root, File.cwd!()) + + case Jido.Shell.Guardrails.check(root: root) do + :ok -> + Mix.shell().info("jido_shell guardrails: ok") + :ok + + {:error, violations} -> + formatted = Jido.Shell.Guardrails.format_violations(violations) + + Mix.raise(""" + jido_shell guardrails failed: + + #{formatted} + """) + end + end +end diff --git a/mix.exs b/mix.exs index ac5c707..3020023 100644 --- a/mix.exs +++ b/mix.exs @@ -95,6 +95,7 @@ defmodule Jido.Shell.MixProject do test: "test --exclude flaky", q: ["quality"], quality: [ + "jido_shell.guardrails", "format --check-formatted", "compile --warnings-as-errors", "credo --min-priority higher", diff --git a/test/jido/shell/guardrails_test.exs b/test/jido/shell/guardrails_test.exs new file mode 100644 index 0000000..a397dee --- /dev/null +++ b/test/jido/shell/guardrails_test.exs @@ -0,0 +1,64 @@ +defmodule Jido.Shell.GuardrailsTest do + use ExUnit.Case, async: true + + alias Jido.Shell.Guardrails + alias Jido.Shell.Guardrails.Rules.ForbiddenPaths + alias Jido.Shell.Guardrails.Rules.NamespacePrefixes + alias Jido.Shell.Guardrails.Rules.RequiredFiles + alias Jido.Shell.Guardrails.Violation + + test "passes for current repository layout" do + assert :ok = Guardrails.check(root: File.cwd!()) + end + + test "forbidden path rule flags legacy namespace paths" do + with_tmp_dir(fn root -> + legacy_file = Path.join(root, "lib/kodo/transport/term_ui.ex") + File.mkdir_p!(Path.dirname(legacy_file)) + File.write!(legacy_file, "defmodule Kodo.Transport.TermUI do\nend\n") + + assert {:error, violations} = Guardrails.check(root: root, rules: [ForbiddenPaths]) + + assert Enum.any?(violations, fn %Violation{file: file, message: message} -> + file == "lib/kodo" and String.contains?(message, "legacy namespace path exists") + end) + end) + end + + test "required file rule flags missing canonical modules" do + with_tmp_dir(fn root -> + assert {:error, violations} = Guardrails.check(root: root, rules: [RequiredFiles]) + + assert Enum.any?(violations, fn %Violation{file: file, message: message} -> + file == "lib/jido/shell/shell_session.ex" and + String.contains?(message, "missing required file") + end) + end) + end + + test "namespace prefix rule flags incorrect module prefixes" do + with_tmp_dir(fn root -> + path = Path.join(root, "lib/jido/shell/command/bad.ex") + File.mkdir_p!(Path.dirname(path)) + File.write!(path, "defmodule Kodo.Command.Bad do\nend\n") + + assert {:error, violations} = Guardrails.check(root: root, rules: [NamespacePrefixes]) + + assert Enum.any?(violations, fn %Violation{file: file, message: message} -> + file == "lib/jido/shell/command/bad.ex" and + String.contains?(message, "expected prefix Jido.Shell") + end) + end) + end + + defp with_tmp_dir(fun) do + root = Path.join(System.tmp_dir!(), "jido-shell-guardrails-#{System.unique_integer([:positive])}") + File.mkdir_p!(root) + + try do + fun.(root) + after + File.rm_rf!(root) + end + end +end diff --git a/test/mix/tasks/jido_shell.guardrails_test.exs b/test/mix/tasks/jido_shell.guardrails_test.exs new file mode 100644 index 0000000..5b29119 --- /dev/null +++ b/test/mix/tasks/jido_shell.guardrails_test.exs @@ -0,0 +1,41 @@ +defmodule Mix.Tasks.JidoShell.GuardrailsTest do + use ExUnit.Case, async: false + import ExUnit.CaptureIO + + setup do + Mix.Task.reenable("jido_shell.guardrails") + :ok + end + + test "succeeds for current repository" do + output = + capture_io(fn -> + assert :ok = Mix.Tasks.JidoShell.Guardrails.run(["--root", File.cwd!()]) + end) + + assert output =~ "jido_shell guardrails: ok" + end + + test "raises with formatted violations when guardrails fail" do + with_tmp_dir(fn root -> + legacy_file = Path.join(root, "lib/kodo/transport/term_ui.ex") + File.mkdir_p!(Path.dirname(legacy_file)) + File.write!(legacy_file, "defmodule Kodo.Transport.TermUI do\nend\n") + + assert_raise Mix.Error, ~r/jido_shell guardrails failed/, fn -> + Mix.Tasks.JidoShell.Guardrails.run(["--root", root]) + end + end) + end + + defp with_tmp_dir(fun) do + root = Path.join(System.tmp_dir!(), "jido-shell-mix-guardrails-#{System.unique_integer([:positive])}") + File.mkdir_p!(root) + + try do + fun.(root) + after + File.rm_rf!(root) + end + end +end