fix(recover-pending-cut): refuse a fold-back whose merge would lose text (#1491) #1493
No reviewers
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!1493
Loading…
Reference in a new issue
No description provided.
Delete branch "claude/1491-foldback-refuse-drop"
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 #1491
Base
main. Head2060ed13ffb3834928b2aad3a32916eab485acb3, three commits on55a12b9:ff3345f: the change. It was rebased from8d7c1cdonce #1486 landed;git range-diffshows only surroundingdocs/integration.mdtext changed.0935704: answers review 7158 (findings 1-6).2060ed1: the non-blocking nit from re-review 7160. The refusal now opens with what was not written, so the remedy's "them" directly follows the headings it refers to.Each is a new commit, with no amend or force-push.
Problem (measured)
As filed in #1491:
FoldBackIntoUnreleased(cl, "0.2.0")returnserr=nilwhen[Unreleased]holds prose plus a### Notesblock and the pending section holds### Added/- Shipped..[Unreleased]comes back as### Added\n\n- Shipped.only. The new tests reproduce this: on the pre-fix tree, every refusing case failed witherr = <nil>.The pending section can lose text the same way, for example through a hand-edited
### Notesin the stranded section.What changes
changelog.FoldBackIntoUnreleased: the merge arm runs only when both bodies are non-empty. It now grades both bodies before merging and returns*changelog.FoldBackDropError, which names each line by its CHANGELOG line and returns no changelog.[Unreleased]'s last###line (Inherited). That is what decides whether a pending line above its first heading is moved or dropped.rt recover-pending-cut: routes the error to rc=1 (refused), with--dry-runtoo, before anything is written. Verbatim from the verb at2060ed1(--dry-run, exit 1):A pending line above its first heading reads, depending on
[Unreleased]'s last heading:changelog.AcceptedHeadingsNote/changelog.UncarriedRemedy: one source for the closing lines of both refusals.rt prepnow uses them too, and its text is byte-identical (pinned ininternal/prep/unreleased_drop_test.go).docs/integration.md§ recovery (B): adds the new refusal to the exit-code contract, plus a paragraph on which text the fold refuses.changelog.d/1491-recover-pending-cut-drop.fixed.md.Review 7158
0935704Inherited+movedLine. The reason says move only when that heading is accepted, otherwise drop; with no heading it says so.TestFoldBackDescribeSaysWhatTheMergeDoes. The oracle now measures move vs drop inMergeSections' output for every reported line and compares it with the error's own classification.\ron a CRLF changelogTextandPendingHeadingstay verbatim in the struct.TestFoldBackDescribeOnACRLFChangelog(same file lines as LF, no\rin the text)would drop N line(s),would move N line(s) under a heading they were not written under, or both, counted separatelyTestFoldBackDescribeSaysWhatTheMergeDoes(moved, dropped, both)uncarriedAtassumes a contiguous slicefileRecord,bodyRecords,joinRecords,uncarriedIn: the merge input and the grading come from one record sequence, and the mapping holds for any subsequenceAcceptedHeadingsNoteandUncarriedRemedyinmerge.go, used by both refusalsFoldBackDropError; the verb prints it (since2060ed1, as its first line)Describemust not say it; the verb's output must open with itAlso measured, kept out of scope:
rt prep'sUnreleasedDropErrorprintsl.Textverbatim too, so on a CRLF CHANGELOG its refusal line carries the same raw\r(Go probe at0935704). It is #1486's surface, not #1491's, so it is left for a separate issue and PR (not filed yet).Decision tree
The pending body: check it, and how?
The pending body is
MergeSections' second argument. The merge's current kind carries into it from[Unreleased]'s last heading, andUncarriedLinesis documented as exact only for the first argument.### Notesis dropped (testa hand-edited heading is dropped).[Unreleased]. Right if a line moved under a heading it was not written under still counts as preserved. With this, a pending intro line would land under[Unreleased]'s last heading, straight after its last bullet, where markdown reads it as that bullet's continuation.UncarriedLines(taken). Right when a line survives only under the heading it was written under:###heading on, the result equals the carry-aware one, because a heading resets the kind.0935704the refusal says which.The oracle test defines "lost" independently, as "not emitted under its source heading". It grades against
MergeSections' own output, not againstUncarriedLines.Which paths refuse?
[Unreleased]) and the empty-pending path. Nothing is lost there, and the nextrt preprefuses the same text under #1486. Both are pinned byte for byte.Exit code
[Unreleased]section" refusal.Interaction with #1490
#1490 (for #1487) leaves the merge arm alone. It builds the pending body from
subtractCovered(recs[nextAt+1:endAt], covered), though. Since0935704, the bodies are[]fileRecord, anduncarriedInmaps line numbers correctly for any subsequence of the file's records. Whichever PR lands second only needssubtractCoveredto filter the pending records (or to return records). The grading then follows automatically, because it reads the same records the merge is fed.#1491 acceptance criteria
[Unreleased]: refuse and write nothing on anyUncarriedLineshit, naming lines +MergeKindsTestFoldBackRefusesUnreleasedTextTheMergeWouldDrop,TestRecoverPendingCutRefusesTextTheFoldWouldDrop(both modes)TestFoldBackRefusesPendingTextTheMergeWouldLose,TestFoldBackDescribeSaysWhatTheMergeDoesTestFoldBackVerbatimArmsDoNotRefuse(splice + empty-pending)main, mutation-verifiedFoldBackDropError, which this PR adds. Mutation tables belowRefs, not a closing keyword, as in #1490. Ticking the ACs is the reviewer's call.Verification
Mutation runs (AGENTS.md section 8, Axis B). The clone was mounted read-only, and each mutant ran on a copy.
19/19 killed at
0935704: 12 for the review findings, plus the 7 behavioural guards from the first round re-run on the refactored code. At2060ed1the verb-side mutants were re-run (5/5 killed), including the old ordering.###, not the last\rin line text / in the pending headingTestFoldBackPreservesTheBodyByteForByteTestFoldBackEdgeShapes2060ed1order)2060ed1)[Unreleased]check / no pending check / carry-aware pending checkTestFoldBackVerbatimArmsDoNotRefuse(each)The first round's other three mutants (the two old line-offset bugs and
Describedropping the "moved" reason) target code0935704replaced. The record-mapping, leading/trailing-blank and swapped-reason mutants cover them now.2060ed1,go test -count=1 ./...(29 packages), gofmt,go vetand thert --helpsnapshot, ingolang:1.25on an LF clone.rt fragment-check(density + issue reference),rt register-checkandrt changelog-body-checkpass.ff3345f, the commit statuses report success on all 30 contexts,go-ci / lint + build + testincluded. At0935704and at2060ed1, the same: success on all 30 contexts,go-ci / lint + build + testincluded. These are reported statuses (AGENTS.md section 8, Axis C); I did not read the job logs.AGENTS.md notes
recover-pending-cutis not one of the cut verbs, so no re-pin is needed.2060ed1, both halves of the refusal: the lines it names, and what it says the merge does with each one.🤖 Generated with Claude Code
8d7c1cd781toff3345f3e4Review of head
ff3345f3e4eff91aaff1e4cb3c67f0544490a299(rt review-submit-checkrc=0 against this SHA right before submitting).Posted as COMMENT because this account authored the PR, so it cannot approve or request changes. Findings 1-3 are what I would hold the merge on.
The detection is sound. The merge arm grades both bodies before merging, and
uncarriedAt's file-line offset is correct. The oracle test grades againstMergeSections' own output and agrees with the refusal on all 11 pairs.go test ./internal/changelogpasses at this head. Not run:cmd/rttests, because the package doesn't build on Windows (syscall.Flock) and this was a Windows host.Findings
1.
internal/changelog/foldback.go:174: a pending intro line is reported as "moved" when the merge drops it.Describelabels every pending line above the first###headingthe merge would move it under [Unreleased]'s last heading. That is false when [Unreleased]'s last heading is not accepted, or when [Unreleased] has no###heading at all: the inherited kind is then unaccepted or"", and the line is dropped. Measured on the oracle's own fixturepending intro inherits an unaccepted kind:The last heading there is
### Notes. The same text appears with [Unreleased] =Just prose., which has no heading at all. No test checks this reason string for the unaccepted-inherited case.2.
foldback.go:165: on a CRLF changelog, a raw\rreaches the refusal text.PendingHeadingandUncarriedLine.Textare copied verbatim from the records. Measured:"folding ## [v0.2.0] - 2026-10-02\r back under ..."and"line 5: Prose.\r <- before the first ### heading". In a terminal the\rreturns to column 0, so the rest of the line overwrites the heading, line number and text. The merge grammar handles CRLF (classifyMergeRecordtrims it), andReason()already strips\rfromHeading, butTextandPendingHeadingare not stripped.3.
foldback.go:165: the header says "would lose N line(s)" but counts moved lines. With [Unreleased] =### Fixed / - Later fix.and pending =Release intro. / ### Added / - Shipped., the output sayswould lose 1 line(s)and then lists that one line asthe merge would move it. The header should say "lose or move", or count the two classes separately.4.
foldback.go:194:uncarriedAtassumes the body is a contiguous slice ofrecs. That holds today, but not under #1490'ssubtractCovered, as the PR body says. Suggestion: carry each record with its file line through to the merge input, so the merge input and the grading come from one sequence and the subtraction cannot split them.5.
foldback.go:182: duplicated text. The accepted-headings line is a copy ofinternal/prep/prep.go:175-180, and the "Move each line under one of them, or delete it, then re-run" remedy is duplicated too. One helper next toMergeKinds()would keeprt prepandrt recover-pending-cutfrom drifting apart.6.
foldback.go:153: a pure library error claims "Nothing was written."FoldBackDropErrorlives ininternal/changelogand never writes anything, so that sentence describes what the caller did. It holds for today's only caller. Any future caller that printsError()after a partial write (e.g. #1490's Except variant) would report it falsely.prep.UnreleasedDropErrorkeeps this sentence in its own error type. Move it to the verb'sred(...)wrapper.PR body is out of date
It still says Stacked on #1486, head
8d7c1cd, and "this stacked PR gets no CI until it is retargeted". The PR now targetsmain(merge base55a12b9) with headff3345f.git range-diff e2fc222..8d7c1cd origin/main..ff3345fshows the only change is surrounding text indocs/integration.mdfrom the rebase, but the Verification section names a head that is no longer the PR head. Please update the body and state the CI result onff3345f.🤖 Generated with Claude Code
HOLD: I'm addressing review 7158 (findings 1-6, plus the out-of-date body) in a new commit on top of
ff3345f3e4eff91aaff1e4cb3c67f0544490a299, with no amend and no force-push. I'll post the new full head SHA once it is pushed and the body is updated. Please wait for that before any approval read.HOLD released. New head:
0935704fcb110793e0c3da3e20367b11dd68d941. It is one new commit onff3345f, with nothing rewritten.mainand maps each finding to its change and test.rt prep's refusal has the same CRLF\rexposure. It is left for a separate issue.Before submitting, pin the read to this head:
Re-review of head
0935704fcb110793e0c3da3e20367b11dd68d941(rt review-submit-checkrc=0 against this SHA right before submitting). Posted as COMMENT because this account authored the PR, so it cannot approve. Review 7158 is addressed at this head, and nothing here holds the merge.Process
One new commit on
ff3345f, with nothing rewritten (git merge-base --is-ancestorholds). HOLD was announced before the push (113872) and released after it (113876).Findings 1-6: verified at source
Messages below are measured at
0935704with the inputs used in 7158:the merge would drop it under [Unreleased]'s ### Notes, not an accepted heading. No heading at all:[Unreleased] has no ### heading to carry it; the merge would drop it.\r.Text/PendingHeadingstay verbatim in the struct, andunCRapplies only when rendering.would move 1 line(s) under a heading they were not written under. Drop-only readswould drop N line(s), and the mixed case reports both counts.bodyRecords/joinRecords/uncarriedIn: the merge input and the grading come from one[]fileRecord.Line = rs[Line-1].linemaps correctly for any subsequence.AcceptedHeadingsNote()/UncarriedRemedy(verb)sit next toMergeKinds.prep.UnreleasedDropError's output is byte-identical to the old literal (old%s.\n+ remedy vs new note +"\n"+ remedy).FoldBackDropErrorand now comes from the verb'sred(...).Verification
go veton the changed packages,gofmt -lclean, andgo test -count=1 ./...all pass ingolang:1.25on a fresh LF clone of0935704, with no failing package. That includescmd/rt, which I could not run on the first pass.0935704:success, 30/30.main, head0935704, no more "stacked on #1486".Nit (non-blocking)
The verb now prints
...Accepted headings (...)\nNothing was written.\nMove each line under one of them, ..., so "them" no longer follows the headings it refers to. Printing "Nothing was written." beforeDescribe(...), or as part of its first line, restores that. This is a wording change only.The CRLF
\rinrt prep'sUnreleasedDropErroris acknowledged as out of scope here. It needs its own issue.🤖 Generated with Claude Code
HOLD: I'm applying the non-blocking nit from re-review 7160 (the remedy's "them" should follow the headings it refers to) as a new commit on top of
0935704fcb110793e0c3da3e20367b11dd68d941, with no amend and no force-push. I'll post the new full head SHA once it is pushed.HOLD released. New head:
2060ed13ffb3834928b2aad3a32916eab485acb3. It is one new commit on0935704, with nothing rewritten.recover-pending-cut: nothing was written., and "Move each line under one of them" directly follows the accepted-headings line.Before submitting, pin the read to this head: