fix(reconcile): repair a pointer section instead of appending a second one - #55
Merged
Merged
Conversation
…d one `_ensure_pointer` recognised the pointer by heading AND `_POINTER_PREFIX`, so a section already headed `Archived` whose body lacked the prefix matched neither branch: the loop fell through and appended a SECOND `## Archived` beside it. #53 flagged this and left it. It is not hypothetical — that PR nearly shipped it. Its first end-to-end fixture wrote the legacy pointer with an ASCII `->` instead of the prefix's Unicode `→`, produced two `## Archived` sections, and passed the whole unit suite. Two ways in: an operator writes the heading by hand, or an older run's wording drifts from the constant. Matching on heading alone and branching on the body inside. A section under the pointer heading with no pointer line is now repaired rather than ignored. Repaired, not replaced. Replacing wholesale would discard whatever prose is already under that heading, and this module goes out of its way elsewhere to preserve an operator's annotations on a portable pointer — so the pointer line is inserted under the heading and the remaining body kept verbatim. That also keeps the invariant the caller actually needs: after a prune, exactly one `## Archived` section, carrying a correct portable ref. 6 tests. Each fails against the pre-fix predicate — checked by reverting it, which is worth stating because two of them did NOT fail on the first draft: the pre-fix code also preserves the prose and is also stable on re-run, just at two sections rather than one. Both now assert the section count, which is the thing that actually distinguishes the behaviours. Suite 488 -> 494 passed; the 16 failures are unchanged and identical, and `test_committed.py` / `test_signing.py` still fail to import for lack of `cryptography` in this environment. No narrative log entry, following #53: this repo's log is at exactly 400/400, so adding one would trip RC1 and force a prune to land a commit about pruning. Custodian ADR 0001 argues rationale belongs in the commit message instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ProtocolWarden
force-pushed
the
fix/ensure-pointer-duplicate-section
branch
from
August 4, 2026 20:01
bcc3864 to
76d4ee4
Compare
ProtocolWarden
added a commit
that referenced
this pull request
Aug 4, 2026
…#57) 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.
Summary
_ensure_pointerrecognised the pointer by heading and_POINTER_PREFIX, so a section already headedArchivedwhose body lacked the prefix matched neither branch — the loop fell through and appended a second## Archivedbeside it.#53 flagged this and deliberately left it. It is not hypothetical: that PR nearly shipped it. Its first end-to-end fixture wrote the legacy pointer with an ASCII
->instead of the prefix's Unicode→, produced two## Archivedsections, and passed the whole unit suite.Two ways in:
The change
Match on heading alone, branch on the body inside:
Repaired, not replaced. Replacing wholesale would discard whatever prose sits under that heading, and this module goes out of its way elsewhere to preserve an operator's annotations on a portable pointer. So the pointer line is inserted under the heading and the remaining body kept verbatim — which also holds the invariant the caller needs: after a prune, exactly one
## Archivedsection carrying a correct portable ref.Testing
488 → 494 passed. The 16 failures are unchanged and identical;test_committed.py/test_signing.pystill fail to import for lack ofcryptographyin this environment (pre-existing, unrelated).All 6 new tests were mutation-checked by reverting the predicate — worth stating precisely, because two of them did not fail on the first draft. The pre-fix code also preserves the prose and is also stable on re-run; it is just stable at two sections rather than one. Both now assert the section count, which is the property that actually distinguishes the behaviours. A test that passes against the bug it names is worse than no test.
Notes for reviewer
No narrative log entry, following #53's precedent: this repo's
.console/log.mdis at exactly 400/400, so adding one would trip RC1 and force a prune in order to land a commit about pruning. Custodian ADR 0001 argues rationale belongs in the commit message rather than duplicated into the log.Left unfixed, deliberately: ContextLifecycle#54 reports a separate defect in the same file —
prune --dry-rununder-reports, because_retain_recent_logarchives unclaimed sections by age that never appear inplan.moves. That is a policy question about whatrecent_nmeans, not a predicate bug, so it does not belong in this change.🤖 Generated with Claude Code