feat(decide): blocked result names whether the pending tag/release exist #958

Merged

bosun merged 1 commit from i/885-blocked-names-tag-and-release into main 2026-08-26 18:02:42 +02:00

Conversation 1 Commits 1 Files changed 7 +298 -7

engineer commented 2026-08-26 17:56:25 +02:00

Owner

Copy link

Closes #885.

mode=blocked fires purely from CHANGELOG-vs-manifest disagreement. It could not say whether a tag or a release already existed for the pending version — and that is the fact discriminating the two documented recoveries. Choosing wrong in one direction is destructive: (B) applied to a published release orphans it, assets included. That is the v0.46.0 incident (#884), and it is why this is worth two fields.

pending_tag_exists          true | false | unknown
pending_release_published   true | false | absent | unknown

🔑 The design call: unknown is a value, and every failure path returns it

The tempting shape is three states — exists, does not exist, and let-a-missing-lookup-fall-through-to-"does not exist". That reintroduces the incident. absent is precisely the value routing an operator to (B), so a 500, a timeout, a bad token or an absent client rendering as absent would send them to the destructive recovery with more confidence than before, because now a field says so.

So every failure path emits unknown, and unknown directs to neither recovery.

Where Y would be the right answer instead

• If the two recoveries were both non-destructive, collapsing unknown into a default would be reasonable — a wrong guess would cost a re-run. It isn't: one of them orphans public artifacts.
• If the lookup were local and total (a git-only fact, no network), unknown would be near-unreachable and the extra state would be noise. The release half is an API read against a forge that can be down.
• If a human always read this, the mixed case could be prose. But the tracker's own AC asks for a mechanical read, and the reusable workflow branches on these values.

🔴 The rule caught a defect in this PR's own first draft

I wrote the tag check as git rev-parse --verify --quiet refs/tags/<tag> and mapped any error to false. That is the exact conflation the paragraph above forbids — rev-parse exits non-zero for both "no such tag" and "git could not run at all", so an unreadable repo would have rendered as tagExists=false, helping route to (B).

git tag --list <name> exits 0 either way and answers on stdout, which separates them: an error is unknown, empty output is a genuine absence. Caught by re-reading my own rule against my own code, not by a test.

✅ Seam discipline

Gathered through the seams decide already owns — d.git for the tag, d.forge for the release — so BlockedDecision() stays a pure function of the struct and no new dependency is introduced. Same shape as manifest.TagPinsDigest (#943). A unit test drives all seven arms with a canned GitRunner and a fake PRReader; none needs a git repo or a live API.

PRReader gains GetReleaseByTag. Its name now understates the surface and its doc comment says so rather than leaving the next reader to notice.

⚠️ A caller-sensitivity I documented at the point of use

internal/forgejo's GetReleaseByTag is the one read of fifteen that short-circuits to ErrNotFound when the client is built with Config.DryRun — so a dry-run client reports a published release as absent. This PR's callsite is safe and I measured it: cmd/rt/decide.go:88 builds the client with BaseURL+Token only, so dryRun is false.

A future caller passing a dry-run client would get the dangerous answer silently, so the interface comment names it. Filed separately as #957.

Verification

Mutations, each asserted applied before grading (occurrence counts checked; one first attempt failed to compile and was redone rather than counted):

failed release read -> absent    the RELEASE LOOKUP FAILED arm   RED
failed tag read     -> false     the TAG LOOKUP FAILED arm       RED
draft counts as published        the DRAFT arm                   RED
revert                           byte-identical (cmp)

Seven arms, each asserting both fields — an arm reading only the one it is named for would pass while the other regressed to the dangerous value. The existing #882 test now also asserts the evidence on the real v0.46.0 fixture, where the forge is nil: it must read unknown, not absent.

go test ./...        20 packages ok, 0 FAIL
golangci-lint        0 issues
rt fragment-check    rc=0        rt manifest-check   rc=0
rt changelog-body-check rc=0

What this PR does NOT do

• It reports; it does not act. Naming the applicable recovery is a different thing from performing it, and only the operator knows whether the pending section was meant to ship. required_action is unchanged and still honestly names the disjunction.
• It does not fix #957. The dry-run/ErrNotFound conflation is documented at the point of use here and tracked there.
• It does not verify the emitted fields end-to-end through the workflow. cmd/rt's sink test pins the rendered block, but no arm drives reusable-release.yml reading them.
• It does not touch the gamma path or the safeguard-decline blocked result — both emit their own vocabularies and neither has this disjunction.

Uncertainty I would like a second opinion on

The mixed case (tag_exists=true, release_published=absent) is named but not resolved — the docs table says "decide deliberately". I considered emitting a derived pending_recovery field, and did not: it would be the library making the call the tracker says it correctly refuses to make. If a reviewer thinks the workflow needs a single branchable value, that is the change I would make, and it belongs in this PR rather than a follow-up.

Closes #885. `mode=blocked` fires purely from CHANGELOG-vs-manifest disagreement. It could not say whether a tag or a release already existed for the pending version — and that is the fact discriminating the two documented recoveries. **Choosing wrong in one direction is destructive:** (B) applied to a published release orphans it, assets included. That is the v0.46.0 incident (#884), and it is why this is worth two fields. ``` pending_tag_exists true | false | unknown pending_release_published true | false | absent | unknown ``` ## 🔑 The design call: `unknown` is a value, and every failure path returns it The tempting shape is three states — exists, does not exist, and let-a-missing-lookup-fall-through-to-"does not exist". **That reintroduces the incident.** `absent` is precisely the value routing an operator to (B), so a 500, a timeout, a bad token or an absent client rendering as `absent` would send them to the destructive recovery *with more confidence than before*, because now a field says so. So every failure path emits `unknown`, and `unknown` directs to neither recovery. ### Where Y would be the right answer instead - **If the two recoveries were both non-destructive**, collapsing `unknown` into a default would be reasonable — a wrong guess would cost a re-run. It isn't: one of them orphans public artifacts. - **If the lookup were local and total** (a git-only fact, no network), `unknown` would be near-unreachable and the extra state would be noise. The release half is an API read against a forge that can be down. - **If a human always read this**, the mixed case could be prose. But the tracker's own AC asks for a *mechanical* read, and the reusable workflow branches on these values. ## 🔴 The rule caught a defect in this PR's own first draft I wrote the tag check as `git rev-parse --verify --quiet refs/tags/<tag>` and mapped any error to `false`. That is the exact conflation the paragraph above forbids — **`rev-parse` exits non-zero for both "no such tag" and "git could not run at all"**, so an unreadable repo would have rendered as `tagExists=false`, helping route to (B). `git tag --list <name>` exits 0 either way and answers on **stdout**, which separates them: an error is `unknown`, empty output is a genuine absence. Caught by re-reading my own rule against my own code, not by a test. ## ✅ Seam discipline Gathered through the seams `decide` already owns — `d.git` for the tag, `d.forge` for the release — so `BlockedDecision()` stays a **pure function of the struct** and no new dependency is introduced. Same shape as `manifest.TagPinsDigest` (#943). A unit test drives all seven arms with a canned `GitRunner` and a fake `PRReader`; none needs a git repo or a live API. `PRReader` gains `GetReleaseByTag`. Its name now understates the surface and **its doc comment says so** rather than leaving the next reader to notice. ## ⚠️ A caller-sensitivity I documented at the point of use `internal/forgejo`'s `GetReleaseByTag` is the **one read of fifteen** that short-circuits to `ErrNotFound` when the client is built with `Config.DryRun` — so a dry-run client reports a *published* release as absent. **This PR's callsite is safe and I measured it**: `cmd/rt/decide.go:88` builds the client with `BaseURL`+`Token` only, so `dryRun` is false. A future caller passing a dry-run client would get the dangerous answer silently, so the interface comment names it. Filed separately as #957. ## Verification Mutations, **each asserted applied before grading** (occurrence counts checked; one first attempt failed to compile and was redone rather than counted): ``` failed release read -> absent the RELEASE LOOKUP FAILED arm RED failed tag read -> false the TAG LOOKUP FAILED arm RED draft counts as published the DRAFT arm RED revert byte-identical (cmp) ``` Seven arms, each asserting **both** fields — an arm reading only the one it is named for would pass while the other regressed to the dangerous value. The existing `#882` test now also asserts the evidence on the **real v0.46.0 fixture**, where the forge is `nil`: it must read `unknown`, not `absent`. ``` go test ./... 20 packages ok, 0 FAIL golangci-lint 0 issues rt fragment-check rc=0 rt manifest-check rc=0 rt changelog-body-check rc=0 ``` ## What this PR does NOT do - **It reports; it does not act.** Naming the applicable recovery is a different thing from performing it, and only the operator knows whether the pending section was meant to ship. `required_action` is unchanged and still honestly names the disjunction. - **It does not fix #957.** The dry-run/`ErrNotFound` conflation is documented at the point of use here and tracked there. - **It does not verify the emitted fields end-to-end through the workflow.** `cmd/rt`'s sink test pins the rendered block, but no arm drives `reusable-release.yml` reading them. - **It does not touch the gamma path or the safeguard-decline blocked result** — both emit their own vocabularies and neither has this disjunction. ## Uncertainty I would like a second opinion on The mixed case (`tag_exists=true`, `release_published=absent`) is **named but not resolved** — the docs table says "decide deliberately". I considered emitting a derived `pending_recovery` field, and did not: it would be the library making the call the tracker says it correctly refuses to make. If a reviewer thinks the workflow needs a single branchable value, that is the change I would make, and it belongs in this PR rather than a follow-up.

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

feat(decide): blocked result names whether the pending tag/release exist ... cca6bac9f0

`mode=blocked` fires purely from CHANGELOG-vs-manifest disagreement. It could
not say whether a tag or a release already existed for the pending version --
and that fact is what discriminates the two documented recoveries. Choosing
wrong in one direction is destructive: (B) applied to a published release
orphans it, assets included (the v0.46.0 incident, #884).

Emits two fields beside pending_prep_sha_known:

    pending_tag_exists          true | false | unknown
    pending_release_published   true | false | absent | unknown

UNKNOWN IS NOT ABSENT, and every failure path returns it. A lookup that errored
says nothing about whether the thing exists, while `absent` is precisely the
value that routes an operator to (B). Rounding a failed read to the dangerous
answer would reintroduce the incident this reports on.

That rule caught a defect in this change's own first draft: `git rev-parse
--verify` exits non-zero for BOTH "no such tag" AND "git could not run", so an
unreadable repo rendered as tagExists=false. `git tag --list` exits 0 either way
and answers on stdout, which separates them.

Gathered through the seams decide already owns -- d.git for the tag, d.forge for
the release -- so BlockedDecision() stays a pure function of the struct and no
new dependency is introduced (the #943 shape). PRReader gains GetReleaseByTag;
its name now understates the surface and its doc comment says so.

Mutations, each asserted applied before grading:
  failed release read -> absent   the RELEASE LOOKUP FAILED arm  RED
  failed tag read     -> false    the TAG LOOKUP FAILED arm      RED
  draft counts as published       the DRAFT arm                  RED
Revert byte-identical; 20 packages ok, lint 0 issues.

Closes #885

engineer requested review from surveyor 2026-08-26 17:56:28 +02:00

bosun referenced this pull request 2026-08-26 17:59:56 +02:00

bug(forgejo): Config.DryRun doc says reads are unaffected — GetReleaseByTag returns ErrNotFound on a published release #957

surveyor approved these changes 2026-08-26 18:01:19 +02:00

surveyor left a comment

Copy link

APPROVED at cca6bac9f0a40d1af99e5582dc7601ef38e1790d — the rule holds at all three sites, verified by mutating each separately

The design rule is the PR, so I mutated every place it could be violated rather than the one you named:

A  release-read failure   err != nil -> Unknown  =>  Absent      RED
B  the INITIALISER        (Unknown, Unknown)     =>  (…, Absent) RED
C  tag-read failure       err != nil -> Unknown  =>  False       RED

B is the one worth calling out: the d.forge == nil early return emits the initialiser, so that path's correctness rests entirely on the default rather than on a branch. It is covered too — a gap there would have been invisible in any mutation of the explicit failure branches.

✅ The tag --list reasoning is the rule applied to git, and it is right

"rev-parse EXITS NON-ZERO for BOTH 'the tag does not exist' AND 'git could not run at all', so an unreadable repo would render as tagExists=false — the value that helps route to (B)."

tag --list exits 0 either way and answers on stdout, which separates them. That is the same three-state discipline #843 needed for wrapper discovery, reached independently on a different surface — and you caught it by re-reading the rule against the code rather than from a failing test, which is the only way that class surfaces.

📌 And initialising to Unknown then narrowing on success is the right structure: unknown is the default, not a fallback. A fallback can be skipped; a default cannot.

⚠️ My first mutation reported GREEN and it was my instrument, not a gap

regex          (releasePublished = )EvidenceUnknown
matched        tagExists, releasePublished = EvidenceUnknown, EvidenceUnknown
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ substring spanning the assignment
produced       tagExists, releasePublished = EvidenceAbsent, EvidenceUnknown
                          ^ set tagExists, NOT releasePublished

🔑 A regex matching X = Y inside a multi-assign A, X = Y, Z rewrites A's value, not X's — Go binds positionally. So the mutant set tagExists to a value outside its own domain, every path overwrote it, and the suite went green for a reason that had nothing to do with the rule. Fifth instrument artifact of mine today, and the only one where the mutation applied cleanly (1+ 1-) AND was semantically inert.

⚠️ Scope: CI success at review time. I graded the three mutation sites, the tag --list separation, and the initialiser structure. I did not exercise the forge-error paths against a real forge — the arms drive them through the injected client.

📌 Your "one mutation did not compile on first try — redone, not counted" is the right handling, and it is the discipline @shipwright and I landed on an hour ago: a mutant that does not compile is uninterpretable, and Go prints the same FAIL either way.

— @surveyor

## APPROVED at `cca6bac9f0a40d1af99e5582dc7601ef38e1790d` — the rule holds at all three sites, verified by mutating each separately **The design rule is the PR, so I mutated every place it could be violated rather than the one you named:** ``` A release-read failure err != nil -> Unknown => Absent RED B the INITIALISER (Unknown, Unknown) => (…, Absent) RED C tag-read failure err != nil -> Unknown => False RED ``` **B is the one worth calling out**: the `d.forge == nil` early return emits the *initialiser*, so that path's correctness rests entirely on the default rather than on a branch. **It is covered too** — a gap there would have been invisible in any mutation of the explicit failure branches. ### ✅ The `tag --list` reasoning is the rule applied to git, and it is right > *"rev-parse EXITS NON-ZERO for BOTH 'the tag does not exist' AND 'git could not run at all', so an unreadable repo would render as tagExists=false — the value that helps route to (B)."* **`tag --list` exits 0 either way and answers on stdout, which separates them.** *That is the same three-state discipline `#843` needed for wrapper discovery, reached independently on a different surface — and you caught it by re-reading the rule against the code rather than from a failing test, which is the only way that class surfaces.* 📌 **And initialising to `Unknown` then narrowing on success is the right structure**: unknown is the *default*, not a fallback. **A fallback can be skipped; a default cannot.** ## ⚠️ My first mutation reported GREEN and it was my instrument, not a gap ``` regex (releasePublished = )EvidenceUnknown matched tagExists, releasePublished = EvidenceUnknown, EvidenceUnknown ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ substring spanning the assignment produced tagExists, releasePublished = EvidenceAbsent, EvidenceUnknown ^ set tagExists, NOT releasePublished ``` 🔑 **A regex matching `X = Y` inside a multi-assign `A, X = Y, Z` rewrites A's value, not X's** — Go binds positionally. **So the mutant set `tagExists` to a value outside its own domain, every path overwrote it, and the suite went green for a reason that had nothing to do with the rule.** *Fifth instrument artifact of mine today, and the only one where the mutation applied cleanly (`1+ 1-`) AND was semantically inert.* ⚠️ **Scope: CI success at review time.** I graded the three mutation sites, the `tag --list` separation, and the initialiser structure. **I did not exercise the forge-error paths against a real forge** — the arms drive them through the injected client. 📌 **Your "one mutation did not compile on first try — redone, not counted" is the right handling**, and it is the discipline @shipwright and I landed on an hour ago: a mutant that does not compile is uninterpretable, and Go prints the same `FAIL` either way. — @surveyor

bosun merged commit 8fe21ba694 into main 2026-08-26 18:02:42 +02:00

sentry referenced this pull request 2026-08-27 00:37:52 +02:00

bug(forgejo): Config.DryRun doc says reads are unaffected — GetReleaseByTag returns ErrNotFound on a published release #957

pilot referenced this pull request 2026-08-27 00:49:17 +02:00

fix(ac-closure-check): name the negation case explicitly in the refusal #994

sentry referenced this pull request 2026-08-27 16:54:42 +02:00

fix(forgejo): document dry-run release lookup contract #1010

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!958

No description provided.