Skip to content

Follow-ups from the lifecycle split review: a misplaced pair, a vacuous guard, and four notes #533

Description

@blooop

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions