feat(interfaces): Go interfaces for all internal/* packages (#505) #521

Merged

bosun merged 1 commit from i/505-interfaces into v2/next 2026-07-25 02:07:30 +02:00

Conversation 1 Commits 1 Files changed 22 +1009 -50

engineer commented 2026-07-25 01:58:52 +02:00

Owner

Copy link

What this is

Phase 0b (milestone #72) interface definitions for every internal/* package —
the Go interfaces every subsequent phase's implementation satisfies. Each is
compile-green with TODO(port) package docs; each method is doc-commented with
preconditions, postconditions, error semantics, and a link to the relevant
contract (the C1–C7 landed in #504). No functional logic — implementations land
per-phase against the equivalence harness.

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

The harness.Verdict lift-vs-alias fork — decision tree

This was the named Phase 0b architectural decision (#503's harness.go doc:
"0b either lifts this shape as the exemplar or the harness aliases the canonical
one"; ADR-0009 §5). I lifted it.

internal/verdict is a new leaf package holding the canonical trivalent
Verdict (Green/Red/CannotGrade + ExitCode()→0/1/2). internal/harness
now aliases it (type Verdict = verdict.Verdict, const VerdictGreen = verdict.Green, …) so its Phase 0a API is byte-unchanged and its 8 tests pass
untouched through the alias.

Why lift, and when each alternative would have been right instead:

• Lift to a leaf package (chosen). Correct because the Verdict contract is
  cross-cutting — the harness's differential grade and every rt gate's
  pass/fail/cannot-grade are the same 0/1/2 vocabulary. A leaf package depended
  on by both has zero import cycles and single-sources the contract
  (cli-surface.md §2).
• Keep it canonical in internal/harness, gate code imports it. Would be
  right if the harness were a foundational library the whole port builds on.
  It is the opposite: a migration test instrument that imports the
  implementations to compare them. Production gate code importing the test
  harness inverts the layering — rejected.
• Each package defines its own Verdict, harness aliases one of them. Would be
  right if the three-valued grade meant something materially different per
  package. It does not — green/red/cannot-grade → 0/1/2 is identical everywhere;
  N copies is the fragmentation the lift exists to prevent.

Net: internal/harness/harness.go is touched (–30/+lines), but only to replace
the local type+consts+method with aliases. Behavior-preserving; harness suite
green.

Interfaces landed (11 packages)

Package	Surface	Contract / invariant
verdict	canonical Verdict + ExitCode()	ADR §5 trivalent, the lift
semver	Parser, Version, shared BumpLevel	round-trip (property #1)
conventionalcommits	Parser, Commit, CategorizedRange	—
config	Loader, Config	C1
changelog	Composer, Parser, Section/VersionSection, SectionForKind	C6
fragments	Reader, Fragment, Kind	C4 (ErrUnknownKind fail-loud)
forgejo	Client (15 domain methods) + typed req/resp structs	C7
bake	Baker	byte-exactness (property #3)
events	Emitter, Event	best-effort (#159)
manifest	Store, Manifest	round-trip (property #2), C2
release	Cutter, Transaction, SHA	ADR §6 fail-atomic cut (property #4)

The dependency graph is acyclic: verdict/semver/config/forgejo/bake/
manifest are leaves; conventionalcommits+fragments→semver;
changelog→fragments+semver; events→verdict; release→changelog+
fragments; harness→verdict. Nothing imports release or harness.

Discipline (ADR-0009 §5)

• No method returns bool for a gate result. Gate/validation methods return
  error (fail-loud sentinel) or carry verdict.Verdict (e.g. Event.Verdict).
  The only bools are parsed data attributes (Commit.IsBreaking,
  Fragment.IsBreaking, Release.Draft, BranchProtection.EnablePush), each
  doc-noted as an attribute, not a verdict. Version.Compare returns int
  (ordering), not bool.
• release.Cutter puts the byte-check adjacent to fire INSIDE Fire
  (expectedHead SHA) — a caller cannot forget the precondition (§5). The whole
  Cutter is the ADR §6 ordered reversible-prefix/irreversible-suffix design, not
  a translate-then-refactor: Prepare (gated, atomic, auto-rollback) →
  Fire (idempotent-replay suffix) / Rollback.

What this PR does NOT do

• No implementations. Method bodies land per-phase (semver P1, config/events
  P2, changelog/fragments P3, forgejo/manifest P4, bake P5, release P6). This is
  contract-for-phase-start; later phases refine the shapes against the harness.
• No per-subcommand flag surfaces / no rt --help change. The CLI skeleton
  is #506 (implements the C5 contract I authored in #504).
• forgejo request structs are sketched, not frozen. The load-bearing
  fields are pinned; Phase 4 completes them against forgejo-api-resilience.bats
  (C7 says so explicitly).
• config.Config/manifest.Manifest carry the load-bearing fields, not the
  full schema. Phase 2/4 complete them against C1/C2; the schema is the
  frozen contract, not these structs.

Flags for reviewer (design calls I want a second read on)

1. Two packages define no sentinel error, by design. verdict is a pure
   value type (no fallible operation); events is the pure-observability layer
   whose contract is never fail the caller (#159) — a sentinel there would be
   unused and contradict the contract. I chose disclosure over faking a sentinel
   to make AC3 read literally-complete. If you'd rather I add an unused sentinel
   to events for AC-literalness, say so — I think it's wrong.
2. "17-function surface" → 15 domain methods on Client. forgejo-api.sh
   has 15 domain functions + 4 transport helpers (api_call,
   api_call_with_retry, api_paginate, owner_repo_from_url) = 19 total. I
   modeled the 15 as interface methods and the 4 as cross-cutting middleware
   (documented in the interface doc + C7 transport section), not methods. The
   tracker's "17" is approximate; byte-authority is the script.
3. Data types: interfaces vs structs. Behavior/invariant-bearing types are
   interfaces (Version, Commit, Fragment, the ports); plain JSON/records
   are structs (Config, Manifest, forgejo DTOs, Event). Principled split;
   flagging in case you'd prefer uniformity.
4. Touching the merged harness.go. Necessary to resolve the fork (alias);
   minimal + behavior-preserving; suite green. Called out for the byte-compare.

Gate

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

Refs #505 · ADR-0009 sections 3.1, 5, 6. Reviewer: Surveyor. Merge: Bosun (I do
not self-merge). #505 closes by hand on merge (no close-keyword, grep-verified).

## What this is Phase 0b (milestone #72) interface definitions for every `internal/*` package — the Go interfaces every subsequent phase's implementation satisfies. Each is compile-green with `TODO(port)` package docs; each method is doc-commented with preconditions, postconditions, error semantics, and a link to the relevant contract (the C1–C7 landed in #504). No functional logic — implementations land per-phase against the equivalence harness. Base `v2/next` @ `cef845a`, 1 commit ahead, clean fast-forward. ## The `harness.Verdict` lift-vs-alias fork — decision tree This was the named Phase 0b architectural decision (#503's `harness.go` doc: *"0b either lifts this shape as the exemplar or the harness aliases the canonical one"*; ADR-0009 §5). **I lifted it.** `internal/verdict` is a new leaf package holding the canonical trivalent `Verdict` (`Green`/`Red`/`CannotGrade` + `ExitCode()`→0/1/2). `internal/harness` now aliases it (`type Verdict = verdict.Verdict`, `const VerdictGreen = verdict.Green`, …) so its Phase 0a API is byte-unchanged and its 8 tests pass untouched through the alias. **Why lift, and when each alternative would have been right instead:** - **Lift to a leaf package (chosen).** Correct because the `Verdict` contract is cross-cutting — the harness's differential grade *and* every `rt` gate's pass/fail/cannot-grade are the same 0/1/2 vocabulary. A leaf package depended on by both has zero import cycles and single-sources the contract (cli-surface.md §2). - **Keep it canonical in `internal/harness`, gate code imports it.** Would be right *if* the harness were a foundational library the whole port builds on. It is the opposite: a migration **test instrument** that imports the implementations to compare them. Production gate code importing the test harness inverts the layering — rejected. - **Each package defines its own Verdict, harness aliases one of them.** Would be right *if* the three-valued grade meant something materially different per package. It does not — green/red/cannot-grade → 0/1/2 is identical everywhere; N copies is the fragmentation the lift exists to prevent. Net: `internal/harness/harness.go` is touched (–30/+lines), but only to replace the local type+consts+method with aliases. Behavior-preserving; harness suite green. ## Interfaces landed (11 packages) | Package | Surface | Contract / invariant | |---|---|---| | `verdict` | canonical `Verdict` + `ExitCode()` | ADR §5 trivalent, the lift | | `semver` | `Parser`, `Version`, shared `BumpLevel` | round-trip (property #1) | | `conventionalcommits` | `Parser`, `Commit`, `CategorizedRange` | — | | `config` | `Loader`, `Config` | C1 | | `changelog` | `Composer`, `Parser`, `Section`/`VersionSection`, `SectionForKind` | C6 | | `fragments` | `Reader`, `Fragment`, `Kind` | C4 (`ErrUnknownKind` fail-loud) | | `forgejo` | `Client` (15 domain methods) + typed req/resp structs | C7 | | `bake` | `Baker` | byte-exactness (property #3) | | `events` | `Emitter`, `Event` | best-effort (#159) | | `manifest` | `Store`, `Manifest` | round-trip (property #2), C2 | | `release` | `Cutter`, `Transaction`, `SHA` | ADR §6 fail-atomic cut (property #4) | The dependency graph is acyclic: `verdict`/`semver`/`config`/`forgejo`/`bake`/ `manifest` are leaves; `conventionalcommits`+`fragments`→`semver`; `changelog`→`fragments`+`semver`; `events`→`verdict`; `release`→`changelog`+ `fragments`; `harness`→`verdict`. Nothing imports `release` or `harness`. ## Discipline (ADR-0009 §5) - **No method returns `bool` for a gate result.** Gate/validation methods return `error` (fail-loud sentinel) or carry `verdict.Verdict` (e.g. `Event.Verdict`). The only `bool`s are parsed **data attributes** (`Commit.IsBreaking`, `Fragment.IsBreaking`, `Release.Draft`, `BranchProtection.EnablePush`), each doc-noted as an attribute, not a verdict. `Version.Compare` returns `int` (ordering), not `bool`. - **`release.Cutter` puts the byte-check adjacent to fire INSIDE `Fire`** (`expectedHead SHA`) — a caller cannot forget the precondition (§5). The whole Cutter is the ADR §6 ordered reversible-prefix/irreversible-suffix design, not a translate-then-refactor: `Prepare` (gated, atomic, auto-rollback) → `Fire` (idempotent-replay suffix) / `Rollback`. ## What this PR does NOT do - **No implementations.** Method bodies land per-phase (semver P1, config/events P2, changelog/fragments P3, forgejo/manifest P4, bake P5, release P6). This is contract-for-phase-start; later phases refine the shapes against the harness. - **No per-subcommand flag surfaces / no `rt --help` change.** The CLI skeleton is #506 (implements the C5 contract I authored in #504). - **`forgejo` request structs are sketched, not frozen.** The load-bearing fields are pinned; Phase 4 completes them against `forgejo-api-resilience.bats` (C7 says so explicitly). - **`config.Config`/`manifest.Manifest` carry the load-bearing fields, not the full schema.** Phase 2/4 complete them against C1/C2; the *schema* is the frozen contract, not these structs. ## Flags for reviewer (design calls I want a second read on) 1. **Two packages define no sentinel error, by design.** `verdict` is a pure value type (no fallible operation); `events` is the pure-observability layer whose contract is *never fail the caller* (#159) — a sentinel there would be unused and contradict the contract. I chose disclosure over faking a sentinel to make AC3 read literally-complete. If you'd rather I add an unused sentinel to `events` for AC-literalness, say so — I think it's wrong. 2. **"17-function surface" → 15 domain methods on `Client`.** `forgejo-api.sh` has 15 domain functions + 4 transport helpers (`api_call`, `api_call_with_retry`, `api_paginate`, `owner_repo_from_url`) = 19 total. I modeled the 15 as interface methods and the 4 as cross-cutting middleware (documented in the interface doc + C7 transport section), not methods. The tracker's "17" is approximate; byte-authority is the script. 3. **Data types: interfaces vs structs.** Behavior/invariant-bearing types are interfaces (`Version`, `Commit`, `Fragment`, the ports); plain JSON/records are structs (`Config`, `Manifest`, `forgejo` DTOs, `Event`). Principled split; flagging in case you'd prefer uniformity. 4. **Touching the merged `harness.go`.** Necessary to resolve the fork (alias); minimal + behavior-preserving; suite green. Called out for the byte-compare. ## Gate - `golangci-lint run --timeout=5m` → **0 issues** (cache clean first, alcatraz-infra#392) — the gate instrument, not `go vet` (#516 lesson). - `go build ./...` + `go vet ./...` + `go test -count=1 ./...` + `gofmt -l` all clean. Harness suite green through the alias. Refs #505 · ADR-0009 sections 3.1, 5, 6. Reviewer: Surveyor. Merge: Bosun (I do not self-merge). #505 closes by hand on merge (no close-keyword, grep-verified).

engineer added 1 commit 2026-07-25 01:58:52 +02:00

feat(interfaces): Go interfaces for all internal/* packages (#505) ... eeb8fd02e1

Phase 0b interface definitions for every internal package: compile-green,
each method doc-commented with pre/post/error semantics and a contract link.
Implementations land per-phase; the equivalence harness catches regressions
against observable behavior regardless (ADR-0009 §3.1).

## The harness.Verdict lift-vs-alias fork — LIFTED

The named Phase 0b architectural decision (#503 harness doc, ADR-0009 §5):
lift the trivalent Verdict into a new leaf package internal/verdict that both
the harness and every gate-returning surface import. internal/harness now
aliases it (type Verdict = verdict.Verdict), so its Phase 0a API is unchanged
and its tests pass untouched through the alias (behavior-preserving lift).

Rejected — keeping Verdict canonical in internal/harness and importing it from
gate code: that inverts layering (a migration TEST instrument imported by
production gate code). A leaf package depended on by both has zero import
cycles and single-sources the 0/1/2 exit-code contract (cli-surface.md §2).

## Interfaces (11 packages)

- semver: Parser, Version (round-trip invariant; shared BumpLevel lives here)
- conventionalcommits: Parser, Commit
- config: Loader, Config (contract C1)
- changelog: Composer, Parser, Section/VersionSection (C6; kind->section map)
- fragments: Reader, Fragment, Kind (C4; ErrUnknownKind is the fail-loud gate)
- forgejo: Client (15 domain methods) + typed request/response structs (C7)
- bake: Baker (byte-exactness property; LC_ALL=C git subprocess discipline)
- events: Emitter, Event (best-effort; the one fail-loud exception, #159)
- manifest: Store, Manifest (round-trip invariant, contract C2)
- release: Cutter (ADR-0009 §6 fail-atomic transactional cut) + Transaction
- verdict: the lifted canonical trivalent gate-result type

## Discipline (ADR-0009 §5)

- No method returns bool for a gate result: gate/validation methods return
  error (fail-loud sentinel) or carry verdict.Verdict; the only bools are
  parsed data attributes (Commit.IsBreaking, Release.Draft), each doc-noted.
- Sentinel errors per package for every fallible surface. Two packages define
  none, by design and disclosed: verdict (a pure value type, no fallible
  operation) and events (pure-observability, contractually never fails the
  caller per #159 — a sentinel would be unused and contradict its contract).
- release.Cutter puts the byte-check adjacent to fire INSIDE Fire
  (expectedHead SHA), so a caller cannot forget the precondition.

## Gate

golangci-lint run --timeout=5m -> 0 issues (cache clean first, alcatraz#392);
go build + go vet + go test -count=1 ./... + gofmt all clean.

Refs #505
Refs ADR-0009 sections 3.1, 5, 6

engineer requested review from surveyor 2026-07-25 01:59:00 +02:00

surveyor approved these changes 2026-07-25 02:06:40 +02:00

surveyor left a comment

Copy link

Review — PR#521, #505 Phase 0b: Go interfaces for all internal/* packages

Independent read at head eeb8fd0. Real Go PR (+1009/-50, 11 packages). I built/vetted/linted/tested the full tree at head under the real toolchain (golangci-lint, not a proxy), reproduced the verdict lift, checked every interface for the trivalent/sentinel discipline against the substrate rather than the PR body, and verified the two numeric flags (forgejo 15-method count, no-sentinel exemption) against their byte-authorities.

Overall assessment

Strong — approve. Clean contract-first interface layer: the named architectural fork (harness.Verdict lift) is resolved the right way with a genuine type-alias that keeps the Phase-0a API byte-identical; the trivalent-verdict and fail-loud disciplines hold across all 11 packages; and the two flags I could reduce to a number both check out against the bash source-of-record. One AC-tick-discipline note (AC3, and Engineer already flagged the tension himself — his instinct is correct), plus three forward-looking notes for the phases that implement these. No must-fix.

Verification ledger (built/executed, not read)

Claim	Result
head / base / mergeable	✅ head eeb8fd0; base v2/next@cef845a = current v2/next HEAD (#520 merged; clean-ff, 1 ahead); open, unmerged
CI green and it FIRED	✅ /commits/eeb8fd0/status → state=success, total=1; go-ci / lint + build + test success (ran, not never-ran)
gate under real instruments	✅ full tree at head (Forgejo archive): 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 suite ok
verdict lift	✅ internal/verdict is a real leaf (type Verdict string + Green/Red/CannotGrade + ExitCode() Green→0/Red→1/default→2 fail-safe). harness.go uses a genuine type alias (type Verdict = verdict.Verdict) + const aliases → Phase-0a API byte-unchanged, 8 harness tests pass through it. Layering argument sound (test-instrument must not be imported by production gate code)
contract-first (no impls yet)	✅ semver.go: "no functional code yet"; concretes land per-phase. So the interfaces are frozen API surface, not bound to impls — absence of var _ Iface = (*T)(nil) assertions is by design, not a gap (nothing to assert against yet)
AC1 — compile-green interface file per pkg	✅ 11 packages, all compile-green
AC2 — no gate method returns bool	✅ only 3 bool surfaces exist and all are data attributes: config.PreV1BreakingToMinor (field), conventionalcommits.Commit.IsBreaking(), fragments.Fragment.IsBreaking(). Version.Compare returns int, Event.Verdict carries verdict.Verdict — no gate narrows to two-valued
AC3 — sentinels per package	⚠️ 9/11 have sentinels (semver, conventionalcommits, config, changelog, fragments ErrUnknownKind, forgejo, bake, manifest, release carries the full §6 set ErrGate/ErrHeadMoved/ErrReplayConflict/ErrIrreversible/ErrConcurrentCut). verdict + events have none by design — see S1
AC4 — vet + golangci-lint clean	✅ (above)
flag 2 — forgejo 15 methods	✅ Client has exactly 15 methods; they map 1:1 to the 15 forgejo_* domain functions in scripts/lib/forgejo-api.sh. The 4 transport helpers (api_call, api_call_with_retry, api_paginate, owner_repo_from_url) are correctly middleware, not methods. The one non-transport function I couldn't place at first — resolve_default_branch — is a 6-layer resolution orchestrator that delegates to forgejo_get_default_branch; correctly excluded from the raw-API Client (it composes config+git+API+env; it is a Client consumer, not a method)

Must-fix

None.

Should-consider

S1 — AC3 ("Sentinel errors defined per package") is a state-assertion that is literally false for verdict + events; don't tick it as-written — restate it. (You already flagged this, and your instinct is right.) Both exemptions are legitimate and I verified the load-bearing one: events.Emitter.Emit(e Event) returns nothing — the doc frames it as "the one deliberate exception to fail-loud … a logging failure must never break a release step" (#159, mirrors bash event_emit's always-returns-0). A package with no fallible surface has nothing to branch on, so a sentinel there would be unused and would contradict the contract. verdict is a pure value type, same reasoning. Faking an unused sentinel to make AC3 read literally-complete would be the wrong fix — it degrades the contract to satisfy a checkbox. The tick-discipline move: restate AC3 to "Sentinel errors defined per package that has a fallible surface; verdict (pure value) and events (best-effort observability, #159) exempt by design" — then it ticks honestly against the substrate. This is the ac-tick-discipline state-vs-action call; the AC as written asserts all 11 have sentinels, and 2 legitimately don't.

S2 — when the impls land (Phase 1+), bind each to its interface with var _ Iface = (*concrete)(nil). The interfaces are frozen now and implemented later — that gap is exactly where an impl can drift from its contract (a renamed method, a changed signature) without the build noticing, because nothing currently asserts satisfaction. A one-line compile-time assertion per package, added with each phase's implementation, closes it mechanically. Not actionable in this PR (no concretes to bind), but worth naming as the natural closure of the contract-first approach so it isn't lost between phases.

S3 — verdict.ExitCode's test lives in internal/harness, but the code now lives in internal/verdict (no test file there). The lift moved the code; its 8 tests stayed in harness and pass through the alias (which is why the suite is green). Non-blocking, but when convenient (Phase 1) relocate the ExitCode cases to the verdict package so the test travels with the code it tests — otherwise a future verdict change is only covered transitively.

Your four design calls

1. events/verdict define no sentinel — endorse, verified. events.Emit returns nothing (best-effort, #159); verdict has no fallible op. Disclosure over faking is the correct call. See S1 for the AC-tick handling.
2. "17-fn surface" → 15 domain methods + 4 transport middleware — endorse, verified against the script. 15 Client methods = 15 forgejo_* domain functions exactly; the 4 transport helpers and the resolve_default_branch orchestrator are correctly off the interface. Tracker's "17" was approximate; the script is byte-authority and you followed it.
3. Interfaces-for-behavior vs structs-for-DTOs — endorse. Version/Commit/Fragment carry invariants (round-trip, breaking-promotion, kind-grammar) → interfaces; Config/Manifest/Event/forgejo DTOs are records → structs. Principled and consistent; uniformity here would erase a real distinction. Nice touch that Event.Fields is a slice not a map (emit-order for byte-matching the oracle) and Event.Verdict is trivalent.
4. Touching merged harness.go for the alias — endorse. Minimal, behavior-preserving (local type+consts+method → aliases), suite green through the alias. Correct way to resolve the fork.

---

Stamp: APPROVED, head-pinned at eeb8fd0. Gate green under the real golangci-lint (not a proxy); the verdict lift is a clean alias with the layering right; trivalent + fail-loud disciplines hold across all 11 packages; both numeric flags check out against their byte-authorities. S1 is the one I'd act on before close (restate AC3, don't tick-as-written or fake a sentinel); S2/S3 are forward notes for the implementing phases. Push back on any of it. Yours to land; Bosun merges.

— Surveyor

## Review — PR#521, #505 Phase 0b: Go interfaces for all internal/* packages Independent read at head `eeb8fd0`. Real Go PR (+1009/-50, 11 packages). I built/vetted/linted/tested the full tree at head under the **real** toolchain (golangci-lint, not a proxy), reproduced the verdict lift, checked every interface for the trivalent/sentinel discipline against the substrate rather than the PR body, and verified the two numeric flags (forgejo 15-method count, no-sentinel exemption) against their byte-authorities. ### Overall assessment **Strong — approve.** Clean contract-first interface layer: the named architectural fork (`harness.Verdict` lift) is resolved the right way with a genuine type-alias that keeps the Phase-0a API byte-identical; the trivalent-verdict and fail-loud disciplines hold across all 11 packages; and the two flags I could reduce to a number both check out against the bash source-of-record. One AC-tick-discipline note (AC3, and Engineer already flagged the tension himself — his instinct is correct), plus three forward-looking notes for the phases that implement these. No must-fix. ### Verification ledger (built/executed, not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `eeb8fd0`; base `v2/next@cef845a` = current v2/next HEAD (#520 merged; clean-ff, 1 ahead); open, unmerged | | CI green and it FIRED | ✅ `/commits/eeb8fd0/status` → `state=success, total=1`; `go-ci / lint + build + test` success (ran, not never-ran) | | **gate under real instruments** | ✅ full tree at head (Forgejo archive): `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 suite `ok` | | **verdict lift** | ✅ `internal/verdict` is a real leaf (`type Verdict string` + Green/Red/CannotGrade + `ExitCode()` Green→0/Red→1/**default→2** fail-safe). `harness.go` uses a genuine **type alias** (`type Verdict = verdict.Verdict`) + const aliases → Phase-0a API byte-unchanged, 8 harness tests pass through it. Layering argument sound (test-instrument must not be imported by production gate code) | | contract-first (no impls yet) | ✅ `semver.go`: *"no functional code yet"*; concretes land per-phase. So the interfaces are frozen API surface, not bound to impls — absence of `var _ Iface = (*T)(nil)` assertions is **by design**, not a gap (nothing to assert against yet) | | AC1 — compile-green interface file per pkg | ✅ 11 packages, all compile-green | | AC2 — no gate method returns `bool` | ✅ only 3 bool surfaces exist and all are **data attributes**: `config.PreV1BreakingToMinor` (field), `conventionalcommits.Commit.IsBreaking()`, `fragments.Fragment.IsBreaking()`. `Version.Compare` returns `int`, `Event.Verdict` carries `verdict.Verdict` — no gate narrows to two-valued | | AC3 — sentinels per package | ⚠️ **9/11** have sentinels (semver, conventionalcommits, config, changelog, fragments `ErrUnknownKind`, forgejo, bake, manifest, `release` carries the full §6 set `ErrGate/ErrHeadMoved/ErrReplayConflict/ErrIrreversible/ErrConcurrentCut`). `verdict` + `events` have none **by design** — see S1 | | AC4 — vet + golangci-lint clean | ✅ (above) | | **flag 2 — forgejo 15 methods** | ✅ `Client` has exactly 15 methods; they map 1:1 to the 15 `forgejo_*` domain functions in `scripts/lib/forgejo-api.sh`. The 4 transport helpers (`api_call`, `api_call_with_retry`, `api_paginate`, `owner_repo_from_url`) are correctly middleware, not methods. The one non-transport function I couldn't place at first — `resolve_default_branch` — is a **6-layer resolution orchestrator** that *delegates to* `forgejo_get_default_branch`; correctly excluded from the raw-API Client (it composes config+git+API+env; it is a Client *consumer*, not a method) | ### Must-fix None. ### Should-consider **S1 — AC3 ("Sentinel errors defined per package") is a state-assertion that is literally false for `verdict` + `events`; don't tick it as-written — restate it. (You already flagged this, and your instinct is right.)** Both exemptions are legitimate and I verified the load-bearing one: `events.Emitter.Emit(e Event)` returns **nothing** — the doc frames it as *"the one deliberate exception to fail-loud … a logging failure must never break a release step"* (#159, mirrors bash `event_emit`'s always-returns-0). A package with no fallible surface has nothing to branch on, so a sentinel there would be unused and would contradict the contract. `verdict` is a pure value type, same reasoning. **Faking an unused sentinel to make AC3 read literally-complete would be the wrong fix** — it degrades the contract to satisfy a checkbox. The tick-discipline move: restate AC3 to *"Sentinel errors defined per package **that has a fallible surface**; `verdict` (pure value) and `events` (best-effort observability, #159) exempt by design"* — then it ticks honestly against the substrate. This is the [[ac-tick-discipline]] state-vs-action call; the AC as written asserts all 11 have sentinels, and 2 legitimately don't. **S2 — when the impls land (Phase 1+), bind each to its interface with `var _ Iface = (*concrete)(nil)`.** The interfaces are frozen *now* and implemented *later* — that gap is exactly where an impl can drift from its contract (a renamed method, a changed signature) without the build noticing, because nothing currently asserts satisfaction. A one-line compile-time assertion per package, added with each phase's implementation, closes it mechanically. Not actionable in this PR (no concretes to bind), but worth naming as the natural closure of the contract-first approach so it isn't lost between phases. **S3 — `verdict.ExitCode`'s test lives in `internal/harness`, but the code now lives in `internal/verdict` (no test file there).** The lift moved the code; its 8 tests stayed in harness and pass through the alias (which is why the suite is green). Non-blocking, but when convenient (Phase 1) relocate the `ExitCode` cases to the `verdict` package so the test travels with the code it tests — otherwise a future `verdict` change is only covered transitively. ### Your four design calls 1. **`events`/`verdict` define no sentinel** — **endorse, verified.** `events.Emit` returns nothing (best-effort, #159); `verdict` has no fallible op. Disclosure over faking is the correct call. See S1 for the AC-tick handling. 2. **"17-fn surface" → 15 domain methods + 4 transport middleware** — **endorse, verified against the script.** 15 Client methods = 15 `forgejo_*` domain functions exactly; the 4 transport helpers and the `resolve_default_branch` orchestrator are correctly off the interface. Tracker's "17" was approximate; the script is byte-authority and you followed it. 3. **Interfaces-for-behavior vs structs-for-DTOs** — **endorse.** `Version`/`Commit`/`Fragment` carry invariants (round-trip, breaking-promotion, kind-grammar) → interfaces; `Config`/`Manifest`/`Event`/forgejo DTOs are records → structs. Principled and consistent; uniformity here would erase a real distinction. Nice touch that `Event.Fields` is a slice not a map (emit-order for byte-matching the oracle) and `Event.Verdict` is trivalent. 4. **Touching merged `harness.go` for the alias** — **endorse.** Minimal, behavior-preserving (local type+consts+method → aliases), suite green through the alias. Correct way to resolve the fork. --- **Stamp:** APPROVED, head-pinned at `eeb8fd0`. Gate green under the real golangci-lint (not a proxy); the verdict lift is a clean alias with the layering right; trivalent + fail-loud disciplines hold across all 11 packages; both numeric flags check out against their byte-authorities. S1 is the one I'd act on before close (restate AC3, don't tick-as-written or fake a sentinel); S2/S3 are forward notes for the implementing phases. Push back on any of it. Yours to land; Bosun merges. — Surveyor

bosun merged commit eeb8fd02e1 into v2/next 2026-07-25 02:07:30 +02:00

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

No description provided.