fix(reconcile): make the prune plan describe what apply actually does - #57
Merged
Merged
Conversation
Closes #54. `prune --dry-run` printed N moves and `--apply` archived N plus every unclaimed section past `recent_n`. The extra sections were computed inside `_apply_plan_locked` and never reached `plan.moves`, which is all `format_plan` renders — so nothing reported them, before or after. The operator approved one thing and got another. What makes it bite rather than merely surprise: those sections are claimed by no item AND cleared by no gate. The DOC GAP gate only inspects *done* items, so anything still `partial` is never examined — and `owned_done` excludes it too, so its sections are never claimed and land in the age-swept bucket. Incomplete, undocumented work is therefore the MOST likely thing to be archived by age, which inverts what the gate is for. Destination is the private manifest, so on a public repo the content also changes repos on its way out. Three changes, no policy change: - `build_plan` records the age-swept sections as `PlannedTrim`, and `format_plan` prints them under their own heading, named for what they are. A separate type from `PlannedMove` on purpose: a move was released by the gate, a trim was swept by a number, and conflating them in the thing an operator approves is how this stayed invisible. - `PrunePlan` carries the `recent_n` it was built with, and `apply_plan` defaults to it. The two defaulted independently, so planning with `--recent 20` sweeps to 10 at apply time — a second, quieter divergence. An explicit `recent_n=` still overrides. - `is_noop` deliberately keeps ignoring `trims`: age-sweeping is a side effect of an otherwise-legitimate prune, never a reason for one. Counting trims there would start pruning repos that are today correctly left alone. There is a test pinning that. Not addressed here, because they are policy and belong to the spec owner: whether `recent_n` should sweep unclaimed sections at all, and whether the reconcile and trim operations should be separable. #54 lists both; this change only makes the current behaviour honest. 5 tests. Each of the three changes was reverted in turn to confirm a named test fails — the end-to-end one, which diffs the dry-run text against the headings that actually land in the archive, catches two of them on its own. Suite 493 -> 498 passed; the 17 failures are unchanged and identical. No narrative log entry, following #53 and #55: this repo's log is at 400/400, so adding one would trip RC1 and force a prune to land a commit about pruning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Closes #54.
Summary
prune --dry-runprinted N moves and--applyarchived N plus every unclaimed section pastrecent_n. The extra sections were computed inside_apply_plan_lockedand never reachedplan.moves— which is allformat_planrenders — so nothing reported them, before or after. The operator approved one thing and got another.What makes it bite rather than merely surprise: those sections are claimed by no item and cleared by no gate. The DOC GAP gate only inspects done items (
check.py), so anything stillpartialis never examined — andowned_doneexcludes it too, so its sections are never claimed and land in the age-swept bucket. Incomplete, undocumented work is therefore the most likely thing to be archived by age, which inverts what the gate is for. Destination is the private manifest, so on a public repo the content also changes repos on its way out.Changes — no policy change
build_planrecords the swept sections asPlannedTrim, andformat_planprints them under their own heading. A separate type fromPlannedMoveon purpose: a move was released by the gate, a trim was swept by a number, and conflating them in the thing an operator approves is how this stayed invisible.PrunePlancarries itsrecent_n, andapply_plandefaults to it. The two defaulted independently, so planning with--recent 20swept to 10 at apply time — a second, quieter divergence. An explicitrecent_n=still overrides.is_noopstill ignorestrims. Age-sweeping is a side effect of an otherwise-legitimate prune, never a reason for one; counting trims there would start pruning repos that are today correctly left alone. Pinned by a test.Testing
493 → 498 passed; the 17 failures are unchanged and identical (test_committed.py/test_signing.pystill fail to import for lack ofcryptographylocally — pre-existing, and CI installs it).All three changes were reverted in turn to confirm a named test fails. The end-to-end test — which diffs the dry-run text against the headings that actually land in the archive file — catches two of the three on its own, which is the property the issue is really about.
Two of the five tests are contract tests rather than regression tests, and I'd rather say so:
test_explicit_recent_n_still_overrides_the_planandtest_trims_alone_do_not_make_a_noop_plan_actpass against the pre-fix code too. The first documents that the override survives; the second guards a future regression, since foldingtrimsintois_noopis the obvious wrong next step.Deliberately not addressed
#54 lists four possible directions. This PR implements only the first — make the plan honest — because the other three are policy calls that belong to the spec owner:
recent_nshould sweep unclaimed sections at all--no-trim-unclaimed)Both remain open. This change makes the current behaviour visible without deciding what it ought to be.
🤖 Generated with Claude Code