test: assert that Castle ships no Mix tasks - #35
Merged
Conversation
Since forecastle#24 the build-time tasks are named `castle.*` while still being compiled into Forecastle, because the namespace follows the vocabulary a developer thinks in rather than the package implementing 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 the containment is asserted rather than remembered, the same way the coverage floor and the `restart_new_emulator` refusal are. The beam check reads Castle's own `ebin` - `Mix.Project.compile_path()` - and rejects any `Elixir.Mix.Tasks.*.beam` in it. That filename is Mix's own discovery rule: `Mix.Task.load_all/0` scans the code path for it and consults no application metadata at all. Scoping to Castle's `ebin` rather than the code path is the point, since Forecastle's is on that path during the test and its `Mix.Tasks.Castle.Relup` belongs there. A source check covers what a beam check structurally cannot - a module compiled only in an environment `mix test` does not build - by parsing `lib` and matching literal `defmodule` nodes. It matches the two forms a name is written in, an alias and a literal atom, and deliberately does not resolve names: `alias Mix.Tasks, as: N` then `defmodule N.Castle.X` is invisible to it, and a case pins that as a known limit rather than a gap to rediscover. Resolving it means alias scoping and constant folding inside a test. The beam check cannot be fooled by any of those forms, so what escapes both is a module named indirectly *and* built only in another environment - circumvention rather than the accident this guards. Both assert they are looking at something before concluding it is clean, because an empty directory filters to no tasks and reads exactly like a clean result. Closes #33 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd096cd42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`defmodule Elixir.Mix.Tasks.Castle.X` names the same module as `defmodule Mix.Tasks.Castle.X`, but parses to an alias with an extra leading segment - `[Elixir, :Mix, :Tasks, ...]` rather than `[:Mix, :Tasks, ...]` - so the matcher walked straight past it. Guarded by a `Mix.env()` branch it was invisible to the beam check as well, which is the state the two checks exist to make impossible between them. Recognition now strips the leading `Elixir` before testing the segments. `Module.concat/1` already renders both spellings as `Mix.Tasks.Castle.X`, so only the recognition needed to know about the prefix, and the reported name is unchanged. A case pins the qualified form inside an environment branch, alongside the parenthesised and literal-atom ones. Verified against a planted `lib/mix/tasks/qualified.ex`, which the source check now reports and the test-environment beam check still cannot see. Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #33. Part of #32; rationale in
design/upgrade-tooling.md§D1.What the issue asked for
ausimian/forecastle#24 named the build-time tasks
mix castle.*while leaving themcompiled inside Forecastle, so
Mix.Tasks.Castle.*is a module namespace bothpackages could write into. Mix resolves a task by module name alone, so if both ever
defined the same module, whichever
ebincame first on the code path would win —silently, with nothing from the compiler, Mix or the release to say which.
A convention nobody enforces is a convention that lapses, so it fails a build instead.
What changed
test/castle/no_mix_tasks_test.exs, plus the AGENTS.md account of it. Two checks:ebinviaMix.Project.compile_path()andrejects any
Elixir.Mix.Tasks.*.beam. That filename is Mix's discovery rule:Mix.Task.load_all/0scans the code path for exactly it and consults no applicationmetadata. Scoped to Castle's own
ebinrather than the code path, because Forecastle'sis on that path during the test and its
Mix.Tasks.Castle.Relupbelongs there.in an environment
mix testdoes not build, by parsingliband matching literaldefmoduleAST nodes.Both assert they are looking at something before concluding it is clean, since an empty
directory filters to no tasks and reads exactly like a clean result.
Verification
Both directions were checked by planting a task, not just by watching green:
defmodule Mix.Tasks.Castle.Relupinlib/→ both checks fail, naming the module.defmodule(Mix.Tasks.Castle.Hidden)→ the source checkcatches it at
hidden.ex:4while the beam check correctly stays silent, which isprecisely why the source check exists.
mix precommitgreen: 192 tests, 0 failures, credo --strict clean,coverage 90.36% against the 88 floor.
Independent review (Codex, 3 rounds)
Both substantive findings were reproduced before being accepted, not taken on trust.
Round 1 — application metadata is not Mix's source of truth (high). Confirmed and
fixed. The first version read
Application.spec(:castle, :modules).Mix.Tasks.Compile.Appfills
:modulesin withKeyword.put_new_lazy/3, so a project supplying its own list inapplication/0keeps it. Addingmodules: [Castle]plus a task underlib/mix/tasks/produced a tree where the metadata read
[Castle], the beam sat inebin, andMix.Task.load_all/0found the task — the old check reported clean on exactly thehazard it existed to prevent. Replaced with the ebin scan.
Round 2 — source regex misses parenthesised definitions (medium). Confirmed and fixed.
defmodule(Mix.Tasks.Castle.Hidden)is ordinary Elixir thatmix formatpreserves, and aregex anchored on whitespace after
defmodulenever matched it; inside a:prodbranch itescaped both checks. Relaxing the regex only trades that for the opposite error, since
module-looking prose in a
@moduledocstarts matching. Replaced withCode.string_to_quoted+Macro.prewalk, with regression cases pinning both directions.Round 3 — lexical aliases bypass the detector (high). Partly taken, partly declined.
defmodule :"Elixir.Mix.Tasks.X") are now matched, and theAGENTS.md claim is narrowed to the forms actually enforced.
alias Mix.Tasks, as: N→defmodule N.Castle.X, and failingclosed on dynamic names. Resolving aliases means implementing alias scoping and constant
folding inside a test — a compiler — and failing closed on dynamic
defmodulewould flaglegitimate metaprogramming. The limit costs little because the two checks fail
differently: the beam check cannot be fooled by any of those forms, since each still
writes
Elixir.Mix.Tasks.<name>.beamintoebin. What escapes both is a module namedindirectly and compiled only in an environment
mix testdoes not build — which iscircumvention of an invariant stated in three places, not the accident this guards
(someone adding
lib/mix/tasks/foo.exbecause it looked like the natural home).A case in the file pins the aliased form as a known limit, so it reads as a decision
rather than a gap to rediscover.
The loop was stopped at round 3 under the review-loop convergence rule: three rounds of the
same class of finding — "the source detector misses naming form X" — is the signal that
static analysis of a metaprogramming language cannot be made complete, not that a defect
remains.
Deliberately not done
MIX_ENV=prodpublish gate. Codex recommended a clean prod build scanned for taskbeams. That would close the environment gap completely, but it is a publish-time gate
rather than a test, and Assert that Castle ships no Mix tasks #33 scopes this to something that runs under
mix precommit.AGENTS.md says explicitly that it is not built.
mix forecastle.relup(README.md:101,lib/castle.ex:70,AGENTS.md:1200) went stale when Rename the build-time Mix tasks to the castle.* namespace forecastle#24 merged. That is README documents an appup compiler andmix castle.reluptask that no longer live in Castle #9 territory andis not fixed here.
https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy