docs: document the build-time tasks as castle.* - #36
Merged
Conversation
The README described build-time tooling as Castle's after it had moved to Forecastle in 0.3.0. The assumed fix was to correct it to say `forecastle.*`; forecastle#24 renamed the tasks back into the `castle.*` namespace instead, leaving them implemented in Forecastle, so the README is correct roughly as first written plus a sentence saying where they come from. So `mix forecastle.relup` becomes `mix castle.relup` in the README, in `customize/1`'s `@doc` and in the release-integration notes. The appup compiler is called out separately rather than folded in with it: it is named for what it does rather than for either package, stays `mix compile.appup`, and there is no `mix castle.appup` to infer. `RELEASE.md` carries the rename because Castle's consumers are the ones who run the task and nobody depends on Forecastle directly, so Forecastle's own breaking-change note reaches nobody who needs it. `mix.lock` moves to the Forecastle that has the rename, and with it forecastle#34's correction to Forecastle's own README - which claimed the appup compiler was named for Castle too, and so contradicted what this commit writes here. Without the bump the committed lock pinned e43f880, whose tree still defines `Mix.Tasks.Forecastle.Relup`, so a clean checkout would have had no `mix castle.relup` while every page here documented one. Verified from an empty `deps/` and `_build/`: `castle.relup` and `compile.appup` resolve, `forecastle.relup` and `castle.appup` do not. `customize/1` stops restating Forecastle's strategy rules. Three review rounds each corrected that paragraph and each correction was wrong in a case the next one found - `--restart` reads no appups, `auto` does not fall back for a missing owned appup, and an appup may itself ask for `restart_emulator`. A summary short enough for a `@doc` is wrong somewhere; a correct one is that task's `@moduledoc` copied into a repository that cannot see it change, which is the drift AGENTS.md already warns about. So it states the requirement and names `mix castle.relup` as the authority for which transitions need one. The Known limitations entry claiming the README is out of date goes with it, replaced by the limitation that is actually left. Forecastle's suite covers the task implementations, but its fixture takes Forecastle as a path dependency with `override: true`, so nothing anywhere runs the claim the README makes - that depending on Castle alone is enough to get them. The stale lock above is that gap producing a real defect with the suite green, so it is recorded rather than described as covered. Closes #9 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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 #9. Part of #32; rationale in
design/upgrade-tooling.md§D1.What the issue asked for
The README described build-time tooling as Castle's after it moved to Forecastle in 0.3.0. The assumed fix was to correct it to say
forecastle.*. ausimian/forecastle#24 renamed the tasks back into thecastle.*namespace instead — implemented in Forecastle, named for the package people actually depend on — so, as the issue's own comment predicted, the README becomes correct roughly as first written plus a sentence saying where the tasks come from.What changed
README.md—mix forecastle.relup→mix castle.relup, plus a paragraph naming Forecastle as the implementer. The appup compiler is stated separately: it is named for neither package, staysmix compile.appup, and there is nomix castle.appup.lib/castle.ex— the same rename incustomize/1's@doc, and the appup prerequisite restructured (see below).AGENTS.md— same rename in the release-integration notes; the "The README is out of date" known limitation replaced, since closing README documents an appup compiler andmix castle.reluptask that no longer live in Castle #9 makes it false.RELEASE.md— a### Changedentry. Forecastle carries the breaking-change note, but nobody depends on Forecastle directly, so a Castle consumer withmix forecastle.relupin their pipeline would never see it.design/upgrade-tooling.md— §1.2's executable workflow step now saysmix castle.relup; theStatus:line records D1 as implemented. D1's two remainingforecastle.relupmentions are deliberately historical rationale.mix.lock— advanced toa77e97c.The lockfile, which is the part worth reading
The committed lock pinned Forecastle at
e43f8808, whose tree still definesMix.Tasks.Forecastle.Relup. So a clean checkout of this branch honouring its own lock had nomix castle.relupwhile every page in it documented one. The suite stayed green throughout, because nothing on this side tests the README's instructions.Verified from an emptied
deps/and_build/:mix castle.relupmix compile.appupmix forecastle.relupmix castle.appupThe range
e43f880..a77e97ccarries no Forecastle behaviour change beyond the rename — confirmed independently by the reviewer. It also picks up ausimian/forecastle#34.The paired-repo fix (ausimian/forecastle#34)
Review of this branch caught an error in already-merged Forecastle: its README said "Both are named for
Castle", taking in the appup compiler. That is the same mistake this PR fixes on the Castle side, and left alone the two halves of one release would have given a reader contradictory naming rules for the same compiler. Fixed and merged as ausimian/forecastle#34, which this branch then re-pins to.customize/1stops restating Forecastle's rulesThree review rounds each corrected the appup prerequisite, and each correction was wrong in a case the next one found:
--restartreads noneautofalls back when it cannot make a transition hot" — a missing owned appup is an error, not a restartautorestarts only for ERTS or uncovered non-owned changes" — an appup may itself requestrestart_emulatorThree rounds of one class is a design signal, not three defects. Castle was restating rules that span ERTS changes, ownership, per-direction classification, appup-supplied restarts, and added/removed applications: a summary short enough for a
@docis wrong somewhere, and a correct one is Forecastle's@moduledoccopied into a repository that cannot see it change — precisely the driftAGENTS.mdalready warns about for@docs. So it now states the requirement and namesmix castle.relupas the authority.Declined: the reviewer's recommendation to enumerate all five rules here, for that reason.
Deliberately not done
pathdependency withoverride: true, so nothing anywhere runs the claim this README makes — that depending on Castle alone is enough. That gap is what let the stale lock through, soAGENTS.mdnow records it as a known limitation with the near-miss written out, rather than describing it as covered. Building the fixture is a test-infrastructure change, not a documentation fix.design/upgrade-tooling.mdnot swept. It is the why and says it will not be updated to track implementation; only the executable workflow step and the status line changed.Verification
mix precommitgreen: 193 tests, 0 failures, credo --strict clean, coverage 90.36% against the 88 floor.approve: 4 substantive findings fixed (1 high, 3 medium), 1 low fixed, 1 recommendation declined with reasoning above. Final verdict found no additional drifting restatement.https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy