feat: baseline resolver for rel:, tar: and ref: specs - #35
Merged
Conversation
A relup needs a previous release to compare against, and the only way to name one was a path to an assembled release. `Forecastle.Baseline` turns one grammar into three sources: `rel:` an assembled release, `tar:` a shipped artefact, `ref:` a git ref that is checked out and built. A value with no prefix is a `rel:` path, so nothing that named a baseline before this needs changing. `tar:` is the source to recommend, and for correctness rather than convenience. `release_handler` selects a relup entry by from-version string and never verifies that the running code is what the relup assumed, so a baseline rebuilt today - with today's Elixir, OTP and hex tarballs for whatever the lock does not pin - can carry a different module set from the one deployed, and the instructions then miss modules. So `ref:` says on every resolution, cache hit included, that what it produced was rebuilt: the claim is about the baseline, not about the work done to get it. A prefix-shaped value naming no source is a mistyped spec rather than a path, because `re:v1.0.0` read as a filename fails looking for a release of that name and mentions neither the typo nor the grammar. Two characters are required before the colon, which keeps a Windows drive letter out of it. `ref:` is cached on the resolved sha and `tar:` on a digest of the artefact's contents, so a moved tag and a rewritten tarball each get a fresh entry rather than a stale hit. Both are published by renaming a staging directory into place, so an interrupted run leaves nothing that looks like a hit, and stamp files written last are what say a build finished. Everything the build writes goes outside the worktree - MIX_BUILD_ROOT, MIX_DEPS_PATH and the release path all point into the cache, and MIX_BUILD_PATH is unset for the child because it would override the first - which is what makes removing the worktree and keeping the artefacts true rather than aspirational. Building an old commit runs that commit's own mix.exs, which in a project using Castle configures its release and may want a relup of its own, so CASTLE_BASELINE carries the sha being built and a resolution that finds it set refuses. A refusal rather than a depth limit: there is no build in which a baseline of a baseline is the right thing. A shallow clone missing the ref is told to `git fetch --tags --unshallow` rather than being left with an unknown revision out of `git worktree add`, which is the shape CI hits constantly. Two levels, because an appup coverage check needs only `mix compile` in the worktree while a relup needs `mix release`. The suite drives `ref:` through `mix run` in a throwaway repository rather than in process, because the resolver reads its repository from the working directory and would otherwise add worktrees to Forecastle's own checkout; `File.cd/1` is no answer, as two of this suite's modules are async. Refs: #26, ausimian/castle#32 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
`--fromto`, `--upfrom` and `--downto` now take a baseline spec rather than only a path, so a relup can be generated against the artefact that shipped or against a git ref without assembling the old release by hand first. Direction stays on the switch name and the source stays in the value; crossing them into separate switches would be twelve of them. A bare path is still a `rel:` path, which is the whole of the compatibility story - every invocation written before this means what it meant then, and the suite's existing cases are what say so. Each distinct spec is resolved once and the result mapped back onto the switch order, so `--fromto ref:v1.0.0` builds that commit once rather than once per direction. Resolution happens after the target has been read, because it can mean unpacking an artefact or building a commit and spending minutes on that before noticing the target is not where the caller said it was is the wrong order to fail in. That ordering is pinned by a case where both are wrong: which failure comes out is the assertion, since neither message would otherwise show it. `--target` is not a spec and says so when it is given one. It names the release being generated for, which has just been assembled, so it is always a path; read as one, `--target tar:my_app-1.0.0.tar.gz` fails looking for `tar:my_app-1.0.0.tar.gz.rel`, which mentions neither the switches that do take a spec nor the reason this one does not. The fixture's release steps already end in `:tar`, so the `tar:` case costs the relup suite nothing beyond the assemblies it was doing anyway. `ref:` is covered in `Forecastle.BaselineTest`, which builds a project small enough not to need a second `mix release` here. Closes #26 Refs: ausimian/castle#32 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: 3b13b3f30d
ℹ️ 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".
The test for two archive members unpacking to one path built its archive by naming one file twice on a `tar` command line. bsdtar stores that as two ordinary members; GNU tar notices the repeated inode and writes the second as a hard link - so on Linux the member-type check refused the artefact before the collision check was reached, and the assertion on the collision message failed. Green on macOS, red on every Linux cell. Built with `:erl_tar.create/3` instead, from two genuinely different files named onto one destination, which writes exactly the members it is given. That is the opposite choice from the hard-link and fifo artefacts beside it, and for the same underlying reason: each needs a builder that can actually produce the member type under test, and `:erl_tar` cannot write either of those two at all. Both spellings are now asserted present, and asserted `:regular`, before the artefact is used - otherwise a builder that quietly stopped producing one would leave the test exercising the type check again rather than the collision check, and passing. Refs: #26 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
`staging_dir/0` built a name from the OS pid and a per-run counter, cleared it with `File.rm_rf!/1` and then created it, on the reasoning that a name carrying both could not already be taken. Both halves of that are weaker than they look. Two BEAMs in separate PID namespaces - two containers over one bind-mounted `_build`, which is an ordinary way to run a build - can hold the same OS pid, and `System.unique_integer/1` is unique within a BEAM rather than between them. Two runs can therefore agree on a name, and the second then deletes the first's live workspace: a snapshot being hashed, a tree being unpacked, a worktree being built in. The directory is now claimed instead. `File.mkdir/1` is `mkdir(2)`, which either creates it or reports that somebody else holds it, and a collision retries under a fresh name rather than deleting anything. The name carries random bytes as well as the pid, so a collision is vanishingly unlikely, and `mkdir` makes it harmless when it happens anyway. Nothing in the resolver deletes a directory it did not create. Left deliberately: a staging directory is removed on every path out, so one that survives means a run was killed outright, and clearing those in passing would be the `git worktree prune` mistake with a different directory - nothing here can tell a dead workspace from a live one. They wait for the cache-clearing task the design defers. A test pins that, since sweeping them is exactly the tidy-up somebody would otherwise add. Refs: #26 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5372be0f6b
ℹ️ 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".
`project_prefix!/2` and `worktree_records/1` read paths out of git and put them through `String.trim/1`, which does not know the difference between the newline git terminates its output with and the value in front of it. A directory whose name begins with a space is legal, so a repository subdirectory named ` odd` yields a prefix of ` odd/` and came back as `odd/`. What that costs is not cosmetic. The resolver would look for `mix.exs` one directory over and report that the commit has none there, or - in a repository that also holds an `odd/` - find a genuinely different project and build that instead. And since the prefix is part of the cache key now, two different projects would key to the same entry. `trim_eol/1` removes the terminator and nothing else. The sha and the shallow flag keep `String.trim/1`: neither can contain whitespace, and the sha is pattern-checked against a hex object id besides. Refs: #26 Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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.
What the issue asked for
One grammar naming the three places a relup's baseline can come from, wired into
mix castle.relup's--fromto/--upfrom/--downto.A value with no prefix is a
rel:path, so every invocation written before specsexisted means exactly what it meant then. Direction stays on the switch name and
source stays in the value.
--targetis not a spec — it names the release beinggenerated for, which has just been assembled — and it says so if handed one.
Closes #26. Part of ausimian/castle#32.
What is here
Forecastle.Baseline— the resolver.parse!/1splits a spec withouttouching the filesystem;
resolve!/2produces a%Baseline{}naming a.relpath and a library directory.
spec?/1is the "does this carry a prefix" test,which is how
--targetanswers a spec rather than resolving it as a filename.Two levels.
:compilestops at compiled modules,:releaseassembles. Onlyref:is affected — the other two name something already built. The relup taskasks for
:release;:compileis what the appup coverage check (#27) will use.tar:is documented as the source to prefer, and the reason is correctness.release_handlerselects a relup entry by from-version string and never checksthat the running code is the code the relup was generated against, so a rebuilt
baseline can carry a different module set from what is deployed and the relup then
misses modules.
ref:says on every resolution — cache hit included — that whatit produced was rebuilt.
The cache. Everything produced is kept under
_build/castle/baselines, andevery entry is immutable: work happens in a per-run staging directory and the
finished thing is renamed into place, so an entry exists only once it is whole.
tar:entries are keyed on a digest of the artefact's bytes;ref:entries onthe resolved sha plus the build context — level, project,
MIX_ENV,MIX_TARGET, Elixir, ERTS, OS — recorded in acontext.txtinside the entry.ref:specifics. The commit is checked out into a worktree that is a siblingof the artefacts rather than a parent of them, built, and removed — so a cleanup
failure cannot put a checkout inside a published entry.
CASTLE_BASELINEcarriesthe sha being built and a resolution that finds it set refuses rather than
recursing. A shallow clone missing the ref is told to
git fetch --tags --unshallow. A project that is not at the top of its repositoryis built where it actually is. Only worktree registrations inside the baseline
cache are ever cleaned up —
git worktree pruneis not used, because it wouldderegister a checkout of yours that happens to be on an unmounted disk.
Deliberately not done
ref:case inrelup_test.exs. Thetar:acceptance case costs thatsuite nothing (the fixture's release steps already end in
:tar), while aref:one would cost anothermix releaseof the sample plus a second fetchof Castle from GitHub.
ref:is covered end to end inbaseline_test.exsagainst a Mix project small enough to build in seconds.
mix castle.baseline.cleanis deferred in thedesign (§D4 "Deferred") and is its own piece of work.
--targetstill takes only a path. The issue scopes specs to the threefrom-switches.
Review
Four rounds of independent adversarial review (Codex) before this PR was opened.
Every finding was verified against the Elixir and OTP sources before being
accepted; fifteen were real and are fixed, none were declined as false
positives. One recommendation was declined, on the last round — see below.
ref:key omitted the build contextMIX_ENV,MIX_TARGET, Elixir, ERTSgit worktree pruneref:builds ran at the repository rootgit rev-parse --show-prefixinside the worktreeMIX_TARGETnever reached the child; lib dir reconstructed asbuild/<env>/libMIX_TARGETpassed; directory found by glob, covering target prefixes andbuild_per_environment: falseMIX_EXSwould make the child load today'smix.exs:erl_tarsilently drops hard links and still returns:oksrc/andout/as siblings; onlyout/is published, and the fallback reports what actually happenedchar/block/fifobecame empty files rather than being refusedregular,directory,symlink_buildand so shared a cache entry:erl_taritself applies.relfile's device and inode, falling back toPath.expand/1Declined (round 4): the recommendation to "key every permitted
project-specific build input, or require an explicit caller-supplied context
fingerprint." A
mix.exsis arbitrary code and may read anything — anenvironment variable, a file, the clock — to decide what it builds; no key
computable here can name that, and Mix's own build directory does not try. The
bounded parts of that finding (project prefix, OS) were fixed. The rest is
stated as a boundary instead: the key covers what Forecastle chooses and what the
toolchain is, and a build depending on anything outside that should name a
tar:artefact or clear the cache. This is recorded in
AGENTS.mdso the next reviewerfinds the decision rather than the gap.
Why the loop stopped at four rounds. Cache-key completeness was raised in
rounds 1, 2 and 4 — three rounds of one class, which is a design question rather
than a defect. The structural answer was to fix the last bounded inputs and write
the boundary down, which is what the decline above records.
Verified rather than assumed, in OTP 28.3
stdlib-7.2and Elixir 1.19.5:erl_tar.erl'swrite_extracted_element/3sendslinkto a clause that logs"unsupported type" and returns
not_written, and createschar/block/fifoas empty files;
Mix.Project.build_path/1composes<target_><env|shared>;Mix.CLI.main/2readsMIX_EXS.mix precommit(compile--warnings-as-errors,deps.unlock --unused,format,credo --strict,test --include e2e) is green — 404 tests.