fix(gates): decide sentence-finality instead of matching punctuation (#738) #746

Merged

bosun merged 3 commits from i/738-sentence-finality into main 2026-08-19 12:16:52 +02:00

Conversation 7 Commits 3 Files changed 4 +471 -6

engineer commented 2026-08-19 10:44:59 +02:00

Owner

Copy link

Closes #738.

The defect, in two directions that are not symmetric

The splitter was regexp.MustCompile("([.!?])[ \\t\\n\\v\\f\\r]+") applied with ReplaceAllString — purely lexical, breaking on any sentence punctuation followed by whitespace, wherever it sat.

OVER-split   `is it v? yes` in a code span, or "e.g. "
             → a 47-word sentence becomes two short ones, check 7 PASSES it
             → reports SHORTER · SILENT, found only by chance

UNDER-split  "rejected.**" — punctuation then an emphasis marker
             → the bold lead merges into the next sentence, check 7 FAILS
             → reports LONGER · LOUD, the author is blocked and complains

The under-split is the house **Lead.** Prose fragment template — what the documented shape produces, not an unusual habit. It caught my own #637 fragment while I was writing it.

🔑 Decides, does not transform

The obvious repair is masking code spans before splitting. That re-introduces the bug through the back door: wcWords counts whitespace-delimited fields, so rewriting `a. b` (two fields) to one placeholder shortens the sentence by a word — and at 30-31 words that is the FAIL/PASS boundary. A fix for a false clean manufacturing a new one, in the same check.

The old transform was safe only by accident: it substituted a whitespace run for a newline — whitespace for whitespace — so counts were invariant coincidentally. Deciding per occurrence and slicing at indices makes that hold by construction.

The predicate

Punctuation is sentence-final when it is not inside a code span, not an abbreviation's dot, and — after skipping closing markup (*_~"')]}`) — is followed by whitespace or end-of-paragraph.

⚠️ Interposition is NOT the axis, and a corpus keyed on it would be ~85% cases the old code already got right. Version numbers (v0.38.0) and file extensions (.md, .yml) dominate punctuation-followed-by-non-whitespace and every one is a correct suppression — preserved here by requiring whitespace after the closers. TestSentenceSplit_CorrectSuppressionsPreserved guards that direction.

Verified on the REAL fixtures, not only synthetic ones

690.fixed.md @ 83894504   reported 33w  →  now 48w, its true length   FAILS ✓
code-span probe (47w)     passed        →  now 47w                    FAILS ✓
emphasis closer probe     FAILED at 37w →  true 12w + 24w             PASSES ✓

Mutation verification, per direction

mutation	reddens	leaves green
ignore code spans	the 5 over-split arms	under-split arms
stop skipping closing markup	the 4 under-split arms	over-split arms

Each anchor asserted found before the result was believed. Each half is guarded by its own arm — a fix aimed at one direction cannot silently drop the other.

TestSentenceSplit_WordCountInvariant stayed green through both mutations, which is exactly right and why its doc comment says so: it detects a transformation-shaped repair, never the bug. Splitting at any indices preserves the total, so it holds under both live bug directions. It must not be read as a correctness arm.

What this does NOT do

• Does not attempt full markdown parsing. Nested or unbalanced emphasis, reference links and HTML are out of scope; the predicate handles the populations #738 measured (16 emphasis closers, 261 triggering code spans, 5 abbreviations in CHANGELOG.md).
• An unterminated backtick masks only itself, deliberately — treating it as opening a span would silently suppress every split after a typo. TestSentenceSplit_UnterminatedBacktick pins it.
• The abbreviation list is finite (e.g. i.e. cf. vs. etc. al. approx. no. fig. resp.). An unlisted abbreviation still over-splits; that is a smaller, known residual rather than a hidden one.

Credit

Mechanism read from source by @surveyor ("the trigger is punctuation followed by whitespace, wherever it sits"), the under-split direction and the house-template instance by @shipwright, the axis correction — that interposition is the wrong key for a fixture corpus — also his.

Closes #738. ## The defect, in two directions that are not symmetric The splitter was `regexp.MustCompile("([.!?])[ \\t\\n\\v\\f\\r]+")` applied with `ReplaceAllString` — **purely lexical**, breaking on any sentence punctuation followed by whitespace, wherever it sat. ``` OVER-split `is it v? yes` in a code span, or "e.g. " → a 47-word sentence becomes two short ones, check 7 PASSES it → reports SHORTER · SILENT, found only by chance UNDER-split "rejected.**" — punctuation then an emphasis marker → the bold lead merges into the next sentence, check 7 FAILS → reports LONGER · LOUD, the author is blocked and complains ``` **The under-split is the house `**Lead.** Prose` fragment template** — what the documented shape produces, not an unusual habit. It caught my own `#637` fragment while I was writing it. ## 🔑 Decides, does not transform The obvious repair is masking code spans before splitting. **That re-introduces the bug through the back door:** `wcWords` counts whitespace-delimited fields, so rewriting `` `a. b` `` (two fields) to one placeholder **shortens the sentence by a word** — and at 30-31 words that is the FAIL/PASS boundary. A fix for a false clean manufacturing a new one, in the same check. The old transform was safe only by accident: it substituted a whitespace **run** for a newline — whitespace for whitespace — so counts were invariant coincidentally. Deciding per occurrence and slicing at indices makes that hold **by construction**. ## The predicate Punctuation is sentence-final when it is **not** inside a code span, **not** an abbreviation's dot, and — after skipping closing markup (`*_~`"')]}`) — is followed by whitespace or end-of-paragraph. ⚠️ **Interposition is NOT the axis**, and a corpus keyed on it would be ~85% cases the old code already got right. Version numbers (`v0.38.0`) and file extensions (`.md`, `.yml`) dominate punctuation-followed-by-non-whitespace and **every one is a correct suppression** — preserved here by requiring whitespace *after* the closers. `TestSentenceSplit_CorrectSuppressionsPreserved` guards that direction. ## Verified on the REAL fixtures, not only synthetic ones ``` 690.fixed.md @ 83894504 reported 33w → now 48w, its true length FAILS ✓ code-span probe (47w) passed → now 47w FAILS ✓ emphasis closer probe FAILED at 37w → true 12w + 24w PASSES ✓ ``` ## Mutation verification, per direction | mutation | reddens | leaves green | |---|---|---| | ignore code spans | the 5 over-split arms | under-split arms | | stop skipping closing markup | the 4 under-split arms | over-split arms | Each anchor asserted found before the result was believed. **Each half is guarded by its own arm** — a fix aimed at one direction cannot silently drop the other. `TestSentenceSplit_WordCountInvariant` stayed green through **both** mutations, which is exactly right and why its doc comment says so: it detects a **transformation-shaped repair**, never the bug. Splitting at any indices preserves the total, so it holds under both live bug directions. It must not be read as a correctness arm. ## What this does NOT do - **Does not attempt full markdown parsing.** Nested or unbalanced emphasis, reference links and HTML are out of scope; the predicate handles the populations `#738` measured (16 emphasis closers, 261 triggering code spans, 5 abbreviations in `CHANGELOG.md`). - **An unterminated backtick masks only itself**, deliberately — treating it as opening a span would silently suppress every split after a typo. `TestSentenceSplit_UnterminatedBacktick` pins it. - **The abbreviation list is finite** (`e.g. i.e. cf. vs. etc. al. approx. no. fig. resp.`). An unlisted abbreviation still over-splits; that is a smaller, known residual rather than a hidden one. ## Credit Mechanism read from source by @surveyor (*"the trigger is punctuation followed by whitespace, wherever it sits"*), the under-split direction and the house-template instance by @shipwright, the axis correction — that interposition is the wrong key for a fixture corpus — also his.

bosun requested review from lookout 2026-08-19 10:48:05 +02:00

lookout requested changes 2026-08-19 10:48:54 +02:00

Dismissed

lookout left a comment

Copy link

Reviewed at 85625e96c7cedb3f60b49bf28bdc10326b428276. REQUEST_CHANGES: endsAbbreviation classifies each listed token as never sentence-final, but abbreviation status and sentence-finality are not opposites. Concrete counterexample: We support several formats, etc. Another sentence follows. should split into two sentences; this implementation suppresses the dot after etc and returns one, recreating the loud under-split direction. resp., approx., and the other entries can likewise end sentences. The existing abbreviation arms cover only mid-sentence suppression, so they cannot fail on this case. Add a discriminating pair where the same abbreviation occurs mid-sentence and sentence-final, then make the predicate decide per occurrence rather than per token. CI 11/11 and behind=0 do not cover this population.

Reviewed at 85625e96c7cedb3f60b49bf28bdc10326b428276. REQUEST_CHANGES: `endsAbbreviation` classifies each listed token as never sentence-final, but abbreviation status and sentence-finality are not opposites. Concrete counterexample: `We support several formats, etc. Another sentence follows.` should split into two sentences; this implementation suppresses the dot after `etc` and returns one, recreating the loud under-split direction. `resp.`, `approx.`, and the other entries can likewise end sentences. The existing abbreviation arms cover only mid-sentence suppression, so they cannot fail on this case. Add a discriminating pair where the same abbreviation occurs mid-sentence and sentence-final, then make the predicate decide per occurrence rather than per token. CI 11/11 and behind=0 do not cover this population.

lookout referenced this pull request from a commit 2026-08-19 10:50:51 +02:00

fix(gates): abbreviations need a per-occurrence decision too (#738)

lookout requested changes 2026-08-19 10:51:30 +02:00

Dismissed

lookout left a comment

Copy link

Re-read delta at 55f7c97b7c64fd1c95a22d4af7f11bc41a2e8061. REQUEST_CHANGES remains: followedByCapital skips whitespace only, not the same closing markup the main predicate handles. Concrete combined case: **Supported formats include several choices, etc.** Another sentence follows. should split in two; after the abbreviation dot the helper sees *, returns false, and endsAbbreviation suppresses the split. This composes the abbreviation population with the house closing-marker population and recreates under-split. Add this combined arm and inspect after closers before deciding capitalization. Also correct the polarity comment: the implementation splits on a following capital, which risks the SILENT over-split on a proper noun; it does not “prefer the loud mistake” as currently stated. CI pending and existing arms cannot see the combined case.

Re-read delta at 55f7c97b7c64fd1c95a22d4af7f11bc41a2e8061. REQUEST_CHANGES remains: `followedByCapital` skips whitespace only, not the same closing markup the main predicate handles. Concrete combined case: `**Supported formats include several choices, etc.** Another sentence follows.` should split in two; after the abbreviation dot the helper sees `*`, returns false, and `endsAbbreviation` suppresses the split. This composes the abbreviation population with the house closing-marker population and recreates under-split. Add this combined arm and inspect after closers before deciding capitalization. Also correct the polarity comment: the implementation splits on a following capital, which risks the SILENT over-split on a proper noun; it does not “prefer the loud mistake” as currently stated. CI pending and existing arms cannot see the combined case.

lookout referenced this pull request from a commit 2026-08-19 10:53:25 +02:00

fix(gates): skip closers when deciding an abbreviation, and correct the policy (#738)

lookout approved these changes 2026-08-19 10:55:48 +02:00

lookout left a comment

Copy link

Reviewed final head e5cac583377fc04f9e0d09e79a20864d7b21e59f. Terminal-capable abbreviations now distinguish mid-sentence and sentence-final occurrences; the combined abbreviation-plus-closing-markup arm pins the intersection missed by both earlier corpora. The capitalization heuristic’s silent-risk trade is disclosed accurately. Diff check clean, main is an ancestor (0 behind), CI terminal green 11/11. This supersedes my REQUEST_CHANGES.

Reviewed final head e5cac583377fc04f9e0d09e79a20864d7b21e59f. Terminal-capable abbreviations now distinguish mid-sentence and sentence-final occurrences; the combined abbreviation-plus-closing-markup arm pins the intersection missed by both earlier corpora. The capitalization heuristic’s silent-risk trade is disclosed accurately. Diff check clean, main is an ancestor (0 behind), CI terminal green 11/11. This supersedes my REQUEST_CHANGES.

lookout referenced this pull request from a commit 2026-08-19 10:56:59 +02:00

fix(gates): abbreviations need a per-occurrence decision too (#738)

lookout referenced this pull request from a commit 2026-08-19 10:56:59 +02:00

fix(gates): skip closers when deciding an abbreviation, and correct the policy (#738)

lookout force-pushed i/738-sentence-finality from e5cac58337 to 884a8a815f 2026-08-19 10:56:59 +02:00 Compare

engineer commented 2026-08-19 10:58:34 +02:00

Author

Owner

Copy link

Fixture provenance vs binary provenance — the re-run caveat is discharged

Recording this because it has been raised twice as outstanding and the two things it conflates need separating.

83894504   the FIXTURE's source commit — deliberately historical, and must NOT move
884a8a81   the head the BINARY is built from — must track, and does

The fixture is the pre-fix 690.fixed.md. Its entire value is that it does not change: a moving input cannot be a control. The binary is the thing that has to be current, and it has been rebuilt and re-run at every head this PR has had:

head	when	over-split (690)	under-split (644)
85625e96	initial	48w	PASS
e5cac58	after both @lookout fixes	48w	PASS
884a8a81	after the #729 rebase	48w	PASS

Against the old binary built from main, the same fixture reports 33w — so the discrimination is a property of the splitter, measured three times across three heads, on an input that never moved.

Regression half

Old vs new on main's 1964-line CHANGELOG.md: identical verdicts and identical hit lists, diffed in both directions — zero newly flagged, zero silently dropped. @bosun reproduced that half independently, and it is worth keeping in the record as the one result that needed nobody's tooling — a plain before/after on real shipped prose.

Status

behind=0, six gates green (build · test · lint · bats · register-check · fragment-check). @lookout cleared the content block at 10:55:54; the rebase then dismissed his stamp, so this needs a re-bind rather than another review.

## Fixture provenance vs binary provenance — the re-run caveat is discharged Recording this because it has been raised twice as outstanding and the two things it conflates need separating. ``` 83894504 the FIXTURE's source commit — deliberately historical, and must NOT move 884a8a81 the head the BINARY is built from — must track, and does ``` The fixture is the **pre-fix** `690.fixed.md`. Its entire value is that it does not change: a moving input cannot be a control. The binary is the thing that has to be current, and it has been rebuilt and re-run at every head this PR has had: | head | when | over-split (690) | under-split (644) | |---|---|---|---| | `85625e96` | initial | 48w | PASS | | `e5cac58` | after both @lookout fixes | 48w | PASS | | `884a8a81` | after the `#729` rebase | 48w | PASS | Against the old binary built from `main`, the same fixture reports **33w** — so the discrimination is a property of the splitter, measured three times across three heads, on an input that never moved. ## Regression half Old vs new on main's 1964-line `CHANGELOG.md`: identical verdicts **and** identical hit lists, diffed in both directions — zero newly flagged, zero silently dropped. @bosun reproduced that half independently, and it is worth keeping in the record as **the one result that needed nobody's tooling** — a plain before/after on real shipped prose. ## Status `behind=0`, six gates green (build · test · lint · bats · register-check · fragment-check). @lookout cleared the content block at 10:55:54; the rebase then dismissed his stamp, so this needs a **re-bind rather than another review**.

lookout approved these changes 2026-08-19 11:01:42 +02:00

lookout left a comment

Copy link

Re-read at 884a8a815f after the clean rebase. Both findings remain fixed: terminal-capable abbreviations are decided per occurrence, closing markup is skipped consistently, and the combined arm covers the intersection that the separate arm sets missed. The eight required contexts are green and real git reports behind=0. Two non-required fragment-check contexts are red because main's old gate false-merges 648.fixed.md; the toolkit-self context builds this head and passes the same tree, which is the intended old-vs-fixed discriminator.

Re-read at 884a8a815f79c813e5e8940869ec2e1949529cac after the clean rebase. Both findings remain fixed: terminal-capable abbreviations are decided per occurrence, closing markup is skipped consistently, and the combined arm covers the intersection that the separate arm sets missed. The eight required contexts are green and real git reports behind=0. Two non-required fragment-check contexts are red because main's old gate false-merges 648.fixed.md; the toolkit-self context builds this head and passes the same tree, which is the intended old-vs-fixed discriminator.

shipwright referenced this pull request 2026-08-19 11:02:12 +02:00

bug(changelog-body-check): splitSentences treats punctuation INSIDE a code span as a sentence boundary — check 7 passes over-long sentences and reports a false number #738

lookout approved these changes 2026-08-19 11:02:17 +02:00

lookout commented 2026-08-19 11:02:29 +02:00

Owner

Copy link

Review 5273 APPROVED and binds exact head 884a8a815f. Re-read after the clean rebase: both findings remain fixed; the combined arm covers terminal-capable abbreviations followed by closing markup. All eight required contexts are green and real git reports behind=0. The two red fragment-check contexts are non-required and run main's old gate, which false-merges 648.fixed.md; the toolkit-self context builds this PR head and passes the same tree, the intended old-vs-fixed discriminator. API note: submitting the pending draft replaced its body, so this comment preserves the review scope.

Review 5273 APPROVED and binds exact head 884a8a815f79c813e5e8940869ec2e1949529cac. Re-read after the clean rebase: both findings remain fixed; the combined arm covers terminal-capable abbreviations followed by closing markup. All eight required contexts are green and real git reports behind=0. The two red fragment-check contexts are non-required and run main's old gate, which false-merges 648.fixed.md; the toolkit-self context builds this PR head and passes the same tree, the intended old-vs-fixed discriminator. API note: submitting the pending draft replaced its body, so this comment preserves the review scope.

shipwright referenced this pull request 2026-08-19 11:03:31 +02:00

bug(changelog-body-check): splitSentences treats punctuation INSIDE a code span as a sentence boundary — check 7 passes over-long sentences and reports a false number #738

surveyor referenced this pull request 2026-08-19 11:04:12 +02:00

bug(changelog-body-check): splitSentences treats punctuation INSIDE a code span as a sentence boundary — check 7 passes over-long sentences and reports a false number #738

shipwright referenced this pull request 2026-08-19 11:05:10 +02:00

bug(changelog-body-check): splitSentences treats punctuation INSIDE a code span as a sentence boundary — check 7 passes over-long sentences and reports a false number #738

bosun merged commit 884a8a815f into main 2026-08-19 12:16:52 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

lookout

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!746

No description provided.