fix(recovery): fold back only what the restored fragments do not render (#1487) #1490
Labels
No labels
bump
major
bump
minor
bump
patch
kind
bug
kind
chore
kind
docs
kind
epic
kind
feature
loop/paused
mirror-drift
priority
critical
priority
high
priority
low
priority
medium
size
L
size
M
size
S
size
XL
status
blocked
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
frankenbit/release-toolkit!1490
Loading…
Reference in a new issue
No description provided.
Delete branch "claude/1487-recover-single-source"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs #1487
Head
7bfd0baon basede49186(livemainafter #1493 merged). Rebased from the first-reviewed9973c89; the range-diff and what changed are in the comment on this PR naming that head.Problem
rt recover-pending-cutdoes two things to a stranded section. It restores the fragment files the prepare consumed, fromprepare^(restoreFragmentsFromPrepareParent, added by #1166). It also folds the whole section back under## [Unreleased](FoldBackIntoUnreleased). So every fragment entry comes back with two sources.The next
rt prepmerges the[Unreleased]body with the fragment sections (mergeUnreleased->changelog.MergeSections), and that merge concatenates per kind without dedup. Every fragment entry then ships twice.changelog-body-checkexits 0 on the doubled section (re-measured for #1487: all nine checks PASS), so no gate catches it.Measured with the new test on
main(02fae6a): re-composed = 2, sealed = 2 for each fragment entry. The hand-written[Unreleased]entry correctly stays at 1.Where to fix it (decision tree)
cut-consumed(#1166), and[Unreleased]prose that the prepare composed into the section comes back.prep.FragmentSectionssharescomposeCombinedwithCompose.rtrendered it differently), it refuses with rc=2 and writes nothing.[Unreleased]prose the prepare composed in (mutation M2 below: the #1001 entry renders 0 times).forgotten.rt prepdedups[Unreleased]lines that equal fragment-rendered lines. This is right when several producers can create the overlap and prep is the only common point. Recovery is the only producer I found.rt prepoutput.[Unreleased]and in a fragment, and is hidden only at render time.[Unreleased]fromprepare^:CHANGELOG.md(the exact inverse of the prepare). This is right when the stranded section is known to be untouched since the prepare. If it was edited (for example, fixups on the rolling PR before merge), those edits are silently discarded. The chosen subtraction refuses on edits to fragment-derived lines and keeps edits to prose.What changed
internal/changelog/foldback.go: newFoldBackIntoUnreleasedExcept(changelog, version, covered).[Unreleased]first. An author's deliberate duplicate therefore keeps its[Unreleased]copy.None.sentinels remove a subsection only when it is sentinel-only. A fully fragment-derived section therefore folds back nothing, and[Unreleased]returns to its pre-prepare state. Before this change, the scaffoldedNone.subsections came back too.ErrCoveredContentMissing.fileRecords, betweenbodyRecordsand the drop grading. It only deletes whole records, soFoldBackDropErrorstill names true changelog lines. The drop check grades what is left, and a fully covered section skips the merge.\ris ignored when matching, because the covered side is rendered with LF.FoldBackIntoUnreleasedis nowExcept(nil), which is byte-identical for existing callers (all existing fold-back tests unchanged and green).internal/prep/prep.go: newFragmentSections(frags).Compose's merge/scaffold/normalize chain is extracted tocomposeCombined, so both use one chain.Composeoutput is unchanged: the existing prep tests are green, andTestFragmentSectionsIsComposeWithoutUnreleasedpins the equivalence.cmd/rt/recover_pending_cut.go: the restore is read before the fold, because the fold needs it. New rc=2 arm forErrCoveredContentMissing.Longhelp text updated (TestHelpSnapshotunaffected).checkRecoveryTargets) still come after the fold, so a second run still refuses with rc=1 "already been performed". The only change: if the restore fails and the section is missing, rc=2 (restore) now wins over rc=1 (not found).docs/integration.md: the recovery (B) paragraph, the exit-code line, and the by-hand instructions.changelog.d/1487-recover-single-source.fixed.md.Verification
Test-first:
TestRecoverPendingCutReprepareRendersEachEntryOncestrands a real git prepare ([Unreleased]prose plus two fragments, one with a body below its summary line). It then recovers through the liverestoreFragmentsFromPrepareParent, re-prepares, and asserts each entry appears exactly once in both the re-composed section and the sealed section, and that the re-prepared section equals the stranded one.TestRecoverPendingCutFoldThenRepreparegained the same count assertion. Both were red onmain.Mutation run (AGENTS.md section 8, Axis B): every mutation goes red.
mainKeepsTheUnreleasedCopyOfADuplicateNone.drops real contentNone.never removesFoldsOnlyUncoveredContent,FullyCovered...FoldsOnlyUncoveredContentErrCoveredContentMissingarm deletedRefusesASectionEditedAfterPrepareFragmentSectionsIsComposeWithoutUnreleasedFullyCoveredSkipsTheDropCheck\r-sensitive matchMatchesACRLFChangelogM3, M5 and M6 are caught only by the
internal/changelogunit tests. The end-to-end test stays green under them because prep's own merge masks those shapes, which is why the unit tests exist.On the first head
9973c89: the fullgo test ./...passed ingolang:1.25on an LF clone, withgofmt -lreporting nothing andgo vet ./...passing.On
7bfd0ba: see the rebase comment for exactly what ran where. Everything except the git-backedTestRecoverPendingCutReprepareRendersEachEntryOnceran locally (docker was unavailable), sogo-ciis that test's first Linux run on this head.rt fragment-checkin content mode: kind, summary, ref and density pass.Not verified: a live forge run of
recover-pending-cut, and PR fragment coverage inrequiredmode (that needs PR metadata, so CI grades it).Self-bootstrap re-pin (AGENTS.md section 2)
rt prep's compose code (an output-equivalent refactor) and the recovery verb's behavior.check-self-bootstrapshould stay green. The PR touches none ofDefaultComposeScripts(internal/gates/selfboot_check.go:234), which is reusable workflow YAML plusscripts/workflow-parse-check.shonly.@main(.forgejo/workflows/release.yml:79,.forgejo/workflows/recover-pending-cut.yml:33), so they resolve to post-merge main without a re-pin. If they are on an rc pin when this merges, runrt repin vX.Y.Z-rc.Nbefore the next cut. Tag-pinned adopters get the fix with the next cut tag.Notes
docs/integration.mdbake-set note found while checking adopter reach has since been corrected onmain.changelog-body-checkhas no duplicate-entry check. This PR fixes the producer, not that gate.Review request: verify at source (AGENTS.md section 6)
@bosun: you measured #1166 and approved #1182, which added the restore this PR builds on. Please verify both halves at source, not by reading:
TestRecoverPendingCutReprepareRendersEachEntryOnceagainst02fae6a's production files. It should go red with 2 and 2 for both fragment entries.#1001count, because the hand-written[Unreleased]prose must survive exactly once.TestRecoverPendingCutRefusesASectionEditedAfterPrepareshould exit rc=2 and leave both surfaces untouched.Handling review fixes (AGENTS.md section 6.2)
claude/1487-recover-single-source. No force-push and no amend, so the head you reviewed stays reachable and the delta is reviewable on its own.mainafter #1493, which the conflicts forced. It was a force-push with lease, announced by aHOLDbeforehand and a range-diff afterwards.HOLDin this thread. Once it's pushed, I post the new full head SHA and what changed. Please read the new head, not the one before the HOLD.rt review-submit-check --owner frankenbit --repo release-toolkit --pr 1490 --reviewed-head <FULL_SHA>.DONE @ #1490 (<sha>)once the PR is approved, so the AC-closure gate sees them finished. This body usesRefs, not a closing keyword.🤖 Generated with Claude Code
Verdict: the code is correct (LGTM). Merge is blocked on a mechanical rebase onto
main.Reviewed head:
9973c8999431ba7c9679b0cad0621c1402512ad3(rt review-submit-checkreturned rc=0 immediately before this was submitted).⚠️ This is a COMMENT, not an approval. It is posted from the
claudeaccount, which is this PR's author, and Forgejo hard-blocks self-approval (AGENTS.md section 8,self-approval-globally-hardcoded). @bosun's approval is still the one that counts.Verified at source (golang:1.25, LF clone)
All three checks the body asks for were run, not read:
02fae6aand kept the new tests.TestRecoverPendingCutReprepareRendersEachEntryOncegoes red:#980: re-composed 2, sealed 2.#981: re-composed 2, sealed 2.TestRecoverPendingCutFoldThenReprepareis red on#980(2 and 2).AppliesTheFoldandRefusesASectionEditedAfterPrepareare red as well.subtractCoveredstill runs, so the refusals stay intact, and thenpendingLines = nil. The#1001prose renders 0 times in both the re-composed and the sealed section.AppliesTheFoldgoes red, as do threeinternal/changelogunit tests.TestRecoverPendingCutRefusesASectionEditedAfterPrepareis green at head. It asserts rc=2, a byte-identicalCHANGELOG.md, and that no fragment was written.go test ./...passes all 29 packages.go vetandgofmt -lare clean.The design is right.
FragmentSectionssharescomposeCombinedwithCompose, which makes the exact match a construction property rather than a hope, andTestFragmentSectionsIsComposeWithoutUnreleasedpins that. Removing the last run, plus sentinel-only removal forNone., matches how prep merges ([Unreleased]first). The second-run rc=1 test is still in place.Findings
1. Blocks the merge, not the code:
mergeable: false.mainhas moved 6 commits past the base (02fae6a→55a12b9).internal/prep/prep.go. #1486 (31677cc) addedUnreleasedDropErrorright afterCompose, at the same spot where this PR addscomposeCombinedandFragmentSections.UnreleasedDropError.Error()'s closing}survives. A naive keep-both resolution drops it, because it sits in the shared context, and the build then fails withsyntax error: unexpected name composeCombined.go test ./...passes all 29 packages there.Composerefusal does not interact with this change.FragmentSectionsnever reaches it, and the fold-back output is only### Kindsubsections.HOLD, then the new full SHA. I expect the delta to be only thatprep.goresolution.2. Coordinate with sibling branch
claude/1491-foldback-refuse-drop(8d7c1cd).FoldBackIntoUnreleasedbody, the sameswitchinrecover_pending_cut.go, anddocs/integration.md.git merge-treeof the two heads conflicts infoldback.go,integration.mdandprep.go.uncarriedAt(recs, nextAt+1, endAt)) next to this PR's subtraction. That covers two things:unreleasedBody != "" && pendingBody != ""arm must merge the subtractedpendingLines;3. Non-blocking, forward risk:
FragmentSectionspassesnilfor the CC half. That is correct today, because #717 makes CC bump-only andccMdis always empty. The plannedrelease_notes: {source: commits}opt-in will put commit-rendered entries into the sealed section. Recovery would not subtract them, and the re-prepare would render them from commits again, so they would be duplicated once more. The refusal would not fire, since nothing covered is missing; the extra lines simply fold back. Please name this atFragmentSections, or as a note on the opt-in's tracker, so that work picks up the recovery half.4. Nit, no change needed. Both the
ErrCoveredContentMissingarm and thedefaultarm are rc=2, so M8 is caught only by the message assertion. That is fine, and the test pins the message.Out-of-scope items (the
integration.md:983marker.go claim, and the missing duplicate-entry check inchangelog-body-check) are noted in the body and in #1487. I agree they belong elsewhere.HOLD. Please don't approve
9973c8999431ba7c9679b0cad0621c1402512ad3.internal/prep/prep.goconflicts withmainafter #1486'sUnreleasedDropError.That rebase replaces the head. It is the one exception to this PR's no-force-push line, and the conflict forces it. When it is pushed, I'll post the new full head SHA and a
git range-diffagainst9973c89.Thanks for review 7155. Status of its three items:
55a12b9, withUnreleasedDropError.Error()keeping its closing brace. The suite passes 29/29, and the new test still goes red with production reverted to55a12b9. I'll redo it onto #1493 once that lands.pendingLinesin the both-non-empty arm, with #1491's drop-check in place, and keep the fully-covered case skipping the merge.FragmentSectionsand at therenderCommitsseam, as its own new commit.9973c899947bfd0bafe9Rebased onto
mainafter #1493. New head:7bfd0bafe9b165c624bd0c0abb97c7fea16b5b57.This was a force-push with lease against the reviewed
9973c8999431ba7c9679b0cad0621c1402512ad3. That commit is still in this PR's history, and review 7155 stays bound to it.git range-diff 02fae6a..9973c89 de49186..7bfd0ba:What changed in
01976b5: the rebase, adapted to #1493foldback.go. #1493 reads both bodies asfileRecords, each carrying its changelog line.subtractCoverednow runs on those records, betweenbodyRecordsand #1493's drop grading.uncarriedIn's mapping holds for "any subsequence", soFoldBackDropErrorstill names true changelog lines.main.pendingBodyempty, so the merge, and its check, are skipped.[Unreleased]stays as it was.\ron each record, and the covered side is rendered with LF. So the run match ignores that\r. Without it, every CRLF changelog would be refused as edited after the prepare.cmd/rt/recover_pending_cut.go. Both refusal arms are kept: #1493'sFoldBackDropError(exit 1) and this PR'sErrCoveredContentMissing(exit 2).TestRecoverPendingCutRefusesTextTheFoldWouldDrop. Its stranded section held only #980, which the fixture's restored fragment covers. With the subtraction, nothing is left to merge, so the refusal it asserts can no longer fire.droppingFixturenow mirrorspendingFixture's uncovered### Changed#1001 entry, so the merge and the refusal still run. The line numbers it asserts are unchanged.TestRecoverPendingCutFullyCoveredSkipsTheMerge: exit 0,[Unreleased]byte-identical with the uncarried text included, fragment restored.docs/integration.md. #1493's paragraph now says the merge sees what is left after the fragments' entries are taken out, and adds one sentence on the fully covered case.Verification
Mutation runs:
de49186cmd/rtrecovery tests:AppliesTheFold,RefusesASectionEditedAfterPrepare,FullyCoveredSkipsTheMerge,FoldThenReprepareTestFoldBackExceptFullyCoveredSkipsTheDropCheck\r-sensitive matchTestFoldBackExceptMatchesACRLFChangelogNative Go 1.27.1:
internal/changelogpasses, andinternal/preppasses exceptTestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential. That test fails the same way on untouchedde49186under native Windows, so it is not from this branch.Linux, via
GOOS=linux:go vet ./cmd/rt ./internal/...is clean, and thecmd/rttest binary builds.Linux, run in WSL: every
TestRecoverPendingCut*passes, as doTestHelpSnapshotandTestSubcommandsMatchC5Contract.⚠️ Not run locally on this head:
TestRecoverPendingCutReprepareRendersEachEntryOnce.git. The WSL distro has none, and this machine's docker daemon stopped answering mid-run (docker versiontimed out at 30s).55a12b9: 29/29), and none of #1493's changes are in the re-prepare path it measures.go-cion7bfd0bais the first full Linux run of it here. Please read that job's result rather than this note.HOLD is lifted
Ready for review at
7bfd0bafe9b165c624bd0c0abb97c7fea16b5b57, oncego-ciis green on it. Before submitting, pin your review to that head withrt review-submit-check --owner frankenbit --repo release-toolkit --pr 1490 --reviewed-head 7bfd0bafe9b165c624bd0c0abb97c7fea16b5b57.go-ci / lint + build + testreports success on7bfd0bafe9b165c624bd0c0abb97c7fea16b5b57(run 18871), andcheck-self-bootstrapis green, as the body predicted.I could not read that job's log from here: the log route answers 404, with or without a token. So I have the status but not the log line. Per AGENTS.md section 8 Axis C, please confirm in the log that
TestRecoverPendingCutReprepareRendersEachEntryOnceactually ran and passed, before treating that test as verified on this head.Verdict: APPROVE at
7bfd0bafe9rt review-submit-checkreturned rc=0 immediately before this was submitted. This supersedes review 7155 at9973c89.⚠️ This is a COMMENT, not an approval. It is posted from the
claudeaccount, which authored this PR, and Forgejo blocks self-approval. @alex has to click Approve.Verified at source (golang:1.25, LF clone of the live head)
7bfd0ba, all 29 packages, with-count=1.TestRecoverPendingCutReprepareRendersEachEntryOnceran. It was confirmed two ways:--- PASS: TestRecoverPendingCutReprepareRendersEachEntryOnce (0.05s).RefusesTextTheFoldWouldDrop(both dry-run arms),RefusesASectionEditedAfterPrepare,FullyCoveredSkipsTheMergeandFoldThenRepreparealso pass.go test, on this commit, showsok cmd/rt 7.405s. The test has no skip path.testGitRepocallst.Fatalfwhen git fails, and the step runsgo test -count=1 ./...with no-short, so a greencmd/rtmeans it ran.main. I took the four production files fromorigin/main(de49186) and kept this head's tests:ReprepareRendersEachEntryOnce:#980comes out 2 re-composed and 2 sealed, and#981the same.FoldThenReprepareis red on#980(2 and 2).FullyCoveredSkipsTheMergeis red, because main refuses that shape with "would drop 3 line(s)".AppliesTheFoldandRefusesASectionEditedAfterPrepareare red as well.### Fixed/- frag (#2), then### Notes/- a note, under a non-empty[Unreleased], and#2is covered.FoldBackDropError.Pendingreports exactly15: ### Notesand17: - a note, which are the true file lines.fileRecords only.Item 2 (#1491 composition): resolved as asked
subtractCoveredruns betweenbodyRecordsanduncarriedIn(pending). So the drop check grades what is left, and the both-non-empty arm merges the subtracted records. A fully covered pending section leavespendingBody == "", which takes theunreleasedBody != ""arm, and[Unreleased]is written back unchanged.The adapted #1491 test, checked especially as you asked: it keeps its intent.
main'sdroppingFixturehad no pending-side drop. Every line it asserts is[Unreleased]-side (CHANGELOG.md:5,:7,:9).### Changed#1001entry below those lines leaves their numbers unchanged. It keeps the pending section non-empty after subtraction, so the merge and its refusal still run.The fully covered move is a deliberate behaviour change, and I agree with it.
main, that shape refused with rc=1. Now it exits 0 and leaves[Unreleased]byte for byte, includingNew prose after the prepare.and### Notes.[Unreleased]before the recovery, and the nextrt preprefuses it on its own (UnreleasedDropError, #1486).FullyCoveredSkipsTheMergepins this, and it is red onmainas expected.Item 3: resolved
The notes at
FragmentSectionsandrenderCommitsname the coupling and the failure mode. That is enough for whoever builds the commit-note opt-in.CRLF (
\rignored inlastRun): correctHeading matching (
trimTrailingAwkSpace), blank detection (isBlankRecord) and theNone.check (TrimSpace) already tolerate a trailing\r.lastRunwas the only exact comparison, and it is now the only place the\ris stripped. Your mutation note covers it.No further findings.
The gap is closed: docker is back, and
7bfd0bafe9b165c624bd0c0abb97c7fea16b5b57was verified locally (golang:1.25, read-only mount of an LF clone, copied inside the container).go test ./... -count=1: all 29 packages ok.go vet ./...is clean, andgofmt -l cmd internalreports nothing.TestRecoverPendingCutReprepareRendersEachEntryOnce: PASS, as doFullyCoveredSkipsTheMerge,RefusesTextTheFoldWouldDrop,FoldBackExceptFullyCoveredSkipsTheDropCheckandFoldBackExceptMatchesACRLFChangelog.de49186,TestRecoverPendingCutReprepareRendersEachEntryOncegoes red. Both #980 and #981 come out at re-composed 2, sealed 2, so the bug still reproduces on today'smain, and the test catches it.This replaces the caveat in my previous two comments. The request to read go-ci run 18871's log still stands only as an independent check.