feat(manifest): implement Store — Read/Write/CheckDesync + round-trip (#542) #543

Merged

bosun merged 1 commit from i/542-manifest-store into v2/next 2026-07-26 10:26:00 +02:00

Conversation 2 Commits 1 Files changed 12 +935 -2

engineer commented 2026-07-26 10:07:36 +02:00

Owner

Copy link

What

First of Phase 4's two package ports (sibling: #541 forgejo client). Implements internal/manifest.Store against the frozen #505 interface + the C2 schema (manifest.schema.json), via contract-driven TDD-per-phase (ADR-0009 §3.1). Base is v2/next.

Three methods:

• Write(path, m) — canonical serialization, byte-compatible with the bash jq -n writer (reusable-release.yml:429-433): C2 key order (schema, last_released_sha, last_released_version, last_released_tag, last_released_at), integer schema: 1, 2-space indent, trailing newline, no HTML-escaping. Atomic (path+".tmp" + rename), mirroring the bash writer.
• Read(path) — strict decode (DisallowUnknownFields = C2 additionalProperties:false) + C2 validation (required fields, schema const 1, 40-hex last_released_sha pattern). Round-trip read(write(m)) == m holds for every C2-valid record (property-invariants.md §2), asserted by TestManifestRoundTrip.
• CheckDesync(m) — the git-state sub-checks of manifest-check.sh §5 (#56): 5a SHA exists, 5b SHA is an ancestor of HEAD, 5c the recorded tag resolves and points at the SHA. ErrDesync fail-loud with the specific disagreement.

Design decisions (decision-tree, not conclusion)

1. Canonical serialization by struct-field-order, not encoding/json's default nor jq. The C2 key order differs from both the Go Manifest struct field order (frozen #505: Schema, Version, Tag, SHA, At) and encoding/json's map-sorted order. So Write marshals through an internal wireManifest whose field order IS the canonical key order, with a json.Encoder configured SetEscapeHTML(false) + SetIndent("", " "). Y would be right instead if: the writer emitted sorted keys (then a map would do), or if Go struct order happened to match canonical (it doesn't), or if a byte-exact match weren't required (it is — the round-trip contract is byte-canonical so a cross-substrate diff shows only intended field changes).

2. schema is modelled as a string but serialized as an integer. C2 says schema is const 1 (integer); the frozen #505 Manifest.Schema is a string. Write maps the canonical decimal string "1" → integer 1; Read maps back. Validation requires exactly "1" (write) / 1 (read) — the only value C2 permits today. A future schema bump revisits the const (named in-code).

3. Lenient-vs-strict is inverted here vs the gates. The Phase-3 gates needed lenient extraction (match bash grep/awk, never crash). The manifest Store is the opposite: C2 is a fail-loud schema contract (ADR-0009 §5), so Read is strict — an unknown key or missing required field is a hard ErrSchemaViolation, not a tolerated zero value.

4. Sentinel reconciliation (restate-before-tick). The frozen #505 interface names three sentinels (ErrNotFound, ErrSchemaViolation, ErrDesync); the #542 AC names four error kinds (adds parse-error + atomic-write-failure). I add ErrParse (JSON-syntax) and ErrAtomicWrite (write-transaction). ErrParse joins ErrSchemaViolation — a parse failure satisfies errors.Is for both — so the frozen contract ("Read returns ErrSchemaViolation if malformed") stays literally true while a caller that wants the finer distinction can test ErrParse. TestRead_ParseImpliesSchema locks this invariant.

Disclosed boundaries

• Go is stricter than bash on schema, by contract, with no bash byte-oracle. There is no JSON-schema validator anywhere in the bash toolkit (no jsonschema/ajv/check-jsonschema; validate-grammars.sh covers C4/C6 only; release-decide.sh + manifest-check.sh read the manifest via lenient jq -r '.field // empty' + jq empty). So additionalProperties:false, const 1, the SHA pattern, and required-completeness beyond manifest-check.sh §5's ad-hoc SHA-presence check are Go strictness per the C2 contract + §5 fail-loud, not bash-equivalence. The equivalence harness greens only where bash has a behaviour (well-formedness verdict, field extraction, canonical write bytes). TestManifestSchemaStricterThanBash asserts the split (bash lenient-accepts exit 0, Go strict-rejects exit 2) so the disclosure is a standing test, not prose alone. This is the sanctioned "disclose honestly if bash surface is scattered" path from the AC.
• CheckDesync ports §5's 5a/5b/5c, not 5d. manifest-check.sh §5d cross-checks last_released_tag == config_render_tag(last_released_version) — a config-render consistency check, not a git-state check. The frozen CheckDesync(m Manifest) signature carries no config handle, and assuming the default v{version} format would false-desync a consumer using a custom tag_format. So 5d belongs to the config-aware caller (Phase 6 wiring), not this method. The three git-state checks are the load-bearing #56 core the interface comment names.
• The jq -n writer is transcribed verbatim into the oracle, not called. The byte-authority writer is inline YAML in reusable-release.yml (not a standalone script), so manifest-oracle.sh transcribes the exact jq -n expression (lines 429-433) with an in-file coupling note. If that writer changes, the oracle must change with it.
• Atomic-write is path+".tmp" + rename (bash-faithful), not flock-serialized. Matches the bash writer; concurrency serialization is #499's fail-atomic remit.

Verification (closed loop)

• Round-trip property — TestManifestRoundTrip over a field matrix incl. empty non-SHA fields + values with quotes/spaces.
• Equivalence harness (#503 vehicle — oracleshim + bash dispatcher, prebuilt binary not go run): Write byte-equivalence vs the real jq -n writer (3 cases); Read verdict vs bash jq empty/jq -r (valid + malformed). The bash side empirically confirms jq's 2-space canonical form.
• CheckDesync — 6 scratch-git-repo unit cases mirroring manifest-check.bats §5 (ok / ancestor-no-tag / sha-absent / sha-not-ancestor / tag-absent / tag-points-elsewhere).
• Harness teeth mutation-verified on two axes, each reverted by re-edit byte-identical:
  • Write indent 2 -> 4 spaces → TestEquivalence_ManifestWrite RED (bash 2-space vs Go 4-space stdout).
  • drop DisallowUnknownFields → TestManifestSchemaStricterThanBash/unknown-field RED (Go exit 0 vs required 2).

⚠️ Mutation-verify requires go test -count=1 — the oracleshim is runtime-built (rebuilt in TestMain), invisible to go test's cache; a cached GREEN masks a mutation.

Gate

golangci-lint run --timeout=5m (cache clean first, #392) → 0 issues · go build ./... · go vet ./... · go test -count=1 ./... green · gofmt -l clean · shellcheck clean on manifest-oracle.sh.

What this does NOT do

• Does not implement the forgejo client — that is #541 (sibling Phase-4 package), the sequential continuation after this lands.
• Does not wire Store into internal/release.Cutter — Phase 6 (#508) consumes Store; #499's fail-atomic transactional cut lands its concurrency-serialized atomic write against this Store.
• Does not port manifest-check.sh §5d (config-render tag consistency) or §1-4 (config/version_files/CHANGELOG checks) — those belong to the config/changelog packages + the config-aware Phase-6 caller (disclosed above).
• Does not add a JSON-schema dependency — C2's constraints (required, const, pattern, additionalProperties) are enforced with stdlib encoding/json (DisallowUnknownFields) + a small hand validator; no schema-validation library is warranted for a five-field fixed schema.

---

Refs #542 · reviewer @surveyor · merge @bosun (no self-merge). On merge, tick the #542 ACs + close the tracker by hand (Refs-only, consistent with the Phase-3 pattern).

## What First of Phase 4's two package ports (sibling: #541 forgejo client). Implements `internal/manifest.Store` against the frozen #505 interface + the C2 schema (`manifest.schema.json`), via contract-driven TDD-per-phase (ADR-0009 §3.1). Base is `v2/next`. Three methods: - **`Write(path, m)`** — canonical serialization, **byte-compatible with the bash `jq -n` writer** (`reusable-release.yml:429-433`): C2 key order (`schema, last_released_sha, last_released_version, last_released_tag, last_released_at`), integer `schema: 1`, 2-space indent, trailing newline, no HTML-escaping. Atomic (`path+".tmp"` + `rename`), mirroring the bash writer. - **`Read(path)`** — strict decode (`DisallowUnknownFields` = C2 `additionalProperties:false`) + C2 validation (required fields, `schema` const 1, 40-hex `last_released_sha` pattern). Round-trip `read(write(m)) == m` holds for every C2-valid record (property-invariants.md §2), asserted by `TestManifestRoundTrip`. - **`CheckDesync(m)`** — the git-state sub-checks of `manifest-check.sh` §5 (#56): 5a SHA exists, 5b SHA is an ancestor of HEAD, 5c the recorded tag resolves and points at the SHA. `ErrDesync` fail-loud with the specific disagreement. ## Design decisions (decision-tree, not conclusion) **1. Canonical serialization by struct-field-order, not `encoding/json`'s default nor `jq`.** The C2 key order differs from both the Go `Manifest` struct field order (frozen #505: Schema, Version, Tag, SHA, At) and `encoding/json`'s map-sorted order. So `Write` marshals through an internal `wireManifest` whose **field order IS the canonical key order**, with a `json.Encoder` configured `SetEscapeHTML(false)` + `SetIndent("", " ")`. Y would be right instead if: the writer emitted sorted keys (then a `map` would do), or if Go struct order happened to match canonical (it doesn't), or if a byte-exact match weren't required (it is — the round-trip contract is byte-canonical so a cross-substrate diff shows only intended field changes). **2. `schema` is modelled as a string but serialized as an integer.** C2 says `schema` is `const 1` (**integer**); the frozen #505 `Manifest.Schema` is a **string**. `Write` maps the canonical decimal string `"1"` → integer `1`; `Read` maps back. Validation requires exactly `"1"` (write) / `1` (read) — the only value C2 permits today. A future schema bump revisits the const (named in-code). **3. Lenient-vs-strict is inverted here vs the gates.** The Phase-3 gates needed *lenient* extraction (match bash grep/awk, never crash). The manifest Store is the opposite: C2 is a **fail-loud schema contract** (ADR-0009 §5), so `Read` is *strict* — an unknown key or missing required field is a hard `ErrSchemaViolation`, not a tolerated zero value. **4. Sentinel reconciliation (restate-before-tick).** The frozen #505 interface names three sentinels (`ErrNotFound`, `ErrSchemaViolation`, `ErrDesync`); the #542 AC names **four error kinds** (adds parse-error + atomic-write-failure). I add `ErrParse` (JSON-syntax) and `ErrAtomicWrite` (write-transaction). **`ErrParse` joins `ErrSchemaViolation`** — a parse failure satisfies `errors.Is` for *both* — so the frozen contract ("`Read` returns `ErrSchemaViolation` if malformed") stays literally true while a caller that wants the finer distinction can test `ErrParse`. `TestRead_ParseImpliesSchema` locks this invariant. ## Disclosed boundaries - **Go is stricter than bash on schema, by contract, with no bash byte-oracle.** There is **no JSON-schema validator anywhere in the bash toolkit** (no `jsonschema`/`ajv`/`check-jsonschema`; `validate-grammars.sh` covers C4/C6 only; `release-decide.sh` + `manifest-check.sh` read the manifest via lenient `jq -r '.field // empty'` + `jq empty`). So `additionalProperties:false`, `const 1`, the SHA pattern, and required-completeness beyond `manifest-check.sh` §5's ad-hoc SHA-presence check are **Go strictness per the C2 contract + §5 fail-loud, not bash-equivalence**. The equivalence harness greens only where bash *has* a behaviour (well-formedness verdict, field extraction, canonical write bytes). `TestManifestSchemaStricterThanBash` asserts the split (bash lenient-accepts exit 0, Go strict-rejects exit 2) so the disclosure is a standing test, not prose alone. This is the sanctioned "disclose honestly if bash surface is scattered" path from the AC. - **`CheckDesync` ports §5's 5a/5b/5c, not 5d.** `manifest-check.sh` §5d cross-checks `last_released_tag == config_render_tag(last_released_version)` — a **config-render** consistency check, not a git-state check. The frozen `CheckDesync(m Manifest)` signature carries **no config handle**, and assuming the default `v{version}` format would false-desync a consumer using a custom `tag_format`. So 5d belongs to the config-aware caller (Phase 6 wiring), not this method. The three git-state checks are the load-bearing #56 core the interface comment names. - **The `jq -n` writer is transcribed verbatim into the oracle, not called.** The byte-authority writer is inline YAML in `reusable-release.yml` (not a standalone script), so `manifest-oracle.sh` transcribes the exact `jq -n` expression (lines 429-433) with an in-file coupling note. If that writer changes, the oracle must change with it. - **Atomic-write is `path+".tmp"` + rename (bash-faithful), not `flock`-serialized.** Matches the bash writer; concurrency serialization is #499's fail-atomic remit. ## Verification (closed loop) - **Round-trip property** — `TestManifestRoundTrip` over a field matrix incl. empty non-SHA fields + values with quotes/spaces. - **Equivalence harness** (#503 vehicle — oracleshim + bash dispatcher, **prebuilt binary** not `go run`): `Write` byte-equivalence vs the real `jq -n` writer (3 cases); `Read` verdict vs bash `jq empty`/`jq -r` (valid + malformed). The bash side empirically confirms jq's 2-space canonical form. - **CheckDesync** — 6 scratch-git-repo unit cases mirroring `manifest-check.bats` §5 (ok / ancestor-no-tag / sha-absent / sha-not-ancestor / tag-absent / tag-points-elsewhere). - **Harness teeth mutation-verified** on two axes, each reverted by re-edit byte-identical: - `Write` indent `2 -> 4` spaces → `TestEquivalence_ManifestWrite` **RED** (bash 2-space vs Go 4-space stdout). - drop `DisallowUnknownFields` → `TestManifestSchemaStricterThanBash/unknown-field` **RED** (Go exit 0 vs required 2). ⚠️ **Mutation-verify requires `go test -count=1`** — the `oracleshim` is runtime-built (rebuilt in `TestMain`), invisible to `go test`'s cache; a cached GREEN masks a mutation. ## Gate `golangci-lint run --timeout=5m` (cache clean first, #392) → **0 issues** · `go build ./...` · `go vet ./...` · `go test -count=1 ./...` green · `gofmt -l` clean · `shellcheck` clean on `manifest-oracle.sh`. ## What this does NOT do - **Does not implement the forgejo client** — that is #541 (sibling Phase-4 package), the sequential continuation after this lands. - **Does not wire `Store` into `internal/release.Cutter`** — Phase 6 (#508) consumes `Store`; #499's fail-atomic transactional cut lands its concurrency-serialized atomic write against this `Store`. - **Does not port `manifest-check.sh` §5d** (config-render tag consistency) or §1-4 (config/version_files/CHANGELOG checks) — those belong to the config/changelog packages + the config-aware Phase-6 caller (disclosed above). - **Does not add a JSON-schema dependency** — C2's constraints (required, const, pattern, additionalProperties) are enforced with stdlib `encoding/json` (`DisallowUnknownFields`) + a small hand validator; no schema-validation library is warranted for a five-field fixed schema. --- Refs #542 · reviewer @surveyor · merge @bosun (no self-merge). On merge, tick the #542 ACs + close the tracker by hand (Refs-only, consistent with the Phase-3 pattern).

engineer added 1 commit 2026-07-26 10:07:36 +02:00

feat(manifest): implement Store — Read/Write/CheckDesync + round-trip (#542) ... c3268f7db3

First of Phase 4's two package ports. Implements internal/manifest.Store
against the frozen #505 interface + the C2 schema (manifest.schema.json),
landed via contract-driven TDD-per-phase (ADR-0009 §3.1).

Store.Write serializes canonically — byte-compatible with the bash `jq -n`
writer (reusable-release.yml:429-433): the C2 key order (schema,
last_released_sha, last_released_version, last_released_tag, last_released_at),
the integer `schema: 1`, 2-space indent, trailing newline, no HTML-escaping.
The write is atomic (path+".tmp" + rename), mirroring the bash writer;
concurrency serialization is #499's fail-atomic remit.

Store.Read decodes strictly (DisallowUnknownFields = C2 additionalProperties:
false) and validates against C2 (required fields, schema const 1, 40-hex
last_released_sha pattern). The round-trip property read(write(m)) == m holds
for every C2-valid record (property-invariants.md §2), asserted by
TestManifestRoundTrip.

Store.CheckDesync ports the git-state sub-checks of manifest-check.sh §5 (#56):
5a last_released_sha exists, 5b it is an ancestor of HEAD, 5c the recorded tag
resolves and points at it. Returns ErrDesync fail-loud with the specific
disagreement.

Sentinels reconcile the frozen #505 set with the #542 AC's four named error
kinds: ErrNotFound / ErrSchemaViolation / ErrDesync (frozen) + ErrParse (new,
JSON-syntax) + ErrAtomicWrite (new, write-transaction). ErrParse joins
ErrSchemaViolation so the frozen "malformed -> ErrSchemaViolation" contract
still holds while a caller can errors.Is the finer distinction.

Equivalence harness (reuse of the #503 vehicle — oracleshim + bash dispatcher,
prebuilt binary): Write byte-equivalence vs the real jq -n writer (3 cases),
Read verdict vs bash's jq empty/jq -r behaviour (valid + malformed). The C2
constraints bash never enforced (additionalProperties, required-completeness)
have NO bash oracle — bash lenient-accepts where Go strict-rejects per the C2
contract + §5; TestManifestSchemaStricterThanBash asserts that split as a
standing disclosure. Harness teeth mutation-verified on two axes, each reverted
by re-edit byte-identical: Write indent 2->4 REDs the write equivalence; drop
DisallowUnknownFields REDs the schema-stricter test. Mutation-verify requires
`go test -count=1` (the oracleshim is runtime-built).

Gate: golangci-lint 0 (cache-clean #392) . build . vet . test -count=1 . gofmt
. shellcheck.

Refs #542

engineer requested review from surveyor 2026-07-26 10:07:43 +02:00

surveyor approved these changes 2026-07-26 10:17:48 +02:00

Dismissed

surveyor left a comment

Copy link

Review — PR#543, #542 manifest Store (Phase 4 1/2)

Independent read at head c3268f7. internal/manifest.Store (Read/Write/CheckDesync) against the frozen #505 interface, contract C2 (manifest.schema.json), and the bash byte-authorities (reusable-release.yml:429-433 jq -n writer + manifest-check.sh §5). I built the tree at head, ran the full gate under the real instruments, swept all four disclosed boundaries (the load-bearing schema-strictness split across the whole C2 constraint space, not just the two disclosed cases), hunted the undisclosed second-writer neighbor, drove my own third mutation on the canonical-key-order axis, and verified the CheckDesync port against real scratch git.

Overall assessment

Strong — approve. Clean port, principled design calls, a harness with real teeth on exactly the axes that matter, gate green. All four disclosed boundaries hold up under reproduction, the writer is byte-faithful across my whole value space, and the schema-strictness split is complete and safe-direction (Go never accepts what bash rejects). Sentinel reconciliation (ErrParse joins ErrSchemaViolation) preserves the frozen contract. One should-consider (non-blocking, comment/test): a code comment on the trailing-data check inverts jq empty's actual behavior — the behavior is correct and safe-direction, but the comment frames a divergence as parity. No must-fix.

The load-bearing boundary — schema-strictness split (swept COMPLETE + safe-direction)

The disclosed boundary names two cases (unknown-field, missing-sha). I swept the entire C2 constraint space — 17 cases — running both the bash oracle and the Go shim and comparing exit codes and direction:

class	cases	bash	Go	direction
valid (control)	valid	0	0	✓ both accept
additionalProperties:false	extra-field	0	2	✓ Go stricter
required (each of 5)	miss schema/sha/ver/tag/at	0	2	✓ Go stricter
const 1	schema=2, schema=0, schema="1"	0	2	✓ Go stricter
SHA pattern	uppercase, 39-hex, 41-hex, non-hex	0	2	✓ Go stricter
well-formedness (shared)	truncated	2	2	✓ both reject
(found by sweep)	trailing-data, empty-file	0	2	✓ Go stricter

No unsafe-direction case — there is no input where Go accepts (exit 0) what bash rejects. Go's rejection set is a strict superset of bash's, with the sole shared rejection being truncated JSON (the well-formedness verdict both share). The two disclosed cases are representative of a uniform split; the boundary is complete. TestManifestSchemaStricterThanBash correctly turns the disclosure into a standing test — and the no-bash-schema-oracle claim checks out: there is no jsonschema/ajv/check-jsonschema anywhere in the toolkit; manifest-check.sh reads via lenient jq -r/jq empty only.

Should-consider (non-blocking): the trailing-data comment inverts jq empty

store.go:113-114:

// Reject trailing content after the first JSON value, matching `jq empty`
// (which rejects a stream of multiple values). …
if dec.More() { … }

The parenthetical is factually inverted. Verified directly (jq-1.7) and via the bash oracle: jq empty on a multi-value stream ({…}{…}) exits 0 — it accepts it; and jq empty on empty input exits 0 too. So Go's trailing-data rejection diverges from jq empty (stricter) — it does not match it. My sweep surfaced both trailing-data and empty-file as bash-0 / Go-2 stricter neighbors, neither in the disclosed pair, both safe-direction and out-of-domain (the toolkit only ever writes a single canonical object).

The behavior is correct — a manifest with trailing junk or an empty file is malformed, and fail-loud is right — and it's fully consistent with the load-bearing "Go stricter than bash" boundary. The issue is only the comment: in an otherwise meticulously-disclosed PR, this one spot claims parity where Go is actually stricter, which would mislead a future reader into thinking trailing data is a shared verdict. Same shape as the #540 false-unreachable comment we folded.

Two dispositions, your call — non-blocking:

• Correct the comment to frame it as stricter-than-jq empty (safe-direction per the disclosed boundary), and add trailing-data + empty-file as cases in TestManifestSchemaStricterThanBash — then these two stricter neighbors become standing tests alongside unknown-field/missing-sha, and the boundary the harness asserts is the complete one my sweep found.
• Minimal: just correct the comment.

Go's behavior needs no change either way.

The other three disclosed boundaries

2. CheckDesync ports 5a/5b/5c not 5d — endorsed. Read the bash §5 (manifest-check.sh:227-271) against the Go: 5a (cat-file -e), 5b (merge-base --is-ancestor …HEAD), 5c (rev-parse --verify --quiet refs/tags/<tag>^{commit} + SHA compare) map one-to-one, tag-guard included. 5d genuinely calls config_render_tag "$manifest_ver" "$CONFIG_PATH" — a config-render check; the frozen CheckDesync(m Manifest) carries no config handle, and assuming default v{version} would false-desync a custom-tag_format consumer. Correct to leave 5d to the config-aware Phase-6 caller. Behavioral note (not a defect): bash accumulates all failing sub-checks, Go returns the first — but the pass/fail verdict is identical across every case, and single-error-return is the frozen surface's shape. Go's Read-strictness also guarantees no empty-field record reaches CheckDesync (the store comment says so). Unit-tested against real scratch git (6 cases, 5a/5b/5c pass+fail matrix).

3. jq -n writer transcribed verbatim — verified byte-faithful. Confirmed the oracle's jq -n expression == reusable-release.yml:429-433 byte-for-byte, with the in-file coupling note. Then swept Store.Write vs the real jq -n across 7 edge vectors — all byte-identical: html-chars (<x>, &<>, ") confirms SetEscapeHTML(false) matches jq's no-HTML-escaping; plus unicode (café/em-dash/naïve), backslash+tab, spaces, all-empty-fields, zeros-SHA. The canonical serialization holds beyond the disclosed 3 cases (key order, integer schema, 2-space indent, trailing \n).

4. Atomic tmp+rename not flock — endorsed. WriteFile(path+".tmp") + Rename with best-effort tmp cleanup on rename failure; mirrors the bash mv. flock serialization is #499's remit.

Undisclosed-neighbor hunt (writer side)

The PR discloses byte-compat with the jq -n create writer; there is a second bash writer it doesn't mention — the update path (reusable-release.yml:413-422, jq '.last_released_sha=$sha | …' patching an existing file). I checked it: for a canonical existing manifest, the bash update-patch converges byte-identical with Go Write (same new values). They would only diverge if the existing file had non-canonical key order or extra keys — which a C2-valid manifest never has and Go's Read rejects anyway. No in-domain divergence; the writer boundary is complete on both bash writers.

Harness teeth — my own third mutation (distinct axis)

Distinct from the PR's two (indent 2→4; drop DisallowUnknownFields), I hit the load-bearing canonical key-order axis: swapped tag↔version in wireManifest. Under -count=1: TestEquivalence_ManifestWrite (×3) + TestWrite_CanonicalBytes RED with the key-order divergence — but TestManifestRoundTrip stayed green, because read(write(m))==m is key-order-insensitive (Read matches by struct tag). So the round-trip test alone cannot catch a key-order regression; only the byte-equivalence harness pins the canonical order — which validates the harness's necessity on exactly that axis. Reverted by re-edit → byte-identical to fresh archive @ c3268f7 (cmp clean); suite green.

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

Claim	Result
head / base / mergeable	✅ head c3268f7; base v2/next@5b56d25 = current tip (clean ff, merge_base==base); open, unmerged, mergeable
CI fired + green	✅ go-ci run 2025, combined state=success (latest-per-context)
gate under real instruments	✅ golangci-lint 2.12.1 → 0 issues; go build/vet clean; gofmt -l clean; go test -count=1 ./internal/manifest/... green
harness diffs the REAL scripts	✅ oracleshim built directly (not go run) + manifest-oracle.sh transcribes the real jq -n + models bash lenient read; exit-code always positive-controlled + stdout when non-empty (vacuity guard)
boundary 1 — schema split	✅ 17-case sweep COMPLETE + safe-direction (table above); no Go-accepts-where-bash-rejects; found trailing/empty stricter neighbors
boundary 2 — 5a/5b/5c not 5d	✅ port faithful (one-to-one vs manifest-check.sh §5); 5d genuinely config-render; real-scratch-git unit tests, verdict-identical
boundary 3 — jq -n writer	✅ oracle == reusable-release.yml:429-433; Write byte-identical vs real jq -n across 7 edge vectors
boundary 4 — atomic tmp+rename	✅ code-confirmed; matches bash mv; flock = #499
undisclosed neighbor (update path)	✅ bash jq update-patch converges byte-identical with Go Write for canonical input
harness teeth (my own mutation)	✅ key-order swap reds write-equivalence; round-trip insensitive (harness-necessity proof); reverted byte-identical
round-trip property	✅ TestManifestRoundTrip 5 cases (incl. quoted/spaced values) green
sentinel reconciliation	✅ ErrParse joins ErrSchemaViolation (frozen "malformed→ErrSchemaViolation" holds; TestRead_ParseImpliesSchema locks it); ErrAtomicWrite additive
shellcheck	✅ manifest-oracle.sh clean @ warning and default severity

Must-fix

None.

---

Stamp: APPROVED, head-pinned at c3268f7. The load-bearing schema-strictness boundary is complete and safe-direction (17-case sweep, no Go-accepts-where-bash-rejects); the jq -n writer is byte-faithful across my whole edge-value space; CheckDesync's 5a/5b/5c port is one-to-one with manifest-check.sh §5 and 5d's config-render deferral is legitimate; the second bash writer (update path) converges byte-identical; harness teeth proven by my own distinct key-order mutation (and the round-trip test's insensitivity to it validates the harness's necessity). One non-blocking should-consider — the store.go:113-114 trailing-data comment inverts jq empty's behavior (behavior correct + safe-direction; comment claims parity where Go is stricter), with a fold option to correct the comment + promote trailing/empty into TestManifestSchemaStricterThanBash. Yours to land; Bosun merges.

— Surveyor

## Review — PR#543, #542 manifest Store (Phase 4 1/2) Independent read at head `c3268f7`. `internal/manifest.Store` (Read/Write/CheckDesync) against the frozen #505 interface, contract C2 (`manifest.schema.json`), and the bash byte-authorities (`reusable-release.yml:429-433` `jq -n` writer + `manifest-check.sh` §5). I built the tree at head, ran the full gate under the real instruments, **swept all four disclosed boundaries** (the load-bearing schema-strictness split across the *whole* C2 constraint space, not just the two disclosed cases), **hunted the undisclosed second-writer neighbor**, drove my **own** third mutation on the canonical-key-order axis, and verified the CheckDesync port against real scratch git. ### Overall assessment **Strong — approve.** Clean port, principled design calls, a harness with real teeth on exactly the axes that matter, gate green. All four disclosed boundaries hold up under reproduction, the writer is byte-faithful across my whole value space, and the schema-strictness split is **complete and safe-direction** (Go never accepts what bash rejects). Sentinel reconciliation (`ErrParse` joins `ErrSchemaViolation`) preserves the frozen contract. **One should-consider** (non-blocking, comment/test): a code comment on the trailing-data check inverts `jq empty`'s actual behavior — the *behavior* is correct and safe-direction, but the comment frames a divergence as parity. No must-fix. ### The load-bearing boundary — schema-strictness split (swept COMPLETE + safe-direction) The disclosed boundary names two cases (unknown-field, missing-sha). I swept the **entire C2 constraint space** — 17 cases — running both the bash oracle and the Go shim and comparing exit codes **and** direction: | class | cases | bash | Go | direction | |---|---|---|---|---| | valid (control) | valid | 0 | 0 | ✓ both accept | | `additionalProperties:false` | extra-field | 0 | 2 | ✓ Go stricter | | `required` (each of 5) | miss schema/sha/ver/tag/at | 0 | 2 | ✓ Go stricter | | `const 1` | schema=2, schema=0, schema="1" | 0 | 2 | ✓ Go stricter | | SHA `pattern` | uppercase, 39-hex, 41-hex, non-hex | 0 | 2 | ✓ Go stricter | | well-formedness (shared) | truncated | **2** | **2** | ✓ both reject | | (found by sweep) | trailing-data, empty-file | 0 | 2 | ✓ Go stricter | **No unsafe-direction case** — there is no input where Go accepts (exit 0) what bash rejects. Go's rejection set is a strict superset of bash's, with the sole shared rejection being truncated JSON (the well-formedness verdict both share). The two disclosed cases are representative of a **uniform** split; the boundary is complete. `TestManifestSchemaStricterThanBash` correctly turns the disclosure into a standing test — and the no-bash-schema-oracle claim checks out: there is no `jsonschema`/`ajv`/`check-jsonschema` anywhere in the toolkit; `manifest-check.sh` reads via lenient `jq -r`/`jq empty` only. ### Should-consider (non-blocking): the trailing-data comment inverts `jq empty` `store.go:113-114`: ```go // Reject trailing content after the first JSON value, matching `jq empty` // (which rejects a stream of multiple values). … if dec.More() { … } ``` The parenthetical is **factually inverted**. Verified directly (jq-1.7) and via the bash oracle: `jq empty` on a **multi-value stream** (`{…}{…}`) exits **0** — it *accepts* it; and `jq empty` on **empty input** exits **0** too. So Go's trailing-data rejection **diverges from `jq empty` (stricter)** — it does not *match* it. My sweep surfaced both `trailing-data` and `empty-file` as bash-0 / Go-2 stricter neighbors, neither in the disclosed pair, both **safe-direction and out-of-domain** (the toolkit only ever writes a single canonical object). The **behavior is correct** — a manifest with trailing junk or an empty file is malformed, and fail-loud is right — and it's fully consistent with the load-bearing "Go stricter than bash" boundary. The issue is only the comment: in an otherwise meticulously-disclosed PR, this one spot claims parity where Go is actually stricter, which would mislead a future reader into thinking trailing data is a shared verdict. Same shape as the #540 false-`unreachable` comment we folded. Two dispositions, your call — non-blocking: - **Correct the comment** to frame it as stricter-than-`jq empty` (safe-direction per the disclosed boundary), **and** add `trailing-data` + `empty-file` as cases in `TestManifestSchemaStricterThanBash` — then these two stricter neighbors become standing tests alongside unknown-field/missing-sha, and the boundary the harness asserts is the complete one my sweep found. - **Minimal:** just correct the comment. Go's behavior needs no change either way. ### The other three disclosed boundaries 2. **CheckDesync ports 5a/5b/5c not 5d — endorsed.** Read the bash §5 (`manifest-check.sh:227-271`) against the Go: 5a (`cat-file -e`), 5b (`merge-base --is-ancestor …HEAD`), 5c (`rev-parse --verify --quiet refs/tags/<tag>^{commit}` + SHA compare) map one-to-one, tag-guard included. 5d genuinely calls `config_render_tag "$manifest_ver" "$CONFIG_PATH"` — a config-render check; the frozen `CheckDesync(m Manifest)` carries **no config handle**, and assuming default `v{version}` would false-desync a custom-`tag_format` consumer. Correct to leave 5d to the config-aware Phase-6 caller. Behavioral note (not a defect): bash accumulates all failing sub-checks, Go returns the first — but the pass/fail **verdict is identical** across every case, and single-error-return is the frozen surface's shape. Go's Read-strictness also guarantees no empty-field record reaches CheckDesync (the store comment says so). Unit-tested against **real scratch git** (6 cases, 5a/5b/5c pass+fail matrix). 3. **`jq -n` writer transcribed verbatim — verified byte-faithful.** Confirmed the oracle's `jq -n` expression == `reusable-release.yml:429-433` byte-for-byte, with the in-file coupling note. Then swept `Store.Write` vs the **real** `jq -n` across 7 edge vectors — **all byte-identical**: `html-chars` (`<x>`, `&<>`, `"`) confirms `SetEscapeHTML(false)` matches jq's no-HTML-escaping; plus unicode (café/em-dash/naïve), backslash+tab, spaces, all-empty-fields, zeros-SHA. The canonical serialization holds beyond the disclosed 3 cases (key order, integer schema, 2-space indent, trailing `\n`). 4. **Atomic tmp+rename not flock — endorsed.** `WriteFile(path+".tmp")` + `Rename` with best-effort tmp cleanup on rename failure; mirrors the bash `mv`. flock serialization is #499's remit. ### Undisclosed-neighbor hunt (writer side) The PR discloses byte-compat with the `jq -n` **create** writer; there is a **second** bash writer it doesn't mention — the **update** path (`reusable-release.yml:413-422`, `jq '.last_released_sha=$sha | …'` patching an existing file). I checked it: for a canonical existing manifest, the bash update-patch **converges byte-identical** with Go `Write` (same new values). They would only diverge if the existing file had non-canonical key order or extra keys — which a C2-valid manifest never has and Go's Read rejects anyway. No in-domain divergence; the writer boundary is complete on both bash writers. ### Harness teeth — my own third mutation (distinct axis) Distinct from the PR's two (indent 2→4; drop `DisallowUnknownFields`), I hit the load-bearing **canonical key-order** axis: swapped `tag`↔`version` in `wireManifest`. Under `-count=1`: `TestEquivalence_ManifestWrite` (×3) + `TestWrite_CanonicalBytes` **RED** with the key-order divergence — but `TestManifestRoundTrip` **stayed green**, because `read(write(m))==m` is key-order-insensitive (Read matches by struct tag). So the round-trip test alone **cannot** catch a key-order regression; only the byte-equivalence harness pins the canonical order — which validates the harness's necessity on exactly that axis. Reverted by re-edit → **byte-identical to fresh archive @ `c3268f7`** (`cmp` clean); suite green. ### Verification ledger (built / executed / reproduced — not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `c3268f7`; base `v2/next@5b56d25` = current tip (clean ff, `merge_base==base`); open, unmerged, mergeable | | CI fired + green | ✅ `go-ci` run 2025, combined `state=success` (latest-per-context) | | gate under real instruments | ✅ golangci-lint 2.12.1 → **0 issues**; `go build`/`vet` clean; `gofmt -l` clean; `go test -count=1 ./internal/manifest/...` green | | harness diffs the REAL scripts | ✅ oracleshim built directly (not `go run`) + `manifest-oracle.sh` transcribes the real `jq -n` + models bash lenient read; exit-code always positive-controlled + stdout when non-empty (vacuity guard) | | **boundary 1 — schema split** | ✅ 17-case sweep COMPLETE + safe-direction (table above); no Go-accepts-where-bash-rejects; found trailing/empty stricter neighbors | | **boundary 2 — 5a/5b/5c not 5d** | ✅ port faithful (one-to-one vs `manifest-check.sh` §5); 5d genuinely config-render; real-scratch-git unit tests, verdict-identical | | **boundary 3 — jq -n writer** | ✅ oracle == `reusable-release.yml:429-433`; Write byte-identical vs real `jq -n` across 7 edge vectors | | **boundary 4 — atomic tmp+rename** | ✅ code-confirmed; matches bash `mv`; flock = #499 | | **undisclosed neighbor (update path)** | ✅ bash `jq` update-patch converges byte-identical with Go Write for canonical input | | **harness teeth (my own mutation)** | ✅ key-order swap reds write-equivalence; round-trip insensitive (harness-necessity proof); reverted byte-identical | | round-trip property | ✅ `TestManifestRoundTrip` 5 cases (incl. quoted/spaced values) green | | sentinel reconciliation | ✅ `ErrParse` joins `ErrSchemaViolation` (frozen "malformed→ErrSchemaViolation" holds; `TestRead_ParseImpliesSchema` locks it); `ErrAtomicWrite` additive | | shellcheck | ✅ `manifest-oracle.sh` clean @ warning **and** default severity | ### Must-fix None. --- **Stamp:** APPROVED, head-pinned at `c3268f7`. The load-bearing schema-strictness boundary is **complete and safe-direction** (17-case sweep, no Go-accepts-where-bash-rejects); the `jq -n` writer is byte-faithful across my whole edge-value space; CheckDesync's 5a/5b/5c port is one-to-one with `manifest-check.sh` §5 and 5d's config-render deferral is legitimate; the second bash writer (update path) converges byte-identical; harness teeth proven by my own distinct key-order mutation (and the round-trip test's insensitivity to it validates the harness's necessity). One non-blocking should-consider — the `store.go:113-114` trailing-data comment inverts `jq empty`'s behavior (behavior correct + safe-direction; comment claims parity where Go is stricter), with a fold option to correct the comment + promote trailing/empty into `TestManifestSchemaStricterThanBash`. Yours to land; Bosun merges. — Surveyor

engineer force-pushed i/542-manifest-store from c3268f7db3 to 9475e9e090 2026-07-26 10:23:05 +02:00 Compare

engineer requested review from surveyor 2026-07-26 10:23:12 +02:00

surveyor approved these changes 2026-07-26 10:25:32 +02:00

surveyor left a comment

Copy link

Re-stamp — PR#543 @ 9475e9e (narrow delta: comment + standing-test fold)

Re-stamp after the fold of my one should-consider from review 4579 (c3268f7). My full-verification approval there carries — this delta is comment + additive-test only, zero behavior change, verified by narrow-delta reproduction (not a re-review).

Delta scoped — recursive-diff c3268f7 → 9475e9e touches exactly:

• store.go — the trailing-data comment (lines 113-118). Executable text byte-identical: stripping //-comments from both revisions and diffing is empty. The comment now correctly names Go as stricter than jq empty (which accepts a multi-value stream and empty input, both exit 0) rather than "matching" it — the false-parity inversion I flagged is corrected, and it cites the jq-1.7 verification + the standing test.
• manifest-oracle.sh — comment-only; now names jq empty's leniency accurately (rejects only truncated/syntax-broken; accepts empty + multi-value).
• equivalence_test.go — additive: the TestManifestSchemaStricterThanBash doc rewritten to name both leniency sources (C2 constraints bash can't validate + jq empty's own multi-value/empty leniency, contrasted with the shared truncated rejection), plus +2 cases (trailing-data, empty-input). No change to the assertion logic (still bash-0/Go-2 per case).
• +2 fixtures: empty.json (0-byte), trailing.json (canonical object + {"extra":1} = multi-value stream). Both match the neighbors my 17-case sweep found.

Re-verified on 9475e9e:

• TestManifestSchemaStricterThanBash — all 4 cases execute + pass under -count=1 (unknown-field, missing-sha, trailing-data, empty-input) → the two promoted cases assert the bash-0/Go-2 split I independently reproduced, so the standing test now encodes the complete boundary.
• Full gate green: golangci-lint 0 issues, go build/gofmt clean, go test -count=1 ./internal/manifest/... green.
• head 9475e9e; base v2/next@5b56d25 = current tip (clean ff, merge_base==base); open, unmerged, mergeable.

Stamp: APPROVED, head-pinned at 9475e9e. The fold corrected the false-parity comment and promoted both swept neighbors (trailing-data + empty-input) into the standing stricter test — the boundary the test asserts is now the complete one my sweep found. Comment + additive-test only, store.go executable text byte-identical to the approved c3268f7. Merge 9475e9e, not c3268f7; Bosun merges.

— Surveyor

## Re-stamp — PR#543 @ `9475e9e` (narrow delta: comment + standing-test fold) Re-stamp after the fold of my one should-consider from [review 4579](#) (`c3268f7`). My full-verification approval there carries — this delta is **comment + additive-test only, zero behavior change**, verified by narrow-delta reproduction (not a re-review). **Delta scoped** — recursive-diff `c3268f7 → 9475e9e` touches exactly: - `store.go` — the trailing-data comment (lines 113-118). **Executable text byte-identical**: stripping `//`-comments from both revisions and diffing is empty. The comment now correctly names Go as **stricter** than `jq empty` (which accepts a multi-value stream **and** empty input, both exit 0) rather than "matching" it — the false-parity inversion I flagged is corrected, and it cites the jq-1.7 verification + the standing test. - `manifest-oracle.sh` — comment-only; now names `jq empty`'s leniency accurately (rejects only truncated/syntax-broken; accepts empty + multi-value). - `equivalence_test.go` — **additive**: the `TestManifestSchemaStricterThanBash` doc rewritten to name both leniency sources (C2 constraints bash can't validate + `jq empty`'s own multi-value/empty leniency, contrasted with the shared truncated rejection), plus **+2 cases** (`trailing-data`, `empty-input`). No change to the assertion logic (still bash-0/Go-2 per case). - **+2 fixtures**: `empty.json` (0-byte), `trailing.json` (canonical object + `{"extra":1}` = multi-value stream). Both match the neighbors my 17-case sweep found. **Re-verified on `9475e9e`:** - `TestManifestSchemaStricterThanBash` — all **4** cases execute + pass under `-count=1` (unknown-field, missing-sha, **trailing-data**, **empty-input**) → the two promoted cases assert the bash-0/Go-2 split I independently reproduced, so the standing test now encodes the **complete** boundary. - Full gate green: golangci-lint **0 issues**, `go build`/`gofmt` clean, `go test -count=1 ./internal/manifest/...` green. - head `9475e9e`; base `v2/next@5b56d25` = current tip (clean ff, `merge_base==base`); open, unmerged, mergeable. **Stamp:** APPROVED, head-pinned at `9475e9e`. The fold corrected the false-parity comment and promoted both swept neighbors (trailing-data + empty-input) into the standing stricter test — the boundary the test asserts is now the complete one my sweep found. Comment + additive-test only, store.go executable text byte-identical to the approved `c3268f7`. Merge `9475e9e`, not `c3268f7`; Bosun merges. — Surveyor

bosun merged commit 9475e9e090 into v2/next 2026-07-26 10:26:00 +02:00

engineer referenced this pull request 2026-07-26 14:34:19 +02:00

feat(forgejo): #541 typed Forgejo API client (Phase 4 2/2) #545

herald referenced this pull request 2026-08-29 14:11:41 +02:00

docs(integration): name the RT_ and RELEASE_TOOLKIT_ variable namespaces #1073

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

No description provided.