Skip to content

Commit 029bf07

Browse files
ausimianclaude
andcommitted
docs: reconcile the coverage account against a fresh report
The residual-coverage list was written from analysis rather than from the report, and it was wrong three ways: it claimed closed/2 was uncovered while the report showed the clause hit, and it grouped publish/2 generic error and call/2 unrecognised answer with paths needing modes or mounts when neither needs either. All three are now covered, and the list is rebuilt line by line against a regenerated cover/ - stale HTML from before the test/support exclusion is how the inconsistency survived, so the note says to delete it first. The mechanism claim is corrected: cover is not unable to run in a non-distributed VM, it is that Mix instruments this node and nothing instruments the peer. The remaining 21 observable lines are split into the five compiler generated clauses and the sixteen that need a mode, a device node or a provider sabotaging the working directory, with the arithmetic for why 90% is out of reach honestly rather than absolutely. Also records why fill/3 needs three different handles to pin its three steps, and why stub_castle/2 compiles with :compile.forms/2. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent bc4329d commit 029bf07

1 file changed

Lines changed: 131 additions & 63 deletions

File tree

AGENTS.md

Lines changed: 131 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,7 +1143,9 @@ one Castle set silently would be one a consumer could not see in their own
11431143

11441144
- Run `mix precommit` before committing. It is the single validation gate —
11451145
`compile --warnings-as-errors`, `deps.unlock --unused`, `format`,
1146-
`credo --strict`, `test`. Do not run the individual checks piecemeal.
1146+
`credo --strict`, `test --cover`. Do not run the individual checks piecemeal.
1147+
The `--cover` is what enforces the coverage threshold; see *What
1148+
`mix test --cover` measures* for the figure and why it is what it is.
11471149
- `@version` in `mix.exs` is the single source of truth for the version.
11481150
- Add user-visible changes to `RELEASE.md` on the feature branch, using
11491151
[Keep a Changelog](https://keepachangelog.com/) sections. Do not defer release
@@ -1259,6 +1261,18 @@ shapes differ in the version directory's release files: Mix assembles one,
12591261
shape is how `Castle.Peer` came to refuse every unpacked release as ambiguous,
12601262
having been reviewed seven times against a directory the peer path never meets.
12611263

1264+
`build/2`'s `:override` points `lib/<app>-<vsn>` at a directory other than this
1265+
node's own copy, leaving the release file alone — which is what a target
1266+
carrying a *different build* of one of preboot's applications looks like, and the
1267+
only way to be on the far side of `{Castle.Peer, :resolve, 1}`.
1268+
`stub_castle/2` builds that castle: one exported `resolve/1` answering a fixed
1269+
atom. **It compiles with `:compile.forms/2` rather than from Elixir source**,
1270+
and that is load bearing rather than tidy — the module is named `Castle.Peer`,
1271+
so `Code.compile_string/1` would load the stub over the running one and every
1272+
peer test after it would be asserting against the fixture. Abstract forms are
1273+
the one route to a beam the code server never sees. Do not "simplify" it onto
1274+
`provider_app/5`.
1275+
12621276
`Castle.Peer.materialise/2` takes `:boot_timeout` and `:resolve_timeout` for the
12631277
same reason `Castle.Commands` takes the module to talk to — a deadline nothing
12641278
can shorten is a deadline no test can show is enforced. Two tests give it a
@@ -1292,6 +1306,23 @@ asserting the acknowledged cost, that the by-path `chmod` does land on the
12921306
swapped name. With the name left alone the two behaviours are identical, so
12931307
there is no other way to tell them apart.
12941308

1309+
**`fill/3`'s three steps need three different handles to be told apart, and a
1310+
handle that is merely closed only reaches the first.** An already-closed handle
1311+
fails the write *and* the close, `with :ok <- written, :ok <- closed` reports the
1312+
write, and both helpers produce the same `"Cannot write #{path}"` prefix — so
1313+
that case pins `written/3` (its discriminator is that `IO.binwrite/2` in place of
1314+
`:file.write/2` raises instead of returning) and says nothing whatever about
1315+
`closed/2`. It executed `closed/2`'s error clause without being able to detect it
1316+
breaking, and this file claimed the clause was uncovered while the report showed
1317+
it hit; both halves were wrong in the same place. Telling them apart needs a
1318+
handle whose **write succeeds and whose close does not**, which is a process:
1319+
`File.io_device/0` is `pid | file_descriptor`, so a pid is within what `fill/3`
1320+
accepts, and `:file.write/2` sends a pid an io request while `File.close/1` sends
1321+
it a `:file_request` — two protocols, so one can answer and the other refuse.
1322+
That test asserts the close's own reason and that the file was **not** narrowed,
1323+
which is the "mode only once both have succeeded" half of the contract. A
1324+
`chmod` that cannot land is the third step, reached by removing the name.
1325+
12951326
`secure_dir/1` is public so that the `mkdir`-to-`chmod` window can be stood in:
12961327
a test creates a directory at 0777, plants a `sys.config` symlink inside it, and
12971328
asserts that securing it is refused, the directory removed and what the symlink
@@ -1502,69 +1533,106 @@ seven `test/support` modules out of it. A fixture is covered by being run at
15021533
all, so counting them moved the total without their ever being the thing
15031534
measured — and two of them moved it *down* for a reason that is not about the
15041535
suite. `Castle.PeerProviderStub` sat at 4.17% and `Castle.IoSink` at 76% while
1505-
being exercised by every peer test, because they execute in the peer's VM and
1506-
`cover` runs on this node. The list is module names rather than a pattern: a
1507-
regex over `Stub` would swallow a production module spelled that way, which is
1508-
the one thing an exclusion must not do.
1509-
1510-
Lib-only that is **87.95%**`Castle.Commands` 94.85%, `Castle` 90.48%,
1511-
`Castle.Peer` 79.81%, `Castle.Deployment` and `Castle.Error` 100%.
1512-
1513-
**The threshold is 85 because 90 is unreachable by construction rather than for
1514-
want of tests.** Everything below the `## In the peer` comment in
1515-
`lib/castle/peer.ex``resolve/1`, the standard-error relay, the pipeline and
1516-
the compile-environment check — runs in the peer's VM, which has no node name
1517-
and `is_alive() == false`, so `cover` cannot be started on it. That is 33 lines,
1518-
about 7% of the shipped total, executed by `Castle.PeerTest` on every run and
1519-
counted as missed, which puts the ceiling near 93%. Do not raise it by calling
1520-
those functions on the test node: that runs Elixir's pipeline in the very VM the
1521-
whole mechanism exists to keep it out of, and it would assert less than the peer
1536+
being exercised by every peer test, because they execute in the peer's VM. The
1537+
list is module names rather than a pattern: a regex over `Stub` would swallow a
1538+
production module spelled that way, which is the one thing an exclusion must not
1539+
do.
1540+
1541+
**It is part of `mix precommit`**, which is what makes the threshold a gate
1542+
rather than a number in a comment. Nothing else runs it, and CI's `test` matrix
1543+
stays on a plain `mix test` deliberately, because cover's line attribution can
1544+
differ between Elixir versions and the figure is measured on one toolchain — the
1545+
pinned `precommit` job is where it is enforced.
1546+
1547+
Lib-only that is **88.58%** — 419 of 473 relevant lines. Per module:
1548+
`Castle.Commands` 94.85%, `Castle` 90.48%, `Castle.Peer` 81.22%,
1549+
`Castle.Deployment` and `Castle.Error` 100%.
1550+
1551+
**The threshold is the measured figure, 88.58, and not a rounder number near
1552+
it.** One uncovered line added to `lib` gives 88.40% and fails; that was
1553+
measured rather than assumed. It replaced an 85 that sat *below* the figure it
1554+
was meant to floor, so it ratcheted nothing and licensed a thirteen-line
1555+
regression. The cost of a threshold with no slack is that a refactor which
1556+
legitimately removes covered lines fails it too, and the answer then is to
1557+
re-measure and edit the number deliberately — the same rule every other claim
1558+
here is held to.
1559+
1560+
**What cannot be measured is the peer's VM, and the reason is where
1561+
instrumentation is applied — not anything cover is unable to do.** An earlier
1562+
version of this file said the peer "has no node name and `is_alive() == false`,
1563+
so `cover` cannot be started on it", and that is false: `:cover.start/0` works
1564+
in a VM with no distribution. What actually happens is that Mix starts cover on
1565+
*this* node and instruments the modules loaded here, while the peer is a
1566+
separate VM loading `Castle.Peer` from the target release's own beam files,
1567+
which nothing has instrumented. Cover's only mechanism for reaching another VM
1568+
is `:cover.start/1` over a *distributed* node, and this peer deliberately has
1569+
none. So `resolve/1` and everything below the `## In the peer` comment — 33
1570+
lines, about 7% of the shipped total — runs on every `Castle.PeerTest` and is
1571+
counted as missed, which puts the observable ceiling near 93%.
1572+
1573+
Do not raise it by calling those functions on the test node: that runs Elixir's
1574+
pipeline in the very VM the whole mechanism exists to keep it out of, mutates
1575+
this node's `:elixir` application environment, and asserts less than the peer
15221576
tests already do. Splitting them into a module of their own to exclude it is
1523-
worse still — `{Castle.Peer, :resolve, 1}` is a contract with the *next* version
1524-
of Castle, so the MFA is not free to move for a metric.
1525-
1526-
`--cover` is deliberately not part of `mix precommit`. Every test here is
1527-
justified by what it fails against, and a merge-blocking percentage cannot tell
1528-
a test that discriminates from one written to move the number.
1529-
1530-
**What is left uncovered, and why each is a decision.** Every one of them is a
1531-
failing branch, and the fixture is the problem in each case:
1532-
1533-
* **A filesystem that stops behaving between one statement and the next.**
1534-
`arm/4`'s three ways of failing to publish — a working directory that cannot
1535-
be made, an `lstat` that fails for anything but `:enoent`, and a staged marker
1536-
that cannot be written or linked — together with `unarmed/3` and `detail/1`,
1537-
the message all three share. Reaching any of them needs a mode, a read-only
1538-
mount or a cross-device link, and a mode is refused here for the reason
1539-
`stub_stat/1` exists: root and some filesystems ignore one, so the fixture
1540-
would only sometimes describe the state it names. The one of the three the
1541-
filesystem *can* be made to produce is covered — `armed(:taken, …)`, the name
1542-
claimed between `unclaimed/3` and the publish, which is the second VM
1543-
`serialised/2` cannot reach and the case `publish/2` refusing rather than
1544-
replacing exists for. The seam is the materialisation, being the one step
1545-
between those two.
1546-
* Same class, and the same answer: `armed_version/1`'s unreadable marker;
1547-
`describe_type/1`'s remaining catch-all, which only `:device` reaches now and
1548-
a device node needs root to make; `Castle.Peer`'s `empty/1` and
1549-
`release_file/1` listing failures; `closed/2`; `publish/2`'s non-`:eexist`
1550-
error; and the two writes at the end of `expand/2`.
1551-
* **The compiler's own default-argument clauses**, five lines, for the arities
1552-
nothing calls: `install/2` and `install/3` in both `Castle` and
1553-
`Castle.Commands`, and `commit/2` and `commit/3`. There is no behaviour there
1554-
— a case that called an intermediate arity would be moving the number and
1555-
nothing else.
1556-
* **A target release carrying a different `Castle.Peer`**, which is `call/2`'s
1557-
"may carry a version of Castle that predates this mechanism". A cross-version
1558-
condition: `Castle.SyntheticRelease` symlinks the running castle's own ebin
1559-
into the fixture, so reaching it needs a second `castle` application built to
1560-
answer differently, which is machinery out of proportion to one refusal.
1561-
* **`stop/1`'s rescue**, which needs a control process that has already gone,
1562-
and about which the code deliberately makes nothing.
1563-
* **A booted release**, which is Forecastle's `:e2e` suite, described just
1564-
above. That suite is where the in-peer section is exercised against a real
1565-
release rather than a synthetic one, where the marker is consumed by a real
1566-
launcher, where `running/1` is polled across a real reboot, and where the exit
1567-
statuses `bin/castle` returns are asserted. None of it is measured here.
1577+
worse — `{Castle.Peer, :resolve, 1}` is a contract with the *next* version of
1578+
Castle, so the MFA is not free to move for a metric.
1579+
1580+
**Why 90% is not the threshold, stated as arithmetic rather than as a claim of
1581+
impossibility.** 90% needs 426 covered, seven more than there are. Twenty-one
1582+
missed lines are observable in principle, and they divide cleanly:
1583+
1584+
* **Five are the compiler's own generated clauses**, one line per defaulted
1585+
arity nothing calls: `Castle.install/2` and `/3`, `Castle.Commands.install/2`
1586+
and `/3`, and `Castle.Commands.commit/3`. These *are* hittable — calling the
1587+
intermediate arities was measured to cover all five and to take the total to
1588+
89.64% — and that is exactly the problem. Each is a delegation whose defaults
1589+
are a subset of an arity that is already called, so `Castle.install/1` in
1590+
`castle_test.exs` already establishes that the defaults are the real modules.
1591+
A case calling `install/3` would assert nothing that arity 1 and arity 4 do
1592+
not, and would move this number. That is the move this project does not make,
1593+
and it is the reason 90% is out of reach: five of the seven can only come from
1594+
here.
1595+
* **Sixteen need a file mode, a device node, or a provider sabotaging Castle's
1596+
own working directory.** In `Castle.Commands`: `arm/4`'s three publish
1597+
failures (`work_dir/1` refusing, an `lstat` failing for anything but
1598+
`:enoent`, a staged marker that cannot be written or linked) together with
1599+
`unarmed/3` and `detail/1`, the message all three share — seven lines that
1600+
stand or fall together, and every route to them needs the releases directory
1601+
to stop behaving between one statement and the next. Plus
1602+
`armed_version/1`'s unreadable marker, and `describe_type/1`'s catch-all,
1603+
which only `:device` reaches now and a device node needs root to make. In
1604+
`Castle.Peer`: `release_file/1`'s and `empty/1`'s listing failures (the
1605+
second unreachable through `work_dir/1` at all, which has just created the
1606+
directory — reaching it means calling `secure_dir/1` on something it never
1607+
hands over, whose one observable effect is that Castle deletes what you
1608+
pointed it at); `keep/2`'s generic publish error, where `write_like/3` has
1609+
created the staging file immediately before, so the only `File.ln/2` failures
1610+
left are cross-device; `stop/1`'s rescue, which needs the control process
1611+
already gone and is a race no fixture settles; and the two writes at the end
1612+
of `expand/2`, whose only route is a config provider removing the working
1613+
directory while the peer runs.
1614+
1615+
A mode fixture is refused here for the reason `stub_stat/1` exists: root and
1616+
some filesystems ignore one, so it would only sometimes describe the state it
1617+
names.
1618+
1619+
**Two things that were in this list and should not have been.** `publish/2`'s
1620+
generic error needs none of the above — `File.ln/2` with a staging file that is
1621+
not there answers `:enoent` and creates no destination — and it is now covered.
1622+
So is `call/2`'s "answered … may carry a version of Castle that predates this
1623+
mechanism": `SyntheticRelease.stub_castle/2` builds a castle application for the
1624+
target alone, which is what `build/2`'s `:override` is for. Both were dismissed
1625+
as needing fixtures they did not need. And `closed/2` was listed as uncovered
1626+
while the report showed its error clause *hit* — executed incidentally by the
1627+
closed-handle test, which cannot discriminate it, and now pinned properly by a
1628+
test of its own. Read this list against a freshly regenerated `cover/`; stale
1629+
HTML from a previous run is how the inconsistency survived.
1630+
1631+
**A booted release** is the last of it, and that is Forecastle's `:e2e` suite,
1632+
described just above. That suite is where the in-peer section is exercised
1633+
against a real release rather than a synthetic one, where the marker is consumed
1634+
by a real launcher, where `running/1` is polled across a real reboot, and where
1635+
the exit statuses `bin/castle` returns are asserted. None of it is measured here.
15681636

15691637
## Known limitations
15701638

0 commit comments

Comments
 (0)