test: assert that Castle ships no Mix tasks

AGENTS.md

@@ -1615,6 +1615,75 @@ underneath it, and answers with the new value. What lets it is that materialisin
 leaves no `config_provider_booted` marker behind and preserves the header Mix
 wrote, so Elixir's pipeline is still armed in the file the launcher reads.
 
+`test/castle/no_mix_tasks_test.exs` asserts something about the *pair* rather
+than about any of Castle's behaviour: **Castle ships no Mix tasks.** Every
+build-time task lives in Forecastle, whatever it is called — and since
+[forecastle#24](https://github.com/ausimian/forecastle/issues/24) they are called
+`castle.*`, because the namespace follows the vocabulary a developer thinks in
+rather than the package that implements them. That leaves `Mix.Tasks.Castle.*`
+a namespace both projects could write into, and Mix resolves a task by module
+name alone, so a module defined on both sides would be decided by whichever
+`ebin` came first on the code path, with nothing to say which had won.
+
+It reads the `ebin` rather than the application metadata, and the difference is
+not pedantry. `Mix.Task.load_all/0` walks `:code.get_path()` and matches each
+directory entry against `Elixir.Mix.Tasks.<name>.beam` — a filename, with no
+reference to a `.app` anywhere in it — so the beams are what the invariant is
+about, and `Mix.Project.compile_path/0` is where Castle's own beams are. The
+first version of this test asked `Application.spec(:castle, :modules)` instead
+and would have passed while Castle shipped a task: `Mix.Tasks.Compile.App` fills
+`:modules` in with `Keyword.put_new_lazy/3`, so a project that supplies its own
+list in `application/0` keeps it. Adding `modules: [Castle]` there and a task
+under `lib/mix/tasks/` produces a tree where the metadata says `[Castle]`, the
+beam sits in `ebin`, and `Mix.Task.load_all/0` finds the task — which is the
+whole hazard, reported clean.
+
+Scoping to Castle's *own* `ebin` rather than the code path is the other half:
+Castle takes Forecastle as a build-time dependency, so Forecastle's `ebin` is on
+the code path during this very test and `Mix.Tasks.Castle.Relup` is in it. That
+one is Forecastle's and is supposed to be there. A code-path check would fail on
+it; this one is scoped to the only side of the collision this project controls.
+
+There are two cases because a beam check can only see the environment that
+compiled it. `mix test` compiles one, and a module behind a `Mix.env()`
+condition would be absent there and present elsewhere, so the source is checked
+too. That second case reads the source with **Elixir's parser**, not a regex,
+and the reason is a concrete miss rather than taste: `defmodule(Mix.Tasks.X)` is
+ordinary Elixir that `mix format` preserves, and a pattern anchored on
+whitespace after `defmodule` never matched it — put that inside a `:prod` branch
+and *both* checks reported clean on a tree that ships a task. Relaxing the
+pattern only trades the miss for the opposite error, since module-looking prose
+in a `@moduledoc` would start matching. A `defmodule` is an AST node whatever
+the spacing and parenthesisation, and a heredoc is a binary in that AST rather
+than a node, so asking the parser settles both directions at once. Two cases in
+the file pin exactly those two, because both were live bugs in the first draft.
+
+Each check guards against looking at nothing: `Elixir.Castle.beam` must be among
+the entries, and `castle.ex` among the sources, because an empty directory
+filters to no tasks and that is indistinguishable from a clean result.
+
+**What the source check enforces is narrower than "no task in `lib`", and the
+difference is on the record rather than assumed.** It matches the two forms a
+module name is *written* in — an alias, `defmodule Mix.Tasks.X`, and a literal
+atom, `defmodule :"Elixir.Mix.Tasks.X"`. It does not *resolve* names, so
+`alias Mix.Tasks, as: N` followed by `defmodule N.Castle.X` defines the module
+and is invisible to it, as are a name built by `Module.concat/1`, one produced
+by a macro, and `Module.create/3`. Doing better means implementing alias scoping
+and constant folding inside a test, which is a compiler; the stopping point is
+to say so, and a case in the file pins the aliased form as a known limit so it
+reads as a decision rather than a gap someone rediscovers.
+
+That limit costs less than it looks, because the two checks fail differently.
+The beam check cannot be fooled by *any* of those forms — each still writes
+`Elixir.Mix.Tasks.<name>.beam` into `ebin`, which is the file Mix actually
+reads — so the only state that escapes both is a module named indirectly **and**
+compiled only in an environment `mix test` does not build. Reaching it is not a
+slip; it is circumvention of an invariant stated in words here, in the test, and
+in `design/upgrade-tooling.md`. Closing it would take a clean `MIX_ENV=prod`
+build scanned for task beams, which is a publish-time gate rather than a test,
+and it is not built. What this guards is the accident — someone adding
+`lib/mix/tasks/foo.ex` because it looked like the natural home for it.
+
 ### What `mix test --cover` measures
 
 `mix test --cover` measures `lib`, and `test_coverage` in `mix.exs` names the

test/castle/no_mix_tasks_test.exs

@@ -0,0 +1,221 @@
+defmodule Castle.NoMixTasksTest do
+  # **Castle ships no Mix tasks.** Every build-time task lives in Forecastle,
+  # whatever it is called.
+  #
+  # The two halves divide one job between them, and since forecastle#24 the
+  # build-time tasks are named `castle.*` while still being compiled into
+  # Forecastle: the namespace follows the vocabulary a developer thinks in
+  # rather than the package that implements it. That leaves `Mix.Tasks.Castle.*`
+  # a namespace both projects could write into, and Mix resolves a task by
+  # module name alone. If both ever defined the same module, whichever `ebin`
+  # came first on the code path would win - and nothing would say which, not the
+  # compiler, not Mix, not the release.
+  #
+  # A convention nobody enforces is a convention that lapses, so it is asserted
+  # here rather than remembered. That is the same move the project already makes
+  # for the coverage floor, for `verify_relup!/2` and for the
+  # `restart_new_emulator` refusal.
+  use ExUnit.Case, async: true
+
+  # Mix's own rule, and deliberately the same one. `Mix.Task.load_all/0` walks
+  # `:code.get_path()` and matches each directory entry against
+  # `Elixir.Mix.Tasks.<name>.beam` - a filename, in an `ebin`, with no reference
+  # to application metadata anywhere in it.
+  @task_beam ~r/^Elixir\.Mix\.Tasks\..+\.beam$/
+
+  # A sample for the heredoc case below. `\"\"\"` is an escaped delimiter rather
+  # than a nested heredoc, so `mix format` leaves it alone.
+  @heredoc_sample """
+  defmodule Castle.Docs do
+    @moduledoc \"\"\"
+    Do not do this:
+
+        defmodule Mix.Tasks.Castle.Nope do
+        end
+    \"\"\"
+  end
+  """
+
+  # `Application.spec(:castle, :modules)` was the obvious source here and is the
+  # wrong one. `Mix.Tasks.Compile.App` fills `:modules` in with
+  # `Keyword.put_new_lazy/3`, so a project that supplies its own list in
+  # `application/0` *keeps* it - the metadata then says whatever that list says,
+  # while Mix goes on finding every task beam in the directory regardless. The
+  # metadata is a description of the artefact that something else is allowed to
+  # write; the artefact is what Mix reads.
+  #
+  # `Mix.Project.compile_path/0` is Castle's *own* `ebin`, which is the point:
+  # Castle takes Forecastle as a build-time dependency, so Forecastle's `ebin` is
+  # on the code path during this very test and `Mix.Tasks.Castle.Relup` is in it.
+  # That is Forecastle's and is supposed to be there. A check over the code path
+  # would fail on it; this one is scoped to the only side of the collision this
+  # project controls.
+  test "no Mix task beam is compiled into Castle's ebin" do
+    entries = File.ls!(Mix.Project.compile_path())
+
+    # Without this the check could pass by looking at the wrong directory, or at
+    # one a failed build left empty: no entries filters to no tasks, which is
+    # indistinguishable from a clean result.
+    assert "Elixir.Castle.beam" in entries,
+           "#{Mix.Project.compile_path()} does not contain Elixir.Castle.beam, so this " <>
+             "test was not looking at Castle's compiled output"
+
+    tasks = Enum.filter(entries, &Regex.match?(@task_beam, &1))
+
+    assert tasks == [], """
+    Castle must ship no Mix tasks, and these beams are in its ebin:
+
+    #{Enum.map_join(tasks, "\n", &"        #{&1}")}
+
+    Mix finds a task by looking for exactly this filename on the code path, so a
+    task beam here shares a namespace with the build-time tasks Forecastle
+    compiles into its own ebin - which are called `castle.*` precisely because
+    the name follows the user's vocabulary and not the package implementing it.
+    If both projects ever define the same module, whichever ebin comes first
+    wins, silently.
+
+    Every build-time task lives in Forecastle, whatever it is called. Move this
+    one there - it can keep the `castle.*` name it has.
+    """
+  end
+
+  # The beam check can only see what this environment compiled, and `mix test`
+  # compiles one. A module defined behind a `Mix.env()` condition would be
+  # absent from that build and present in another, so the source is checked too:
+  # it is the one form of the answer that does not depend on which environment
+  # asked the question.
+  test "no Mix task is defined in Castle's lib" do
+    lib = Mix.Project.project_file() |> Path.dirname() |> Path.join("lib")
+    sources = Path.wildcard(Path.join(lib, "**/*.ex"))
+
+    assert Path.join(lib, "castle.ex") in sources,
+           "#{lib} does not contain castle.ex, so this test was not looking at Castle's source"
+
+    defined =
+      Enum.flat_map(sources, fn path ->
+        path |> File.read!() |> task_definitions(Path.relative_to(path, lib))
+      end)
+
+    assert defined == [], """
+    Castle must ship no Mix tasks, and these are defined in its lib:
+
+    #{Enum.map_join(defined, "\n", &"        #{&1}")}
+
+    Every build-time task lives in Forecastle, whatever it is called. Move this
+    one there - it can keep the `castle.*` name it has.
+    """
+  end
+
+  # Both of these are about the detector rather than about Castle, and they are
+  # here because the first version of it was a regex over lines and was wrong in
+  # both directions.
+  #
+  # `defmodule(Mix.Tasks.Castle.Hidden)` is ordinary Elixir that `mix format`
+  # preserves, and a regex anchored on whitespace after `defmodule` never saw
+  # it. Wrapped in an environment branch it is invisible to the beam check too,
+  # so between them the two checks reported clean on a tree that ships a task -
+  # exactly the hazard this file exists to prevent.
+  test "the source check sees a parenthesised definition inside an environment branch" do
+    source = """
+    defmodule Castle.Conditional do
+      if Mix.env() == :prod do
+        defmodule(Mix.Tasks.Castle.Hidden) do
+          use Mix.Task
+        end
+      end
+    end
+    """
+
+    assert task_definitions(source, "conditional.ex") == [
+             "conditional.ex:3: Mix.Tasks.Castle.Hidden"
+           ]
+  end
+
+  # And the other direction: relaxing that regex would have started matching
+  # module-looking prose. Documentation showing what not to do is a string
+  # literal rather than a definition, and the parser is what knows the
+  # difference.
+  test "the source check ignores module-looking text in a heredoc" do
+    assert task_definitions(@heredoc_sample, "docs.ex") == []
+  end
+
+  # The other written form of a module name.
+  test "the source check sees a literal atom module name" do
+    source = ~S"""
+    defmodule :"Elixir.Mix.Tasks.Castle.Atom" do
+      use Mix.Task
+    end
+    """
+
+    assert task_definitions(source, "atom.ex") == ["atom.ex:1: Mix.Tasks.Castle.Atom"]
+  end
+
+  # A limit, pinned so that it is a decision on the record rather than a gap
+  # someone rediscovers. `alias Mix.Tasks, as: N` and then `defmodule N.Castle.X`
+  # defines `Mix.Tasks.Castle.X`, and nothing short of alias resolution sees it.
+  # The beam check does, for the environment it compiled - see the note on
+  # `task_definitions/2` for why that is where this stops.
+  test "the source check does not resolve aliases, and says so" do
+    source = ~S"""
+    alias Mix.Tasks, as: N
+
+    defmodule N.Castle.Aliased do
+      use Mix.Task
+    end
+    """
+
+    assert task_definitions(source, "aliased.ex") == []
+  end
+
+  # Elixir's own parser rather than a pattern over the text. A `defmodule` is an
+  # AST node whatever the spacing, parenthesisation or line breaks around it
+  # are, and text inside a string literal is a binary in that AST rather than a
+  # node - so both of the cases above fall out of asking the parser instead of
+  # the characters. It matches the two forms a module name is *written* in: an
+  # alias (`Mix.Tasks.X`) and a literal atom (`:"Elixir.Mix.Tasks.X"`).
+  #
+  # **What it does not do is resolve names, and that is a deliberate limit
+  # rather than an oversight.** `alias Mix.Tasks, as: N` followed by
+  # `defmodule N.Castle.X` names the same module and is invisible here, as are a
+  # name built by `Module.concat/1`, one produced by a macro, and
+  # `Module.create/3`. Resolving those means implementing alias scoping and
+  # constant folding - a compiler, in a test - and the honest stopping point is
+  # to say so instead.
+  #
+  # It costs less than it looks. The beam check is exact and cannot be fooled by
+  # *any* of them, because every one still writes
+  # `Elixir.Mix.Tasks.<name>.beam` into `ebin`, which is the file Mix reads. The
+  # only gap the pair leaves is a module named indirectly *and* compiled only in
+  # an environment `mix test` does not build, and reaching that state is not a
+  # mistake anyone makes by accident - it is circumvention of an invariant this
+  # file, `AGENTS.md` and `design/upgrade-tooling.md` all state in words. This
+  # guards against the slip, which is someone adding `lib/mix/tasks/foo.ex`
+  # because it seemed like the natural home for it.
+  defp task_definitions(source, label) do
+    ast =
+      case Code.string_to_quoted(source) do
+        {:ok, ast} -> ast
+        {:error, reason} -> flunk("#{label} does not parse: #{inspect(reason)}")
+      end
+
+    {_ast, found} =
+      Macro.prewalk(ast, [], fn
+        {:defmodule, meta, [{:__aliases__, _, [:Mix, :Tasks | _] = segments} | _]} = node, acc ->
+          {node, [{line(meta), Module.concat(segments)} | acc]}
+
+        {:defmodule, meta, [module | _]} = node, acc when is_atom(module) ->
+          if match?("Elixir.Mix.Tasks." <> _, Atom.to_string(module)),
+            do: {node, [{line(meta), module} | acc]},
+            else: {node, acc}
+
+        node, acc ->
+          {node, acc}
+      end)
+
+    found
+    |> Enum.reverse()
+    |> Enum.map(fn {line, module} -> "#{label}:#{line}: #{inspect(module)}" end)
+  end
+
+  defp line(meta), do: Keyword.get(meta, :line, 0)
+end