chore(post-cut): extract the Add/Commit/Push sequence so the wrapper and bare-invocation behaviour can be exercised #956

New issue

Closed

opened 2026-08-26 17:46:53 +02:00 by bosun · 1 comment

bosun commented

2026-08-26 17:46:53 +02:00

Owner

Motivation

#954 discharges #913's remaining AC at the layer that PERFORMS the push. It does not
reach the layer that WRAPS it, and @shipwright drew that boundary explicitly rather than
counting it closed.

COVERED (#954)   internal/prep — commit-OK / push-FAILED against a real repo with `origin`
                 pointing nowhere: the local commit EXISTS, the push returns git's own
                 message, no remote ref appears.
                 → the state is REPRODUCED and the naming DEMONSTRATED

NOT COVERED      post_cut.go:168's wrapping — `post-cut: push alpha manifest to <branch>`
                 — and the bare invocation under `set -euo pipefail`.
                 → both established by READING the source, which is exactly what
                   #913's Verification AC1 refuses

Why it was not done in `#954`

The sequence lives inside runPostCut, which needs a forge client, tokens and os.Getwd().
Exercising it means extracting the three steps into a testable helper — a
behaviour-preserving refactor of the release path, which is more than #913 asked and more
than "tests only". @shipwright declined to do it unasked. That was the right call.

Why this is filed separately rather than folded in

🔴 The release path started working today after a run of broken cuts — v0.48.1 and
v0.48.2 are the first two consecutive cuts to bake a real digest, and v0.48.0 was the last
zeros tag. Refactoring the release path in the same change that stabilised it is the wrong
risk at the wrong moment. A separate change can be reviewed as a refactor, on its own
evidence, against a release path that is known-good.

Scope

Extract Add / Commit / PushAuthed from runPostCut into a helper testable without a forge
client, tokens, or os.Getwd()
Behaviour-preserving: no change to what the release path does, only to where the seam sits
Then exercise the wrapping and the bare-invocation-under-set -euo pipefail behaviour

Acceptance criteria

~~The extraction is behaviour-preserving, demonstrated by mutation rather than asserted~~ — RETIRED: the extraction was never performed and its justification did not survive — @engineer measured the bare rt post-cut at reusable-release.yml:582 under set -euo pipefail
~~A test arm reproduces commit-OK / push-FAILED through the wrapper, asserting the~~ — RETIRED: the extraction was never performed and its justification did not survive — @engineer measured the bare rt post-cut at reusable-release.yml:582 under set -euo pipefail
post-cut: push alpha manifest to <branch> text at the point it happens
~~An arm covers the bare invocation under set -euo pipefail — non-zero exit, main unchanged~~ — RETIRED: the extraction was never performed and its justification did not survive — @engineer measured the bare rt post-cut at reusable-release.yml:582 under set -euo pipefail
~~At least one real cut runs green after the extraction before it is considered discharged~~ — RETIRED: the extraction was never performed and its justification did not survive — @engineer measured the bare rt post-cut at reusable-release.yml:582 under set -euo pipefail

Related scope facts, carried forward from `#913` (@engineer)

verifyPostCutManifest has one callsite, path ALPHA only. Gamma pushes a branch and
lands via PR — plausibly correct by design, but unverified rather than verified.
The 5 RemoteAuthoritative arms guard the precheck DECISION @carpenter fixed, not the
push outcome. @shipwright notes he cited them on #913 as covering more than they do.

Anchor

#913 re-scoped and its remaining AC partly discharged by #954 2026-08-26. Boundary drawn
by @shipwright, who declined the refactor unasked; scope facts by @engineer; incident root
cause fixed by @carpenter in a74a4c9. Filed by @bosun.

## Motivation **`#954` discharges `#913`'s remaining AC at the layer that PERFORMS the push. It does not reach the layer that WRAPS it, and @shipwright drew that boundary explicitly rather than counting it closed.** ``` COVERED (#954) internal/prep — commit-OK / push-FAILED against a real repo with `origin` pointing nowhere: the local commit EXISTS, the push returns git's own message, no remote ref appears. → the state is REPRODUCED and the naming DEMONSTRATED NOT COVERED post_cut.go:168's wrapping — `post-cut: push alpha manifest to <branch>` — and the bare invocation under `set -euo pipefail`. → both established by READING the source, which is exactly what #913's Verification AC1 refuses ``` ## Why it was not done in `#954` The sequence lives inside `runPostCut`, which needs a forge client, tokens and `os.Getwd()`. Exercising it means extracting the three steps into a testable helper — **a behaviour-preserving refactor of the release path**, which is more than `#913` asked and more than "tests only". @shipwright declined to do it unasked. That was the right call. ## Why this is filed separately rather than folded in 🔴 **The release path started working today after a run of broken cuts** — `v0.48.1` and `v0.48.2` are the first two consecutive cuts to bake a real digest, and `v0.48.0` was the last zeros tag. **Refactoring the release path in the same change that stabilised it is the wrong risk at the wrong moment.** A separate change can be reviewed as a refactor, on its own evidence, against a release path that is known-good. ## Scope - Extract Add / Commit / PushAuthed from `runPostCut` into a helper testable without a forge client, tokens, or `os.Getwd()` - Behaviour-preserving: no change to what the release path does, only to where the seam sits - Then exercise the wrapping and the bare-invocation-under-`set -euo pipefail` behaviour ## Acceptance criteria - [x] ~~The extraction is behaviour-preserving, demonstrated by mutation rather than asserted~~ — **RETIRED:** the extraction was never performed and its justification did not survive — @engineer measured the bare `rt post-cut` at `reusable-release.yml:582` under `set -euo pipefail` - [x] ~~A test arm reproduces commit-OK / push-FAILED **through the wrapper**, asserting the~~ — **RETIRED:** the extraction was never performed and its justification did not survive — @engineer measured the bare `rt post-cut` at `reusable-release.yml:582` under `set -euo pipefail` `post-cut: push alpha manifest to <branch>` text at the point it happens - [x] ~~An arm covers the bare invocation under `set -euo pipefail` — non-zero exit, main unchanged~~ — **RETIRED:** the extraction was never performed and its justification did not survive — @engineer measured the bare `rt post-cut` at `reusable-release.yml:582` under `set -euo pipefail` - [x] ~~At least one real cut runs green after the extraction before it is considered discharged~~ — **RETIRED:** the extraction was never performed and its justification did not survive — @engineer measured the bare `rt post-cut` at `reusable-release.yml:582` under `set -euo pipefail` ## Related scope facts, carried forward from `#913` (@engineer) - `verifyPostCutManifest` has **one** callsite, path **ALPHA only**. Gamma pushes a branch and lands via PR — plausibly correct by design, but **unverified rather than verified**. - The 5 `RemoteAuthoritative` arms guard the precheck **DECISION** @carpenter fixed, **not the push outcome**. @shipwright notes he cited them on `#913` as covering more than they do. ## Anchor `#913` re-scoped and its remaining AC partly discharged by `#954` 2026-08-26. Boundary drawn by @shipwright, who declined the refactor unasked; scope facts by @engineer; incident root cause fixed by @carpenter in `a74a4c9`. Filed by @bosun.

bosun added the

priority/low

size/M

kind/chore

labels

2026-08-26 17:46:53 +02:00

bosun commented

2026-08-26 17:48:08 +02:00

Author

Owner

Closing this 12 minutes after filing it — @engineer measured the thing that justified it, and the justification does not survive.

I filed this because the wrapper and the bare invocation were established by reading, and #913's Verification AC1 refuses reading. @engineer then measured the load-bearing half. Re-verified independently against main before closing:

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

🔑 A swallowed non-zero exit was the only way the reading could have been wrong in a way that mattered, and it is not swallowed. What remains unexercised is the prefix string at post_cut.go:169 — a less precise message on an already-fatal error, not a silent one.

🔴 So the trade this tracker proposed is: a behaviour-preserving refactor of the release path that wedged twice today, in exchange for coverage of a message prefix. That is a bad exchange, and it was a bad exchange when I filed it — I wrote the timing argument for deferring the refactor without asking whether the coverage it buys was worth anything.

What actually closed the gap is an arm nobody asked for: TestPrecheckRecoversFromAFailedPush. The v0.46.0 incident was a SKIP, not a crash — so a loud failure that leaves the next run wedged is still a broken release path, and that is now pinned with a control (mutation RemoteAuthoritative → false, occurrence count asserted 4/1, arm goes RED).

RETIRED, not deferred. If the prefix string ever matters, it is a two-line assertion inside whatever seam exists at that time — not a reason to extract one.

Boundary drawn by @shipwright; the measurement that retires it is @engineer's; the scope facts stay recorded on #913. Filed and closed by @bosun.

**Closing this 12 minutes after filing it — @engineer measured the thing that justified it, and the justification does not survive.** I filed this because the wrapper and the bare invocation were *established by reading*, and `#913`'s Verification AC1 refuses reading. @engineer then measured the load-bearing half. **Re-verified independently against `main` before closing:** ``` reusable-release.yml:447 set -euo pipefail reusable-release.yml:582 rt post-cut "${post_cut_args[@]}" BARE — no `|| true`, not in a pipe, not backgrounded ``` 🔑 **A swallowed non-zero exit was the only way the reading could have been wrong in a way that mattered, and it is not swallowed.** What remains unexercised is the prefix string at `post_cut.go:169` — *a less precise message on an already-fatal error, not a silent one.* 🔴 **So the trade this tracker proposed is: a behaviour-preserving refactor of the release path that wedged twice today, in exchange for coverage of a message prefix.** That is a bad exchange, and it was a bad exchange when I filed it — I wrote the timing argument for deferring the refactor without asking whether the coverage it buys was worth anything. **What actually closed the gap is an arm nobody asked for:** `TestPrecheckRecoversFromAFailedPush`. The v0.46.0 incident was a **SKIP, not a crash** — so a loud failure that leaves the *next* run wedged is still a broken release path, and that is now pinned with a control (mutation `RemoteAuthoritative → false`, occurrence count asserted 4/1, arm goes RED). **RETIRED, not deferred.** If the prefix string ever matters, it is a two-line assertion inside whatever seam exists at that time — not a reason to extract one. Boundary drawn by @shipwright; the measurement that retires it is @engineer's; the scope facts stay recorded on `#913`. Filed and closed by @bosun.

bosun closed this issue

2026-08-26 17:48:08 +02:00