Split flows/lifecycle.rs along its banners - #532
Conversation
The file held five unrelated commands -- stop, delete, purge, prune, reconcile -- plus the refresh latch, the fetch sweep and the on-disk placement rules that serve the launch path rather than any of them, and it carried thirteen banner comments marking exactly where the modules were. None of them had ever been made into one. The cut follows those banners. What is left in flows/lifecycle.rs is a table of contents: the family's documentation, thirteen private module declarations and the glob re-exports that keep every flows::lifecycle::Thing path where callers already had it. All three cargo public-api snapshots are byte identical across the split. Every production hunk between the old file and a reassembly of the new modules is a pub(super) keyword or the rewrap that keyword caused. pub(super) is the visibility these items already had: the module they were private to was the whole family. tests/lifecycle_is_split.rs is the guard. It fails if the root grows a body of its own, which is the only road back to one file, and if any one member grows past a third of the family. Two existing guards learned the new paths and neither changed what it asserts: volume_names' one door is now lifecycle/delete.rs, and one_seam's list of files whose cfg(test) gate is declared elsewhere gained lifecycle/tests.rs. Closes #314
Twelve intra-doc links resolved only because the single file imported the name they used: the members do not, so rustdoc lost them. Each one keeps its displayed text and names the path explicitly. cargo doc is back to zero unresolved links, which is where main is.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @blooop, your pull request is larger than the review limit of 150,000 diff characters
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed the three-dot diff against merge base 5fa0fd1a7ace5add4d41d39570a75f9145299ac0 (origin/main, identical to the PR's base tip), head 4616253.
This is a refactor whose whole claim is "nothing changed but the file boundaries", so the review is mostly an attack on that claim rather than on the code. I reconstructed the evidence independently rather than re-reading the PR body's accounting. The claim holds, and one of its four parts is understated.
Standards
Verification of the move (my own reconstruction, not the author's)
I rebuilt the check a different way: strip each member's //! header and use block mechanically, concatenate the thirteen in banner order, and diff against the pre-split production region cut at the same banner offsets.
- Production region: 35 hunks. Every one is either an added
pub(super)keyword (plus the rustfmt rewrap a longer signature caused) or an intra-doc link given an explicit path. No statement body changed. 35 rather than the PR's 34 is hunk coalescing, not a discrepancy: 9 doc-link hunks exactly as claimed, the rest visibility. tests.rs: exactly 7 hunks, all rustfmt re-joining a wrapped statement after the suite lost an indent level, plus the one import-block change.#[test]count is 159 on both sides.cargo docwarnings went 140 → 138, zero unresolved links. The split introduced no newlinks to private itemwarning, which was a live risk once items becamepub(super).- Public-API snapshots:
git diff <base>...HEAD -- '*public-api*'is empty. cargo test --workspacegreen locally;scripts/changelog_frozen.pyreports 65 released sections untouched; CHANGELOG is 21 insertions / 0 deletions under[Unreleased]→### Changed.
The pub(super) claim is right, and stronger than stated
I probed it with the compiler rather than by reading. Extracting all 112 top-level item names from the pre-split production region and importing them from flows::launch on both trees:
| merge base | this branch | |
|---|---|---|
reachable from flows::launch |
81 | 81, same names |
E0603 (path resolves, private) |
31 | 15 |
E0432 (path does not resolve) |
0 | 16 |
Zero items widened. A separate probe over the pub(super) struct fields and inherent methods (PurgePlan::ownership, PrunePlan's six, Candidates' two, ReconcilePlan's two, Removal::{persistence,probe}) returns byte-identical E0616/E0624 sets on both trees.
The understated part: 16 items narrowed. LastSweep, delete_call, fetch_trouble, has_url_scheme, is_scp_like, last_sweep_note, leaf_spellings, orphaned_workspaces, purge_delete_call, reclaim_volumes, reclaimable_volumes, record_sweep_note, records_for_absent_directories, refs_not_packed, repository_of and stop_call were family-visible before and are now module-private, because they did not need pub(super). I checked each: every one is referenced only inside its own member. That is a real gain the PR body does not claim, and it is the split doing what a split is for.
The guard is non-vacuous (mutation-tested)
Rule 2 catches all three shapes I threw at the root: a pub fn one-liner, a pub struct with a body, and a pub const array whose inner lines are all comma-terminated and would individually pass is_declaration. The opening line always fails, and the test collects every offending line rather than short-circuiting. I also confirmed volume_names' edited DOOR still fires: planting a remove_volumes( call in purge.rs fails only_one_module_can_remove_a_volume naming the line.
Findings
Non-blocking — {notices, delete, delete_guard} is a mutual dependency cycle. lifecycle/notices.rs:5-6 imports from both delete and delete_guard; both import back (delete.rs:9, delete_guard.rs:7), and delete ↔ delete_guard closes the triangle. 933 lines that cannot be read or moved independently. Legal, and inherent to one shared notice vocabulary, but these three are one module wearing three names, and the "narrow interfaces" framing overstates it for them.
Non-blocking — ClonePlacement and leaf_of are in the wrong member. lifecycle/prune_plan.rs:198 and :455. Both are purely about where a clone sits on disk, which is locations.rs's stated topic, and ClonePlacement wraps locations::WorkspaceLocations. They are the only reason reconcile.rs:10 depends on 457 lines of prune classification. Moving them deletes that edge and moves no API row, since ClonePlacement is pub and glob-re-exported either way. Textbook Feature Envy, and the one place the mechanical banner cut is visibly not a module boundary. Follow-up, not a merge blocker: doing it here would cost the byte-for-byte reassembly that makes this PR checkable.
Non-blocking — tests/volume_names.rs:39 now points at a file that can move. only_one_module_can_remove_a_volume only asserts the absence of strays outside DOOR. If remove_volumes( ever leaves delete.rs and nothing replaces it, the test passes vacuously. This vacuity is pre-existing, not introduced here, but the namer beside it has a positive counterpart (that_module_does_build_them) and the remover does not. One extra assertion, that DOOR mentions REMOVAL at least once, closes it.
Nit — is_declaration is too tight in two plausible spots. tests/lifecycle_is_split.rs:88: starts_with("#[") misses inner attributes, so a #![allow(...)] on the root fails with "neither documentation, an import, a module declaration nor a re-export". Same for pub(super) use / pub(super) mod, which the use/mod arms do not list. Both are edits someone would reasonably make to a module root.
Nit — rule 3's doc oversells what rule 3 does. lifecycle_is_split.rs:44-48 argues a third beats a half, but the denominator includes the offending file, so the effective rule is "no member exceeds half of all other members combined". Measured: delete.rs (478 lines) must reach 1,606 to fail, and I confirmed padding it to 1,378 still passes. It is an anti-degenerate-re-merge guard, which is what rule 3's own text says it is; the LARGEST_SHARE comment reads as though it were a size cap. Worth one sentence so the next reader does not trust it as one. Related: it is a relative rule, so it can also fail an untouched file if members are deleted, and inline #[cfg(test)] blocks in a member inflate the denominator for everyone.
Nit — the_lifecycle_root_declares_and_defines_nothing (:112) contradicts its own body. The root declares 26 things; the assert message ("defines nothing itself") is the accurate half. the_lifecycle_root_defines_nothing says it.
Nit — "thirteen" is now a hand-maintained fact in three places (lifecycle.rs module doc, lifecycle_is_split.rs doc, CHANGELOG) while the guard asserts only > 1. AGENTS.md's standing rule wants a diff test beside a second copy of a fact. Asserting == 13 would be churn; dropping the numeral from the prose is the cheaper fix.
Leaving tests.rs whole was the right call
Redistributing 5,450 lines of tests would have destroyed the one property that makes this PR reviewable at all: a byte-for-byte reassembly against the old file. Deciding which test belongs to which member is a judgement, and a judgement inside a move is exactly what makes a move unprovable. Excluding it from rule 3 is correct and documented at lifecycle_is_split.rs:55-58. The one thing worth recording somewhere is that the deferral exists: the family's largest file is 5,450 lines, larger than all thirteen production members combined (3,689), and nothing currently says a second half is still owed.
The layout convention claim checks out: mod.rs appears only on clients/, domain/, flows/, and all five leaf families (clients/git, domain/workspace_state, flows/agent_worktrees, flows/provision, flows/lifecycle) are foo.rs beside foo/. No exceptions.
test/test_docs_prose.py globs README.md and docs/*.md, so CHANGELOG and Rust doc comments are out of its scope. The em dashes in both are not findings.
Spec
Spec is #314 — Split flows/lifecycle.rs along its banners, on map #299. No blocking spec findings.
"13 banner-delimited sections that map 1:1 onto modules never created"
Verified mechanically: the thirteen banner sections were extracted by their // === dividers and each section's ordered top-level item list diffed against the corresponding new module. All thirteen match exactly, in the original banner order. No section merged, split or resequenced, and the module names track the banner titles (where a workspace is on this disk → locations.rs, prune: the plan → prune_plan.rs).
"Split into
flows/lifecycle/{...}.rs"
Satisfied literally: the members are flows/lifecycle/*.rs. The ticket says nothing about mod.rs, so keeping lifecycle.rs as the root is a deviation from one reading of the phrasing, not from its text, and the repo convention it cites is real (previous paragraph). Not a spec miss.
"Blocked on the snapshot re-scope so
public-api.txtmoves once, not twice."
The blocker, #338, landed as #347; the tree carries public-api.api.txt, public-api.rest.txt and the runner's own. The snapshot diff here is empty, so it moved zero times. Clause fully honoured.
"2,811 production lines, 45 top-level types"
Both exact at the commit the ticket was written against (#[cfg(test)] at line 2811, 45 top-level type declarations above it). The merge base has since drifted to 3,652 / 57. Ticket numbers correct as written; no PR defect.
Nit —
"the whole crate has zero production submodules"
Has gone stale rather than being wrong: flows/provision.rs:79 carries an ungated pub mod verdict_cache; at the merge base, so this is the second production submodule tree, not the first. The rationale aged; the instruction it supports did not.
Non-blocking, arguably in charter — scope. Nothing the ticket asked for is missing. tests/lifecycle_is_split.rs is unrequested, but #299 runs build tickets through wf-tdd and the guard carries no hand-maintained roll call, so it is the shape of guard the repo asks for. Its one blind spot is the tests.rs exclusion noted under Standards.
Conventions: Closes #314 present, branch wayfinder/devlaunch-314 correct, CHANGELOG entry under [Unreleased].
Verdict
Approve. Nothing blocks the merge.
The claim this PR rests on is the strongest part of it. Every production body moved unchanged, measured two independent ways; the visibility surface outside flows::lifecycle is provably identical name-for-name, with sixteen items narrowed and none widened; and the new guard fails against three adversarial mutations of the root rather than only against yesterday's single file.
Six non-blocking findings, none of which should be fixed in this PR because all of them cost the byte-for-byte reassembly that makes it checkable. Worth a follow-up ticket, in rough order of value:
- Move
ClonePlacementandleaf_offromprune_plan.rstolocations.rs, deletingreconcile.rs's edge onto prune classification. - Give
only_one_module_can_remove_a_volumethe positive assertion its sibling has. is_declaration: accept inner attributes andpub(super) use/pub(super) mod.- One sentence on rule 3 saying what it actually bounds, and rename
the_lifecycle_root_declares_and_defines_nothing. - Record that
tests.rsis a deferral, not a finished half. - Consider whether
{notices, delete, delete_guard}should be one module.
CI is green except review and gate, which are red only because this report did not exist yet.
1 participant
Closes #314
flows/lifecycle.rsheld five unrelated commands — stop, delete, purge, prune, reconcile — plus the refresh latch, the fetch sweep and the on-disk placement rules that serve the launch path rather than any of them. It carried thirteen banner comments marking exactly where the modules were, and none of them had ever been made into one.The cut follows those banners, one banner to one file:
Layout follows the repo's own convention rather than the ticket's phrasing:
mod.rsis used for the three directory groupings (clients,domain,flows) and every leaf module with children isfoo.rsbesidefoo/(provision,agent_worktrees,git,workspace_state). Solifecycle.rsstays and becomes the table of contents.This is a move, and here is the proof
All three
cargo public-apisnapshots are byte identical. The members are privatemods and the glob re-exports are what is public, so there is still exactly one path to each of these types — the path callers already had.rust/dl/tests/lifecycle.rsneeded nousechange at all, which is the same fact from the other side.Generated with nightly and the pinned cargo-public-api 0.52.0 via
scripts/public-api-snapshots.sh, after a control run confirmed that toolchain reproducesorigin/main's three checked-in files byte for byte.Every body moved unchanged, measured rather than asserted. The cut was mechanical, by line range. A reassembly of the thirteen members in their original order — each with its added
//!doc anduseblock stripped — was diffed against the pre-split production region. What survives, hunk by hunk:pub(super)keyword, and the rustfmt rewrap a longer signature line caused.pub(super)is not a widening. It is precisely the visibility these items already had, since the module they were private to was the whole family.cargo docis back to zero unresolved links, matching main.lifecycle/tests.rsinherited.use super::*in a child module picks up the parent's private imports too, so the suite silently borrowed the file'suseblock; it now declares its own.tests.rs, because the suite lost one level of indentation and four columns came free.No statement body changed.
The guard
rust/devlaunch-core/tests/lifecycle_is_split.rs, in the spirit ofdevpod_layout.rs— a filesystem walk, three assertions, no hand-maintained roll call of module names:All three fail against today's single file and pass after the split. Scoped to this family on purpose:
flows::launchandflows::provisioncarry banners of their own and are not split yet, and a guard born with an exemption list is a guard that stops meaning anything.Two existing guards learned the new paths
Neither changed what it asserts:
volume_names' one door fordocker volume rmis nowflows/lifecycle/delete.rs.one_seam'sTEST_ONLY_FILES— files whose#[cfg(test)]gate is declared elsewhere — gainedflows/lifecycle/tests.rs, which is where theDevpodfake now lives.Gates
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check,pixi run pytest test/, andpixi run ci(pylint 10.00/10) all green.scripts/changelog_frozen.pyreports every released section untouched;git diff origin/main -- CHANGELOG.mdis 21 insertions and zero deletions.🤖 Generated with Claude Code