Six findings from the adversarial review of #532
(the flows/lifecycle.rs split, ticket #314). None blocked that merge, and none
should have been fixed inside it — every one of them would have cost the
byte-for-byte reassembly that made the move checkable. They are recorded here so
they are not buried in the review thread of a merged PR.
Roughly in order of how much they are worth.
1. The one place the banner cut is visibly not a module boundary
ClonePlacement and leaf_of live in prune_plan.rs but are purely
locations.rs's topic. They are the only reason reconcile.rs depends on 457
lines of prune classification. Textbook Feature Envy, and the single case where
following the banners produced a module boundary that the code itself argues
against. Moving them to locations.rs should drop the dependency outright.
2. only_one_module_can_remove_a_volume passes vacuously if the door moves
The guard names a single door for docker volume rm and asserts nothing else
carries one. It has no positive counterpart, so if remove_volumes( ever leaves
delete.rs entirely the guard goes green while asserting nothing. Pre-existing,
but #532 pointed it at a file that is easier to move than the one it used to name.
Same species as #517 and #529: a check whose green ranges over less than the claim
it appears to support. The fix is one added assertion that the door exists.
3. is_declaration rejects plausible future edits to a module root
The split guard's rule 2 ("the root defines nothing") classifies each root line as
docs / mod / use / re-export. It rejects inner attributes (#![allow(...)])
and pub(super) use / pub(super) mod, both of which are reasonable things to
add to a module root later. A guard that fails on a legitimate edit gets edited
until it stops failing, which is how it stops meaning anything.
4. Rule 3's documentation oversells what it measures
The rule reads "no member exceeds a third of the family", but the denominator
includes the offending file, so the real rule is "no member exceeds half of all
others combined". Measured: delete.rs must reach 1,606 lines to fail, and
padding it to 1,378 still passes.
It is an anti-re-merge shape guard, not a size cap, and it works fine as one. The
prose should say that, since a reader who believes it is a size cap will be
surprised by a 1,300-line member sailing through.
5. tests.rs at 5,450 lines is a deferral nothing records
It is larger than all 13 production members combined (3,689). Leaving it whole in
#532 was the right call and the review agreed — deciding which test belongs to
which of 13 modules is a judgement, and making judgements inside a move is how a
move stops being provable. But nothing in the tree now says it was deferred rather
than decided, and the guard explicitly exempts it. Either split it or write down
why it stays whole.
6. {notices, delete, delete_guard} is a mutual import cycle
933 lines that cannot be read independently of each other. Not a defect, but it is
the residue the banner cut could not resolve, and worth knowing before anyone
assumes the 13 members are independently comprehensible.
Two stale premises in #314 itself, for the record
- "The whole crate has zero production submodules" was already false at the merge
base: flows/provision.rs:79 carries pub mod verdict_cache;.
- "2,811 lines / 45 types" were exact when written and still close at the split.
Not in scope here
flows/launch.rs (42 banners) and flows/provision.rs (22) are unsplit. #532
deliberately scoped its guard to flows::lifecycle rather than being born with an
exemption list, which is the right shape; widening it is a separate decision from
these six.
Six findings from the adversarial review of #532
(the
flows/lifecycle.rssplit, ticket #314). None blocked that merge, and noneshould have been fixed inside it — every one of them would have cost the
byte-for-byte reassembly that made the move checkable. They are recorded here so
they are not buried in the review thread of a merged PR.
Roughly in order of how much they are worth.
1. The one place the banner cut is visibly not a module boundary
ClonePlacementandleaf_oflive inprune_plan.rsbut are purelylocations.rs's topic. They are the only reasonreconcile.rsdepends on 457lines of prune classification. Textbook Feature Envy, and the single case where
following the banners produced a module boundary that the code itself argues
against. Moving them to
locations.rsshould drop the dependency outright.2.
only_one_module_can_remove_a_volumepasses vacuously if the door movesThe guard names a single door for
docker volume rmand asserts nothing elsecarries one. It has no positive counterpart, so if
remove_volumes(ever leavesdelete.rsentirely the guard goes green while asserting nothing. Pre-existing,but #532 pointed it at a file that is easier to move than the one it used to name.
Same species as #517 and #529: a check whose green ranges over less than the claim
it appears to support. The fix is one added assertion that the door exists.
3.
is_declarationrejects plausible future edits to a module rootThe split guard's rule 2 ("the root defines nothing") classifies each root line as
docs /
mod/use/ re-export. It rejects inner attributes (#![allow(...)])and
pub(super) use/pub(super) mod, both of which are reasonable things toadd to a module root later. A guard that fails on a legitimate edit gets edited
until it stops failing, which is how it stops meaning anything.
4. Rule 3's documentation oversells what it measures
The rule reads "no member exceeds a third of the family", but the denominator
includes the offending file, so the real rule is "no member exceeds half of all
others combined". Measured:
delete.rsmust reach 1,606 lines to fail, andpadding it to 1,378 still passes.
It is an anti-re-merge shape guard, not a size cap, and it works fine as one. The
prose should say that, since a reader who believes it is a size cap will be
surprised by a 1,300-line member sailing through.
5.
tests.rsat 5,450 lines is a deferral nothing recordsIt is larger than all 13 production members combined (3,689). Leaving it whole in
#532 was the right call and the review agreed — deciding which test belongs to
which of 13 modules is a judgement, and making judgements inside a move is how a
move stops being provable. But nothing in the tree now says it was deferred rather
than decided, and the guard explicitly exempts it. Either split it or write down
why it stays whole.
6.
{notices, delete, delete_guard}is a mutual import cycle933 lines that cannot be read independently of each other. Not a defect, but it is
the residue the banner cut could not resolve, and worth knowing before anyone
assumes the 13 members are independently comprehensible.
Two stale premises in #314 itself, for the record
base:
flows/provision.rs:79carriespub mod verdict_cache;.Not in scope here
flows/launch.rs(42 banners) andflows/provision.rs(22) are unsplit. #532deliberately scoped its guard to
flows::lifecyclerather than being born with anexemption list, which is the right shape; widening it is a separate decision from
these six.