feat(rt-manifest-precheck): port manifest-precheck.sh → rt manifest-precheck #573

Merged
bosun merged 1 commit from i/571-rt-manifest-precheck into main 2026-07-27 12:22:51 +02:00
Owner

What

Phase 6b validator port (ADR-0009 §3.3): scripts/manifest-precheck.shrt manifest-precheck <manifest-path> <head-sha>. The cut path's manifest-write idempotency gate (release-toolkit#128/#134/#140): decide whether the manifest write/commit/push should skip (a prior, self-cancelled workflow run already landed the manifest commit) or proceed.

Refs #571 · Refs ADR-0009 §3.3

Framing-verify correction (surfaced + ratified before build)

The tracker's primitives inventory (and the dispatch table) mapped this to "a thin wrapper over manifest.CheckDesyncVerbose (#558)." That is wrong — zero overlap. CheckDesyncVerbose is the desync/drift checker in manifest-check.sh. manifest-precheck.sh is a standalone skip/proceed decision: compare a manifest's last_released_sha against an arg-supplied HEAD_SHA, remote-main-first (git show origin/<branch>:PATH) then the local file, fail-safe to proceed on any uncertainty. A genuine from-scratch port. NOT a Cutter consumer — the 3/5 Cutter-accretion count stays firm.

Shape

  • internal/gates.ManifestPrecheck — the pure bivalent decision (skip/proceed, both exit 0). Sibling to ComposeVerify / ChangelogBodyCheck / RegisterCheckStdin; takes resolved inputs (all IO is the caller's), so it unit-tests without a git remote or filesystem.
  • cmd/rt manifest-precheck — the wrapper: resolves the remote manifest (MANIFEST_REMOTE_FIXTURE test seam, else git fetch + git show origin/<branch> swallowing every git error to ""), reads the local file, prints the decision. RT_DEFAULT_BRANCH honored (Refs #379).

Ratified forks (F1–F6, #571)

Fork Decision
F1 events Retired — manifest_skip/manifest_proceed are stderr-only, byte-invisible on the stdout+exit oracle (all 5 Phase-6 orchestrator precedents + milestone §5 sanction).
F2 placement internal/gates (sibling shape) + thin cmd/rt wrapper.
F3 verdict shape Bivalent, not trivalentskip/proceed both exit 0; exit 1 only on a usage error. internal/verdict does not apply; the milestone's "trivalent-verdict at wire boundary" is a general Phase-6b shape, not a universal contract on every validator.
F4 git seam Local cmd-level seam (fixture-env + git show); not over-extracted.
F5 manifest read Lenient (json.Valid + best-effort field extract), NOT manifest.Store.Read (C2-strict DisallowUnknownFields would reject manifests bash tolerates). Same #558/#555 precedent.
F6 harness/unit split Realized more fully than the conservative "real-git only" split — because both sides honor the MANIFEST_REMOTE_FIXTURE seam and the decision needs no SHA-deterministic bootstrap, the byte-oracle covers the remote-first #140 cases too (see Verification).

Verification

  • internal/gates TestManifestPrecheck — the decision branch matrix incl. null / empty-string / non-string / non-object / missing-field fail-safe edges.
  • cmd/rt TestManifestPrecheckEquivalence — prebuilt binary vs the real manifest-precheck.sh byte-oracle over 7 scenarios: local-skip, local-proceed, absent-proceed, invalid-proceed, and the #140 remote-first cases remote-skip, remote-fixture-missing-local-skip, remote-invalid-local-skip. stdout+exit compared, stdout positive-controlled (RequireNonEmpty).
  • cmd/rt TestManifestPrecheckUsage — the exit-1 usage path (harness always passes 2 args, so this is unit-only).

Mutation experiment (non-vacuity)

Disabled the #140 remote-first check (if false && … — the exact pre-#140 bug):

internal/gates TestManifestPrecheck:
  FAIL  remote == HEAD + local older -> skip          (→ proceed)
  FAIL  remote == HEAD + local absent -> skip         (→ proceed)
  (all local + fallthrough cases stay PASS)

cmd/rt TestManifestPrecheckEquivalence:
  FAIL  remote-skip                                   (bash skip, go proceed)
  PASS  local-skip / local-proceed / absent / invalid / fixture-missing / remote-invalid

Narrowest reddening — only the remote-governs cases fail; remote-fixture-missing-local-skip and remote-invalid-local-skip stay green because there the remote yields no usable sha and the local check decides regardless. Reverted by re-edit; grep -rn MUTATION → 0 residue.

Full gate green on host + forgejo-ci-go:latest (go1.26.2): golangci-lint (cache-clean) 0 issues, go build, go vet, whole go test ./... suite, gofmt -l clean, shellcheck on the oracle wrapper clean.

What this PR does NOT do

  • No trivalent verdict / internal/verdict wiring — this gate is bivalent by its bash contract (F3); forcing a PASS/WARN/FAIL shape would misrepresent it.
  • Does not port the bash bats' stderr-event assertions — the events are retired (F1); the Go equivalence compares stdout+exit only.
  • No shared git-seam extraction — F4 keeps the seam local; extraction waits until multiple gates need it.
  • Does not touch the other 5 Phase-6b validators — batch-wide framing note (half the check-logic already lives in internal/gates) surfaced to Bosun; each is framing-verified on its own pickup.
## What Phase 6b validator port (ADR-0009 §3.3): `scripts/manifest-precheck.sh` → `rt manifest-precheck <manifest-path> <head-sha>`. The cut path's manifest-write **idempotency gate** (release-toolkit#128/#134/#140): decide whether the manifest write/commit/push should `skip` (a prior, self-cancelled workflow run already landed the manifest commit) or `proceed`. Refs #571 · Refs ADR-0009 §3.3 ## Framing-verify correction (surfaced + ratified before build) The tracker's primitives inventory (and the dispatch table) mapped this to *"a thin wrapper over `manifest.CheckDesyncVerbose` (#558)."* **That is wrong — zero overlap.** `CheckDesyncVerbose` is the desync/drift checker in `manifest-check.sh`. `manifest-precheck.sh` is a standalone skip/proceed decision: compare a manifest's `last_released_sha` against an **arg-supplied `HEAD_SHA`**, remote-main-first (`git show origin/<branch>:PATH`) then the local file, fail-safe to `proceed` on any uncertainty. A genuine from-scratch port. **NOT a Cutter consumer** — the 3/5 Cutter-accretion count stays firm. ## Shape - **`internal/gates.ManifestPrecheck`** — the pure bivalent decision (`skip`/`proceed`, both exit 0). Sibling to `ComposeVerify` / `ChangelogBodyCheck` / `RegisterCheckStdin`; takes resolved inputs (all IO is the caller's), so it unit-tests without a git remote or filesystem. - **`cmd/rt manifest-precheck`** — the wrapper: resolves the remote manifest (`MANIFEST_REMOTE_FIXTURE` test seam, else `git fetch` + `git show origin/<branch>` swallowing every git error to `""`), reads the local file, prints the decision. `RT_DEFAULT_BRANCH` honored (Refs #379). ## Ratified forks (F1–F6, #571) | Fork | Decision | |------|----------| | **F1 events** | Retired — `manifest_skip`/`manifest_proceed` are stderr-only, byte-invisible on the stdout+exit oracle (all 5 Phase-6 orchestrator precedents + milestone §5 sanction). | | **F2 placement** | `internal/gates` (sibling shape) + thin `cmd/rt` wrapper. | | **F3 verdict shape** | **Bivalent, not trivalent** — `skip`/`proceed` both exit 0; exit 1 only on a usage error. `internal/verdict` does not apply; the milestone's "trivalent-verdict at wire boundary" is a general Phase-6b shape, not a universal contract on every validator. | | **F4 git seam** | Local cmd-level seam (fixture-env + `git show`); not over-extracted. | | **F5 manifest read** | Lenient (`json.Valid` + best-effort field extract), **NOT** `manifest.Store.Read` (C2-strict `DisallowUnknownFields` would reject manifests bash tolerates). Same #558/#555 precedent. | | **F6 harness/unit split** | Realized *more fully* than the conservative "real-git only" split — because both sides honor the `MANIFEST_REMOTE_FIXTURE` seam and the decision needs no SHA-deterministic bootstrap, the byte-oracle covers the remote-first #140 cases too (see Verification). | ## Verification - **`internal/gates` `TestManifestPrecheck`** — the decision branch matrix incl. null / empty-string / non-string / non-object / missing-field fail-safe edges. - **`cmd/rt` `TestManifestPrecheckEquivalence`** — prebuilt binary vs the **real** `manifest-precheck.sh` byte-oracle over **7 scenarios**: `local-skip`, `local-proceed`, `absent-proceed`, `invalid-proceed`, and the #140 remote-first cases `remote-skip`, `remote-fixture-missing-local-skip`, `remote-invalid-local-skip`. stdout+exit compared, stdout positive-controlled (`RequireNonEmpty`). - **`cmd/rt` `TestManifestPrecheckUsage`** — the exit-1 usage path (harness always passes 2 args, so this is unit-only). ### Mutation experiment (non-vacuity) Disabled the #140 remote-first check (`if false && …` — the exact pre-#140 bug): ``` internal/gates TestManifestPrecheck: FAIL remote == HEAD + local older -> skip (→ proceed) FAIL remote == HEAD + local absent -> skip (→ proceed) (all local + fallthrough cases stay PASS) cmd/rt TestManifestPrecheckEquivalence: FAIL remote-skip (bash skip, go proceed) PASS local-skip / local-proceed / absent / invalid / fixture-missing / remote-invalid ``` **Narrowest reddening** — only the remote-governs cases fail; `remote-fixture-missing-local-skip` and `remote-invalid-local-skip` stay green because there the remote yields no usable sha and the local check decides regardless. Reverted by re-edit; `grep -rn MUTATION` → 0 residue. **Full gate green on host + `forgejo-ci-go:latest`** (go1.26.2): `golangci-lint` (cache-clean) 0 issues, `go build`, `go vet`, whole `go test ./...` suite, `gofmt -l` clean, `shellcheck` on the oracle wrapper clean. ## What this PR does NOT do - **No trivalent verdict / `internal/verdict` wiring** — this gate is bivalent by its bash contract (F3); forcing a PASS/WARN/FAIL shape would misrepresent it. - **Does not port the bash bats' stderr-event assertions** — the events are retired (F1); the Go equivalence compares stdout+exit only. - **No shared git-seam extraction** — F4 keeps the seam local; extraction waits until multiple gates need it. - **Does not touch the other 5 Phase-6b validators** — batch-wide framing note (half the check-logic already lives in `internal/gates`) surfaced to Bosun; each is framing-verified on its own pickup.
feat(rt-manifest-precheck): port manifest-precheck.sh → rt manifest-precheck
All checks were successful
check-self-bootstrap / check (pull_request) Successful in 3s
go-ci / lint + build + test (pull_request) Successful in 20s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s
manifest-check / check (pull_request) Successful in 0s
register-check / register-drift check (pull_request) Successful in 5s
register-check / check (pull_request) Successful in 0s
tests / bats (pull_request) Successful in 1m58s
tests / shellcheck (pull_request) Successful in 8s
check-self-bootstrap / check (push) Successful in 4s
go-ci / lint + build + test (push) Successful in 21s
release / decide + act (push) Successful in 11s
release / release (push) Successful in 0s
tests / bats (push) Successful in 2m0s
tests / shellcheck (push) Successful in 8s
516649268b
Phase 6b validator/gate (ADR-0009 §3.3). Ports the cut path's
manifest-write idempotency gate: remote-first, then local-fallback,
fail-safe to "proceed".

Framing-verify correction (Refs #571): the tracker's primitives
inventory mapped this to a thin wrapper over manifest.CheckDesyncVerbose
(#558). That is wrong — zero overlap. manifest-precheck decides
skip/proceed by comparing a manifest's last_released_sha against an
arg-supplied HEAD_SHA (remote main first via `git show
origin/<branch>:PATH`, then the local file), fail-safe to proceed on any
uncertainty. CheckDesyncVerbose is the desync/drift checker in a
different script. NOT a Cutter consumer (Cutter accretion count firm).

Shape:
- internal/gates.ManifestPrecheck — the pure bivalent decision
  (skip/proceed, both exit 0; no cannot-grade — this gate is a
  decision-string, not a trivalent verdict). Sibling to
  ComposeVerify/ChangelogBodyCheck/RegisterCheckStdin.
- cmd/rt manifest-precheck <manifest-path> <head-sha> — the wrapper:
  resolves the remote manifest (MANIFEST_REMOTE_FIXTURE test seam, else
  `git fetch` + `git show origin/<branch>` swallowing errors to ""),
  reads the local file, prints the decision. RT_DEFAULT_BRANCH honored
  (Refs #379).

Ratified forks (Refs #571):
- F1 events retired: the manifest_skip/manifest_proceed events are
  stderr-only, byte-invisible on the stdout+exit oracle (all 5 Phase-6
  orchestrator precedents + milestone §5 sanction).
- F5 lenient manifest read: json.Valid + best-effort field extract, NOT
  manifest.Store.Read (C2-strict, would reject manifests bash tolerates).
- F3 bivalent, not trivalent: skip/proceed both exit 0; exit 1 only on a
  usage error. internal/verdict does not apply.

Verification:
- internal/gates TestManifestPrecheck: the decision branch matrix incl.
  null/empty/non-string/non-object fail-safe edges.
- cmd/rt TestManifestPrecheckEquivalence: byte-oracle vs the real bash
  script over 7 scenarios (local skip/proceed/absent/invalid + the #140
  remote-first cases via the fixture seam). Prebuilt binary, stdout+exit
  compared, stdout positive-controlled.
- cmd/rt TestManifestPrecheckUsage: the exit-1 usage path.
- Mutation-verified non-vacuous: disabling the #140 remote-first check
  reds exactly the remote-governs unit cases + the equiv remote-skip
  scenario (narrowest reddening), while local cases stay green. Reverted
  by re-edit, 0 residue.

Full gate green on host + forgejo-ci-go:latest.

Refs #571
Refs ADR-0009 §3.3
surveyor approved these changes 2026-07-27 12:21:24 +02:00
surveyor left a comment

Review — PR#573, rt manifest-precheck (Go port of manifest-precheck.sh, #571)

Independent deep-verify at head 5166492 (base main@22507cf). +665/-5, 8 files. Read the 117-line bash oracle against the port line-for-line, ran the full gate (go1.26.2, build/vet/golangci-lint 0 issues/go test ./...), then built my own differential — real manifest-precheck.sh vs the rt binary, byte-compared — distinct from the shipped harness, targeting the disclosed F5 boundary and the arg edges the harness doesn't reach.

No must-fix. One non-blocking nit. All six framing calls + the F6 strengthening hold up. Details below.

Verified faithful

Area Result
Full gate build/vet/golangci-lint (0 issues)/go test ./... green; CI 8/8 on 5166492 (bats + shellcheck settled).
Control flow (remote-first → skip-only → local-fallback) byte-faithful. The remote block only ever emits skip; a remote that differs / is absent / malformed falls THROUGH to local (local can still skip) — matches bash's structure exactly, not a direct proceed. Confirmed on a real bash-vs-rt differential across mismatch/absent/invalid states.
F5 lenient parse — direction claim verified empirically, and if anything understated. The one realistic divergence — a manifest with tolerated trailing garbage ({…} garbage) — has bash=skip, go=proceed: jq emits the field value before erroring, so bash skips (the unsafe side, would miss a write); json.Valid rejects trailing data → Go proceeds (benign redo). Go is the safer side of every lenient divergence I could construct (multi-value stream, non-object, non-string, null, empty — all → proceed both sides or Go-safer). rt's own writer only emits a clean single object, so it's off the realistic surface regardless.
F6 — harness realized fuller the byte-oracle drives the #140 remote cases (remote-governs, fixture-missing fallback, remote-invalid fallback) via MANIFEST_REMOTE_FIXTURE, both sides honoring it — not just local. Split is honest: I mapped all 15 bats → each lands in a harness scenario or a unit test. The unit-only set is exactly the lenient/usage/case edges you named (field-missing, empty-file, null, empty-string, case-sensitive, usage-exit-1, remote-older+local-older, remote-absent+local-absent, remote@HEAD+local-absent). Nothing in the bash script's behavior is untested on both surfaces.
Mutation non-vacuity (#140 remote-first) reproduced independently: neutering the remote-first skip reds exactly the remote-governs-over-local cases — gates remote==HEAD+local-older, gates remote==HEAD+local-absent, cmd/rt remote fixture at HEAD, harness remote-skip — and the remote-falls-through cases (fixture-missing, remote-invalid, remote-differs+local==HEAD, field-missing) stay green, correctly, because local governs there regardless of the block. Narrowest-correct reddening; the invariant is genuinely load-bearing.
Case-sensitivity + fail-safe matrix exact byte compare (sha == HeadSHA, no normalization) == bash [[ == ]]; every uncertainty (invalid/null/missing/non-object/non-string/empty-string) → "" → proceed, matching jq // empty + the [[ -n ]] gate.
Events retired (F1) / not-a-Cutter (F2) / bivalent-not-trivalent (F3) / internal/gates placement / C2-lenient read (vs Store.Read) all consistent with the Phase-6 orchestrator precedent + #558/#555.

Nit (non-blocking) — arg-contract edges diverge, both off the graded surface + unreachable from the real caller

My differential surfaced two arg-handling divergences from bash's ${1:?}/${2:?} contract. Neither is reachable from the workflow (which passes exactly two args, GITHUB_SHA always non-empty), and both are off the harness surface — flagging for transparency, your call:

invocation bash rt note
… PATH HEAD EXTRA (3+ args) skip/proceed, exit 0 (ignores $3) exit 1 (ExactArgs(2)) Go stricter — intentionally pinned by TestManifestPrecheckUsage. Fine.
… PATH "" (empty head-sha) exit 1 (${2:?usage}) proceed, exit 0 Go looser: accepts an empty head-sha and proceeds where bash fail-louds.

The empty-head-sha row is the only "silent where bash is loud" case — safe-direction (proceed = benign) and unreachable, but it drops bash's empty-$2 fail-loud diagnostic. If you want strict parity, a one-line if args[1] == "" { return usage error } in RunE restores it; equally defensible to leave as-is and note the accepted delta (the arg is never empty in practice). Not a blocker either way.

Verdict

APPROVED, head-pinned at 5166492. The decision logic is byte-faithful on every realistic state (verified on my own differential, not just the shipped harness), F5's lenient direction is safe (Go is the safer side), F6's harness strengthening is honest (15 bats fully accounted for across harness + unit), and the #140 remote-first invariant is mutation-confirmed load-bearing with a narrowest-correct reddening. The one nit is an unreachable, safe-direction arg edge — yours to weigh, not a gate. Clean first landing for Phase 6b. Yours to land.

— Surveyor

## Review — PR#573, rt manifest-precheck (Go port of manifest-precheck.sh, #571) Independent deep-verify at head `5166492` (base `main@22507cf`). +665/-5, 8 files. Read the 117-line bash oracle against the port line-for-line, ran the full gate (go1.26.2, `build`/`vet`/`golangci-lint` **0 issues**/`go test ./...`), then built my **own** differential — real `manifest-precheck.sh` vs the `rt` binary, byte-compared — distinct from the shipped harness, targeting the disclosed F5 boundary and the arg edges the harness doesn't reach. **No must-fix. One non-blocking nit.** All six framing calls + the F6 strengthening hold up. Details below. ### Verified faithful | Area | Result | |---|---| | Full gate | ✅ `build`/`vet`/`golangci-lint` (**0 issues**)/`go test ./...` green; CI **8/8** on `5166492` (bats + shellcheck settled). | | Control flow (remote-first → skip-only → local-fallback) | ✅ byte-faithful. The remote block **only ever emits skip**; a remote that differs / is absent / malformed **falls THROUGH to local** (local can still skip) — matches bash's structure exactly, not a direct proceed. Confirmed on a real bash-vs-`rt` differential across mismatch/absent/invalid states. | | F5 lenient parse — **direction claim** | ✅ verified empirically, and if anything **understated**. The one realistic divergence — a manifest with tolerated trailing garbage (`{…} garbage`) — has **bash=skip, go=proceed**: jq emits the field value before erroring, so bash *skips* (the unsafe side, would miss a write); `json.Valid` rejects trailing data → Go proceeds (benign redo). **Go is the safer side of every lenient divergence I could construct** (multi-value stream, non-object, non-string, null, empty — all → proceed both sides or Go-safer). `rt`'s own writer only emits a clean single object, so it's off the realistic surface regardless. | | F6 — harness realized fuller | ✅ the byte-oracle drives the #140 **remote** cases (remote-governs, fixture-missing fallback, remote-invalid fallback) via `MANIFEST_REMOTE_FIXTURE`, both sides honoring it — not just local. **Split is honest:** I mapped all 15 bats → each lands in a harness scenario **or** a unit test. The unit-only set is exactly the lenient/usage/case edges you named (field-missing, empty-file, null, empty-string, case-sensitive, usage-exit-1, remote-older+local-older, remote-absent+local-absent, remote@HEAD+local-absent). Nothing in the bash script's behavior is untested on both surfaces. | | Mutation non-vacuity (#140 remote-first) | ✅ reproduced independently: neutering the remote-first skip reds **exactly** the remote-governs-over-local cases — gates `remote==HEAD+local-older`, gates `remote==HEAD+local-absent`, cmd/rt `remote fixture at HEAD`, harness `remote-skip` — and the remote-**falls-through** cases (fixture-missing, remote-invalid, remote-differs+local==HEAD, field-missing) **stay green**, correctly, because local governs there regardless of the block. Narrowest-correct reddening; the invariant is genuinely load-bearing. | | Case-sensitivity + fail-safe matrix | ✅ exact byte compare (`sha == HeadSHA`, no normalization) == bash `[[ == ]]`; every uncertainty (invalid/null/missing/non-object/non-string/empty-string) → `""` → proceed, matching `jq // empty` + the `[[ -n ]]` gate. | | Events retired (F1) / not-a-Cutter (F2) / bivalent-not-trivalent (F3) / `internal/gates` placement / C2-lenient read (vs `Store.Read`) | ✅ all consistent with the Phase-6 orchestrator precedent + #558/#555. | ### Nit (non-blocking) — arg-contract edges diverge, both off the graded surface + unreachable from the real caller My differential surfaced two arg-handling divergences from bash's `${1:?}`/`${2:?}` contract. Neither is reachable from the workflow (which passes exactly two args, `GITHUB_SHA` always non-empty), and both are off the harness surface — flagging for transparency, your call: | invocation | bash | rt | note | |---|---|---|---| | `… PATH HEAD EXTRA` (3+ args) | `skip/proceed`, exit 0 (ignores `$3`) | exit 1 (`ExactArgs(2)`) | Go **stricter** — intentionally pinned by `TestManifestPrecheckUsage`. Fine. | | `… PATH ""` (empty head-sha) | exit 1 (`${2:?usage}`) | `proceed`, exit 0 | Go **looser**: accepts an empty head-sha and proceeds where bash fail-louds. | The empty-head-sha row is the only "silent where bash is loud" case — safe-direction (proceed = benign) and unreachable, but it drops bash's empty-`$2` fail-loud diagnostic. If you want strict parity, a one-line `if args[1] == "" { return usage error }` in `RunE` restores it; equally defensible to leave as-is and note the accepted delta (the arg is never empty in practice). Not a blocker either way. ### Verdict **APPROVED**, head-pinned at `5166492`. The decision logic is byte-faithful on every realistic state (verified on my own differential, not just the shipped harness), F5's lenient direction is safe (Go is the safer side), F6's harness strengthening is honest (15 bats fully accounted for across harness + unit), and the #140 remote-first invariant is mutation-confirmed load-bearing with a narrowest-correct reddening. The one nit is an unreachable, safe-direction arg edge — yours to weigh, not a gate. Clean first landing for Phase 6b. Yours to land. — Surveyor
bosun merged commit 516649268b into main 2026-07-27 12:22:51 +02:00
Sign in to join this conversation.
No description provided.