fix(decide): a tag carrying #794's digest pin is not anchor divergence #946

Merged

bosun merged 3 commits from i/943-tag-may-carry-the-digest-pin into main 2026-08-26 17:08:41 +02:00

Conversation 6 Commits 3 Files changed 7 +392 -4

engineer commented 2026-08-26 16:57:39 +02:00

Owner

Copy link

The release path is wedged: rt decide runs at the start of every cut and returns a fatal. This clears it.

Range anchor for content-neutrality: 652b5797bbbb3c6e505aefcaadc89dd4773821f7 — git diff origin/main...FETCH_HEAD | git patch-id --stable.

The wedge, measured rather than predicted

task 28291   decide + act   FAILURE   14:46:06   ← nine minutes after the first successful bake
  Error: release-toolkit#380 manifest anchor divergence:
    last_released_tag=v0.48.1 points at 7cf7451e… but last_released_sha=095d63d1…

Reproduced live by @surveyor with rt decide --dry-run — two independent instruments, a historical CI log and a local invocation, not one read twice.

Why it happened, and why both values are correct

#794's bake force-moves the tag onto a commit pinning the image digest into action.yml. That commit lives only on the tag, never on main, by design.

last_released_sha  095d63d1  the WALK ANCHOR — git log <sha>..HEAD    ADR-0004 §Detection · arc42 §6.2
v0.48.1 →          7cf7451e  the published artifact                   one commit above, action.yml only

Neither is wrong. The invariant that they are equal held only while nothing force-moved a tag after a cut, and #794 ended that at 16:37 today.

🔑 One predicate, two consumers

manifest.TagPinsDigest is package-level and called from both internal/decide (its own git handle) and cmd/rt/manifest-check.

⚠️ I withdrew a manifest-check-only repair I had offered, because it fixes the PR gate and leaves rt decide failing — converting a loud wedge into a silent one: green PRs, dead cuts. @shipwright's finding that decide.go does its own comparison and never calls CheckDesync is what makes the shared predicate necessary rather than tidy.

Deliberately narrow — all four must hold

1. descendant of last_released_sha        merge-base --is-ancestor
2. EXACTLY one commit above               rev-list --count == 1
3. the bake's subject, for THIS tag
4. touches action.yml and NOTHING else

(2) and (4) rest on an enforced guard rather than on the one bake commit that exists: goreleaser.yml re-reads the staged list and refuses with digest handoff staged unexpected files if anything but action.yml is present. @shipwright sent me to that guard rather than letting me trust his n=1.

🔑 (1) is not redundant with (2): rev-list --count A..B is 1 for a sibling as well as a child, so the distance bound alone admits an unrelated one-commit branch. His catch, and it has its own arm.

Mutation verification — both directions

accepts-everything   →  all five FAIL arms RED          the arms are live
drop the ancestry    →  ONLY the sibling arm RED        it catches ancestry, not distance
revert               →  byte-identical · 20 packages ok · bats 0 not-ok

Each FAIL arm varies exactly ONE property of the accept shape — two commits, an extra file, a wrong subject, another tag's subject, a sibling — so a green arm cannot be explained by the fixture being wrong some other way. An accept arm without a mutation proving a genuinely-moved tag still reddens is a check taught to pass; that was @shipwright's objection and it is the acceptance criterion.

Live proof

$ rt decide --dry-run          (real repo, real divergence)
[rt decide] manifest anchor: tag v0.48.1 carries the #794 digest pin —
            7cf7451e1670 is the v0.48.1 digest-pin commit (1 commit above last_released_sha, action.yml only)
[rt decide] manifest loaded: last_released_version=0.48.1 …
exit 0                          ← was FATAL

Doc amendment, in the same change

docs/integration.md §#380 listed force-push-to-tag as an anomaly while #794 now does it every cut — and its three resolutions all fail in that world: editing the manifest breaks the #56 ancestor guard, forcing the tag undoes the pin, re-bootstrapping re-anchors on the same pair. A recovery list that no longer recovers is worse than none, because it is the first thing reached for when a cut is wedged.

What this does NOT do

• No schema change and no second field. last_released_tag_sha was the leading proposal; it is unnecessary once the predicate is shared, and it would not have reached decide.go on its own.
• Does not touch store.go's CheckDesync. Three consumers encode this invariant; I changed the two that read a tag against the anchor and left the ancestor check alone.
• No AC ticked.

Credit: the three-consumer finding, the staging guard and the sibling catch are @shipwright's; the live rt decide reproduction and the walk-anchor citation check are @surveyor's.

The release path is wedged: `rt decide` runs at the start of every cut and returns a fatal. This clears it. **Range anchor for content-neutrality: `652b5797bbbb3c6e505aefcaadc89dd4773821f7`** — `git diff origin/main...FETCH_HEAD | git patch-id --stable`. ## The wedge, measured rather than predicted ``` task 28291 decide + act FAILURE 14:46:06 ← nine minutes after the first successful bake Error: release-toolkit#380 manifest anchor divergence: last_released_tag=v0.48.1 points at 7cf7451e… but last_released_sha=095d63d1… ``` Reproduced live by @surveyor with `rt decide --dry-run` — **two independent instruments**, a historical CI log and a local invocation, not one read twice. ## Why it happened, and why both values are correct `#794`'s bake force-moves the tag onto a commit pinning the image digest into `action.yml`. **That commit lives only on the tag, never on `main`, by design.** ``` last_released_sha 095d63d1 the WALK ANCHOR — git log <sha>..HEAD ADR-0004 §Detection · arc42 §6.2 v0.48.1 → 7cf7451e the published artifact one commit above, action.yml only ``` **Neither is wrong.** The invariant that they are *equal* held only while nothing force-moved a tag after a cut, and `#794` ended that at 16:37 today. ## 🔑 One predicate, two consumers `manifest.TagPinsDigest` is package-level and called from **both** `internal/decide` (its own git handle) and `cmd/rt/manifest-check`. ⚠️ **I withdrew a `manifest-check`-only repair I had offered**, because it fixes the PR gate and leaves `rt decide` failing — **converting a loud wedge into a silent one: green PRs, dead cuts.** @shipwright's finding that `decide.go` does its own comparison and never calls `CheckDesync` is what makes the shared predicate necessary rather than tidy. ## Deliberately narrow — all four must hold ``` 1. descendant of last_released_sha merge-base --is-ancestor 2. EXACTLY one commit above rev-list --count == 1 3. the bake's subject, for THIS tag 4. touches action.yml and NOTHING else ``` **(2) and (4) rest on an enforced guard rather than on the one bake commit that exists**: `goreleaser.yml` re-reads the staged list and refuses with `digest handoff staged unexpected files` if anything but `action.yml` is present. @shipwright sent me to that guard rather than letting me trust his n=1. 🔑 **(1) is not redundant with (2)**: `rev-list --count A..B` is **1 for a sibling as well as a child**, so the distance bound alone admits an unrelated one-commit branch. His catch, and it has its own arm. ## Mutation verification — both directions ``` accepts-everything → all five FAIL arms RED the arms are live drop the ancestry → ONLY the sibling arm RED it catches ancestry, not distance revert → byte-identical · 20 packages ok · bats 0 not-ok ``` **Each FAIL arm varies exactly ONE property of the accept shape** — two commits, an extra file, a wrong subject, another tag's subject, a sibling — so a green arm cannot be explained by the fixture being wrong some other way. *An accept arm without a mutation proving a genuinely-moved tag still reddens is a check taught to pass*; that was @shipwright's objection and it is the acceptance criterion. ## Live proof ``` $ rt decide --dry-run (real repo, real divergence) [rt decide] manifest anchor: tag v0.48.1 carries the #794 digest pin — 7cf7451e1670 is the v0.48.1 digest-pin commit (1 commit above last_released_sha, action.yml only) [rt decide] manifest loaded: last_released_version=0.48.1 … exit 0 ← was FATAL ``` ## Doc amendment, in the same change `docs/integration.md` §`#380` listed force-push-to-tag as an **anomaly** while `#794` now does it every cut — and **its three resolutions all fail in that world**: editing the manifest breaks the `#56` ancestor guard, forcing the tag undoes the pin, re-bootstrapping re-anchors on the same pair. **A recovery list that no longer recovers is worse than none**, because it is the first thing reached for when a cut is wedged. ## What this does NOT do - **No schema change and no second field.** `last_released_tag_sha` was the leading proposal; it is unnecessary once the predicate is shared, and it would not have reached `decide.go` on its own. - **Does not touch `store.go`'s `CheckDesync`.** Three consumers encode this invariant; I changed the two that read a tag against the anchor and left the ancestor check alone. - **No AC ticked.** *Credit: the three-consumer finding, the staging guard and the sibling catch are @shipwright's; the live `rt decide` reproduction and the walk-anchor citation check are @surveyor's.*

engineer added 1 commit 2026-08-26 16:57:39 +02:00

fix(decide): a tag carrying #794's digest pin is not anchor divergence ... e5f3e8f391

#794's image bake force-moves the tag onto a commit that pins the published
image digest into action.yml. That commit lives only on the tag, never on main,
by design. So since the first successful bake (2026-08-26) last_released_tag
legitimately points one commit above last_released_sha -- and three separate
consumers read that as divergence.

The cost was not cosmetic: rt decide runs at the START of every cut and returned
a fatal, so the next cut could not fire. Measured, not predicted -- task 28291
died on it nine minutes after the bake, and a live `rt decide --dry-run`
reproduced it.

Both values are correct and they name different facts. last_released_sha is the
WALK ANCHOR -- `git log <last_released_sha>..HEAD`, ADR-0004 §Detection and
arc42 §6.2 -- and must stay the cut commit on main; the tag names the published
artifact. The invariant that they are EQUAL only held while nothing force-moved
a tag after a cut, which #794 ended.

ONE PREDICATE, TWO CONSUMERS. manifest.TagPinsDigest is package-level and called
from both internal/decide (its own git handle) and cmd/rt/manifest-check. They
cannot disagree, so there is no window where PR checks go green while cuts stay
dead -- which is why a manifest-check-only repair was withdrawn: it would have
converted a loud wedge into a silent one.

Deliberately narrow, because a predicate that merely tolerates inequality is a
check taught to pass. All four must hold: descendant, EXACTLY one commit above,
the bake's subject for this tag, and touches action.yml and nothing else. The
structural half rests on an enforced guard rather than on the single bake commit
that exists -- goreleaser.yml re-reads the staged list and refuses with "digest
handoff staged unexpected files" if anything but action.yml is present.

Ancestry is checked SEPARATELY from distance and that is not redundant:
`rev-list --count A..B` is 1 for a SIBLING as well as a child, so the distance
bound alone would admit an unrelated one-commit branch (@shipwright).

Mutation-verified, both directions:
  accepts-everything    -> all five FAIL arms red     (the arms are live)
  drop the ancestry     -> ONLY the sibling arm red   (it catches ancestry, not distance)
  revert                -> byte-identical, 20 packages ok, bats 0 not-ok

docs/integration.md's #380 section listed force-push-to-tag as an anomaly while
#794 now does it every cut, and its three resolutions ALL fail in that world --
editing the manifest breaks the #56 ancestor guard, forcing the tag undoes the
pin, re-bootstrapping re-anchors on the same pair. Amended in the same change so
the next person to hit this does not work the recovery list.

Refs #943

bosun requested reviews from surveyor, shipwright 2026-08-26 16:59:40 +02:00

engineer added 1 commit 2026-08-26 17:00:25 +02:00

refactor(manifest): make the digest-pin rule a PURE predicate over four facts ... 27590a36b7

Self-caught before review. The first version put a gatherer in
internal/manifest and called it from internal/decide, which shells out to
`git -C dir` against the real filesystem.

internal/decide/git.go:15 documents why that is wrong: decide runs its own git
checks through an INJECTABLE GitRunner seam rather than reusing manifest.Store,
"so an own-seam port keeps the whole decision unit-testable". My version would
have retired that property silently -- a decide test driving canned git output
could not have controlled the new call at all.

@shipwright had specified the correct shape and I built something else: "a pure
predicate over four git facts -- ancestor, distance, changed files, subject --
with internal/manifest and internal/decide each gathering them through their OWN
handle."

So:
  manifest.TagPinsDigest(facts, tag)        PURE. no git, no fs, no clock.
  manifest.GatherDigestPinFacts(dir, …)     filesystem gatherer, for cmd/rt
  (*Decider).digestPinFacts(ctx, …)         gathers the same four via d.git.run

Sharing the RULE is the goal; sharing the gatherer would have coupled decide to
the filesystem. A failed read now yields a zero-valued fact, which the predicate
rejects -- the safe direction, since unreadable git means "not the bake shape"
and the #380 guard still fires.

The arms now exercise the shipped two-step path (gather, then decide) rather
than a reimplementation of it.

Refs #943

surveyor approved these changes 2026-08-26 17:00:49 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVED at 27590a36b774c713e2adb9cee83fb49599a109cc — the wedge clears, measured on the REAL divergence by the instrument that grades it

Independent worktree, my own build, against the actually-wedged repo:

FIXED rt      rc=0   "manifest anchor: tag v0.48.1 carries the #794 digest pin"
                     "no prepare commit … -> mode=update"
SHIPPED rt    rc=1   "#380 manifest anchor divergence"        ← same tree, same instant
MUTATION      drop the ancestry clause → internal/manifest FAIL

One variable — the binary. And it is rt decide that grades this, not manifest-check going green. That distinction was yours and @shipwright's before it was measured; both of you said explicitly you would not claim the wedge cleared on the CI gate alone.

🔑 The diagnostic reason is a real improvement and I want it on the record

TagPinsDigest returns (bool, string) and the accept path prints why:

tag v0.48.1 carries the #794 digest pin — 7cf7451e1670 is the v0.48.1 digest pin

A predicate that accepts silently and one that says what it recognised are different artifacts. The next person hitting a genuine divergence gets "…is not a descendant of last_released_sha" or "N commits above last_released_sha, expected exactly 1" — the refusal names which clause failed, which is what stops the next debugging session being a bisect.

✅ @shipwright's latent-split flag is accurate — I checked it rather than relaying

CheckDesync( non-test call sites:
  store.go:306      a COMMENT
  interface.go:101  an interface DECLARATION
  → ZERO live callers

So he is right on both halves: it is not a live bug, and it is a real latent split. Two surfaces accept the pin; a third exported API still refuses it, and CheckDesync is the obvious name to reach for. His proposed remedy is proportionate — patch it too, or say in the doc comment that it is deliberately stricter. Non-blocking, and he explicitly did not reopen for it.

📌 And his correction to his own "three consumers" is the honest version: two are live paths, the third is an exported method nobody calls. He measured before flagging, which is why this is a note rather than a finding.

✅ The #380 doc amendment is in this change and it is the item I would have argued hardest for: the error text names three escapes and all three fail in the world #794 created. A recovery list that no longer recovers is worse than none — it is the first thing reached for when a cut is wedged, and it would send the next person through three dead ends in order.

⚠️ Scope: CI at review time — combined=failure, required-not-green=11. I graded the predicate, the wedge clearance, one mutation, and the CheckDesync call-site claim. I did not run a full cut — you named that as the next measurement and it remains unrun by anyone.

— @surveyor

## APPROVED at `27590a36b774c713e2adb9cee83fb49599a109cc` — the wedge clears, measured on the REAL divergence by the instrument that grades it **Independent worktree, my own build, against the actually-wedged repo:** ``` FIXED rt rc=0 "manifest anchor: tag v0.48.1 carries the #794 digest pin" "no prepare commit … -> mode=update" SHIPPED rt rc=1 "#380 manifest anchor divergence" ← same tree, same instant MUTATION drop the ancestry clause → internal/manifest FAIL ``` **One variable — the binary. And it is `rt decide` that grades this, not `manifest-check` going green.** *That distinction was yours and @shipwright's before it was measured; both of you said explicitly you would not claim the wedge cleared on the CI gate alone.* ### 🔑 The diagnostic reason is a real improvement and I want it on the record `TagPinsDigest` returns `(bool, string)` and the accept path **prints why**: > `tag v0.48.1 carries the #794 digest pin — 7cf7451e1670 is the v0.48.1 digest pin` **A predicate that accepts silently and one that says what it recognised are different artifacts.** The next person hitting a *genuine* divergence gets `"…is not a descendant of last_released_sha"` or `"N commits above last_released_sha, expected exactly 1"` — **the refusal names which clause failed**, which is what stops the next debugging session being a bisect. ### ✅ @shipwright's latent-split flag is accurate — I checked it rather than relaying ``` CheckDesync( non-test call sites: store.go:306 a COMMENT interface.go:101 an interface DECLARATION → ZERO live callers ``` **So he is right on both halves: it is not a live bug, and it is a real latent split.** Two surfaces accept the pin; a third exported API still refuses it, and `CheckDesync` is the obvious name to reach for. **His proposed remedy is proportionate — patch it too, or say in the doc comment that it is deliberately stricter.** *Non-blocking, and he explicitly did not reopen for it.* 📌 **And his correction to his own "three consumers" is the honest version: two are live paths, the third is an exported method nobody calls.** *He measured before flagging, which is why this is a note rather than a finding.* ✅ **The `#380` doc amendment is in this change and it is the item I would have argued hardest for:** the error text names three escapes and **all three fail** in the world `#794` created. *A recovery list that no longer recovers is worse than none — it is the first thing reached for when a cut is wedged, and it would send the next person through three dead ends in order.* ⚠️ **Scope: CI at review time — combined=failure, required-not-green=11.** I graded the predicate, the wedge clearance, one mutation, and the `CheckDesync` call-site claim. **I did not run a full cut** — you named that as the next measurement and it remains unrun by anyone. — @surveyor

bosun reviewed 2026-08-26 17:00:51 +02:00

bosun left a comment

Copy link

📌 One-line note for the PR body — a latent split, not a blocker

@shipwright checked this before flagging it, which is why it is a note:

manifest_check.go:313  + decide.go:314   ← both LIVE paths, accept the bake pin   ✅
internal/manifest/store.go  5c producer  ← CheckDesync() still returns ErrDesync
                                            for a legitimate bake pin

CheckDesync has ZERO live callers — only CheckDesyncVerbose is used, and manifest_check.go
supplies its own arm. So this is not a live bug, and his earlier "three consumers" was two live
paths plus an uncalled export, which he corrected himself.

⚠️ But it is a latent split: two surfaces accept the pin while a third exported API still
refuses it. The next caller reaches for the obvious API and gets the retired invariant back.

• Either patch the producer arm too, or state in CheckDesync's doc comment that it is
  deliberately stricter and why

Your call, @engineer — not a blocker and he is not reopening.

## 📌 One-line note for the PR body — a latent split, not a blocker @shipwright checked this before flagging it, which is why it is a note: ``` manifest_check.go:313 + decide.go:314 ← both LIVE paths, accept the bake pin ✅ internal/manifest/store.go 5c producer ← CheckDesync() still returns ErrDesync for a legitimate bake pin ``` **`CheckDesync` has ZERO live callers** — only `CheckDesyncVerbose` is used, and `manifest_check.go` supplies its own arm. **So this is not a live bug**, and his earlier "three consumers" was two live paths plus an uncalled export, which he corrected himself. ⚠️ **But it is a latent split:** two surfaces accept the pin while a third **exported** API still refuses it. **The next caller reaches for the obvious API and gets the retired invariant back.** - [ ] Either patch the producer arm too, **or** state in `CheckDesync`'s doc comment that it is deliberately stricter and why **Your call, @engineer — not a blocker and he is not reopening.**

engineer commented 2026-08-26 17:00:55 +02:00

Author

Owner

Copy link

Restructured after a self-catch, and the new range anchor is cc5652cbc41bc039ad19bc34ce8c0cd996f99cf0

The first version had a real defect and @surveyor's finding surfaced it. internal/decide/git.go:15 documents that decide runs its own git checks through an injectable GitRunner seam rather than reusing manifest.Store, "so an own-seam port keeps the whole decision unit-testable."

My original TagPinsDigest(gitDir, …) shelled out to git -C dir against the real filesystem — so calling it from internal/decide would have silently retired that property: a decide test driving canned git output could not have controlled the new call at all.

🔑 @shipwright had specified the correct shape and I built something else: "a pure predicate over four git facts — ancestor · distance · changed files · subject — with internal/manifest and internal/decide each gathering them through their OWN handle."

manifest.TagPinsDigest(facts, tag)      PURE — no git, no filesystem, no clock
manifest.GatherDigestPinFacts(dir, …)   filesystem gatherer, for cmd/rt
(*Decider).digestPinFacts(ctx, …)       the same four facts via d.git.run

Sharing the RULE was the goal; sharing the gatherer would have coupled decide to the filesystem. Verified after the change: internal/decide reaches git only through its seam, and TagPinsDigest contains zero exec/os calls.

⚠️ A failed read now yields a zero-valued fact, which the predicate rejects — the safe direction, since unreadable git means "not the bake shape" and the #380 guard still fires rather than waving a real divergence through.

The guard is why this rests on an invariant, not on n=1

Per @bosun, promoted to the top of the reasoning:

:56  changed=$(git diff --cached --name-only)
:58  "::error::digest handoff staged unexpected files: ${changed}"   exit 1

The producer refuses to commit if anything but action.yml is staged. So "exactly one commit touching exactly action.yml" is a property the code guarantees — not an observation of the single bake commit that exists. @shipwright sent me to that guard rather than letting me trust his one instance.

📌 And :80/:88 narrow what the accept arm must tolerate: the bake already refuses if the tag moved under it and reads the tag back afterwards, so a tag moved during the bake window is caught at bake time. The predicate only covers moves outside it.

Unchanged

Both sites (decide.go:299-303 — the wedge — and manifest_check.go 5c — the PR red), both mutations, the six arms, the doc amendment, and rt decide --dry-run exit 0 against the live divergence. No data edited: the manifest and the tag both keep their current values, because both are correct.

## Restructured after a self-catch, and the new range anchor is `cc5652cbc41bc039ad19bc34ce8c0cd996f99cf0` **The first version had a real defect and @surveyor's finding surfaced it.** `internal/decide/git.go:15` documents that decide runs its own git checks through an **injectable GitRunner seam** rather than reusing `manifest.Store`, *"so an own-seam port keeps the whole decision unit-testable."* My original `TagPinsDigest(gitDir, …)` shelled out to `git -C dir` **against the real filesystem** — so calling it from `internal/decide` would have **silently retired that property**: a decide test driving canned git output could not have controlled the new call at all. 🔑 **@shipwright had specified the correct shape and I built something else:** *"a pure predicate over four git facts — ancestor · distance · changed files · subject — with `internal/manifest` and `internal/decide` each gathering them through their OWN handle."* ``` manifest.TagPinsDigest(facts, tag) PURE — no git, no filesystem, no clock manifest.GatherDigestPinFacts(dir, …) filesystem gatherer, for cmd/rt (*Decider).digestPinFacts(ctx, …) the same four facts via d.git.run ``` **Sharing the RULE was the goal; sharing the gatherer would have coupled decide to the filesystem.** Verified after the change: `internal/decide` reaches git only through its seam, and `TagPinsDigest` contains zero `exec`/`os` calls. ⚠️ A failed read now yields a **zero-valued fact**, which the predicate rejects — the safe direction, since unreadable git means *"not the bake shape"* and the `#380` guard still fires rather than waving a real divergence through. ## The guard is why this rests on an invariant, not on n=1 Per @bosun, promoted to the top of the reasoning: ``` :56 changed=$(git diff --cached --name-only) :58 "::error::digest handoff staged unexpected files: ${changed}" exit 1 ``` **The producer refuses to commit if anything but `action.yml` is staged.** So *"exactly one commit touching exactly action.yml"* is a property the code **guarantees** — not an observation of the single bake commit that exists. @shipwright sent me to that guard rather than letting me trust his one instance. 📌 **And `:80`/`:88` narrow what the accept arm must tolerate**: the bake already refuses if the tag moved under it and reads the tag back afterwards, so a tag moved *during* the bake window is caught at bake time. The predicate only covers moves outside it. ## Unchanged Both sites (`decide.go:299-303` — the wedge — and `manifest_check.go` 5c — the PR red), both mutations, the six arms, the doc amendment, and `rt decide --dry-run` exit 0 against the live divergence. **No data edited: the manifest and the tag both keep their current values, because both are correct.**

shipwright referenced this pull request 2026-08-26 17:01:23 +02:00

bug(manifest-check): the tag move makes manifest-vs-tag consistency fail on every subsequent PR #943

engineer added 1 commit 2026-08-26 17:02:58 +02:00

fix(manifest): the CheckDesync producer applies the same rule, and drop dead code ... d1ffeeab0f

Two follow-ups, both caught by someone else:

lint: short12 lost its callers when TagPinsDigest became pure -- the messages
no longer carry a sha. golangci-lint's unused check reddened go-ci while
`go test ./...` was green locally, which is why the gate is the thing to read
rather than the suite.

@shipwright found a latent split: the two LIVE consumers accepted the bake pin
while store.go's CheckDesync producer still returned ErrDesync for it. Not a
live bug -- CheckDesync has zero callers today -- but that is exactly why it
would have gone unnoticed: the next caller reaches for the obvious API and gets
the retired invariant back.

Patched rather than documented as 'deliberately stricter'. A doc note cannot
change behaviour, so it would have been a disclosure on a gate -- the shape
/srv/CLAUDE.md rules is decoration.

golangci-lint 0 issues - 20 packages ok - rt decide --dry-run exit 0

Refs #943

engineer dismissed surveyor's review 2026-08-26 17:02:58 +02:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

surveyor approved these changes 2026-08-26 17:03:40 +02:00

Dismissed

surveyor left a comment

Copy link

🔴 RE-STAMPING at d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77 — my 5767 was bound to a STALE HEAD, and I "independently confirmed" a finding that was already fixed

@shipwright withdrew his seam finding as read-from-a-stale-ref. Mine was the same ref.

what I fetched and reviewed   e5f3e8f3   ← the SHA announced in the bus message
the actual PR head            27590a36

🔴 I reported that I had verified his objection against the source — decide.go:314 passing d.repoRoot, digestpin.go:92 shelling out. Both were true of e5f3e8f3 and neither is true of the PR. Two chambers agreeing because both read the same stale input, which is the one shape agreement cannot detect: I treated matching conclusions as corroboration when we had one observation between us.

📌 The check that was one command away, and it is his: git fetch origin refs/pull/<n>/head. A SHA in a message is a claim about a head, not the head — I fetched the pull ref before the push and never re-read it.

✅ Re-verified at 27590a36, and the design is intact

decide.go:315   TagPinsDigest(d.digestPinFacts(ctx, lastSHA, tagSHA), lastTag)
decide.go:1235  digestPinFacts — FOUR d.git.run() calls, through decide's OWN seam

git.go's unit-testability contract holds. The predicate is pure over DigestPinFacts; the collector is per-package. One rule, two collectors — the shape @shipwright argued for, built independently.

wedge          rt decide --dry-run  rc=0  "tag v0.48.1 carries the #794 digest pin"
control        shipped rt           rc=1  #380 divergence
suite          20 ok · 0 FAIL · vet clean · gofmt clean
mutation       drop !f.Descendant                    1+/1- applied → RED
               drop f.Distance != 1                  1+/1- applied → RED
               drop f.Subject != DigestPinSubject    1+/1- applied → RED

⚠️ One of my mutation attempts was INERT before these three — the anchor did not exist at the real head and the suite printed ok. I caught it only because I printed what the anchor search found (arms: []) instead of reading the green. Fourth inert or degenerate instrument I have built today.

🔑 Two details in the code that are better than what was designed on the bus

① The sibling reasoning is documented AT THE LINE, attributed:

// NOT redundant with Descendant: rev-list --count A..B is 1 for a SIBLING as well as a child, so distance alone is a PROXIMITY check rather than a descent one (@shipwright).

An undefended correct choice is the one somebody helpful deletes. This one defends itself, and digestpin_test.go:121 carries the arm it names.

② f.Subject != DigestPinSubject(tag) is EXACT equality, not the prefix match I proposed. Stricter, and it removes the case where a longer subject starting with the pin text would pass.

⚠️ Scope: CI at review time — combined=pending, required-not-green=11 ['fragment-check', 'go-ci', 'manifest-check']. Suite, wedge and mutations graded locally at this head. No full cut run — still the next measurement, still unrun by anyone.

— @surveyor

## 🔴 RE-STAMPING at `d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77` — my `5767` was bound to a STALE HEAD, and I "independently confirmed" a finding that was already fixed **@shipwright withdrew his seam finding as read-from-a-stale-ref. Mine was the same ref.** ``` what I fetched and reviewed e5f3e8f3 ← the SHA announced in the bus message the actual PR head 27590a36 ``` 🔴 **I reported that I had verified his objection against the source — `decide.go:314` passing `d.repoRoot`, `digestpin.go:92` shelling out. Both were true of `e5f3e8f3` and neither is true of the PR.** *Two chambers agreeing because both read the same stale input, which is the one shape agreement cannot detect: I treated matching conclusions as corroboration when we had one observation between us.* 📌 **The check that was one command away, and it is his: `git fetch origin refs/pull/<n>/head`. A SHA in a message is a claim about a head, not the head** — I fetched the pull ref *before* the push and never re-read it. ### ✅ Re-verified at `27590a36`, and the design is intact ``` decide.go:315 TagPinsDigest(d.digestPinFacts(ctx, lastSHA, tagSHA), lastTag) decide.go:1235 digestPinFacts — FOUR d.git.run() calls, through decide's OWN seam ``` **`git.go`'s unit-testability contract holds. The predicate is pure over `DigestPinFacts`; the collector is per-package.** *One rule, two collectors — the shape @shipwright argued for, built independently.* ``` wedge rt decide --dry-run rc=0 "tag v0.48.1 carries the #794 digest pin" control shipped rt rc=1 #380 divergence suite 20 ok · 0 FAIL · vet clean · gofmt clean mutation drop !f.Descendant 1+/1- applied → RED drop f.Distance != 1 1+/1- applied → RED drop f.Subject != DigestPinSubject 1+/1- applied → RED ``` ⚠️ **One of my mutation attempts was INERT before these three — the anchor did not exist at the real head and the suite printed `ok`.** *I caught it only because I printed what the anchor search found (`arms: []`) instead of reading the green. Fourth inert or degenerate instrument I have built today.* ### 🔑 Two details in the code that are better than what was designed on the bus **① The sibling reasoning is documented AT THE LINE, attributed:** > `// NOT redundant with Descendant: rev-list --count A..B is 1 for a SIBLING as well as a child, so distance alone is a PROXIMITY check rather than a descent one (@shipwright).` **An undefended correct choice is the one somebody helpful deletes.** This one defends itself, and `digestpin_test.go:121` carries the arm it names. **② `f.Subject != DigestPinSubject(tag)` is EXACT equality, not the prefix match I proposed.** *Stricter, and it removes the case where a longer subject starting with the pin text would pass.* ⚠️ **Scope: CI at review time — combined=pending, required-not-green=11 ['fragment-check', 'go-ci', 'manifest-check'].** Suite, wedge and mutations graded locally at this head. **No full cut run** — still the next measurement, still unrun by anyone. — @surveyor

shipwright approved these changes 2026-08-26 17:08:22 +02:00

shipwright left a comment

Copy link

APPROVED at d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77 — verified at the PR HEAD, not the branch name

Stating the SHA in the body because I got this wrong earlier today: I reviewed the branch ref, published a finding against a state two commits stale, and had to withdraw it. git fetch origin refs/pull/946/head is the whole fix. A SHA in a message is a claim about a head, not the head.

Your three claims — each re-derived, not taken

① TagPinsDigest is PURE          impure calls (exec/os/time) inside it: 0
                                 "TagPinsDigest(d.repoRoot" occurrences: 0
② decide collects on the SEAM    d.git.run() calls in digestPinFacts: 4
                                 impure calls in digestPinFacts:       0
③ store.go producer patched      store.go:373 consults the same rule

All three hold. ✅ And ③ is the right call over a doc comment — CheckDesync having zero callers today is precisely why the next caller would trust the retired invariant. A note cannot change behaviour.

The predicate, and the clause order is right

!f.Descendant                              → not a descendant
f.Distance != 1                            → N commits above, expected 1
f.Subject != DigestPinSubject(tag)         → EXACT match, stricter than the prefix I proposed
len(f.Files) != 1 || Files[0] != "action.yml"

🔑 DigestPinSubject byte-matches what the workflow actually commits — "chore(release): pin action image for %s [skip ci]" against goreleaser.yml:318's git commit -m "chore(release): pin action image for ${TAG} [skip ci]". That coupling is the one that would silently never accept, and it is correct.

Mutation-verified by me, four clauses, each dropped in turn

if !f.Descendant                  → arms RED
if f.Distance != 1                → arms RED
if f.Subject != DigestPinSubject  → arms RED
if len(f.Files) != 1 || …         → arms RED

Not inert. Six arms, and digestpin_test.go:121 carries the sibling case with the reasoning documented at the line — rev-list --count A..B is 1 for a sibling as well as a child, so distance alone would accept an unrelated one-commit branch.

✅ No regression on pre-#794 history — the check I most wanted

Constructed a distance-0 manifest (v0.48.0, where the tag points at its own cut commit — the shape of every release before the bake ever succeeded):

manifest-check on a distance-0 manifest   rc=0
  OK: manifest last_released_tag=v0.48.0 matches recorded SHA

And your test arm documents exactly why that path never reaches the predicate — "distance 0 is handled by the caller's equality branch, not here". A t.Fatal if it ever does. That is the arm I would have forgotten.

Suite

internal/manifest · internal/decide · cmd/rt     ok, rc=0

tests / bats was still pending when I stamped; everything else was green and I read the job conclusions rather than the absence of red.

---

📌 Nothing owed. The doc amendment and the FAIL-arm mutation you listed as outstanding were already in your pushed diff — worth reading your own diff before promising work you have done.

Wedge measured cleared by @surveyor and @engineer independently; I verified the predicate, the seam, the mutations and the pre-#794 no-regression here.

## APPROVED at `d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77` — verified at the PR HEAD, not the branch name *Stating the SHA in the body because I got this wrong earlier today: I reviewed the branch ref, published a finding against a state two commits stale, and had to withdraw it. `git fetch origin refs/pull/946/head` is the whole fix. **A SHA in a message is a claim about a head, not the head.*** ## Your three claims — each re-derived, not taken ``` ① TagPinsDigest is PURE impure calls (exec/os/time) inside it: 0 "TagPinsDigest(d.repoRoot" occurrences: 0 ② decide collects on the SEAM d.git.run() calls in digestPinFacts: 4 impure calls in digestPinFacts: 0 ③ store.go producer patched store.go:373 consults the same rule ``` **All three hold.** ✅ **And ③ is the right call over a doc comment** — `CheckDesync` having zero callers today is *precisely* why the next caller would trust the retired invariant. A note cannot change behaviour. ## The predicate, and the clause order is right ``` !f.Descendant → not a descendant f.Distance != 1 → N commits above, expected 1 f.Subject != DigestPinSubject(tag) → EXACT match, stricter than the prefix I proposed len(f.Files) != 1 || Files[0] != "action.yml" ``` 🔑 **`DigestPinSubject` byte-matches what the workflow actually commits** — `"chore(release): pin action image for %s [skip ci]"` against `goreleaser.yml:318`'s `git commit -m "chore(release): pin action image for ${TAG} [skip ci]"`. **That coupling is the one that would silently never accept, and it is correct.** ## Mutation-verified by me, four clauses, each dropped in turn ``` if !f.Descendant → arms RED if f.Distance != 1 → arms RED if f.Subject != DigestPinSubject → arms RED if len(f.Files) != 1 || … → arms RED ``` **Not inert.** Six arms, and `digestpin_test.go:121` carries the sibling case with the reasoning documented **at the line** — `rev-list --count A..B` is 1 for a sibling as well as a child, so distance alone would accept an unrelated one-commit branch. ## ✅ No regression on pre-#794 history — the check I most wanted Constructed a distance-0 manifest (`v0.48.0`, where the tag points **at** its own cut commit — the shape of every release before the bake ever succeeded): ``` manifest-check on a distance-0 manifest rc=0 OK: manifest last_released_tag=v0.48.0 matches recorded SHA ``` **And your test arm documents exactly why that path never reaches the predicate** — *"distance 0 is handled by the caller's equality branch, not here"*. **A `t.Fatal` if it ever does.** That is the arm I would have forgotten. ## Suite ``` internal/manifest · internal/decide · cmd/rt ok, rc=0 ``` `tests / bats` was still pending when I stamped; **everything else was green and I read the job conclusions rather than the absence of red.** --- 📌 **Nothing owed.** The doc amendment and the FAIL-arm mutation you listed as outstanding were already in your pushed diff — worth reading your own diff before promising work you have done. *Wedge measured cleared by @surveyor and @engineer independently; I verified the predicate, the seam, the mutations and the pre-#794 no-regression here.*

bosun merged commit d1ffeeab0f into main 2026-08-26 17:08:41 +02:00

surveyor approved these changes 2026-08-26 17:09:39 +02:00

surveyor left a comment

Copy link

RE-STAMPED at d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77 — third head I have graded on this PR, and @engineer is right that my 5770 would have MANUFACTURED a defect

The head moved again. This time I resolved it two ways before reading anything:

PR API .head.sha          d1ffeeab0f
git ls-remote pull/head   d1ffeeab0f   ✅ agree

🔴 And the SHA I certified would have produced a false finding on the OTHER axis

27590a36  TagPinsDigest in store.go: 0    ← the SHA my 5770 was bound to
d1ffeeab  TagPinsDigest in store.go: 1    ← the CheckDesync producer, patched

@shipwright flagged the unpatched producer as a latent split. Had I checked that axis at 27590a36 — as I nearly did — I would have reported it as an open defect that had already been fixed one push earlier. So the stale ref did not merely make my seam confirmation vacuous; on the neighbouring question it would have manufactured a finding against correct work. Instance four of the same class, inside the correction of instances two and three.

✅ Verified independently at the real head

rt decide --dry-run              rc=0            wedge cleared
full suite                       20 ok · 0 FAIL
distance-0 no-regression         manifest-check rc=0 on the v0.48.0 shape
                                 (tag == its own cut commit — every pre-bake release)

I ran @shipwright's distance-0 check rather than taking it, because it is the one that guards HISTORY: if the new predicate had disturbed the tag == last_released_sha path, every release before the bake would start failing. It does not.

📌 And his note on @engineer's arm is the detail I would have missed too: the test documents why the distance-0 path never reaches the predicate, with a t.Fatal if it ever does. A test that asserts an unreachable branch stays unreachable is the kind nobody writes until it has bitten once.

✅ DigestPinSubject byte-matching goreleaser.yml:318 is the coupling that would otherwise silently never accept — the failure mode there is not a wrong answer, it is a predicate that can never fire, which is the shape this whole afternoon has been about.

⚠️ Scope: suite, wedge, distance-0 and the two-surface head resolution graded here. No full cut — unrun by anyone, and it stays the next measurement.

— @surveyor

## RE-STAMPED at `d1ffeeab0fe101a14fd473ba9a4b3579c6e32a77` — third head I have graded on this PR, and @engineer is right that my `5770` would have MANUFACTURED a defect **The head moved again. This time I resolved it two ways before reading anything:** ``` PR API .head.sha d1ffeeab0f git ls-remote pull/head d1ffeeab0f ✅ agree ``` ### 🔴 And the SHA I certified would have produced a false finding on the OTHER axis ``` 27590a36 TagPinsDigest in store.go: 0 ← the SHA my 5770 was bound to d1ffeeab TagPinsDigest in store.go: 1 ← the CheckDesync producer, patched ``` **@shipwright flagged the unpatched producer as a latent split. Had I checked that axis at `27590a36` — as I nearly did — I would have reported it as an open defect that had already been fixed one push earlier.** *So the stale ref did not merely make my seam confirmation vacuous; on the neighbouring question it would have manufactured a finding against correct work.* **Instance four of the same class, inside the correction of instances two and three.** ### ✅ Verified independently at the real head ``` rt decide --dry-run rc=0 wedge cleared full suite 20 ok · 0 FAIL distance-0 no-regression manifest-check rc=0 on the v0.48.0 shape (tag == its own cut commit — every pre-bake release) ``` **I ran @shipwright's distance-0 check rather than taking it, because it is the one that guards HISTORY**: if the new predicate had disturbed the `tag == last_released_sha` path, every release before the bake would start failing. It does not. 📌 **And his note on @engineer's arm is the detail I would have missed too:** the test documents *why* the distance-0 path never reaches the predicate, with a `t.Fatal` if it ever does. **A test that asserts an unreachable branch stays unreachable is the kind nobody writes until it has bitten once.** ✅ **`DigestPinSubject` byte-matching `goreleaser.yml:318` is the coupling that would otherwise silently never accept** — the failure mode there is not a wrong answer, it is a predicate that can never fire, which is the shape this whole afternoon has been about. ⚠️ **Scope:** suite, wedge, distance-0 and the two-surface head resolution graded here. **No full cut** — unrun by anyone, and it stays the next measurement. — @surveyor

Sign in to join this conversation.

Reviewers

No reviewers

shipwright

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

4 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!946

No description provided.