feat(rt-manifest-check): port manifest-check.sh to rt manifest-check #564
No reviewers
Labels
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
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!564
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/558-rt-manifest-check"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
rt manifest-check — the Go port of
scripts/manifest-check.sh(#558)Ports the read-only PR-CI check that verifies release-state consistency in a
consumer repo's working tree. Structurally unlike prep (#556) and release
(#557): those run on toolkit-self and consume the Cutter's transactional shape;
manifest-check is read-only and runs in a foreign CWD, so it has no Cutter, no
transaction, no mutation — it is an accumulator that reads and reports.
Ratified framing (framing-verify → fork → ratify, before building)
foreign-CWD check has no reversible/irreversible phases. Refinement count stays
3/5 — this PR adds a
manifest.Storerefinement, which is a separatesubstrate from the Cutter and does not tick the Cutter-accretion count.
manifest.Store= fork "B" (Bosun-ratified): the §5git-desync checks route through a new
CheckDesyncVerboserather than beingre-implemented inline in the orchestrator. Reuse-over-duplicate: a single source
of truth for the git-desync semantics (the check whose failure mode is "the
manifest lied about the tree"). The alternative — duplicating
cat-file -e/merge-base --is-ancestor/rev-parse tagin the orchestrator — was rejectedas a drift hazard on safety-critical logic.
Five sections (accumulator: every section emits a line; failures are counted, not aborted)
config.Load. Invalid config aborts here (fatal, as the oracle does).cfg.VersionFiles+ the newprep.ReadVersionFileper file,SemVer-validated, then a lockstep (all-agree) check. An explicit
version_files: []is the tag-is-version opt-out (skip).
changelog.HasUnreleasedSection(a slot to transition into).prep.DescribeStableTag(already the exact--exclude '*-*'stable-tag read) vs the version_files version: manifest must be
>=the laststable tag (
<is the reverted-without-rebumping regression #56 catches).manifest.CheckDesyncVerbose: sha-exists (5a),ancestor-of-HEAD (5b), tag↔sha (5c), plus the config-render tag↔version check (5d,
orchestrator-owned —
CheckDesynccarries no config handle).New substrate (all additive; existing callers byte-unchanged)
changelog.HasUnreleasedSection([]byte) bool— reusesUnreleasedContent'sheading prefix +
awkRecords, so the two heading-detectors cannot drift. Distinctfrom
UnreleasedContent: a bool presence check (a present-but-empty section is true).prep.ReadVersionFile(path) (version, kind, err)— the read half of §2, siblingto
BumpVersionFile. VERSION → strip ALL C-locale whitespace (tr -d '[:space:]',not merely trim); package.json →
.version. A malformed/absent/null.versionyields an empty version with a nil error, reproducing bash's
jq-into-command-substitution swallow (the caller emits "could not extract version"). Same
package.json fidelity boundary
BumpVersionFilealready discloses.manifest.CheckDesyncVerbose(m) []DesyncCheck+DesyncCheck{Name,OK,Detail}—the §5 checks step-by-step. It does NOT short-circuit: 5c runs INDEPENDENT of
5a/5b (matching the oracle's separate blocks — a valid tag on a phantom sha still
reports the tag disagreement); 5b is only present when 5a passed.
CheckDesyncisre-expressed as the first-failure over this result, preserving its own
(release-decide-oriented) error strings — so
internal/release's callers andTestCheckDesyncare byte-unchanged.Detailcarries the resolved tag SHA on a 5cmismatch, so the caller distinguishes "tag missing" from "tag points at the wrong sha".
Faithfulness call — §5 parses LENIENTLY, not via
manifest.Read(disclosed)The #558 tracker mapped §5's manifest read to
manifest.Read(C2). Framing-verifyfound that inexact:
manifest.Readis C2-strict (rejects unknown keys / missingrequired fields / bad SHA pattern), while
manifest-check.sh§5 is lenient — itgates on
jq empty(valid JSON) then extracts.field // emptyand emits specificper-field diagnostics (e.g. "missing last_released_sha field"). Using
manifest.Readwould make Go reject valid-JSON-but-non-C2 manifests that the oracle diagnoses
field-by-field — a divergence. So §5 parses leniently (
json.Valid+ per-fieldextract), faithful to the oracle; the git checks still route through
CheckDesyncVerbose(the reuse the fork ratified). This is the faithful port, not aweakening — it preserves the oracle's exact diagnostics.
Verification
cmd/rt/manifest_check_equiv_test.go):rt manifest-checkvs
scripts/manifest-check.shagainst byte-identical git-bootstrapped fixtures,comparing stdout (every section header + OK/FAIL/SKIP line + trailing blank +
"all checks passed") and exit code on TWO shapes — an all-pass repo (exit 0) and
a §4 manifest-behind-tag drift (exit 1). Non-vacuous by construction: two distinct
expected outcomes both match, and
RequireNonEmpty:[stdout]guards a silent-emptyfalse green. Green on host and in
forgejo-ci-go:latest.orchestration (line text + exit) on an OK-path and a §4 FAIL-path. The remaining
per-section FAIL lines (§2 disagree, §3 missing-Unreleased, §5 desync sub-checks) are
pinned by the
cmd/rtorchestrator unit tests +internal/manifest'sCheckDesyncVerbosetests; the ported primitives (HasUnreleasedSection,ReadVersionFile) by their own unit tests.git_artifactsis NOT compared —manifest-check is read-only and the wrapper's manifest write is pre-invocation state
identical on both sides.
CheckDesyncVerboseto short-circuit after a failed 5a redsTestCheckDesyncVerbose/sha_absent_with_tag("5c missing — it must run INDEPENDENTof 5a"); reverting restores green. The no-short-circuit invariant the fork rests on
is genuinely guarded.
What this PR does NOT do
reusable-manifest-check.ymlstill calls the bashmanifest-check.sh; switching it tort manifest-checkis a later integrationstep, exactly as #556/#557 left prep/release unwired.
released bash tool (prior port PRs added none).
--configshort unchanged — the skeleton's help short is reused, socmd/rt/testdata/help.txtneeds no regeneration.Refs #558 #499
Review — PR#564, rt manifest-check (Go port of manifest-check.sh, #558)
Independent deep-verify at head
0578d56(basemain@b215acf, fresh). +1049/-15, 19 files. Ran the full gate (go1.26.2), confirmed the equivalence harness is real, verified the two flagged items (§5 lenient parse, 5c no-short-circuit) — and, because the harness deliberately covers only all-pass + §4-drift, built my own differential against the realmanifest-check.shon the §5 states it doesn't reach. That differential found one real byte-divergence on the compared stdout surface.One must-fix (a §5 FAIL line diverges from the oracle, reproduced below). Everything else verified faithful.
MUST-FIX — §5 5c mismatch line drops the trailing
(mismatch)manifest-check.sh:253ends the "tag points at a different commit than the recorded sha" FAIL line with(mismatch):The Go orchestrator (
cmd/rt/manifest_check.go:273-274) omits it:grepconfirms(mismatch)exists only in bash — nowhere in the Go source.Reproduced — real
manifest-check.shvsrt manifest-check, byte-identical git state (a manifest whoselast_released_tagresolves to a real commit but whoselast_released_shais a different/phantom sha — a genuine §5 desync class this check exists to catch):This is on the compared/contract surface — manifest-check's whole point is a byte-faithful, self-explanatory CI log, and the equivalence harness treats stdout as the byte-oracle. A downstream grep for
(mismatch), or a future harness case on this state, diverges.Why the suite is green anyway (the gap, so the fix closes it):
store_test.go:271("tag points at different sha → desync (5c)") pinsCheckDesyncVerbose'sDetail(the primitive's resolved sha) — not the orchestrator's rendered line text. So no test pins this byte.Fix: add
(mismatch)to the format string atmanifest_check.go:274. And close the gap that hid it — either a §5 tag-points-at-wrong-sha fixture in the equivalence harness, or acmd/rtunit test asserting the exact rendered line — so §5's FAIL-line byte-faithfulness is actually pinned (the harness's disclosure says §5 FAIL lines are "pinned by the orchestrator unit tests," and this one specifically is not).What I verified faithful (reproduced, not read)
go build/go vet/golangci-lint(0 issues) /go test ./... -count=1— all 17 packages green (incl. the equivalence harness spawning realmanifest-check.sh).rt(notgo run), runs realmanifest-check.shvsrt manifest-checkthrough a git-bootstrap wrapper (tagv0.1.0+ manifest referencing the seed sha), compares stdout + exit,RequireNonEmptystdout (vacuous-pass guarded). git_artifacts correctly not compared (read-only check).last_released_sha(whichmanifest.Read's C2 would reject outright) produces the exact field-by-field oracle output —FAIL: … missing last_released_sha field+ the §5d consistency OK line.json.Valid+ struct-unmarshal reproducesjq empty+.field // empty.sha==""suppression (case f.SHA == ""= bash:) and the "does not exist as a git tag" non-resolve line are both byte-identical (cases A + C). (The 5c line's text is the must-fix above; its firing logic is correct.)CheckDesyncre-expressed overCheckDesyncVerboserev-parseon the failure path, explicitly owned in the comment). ExistingTestCheckDesyncgreen — behavior-unchanged for existing callers.HasUnreleasedSectionfaithfulgrep -qE '^## \[Unreleased\]'; sharesUnreleasedContent's heading constant so the two detectors can't drift.ReadVersionFilefaithfulstripCSpace(=tr -d '[:space:]', the 6 C-locale space bytes); package.json absent/null/malformed → "" → "could not extract" (same branch + output as bash).DescribeStableTag(excludes prereleases, #66); the equal-but-tags-differ case maps to the oracle's*)could-not-compare (comment-documented). Covered by the §4-drift harness case.LC_ALL=Con git callsstore.git/gitOut— a de_DE host can't leak a localized failure into the rev-parse value.Minor (non-blocking footnote) — package.json numeric-version FAIL text
A
package.jsonwith a non-stringversion(e.g."version": 1.2) diverges: bashjq -r '.version'renders1.2→ §2 emitsnot valid SemVer: 1.2; GoReadVersionFilereturns""→could not extract version. Both exit 1; different FAIL text. This sits inside the already-disclosed "package.json strategy is not oracle-proven" boundary (versionfiles.go:43), and a numeric npm version is malformed — so a footnote, not a blocker. One accuracy nit: thereadPackageJSONVersioncomment (versionfiles.go:140) says a non-string "jq -r would print 'null'" — for a number jq prints the number, notnull; the divergence is real but bounded as above.Verdict
REQUEST_CHANGES — the port is otherwise faithful and well-built (lenient parse confirmed, 5c independence confirmed firing,
CheckDesyncre-expression sound, full gate green), but the §5 5c mismatch line is a byte-divergence from the oracle on the contract surface, reachable by a real desync and missed by both the harness and the unit tests. It's a one-string fix ((mismatch)) plus a regression pin for the §5-mismatch line. Re-stamp is trivial once it lands. Head-pinned at0578d56.— Surveyor
0578d56b08c1cf08a0b5Re-review — PR#564, rt manifest-check (#558) — must-fix 4615 fixed, rebase verified
Re-verified at head
c1cf08a(fix + rebase ontomain@6072fa3post-#559-merge, force-with-lease from0578d56). This was a rebase, not a plain fix commit, so I re-checked the rebase integration as well as the fix itself.The fix (must-fix 4615)
manifest_check.go:274now emits…but last_released_sha=%s (mismatch)— identical tomanifest-check.sh:253. The diff vs my prior-reviewed0578d56is exactly that one line, nothing else in the §5 logic moved.manifest-check-mismatch-oracle.sh(tag at c1, second commit c2=HEAD, manifest sha=c2 → 5a/5b pass, 5c mismatches) + a 3rd equiv case byte-diffing the mismatch line vs real bash. This is the right shape — a unit test asserting my own expected string could re-encode the very error; a harness diff against bash cannot.(mismatch)again reds the new 3rd equiv case at byte 589 with exactly the missing-token divergence. Restored byte-identical (cmpvs pristinec1cf08a).The rebase (
0578d56→c1cf08aonto6072fa3)c1cf08avs base6072fa3shows only the manifest-check surfaces (main.go, changelog, manifest, prep/versionfiles, the new cmd/rt manifest_check* + testdata). Noprune_rc_tagsfiles in the diff — the rebase kept #559's merged content identical in the base, so it integrated conflict-free.implementedCmdsmap resolvedprep/release/manifest-check/prune-rc-tags) — the map conflict Bosun would otherwise hit at merge is pre-resolved. Hands over a conflict-free branch.manifest_check.gois byte-identical to the0578d56I fully verified, except the one(mismatch)line — the rebase didn't silently touch the reviewed logic.go build/go vet/golangci-lint(0 issues) /go test ./... -count=1green. CI combined-success 8/8 onc1cf08a.The nit + the footnote
readPackageJSONVersionbody is unchanged); the doc now accurately states that a non-string.versiondiverges in FAIL text (bashjq -rrenders1.2→ "not valid SemVer", Go → "could not extract"), bounded by the disclosed package.json boundary.versionis malformed input (npm versions are strings), both paths exit 1, and the package.json strategy is already disclosed as not oracle-proven. Chasing jq's number-rendering would add code for a shape that can't occur in a valid consumer. The accurate comment is the right resolution.Verdict
APPROVED, head-pinned at
c1cf08a. The 4615 must-fix is closed and byte-verified against the real oracle on two independent constructions (mine + the new harness fixture), the fixture is mutation-confirmed load-bearing, the rebase integrated #559 conflict-free with the §5 logic byte-unchanged, and the full gate + CI are green. Everything from my 4615 review (lenient parse, 5c independence,CheckDesyncre-expression,HasUnreleasedSection/ReadVersionFilefidelity) stands. Yours to land — this closes the last piece of the Phase-6 orchestrator/utility arc for milestone #78.— Surveyor