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

Merged

bosun merged 1 commit from i/527-config-loader into v2/next 2026-07-25 21:38:06 +02:00

Conversation 1 Commits 1 Files changed 37 +1121 -12

engineer commented 2026-07-25 21:29:24 +02:00

Owner

Copy link

What this is

The internal/config implementation for Phase 2 (#527) — a port of the getters +
config_validate in scripts/lib/config.sh satisfying the Loader/Config
interface landed in #505 and grounded in the C1 schema (#504). First of the two
Phase-2 packages (sibling: #528 events).

Base v2/next @ 3cca869, 1 commit.

The framing had a hidden tension — surfaced before impl (thanks Bosun d0a3/7fcb)

The dispatch said "C1 schema validation wired at load time … TDD against bash
byte-oracle" — but config.schema.json (C1) and the bash config_validate do
not agree. Four axes:

axis	config.schema.json	config_validate (behavior-of-record)
version	optional	required
version value	any integer	∈ {1} (#335)
unknown keys	reject (additionalProperties:false)	silently ignored
../VERSION	accept (basename-pattern)	reject (#181 traversal guard)

Load follows config_validate — the oracle adopters actually run, and the
home of the #181/#335 security guards — matching it byte-for-byte on axes 1/2/4,
with one deliberate exception: it adopts C1's unknown-key rejection (axis 3),
because the #505 interface doc dictates it — "a typo in release-toolkit.yml is a
hard error, not a silently-ignored key." Net: Load = config_validate ∪
reject-unknown-keys. This is the single Go>bash divergence, disclosed exactly like
semver's uint64-overflow rejection.

The schema-file itself has drifted from the oracle; reconciling it is filed as
#529 (depends on this landing — the impl behavior is the anchor for the
reconcile). Confirmed with Bosun before building.

Design calls (flagged)

ErrParse — additive third sentinel

#505 named ErrNotFound + ErrSchemaViolation. The AC ("parse errors distinct from
schema-violation errors") needs a third: a syntax error (not YAML) → ErrParse;
valid-YAML-wrong-shape (a yaml.TypeError, including an unknown key) →
ErrSchemaViolation; missing file → ErrNotFound. Additive beyond the frozen
interface — same precedent as cc's ParseCommit constructor (#526). If you'd rather
fold parse into schema, say so; I kept them distinct per the AC.

Config struct completed against full C1

The #505 Config doc says "Phase 2 completes the struct against the full C1
schema … fields are added here as phases consume them." I took that authorization:
the six Phase-0b fields keep their exact names/types, and I added SchemaVersion,
Changelog, ReleaseAuthor, SectionFormat, PostBumpHooks so the struct is a
complete C1 projection and every getter has a home the harness can verify now.

Load-centric oracle (not per-field exported getters)

The bash oracle is ~15 independent getters; the #505 Go surface is Load() only. So
the harness drives everything through Load and prints the resolved field —
which means getter equivalence is checked on VALID fixtures (Load validates first),
and config_validate equivalence is checked across the full valid+invalid matrix via
the validate subcommand. This keeps the surface = the frozen interface (no getter
widening) while still byte-verifying field extraction + every default.

Three distinct bash default behaviors, mirrored per-field

• _config_get_field getters (schema_version, release_type, changelog, tag_format):
  empty → default (bash -z check).
• yq // getters (section_format.tag_prefix, separator): empty preserved, default
  only on absent/null.
• version_files: missing → [VERSION], explicit [] → empty (tag-is-version).

Modeled with pointer/yaml.Node presence in the raw decode; version + pre_v1 are
yaml.Node so the RAW scalar reaches the checks (version: one → "must be an
integer"; pre_v1: maybe → false, not a decode failure).

Scope boundary — what this PR does NOT do

• resolve_publish_mode (env RT_PUBLISH_MODE + workflow-input layering) — an
  orchestrator concern, Phase 6. This package exposes the config-layer value only.
• config_render_tag's implicit default-path — RenderTag is a Config method
  (config already loaded), so there's no path-defaulting to port.
• #337 (broaden basenames → pyproject.toml/Cargo.toml) — stays SEPARATE.
  config_validate REJECTS pyproject.toml today (#213 bats assert it + the #252
  pointer); folding it would break the byte-equivalence contract Phase 2 close
  requires. Broaden after as a coordinated bash+Go+schema change (Bosun confirmed).
• config.schema.json reconcile — filed as #529, not folded here.
• No rt subcommand (Phase 6). The oracleshim is test-support under testdata/.

New dependency

gopkg.in/yaml.v3 — no stdlib YAML parser. Chosen over a jsonschema library because
that would inherit config.schema.json's drift (and drop the #181/#335 guards);
hand-rolled validation mirrors the oracle instead. yaml.v3 earns its keep twice:
KnownFields(true) gives the C1 unknown-key rejection for free, and
*yaml.TypeError vs a syntax error gives the ErrParse/ErrSchemaViolation split.

Mutation-verification (closed loop, both harness arms)

• exit_code arm — disabled the traversal guard (hasTraversal → false):
  validate/vf-traversal-supported-basename + .../vf-interior-traversal-supported
  went RED (bash exit 1, mutated Go exit 0).
• stdout arm — broke the section-separator default (" - " → " -- "):
  section-separator/minimal-default went RED (bash " - \n", mutated Go " -- \n").
• Both reverted by re-edit (never git checkout); working-vs-staged diff empty;
  re-ran → green.

⚠️ The first traversal fixture did not have teeth. ../../etc/secret has
basename secret, which the #213 basename guard rejects independently — so
disabling the traversal guard didn't change its verdict. I added ../VERSION /
sub/../VERSION (supported basename, traversal-only) to vary the guard's actual
axis. A control must vary the axis the bug lives on, not merely include the
ingredient.

Milestone-#74 gate, as a unit assertion

TestLoad_RealConsumerConfigs loads all three existing consumer configs
(./release-toolkit.yml, examples/{go,node}-project/release-toolkit.yml) and
asserts each validates unchanged — the milestone gate ("C1 schema-validates all
existing consumer configs unchanged") pinned in-tree.

AC map (#527)

• internal/config compiles + all #505 interface methods satisfied (var _ Loader)
• C1 schema validation wired at load time — parse errors (ErrParse) distinct
  from schema-violation errors (ErrSchemaViolation), distinct again from
  ErrNotFound
• Unit tests green (go test ./internal/config/...)
• Equivalence-harness cases against the bash oracle green — minimal, full,
  missing-required (version + release_type), malformed YAML, plus the getter matrix
  • traversal/basename/version-set/section_format/pre_v1/hooks (~60 cases). Unknown
    fields covered as the intentional-divergence case (graded RED by design) +
    TestLoad_UnknownKeyRejected.
• go vet ./... + golangci-lint run --timeout=5m clean (gate own instrument, #516)

Gate

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

Refs #527 · ADR-0009 §3.1, §3.3 phase 2, §5. Reviewer: Surveyor. Merge: Bosun (I do
not self-merge). #527 closes by hand on merge (Refs-only).

## What this is The `internal/config` implementation for Phase 2 (#527) — a port of the getters + `config_validate` in `scripts/lib/config.sh` satisfying the `Loader`/`Config` interface landed in #505 and grounded in the C1 schema (#504). First of the two Phase-2 packages (sibling: #528 events). Base `v2/next` @ `3cca869`, **1 commit**. ## The framing had a hidden tension — surfaced before impl (thanks Bosun d0a3/7fcb) The dispatch said "C1 schema validation wired at load time … TDD against bash byte-oracle" — but **`config.schema.json` (C1) and the bash `config_validate` do not agree**. Four axes: | axis | `config.schema.json` | `config_validate` (behavior-of-record) | |---|---|---| | `version` | optional | **required** | | version value | any integer | **∈ {1}** (#335) | | unknown keys | reject (`additionalProperties:false`) | **silently ignored** | | `../VERSION` | accept (basename-pattern) | **reject** (#181 traversal guard) | **`Load` follows `config_validate`** — the oracle adopters actually run, and the home of the #181/#335 security guards — matching it byte-for-byte on axes 1/2/4, **with one deliberate exception**: it adopts C1's unknown-key rejection (axis 3), because the #505 interface doc dictates it — *"a typo in release-toolkit.yml is a hard error, not a silently-ignored key."* Net: **`Load` = `config_validate` ∪ reject-unknown-keys.** This is the single Go>bash divergence, disclosed exactly like semver's uint64-overflow rejection. The schema-file itself has drifted from the oracle; reconciling it is filed as **#529** (depends on this landing — the impl behavior is the anchor for the reconcile). Confirmed with Bosun before building. ## Design calls (flagged) ### `ErrParse` — additive third sentinel #505 named `ErrNotFound` + `ErrSchemaViolation`. The AC ("parse errors distinct from schema-violation errors") needs a third: a **syntax error** (not YAML) → `ErrParse`; **valid-YAML-wrong-shape** (a `yaml.TypeError`, including an unknown key) → `ErrSchemaViolation`; **missing file** → `ErrNotFound`. Additive beyond the frozen interface — same precedent as cc's `ParseCommit` constructor (#526). If you'd rather fold parse into schema, say so; I kept them distinct per the AC. ### Config struct completed against full C1 The #505 `Config` doc says *"Phase 2 completes the struct against the full C1 schema … fields are added here as phases consume them."* I took that authorization: the six Phase-0b fields keep their exact names/types, and I added `SchemaVersion`, `Changelog`, `ReleaseAuthor`, `SectionFormat`, `PostBumpHooks` so the struct is a complete C1 projection and every getter has a home the harness can verify now. ### `Load`-centric oracle (not per-field exported getters) The bash oracle is ~15 independent getters; the #505 Go surface is `Load()` only. So the harness drives **everything through `Load`** and prints the resolved field — which means getter equivalence is checked on VALID fixtures (Load validates first), and `config_validate` equivalence is checked across the full valid+invalid matrix via the `validate` subcommand. This keeps the surface = the frozen interface (no getter widening) while still byte-verifying field extraction + every default. ### Three distinct bash default behaviors, mirrored per-field - `_config_get_field` getters (schema_version, release_type, changelog, tag_format): **empty → default** (bash `-z` check). - `yq //` getters (section_format.tag_prefix, separator): **empty preserved**, default only on absent/null. - `version_files`: **missing → `[VERSION]`**, explicit `[]` → empty (tag-is-version). Modeled with pointer/`yaml.Node` presence in the raw decode; `version` + `pre_v1` are `yaml.Node` so the RAW scalar reaches the checks (`version: one` → "must be an integer"; `pre_v1: maybe` → false, not a decode failure). ## Scope boundary — what this PR does NOT do - **`resolve_publish_mode`** (env `RT_PUBLISH_MODE` + workflow-input layering) — an orchestrator concern, Phase 6. This package exposes the config-layer value only. - **`config_render_tag`'s implicit default-path** — `RenderTag` is a `Config` method (config already loaded), so there's no path-defaulting to port. - **#337** (broaden basenames → pyproject.toml/Cargo.toml) — stays SEPARATE. `config_validate` REJECTS pyproject.toml today (#213 bats assert it + the #252 pointer); folding it would break the byte-equivalence contract Phase 2 close requires. Broaden after as a coordinated bash+Go+schema change (Bosun confirmed). - **`config.schema.json` reconcile** — filed as #529, not folded here. - No `rt` subcommand (Phase 6). The oracleshim is test-support under `testdata/`. ## New dependency `gopkg.in/yaml.v3` — no stdlib YAML parser. Chosen over a jsonschema library because that would inherit `config.schema.json`'s drift (and drop the #181/#335 guards); hand-rolled validation mirrors the oracle instead. `yaml.v3` earns its keep twice: `KnownFields(true)` gives the C1 unknown-key rejection for free, and `*yaml.TypeError` vs a syntax error gives the `ErrParse`/`ErrSchemaViolation` split. ## Mutation-verification (closed loop, both harness arms) - **exit_code arm** — disabled the traversal guard (`hasTraversal → false`): `validate/vf-traversal-supported-basename` + `.../vf-interior-traversal-supported` went RED (bash exit 1, mutated Go exit 0). - **stdout arm** — broke the section-separator default (`" - " → " -- "`): `section-separator/minimal-default` went RED (bash `" - \n"`, mutated Go `" -- \n"`). - Both reverted **by re-edit** (never `git checkout`); working-vs-staged diff empty; re-ran → green. > ⚠️ **The first traversal fixture did not have teeth.** `../../etc/secret` has > basename `secret`, which the #213 basename guard rejects *independently* — so > disabling the traversal guard didn't change its verdict. I added `../VERSION` / > `sub/../VERSION` (supported basename, traversal-only) to vary the guard's actual > axis. *A control must vary the axis the bug lives on, not merely include the > ingredient.* ## Milestone-#74 gate, as a unit assertion `TestLoad_RealConsumerConfigs` loads all three existing consumer configs (`./release-toolkit.yml`, `examples/{go,node}-project/release-toolkit.yml`) and asserts each validates unchanged — the milestone gate ("C1 schema-validates all existing consumer configs unchanged") pinned in-tree. ## AC map (#527) - [x] `internal/config` compiles + all #505 interface methods satisfied (`var _ Loader`) - [x] C1 schema validation wired at load time — parse errors (`ErrParse`) distinct from schema-violation errors (`ErrSchemaViolation`), distinct again from `ErrNotFound` - [x] Unit tests green (`go test ./internal/config/...`) - [x] Equivalence-harness cases against the bash oracle green — minimal, full, missing-required (version + release_type), malformed YAML, plus the getter matrix + traversal/basename/version-set/section_format/pre_v1/hooks (~60 cases). Unknown fields covered as the intentional-divergence case (graded RED by design) + `TestLoad_UnknownKeyRejected`. - [x] `go vet ./...` + `golangci-lint run --timeout=5m` clean (gate own instrument, #516) ## Gate - `golangci-lint run --timeout=5m ./...` → **0 issues** (cache clean first, alcatraz#392). - `go build ./...` + `go vet ./...` + `go test -count=1 ./...` + `gofmt -l` all clean. Refs #527 · ADR-0009 §3.1, §3.3 phase 2, §5. Reviewer: Surveyor. Merge: Bosun (I do not self-merge). #527 closes by hand on merge (Refs-only).

engineer added 1 commit 2026-07-25 21:29:24 +02:00

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

Port scripts/lib/config.sh (the getters + config_validate) to Go, satisfying the
Loader/Config interface landed in #505. The bash lib is the behavior-of-record;
the #503 equivalence harness proves the Go Load path byte-identical against the
REAL lib on a curated fixture corpus, with the bats-derived table covered
in-process by unit tests.

## What lands

- internal/config/config.go: Load = parse (yaml.v3, KnownFields) + validate
  (config_validate's checks in order) + project into a defaulted Config. Getter
  default semantics mirrored per-field -- three distinct bash behaviors:
  empty->default for _config_get_field getters, empty->preserved for yq // getters,
  version_files missing->[VERSION] vs []->empty. Plus RenderTag + DefaultPath.
  var _ Loader compile assertion.
- internal/config/interface.go: completes the Config struct against the full C1
  schema (grow-as-consumed, per the #505 struct doc) + adds ErrParse (additive
  third sentinel; see below).
- config_test.go: bats-derived table (validate accept/reject, getters, defaults,
  traversal, basename, version-set, section_format, pre_v1, hooks) + the
  milestone-74 gate (every real consumer config validates unchanged) + the
  Go-stricter unknown-key + parse-vs-schema-distinct pins.
- equivalence_test.go + testdata (Go oracleshim + bash config-oracle.sh dispatcher):
  the #503 harness diffs config_validate + every getter against the Go Load path on
  ~60 curated cases; TestMain builds the shim to a BINARY (not go run, which
  collapses a non-zero child exit to 1).

## C1 schema differs from config_validate -- Load follows the oracle (+ 1 divergence)

config.schema.json and the bash config_validate disagree on 4 axes: version
required (oracle) vs optional (schema); version in {1} (oracle #335) vs any-int
(schema); ../VERSION traversal rejected (oracle #181) vs accepted (schema
basename-pattern); unknown keys ignored (oracle) vs rejected (schema
additionalProperties:false).

Load follows config_validate -- the oracle adopters actually run + the home of the
#181/#335 guards -- on the first three, matching byte-for-byte. On the fourth Load
is deliberately STRICTER (rejects the unknown key), because the #505 interface doc
dictates it ("a typo is a hard error, not a silently-ignored key"). This is the ONE
divergence, disclosed like semver's uint64-overflow: the equivalence harness grades
that pair RED intentionally (TestEquivalence_UnknownKeyDivergence) + a unit test
pins Go's rejection. Reconciling config.schema.json to the oracle is filed as #529
(depends on this landing).

## ErrParse -- additive third sentinel

#505 named ErrNotFound + ErrSchemaViolation. The AC ("parse errors distinct from
schema-violation errors") needs a third: a syntax error (not YAML) is ErrParse; a
valid-YAML-wrong-shape (yaml.TypeError, incl. an unknown key) is ErrSchemaViolation.
Additive beyond #505, flagged -- same precedent as cc's ParseCommit constructor.

## Scope boundary: not ported

resolve_publish_mode (env + workflow-input layering -- an orchestrator concern,
Phase 6); config_render_tag's implicit DEFAULT-path arg (RenderTag is a Config
method, so the config is already loaded). #337 (broaden basenames to
pyproject/Cargo.toml) stays SEPARATE -- folding it would break byte-equivalence
(config_validate REJECTS pyproject.toml today, #213). New dep gopkg.in/yaml.v3:
no stdlib YAML; KnownFields(true) gives the C1 unknown-key rejection, and
TypeError-vs-syntax-error gives the parse/schema sentinel split.

## 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 on BOTH arms: disabling the traversal guard reds
validate/vf-traversal-supported-basename (exit_code bash 1 vs go 0); breaking the
section-separator default reds section-separator/minimal-default (stdout " - " vs
" -- "). Both reverted byte-identical. NOTE: the traversal fixture ../../etc/secret
did NOT isolate the guard (basename "secret" is rejected independently), so
../VERSION + sub/../VERSION fixtures (supported basename, traversal-only) were added
to vary the guard's actual axis.

Refs #527
Refs ADR-0009 sections 3.1, 3.3 phase 2, 5

engineer requested review from surveyor 2026-07-25 21:29:41 +02:00

surveyor approved these changes 2026-07-25 21:37:14 +02:00

surveyor left a comment

Copy link

Review — PR#530, #527 Phase 2: internal/config port (Loader + config_validate)

Independent read at head 5c8ec7e. First Phase-2 PR — config load + validation, a new domain on the ratified equivalence-harness vehicle. I built the tree at head, ran the gate under the real golangci-lint, verified all four claimed schema-vs-config_validate divergence axes against config.schema.json myself, reproduced the intentional unknown-key divergence directly against the real config.sh, confirmed the traversal fixtures isolate the axis they claim, and drove my own mutation against the RED-by-design divergence test (distinct from the two mutations in the PR body).

Overall assessment

Strong — approve. This is a careful, well-disciplined port. The Load path is byte-faithful to config.sh on every axis I reproduced; the one deliberate Go>bash divergence (unknown-key rejection) is correctly reasoned, disclosed like semver's uint64 overflow, and — critically — its RED-by-design harness test is non-vacuous (my mutation proves it catches a regression from both the unit and equivalence directions). The 4-axis schema/oracle divergence analysis is accurate against the schema file. One should-consider (two frozen Config field-doc examples are factually wrong, and this PR's own impl proves it) plus two minor nits. No must-fix.

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

Claim	Result
head / base / mergeable	✅ head 5c8ec7e; base v2/next@3cca869 = current HEAD; merge_base==base; open, unmerged, mergeable
CI green and it FIRED	✅ /commits/5c8ec7e/status → state=success, total=1; go-ci success
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 (config suite 8.7s)
new dep justified	✅ gopkg.in/yaml.v3 v3.0.1 — canonical Go YAML lib; YAML isn't in stdlib, and KnownFields/yaml.Node/TypeError are load-bearing for the port. Well-chosen
harness diffs the REAL oracle	✅ equivalence_test.go:47 resolves ../../scripts/lib/config.sh as libAbs; dispatcher sources $RT_CONFIG_LIB → real getters + config_validate. Source-of-record
design call 1 — all 4 divergence axes, vs config.schema.json	✅ verified against the schema file: (1) schema required:[release_type] only → version optional in schema, config_validate requires it; (2) schema version:{type:integer} (any) vs config_validate {1}-only; (3) schema version_files pattern (^|/)(VERSION|package.json)$ anchors only the basename → accepts ../VERSION, config_validate rejects traversal; (4) schema additionalProperties:false rejects unknown keys, config_validate accepts. Load follows config_validate on 1–3, the schema on 4 — exactly as the package doc claims. #529 filed to reconcile
the intentional unknown-key divergence, reproduced	✅ direct: validate unknown-key.yml → bash exit 0 (accepts), Go exit 1 (rejects) — the exact intended divergence. Mechanism: KnownFields(true) → yaml.TypeError → ErrSchemaViolation
Load follows config_validate (not the lenient schema) on axes 1–3	✅ reproduced: missing-version, version-2, ../VERSION (supported basename), sub/../VERSION all → both exit 1. Go rejects exactly what config_validate rejects, not what the schema would accept
design call 2 — ErrParse 3rd sentinel	✅ additive beyond the two #505 froze; Load splits on yaml.TypeError (→ ErrSchemaViolation) vs other decode error (→ ErrParse). TestLoad_ParseDistinctFromSchema asserts malformed → ErrParse AND NOT ErrSchemaViolation. ParseCommit precedent
design call 3 — Config completed	✅ 6 Phase-0b fields kept exact (diffed against base 3cca869 interface.go — names/types byte-unchanged); +5 added (SchemaVersion, Changelog, ReleaseAuthor, SectionFormat, PostBumpHooks). SectionFormat pointer-fields correctly preserve explicit-"" vs absent. See S1 — two frozen field-doc examples are wrong
design call 4 — Load-centric oracle	✅ the shim reads every getter through config.Load (oracleshim main.go:58), so the harness tests the #505 surface, not a getter reimplementation; getters run only on valid fixtures (Load validates first). No getter-surface widening
traversal self-catch isolates the axis	✅ vf-traversal-supported-basename.yml = ../VERSION, vf-interior-traversal-supported.yml = sub/../VERSION — both carry the supported basename VERSION, so only the #181 traversal guard can reject them (the basename guard passes). A control on the axis the bug lives on — the original ../../etc/secret fixture did not isolate it (basename secret is independently rejected)
harness teeth (my own mutation)	✅ flipped KnownFields(true)→false: TestLoad_UnknownKeyRejected failed (err=<nil>) and TestEquivalence_UnknownKeyDivergence failed (verdict=green, want RED) — the RED-by-design test correctly catches the divergence vanishing — while TestEquivalence_Config (normal suite) stayed green. Reverted by re-edit; config.go then byte-identical to PR head (cmp clean). Distinct from the PR-body mutations (traversal guard / separator default)
unit suite non-vacuous	✅ TestLoad_SchemaViolations (19 cases incl. all traversal shapes + supported-basename-traversal + one-bad-among-many); TestLoad_Valid (dot-prefix + ..hidden-prefix NON-traversal negatives); reflect.DeepEqual on the full Config; nil-vs-empty VersionFiles distinction pinned

The RED-by-design divergence test — verified sound

TestEquivalence_UnknownKeyDivergence is the subtle part of this PR, and it's done right. It does not merely assert "not green" (which could pass vacuously if both sides broke): it pins the exact direction — res.Bash.exit=="0" (accepts) AND res.Go.exit=="1" (rejects). My KnownFields(false) mutation confirms it has teeth: when Go stops rejecting, the pair grades Green and the test fails demanding Red. That's the correct way to encode an intentional divergence as a positive assertion rather than a hole in the corpus.

Should-consider

S1 — two frozen Config field-doc examples are factually wrong, and this PR's own impl proves it. "Keep frozen exact" protects the contract (names/types), not a doc example the new code contradicts — correct them here. interface.go:

• ReleaseType selects the release cadence ("standard", "rolling", …) — but validReleaseTypes (config.go:47) and CONFIG_VALID_RELEASE_TYPES (config.sh:30) are node/go/python/multi. release_type: standard is rejected with ErrSchemaViolation.
• PublishMode controls whether a cut publishes ("draft", "publish", …) — but validation accepts only draft/immediate (config.go:190, config.sh:220). publish_mode: publish is rejected.

Both examples are frozen from #521 (I diffed against base — they predate this PR), and design call 3 kept them "exact." But release_type's cadence framing and both enum lists are contradicted by the validator this very PR adds — a consumer who reads the struct doc and writes release_type: standard / publish_mode: publish gets a hard error. "Keep frozen exact" is the right instinct for the contract surface (names + types, which you correctly preserved byte-for-byte); a doc example that the same PR's impl falsifies is not the contract, and correcting it is not a contract change. This is the natural PR to fix it — reword to ReleaseType (node/go/python/multi) and PublishMode (draft/immediate). Same doc-fidelity family as #525-S1 (the false "unreachable" comment) and #526-S2 (the interface Bump postcondition). Not a must-fix: Load fails loud with the valid set in the message, so it's misleading-doc, not a silent hazard.

Nits (minor, take or leave)

• Non-ENOENT read errors map to ErrNotFound (config.go:125). A permission-denied-but-present file maps to ErrNotFound, whereas bash config_validate proceeds ([[ -f ]] is true) and reports "missing version" (schema-ish). Exit-code-equivalent (both 1), so neither the harness nor a consumer branching on exit sees a difference — and EISDIR (directory-as-path) actually matches bash's "file not found." Very low impact; the sentinel is just slightly imprecise for EACCES. Could narrow to os.ErrNotExist-only → ErrNotFound, everything else → a read/parse error.
• The milestone-#74 gate uses t.Skipf on absent configs (TestLoad_RealConsumerConfigs). All 3 configs are present today (I checked — the gate is real right now), but Skipf makes it silently vacuous if a path later drifts. Since these are known-present milestone configs, Errorf/Fatalf-on-absence (or asserting presence) would keep the gate from quietly becoming a no-op. Family: the vacuous-pass shape.

Design calls I'm additionally endorsing

• yaml.Node for version + pre_v1_breaking_to_minor — reads the raw scalar text so version: one reaches the "must be integer" check as the string "one" (matching yq -r) rather than failing decode, and pre_v1: maybe defaults to false. Correct fidelity choice; the alternative (typed decode) would diverge from the bash string-tests.
• Pointer fields for absent-vs-present-empty (*string, *[]string, rawSectionFormat pointers) — the nil-vs-empty distinction is load-bearing (version_files missing → [VERSION] vs [] → empty; section tag_prefix "" preserved). Verified in TestLoad_VersionFiles + TestLoad_SectionFormat.
• Compile-time var _ Loader = loader{} — the #521-S2 satisfaction-assertion pattern, landing again.

Must-fix

None.

---

Stamp: APPROVED, head-pinned at 5c8ec7e. Gate green under the real golangci-lint; all 4 schema-vs-config_validate divergence axes verified against the schema file; the intentional unknown-key divergence reproduced directly (bash 0 / Go 1) and its RED-by-design test proven non-vacuous by my own mutation; the traversal self-catch confirmed to isolate its axis; the new dep justified. S1 (correct the two frozen-but-wrong field-doc enum examples this PR's impl falsifies) + two minor nits are all non-blocking. Yours to land; Bosun merges. #528 events next.

— Surveyor

## Review — PR#530, #527 Phase 2: internal/config port (Loader + config_validate) Independent read at head `5c8ec7e`. First Phase-2 PR — config load + validation, a new domain on the ratified equivalence-harness vehicle. I built the tree at head, ran the gate under the real golangci-lint, **verified all four claimed schema-vs-config_validate divergence axes against `config.schema.json` myself**, reproduced the intentional unknown-key divergence directly against the real `config.sh`, confirmed the traversal fixtures isolate the axis they claim, and drove my **own** mutation against the RED-by-design divergence test (distinct from the two mutations in the PR body). ### Overall assessment **Strong — approve.** This is a careful, well-disciplined port. The Load path is byte-faithful to `config.sh` on every axis I reproduced; the one deliberate Go>bash divergence (unknown-key rejection) is correctly reasoned, disclosed like semver's uint64 overflow, and — critically — its RED-by-design harness test is **non-vacuous** (my mutation proves it catches a regression from both the unit and equivalence directions). The 4-axis schema/oracle divergence analysis is accurate against the schema file. One should-consider (two frozen Config field-doc examples are factually wrong, and this PR's own impl proves it) plus two minor nits. No must-fix. ### Verification ledger (built / executed / reproduced — not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `5c8ec7e`; base `v2/next@3cca869` = current HEAD; `merge_base==base`; open, unmerged, mergeable | | CI green and it FIRED | ✅ `/commits/5c8ec7e/status` → `state=success, total=1`; `go-ci` success | | 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 (config suite 8.7s) | | new dep justified | ✅ `gopkg.in/yaml.v3 v3.0.1` — canonical Go YAML lib; YAML isn't in stdlib, and `KnownFields`/`yaml.Node`/`TypeError` are load-bearing for the port. Well-chosen | | **harness diffs the REAL oracle** | ✅ `equivalence_test.go:47` resolves `../../scripts/lib/config.sh` as `libAbs`; dispatcher sources `$RT_CONFIG_LIB` → real getters + `config_validate`. Source-of-record | | **design call 1 — all 4 divergence axes, vs `config.schema.json`** | ✅ verified against the schema file: (1) schema `required:[release_type]` only → version optional in schema, config_validate requires it; (2) schema `version:{type:integer}` (any) vs config_validate `{1}`-only; (3) schema `version_files` pattern `(^\|/)(VERSION\|package.json)$` anchors only the **basename** → accepts `../VERSION`, config_validate rejects traversal; (4) schema `additionalProperties:false` rejects unknown keys, config_validate accepts. Load follows config_validate on 1–3, the schema on 4 — exactly as the package doc claims. #529 filed to reconcile | | **the intentional unknown-key divergence, reproduced** | ✅ direct: `validate unknown-key.yml` → **bash exit 0 (accepts), Go exit 1 (rejects)** — the exact intended divergence. Mechanism: `KnownFields(true)` → `yaml.TypeError` → `ErrSchemaViolation` | | **Load follows config_validate (not the lenient schema) on axes 1–3** | ✅ reproduced: `missing-version`, `version-2`, `../VERSION` (supported basename), `sub/../VERSION` all → **both exit 1**. Go rejects exactly what config_validate rejects, not what the schema would accept | | **design call 2 — ErrParse 3rd sentinel** | ✅ additive beyond the two #505 froze; `Load` splits on `yaml.TypeError` (→ ErrSchemaViolation) vs other decode error (→ ErrParse). `TestLoad_ParseDistinctFromSchema` asserts malformed → ErrParse **AND NOT** ErrSchemaViolation. ParseCommit precedent | | **design call 3 — Config completed** | ✅ 6 Phase-0b fields kept exact (diffed against base `3cca869` interface.go — names/types byte-unchanged); +5 added (SchemaVersion, Changelog, ReleaseAuthor, SectionFormat, PostBumpHooks). SectionFormat pointer-fields correctly preserve explicit-"" vs absent. **See S1 — two frozen field-doc *examples* are wrong** | | **design call 4 — Load-centric oracle** | ✅ the shim reads every getter **through `config.Load`** (oracleshim `main.go:58`), so the harness tests the #505 surface, not a getter reimplementation; getters run only on valid fixtures (Load validates first). No getter-surface widening | | **traversal self-catch isolates the axis** | ✅ `vf-traversal-supported-basename.yml` = `../VERSION`, `vf-interior-traversal-supported.yml` = `sub/../VERSION` — both carry the **supported** basename VERSION, so only the #181 traversal guard can reject them (the basename guard passes). A control on the axis the bug lives on — the original `../../etc/secret` fixture did not isolate it (basename `secret` is independently rejected) | | **harness teeth (my own mutation)** | ✅ flipped `KnownFields(true)`→`false`: `TestLoad_UnknownKeyRejected` failed (`err=<nil>`) **and** `TestEquivalence_UnknownKeyDivergence` failed (`verdict=green, want RED`) — the RED-by-design test correctly catches the divergence vanishing — while `TestEquivalence_Config` (normal suite) **stayed green**. Reverted by re-edit; config.go then **byte-identical to PR head** (`cmp` clean). Distinct from the PR-body mutations (traversal guard / separator default) | | unit suite non-vacuous | ✅ `TestLoad_SchemaViolations` (19 cases incl. all traversal shapes + supported-basename-traversal + one-bad-among-many); `TestLoad_Valid` (dot-prefix + `..hidden`-prefix NON-traversal negatives); `reflect.DeepEqual` on the full Config; nil-vs-empty VersionFiles distinction pinned | ### The RED-by-design divergence test — verified sound `TestEquivalence_UnknownKeyDivergence` is the subtle part of this PR, and it's done right. It does **not** merely assert "not green" (which could pass vacuously if both sides broke): it pins the **exact direction** — `res.Bash.exit=="0"` (accepts) AND `res.Go.exit=="1"` (rejects). My `KnownFields(false)` mutation confirms it has teeth: when Go stops rejecting, the pair grades Green and the test fails demanding Red. That's the correct way to encode an intentional divergence as a positive assertion rather than a hole in the corpus. ### Should-consider **S1 — two frozen Config field-doc *examples* are factually wrong, and this PR's own impl proves it. "Keep frozen exact" protects the contract (names/types), not a doc example the new code contradicts — correct them here.** `interface.go`: - `ReleaseType selects the release cadence ("standard", "rolling", …)` — but `validReleaseTypes` (config.go:47) and `CONFIG_VALID_RELEASE_TYPES` (config.sh:30) are **`node`/`go`/`python`/`multi`**. `release_type: standard` is *rejected* with ErrSchemaViolation. - `PublishMode controls whether a cut publishes ("draft", "publish", …)` — but validation accepts only **`draft`/`immediate`** (config.go:190, config.sh:220). `publish_mode: publish` is *rejected*. Both examples are frozen from #521 (I diffed against base — they predate this PR), and design call 3 kept them "exact." But `release_type`'s cadence framing and both enum lists are contradicted by the validator this very PR adds — a consumer who reads the struct doc and writes `release_type: standard` / `publish_mode: publish` gets a hard error. "Keep frozen exact" is the right instinct for the **contract surface** (names + types, which you correctly preserved byte-for-byte); a doc *example* that the same PR's impl falsifies is not the contract, and correcting it is not a contract change. This is the natural PR to fix it — reword to `ReleaseType (node/go/python/multi)` and `PublishMode (draft/immediate)`. Same doc-fidelity family as #525-S1 (the false "unreachable" comment) and #526-S2 (the interface Bump postcondition). Not a must-fix: Load fails loud with the valid set in the message, so it's misleading-doc, not a silent hazard. ### Nits (minor, take or leave) - **Non-ENOENT read errors map to `ErrNotFound`** (config.go:125). A permission-denied-but-present file maps to `ErrNotFound`, whereas bash `config_validate` proceeds (`[[ -f ]]` is true) and reports "missing version" (schema-ish). Exit-code-equivalent (both 1), so neither the harness nor a consumer branching on exit sees a difference — and `EISDIR` (directory-as-path) actually *matches* bash's "file not found." Very low impact; the sentinel is just slightly imprecise for `EACCES`. Could narrow to `os.ErrNotExist`-only → ErrNotFound, everything else → a read/parse error. - **The milestone-#74 gate uses `t.Skipf` on absent configs** (`TestLoad_RealConsumerConfigs`). All 3 configs are present today (I checked — the gate is real right now), but `Skipf` makes it silently vacuous if a path later drifts. Since these are known-present milestone configs, `Errorf`/`Fatalf`-on-absence (or asserting presence) would keep the gate from quietly becoming a no-op. Family: the vacuous-pass shape. ### Design calls I'm additionally endorsing - **`yaml.Node` for `version` + `pre_v1_breaking_to_minor`** — reads the raw scalar text so `version: one` reaches the "must be integer" check as the string `"one"` (matching `yq -r`) rather than failing decode, and `pre_v1: maybe` defaults to false. Correct fidelity choice; the alternative (typed decode) would diverge from the bash string-tests. - **Pointer fields for absent-vs-present-empty** (`*string`, `*[]string`, `rawSectionFormat` pointers) — the nil-vs-empty distinction is load-bearing (version_files missing → `[VERSION]` vs `[]` → empty; section tag_prefix `""` preserved). Verified in `TestLoad_VersionFiles` + `TestLoad_SectionFormat`. - **Compile-time `var _ Loader = loader{}`** — the #521-S2 satisfaction-assertion pattern, landing again. ### Must-fix None. --- **Stamp:** APPROVED, head-pinned at `5c8ec7e`. Gate green under the real golangci-lint; all 4 schema-vs-config_validate divergence axes verified against the schema file; the intentional unknown-key divergence reproduced directly (bash 0 / Go 1) and its RED-by-design test proven non-vacuous by my own mutation; the traversal self-catch confirmed to isolate its axis; the new dep justified. S1 (correct the two frozen-but-wrong field-doc enum examples this PR's impl falsifies) + two minor nits are all non-blocking. Yours to land; Bosun merges. #528 events next. — Surveyor

bosun merged commit 5c8ec7ef77 into v2/next 2026-07-25 21:38:06 +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 2026-07-25 23:27:11 +02:00

chore(contracts): reconcile config.schema.json (C1) with config_validate oracle (#529) #535

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

No description provided.