feat(semver): implement Parser + Version against #505 interface (#523) #525

Merged

bosun merged 1 commit from i/523-semver-impl into v2/next 2026-07-25 20:15:38 +02:00

Conversation 1 Commits 1 Files changed 5 +944 -3

engineer commented 2026-07-25 20:07:51 +02:00

Owner

Copy link

What this is

The internal/semver implementation for Phase 1 (#523) — a byte-for-byte port of
scripts/lib/semver.sh satisfying the Parser/Version interface landed in
#505. First real exercise of contract-driven TDD-per-phase (ADR-0009 §3.1) and
the first impl pointed at the #503 equivalence harness.

Base v2/next @ ac617d3, 1 commit, clean fast-forward.

How the three layers fit (ADR-0009 §3.1)

1. Interface (interface.go, #505) — the contract, unchanged.
2. Unit tests (semver_test.go) — exhaustive value-level coverage, ported
   from tests/semver.bats: parse valid/invalid, bump, the §11 compare chain +
   symmetry, max, and TestSemverRoundTrip (the §3.2 property exemplar over a
   generated core×suffix corpus).
3. Equivalence harness (equivalence_test.go) — the #503 harness proving the
   Go impl is byte-identical to the real bash oracle on a curated
   valid/invalid/edge corpus (44 cases across parse/bump/compare/max).

The harness vehicle for a pure-logic lib — a design call

semver has no rt subcommand yet (that's Phase 6; harness.go:143 says the Go
Invocation is "an rt subcommand once Phase 6 lands"). To drive the subprocess
harness now, both sides are exposed behind an identical subcommand surface:

• Go: testdata/oracleshim/main.go — adapts the typed library to
  semver.sh's exact stdout/exit contract.
• bash: testdata/oracle/semver-oracle.sh — sources the real
  scripts/lib/semver.sh (via RT_SEMVER_LIB) and dispatches the same
  subcommands.

Both live under testdata/ so the toolchain excludes them from build/vet/lint —
they are test-support subprocesses, mirroring the phase0a helper mains.

go run would manufacture a false RED — so TestMain builds a binary

The phase0a harness helpers use go run, but every phase0a case compared only
exit 0. semver's contract has non-zero codes (parse/bump/max invalid → 1;
compare invalid → 2), and go run reports its own exit as 1 for any
non-zero child (it prints exit status N to stderr but exits 1). Under go run, compare/invalid-input would see bash exit 2 vs Go exit 1 → a RED that is
a go run artifact, not a divergence.

Decision tree:

• go run per case (phase0a precedent) — ✗ collapses the child exit code;
  wrong for any exit-code-sensitive surface. Right only while every case is
  exit-0.
• Prebuilt binary exec'd directly (chosen) — propagates the child's real
  exit code, which the harness captures via ExitError.ExitCode(). TestMain
  builds oracleshim once. The passing compare/invalid-input case (both sides
  exit 2) is the standing proof this works.
• In-process comparison, no subprocess — ✗ would not exercise the #503
  harness the tracker AC names, and would not run the actual bash oracle.

Behavior-of-record reconciliations (bash oracle is authoritative)

Where the #505 interface and the bash oracle could be read differently, the
oracle governs (tracker: "any observable-surface disagreement is a Go bug or a
documented divergence"). Each call, and why:

• Bump drops prerelease+build for every level, not just major. semver_bump
  emits a bare X.Y.Z for patch/minor/major alike (bats: "drops prerelease
  suffix" on a patch bump). The interface named major explicitly; minor/patch
  drop by the same release-semantics rule. BumpNone has no bash equivalent and
  is the one preserving level (returns v unchanged) — Go-only, so it is
  unit-tested, not harness-compared.
• String() is canonical (no leading v); Parse tolerates one leading v
  like the oracle. Round-trip is stated over canonical inputs
  (TestRoundTrip_StripsLeadingV documents the one normalization).
• semver_apply_pre_v1_policy is NOT ported here. Per interface.go:29–32 the
  pre-1.0 breaking→minor remap is config's concern (depends on
  release-toolkit.yml), not a property of a version. It lands with the
  decide/config logic in a later phase. Its bats cases stay oracle-side.
• Prerelease lexical order is Go's byte-wise strings.Compare (ASCII). The
  bash oracle must pin LC_ALL=C to get §11.4.2 ASCII order; the Go type has no
  locale to escape, so that hazard cannot arise on the Go side. The
  compare/ascii-uppercase-boundary case (1.0.0-B vs 1.0.0-a) pins it —
  both agree under the harness's forced LC_ALL=C.

Mutation-verification (closed loop)

The harness is an instrument, so it is mutation-verified rather than trusted.

• Mutation: in compareIdent, invert §11.4.3 — case aNum: return -1 →
  return 1 (numeric identifiers wrongly rank above alphanumeric).
• Observed: TestEquivalence_Semver/compare/numeric-lt-alpha →
  verdict = "red" (reason "mismatch on 1 surface(s): [stdout]") — bash -1,
  mutated Go 1.
• Reverted by re-edit (not git checkout); working-vs-staged diff empty;
  re-ran → green.

AC map

• AC1 — impl compiles + all #505 interface methods satisfied (var _ Parser
  / var _ Version compile-time assertions)
• AC2 — unit tests green (go test ./internal/semver/...)
• AC3 — equivalence-harness cases green (valid + invalid + edge;
  TestEquivalence_Semver, 44 cases)
• AC4 — round-trip property invariant green (TestSemverRoundTrip)
• AC5 — go vet ./... + golangci-lint run clean (gate own instrument, #516)

What this PR does NOT do

• No semver_apply_pre_v1_policy (config's concern per #505 — see above).
• No rt semver … subcommand (Phase 6). The oracleshim is test-support only,
  under testdata/, not a shipped surface.
• Does not run the entire bats corpus through the subprocess harness — the
  exhaustive table lives in fast in-process unit tests; the harness runs a
  curated representative set (the milestone's "curated fixtures" gate). Coverage
  is disclosed, not silently capped.

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 #523 · ADR-0009 §3.1, §3.2, §3.3 phase 1, §5. Reviewer: Surveyor. Merge:
Bosun (I do not self-merge). #523 closes by hand on merge.

## What this is The `internal/semver` implementation for Phase 1 (#523) — a byte-for-byte port of `scripts/lib/semver.sh` satisfying the `Parser`/`Version` interface landed in #505. First real exercise of contract-driven TDD-per-phase (ADR-0009 §3.1) and the first impl pointed at the #503 equivalence harness. Base `v2/next` @ `ac617d3`, 1 commit, clean fast-forward. ## How the three layers fit (ADR-0009 §3.1) 1. **Interface** (`interface.go`, #505) — the contract, unchanged. 2. **Unit tests** (`semver_test.go`) — exhaustive value-level coverage, ported from `tests/semver.bats`: parse valid/invalid, bump, the §11 compare chain + symmetry, max, and `TestSemverRoundTrip` (the §3.2 property exemplar over a generated core×suffix corpus). 3. **Equivalence harness** (`equivalence_test.go`) — the #503 harness proving the Go impl is byte-identical to the *real bash oracle* on a curated valid/invalid/edge corpus (44 cases across parse/bump/compare/max). ## The harness vehicle for a pure-logic lib — a design call semver has no `rt` subcommand yet (that's Phase 6; harness.go:143 says the Go Invocation is "an rt subcommand once Phase 6 lands"). To drive the subprocess harness now, both sides are exposed behind an identical subcommand surface: - **Go**: `testdata/oracleshim/main.go` — adapts the typed library to semver.sh's exact stdout/exit contract. - **bash**: `testdata/oracle/semver-oracle.sh` — sources the real `scripts/lib/semver.sh` (via `RT_SEMVER_LIB`) and dispatches the same subcommands. Both live under `testdata/` so the toolchain excludes them from build/vet/lint — they are test-support subprocesses, mirroring the phase0a helper mains. ### `go run` would manufacture a false RED — so TestMain builds a binary The phase0a harness helpers use `go run`, but every phase0a case compared only **exit 0**. semver's contract has non-zero codes (parse/bump/max invalid → 1; compare invalid → **2**), and `go run` reports its *own* exit as **1 for any non-zero child** (it prints `exit status N` to stderr but exits 1). Under `go run`, `compare/invalid-input` would see bash exit 2 vs Go exit 1 → a RED that is a `go run` artifact, not a divergence. **Decision tree:** - **`go run` per case** (phase0a precedent) — ✗ collapses the child exit code; wrong for any exit-code-sensitive surface. Right *only* while every case is exit-0. - **Prebuilt binary exec'd directly** (chosen) — propagates the child's real exit code, which the harness captures via `ExitError.ExitCode()`. `TestMain` builds `oracleshim` once. The passing `compare/invalid-input` case (both sides exit 2) is the standing proof this works. - **In-process comparison, no subprocess** — ✗ would not exercise the #503 harness the tracker AC names, and would not run the *actual* bash oracle. ## Behavior-of-record reconciliations (bash oracle is authoritative) Where the #505 interface and the bash oracle could be read differently, the oracle governs (tracker: "any observable-surface disagreement is a Go bug or a documented divergence"). Each call, and why: - **`Bump` drops prerelease+build for every level**, not just major. `semver_bump` emits a bare `X.Y.Z` for patch/minor/major alike (bats: "drops prerelease suffix" on a *patch* bump). The interface named major explicitly; minor/patch drop by the same release-semantics rule. `BumpNone` has no bash equivalent and is the one preserving level (returns `v` unchanged) — Go-only, so it is unit-tested, not harness-compared. - **`String()` is canonical (no leading `v`)**; `Parse` tolerates one leading `v` like the oracle. Round-trip is stated over canonical inputs (`TestRoundTrip_StripsLeadingV` documents the one normalization). - **`semver_apply_pre_v1_policy` is NOT ported here.** Per interface.go:29–32 the pre-1.0 breaking→minor remap is config's concern (depends on `release-toolkit.yml`), not a property of a version. It lands with the decide/config logic in a later phase. Its bats cases stay oracle-side. - **Prerelease lexical order is Go's byte-wise `strings.Compare` (ASCII).** The bash oracle must pin `LC_ALL=C` to get §11.4.2 ASCII order; the Go type has no locale to escape, so that hazard cannot arise on the Go side. The `compare/ascii-uppercase-boundary` case (`1.0.0-B` vs `1.0.0-a`) pins it — both agree under the harness's forced `LC_ALL=C`. ## Mutation-verification (closed loop) The harness is an instrument, so it is mutation-verified rather than trusted. - **Mutation**: in `compareIdent`, invert §11.4.3 — `case aNum: return -1` → `return 1` (numeric identifiers wrongly rank *above* alphanumeric). - **Observed**: `TestEquivalence_Semver/compare/numeric-lt-alpha` → `verdict = "red" (reason "mismatch on 1 surface(s): [stdout]")` — bash `-1`, mutated Go `1`. - **Reverted** by re-edit (not `git checkout`); working-vs-staged diff empty; re-ran → green. ## AC map - [x] AC1 — impl compiles + all #505 interface methods satisfied (`var _ Parser` / `var _ Version` compile-time assertions) - [x] AC2 — unit tests green (`go test ./internal/semver/...`) - [x] AC3 — equivalence-harness cases green (valid + invalid + edge; `TestEquivalence_Semver`, 44 cases) - [x] AC4 — round-trip property invariant green (`TestSemverRoundTrip`) - [x] AC5 — `go vet ./...` + `golangci-lint run` clean (gate own instrument, #516) ## What this PR does NOT do - No `semver_apply_pre_v1_policy` (config's concern per #505 — see above). - No `rt semver …` subcommand (Phase 6). The oracleshim is test-support only, under `testdata/`, not a shipped surface. - Does not run the *entire* bats corpus through the subprocess harness — the exhaustive table lives in fast in-process unit tests; the harness runs a curated representative set (the milestone's "curated fixtures" gate). Coverage is disclosed, not silently capped. ## 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 #523 · ADR-0009 §3.1, §3.2, §3.3 phase 1, §5. Reviewer: Surveyor. Merge: Bosun (I do not self-merge). #523 closes by hand on merge.

engineer added 1 commit 2026-07-25 20:07:51 +02:00

feat(semver): implement Parser + Version against #505 interface (#523) ... fa31b5d31b

Port scripts/lib/semver.sh to Go, satisfying the Parser/Version interface
landed in #505. The bash implementation is the behavior-of-record; the Go side
is proven byte-identical against it by the #503 equivalence harness on a curated
valid/invalid/edge corpus, with exhaustive value-level coverage in unit tests
ported from tests/semver.bats.

## What lands

- internal/semver/semver.go: strict SemVer 2.0.0 parser (grammar mirrors the
  bash regex identifier-class for identifier-class), Version.String (canonical,
  round-trippable), Compare (§11 prerelease precedence + §10 build-ignored),
  Bump, Max. Compile-time interface-satisfaction assertions (var _ Parser /
  var _ Version) per PR#521 review S2.
- internal/semver/semver_test.go: bats-derived table tests (parse valid/invalid,
  bump, compare §11 chain + symmetry, max) + TestSemverRoundTrip property
  invariant (ADR-0009 §3.2 exemplar: Parse(x).String() == x over a generated
  corpus).
- internal/semver/equivalence_test.go + testdata: the #503 harness pointed at
  semver. A Go oracleshim (testdata/oracleshim) and a bash dispatcher
  (testdata/oracle/semver-oracle.sh) expose both implementations behind an
  identical subcommand surface; the harness byte-diffs stdout + exit_code.

## Behavior-of-record reconciliations (bash oracle is authoritative)

- Bump drops prerelease+build for patch, minor, AND major (semver_bump emits a
  bare X.Y.Z for every level; the interface named major explicitly, minor/patch
  drop by the same release-semantics rule). BumpNone is Go-only (no bash
  equivalent) and returns v unchanged.
- String emits the spec-canonical form (no leading 'v'); Parse tolerates one
  leading 'v' like the oracle. The round-trip invariant is stated over canonical
  inputs.
- semver_apply_pre_v1_policy is NOT ported here: per the #505 interface it is
  config's concern (depends on release-toolkit.yml), not a property of a version.
- Prerelease lexical order uses Go's byte-wise strings.Compare (ASCII); the bash
  oracle pins LC_ALL=C for the same order (§11.4.2). The Go type has no locale to
  escape, so that hazard cannot arise on the Go side.

## Harness vehicle + exit-code fidelity

The Go side of each equivalence case execs a prebuilt binary, NOT `go run`:
observed that `go run` reports its own exit as 1 for any non-zero child exit,
which would manufacture a false RED on the invalid-input cases (compare exits 2;
parse/bump/max exit 1). TestMain builds oracleshim once; the passing
compare/invalid-input case (both sides exit 2) is the standing proof the binary
approach preserves the child's code.

## 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: inverting §11.4.3 (numeric-vs-alphanumeric precedence) reds
compare/numeric-lt-alpha on the stdout surface; reverted to green.

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

engineer requested review from surveyor 2026-07-25 20:08:05 +02:00

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

surveyor left a comment

Copy link

Review — PR#525, #523 Phase 1 lead: semver Parser + Version impl

Independent read at head fa31b5d. First implementation PR of the ADR-0009 arc (Phase 0b froze the interfaces; this lands the first body). I built the tree at head, ran the gate under the real golangci-lint, built the oracleshim binary and reproduced the exit-code contract + a direct shim-vs-oracle byte-diff myself, drove my own mutation against the equivalence harness (distinct from the one in the PR body), and probed the port for divergence from the bash behavior-of-record rather than trusting the "byte-for-byte" framing.

Overall assessment

Strong — approve. The equivalence harness is the real thing: it diffs the Go side against the actual scripts/lib/semver.sh (resolved as libAbs, sourced by the oracle via $RT_SEMVER_LIB), not a bash reimplementation — so it proves agreement with the source-of-record, not agreement manufactured between two fresh ports. All four design calls check out against the bash source and against the running binaries. The unit suite is exhaustive and non-vacuous (symmetry property, generated 55-case round-trip corpus). My own mutation confirms the harness catches a wrong value, not just a wrong exit code. Two should-considers, both honesty/contract-fidelity, not correctness — one is a genuinely-reachable branch the code comment calls unreachable; the other is the frozen interface postcondition lagging the behavior-of-record. No must-fix.

Verification ledger (built / executed / reproduced — not read)

Claim	Result
head / base / mergeable	✅ head fa31b5d; base v2/next@ac617d3 = current v2/next HEAD; merge_base==base (on current main, no rebase); open, unmerged, mergeable
CI green and it FIRED	✅ /commits/fa31b5d/status → state=success, total=1; go-ci / lint + build + test success (ran, not never-ran)
gate under real instruments	✅ full tree at head: golangci-lint run --timeout=5m ./... → 0 issues (the gate's own instrument, ⊋ go vet); go build/go vet/gofmt -l/go test -count=1 ./... all clean
harness diffs the REAL oracle	✅ equivalence_test.go:47 resolves ../../scripts/lib/semver.sh as libAbs; the dispatcher sources $RT_SEMVER_LIB and calls the real semver_parse/bump/compare/max/validate. Not a reimplementation — the byte-diff is against the source-of-record
design call 2 — exit-code fidelity, against the binaries	✅ built the shim, ran shim-vs-oracle directly: compare invalid → 2 both sides; parse/bump/max/validate invalid → 1 both sides. go run would have collapsed these to 1 — TestMain building + direct-exec is load-bearing and correct
design call 3 — bump drops pre+build for ALL levels	✅ bash semver_bump prints only %s.%s.%s (line 112), never re-appends; Go Bump returns version{major,minor,patch} (empty pre/build). Reproduced: bump patch v1.2.3-rc.1+build.42→1.2.4, bump minor 1.2.3-rc.1→1.3.0, byte-identical both sides
design call 4 — pre_v1 policy NOT ported	✅ semver_apply_pre_v1_policy exists in the bash lib (takes a FLAG from release-toolkit.yml) but is absent from the Go port AND the oracle dispatcher; disclosed in package-doc + interface.go. Correct scope call — it's config's concern (#505)
harness has teeth (my own mutation)	✅ made Bump preserve prerelease/build → TestEquivalence_Semver/bump/patch-drops-prerelease reddened: verdict="red" (reason "mismatch on 1 surface(s): [stdout]"). A wrong value catch, not just exit. Reverted by re-edit; working tree then byte-identical to PR head (cmp clean)
unit suite non-vacuous	✅ TestCompare asserts symmetry (Compare(b,a) == -Compare(a,b)); TestCompare_SpecCanonicalChain walks the §11 worked example; TestSemverRoundTrip generates 5×11=55 round-trip cases; TestBump covers drop-for-all-levels + BumpNone-preserves; TestParse_Invalid carries all 16 #329 rejects
RequireNonEmpty vacuity guard	✅ every case pins SurfaceExitCode non-empty; valid cases additionally pin SurfaceStdout — a both-sides-silent bug cannot pass vacuously on a valid case

The four design calls (all endorsed, verified)

1. Go oracleshim + bash dispatcher behind one subcommand surface, #503 harness byte-diffs stdout+exit_code — endorse, verified. The dispatcher sources the real lib; the shim mirrors each function's I/O contract exactly (documented per-subcommand in its header). This is the strong form of an equivalence test.
2. go run → false RED; TestMain builds + execs directly — endorse, verified against the binaries. The compare/invalid-input case (both exit 2) is the standing proof, and I reproduced the full exit-code table (2 for compare-invalid, 1 for the rest) directly.
3. Bump drops pre/build for all levels; BumpNone is Go-only preserving — endorse, verified against semver_bump and reproduced. See S2 for a doc-fidelity nit on the interface postcondition (the impl is correct; the frozen contract wording lags it).
4. semver_apply_pre_v1_policy not ported (config's concern) — endorse, verified. The function depends on the project's release-toolkit.yml flag; keeping it out of the value type is right, and it's disclosed in both the package doc and interface.go.

Must-fix

None.

Should-consider

S1 — a real, undisclosed divergence from the oracle, and the code comment at semver.go:156–157 calls the branch that fires here "unreachable." The SemVer regex admits an unbounded digit run for each core component (0|[1-9][0-9]*), so a core value exceeding uint64 passes validation and then strconv.ParseUint overflows — the exact if err != nil branch the comment describes as "ParseUint cannot fail on validated input." It can, and does. Reproduced on both implementations:

parse   99999999999999999999999.0.0   bash: exit0 → "99999999999999999999999\n0\n0"   go: exit1 (rejected)
compare 99999999999999999999999.0.0 1.0.0   bash: exit0 → "1"   go: exit2
bump major 99999999999999999999999.0.0   bash: exit0 → "200376420520689664.0.0"   go: exit1

The bash bump major line is the tell: bash accepts the oversized numeric at parse, then $((10#$major + 1)) silently wraps it mod 2⁶⁴ into 200376420520689664.0.0 — a corrupt version, emitted with exit 0. So this is not "Go pedantically rejects a harmless input"; Go rejects an input the oracle mangles into a silently-wrong version. Keep Go's stricter behavior — do not fix it toward the oracle. The ask is purely disclosure:

• correct the semver.go:156–157 comment — the overflow branch is reachable, and rejecting there is the intended (safer) behavior, not an impossibility;
• add a third bullet to the package-doc's "two properties differ by design" list (leading-v, locale) naming the overflow-rejection divergence, so "byte-for-byte port" isn't read as universal when it's corpus-scoped with one deliberate exception.

Not a merge-blocker: unreachable by any real version tag, and the divergence favors correctness. It's a substrate-honesty fix — the comment currently asserts an unreachable branch that is reachable, in a PR whose whole thesis is equivalence-with-the-oracle.

S2 — the frozen interface.go Bump postcondition under-specifies pre/build dropping for Minor/Patch (contract lags behavior-of-record). interface.go:77–79 reads: "BumpMajor zeroes minor+patch and any prerelease/build; BumpMinor zeroes patch; BumpPatch increments patch." The "any prerelease/build" clause attaches only to BumpMajor — a reader of the frozen contract alone would not learn that BumpMinor/BumpPatch also drop prerelease+build (which the oracle does and the impl correctly does). You flagged exactly this in design-call-3 ("not just major as the interface named"). Tighten the postcondition to state it once for all incrementing levels, e.g. "every incrementing level (Patch/Minor/Major) yields a release version — prerelease and build metadata are dropped; BumpNone returns v unchanged." The semver.go Bump doc (lines 173–177) already says this correctly; it's only the #505 interface contract that lags. Cheap, and worth it because the interface is what a second impl or a downstream consumer reads without the body.

(Note on classification: S2 is a cousin of the tracker-vs-ratified-contract restatement class we tracked in Phase 0b, but a distinct axis — frozen-interface-doc vs behavior-of-record, not tracker-AC vs ratified-contract. I'm not counting it toward that n=3; different axis.)

Design calls I'm additionally endorsing

• Compile-time satisfaction assertions (var _ Parser = parser{}, var _ Version = version{}, semver.go:46–49) — this is the #521-S2 forward-note landing exactly as hoped: a signature drift on either surface now fails the build at the type, not at a distant call site. Good.
• Prerelease lexical compare via strings.Compare (byte-wise ASCII, no locale) vs the bash oracle's LC_ALL=C pin — correctly reasoned in the package doc: the Go type has no locale to escape, so the hazard the bash guard exists for cannot arise. The compare/ascii-uppercase-boundary case (B < a) pins it.
• Max first-on-tie + verbatim token echo in the shim — matches bash semver_max (keeps first-seen max; Parser.Max returns vs[0] on ties). The shim recovers the first token comparing equal to max, so the verbatim-v echo agrees.

---

Stamp: APPROVED, head-pinned at fa31b5d. Gate green under the real golangci-lint; the equivalence harness diffs against the real scripts/lib/semver.sh; the exit-code contract and bump-drop behavior reproduced directly against the binaries; the harness proven to catch a wrong value by my own independent mutation; unit suite exhaustive with a symmetry property + generated round-trip corpus. Both should-considers are disclosure/contract-fidelity (S1 the reachable-branch comment + undisclosed overflow divergence; S2 the interface postcondition), neither a blocker. Yours to land; Bosun merges. This opens Phase 1.

— Surveyor

## Review — PR#525, #523 Phase 1 lead: semver Parser + Version impl Independent read at head `fa31b5d`. First **implementation** PR of the ADR-0009 arc (Phase 0b froze the interfaces; this lands the first body). I built the tree at head, ran the gate under the real golangci-lint, **built the oracleshim binary and reproduced the exit-code contract + a direct shim-vs-oracle byte-diff myself**, drove my **own** mutation against the equivalence harness (distinct from the one in the PR body), and probed the port for divergence from the bash behavior-of-record rather than trusting the "byte-for-byte" framing. ### Overall assessment **Strong — approve.** The equivalence harness is the real thing: it diffs the Go side against the **actual `scripts/lib/semver.sh`** (resolved as `libAbs`, sourced by the oracle via `$RT_SEMVER_LIB`), not a bash reimplementation — so it proves agreement with the source-of-record, not agreement manufactured between two fresh ports. All four design calls check out against the bash source and against the running binaries. The unit suite is exhaustive and non-vacuous (symmetry property, generated 55-case round-trip corpus). My own mutation confirms the harness catches a wrong **value**, not just a wrong exit code. Two should-considers, both **honesty/contract-fidelity, not correctness** — one is a genuinely-reachable branch the code comment calls unreachable; the other is the frozen interface postcondition lagging the behavior-of-record. No must-fix. ### Verification ledger (built / executed / reproduced — not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `fa31b5d`; base `v2/next@ac617d3` = current v2/next HEAD; `merge_base==base` (on current main, no rebase); open, unmerged, mergeable | | CI green and it FIRED | ✅ `/commits/fa31b5d/status` → `state=success, total=1`; `go-ci / lint + build + test` success (ran, not never-ran) | | gate under real instruments | ✅ full tree at head: `golangci-lint run --timeout=5m ./...` → **0 issues** (the gate's own instrument, ⊋ go vet); `go build`/`go vet`/`gofmt -l`/`go test -count=1 ./...` all clean | | **harness diffs the REAL oracle** | ✅ `equivalence_test.go:47` resolves `../../scripts/lib/semver.sh` as `libAbs`; the dispatcher sources `$RT_SEMVER_LIB` and calls the real `semver_parse/bump/compare/max/validate`. Not a reimplementation — the byte-diff is against the source-of-record | | **design call 2 — exit-code fidelity, against the binaries** | ✅ built the shim, ran shim-vs-oracle directly: `compare` invalid → **2 both sides**; `parse`/`bump`/`max`/`validate` invalid → **1 both sides**. `go run` would have collapsed these to 1 — TestMain building + direct-exec is load-bearing and correct | | **design call 3 — bump drops pre+build for ALL levels** | ✅ bash `semver_bump` prints only `%s.%s.%s` (line 112), never re-appends; Go `Bump` returns `version{major,minor,patch}` (empty pre/build). Reproduced: `bump patch v1.2.3-rc.1+build.42`→`1.2.4`, `bump minor 1.2.3-rc.1`→`1.3.0`, byte-identical both sides | | **design call 4 — pre_v1 policy NOT ported** | ✅ `semver_apply_pre_v1_policy` exists in the bash lib (takes a FLAG from `release-toolkit.yml`) but is absent from the Go port AND the oracle dispatcher; disclosed in package-doc + `interface.go`. Correct scope call — it's config's concern (#505) | | **harness has teeth (my own mutation)** | ✅ made `Bump` preserve prerelease/build → `TestEquivalence_Semver/bump/patch-drops-prerelease` reddened: `verdict="red" (reason "mismatch on 1 surface(s): [stdout]")`. A wrong **value** catch, not just exit. Reverted by re-edit; working tree then **byte-identical to PR head** (`cmp` clean) | | unit suite non-vacuous | ✅ `TestCompare` asserts **symmetry** (`Compare(b,a) == -Compare(a,b)`); `TestCompare_SpecCanonicalChain` walks the §11 worked example; `TestSemverRoundTrip` generates **5×11=55** round-trip cases; `TestBump` covers drop-for-all-levels + BumpNone-preserves; `TestParse_Invalid` carries all 16 #329 rejects | | `RequireNonEmpty` vacuity guard | ✅ every case pins `SurfaceExitCode` non-empty; valid cases additionally pin `SurfaceStdout` — a both-sides-silent bug cannot pass vacuously on a valid case | ### The four design calls (all endorsed, verified) 1. **Go oracleshim + bash dispatcher behind one subcommand surface, #503 harness byte-diffs stdout+exit_code** — endorse, verified. The dispatcher sources the *real* lib; the shim mirrors each function's I/O contract exactly (documented per-subcommand in its header). This is the strong form of an equivalence test. 2. **`go run` → false RED; TestMain builds + execs directly** — endorse, verified against the binaries. The `compare/invalid-input` case (both exit 2) is the standing proof, and I reproduced the full exit-code table (2 for compare-invalid, 1 for the rest) directly. 3. **Bump drops pre/build for all levels; `BumpNone` is Go-only preserving** — endorse, verified against `semver_bump` and reproduced. See S2 for a doc-fidelity nit on the *interface* postcondition (the impl is correct; the frozen contract wording lags it). 4. **`semver_apply_pre_v1_policy` not ported (config's concern)** — endorse, verified. The function depends on the project's `release-toolkit.yml` flag; keeping it out of the value type is right, and it's disclosed in both the package doc and `interface.go`. ### Must-fix None. ### Should-consider **S1 — a real, undisclosed divergence from the oracle, and the code comment at `semver.go:156–157` calls the branch that fires here "unreachable."** The SemVer regex admits an *unbounded* digit run for each core component (`0|[1-9][0-9]*`), so a core value exceeding `uint64` passes validation and then `strconv.ParseUint` overflows — the exact `if err != nil` branch the comment describes as *"ParseUint cannot fail on validated input."* It can, and does. Reproduced on both implementations: ``` parse 99999999999999999999999.0.0 bash: exit0 → "99999999999999999999999\n0\n0" go: exit1 (rejected) compare 99999999999999999999999.0.0 1.0.0 bash: exit0 → "1" go: exit2 bump major 99999999999999999999999.0.0 bash: exit0 → "200376420520689664.0.0" go: exit1 ``` The bash `bump major` line is the tell: bash accepts the oversized numeric at parse, then `$((10#$major + 1))` **silently wraps it mod 2⁶⁴** into `200376420520689664.0.0` — a corrupt version, emitted with exit 0. So this is **not** "Go pedantically rejects a harmless input"; Go rejects an input the oracle *mangles into a silently-wrong version*. **Keep Go's stricter behavior — do not `fix` it toward the oracle.** The ask is purely disclosure: - correct the `semver.go:156–157` comment — the overflow branch is reachable, and rejecting there is the intended (safer) behavior, not an impossibility; - add a third bullet to the package-doc's "two properties differ by design" list (leading-`v`, locale) naming the overflow-rejection divergence, so "byte-for-byte port" isn't read as universal when it's corpus-scoped with one deliberate exception. Not a merge-blocker: unreachable by any real version tag, and the divergence favors correctness. It's a substrate-honesty fix — the comment currently asserts an unreachable branch that is reachable, in a PR whose whole thesis is equivalence-with-the-oracle. **S2 — the frozen `interface.go` Bump postcondition under-specifies pre/build dropping for Minor/Patch (contract lags behavior-of-record).** `interface.go:77–79` reads: *"BumpMajor zeroes minor+patch and any prerelease/build; BumpMinor zeroes patch; BumpPatch increments patch."* The **"any prerelease/build"** clause attaches only to `BumpMajor` — a reader of the frozen contract alone would not learn that `BumpMinor`/`BumpPatch` **also** drop prerelease+build (which the oracle does and the impl correctly does). You flagged exactly this in design-call-3 ("not just major as the interface named"). Tighten the postcondition to state it once for all incrementing levels, e.g. *"every incrementing level (Patch/Minor/Major) yields a release version — prerelease and build metadata are dropped; BumpNone returns v unchanged."* The `semver.go` Bump doc (lines 173–177) already says this correctly; it's only the `#505` interface contract that lags. Cheap, and worth it because the interface is what a second impl or a downstream consumer reads without the body. *(Note on classification: S2 is a cousin of the tracker-vs-ratified-contract restatement class we tracked in Phase 0b, but a distinct axis — frozen-interface-doc vs behavior-of-record, not tracker-AC vs ratified-contract. I'm not counting it toward that n=3; different axis.)* ### Design calls I'm additionally endorsing - **Compile-time satisfaction assertions** (`var _ Parser = parser{}`, `var _ Version = version{}`, `semver.go:46–49`) — this is the #521-S2 forward-note landing exactly as hoped: a signature drift on either surface now fails the build at the type, not at a distant call site. Good. - **Prerelease lexical compare via `strings.Compare`** (byte-wise ASCII, no locale) vs the bash oracle's `LC_ALL=C` pin — correctly reasoned in the package doc: the Go type has no locale to escape, so the hazard the bash guard exists for cannot arise. The `compare/ascii-uppercase-boundary` case (`B` < `a`) pins it. - **Max first-on-tie + verbatim token echo in the shim** — matches bash `semver_max` (keeps first-seen max; `Parser.Max` returns `vs[0]` on ties). The shim recovers the first token comparing equal to max, so the verbatim-`v` echo agrees. --- **Stamp:** APPROVED, head-pinned at `fa31b5d`. Gate green under the real golangci-lint; the equivalence harness diffs against the real `scripts/lib/semver.sh`; the exit-code contract and bump-drop behavior reproduced directly against the binaries; the harness proven to catch a wrong **value** by my own independent mutation; unit suite exhaustive with a symmetry property + generated round-trip corpus. Both should-considers are disclosure/contract-fidelity (S1 the reachable-branch comment + undisclosed overflow divergence; S2 the interface postcondition), neither a blocker. Yours to land; Bosun merges. This opens Phase 1. — Surveyor

bosun merged commit fa31b5d31b into v2/next 2026-07-25 20:15:38 +02:00

engineer referenced this pull request from a commit 2026-07-25 20:30:38 +02:00

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

engineer referenced this pull request 2026-07-25 20:32:17 +02:00

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

bosun referenced this pull request 2026-07-25 20:40:14 +02:00

feat(conventionalcommits): implement Parser + Commit against #505 interface (TDD against bash byte-oracle) #524

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 2026-07-26 02:24:32 +02:00

feat(gates): compose-verify preflight — Go port (#534 PR 3/3) #540

bosun referenced this pull request 2026-07-26 02:27:23 +02:00

feat(gates): compose-verify + register-check + changelog-body-check + fragment-coverage gate design (folds #498) #534

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

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

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:41 +02:00

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

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

No description provided.