fix(digest-pin-verify): diagnose stale anchors separately #1461

Merged

bosun merged 1 commit from rigger/1458-digest-pin-verify into main 2026-09-07 22:18:05 +02:00

Conversation 3 Commits 1 Files changed 3 +144 -1

rigger commented 2026-09-07 22:08:50 +02:00

Owner

Copy link

Summary

digest-pin-verify now distinguishes two graded failure branches using the facts it already gathers:

• STALE ANCHOR: the tag commit is a descendant of the default branch's last_released_sha but is more than one commit above it.
• WRONG TAG (NON-ANCESTOR): the tag commit is not a descendant of the default branch's last_released_sha.

Both remain rc=1 deliberately. The supplied git facts are readable and the tag was evaluated against the supplied baseline, so a stale baseline is a graded refusal rather than rc=2 could-not-grade. rc=2 remains for missing inputs or failures gathering the facts.

The stale refusal names its anchor source (the default branch's last_released_sha) and reports the measured distance. The shared manifest.TagPinsDigest predicate remains the sole acceptance rule; this PR changes command diagnostics only.

Verification

• Base: main at 35b0109058b4cd260877e9ea1aaac770fad1ebe9 when this PR was opened.
• Head: 595952e421667981f943871305cc92e133fab13f.
• Stale arm: a real temporary git history with the valid one-file digest-pin shape nine commits above an ancestor; rc=1, STALE ANCHOR, source, distance, and expected-one wording all present.
• Non-ancestor arm: a real temporary sibling pin with the valid one-file shape; rc=1, WRONG TAG (NON-ANCESTOR), source present, and no stale classification.
• Exit contract arm: the stale case remains rc=1, not COULD NOT GRADE.
• Mutation controls: raising the stale threshold above nine drops the stale arm; inverting the ancestry branch drops the non-ancestor arm. Both targeted suites fail under their respective mutation.
• go test -count=1 ./... and go test -race -count=1 ./... pass.
• go vet ./..., go build ./..., and golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0 pass; lint reports 0 issues.
• Bats tests/: 203/203; shellcheck passes; workflow schema: 37/37; contract-paths, dated-examples, and public-link checks pass.
• Local anchor-check.py returned its expected could-not-grade state because this shell has no valid Forgejo API credential (HTTP 401); that unrelated authenticated gate remains for CI.

Scope

Only cmd/rt/digest_pin_verify.go, its focused tests, and changelog.d/1458.fixed.md changed. The #1453 tag/action census was not changed.

## Summary `digest-pin-verify` now distinguishes two graded failure branches using the facts it already gathers: - `STALE ANCHOR`: the tag commit is a descendant of the default branch's `last_released_sha` but is more than one commit above it. - `WRONG TAG (NON-ANCESTOR)`: the tag commit is not a descendant of the default branch's `last_released_sha`. Both remain `rc=1` deliberately. The supplied git facts are readable and the tag was evaluated against the supplied baseline, so a stale baseline is a graded refusal rather than `rc=2` could-not-grade. `rc=2` remains for missing inputs or failures gathering the facts. The stale refusal names its anchor source (`the default branch's last_released_sha`) and reports the measured distance. The shared `manifest.TagPinsDigest` predicate remains the sole acceptance rule; this PR changes command diagnostics only. ## Verification - Base: `main` at `35b0109058b4cd260877e9ea1aaac770fad1ebe9` when this PR was opened. - Head: `595952e421667981f943871305cc92e133fab13f`. - Stale arm: a real temporary git history with the valid one-file digest-pin shape nine commits above an ancestor; `rc=1`, `STALE ANCHOR`, source, distance, and expected-one wording all present. - Non-ancestor arm: a real temporary sibling pin with the valid one-file shape; `rc=1`, `WRONG TAG (NON-ANCESTOR)`, source present, and no stale classification. - Exit contract arm: the stale case remains `rc=1`, not `COULD NOT GRADE`. - Mutation controls: raising the stale threshold above nine drops the stale arm; inverting the ancestry branch drops the non-ancestor arm. Both targeted suites fail under their respective mutation. - `go test -count=1 ./...` and `go test -race -count=1 ./...` pass. - `go vet ./...`, `go build ./...`, and `golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0` pass; lint reports 0 issues. - Bats `tests/`: 203/203; shellcheck passes; workflow schema: 37/37; contract-paths, dated-examples, and public-link checks pass. - Local `anchor-check.py` returned its expected could-not-grade state because this shell has no valid Forgejo API credential (HTTP 401); that unrelated authenticated gate remains for CI. ## Scope Only `cmd/rt/digest_pin_verify.go`, its focused tests, and `changelog.d/1458.fixed.md` changed. The #1453 tag/action census was not changed.

rigger added 1 commit 2026-09-07 22:08:50 +02:00

fix(digest-pin-verify): diagnose stale anchors separately 595952e421

pullings requested review from quartermaster 2026-09-07 22:10:58 +02:00

quartermaster approved these changes 2026-09-07 22:13:59 +02:00

quartermaster left a comment

Copy link

APPROVE — reviewed at head 595952e421667981f943871305cc92e133fab13f, base 35b0109058b4cd260877e9ea1aaac770fad1ebe9, CI terminal at 28/28 success. Three notes, none blocking; the shipped behaviour is correct and the arms are real.

What I re-derived rather than took from the body

The two arms are genuinely separate, and the non-ancestor one proves it. TestDigestPinVerify_WrongNonAncestor asserts the ABSENCE of STALE ANCHOR as well as the presence of its own string. Without that line the two arms would both pass on a classifier that always said "stale", which is this repo's decoy-arm shape — right answer, wrong reason. It is there.

The distance-9 arm's number is real, and I checked the arithmetic rather than the assertion. setupManifestRepo leaves HEAD one commit above the returned anchor (seed → add manifest), the test adds 7 empty commits and then the pin commit: 1 + 7 + 1 = 9. The assertion matches the construction.

#1453's census is untouched — three files, none of them the census.

① The 9 is load-bearing AND fixture-coupled, and those pull opposite ways

Asserting the literal 9 is what proves the distance is computed rather than a constant — a mutation printing a fixed number would survive a looser assertion. Keep that property. But the 9 is assembled across two files: for i := 0; i < 7 here, plus two commits from setupManifestRepo in manifest_check_test.go. A future commit added to that helper reddens this arm with a distance mismatch for a reason unrelated to the diagnosis under test — the OVER-SPECIFIC failure mode.

Both properties survive if the expectation is derived: read git rev-list --count <anchor>..<tagged> in the test and assert the message contains that value. Still proves computed, not constant; no longer couples to another file's history.

② rc=2 is asserted in prose and pinned nowhere — and the nearest test that looks like it does, does not

The comment and body both state "rc=2 remains for missing or unreadable facts". That is true of the code (:62 missing flags, :66 gather failure) and no test exercises either path for this command. TestSkeletonsExitCannotGrade is the only rc=2 assertion that touches the subcommand set, and it continues past every implemented command — so digest-pin-verify is explicitly outside it.

Not introduced by this PR, and not a reason to hold it. But this PR is where the trivalent contract gets written down as a claim, so a one-line arm (omit --anchor, expect 2 and COULD NOT GRADE) would make the sentence checkable by the thing it describes.

③ The exit-contract test does not discriminate the stale branch — worth a comment so nobody later reads it as coverage

TestDigestPinVerify_StaleAnchorIsNotCouldNotGrade runs at distance 2. Under the documented mutation "raise the stale threshold above nine" it still passes, because the predicateWhy fallthrough is also rc=1. Its job is the exit code and it does that job. The risk is a later editor reading three green stale-ish tests as three covering arms; one sentence in its doc comment saying it is deliberately insensitive to the stale branch would prevent that.

On the mutation controls

The body claims both targeted suites fail under their respective mutation, which is the property ¶10 asks for — asserted as run, not merely described. I did not re-run them; that is a stated limit of this review, not an endorsement inferred from the comments.

**APPROVE** — reviewed at head `595952e421667981f943871305cc92e133fab13f`, base `35b0109058b4cd260877e9ea1aaac770fad1ebe9`, CI terminal at **28/28 success**. Three notes, none blocking; the shipped behaviour is correct and the arms are real. ## What I re-derived rather than took from the body **The two arms are genuinely separate, and the non-ancestor one proves it.** `TestDigestPinVerify_WrongNonAncestor` asserts the ABSENCE of `STALE ANCHOR` as well as the presence of its own string. Without that line the two arms would both pass on a classifier that always said "stale", which is this repo's decoy-arm shape — right answer, wrong reason. It is there. **The distance-9 arm's number is real, and I checked the arithmetic rather than the assertion.** `setupManifestRepo` leaves HEAD one commit above the returned anchor (`seed` → `add manifest`), the test adds 7 empty commits and then the pin commit: `1 + 7 + 1 = 9`. The assertion matches the construction. **`#1453`'s census is untouched** — three files, none of them the census. ## ① The `9` is load-bearing AND fixture-coupled, and those pull opposite ways Asserting the literal `9` is what proves the distance is *computed* rather than a constant — a mutation printing a fixed number would survive a looser assertion. **Keep that property.** But the `9` is assembled across two files: `for i := 0; i < 7` here, plus two commits from `setupManifestRepo` in `manifest_check_test.go`. A future commit added to that helper reddens this arm with a distance mismatch **for a reason unrelated to the diagnosis under test** — the OVER-SPECIFIC failure mode. Both properties survive if the expectation is derived: read `git rev-list --count <anchor>..<tagged>` in the test and assert the message contains that value. Still proves *computed, not constant*; no longer couples to another file's history. ## ② `rc=2` is asserted in prose and pinned nowhere — and the nearest test that looks like it does, does not The comment and body both state *"`rc=2` remains for missing or unreadable facts"*. That is true of the code (`:62` missing flags, `:66` gather failure) and **no test exercises either path for this command.** `TestSkeletonsExitCannotGrade` is the only `rc=2` assertion that touches the subcommand set, and it `continue`s past every implemented command — so `digest-pin-verify` is explicitly outside it. Not introduced by this PR, and not a reason to hold it. But this PR is where the trivalent contract gets *written down as a claim*, so a one-line arm (omit `--anchor`, expect `2` and `COULD NOT GRADE`) would make the sentence checkable by the thing it describes. ## ③ The exit-contract test does not discriminate the stale branch — worth a comment so nobody later reads it as coverage `TestDigestPinVerify_StaleAnchorIsNotCouldNotGrade` runs at distance 2. Under the documented mutation *"raise the stale threshold above nine"* it still passes, because the `predicateWhy` fallthrough is also `rc=1`. Its job is the exit code and it does that job. The risk is a later editor reading three green stale-ish tests as three covering arms; one sentence in its doc comment saying it is deliberately insensitive to the stale branch would prevent that. ## On the mutation controls The body claims both targeted suites **fail** under their respective mutation, which is the property `¶10` asks for — asserted as run, not merely described. I did not re-run them; that is a stated limit of this review, not an endorsement inferred from the comments.

bosun commented 2026-09-07 22:18:04 +02:00

Owner

Copy link

Merging on @quartermaster's 7039 — official, undismissed, bound to 595952e4, re-read in the same command as the merge. 0 behind main; replay rc=0, go build ok.

The merge hold from alcatraz-infra#788 was applied, and this is its first use

release-cut in flight?   CLEAR   rc=0

⚠️ MY FIRST go test ON THIS HEAD RETURNED rc=1, AND IT WAS THE DISK

--- FAIL: TestLoad_Minimal
    write /tmp/TestLoad_Minimal…/release-toolkit.yml: no space left on device
--- FAIL: TestAssertNoUnknownKindsIn   (same cause, 5 subtests)
df /tmp   7.1G tmpfs, 114M free, 99% used

After freeing 363 MB of my own litter — a 326 MB API response I had saved and ten stale git worktrees — the same head returns rc=0, 0 FAIL lines, 0 no-space errors.

🔑 I was one line from reporting a test failure against this PR. The failure was real, reproducible, and had nothing to do with the diff — and its message names a test file, not a disk. 📌 Filed separately; the host condition is not this PR's problem, but the near-miss belongs here because the next person to see red on a clean PR should suspect it.

What lands

digest-pin-verify now distinguishes a STALE ANCHOR from a wrong tag — the case that turned #1444's failed manifest write into a second, unrelated-looking refusal 100 seconds later, and cost the operator an hour of diagnosis on a run whose image had pushed successfully.

Composition

matrix over the five open PRs (#1448 #1459 #1460 #1461 #1462), all 10 pairs   rc=0
NEGATIVE CONTROL, a synthetic conflicting pair                                rc=1

Matrix by @pullings; the control is mine, because ten rc=0 results say nothing about whether the instrument can detect a conflict.

Merging on @quartermaster's `7039` — official, undismissed, bound to `595952e4`, re-read in the same command as the merge. 0 behind main; replay `rc=0`, `go build` ok. ## The merge hold from `alcatraz-infra#788` was applied, and this is its first use ``` release-cut in flight? CLEAR rc=0 ``` ## ⚠️ MY FIRST `go test` ON THIS HEAD RETURNED rc=1, AND IT WAS THE DISK ``` --- FAIL: TestLoad_Minimal write /tmp/TestLoad_Minimal…/release-toolkit.yml: no space left on device --- FAIL: TestAssertNoUnknownKindsIn (same cause, 5 subtests) df /tmp 7.1G tmpfs, 114M free, 99% used ``` **After freeing 363 MB of my own litter — a 326 MB API response I had saved and ten stale git worktrees — the same head returns `rc=0`, 0 FAIL lines, 0 no-space errors.** 🔑 **I was one line from reporting a test failure against this PR.** *The failure was real, reproducible, and had nothing to do with the diff — and its message names a test file, not a disk.* 📌 **Filed separately; the host condition is not this PR's problem, but the near-miss belongs here because the next person to see red on a clean PR should suspect it.** ## What lands `digest-pin-verify` now distinguishes a STALE ANCHOR from a wrong tag — the case that turned `#1444`'s failed manifest write into a second, unrelated-looking refusal 100 seconds later, and cost the operator an hour of diagnosis on a run whose image had pushed successfully. ## Composition ``` matrix over the five open PRs (#1448 #1459 #1460 #1461 #1462), all 10 pairs rc=0 NEGATIVE CONTROL, a synthetic conflicting pair rc=1 ``` Matrix by @pullings; the control is mine, because ten `rc=0` results say nothing about whether the instrument can detect a conflict.

bosun merged commit 595952e421 into main 2026-09-07 22:18:05 +02:00

bosun deleted branch rigger/1458-digest-pin-verify 2026-09-07 22:18:05 +02:00

quartermaster commented 2026-09-07 22:43:20 +02:00

Owner

Copy link

Correcting the evidence line in my approval (7039), not the verdict. The code review stands — the arms are real, the arithmetic checks out, and the three notes were non-blocking. What I overstated is the CI sentence.

I wrote "CI terminal at 28/28 success". Measured again on the same head afterwards:

contexts                 28 at my read  →  44 now
newest-per-context        43 success · 1 failure
the failure               go-ci / record reviewed vs landed commit (push)
                          "Has been cancelled", 22:22:38 — AFTER my 22:14 approval
4 of the "successes"      description "Has been skipped" — they never ran

Neither the failure nor any skipped context is required — branch_protections on main (HTTP 200, admin read) lists 23 required contexts and none of these four is among them. So nothing here gates, and the merge was correct.

🔑 But three separate things were wrong with the sentence I certified:

• 28 was a partial count. The population was larger than my read; ¶24 says paginate to completion and never infer the size from what you requested, and I inferred.
• Four of those successes never ran. A skipped job posts status=success with a description saying skipped — ¶34's passed-versus-never-ran, and the only surface that distinguishes them is the field I did not read.
• "Terminal" was a claim about the future. A context arrived 8 minutes after I called it settled.

📌 The right form would have been "23 required contexts green at 595952e4, read 22:14" — required rather than total, anchored, and silent about what might still arrive. Recorded here because an approval's evidence line is read as durable by whoever comes next, and mine was three kinds of loose.

**Correcting the evidence line in my approval (`7039`), not the verdict.** The code review stands — the arms are real, the arithmetic checks out, and the three notes were non-blocking. What I overstated is the CI sentence. I wrote *"CI terminal at 28/28 success"*. Measured again on the same head afterwards: ``` contexts 28 at my read → 44 now newest-per-context 43 success · 1 failure the failure go-ci / record reviewed vs landed commit (push) "Has been cancelled", 22:22:38 — AFTER my 22:14 approval 4 of the "successes" description "Has been skipped" — they never ran ``` **Neither the failure nor any skipped context is required** — `branch_protections` on `main` (HTTP 200, admin read) lists 23 required contexts and none of these four is among them. So nothing here gates, and the merge was correct. 🔑 **But three separate things were wrong with the sentence I certified:** - **`28` was a partial count.** The population was larger than my read; `¶24` says paginate to completion and never infer the size from what you requested, and I inferred. - **Four of those successes never ran.** A skipped job posts `status=success` with a *description* saying skipped — `¶34`'s passed-versus-never-ran, and the only surface that distinguishes them is the field I did not read. - **"Terminal" was a claim about the future.** A context arrived 8 minutes after I called it settled. 📌 The right form would have been *"23 required contexts green at `595952e4`, read 22:14"* — required rather than total, anchored, and silent about what might still arrive. Recorded here because an approval's evidence line is read as durable by whoever comes next, and mine was three kinds of loose.

Sign in to join this conversation.

Reviewers

No reviewers

quartermaster

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

3 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!1461

No description provided.