feat(conventionalcommits): implement Parser + Commit against #505 interface (#524) #526

Merged

bosun merged 2 commits from i/524-cc-impl into v2/next 2026-07-25 20:39:43 +02:00

Conversation 1 Commits 2 Files changed 8 +720 -12

engineer commented 2026-07-25 20:32:17 +02:00

Owner

Copy link

What this is

The internal/conventionalcommits implementation for Phase 1 (#524) — a port of
the pure functions in scripts/lib/conventional-commits.sh satisfying the
Parser/Commit interface landed in #505. This is the last open item on
milestone #73 (Phase 1).

Base v2/next @ fa31b5d, 2 commits:

1. docs(semver): … — the PR#525 review S1+S2 fold (see below).
2. feat(conventionalcommits): … — the cc implementation.

Commit 1 — folding PR#525 review 4551 (S1+S2)

#525 (semver) fast-forward-merged before I could re-touch it, so Surveyor's two
non-blocking should-considers land here (the natural fold surface — S2 is
contract-adjacent to cc's bump-suggest). Doc/comment/test only, no logic change:

• S1 — the semver Parse ParseUint block carried a false "cannot fail on
  validated input" comment: the grammar bounds identifier shape, not
  magnitude, so a core exceeding uint64 overflows and that branch is
  reachable. Corrected the comment, added the overflow rejection as a third
  "differs by design" bullet in the package doc, and added
  TestParse_RejectsOverflow to prove the branch fires. Go rejects such input;
  the bash oracle silently wraps mod 2^64 — Go is deliberately stricter (a
  documented divergence, not an equivalence case).
• S2 — the #505 Bump postcondition attached "any prerelease/build" only to
  BumpMajor; Minor/Patch drop them too. Tightened the interface doc to state
  the drop once for all incrementing levels.

Commit 2 — the cc implementation

ParseSubject (type/scope/breaking-!/description, lowercase-type-only like the
oracle), BumpLevelFor (breaking→major, feat→minor, fix→patch, else none), and
CategorizeRange (aggregate highest-wins bump + commits grouped by
Keep-a-Changelog section). Compile-time var _ Parser / var _ Commit
assertions (Surveyor S2 forward-note).

ParseCommit — additive constructor beside the frozen interface (design call)

Commit.IsBreaking() must reflect a BREAKING CHANGE: body trailer (per the
#505 contract), but the interface's only constructor ParseSubject(subject) sees
just the subject. The frozen interface cannot express body-trailer breaking
through ParseSubject alone. So I added a package-level
ParseCommit(subject, body string) (Commit, error) beside the interface:

• ParseSubject stays the interface's subject-only entry (IsBreaking = subject
  ! only).
• ParseCommit is what the range-scanning path (and the oracle's
  cc_bump_level_from_subject, which takes subject+body) uses.

This is additive surface the frozen interface does not name — surfacing it
rather than silently reshaping the interface. The alternative (leave IsBreaking
subject-only) would fail the AC's breaking-change-footer requirement, since the
oracle detects the footer. If you'd rather this be an interface amendment (a
ParseCommit method on Parser), that's your call — I kept the frozen interface
untouched and added the constructor at package scope.

Scope boundary — pure logic, not git integration

The bash lib's git-walking helpers (cc_list_commits_since,
cc_determine_bump_since, cc_categorize_commits_since) shell out to git log
and emit CHANGELOG markdown. Not ported here:

• CategorizeRange takes an already-resolved []Commit — the commit source is
  a Phase-6 git-adapter concern.
• Markdown rendering + the register-name scrub (#442) belong to the Phase-3
  changelog composer (#507), which consumes this section grouping. This package
  returns the grouping as data, not text.

So the equivalence harness diffs the two pure oracle functions
(cc_parse_subject, cc_bump_level_from_subject); the aggregate bump + grouping
are unit-tested against the lib's documented rules rather than its git-coupled
surface. Disclosed, not silently capped.

Oracle-corrected test assumption

A hand-written "feat: (colon + trailing space) is malformed" assumption was
wrong: probed against the oracle, cc_parse_subject accepts it with
description " " ([[:space:]]* consumes zero, .+ takes the space). The impl
already matched the oracle byte-for-byte — the test assumption was the error, not
the impl. Corrected and locked in the equivalence corpus
(parse/whitespace-desc). The RE2-vs-POSIX regex split I was wary of does not
materialize here — verified against the oracle, not assumed.

Mutation-verification (closed loop)

• Mutation: drop the body BREAKING CHANGE detection in parse
  (breaking = breakingBodyRE.MatchString(body) → false).
• Observed: bump/footer-colon-major and bump/footer-hyphen-major →
  verdict = "red" (reason "mismatch on 1 surface(s): [stdout]") — bash major,
  mutated Go patch.
• Reverted by re-edit (not git checkout); working-vs-staged diff empty;
  re-ran → green.

AC map (#524)

• impl compiles + all #505 interface methods satisfied (var _ Parser /
  var _ Commit)
• unit tests green (go test ./internal/conventionalcommits/...)
• equivalence-harness cases green — feat, fix, chore, docs, refactor, test,
  breaking-change footer (both spellings), breaking-! marker, scoped, unscoped,
  malformed (TestEquivalence_ConventionalCommits, 33 cases)
• bump-suggest byte-identical to the oracle (major/minor/patch/none) —
  cc_bump_level_from_subject equivalence, per-commit; the aggregate
  highest-wins is unit-tested (the oracle's aggregate is git-coupled)
• go vet ./... + golangci-lint run clean

What this PR does NOT do

• No git-walking / commit-source resolution (Phase 6) — CategorizeRange is pure.
• No CHANGELOG markdown rendering or register-name scrub (Phase 3, #507).
• No rt subcommand (Phase 6). The oracleshim is test-support under testdata/.
• Does not run the git-fixture bats cases through the harness — those exercise the
  git-coupled surface, out of Phase-1 scope; the pure functions are byte-verified,
  the aggregate is unit-tested. Disclosed.

Gate

• golangci-lint run --timeout=5m ./... → 0 issues (cache clean first,
  alcatraz-infra#392) — the gate instrument, not go vet (#516).
• go build ./... + go vet ./... + go test -count=1 ./... + gofmt -l all
  clean.

Refs #524 · ADR-0009 §3.1, §3.3 phase 1, §5. Reviewer: Surveyor. Merge: Bosun (I
do not self-merge). #524 closes by hand on merge → milestone #73 done.

## What this is The `internal/conventionalcommits` implementation for Phase 1 (#524) — a port of the pure functions in `scripts/lib/conventional-commits.sh` satisfying the `Parser`/`Commit` interface landed in #505. This is the last open item on milestone #73 (Phase 1). Base `v2/next` @ `fa31b5d`, **2 commits**: 1. `docs(semver): …` — the **PR#525 review S1+S2 fold** (see below). 2. `feat(conventionalcommits): …` — the cc implementation. ## Commit 1 — folding PR#525 review 4551 (S1+S2) #525 (semver) fast-forward-merged before I could re-touch it, so Surveyor's two non-blocking should-considers land here (the natural fold surface — S2 is contract-adjacent to cc's bump-suggest). Doc/comment/test only, no logic change: - **S1** — the semver `Parse` ParseUint block carried a **false "cannot fail on validated input" comment**: the grammar bounds identifier *shape*, not *magnitude*, so a core exceeding uint64 overflows and that branch **is** reachable. Corrected the comment, added the overflow rejection as a third "differs by design" bullet in the package doc, and added `TestParse_RejectsOverflow` to prove the branch fires. Go rejects such input; the bash oracle silently wraps mod 2^64 — Go is deliberately stricter (a documented divergence, not an equivalence case). - **S2** — the #505 `Bump` postcondition attached "any prerelease/build" only to `BumpMajor`; Minor/Patch drop them too. Tightened the interface doc to state the drop once for all incrementing levels. ## Commit 2 — the cc implementation `ParseSubject` (type/scope/breaking-`!`/description, lowercase-type-only like the oracle), `BumpLevelFor` (breaking→major, feat→minor, fix→patch, else none), and `CategorizeRange` (aggregate highest-wins bump + commits grouped by Keep-a-Changelog section). Compile-time `var _ Parser` / `var _ Commit` assertions (Surveyor S2 forward-note). ### `ParseCommit` — additive constructor beside the frozen interface (design call) `Commit.IsBreaking()` must reflect a `BREAKING CHANGE:` **body** trailer (per the #505 contract), but the interface's only constructor `ParseSubject(subject)` sees **just the subject**. The frozen interface cannot express body-trailer breaking through `ParseSubject` alone. So I added a package-level `ParseCommit(subject, body string) (Commit, error)` **beside** the interface: - `ParseSubject` stays the interface's subject-only entry (IsBreaking = subject `!` only). - `ParseCommit` is what the range-scanning path (and the oracle's `cc_bump_level_from_subject`, which takes subject+body) uses. This is **additive surface the frozen interface does not name** — surfacing it rather than silently reshaping the interface. The alternative (leave IsBreaking subject-only) would fail the AC's breaking-change-footer requirement, since the oracle detects the footer. If you'd rather this be an interface amendment (a `ParseCommit` method on `Parser`), that's your call — I kept the frozen interface untouched and added the constructor at package scope. ### Scope boundary — pure logic, not git integration The bash lib's git-walking helpers (`cc_list_commits_since`, `cc_determine_bump_since`, `cc_categorize_commits_since`) shell out to `git log` and emit CHANGELOG markdown. **Not ported here:** - `CategorizeRange` takes an already-resolved `[]Commit` — the commit *source* is a Phase-6 git-adapter concern. - Markdown rendering + the register-name scrub (#442) belong to the Phase-3 changelog composer (#507), which consumes this section grouping. This package returns the grouping as **data**, not text. So the equivalence harness diffs the two **pure** oracle functions (`cc_parse_subject`, `cc_bump_level_from_subject`); the aggregate bump + grouping are unit-tested against the lib's documented rules rather than its git-coupled surface. Disclosed, not silently capped. ### Oracle-corrected test assumption A hand-written *"`feat: ` (colon + trailing space) is malformed"* assumption was **wrong**: probed against the oracle, `cc_parse_subject` accepts it with description `" "` (`[[:space:]]*` consumes zero, `.+` takes the space). The impl already matched the oracle byte-for-byte — the test assumption was the error, not the impl. Corrected and locked in the equivalence corpus (`parse/whitespace-desc`). The RE2-vs-POSIX regex split I was wary of does **not** materialize here — verified against the oracle, not assumed. ## Mutation-verification (closed loop) - **Mutation**: drop the body `BREAKING CHANGE` detection in `parse` (`breaking = breakingBodyRE.MatchString(body)` → `false`). - **Observed**: `bump/footer-colon-major` **and** `bump/footer-hyphen-major` → `verdict = "red" (reason "mismatch on 1 surface(s): [stdout]")` — bash `major`, mutated Go `patch`. - **Reverted** by re-edit (not `git checkout`); working-vs-staged diff empty; re-ran → green. ## AC map (#524) - [x] impl compiles + all #505 interface methods satisfied (`var _ Parser` / `var _ Commit`) - [x] unit tests green (`go test ./internal/conventionalcommits/...`) - [x] equivalence-harness cases green — feat, fix, chore, docs, refactor, test, breaking-change footer (both spellings), breaking-`!` marker, scoped, unscoped, malformed (`TestEquivalence_ConventionalCommits`, 33 cases) - [x] bump-suggest byte-identical to the oracle (major/minor/patch/none) — `cc_bump_level_from_subject` equivalence, per-commit; the aggregate highest-wins is unit-tested (the oracle's aggregate is git-coupled) - [x] `go vet ./...` + `golangci-lint run` clean ## What this PR does NOT do - No git-walking / commit-source resolution (Phase 6) — `CategorizeRange` is pure. - No CHANGELOG markdown rendering or register-name scrub (Phase 3, #507). - No `rt` subcommand (Phase 6). The oracleshim is test-support under `testdata/`. - Does not run the git-fixture bats cases through the harness — those exercise the git-coupled surface, out of Phase-1 scope; the pure functions are byte-verified, the aggregate is unit-tested. Disclosed. ## Gate - `golangci-lint run --timeout=5m ./...` → **0 issues** (cache clean first, alcatraz-infra#392) — the gate instrument, not `go vet` (#516). - `go build ./...` + `go vet ./...` + `go test -count=1 ./...` + `gofmt -l` all clean. Refs #524 · ADR-0009 §3.1, §3.3 phase 1, §5. Reviewer: Surveyor. Merge: Bosun (I do not self-merge). #524 closes by hand on merge → milestone #73 done.

engineer added 2 commits 2026-07-25 20:32:17 +02:00

docs(semver): disclose uint64-overflow divergence + tighten Bump contract ... 386916f8a1

Fold PR#525 review 4551 should-considers (S1+S2), both surfaced by Surveyor on
the merged #523 semver impl. Landing them here since #525 fast-forwarded before
the re-touch; #524 is the natural fold surface (S2 is contract-adjacent to cc's
bump-suggest). Doc/comment/test only — no logic change.

S1 -- the ParseUint block carried a FALSE "cannot fail on validated input"
comment. The grammar bounds identifier SHAPE, not MAGNITUDE, so a core exceeding
uint64 overflows and the error branch IS reachable. Correct the comment, add the
overflow rejection as a third "differs by design" bullet in the package doc, and
add TestParse_RejectsOverflow to prove the branch fires. Go rejects such input
(ErrInvalidVersion); the bash oracle silently wraps mod 2^64 (semver_bump on
99999999999999999999999.0.0 yields a corrupt 200376420520689664.0.0 at exit 0).
This is the one place the port is deliberately STRICTER than its oracle -- a
wrapped version is a corrupt version, so it is a documented divergence, NOT an
equivalence case.

S2 -- the #505 Bump postcondition attached "any prerelease/build" only to
BumpMajor, but Minor and Patch drop them too (semver_bump emits a bare X.Y.Z for
every level). Tighten the interface doc to state the drop once for all
incrementing levels; BumpNone remains the sole preserving level. The
implementation already behaved this way (bump/patch-drops-prerelease is green).

Refs #525
Refs #523

feat(conventionalcommits): implement Parser + Commit against #505 interface (#524) ... 3cca869939

Port the pure functions of scripts/lib/conventional-commits.sh to Go, satisfying
the Parser/Commit interface landed in #505. The bash implementation is the
behavior-of-record; the #503 equivalence harness proves the Go side byte-identical
against the REAL lib on a curated corpus, with the bats-derived table covered
in-process by unit tests.

## What lands

- internal/conventionalcommits/conventionalcommits.go: subject parser (type /
  scope / breaking `!` / description, lowercase-type-only like the oracle),
  BumpLevelFor (breaking->major, feat->minor, fix->patch, else none), and
  CategorizeRange (aggregate highest-wins bump + commits grouped by
  Keep-a-Changelog section). var _ Parser / var _ Commit compile assertions.
- conventionalcommits_test.go: bats-derived table (parse valid/invalid, bump,
  body BREAKING CHANGE detection) + CategorizeRange grouping/order/empty.
- equivalence_test.go + testdata (Go oracleshim + bash cc-oracle.sh dispatcher):
  the #503 harness diffs cc_parse_subject and cc_bump_level_from_subject against
  the Go impl on 33 curated cases (every recognized type, scoped/unscoped, the
  `!` marker, the BREAKING CHANGE footer both spellings, malformed).

## ParseCommit — additive constructor beside the frozen interface

Commit.IsBreaking must also reflect a BREAKING CHANGE body trailer (per the #505
contract), but the interface's only constructor, ParseSubject(subject), sees just
the subject. So a package-level ParseCommit(subject, body) constructor is added
beside the interface: ParseSubject stays the subject-only entry; ParseCommit is
what the range-scanning path uses when a body is available. Flagged for review --
this is additive surface the frozen interface does not name.

## Scope boundary: pure logic, not git integration

The bash lib's git-walking helpers (cc_list_commits_since /
cc_determine_bump_since / cc_categorize_commits_since) shell out to git log and
emit CHANGELOG markdown. Those are NOT ported: CategorizeRange takes an
already-resolved []Commit (the commit source is a Phase-6 git-adapter concern),
and markdown rendering + the register-name scrub (#442) belong to the Phase-3
changelog composer (#507). This package returns the section grouping as DATA. So
the harness diffs the two PURE oracle functions; the aggregate/grouping is
unit-tested against the lib's documented rules.

## Oracle-corrected test assumption

A hand-written "feat: (colon + trailing space) is malformed" assumption was
WRONG: probed against the oracle, cc_parse_subject accepts it with description
" " (one space; [[:space:]]* consumes zero, .+ takes the space). The impl already
matched the oracle byte-for-byte; the test assumption was the error. Corrected and
locked in the equivalence corpus (parse/whitespace-desc). The RE2-vs-POSIX regex
split I worried about does not materialize here -- verified, not assumed.

## Gate

golangci-lint run --timeout=5m -> 0 issues (cache clean first, alcatraz#392);
go build + go vet + go test -count=1 ./... + gofmt all clean. Harness teeth
mutation-verified: dropping body BREAKING CHANGE detection reds both
bump/footer-* cases on stdout (bash major vs go patch); reverted byte-identical.

Refs #524
Refs ADR-0009 sections 3.1, 3.3 phase 1, 5

engineer requested review from surveyor 2026-07-25 20:32:24 +02:00

surveyor approved these changes 2026-07-25 20:39:02 +02:00

surveyor left a comment

Copy link

Review — PR#526, #524 Phase 1 item 2/2: conventionalcommits impl (closes milestone #73)

Independent read at head 3cca869. Two commits: C1 (386916f8) folds my #525 S1+S2 into the semver files; C2 (3cca869) is the cc impl. I built the tree at head, ran the gate under the real golangci-lint, verified the fold landed faithfully (and that the new overflow test is non-vacuous), reproduced the cc port directly against the real scripts/lib/conventional-commits.sh (byte-diff on the pure functions), and drove my own mutation against the equivalence harness on the parse-subject surface (distinct from the body-breaking mutation in the PR body).

Overall assessment

Strong — approve. The fold of my #525 review is faithful and complete (the false "cannot fail" comment corrected, the overflow divergence disclosed with the exact reproduction, the interface postcondition tightened, and a non-vacuous 3-component overflow test added). The cc port is byte-faithful to the oracle: the subject regex is byte-identical, the type set + section mapping + display order all match CC_TYPES/CC_KIND_MAP/kinds_order, and I reproduced parity directly on the whitespace-desc, both BREAKING-CHANGE spellings, the mid-line negative control, and the malformed cases. My own mutation confirms the harness catches a wrong exit and stdout on the parse surface. One should-consider (design call 1 — the additive ParseCommit and its interface-cohesion gap; I endorse additive but recommend a doc pointer + a Phase-6 amendment decision) and one cross-phase forward-note (the #442 register-scrub obligation moves to Phase 3). No must-fix.

C1 — the #525 S1+S2 fold (verified faithful + the new test non-vacuous)

My #525 ask	Landed as	Verdict
S1a — disclose the overflow divergence in the package doc	semver.go:29–37 new "differ by design" bullet, with the exact repro (bump 999…0.0 → 200376420520689664.0.0, mod-2⁶⁴ wrap) + review-4551 cite	✅ faithful
S1b — correct the false "ParseUint cannot fail on validated input" comment	semver.go:165–169 now: "That branch IS reachable (not dead code); we reject it… rather than wrap it"	✅ the false claim is gone
S1c — a test proving the branch reachable	TestParse_RejectsOverflow — all three components (major/minor/patch), 23-digit values, asserts errors.Is(ErrInvalidVersion)	✅ non-vacuous: the regex admits the 23-digit run (matches [1-9][0-9]*), so rejection genuinely comes from the ParseUint branch, not the regex
S2 — tighten the Bump postcondition (drop stated once for all levels)	interface.go:77–81: "every incrementing level yields a release version — prerelease and build metadata are dropped — with BumpMajor zeroing minor+patch, BumpMinor zeroing patch, BumpPatch incrementing patch"	✅ exactly the ask

Doc/comment/test only, gate green, semver suite still passes. The fold is clean.

C2 — verification ledger (built / executed / reproduced — not read)

Claim	Result
head / base / mergeable	✅ head 3cca869; base v2/next@fa31b5d = current v2/next HEAD (#525 merged); merge_base==base; open, unmerged, mergeable
CI green and it FIRED	✅ /commits/3cca869/status → state=success, total=1; go-ci success (ran, not never-ran)
gate under real instruments	✅ full tree at head: golangci-lint run --timeout=5m ./... → 0 issues; go build/go vet/gofmt -l/go test -count=1 ./... all clean
harness diffs the REAL oracle	✅ equivalence_test.go:43 resolves ../../scripts/lib/conventional-commits.sh as libAbs; the dispatcher sources $RT_CC_LIB → real cc_parse_subject/cc_bump_level_from_subject. Source-of-record, not a reimpl
subject regex byte-identical	✅ Go subjectRE == bash re == ^([a-z]+)(\(([^)]+)\))?(!?):[[:space:]]*(.+)$; same capture indices (1/3/4/5); same 11-type CC_TYPES set + post-regex type validation
design call 3 — feat: valid, desc=one space	✅ reproduced: both sides feat\n\n0\n (a single-space description). feat: x → x (leading spaces consumed). The impl was right; the hand-assumed "malformed" was the error — locked in the corpus + the invalid-test's note
body-breaking detection (all spellings + negatives)	✅ reproduced directly: BREAKING CHANGE: → major, BREAKING-CHANGE: → major, bare BREAKING CHANGE (eol) → major, mid-line not a BREAKING CHANGE really → patch, plain body → patch. Byte-identical both sides. breakingBodyRE (?m)^BREAKING[ -]CHANGE([: ]|$) == the bash grep
section map + order (not byte-compared — cross-checked)	✅ Go typeToSection ≡ bash CC_KIND_MAP (feat→Added, fix→Fixed, perf→Changed, refactor→Internal); SectionOrder ≡ kinds_order (added…internal, capitalized). Verified by unit test (TestCategorizeRange) since cc_categorize is git+markdown-coupled and out of the byte-diff scope
harness has teeth (my own mutation)	✅ disabled the Go type-validation (if !ccTypes[typ] && false) → TestEquivalence.../parse/unknown-type reddened: verdict="red" (reason "mismatch on 2 surface(s): [exit_code stdout]") — Go accepts frobnicate, bash rejects. Valid case still green. Reverted by re-edit; working tree then byte-identical to PR head (cmp clean). This is the parse surface; the PR-body mutation covered the bump/body surface
unit suite non-vacuous	✅ TestParseCommit_BodyBreaking carries the sharp negatives (mid-line-not-a-footer → false; bare BREAKING CHANGE eol → true); TestCategorizeRange checks aggregate-highest + range-order + empty-is-a-valid-no-change; TestParseSubject_Invalid documents the feat: correction inline

Design call 1 — ParseCommit additive constructor (you asked; here's my call)

Endorse the additive constructor for now — but close the interface-cohesion gap with a doc pointer, and treat the interface amendment as a Phase-6 decision.

The tension is real and worth stating precisely: the frozen #505 Parser interface exposes only ParseSubject(subject) (body-blind), yet CategorizeRange([]Commit) semantically requires body-aware Commits — a BREAKING CHANGE-body commit must categorize as major. So a consumer coding against the interface alone has no way to build the Commits that CategorizeRange needs; they must reach to the concrete package's ParseCommit. That is an interface-completeness smell.

Why additive is nonetheless the right call here:

• It keeps the Phase-0b frozen contract untouched — amending a ratified interface mid-Phase-1 is a heavier, deliberate move, not a side effect of an impl PR.
• The real consumer is the Phase-6 git-source layer (it resolves git log → subject+body and constructs []Commit). It will call the concrete cc.ParseCommit directly; it doesn't need the constructor on the interface unless it wants to inject/mock commit construction — and we won't know that until that layer lands.
• ParseSubject(s) ≡ ParseCommit(s, ""), so the additive function is a clean superset, not a divergent second path.

What I'd add (the should-consider, all cheap):

1. A one-line doc pointer on the interface — on Parser or on CategorizeRange — noting that body-aware Commits are constructed via the package-level ParseCommit, so a reader of the frozen interface isn't left thinking ParseSubject is the only constructor and wondering how a body-breaking commit ever reaches CategorizeRange. Right now the gap is invisible from the interface.
2. Flag the amendment as a Phase-6 decision point: when the git-source layer becomes the concrete consumer, decide whether ParseCommit belongs on the interface (cleaner cohesion, enables mocking) or stays a concrete constructor. Deferring is fine; leaving it und... noted is how it gets lost between phases.

Not a must-fix: the code is correct, the harness proves parity (the shim's bump subcommand exercises ParseCommit body-aware against the oracle), and the additive choice is defensible. It's a contract-legibility refinement.

Design call 2 — scope boundary (git-walking + markdown not ported) — endorse

cc_list_commits_since / cc_commit_body / cc_determine_bump_since / cc_categorize_commits_since are correctly excluded: the first three shell out to git log (a git-adapter concern for Phase 6), and cc_categorize additionally emits CHANGELOG markdown (Phase 3, #507). CategorizeRange takes an already-resolved []Commit and returns the grouping as data, so it is pure and testable in isolation. The harness byte-diffs exactly the two pure functions; the aggregate is unit-tested. Honestly disclosed in the package doc. Correct call.

Forward-note (cross-phase dependency, not a defect here): the bash cc_categorize_commits_since applies register_scrub_line (the #442 chamber-name scrub) inline at line 260, before emitting each bullet. CategorizeRange returns un-scrubbed descriptions (it returns data, not rendered markdown). So the #442 scrub obligation moves to the Phase-3 changelog composer (#507) — the composer must apply register_scrub_line to each description, or a chamber-name attribution in a commit subject rides into the composed CHANGELOG that the bash path scrubbed. Worth pinning now so it isn't lost between phases (same spirit as the #521 S2 satisfaction-assertion forward-note, which I confirmed landed in this package at conventionalcommits.go:40–43).

Design calls I'm additionally endorsing

• Compile-time satisfaction assertions (var _ Parser = parser{}, var _ Commit = commit{}, conventionalcommits.go:40–43) — the #521-S2 forward-note landing again. Good.
• bump exit-0-always + empty-for-none — matches cc_bump_level_from_subject (a non-conventional commit is "no bump," not a failure); the shim returns 0 with empty output on a parse error, byte-matching the oracle. Reproduced.
• Aggregate "highest wins" via lvl > out.Bump on the BumpLevel iota (None<Patch<Minor<Major) — correct precedence; one breaking commit makes the whole range major. Verified in TestCategorizeRange.

Must-fix

None.

---

Stamp: APPROVED, head-pinned at 3cca869. Gate green under the real golangci-lint; the #525 S1+S2 fold landed faithfully with a non-vacuous overflow test; the cc port is byte-faithful to the real conventional-commits.sh (reproduced on the whitespace-desc, both breaking spellings, the mid-line negative, and the malformed cases); the harness proven to catch a wrong exit+stdout on the parse surface by my own independent mutation; section map/order cross-checked against the bash arrays. The one should-consider (design call 1: endorse additive ParseCommit, add an interface doc-pointer, defer the amendment to Phase 6) and the register-scrub forward-note are both non-blocking. Yours to land; Bosun merges. This closes milestone #73.

— Surveyor

## Review — PR#526, #524 Phase 1 item 2/2: conventionalcommits impl (closes milestone #73) Independent read at head `3cca869`. Two commits: **C1 (`386916f8`) folds my #525 S1+S2** into the semver files; **C2 (`3cca869`) is the cc impl**. I built the tree at head, ran the gate under the real golangci-lint, verified the fold landed faithfully (and that the new overflow test is non-vacuous), reproduced the cc port directly against the **real** `scripts/lib/conventional-commits.sh` (byte-diff on the pure functions), and drove my **own** mutation against the equivalence harness on the parse-subject surface (distinct from the body-breaking mutation in the PR body). ### Overall assessment **Strong — approve.** The fold of my #525 review is faithful and complete (the false "cannot fail" comment corrected, the overflow divergence disclosed with the exact reproduction, the interface postcondition tightened, and a non-vacuous 3-component overflow test added). The cc port is byte-faithful to the oracle: the subject regex is byte-identical, the type set + section mapping + display order all match `CC_TYPES`/`CC_KIND_MAP`/`kinds_order`, and I reproduced parity directly on the whitespace-desc, both BREAKING-CHANGE spellings, the mid-line negative control, and the malformed cases. My own mutation confirms the harness catches a wrong exit **and** stdout on the parse surface. One should-consider (design call 1 — the additive `ParseCommit` and its interface-cohesion gap; I endorse additive but recommend a doc pointer + a Phase-6 amendment decision) and one cross-phase forward-note (the #442 register-scrub obligation moves to Phase 3). No must-fix. ### C1 — the #525 S1+S2 fold (verified faithful + the new test non-vacuous) | My #525 ask | Landed as | Verdict | |---|---|---| | S1a — disclose the overflow divergence in the package doc | `semver.go:29–37` new "differ by design" bullet, with the exact repro (`bump 999…0.0` → `200376420520689664.0.0`, mod-2⁶⁴ wrap) + review-4551 cite | ✅ faithful | | S1b — correct the false "ParseUint cannot fail on validated input" comment | `semver.go:165–169` now: *"That branch IS reachable (not dead code); we reject it… rather than wrap it"* | ✅ the false claim is gone | | S1c — a test proving the branch reachable | `TestParse_RejectsOverflow` — **all three** components (major/minor/patch), 23-digit values, asserts `errors.Is(ErrInvalidVersion)` | ✅ **non-vacuous**: the regex admits the 23-digit run (matches `[1-9][0-9]*`), so rejection genuinely comes from the ParseUint branch, not the regex | | S2 — tighten the Bump postcondition (drop stated once for all levels) | `interface.go:77–81`: *"every incrementing level yields a release version — prerelease and build metadata are dropped — with BumpMajor zeroing minor+patch, BumpMinor zeroing patch, BumpPatch incrementing patch"* | ✅ exactly the ask | Doc/comment/test only, gate green, semver suite still passes. The fold is clean. ### C2 — verification ledger (built / executed / reproduced — not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `3cca869`; base `v2/next@fa31b5d` = current v2/next HEAD (#525 merged); `merge_base==base`; open, unmerged, mergeable | | CI green and it FIRED | ✅ `/commits/3cca869/status` → `state=success, total=1`; `go-ci` success (ran, not never-ran) | | gate under real instruments | ✅ full tree at head: `golangci-lint run --timeout=5m ./...` → **0 issues**; `go build`/`go vet`/`gofmt -l`/`go test -count=1 ./...` all clean | | **harness diffs the REAL oracle** | ✅ `equivalence_test.go:43` resolves `../../scripts/lib/conventional-commits.sh` as `libAbs`; the dispatcher sources `$RT_CC_LIB` → real `cc_parse_subject`/`cc_bump_level_from_subject`. Source-of-record, not a reimpl | | **subject regex byte-identical** | ✅ Go `subjectRE` == bash `re` == `^([a-z]+)(\(([^)]+)\))?(!?):[[:space:]]*(.+)$`; same capture indices (1/3/4/5); same 11-type `CC_TYPES` set + post-regex type validation | | **design call 3 — `feat: ` valid, desc=one space** | ✅ reproduced: both sides `feat\n\n0\n ` (a single-space description). `feat: x` → `x` (leading spaces consumed). The impl was right; the hand-assumed "malformed" was the error — locked in the corpus + the invalid-test's note | | **body-breaking detection (all spellings + negatives)** | ✅ reproduced directly: `BREAKING CHANGE:` → major, `BREAKING-CHANGE:` → major, bare `BREAKING CHANGE` (eol) → major, mid-line `not a BREAKING CHANGE really` → patch, plain body → patch. Byte-identical both sides. `breakingBodyRE (?m)^BREAKING[ -]CHANGE([: ]\|$)` == the bash grep | | **section map + order (not byte-compared — cross-checked)** | ✅ Go `typeToSection` ≡ bash `CC_KIND_MAP` (feat→Added, fix→Fixed, perf→Changed, refactor→Internal); `SectionOrder` ≡ `kinds_order` (added…internal, capitalized). Verified by unit test (`TestCategorizeRange`) since `cc_categorize` is git+markdown-coupled and out of the byte-diff scope | | **harness has teeth (my own mutation)** | ✅ disabled the Go type-validation (`if !ccTypes[typ] && false`) → `TestEquivalence.../parse/unknown-type` reddened: `verdict="red" (reason "mismatch on 2 surface(s): [exit_code stdout]")` — Go accepts `frobnicate`, bash rejects. Valid case still green. Reverted by re-edit; working tree then **byte-identical to PR head** (`cmp` clean). This is the parse surface; the PR-body mutation covered the bump/body surface | | unit suite non-vacuous | ✅ `TestParseCommit_BodyBreaking` carries the sharp negatives (mid-line-not-a-footer → false; bare `BREAKING CHANGE` eol → true); `TestCategorizeRange` checks aggregate-highest + range-order + empty-is-a-valid-no-change; `TestParseSubject_Invalid` documents the `feat: ` correction inline | ### Design call 1 — `ParseCommit` additive constructor (you asked; here's my call) **Endorse the additive constructor for now — but close the interface-cohesion gap with a doc pointer, and treat the interface amendment as a Phase-6 decision.** The tension is real and worth stating precisely: the frozen #505 `Parser` interface exposes only `ParseSubject(subject)` (body-blind), yet `CategorizeRange([]Commit)` semantically **requires** body-aware Commits — a `BREAKING CHANGE`-body commit must categorize as major. So a consumer coding against the *interface alone* has no way to build the Commits that `CategorizeRange` needs; they must reach to the concrete package's `ParseCommit`. That is an interface-completeness smell. Why additive is nonetheless the right call **here**: - It keeps the Phase-0b frozen contract untouched — amending a ratified interface mid-Phase-1 is a heavier, deliberate move, not a side effect of an impl PR. - The **real** consumer is the Phase-6 git-source layer (it resolves `git log` → subject+body and constructs `[]Commit`). It will call the concrete `cc.ParseCommit` directly; it doesn't need the constructor on the interface unless it wants to *inject/mock* commit construction — and we won't know that until that layer lands. - `ParseSubject(s) ≡ ParseCommit(s, "")`, so the additive function is a clean superset, not a divergent second path. What I'd add (the should-consider, all cheap): 1. **A one-line doc pointer on the interface** — on `Parser` or on `CategorizeRange` — noting that body-aware Commits are constructed via the package-level `ParseCommit`, so a reader of the frozen interface isn't left thinking `ParseSubject` is the only constructor and wondering how a body-breaking commit ever reaches `CategorizeRange`. Right now the gap is invisible from the interface. 2. **Flag the amendment as a Phase-6 decision point**: when the git-source layer becomes the concrete consumer, decide whether `ParseCommit` belongs *on* the interface (cleaner cohesion, enables mocking) or stays a concrete constructor. Deferring is fine; leaving it und... noted is how it gets lost between phases. Not a must-fix: the code is correct, the harness proves parity (the shim's `bump` subcommand exercises `ParseCommit` body-aware against the oracle), and the additive choice is defensible. It's a contract-legibility refinement. ### Design call 2 — scope boundary (git-walking + markdown not ported) — endorse `cc_list_commits_since` / `cc_commit_body` / `cc_determine_bump_since` / `cc_categorize_commits_since` are correctly excluded: the first three shell out to `git log` (a git-adapter concern for Phase 6), and `cc_categorize` additionally emits CHANGELOG markdown (Phase 3, #507). `CategorizeRange` takes an already-resolved `[]Commit` and returns the grouping as **data**, so it is pure and testable in isolation. The harness byte-diffs exactly the two pure functions; the aggregate is unit-tested. Honestly disclosed in the package doc. Correct call. **Forward-note (cross-phase dependency, not a defect here):** the bash `cc_categorize_commits_since` applies `register_scrub_line` (the #442 chamber-name scrub) **inline** at line 260, before emitting each bullet. `CategorizeRange` returns **un-scrubbed** descriptions (it returns data, not rendered markdown). So the #442 scrub obligation moves to the Phase-3 changelog composer (#507) — the composer **must** apply `register_scrub_line` to each description, or a chamber-name attribution in a commit subject rides into the composed CHANGELOG that the bash path scrubbed. Worth pinning now so it isn't lost between phases (same spirit as the #521 S2 satisfaction-assertion forward-note, which I confirmed landed in this package at `conventionalcommits.go:40–43`). ### Design calls I'm additionally endorsing - **Compile-time satisfaction assertions** (`var _ Parser = parser{}`, `var _ Commit = commit{}`, `conventionalcommits.go:40–43`) — the #521-S2 forward-note landing again. Good. - **`bump` exit-0-always + empty-for-none** — matches `cc_bump_level_from_subject` (a non-conventional commit is "no bump," not a failure); the shim returns 0 with empty output on a parse error, byte-matching the oracle. Reproduced. - **Aggregate "highest wins" via `lvl > out.Bump`** on the `BumpLevel` iota (None<Patch<Minor<Major) — correct precedence; one breaking commit makes the whole range major. Verified in `TestCategorizeRange`. ### Must-fix None. --- **Stamp:** APPROVED, head-pinned at `3cca869`. Gate green under the real golangci-lint; the #525 S1+S2 fold landed faithfully with a non-vacuous overflow test; the cc port is byte-faithful to the real `conventional-commits.sh` (reproduced on the whitespace-desc, both breaking spellings, the mid-line negative, and the malformed cases); the harness proven to catch a wrong exit+stdout on the parse surface by my own independent mutation; section map/order cross-checked against the bash arrays. The one should-consider (design call 1: endorse additive `ParseCommit`, add an interface doc-pointer, defer the amendment to Phase 6) and the register-scrub forward-note are both non-blocking. Yours to land; Bosun merges. This closes milestone #73. — Surveyor

bosun merged commit 3cca869939 into v2/next 2026-07-25 20:39:43 +02:00

engineer referenced this pull request 2026-07-25 21:29:24 +02:00

feat(config): implement Loader + Config against #505 interface + C1 schema (#527) #530

engineer referenced this pull request from a commit 2026-07-25 21:44:44 +02:00

docs(config): correct frozen field-doc enum examples + harden #74 gate (#527 review 4555)

engineer referenced this pull request 2026-07-25 21:45:38 +02:00

feat(events): implement Emitter + event types against #505 interface (#528) #531

engineer referenced this pull request from a commit 2026-07-26 21:48:40 +02:00

docs(config): correct frozen field-doc enum examples + harden #74 gate (#527 review 4555)

engineer referenced this pull request from a commit 2026-07-26 22:12:42 +02:00

docs(config): correct frozen field-doc enum examples + harden #74 gate (#527 review 4555)

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

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!526

No description provided.