Close ChangelogService's mid-read failure window and cover it with tests - #337
Conversation
rdeago
added
the
enhancement
github-actions
Bot
added
area:docs
rdeago
added
the
enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 54.65% 56.80% +2.15%
==========================================
Files 157 158 +1
Lines 4435 4457 +22
Branches 786 789 +3
==========================================
+ Hits 2424 2532 +108
+ Misses 1912 1825 -87
- Partials 99 100 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude please review this PR. |
|
Claude finished @rdeago's task in 4m 45s —— View job Review of PR #337
Ric — the extraction is the right call, and I traced both state machines line by line against Four things, in priority order. 1.
|
|
Thanks — findings 1 and 2 were both right, and 1 was right about the changelog entry too. 1. Mid-read window in Two things came along with it, as you predicted: the class remarks (finding 4) are now true as written, and all three methods share one 2. Empty substitute. Taken, plus one step further. The substitute is trimmed once into a local and an empty result is treated as no substitute. Beyond that, No 3. Vacuous LF test. Taken — the unreleased section is now empty, so the CRLF-bearing substitute is actually exercised. (The 4. Resolved by 1. Nits. Sanity check on the new commit: @claude please take another look. |
|
Claude finished @rdeago's task in 6m 9s —— View job Review of PR #337 (round 2)
Ric — all four are properly closed, and I re-traced the parts that moved rather than taking the summary on trust. 1 — mid-read window. Your invalid-UTF-8 claim holds, and for a reason worth recording since it isn't obvious: 2 — empty substitute. 3 — LF test. Now genuinely exercises the substitute: the section body is a single blank line, 4 — true as written now. Culture fix. Nothing blocking. Four leftovers, all small. a.
|
5dcb69d to
c587a7d
Compare
|
Round 2 addressed. The branch was rebased onto a — the 120-column rule. Your question about The limit applies to new and modified lines always, and to a whole file when you are actually working in it, as a separate commit. A mechanical sweep touching many files with a line or two each does not count — otherwise the b — the schema description. Fixed, but not where you pointed. c — the BOM caveat. You were right, and it is now verified rather than recalled. A probe writing A detected BOM does replace the strict encoding with one whose fallback substitutes U+FFFD, so a BOM'd changelog with invalid bytes is read and rewritten with replacement characters. No behavior change here — that is what d — pinning the culture fix. Taken. One thing you did not flag, and should have caught me on. Making a whitespace-only The Sanity check on the rebased branch: @claude please take another look. |
|
Claude finished @rdeago's task in 6m 59s —— View job Review of PR #337 (round 3)
Ric — all four closed, and the two you went past the finding on (the generated schema, the culture pin) are the better calls. Details of what I re-derived rather than took on trust: a. While there: the sweep is complete. No b. Right, and I should have looked for the generator before pointing at the artifact. c. Your probe settles it, and d. The extraction lands where I'd hoped, and the Three leftovers, none blocking. 1. The new changelog entry documents a fix to a feature that has never shipped
But That's the same ground on which the blank-line trimming was excused from an entry in round 1 (" So my honest answer to "you should have caught me" is the opposite direction from the one you expected: the gap isn't a missing entry, it's two entries deciding the same question differently. Fix this → 2. One declaration in a file this PR works in is over the 120-character threshold
The rule can be read either way: the sweep clause says "within the limit" (singular, in the 140 section), while "Declarations are held to a stricter limit" sits two lines above it. You swept this file for 140 already, so the question is live rather than academic — and it'll come up again on the next file someone tidies. Either wrap it, or say in the rule that whole-file cleanup means the 140 limit only. Fix this → 3. A typo rode along into the new file
What I verified, and what I couldn'tRead and re-derived: that Not verified: I still cannot build or run tests here, so the clean Net: ship it. Item 1 is a changelog wording call, not a code one, and items 2–3 are a line and a letter. |
CHANGELOG.md was opened through UserFile.OpenText, which guards the open only: an I/O failure raised while reading through the returned StreamReader escaped as an unhandled-exception stack trace. PrepareForRelease and UpdateNewSectionTitle consume the file to EOF and rewrite it whole, so streaming buys them nothing; they now read through UserFile.ReadAllLines, and a failure at any point is reported as the usual single clean error line. HasUnreleasedChanges keeps streaming, and says why: it stops at the first line of content it finds. The parsing and rewriting state machines move to a new ChangelogUpdater that works on lines already read, leaving file access to the service. This is what makes them testable at all: ChangelogService cannot be constructed from a test, as ServerAdapter has a private protected constructor and VersionService is sealed around a Git repository. The new type takes the section title as a factory rather than a string, so it is still composed only when a section is actually written, and a malformed changelog reports its own failure rather than one raised while building a title nobody will read. Also trim the release.emptyChangelog substitute and surround it with blank lines. It replaces the whole body of the moved section, blank lines included, so it used to come out flush against both the new section title and the heading below it. The setting has not shipped yet, hence no changelog entry of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`bv release` checks the "Unreleased changes" section before rewriting the changelog, so the streaming read left in `HasUnreleasedChanges` was still the first thing to touch `CHANGELOG.md`: a mid-read failure there escaped as an unhandled exception, the very symptom the previous commit closed for the rewrite. `ChangelogUpdater.HasUnreleasedChanges` now takes the lines of the changelog like its siblings, and `ChangelogService` reads them in one guarded call, with the same encoding used to rewrite the file. Also, in the same area: - an all-whitespace `release.emptyChangelog` no longer writes three blank lines into the changelog: it is treated as no substitute at all, both where the substitute is applied and where the setting is resolved, so the release fails with the message that names the missing setting; - the release date in changelog section titles is formatted with the invariant culture, instead of whatever calendar the machine prescribes; - `PrepareForRelease_UsesLineFeedAsLineSeparator` empties the unreleased section, so that the CRLF-bearing substitute it passes is actually used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This shouldn't need an explanation. Left as is where it made sense, which means a very few places.
MakeSectionTitle is a pure function of a version, a URL and a date, so it moves to ChangelogUpdater, next to the only place that composes a section heading from it. A test can then pin the invariant formatting under th-TH, where a culture-formatted date reads 2569-01-01. Also from the round-2 review: - A blank release.emptyChangelog fails the release, so say so in the configuration model's description, whence the JSON schema is generated, and in the changelog. - The strict UTF-8 decoding of CHANGELOG.md is bypassed when the file carries a byte order mark: the file APIs detect it, and a detected BOM replaces the encoding with one that substitutes U+FFFD. Say that, instead of promising a guarantee the code does not make. - Bring the touched files within the 140-character line limit, and wrap the parameter list of PrepareForRelease, which exceeds the stricter 120-character limit for declarations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
release.emptyChangelog ships in this same unreleased version, so a "bugs fixed" entry about a blank one describes a state no released bv was ever in. The breaking-change entry that introduces the setting says instead that an unset or blank value fails the release, which is the whole rule in one place, and the round-2 entry goes away. This also settles the way the blank-line trimming was settled in round 1, rather than the opposite way. Also from the round-3 review: - Say in MakeSectionTitle_UsesInvariantCalendar_WhateverTheCurrentCulture that it needs ICU data: under InvariantGlobalization, th-TH would fall back to the Gregorian calendar and the test would pin nothing. - Fix a typo carried over verbatim from ChangelogService. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whole-file line-length cleanup covers the stricter limit that declarations are held to, so ReleaseSettings.Parse gets its parameter list wrapped, and the two Description attributes of ReleaseConfig that were over the general limit are split. The attribute arguments stay constant expressions, so the generated schema is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c587a7d to
49e155c
Compare
|
All three taken. Branch rebased again (the style-guide clarification below is a new commit on 1 — the changelog entry. You are right, and the direction you took it is the one I should have taken myself: the inconsistency was mine, between excusing the blank-line trimming in round 1 and adding an entry for the whitespace fix in round 2. 2 — That widened this PR's debt by one file, since a sweep that ignores declarations is not the sweep the rule now describes: 3 — typo. Fixed. On the Sanity check on the rebased branch: Thanks for three rounds of it — the two calls you went past a finding to make (the generated schema, this round's changelog reasoning) were both better than the finding. |
1 participant
Proposed changes
ChangelogServiceopenedCHANGELOG.mdthroughUserFile.OpenText, which — as documented — guards theopen only: an I/O failure raised while reading through the returned
StreamReaderstill escaped as anunhandled-exception stack trace. All three entry points now read the file through
UserFile.ReadAllLines,so a failure at any point of the read is reported as the usual single clean error line, and all three use
one strict UTF-8 encoding instead of two different ones.
The switch was deferred out of #329 because both rewrite methods are line-by-line state machines with no
test coverage, and #330 pairs the refactor with the missing tests. Covering them turned out to need one
design change beyond what the issue anticipated:
ChangelogServicecannot be constructed from a test atall, since
ServerAdapterhas aprivate protectedconstructor andVersionServiceissealedarounda live Git repository. Rather than build seams to mock the environment, the parsing and rewriting logic
moved into a new
ChangelogUpdaterthat works on lines already read, leaving file access to the service— the "extract pure logic out of plumbing" route. The state machines are otherwise unchanged: the only
edits are
reader.ReadLine()becoming an indexed read with an EOF sentinel, and the section titlearriving as a
Func<string>.The title is a factory rather than a string so that it is still composed only when a section is actually
written. Passing a computed string would have moved the call ahead of the "contains no sections" check,
and a malformed changelog would then report a failure raised while building a title nobody will read.
Composing the title is itself a pure function of a version, a URL and a date, so it moved to
ChangelogUpdatertoo, where a test can pin its formatting.Three behavior changes came out of writing the tests and reviewing them:
The
release.emptyChangelogsubstitute replaces the whole body of the moved section, blank linesincluded, so it used to come out flush against both the new section's title and the heading below it.
It is now trimmed and surrounded with blank lines of its own.
A
release.emptyChangelogthat is all whitespace substituted nothing for nothing:bv releaseannounced the substitution and then wrote a section with no body. It now fails the release with the
message that says the substitute is missing.
Neither of the two gets a "bugs fixed" entry:
release.emptyChangelogis introduced in this sameunreleased version, so no released
bvever had the old behavior, and an entry would offer a readera fix for something they are meeting for the first time a few bullets earlier. The rule is stated
where the setting is introduced instead — the breaking-change entry now reads "when unset or blank,
an empty section fails the release".
The release date in section titles was formatted with the current culture, so a machine set to
th-THor
ar-SAwrote a date in a non-Gregorian calendar (2569-04-27for2026-04-27), matching neitherthe release tag nor the other section titles. It is now formatted with the invariant culture, pinned by
a test that runs under
th-TH. This bug predates the PR; it was folded in rather than deferred becausea follow-up would have cost far more than the fix.
Unrelated to the above, one commit sweeps
BuildFailedException.ThrowIfNot(<negated condition>)callsites across the repository into
ThrowIf(<condition>). It is committed here because it touchesChangelogUpdater, which this PR rewrites, and would otherwise conflict with it.A last commit wraps the declarations and attributes of the files this PR works in that exceed the
style guide's line limits, kept apart from the changes under review as the rule prescribes.
The changelog entry for #329 was extended rather than joined by a new one: both changes land in the same
unreleased version, so between stable releases they are one story. The date bug, being the only change
here that a released
bvactually shipped, has an entry of its own.Checklist of related issues / discussions
Types of changes
This pull request introduces the following types of changes:
docsdirectory) update.gitattributes,.gitignore)Breaking changes
This pull request introduces breaking changes:
Checklist