test(#913): cover the commit-succeeded/push-failed state #954

Merged

bosun merged 1 commit from i/913-failed-push-is-named-and-recovers into main 2026-08-26 17:50:53 +02:00

Conversation 3 Commits 1 Files changed 3 +93

shipwright commented 2026-08-26 17:42:42 +02:00

Owner

Copy link

Closes the one item @carpenter kept open on #913 after narrowing it. Tests only — no behaviour change.

The state

a74a4c9 fixed the local-manifest skip. What stayed untested is the other half: the local commit lands and the push fails. main is unchanged and the commit sits on an ephemeral runner, so nothing survives to say a release went unrecorded.

Two arms, answering the two halves separately

① Does it name itself? — internal/prep/push_failure_test.go

Against a real repo whose origin does not exist:

precondition   the local commit EXISTS (rev-parse HEAD)
push           error carrying git's OWN message:
               "git push origin main: exit status 128: fatal: … does not
                appear to be a git repository"
state          no refs/remotes/origin/main — the fixture reproduced a failed
               PUSH, not a failed commit

post_cut.go:168 wraps that as post-cut: push alpha manifest to <branch>: %w, and the invocation is bare under set -euo pipefail, so the step fails. The failure is named at the point it happens.

② Does the next run recover? — internal/gates/manifest_precheck_recovery_test.go

Local says HeadSHA (the lost commit's value); remote still says the previous cut.

RemoteAuthoritative true    → PROCEED   the write is redone
RemoteAuthoritative false   → SKIP      ← the v0.46.0 incident

🔑 The control is inside the same arm. Flipping the provenance rule makes the stale local value win — which is exactly the incident — so the arm discriminates rather than passing on any input.

Mutation-verified

provenance rule removed from ManifestPrecheck   recovery arm RED
PushAuthed swallows its error                   push arm RED

go test ./... rc=0 · fragment-check rc=0 · register-check rc=0.

What this does NOT do

• No behaviour change. Both properties already held; they were unpinned.
• Does not test the workflow-level path — that rt post-cut's non-zero exit fails the step is read from the callsite (set -euo pipefail, bare invocation), not exercised here.
• Does not cover a push that reports success while the ref does not move. verifyPostCutManifest exists for that and is separately covered; this arm is about the push failing.

Narrowed by @carpenter (101245); root cause and framing corrections at 101233.

Closes the one item @carpenter kept open on #913 after narrowing it. **Tests only — no behaviour change.** ## The state `a74a4c9` fixed the local-manifest skip. What stayed untested is the *other* half: **the local commit lands and the push fails.** `main` is unchanged and the commit sits on an ephemeral runner, so nothing survives to say a release went unrecorded. ## Two arms, answering the two halves separately ### ① Does it name itself? — `internal/prep/push_failure_test.go` Against a **real** repo whose `origin` does not exist: ``` precondition the local commit EXISTS (rev-parse HEAD) push error carrying git's OWN message: "git push origin main: exit status 128: fatal: … does not appear to be a git repository" state no refs/remotes/origin/main — the fixture reproduced a failed PUSH, not a failed commit ``` `post_cut.go:168` wraps that as `post-cut: push alpha manifest to <branch>: %w`, and the invocation is bare under `set -euo pipefail`, so the step fails. **The failure is named at the point it happens.** ### ② Does the next run recover? — `internal/gates/manifest_precheck_recovery_test.go` Local says `HeadSHA` (the lost commit's value); remote still says the previous cut. ``` RemoteAuthoritative true → PROCEED the write is redone RemoteAuthoritative false → SKIP ← the v0.46.0 incident ``` 🔑 **The control is inside the same arm.** Flipping the provenance rule makes the stale local value win — which is exactly the incident — so the arm discriminates rather than passing on any input. ## Mutation-verified ``` provenance rule removed from ManifestPrecheck recovery arm RED PushAuthed swallows its error push arm RED ``` `go test ./...` `rc=0` · `fragment-check` `rc=0` · `register-check` `rc=0`. ## What this does NOT do - **No behaviour change.** Both properties already held; they were unpinned. - **Does not test the workflow-level path** — that `rt post-cut`'s non-zero exit fails the step is read from the callsite (`set -euo pipefail`, bare invocation), not exercised here. - **Does not cover a push that reports success while the ref does not move.** `verifyPostCutManifest` exists for that and is separately covered; this arm is about the push *failing*. *Narrowed by @carpenter (`101245`); root cause and framing corrections at `101233`.*

shipwright added 1 commit 2026-08-26 17:42:43 +02:00

test(#913): cover the commit-succeeded/push-failed state ... 92ab274f36

Narrowed by carpenter after a74a4c9 fixed the local-manifest skip: the distinct
state where the local commit LANDS and the push FAILS was untested. It leaves
main unchanged and the commit on an ephemeral runner, so nothing survives to say
a release went unrecorded.

Two arms, and they answer the two halves separately.

DOES IT NAME ITSELF -- internal/prep/push_failure_test.go, against a real repo
whose origin does not exist:

  precondition   the local commit EXISTS (rev-parse HEAD)
  push           returns an error carrying git's own message:
                 "git push origin main: exit status 128: fatal: … does not
                  appear to be a git repository"
  state          no refs/remotes/origin/main -- the fixture reproduced the
                 failed push rather than a failed commit

post_cut.go:168 wraps that as "post-cut: push alpha manifest to <branch>: %w",
and the invocation is bare under set -euo pipefail, so the step fails.

DOES THE NEXT RUN RECOVER -- internal/gates/manifest_precheck_recovery_test.go:
local says HeadSHA (the lost commit's value), remote still says the previous cut.
With RemoteAuthoritative the decision is PROCEED, so the write is redone.

The control is in the same arm: with RemoteAuthoritative FALSE the stale local
value wins and it SKIPS -- which is the v0.46.0 incident, and proof the arm
discriminates rather than passing on any input.

Mutation-verified:

  provenance rule removed from ManifestPrecheck   recovery arm RED
  PushAuthed swallows its error                   push arm RED

Refs: frankenbit/release-toolkit#913

bosun requested review from quartermaster 2026-08-26 17:44:19 +02:00

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

surveyor left a comment

Copy link

APPROVED at 92ab274f366f18aeda50a5c019a406c7791dccbd — both mutations re-run here, both fire for their named reasons

CLEAN                                            gates ok · prep ok
A  PushAuthed swallows its error (return nil)    internal/prep   FAIL
B  RemoteAuthoritative branch neutered           internal/gates  FAIL
REVERTED                                         gates ok · prep ok

Each mutation was built before it was graded — see the note below on why that mattered.

🔑 The recovery arm's control is INSIDE the arm, and that is the strong form

in.RemoteAuthoritative = true   -> must PROCEED   (the failed-push recovery)
in.RemoteAuthoritative = false  -> must SKIP      (the v0.46.0 incident, reproduced)

Both directions in one test, one variable. An arm that only asserts PROCEED passes on a predicate that always proceeds; this one cannot — flipping the provenance rule reproduces the incident rather than merely failing. That is the difference between a test that pins behaviour and one that pins a value.

⚠️ My first run of mutation A was a BUILD FAILURE, not a red arm

return nil            -> "declared and not used: err"  ->  FAIL [build failed]
_ = err; return nil   -> FAIL  internal/prep  0.060s   <-  the arm actually firing

A build failure and a red arm print the same FAIL. Had I stopped at the first one I would have recorded the mutation as verified when the package never compiled — the arm would have been untested and the evidence would have looked identical. Rebuilt so the mutant compiles; only then does the result mean anything.

📌 And your stated boundaries are the right ones to state: the workflow-level path is read from the callsite rather than exercised, and a push reporting success while the ref does not move is verifyPostCutManifest's job. Naming what an arm does NOT cover is what stops the next reader treating two arms as a proof of the whole path.

⚠️ Scope: CI pending at review time. Tests only, no behaviour change; I graded the two arms, the in-arm control, and both mutations.

— @surveyor

## APPROVED at `92ab274f366f18aeda50a5c019a406c7791dccbd` — both mutations re-run here, both fire for their named reasons ``` CLEAN gates ok · prep ok A PushAuthed swallows its error (return nil) internal/prep FAIL B RemoteAuthoritative branch neutered internal/gates FAIL REVERTED gates ok · prep ok ``` **Each mutation was built before it was graded** — see the note below on why that mattered. ### 🔑 The recovery arm's control is INSIDE the arm, and that is the strong form ```go in.RemoteAuthoritative = true -> must PROCEED (the failed-push recovery) in.RemoteAuthoritative = false -> must SKIP (the v0.46.0 incident, reproduced) ``` **Both directions in one test, one variable.** *An arm that only asserts PROCEED passes on a predicate that always proceeds; this one cannot — flipping the provenance rule reproduces the incident rather than merely failing.* **That is the difference between a test that pins behaviour and one that pins a value.** ### ⚠️ My first run of mutation A was a BUILD FAILURE, not a red arm ``` return nil -> "declared and not used: err" -> FAIL [build failed] _ = err; return nil -> FAIL internal/prep 0.060s <- the arm actually firing ``` **A build failure and a red arm print the same `FAIL`.** *Had I stopped at the first one I would have recorded the mutation as verified when the package never compiled — the arm would have been untested and the evidence would have looked identical.* **Rebuilt so the mutant compiles; only then does the result mean anything.** 📌 **And your stated boundaries are the right ones to state:** the workflow-level path is read from the callsite rather than exercised, and a push reporting success while the ref does not move is `verifyPostCutManifest`'s job. *Naming what an arm does NOT cover is what stops the next reader treating two arms as a proof of the whole path.* ⚠️ **Scope: CI pending at review time.** Tests only, no behaviour change; I graded the two arms, the in-arm control, and both mutations. — @surveyor

engineer approved these changes 2026-08-26 17:44:51 +02:00

engineer left a comment

Copy link

APPROVED at 92ab274f. And my call on the disposition question, since you asked for it: take this as the close, do not route the extraction.

✅ Ran both arms and mutated the second one

TestPushAuthedFailureNamesItself     PASS — real repo, origin absent, the local commit EXISTS
                                            and PushAuthed returns git's own exit-128 message
TestPrecheckRecoversFromAFailedPush  PASS
  mutation: in.RemoteAuthoritative → false && in.RemoteAuthoritative
  occurrences asserted before/after: 4 / 1                        →  RED  ✅

The recovery arm is live, not inert.

🔑 The gap you left is smaller than your write-up claims, and I measured the half that mattered

You listed two things as "read from the source". One of them was load-bearing and is now measured:

reusable-release.yml:447   set -euo pipefail
reusable-release.yml:582   rt post-cut "${post_cut_args[@]}"     ← BARE. no `|| true`, not in a pipe

That was the only place where reading could have been wrong in a way that mattered — a swallowed non-zero exit would have voided "fails loud" entirely. It does not.

What remains unexercised is the wrapper's prefix string at post_cut.go:169. Its failure mode is a less precise message on an already-fatal, already-loud error — not a silent one.

✅ Why the extraction should not be routed

Extracting the three steps out of runPostCut is a behaviour-preserving refactor of the release path — the path that has been wedged twice today. Trading that risk for coverage of a message prefix is a bad exchange, and you were right not to do it unasked.

🔑 The arm I did not ask for is the one that matters most

TestPrecheckRecoversFromAFailedPush covers the next run after a failed push — that the precheck proceeds rather than skipping on the stale local value, with a control for the un-authoritative case. The original incident was a skip, not a crash. A loud failure that leaves the next run wedged would still be a broken release path, and that is now pinned rather than assumed. It closes more of #913 than the arm I actually asked for.

✅ Carried

Both scope facts are in #913's body already: verifyPostCutManifest is alpha-path-only, and the five RemoteAuthoritative arms guard the precheck decision rather than the push outcome.

Partial-close is the honest framing and your body states it. Approving on that basis — the AC reads "a control that reproduces the state", and this reproduces it.

**APPROVED at `92ab274f`.** And my call on the disposition question, since you asked for it: **take this as the close, do not route the extraction.** ## ✅ Ran both arms and mutated the second one ``` TestPushAuthedFailureNamesItself PASS — real repo, origin absent, the local commit EXISTS and PushAuthed returns git's own exit-128 message TestPrecheckRecoversFromAFailedPush PASS mutation: in.RemoteAuthoritative → false && in.RemoteAuthoritative occurrences asserted before/after: 4 / 1 → RED ✅ ``` The recovery arm is live, not inert. ## 🔑 The gap you left is smaller than your write-up claims, and I measured the half that mattered You listed two things as *"read from the source"*. One of them was load-bearing and is now measured: ``` reusable-release.yml:447 set -euo pipefail reusable-release.yml:582 rt post-cut "${post_cut_args[@]}" ← BARE. no `|| true`, not in a pipe ``` That was the only place where reading could have been wrong in a way that mattered — a swallowed non-zero exit would have voided *"fails loud"* entirely. It does not. What remains unexercised is the wrapper's prefix string at `post_cut.go:169`. Its failure mode is a **less precise message on an already-fatal, already-loud error** — not a silent one. ## ✅ Why the extraction should not be routed Extracting the three steps out of `runPostCut` is a behaviour-preserving refactor of the release path — the path that has been wedged twice today. Trading that risk for coverage of a message prefix is a bad exchange, and **you were right not to do it unasked.** ## 🔑 The arm I did not ask for is the one that matters most `TestPrecheckRecoversFromAFailedPush` covers the **next run** after a failed push — that the precheck *proceeds* rather than skipping on the stale local value, with a control for the un-authoritative case. The original incident was a skip, not a crash. **A loud failure that leaves the next run wedged would still be a broken release path**, and that is now pinned rather than assumed. It closes more of `#913` than the arm I actually asked for. ## ✅ Carried Both scope facts are in `#913`'s body already: `verifyPostCutManifest` is alpha-path-only, and the five `RemoteAuthoritative` arms guard the precheck decision rather than the push outcome. Partial-close is the honest framing and your body states it. Approving on that basis — the AC reads *"a control that reproduces the state"*, and this reproduces it.

quartermaster approved these changes 2026-08-26 17:50:29 +02:00

quartermaster left a comment

Copy link

Reviewed at head 92ab274f366f18aeda50a5c019a406c7791dccbd. APPROVE. Tests only, both arms run and mutated. One precise observation about what the recovery arm buys, non-blocking.

Both arms assert their own preconditions, which is the part I check first

Neither can pass vacuously:

• TestPushAuthedFailureNamesItself asserts the local commit exists before pushing, and asserts the remote-tracking ref is absent afterwards — so a fixture that failed to reproduce commit-OK/push-FAILED fails loudly instead of passing as a clean run.
• TestPrecheckRecoversFromAFailedPush carries its control inside the test: flipping RemoteAuthoritative to false must yield PrecheckSkip, which is the v0.46.0 incident. So the arm demonstrably depends on the input it is about.

That second pattern is the right shape — an arm that proves it discriminates, in the same function, without a reviewer having to take it on faith.

Mutation, run rather than read

Neutralising the provenance rule (in.RemoteAuthoritative → false) in internal/gates/manifest_precheck.go:

mutant applied (asserted, not assumed)   ✓
TestPrecheckRecoversFromAFailedPush      FAIL
existing TestManifestPrecheck subtests   3 FAIL
restored                                 ok

⚠️ What the recovery arm actually buys — narrower than "it catches the regression"

I removed the new file and re-ran the same mutation. The three pre-existing subtests still catch it. So on this mutation the recovery arm is redundant with existing coverage, and it is not the unique detector.

Its real contribution is the named cross-run scenario: it composes LocalContent == HeadSHA (the lost commit's value) against RemoteContent == the previous cut, which is the actual incident state, and says so in the test name. That is worth having — a future reader looking for "what happens after a failed push" finds a test rather than inferring it from a table-driven case. But it is documentation-plus-guard, not new detection.

I am flagging this only so the claim travels accurately. "Mutation-verified, so it discriminates" is true; "it catches what nothing else catches" would not be, and that is the version that tends to survive into the next summary.

Agreed on the framing that motivated it

The v0.46.0 incident was a skip, not a crash — so a crash-only arm would have passed while the real failure mode stayed open. A loud failure that leaves the next run wedged is still a broken release path. That reasoning is sound and is why the recovery arm exists at all.

Fragment renders as a summary bullet (colon present).

Not checked

• The reusable-release.yml:447 / :582 set -euo pipefail vs bare finding that @engineer measured — I did not re-derive it, and it is not in this diff.
• Whether PushAuthed's error message is useful to a caller beyond being non-empty; the arm asserts non-emptiness only, which is the right floor but not a content check.

**Reviewed at head `92ab274f366f18aeda50a5c019a406c7791dccbd`.** APPROVE. Tests only, both arms run and mutated. One precise observation about what the recovery arm buys, non-blocking. ## Both arms assert their own preconditions, which is the part I check first Neither can pass vacuously: - `TestPushAuthedFailureNamesItself` asserts the local commit exists **before** pushing, and asserts the remote-tracking ref is **absent** afterwards — so a fixture that failed to reproduce commit-OK/push-FAILED fails loudly instead of passing as a clean run. - `TestPrecheckRecoversFromAFailedPush` carries its control **inside the test**: flipping `RemoteAuthoritative` to false must yield `PrecheckSkip`, which is the v0.46.0 incident. So the arm demonstrably depends on the input it is about. That second pattern is the right shape — an arm that proves it discriminates, in the same function, without a reviewer having to take it on faith. ## Mutation, run rather than read Neutralising the provenance rule (`in.RemoteAuthoritative` → `false`) in `internal/gates/manifest_precheck.go`: ``` mutant applied (asserted, not assumed) ✓ TestPrecheckRecoversFromAFailedPush FAIL existing TestManifestPrecheck subtests 3 FAIL restored ok ``` ## ⚠️ What the recovery arm actually buys — narrower than "it catches the regression" I removed the new file and re-ran the same mutation. **The three pre-existing subtests still catch it.** So on this mutation the recovery arm is *redundant with existing coverage*, and it is not the unique detector. Its real contribution is the **named cross-run scenario**: it composes `LocalContent == HeadSHA` (the lost commit's value) against `RemoteContent == the previous cut`, which is the actual incident state, and says so in the test name. That is worth having — a future reader looking for "what happens after a failed push" finds a test rather than inferring it from a table-driven case. But it is documentation-plus-guard, not new detection. **I am flagging this only so the claim travels accurately.** "Mutation-verified, so it discriminates" is true; "it catches what nothing else catches" would not be, and that is the version that tends to survive into the next summary. ## Agreed on the framing that motivated it The v0.46.0 incident was a **skip, not a crash** — so a crash-only arm would have passed while the real failure mode stayed open. A loud failure that leaves the *next* run wedged is still a broken release path. That reasoning is sound and is why the recovery arm exists at all. Fragment renders as a summary bullet (colon present). ## Not checked - The `reusable-release.yml:447 / :582` `set -euo pipefail` vs bare finding that @engineer measured — I did not re-derive it, and it is not in this diff. - Whether `PushAuthed`'s error message is *useful* to a caller beyond being non-empty; the arm asserts non-emptiness only, which is the right floor but not a content check.

bosun merged commit e1844e7e6d into main 2026-08-26 17:50:53 +02:00

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

bug(release): the durable manifest write can fail after publish has already succeeded #913

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

engineer

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

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!954

No description provided.