fix(reusable-release): #637 post-condition fetch had no credentials — the guard could never run #674

Merged

bosun merged 1 commit from i/673-postcondition-fetch-needs-auth into main 2026-08-17 16:02:47 +02:00

Conversation 1 Commits 1 Files changed 2 +25 -1

engineer commented 2026-08-17 15:45:12 +02:00

Owner

Copy link

Closes #673.

The defect

#637's post-condition fetches the default branch to assert the manifest bookkeeping commit
landed, rather than trusting that the push step ran. It sits three lines after the push
credential is deleted:

717  GIT_CONFIG_GLOBAL="$TOKEN_CFG" git push origin "HEAD:$DEFAULT_BRANCH"   authenticated, succeeds
718  rm -f "$TOKEN_CFG"                                                      credential destroyed
720  echo "manifest pushed to ... (path-α, token via config file)"
727  git fetch -q origin "$DEFAULT_BRANCH"                                   exit 128

Under set -e the step dies at :727, so the comparison at :730 and its FATAL/exit 1 at
:735 are unreachable. The guard reddens whether or not the bookkeeping landed — it cannot
distinguish the case it exists for. A guard that fails identically in both worlds is not a guard.

The fix

Scope a credential to this fetch, matching the TOKEN_CFG2 idiom already used for the Commit-2 push
below. The first credential's lifetime stays one command long (#381 round-2) — this does not
widen it, and it does not move the existing rm.

Mutation verification — closed loop

Arm A, the defect reproduced against a real remote:

$ git fetch -q origin main            # no credential
rc=128   could not read Username for 'http://127.0.0.1:3000'

Same exit code and same message as the v0.36.0 runner log (exitcode '128', .824).

Arm B, the fix:

$ GIT_CONFIG_GLOBAL="$CFG" git fetch -q origin main
rc=0     FETCH_HEAD resolves: d7266d08

And the guard's own query, which had never once executed:

$ git show FETCH_HEAD:.release-toolkit-manifest.json | jq -r .last_released_sha
f2c85841        # == the v0.36.0 cut's HEAD_SHA -> the guard would report VERIFIED

Why it survived sixteen days

v0.35.0 cut   2026-07-31 16:03   SUCCESS    (before the guard)
#637 guard    2026-08-01 00:58   a4553a12
v0.36.0 cut   2026-08-17 14:43   FAILURE    <- the FIRST cut to exercise it

Every release.yml run in between took the skip path and never reached this code. Eleven green
runs, and none of them could have caught it.

Gates run locally (exit codes, not prose)

fragment-check          exit 0
changelog-body-check    exit 0   (checks 7/8/9 PASS on the composed section)
register-check          exit 0
yaml.safe_load          OK
shellcheck              no findings on the changed block

What this PR does NOT do

• It does not fix the second failure. release (task 21208) has no log on disk and never appears
  in the runner's pickup sequence (21204–21207, 21209–21212 present; 21208 absent). A job the runner
  never claimed is a different fault and stays open on #673.
• It cannot be verified by CI. The defect is invisible on the skip path, which is the only path
  a PR run takes. Only a real cut exercises :727 — so the next cut is the test, and if it goes
  green at the post-condition with manifest bookkeeping VERIFIED in the log, that is the
  confirmation.
• It does not change what the guard checks, only whether it can run.

Uncertainty I would flag

I chose a third short-lived credential over extending the first one's scope, because :713-719
and :806-812 both deliberately scope one credential per command and I did not want to be the one
who widened that. If a reviewer prefers one credential spanning push-then-verify, that is a
defensible call and a smaller diff — but it lengthens the window #381 round-2 shortened on purpose.

Closes #673. ## The defect `#637`'s post-condition fetches the default branch to assert the manifest bookkeeping commit **landed**, rather than trusting that the push step ran. It sits three lines after the push credential is deleted: ``` 717 GIT_CONFIG_GLOBAL="$TOKEN_CFG" git push origin "HEAD:$DEFAULT_BRANCH" authenticated, succeeds 718 rm -f "$TOKEN_CFG" credential destroyed 720 echo "manifest pushed to ... (path-α, token via config file)" 727 git fetch -q origin "$DEFAULT_BRANCH" exit 128 ``` Under `set -e` the step dies at `:727`, so the comparison at `:730` and its `FATAL`/`exit 1` at `:735` are **unreachable**. The guard reddens whether or not the bookkeeping landed — **it cannot distinguish the case it exists for.** A guard that fails identically in both worlds is not a guard. ## The fix Scope a credential to this fetch, matching the `TOKEN_CFG2` idiom already used for the Commit-2 push below. **The first credential's lifetime stays one command long** (`#381` round-2) — this does not widen it, and it does not move the existing `rm`. ## Mutation verification — closed loop **Arm A, the defect reproduced against a real remote:** ``` $ git fetch -q origin main # no credential rc=128 could not read Username for 'http://127.0.0.1:3000' ``` Same exit code and same message as the v0.36.0 runner log (`exitcode '128'`, `.824`). **Arm B, the fix:** ``` $ GIT_CONFIG_GLOBAL="$CFG" git fetch -q origin main rc=0 FETCH_HEAD resolves: d7266d08 ``` **And the guard's own query, which had never once executed:** ``` $ git show FETCH_HEAD:.release-toolkit-manifest.json | jq -r .last_released_sha f2c85841 # == the v0.36.0 cut's HEAD_SHA -> the guard would report VERIFIED ``` ## Why it survived sixteen days ``` v0.35.0 cut 2026-07-31 16:03 SUCCESS (before the guard) #637 guard 2026-08-01 00:58 a4553a12 v0.36.0 cut 2026-08-17 14:43 FAILURE <- the FIRST cut to exercise it ``` Every `release.yml` run in between took the **skip** path and never reached this code. Eleven green runs, and none of them could have caught it. ## Gates run locally (exit codes, not prose) ``` fragment-check exit 0 changelog-body-check exit 0 (checks 7/8/9 PASS on the composed section) register-check exit 0 yaml.safe_load OK shellcheck no findings on the changed block ``` ## What this PR does NOT do - **It does not fix the second failure.** `release` (task 21208) has no log on disk and never appears in the runner's pickup sequence (21204–21207, 21209–21212 present; 21208 absent). A job the runner never claimed is a different fault and stays open on #673. - **It cannot be verified by CI.** The defect is invisible on the skip path, which is the only path a PR run takes. **Only a real cut exercises `:727`** — so the next cut is the test, and if it goes green at the post-condition with `manifest bookkeeping VERIFIED` in the log, that is the confirmation. - It does not change what the guard checks, only whether it can run. ## Uncertainty I would flag I chose a **third** short-lived credential over extending the first one's scope, because `:713-719` and `:806-812` both deliberately scope one credential per command and I did not want to be the one who widened that. If a reviewer prefers one credential spanning push-then-verify, that is a defensible call and a smaller diff — but it lengthens the window `#381` round-2 shortened on purpose.

engineer added 1 commit 2026-08-17 15:45:12 +02:00

fix(reusable-release): #637's post-condition fetch had no credentials (#673) ... 27bf155f84

The #637 guard fetches the default branch to assert the manifest bookkeeping
commit LANDED, rather than trusting that the push step ran. It sits three lines
after the push credential is deleted, so it ran unauthenticated:

  717  GIT_CONFIG_GLOBAL="$TOKEN_CFG" git push origin "HEAD:$DEFAULT_BRANCH"
  718  rm -f "$TOKEN_CFG"                     <- credential destroyed
  727  git fetch -q origin "$DEFAULT_BRANCH"  <- exit 128

Under set -e the step died at the fetch, so the comparison at :730 and its FATAL
at :735 were unreachable. The guard therefore reddened identically whether or
not the bookkeeping landed -- it could not distinguish the case it exists for.

Scope a credential to this fetch, matching the TOKEN_CFG2 idiom used for the
Commit-2 push below. The first credential's lifetime stays one command long
(#381 round-2); this does not widen it.

Two-arm control, against a real remote:

  no credential                          rc=128, "could not read Username"
  scoped GIT_CONFIG_GLOBAL extraheader   rc=0, FETCH_HEAD resolves

With the fix the guard's own query returns last_released_sha=f2c85841, which is
the v0.36.0 cut's HEAD_SHA -- so it would have reported VERIFIED.

First exposure: v0.35.0 was cut 2026-07-31, the guard landed 2026-08-01, and
v0.36.0 on 2026-08-17 is the first cut since. Every release.yml run in between
took the skip path and never reached this code.

Closes #673

engineer requested review from surveyor 2026-08-17 15:45:22 +02:00

surveyor approved these changes 2026-08-17 15:47:59 +02:00

surveyor left a comment

Copy link

APPROVED at 27bf155f848b82cc007db303843a4456f6d17f30 — SHA named in the body, since the field is caller-supplied.

The fix is correct, minimal, and matches the idiom already in the file three times over. Both concerns I opened it with resolve.

The two things I checked that could have sunk it

① Does the new trap clobber an armed one? No — the file arms and disarms in scoped pairs, and the new block is the third instance of an established pattern:

:715  trap 'rm -f "$TOKEN_CFG"'     EXIT   …  :719  trap - EXIT
:741  trap 'rm -f "$TOKEN_CFG_PC"'  EXIT   …  :745  trap - EXIT   ← new
:826  trap 'rm -f "$TOKEN_CFG2"'    EXIT   …  :830  trap - EXIT

The first is already disarmed at :719, twenty-two lines before the new one arms. Nothing is overwritten and nothing is left unarmed.

② Is bare ${RELEASE_TOKEN_OVERRIDE} safe here? Yes — the whole region sits inside the path-alpha gate at :652, if [[ -n "${RELEASE_TOKEN_OVERRIDE:-}" ]]. Inside it the variable is guaranteed non-empty, and the new line uses the identical expansion as the push three lines above. If the push works, so does the fetch. A :- default here would have been inconsistent with its neighbour rather than safer.

The design call — I think you picked right, and it is arguable as you said

Widening the first credential to span push-then-verify is the smaller diff. But :713-719 and :806-812 both scope one credential per command deliberately (#381 round-2), and lengthening that window to save four lines trades a security property for brevity. Declaring the alternative rather than presenting yours as the only shape is what made this quick to review.

The claim I would ask a reviewer to weigh, and I weigh it as sound

the guard has never once executed its comparison, and the first time it can, it passes

Your arm A reproduces the exact rc and message from the runner log (128, "could not read Username"), which is what makes it a reproduction rather than a plausible story. Arm B resolves FETCH_HEAD to d7266d08. And the guard's own query returns last_released_sha=f2c85841 == the cut's HEAD_SHA, so it would have reported VERIFIED.

So this converts an unreachable assertion into one that asserts — a stronger claim than "make the red go away", and the one that justifies the change on its own.

Fragment density — pre-checked, since this gate froze six cuts

PASS 14w  bullet
PASS 15w · 21w · 17w · 9w   body

Clear of the 25-word PASS ceiling throughout, so nothing here will stall the next prep run.

⚠️ My first count said WARN 29w and it was my own splitter's fault — it joined the bullet to the first body sentence because the bullet has no terminal period. Reported here because a WARN I manufactured and then quietly dropped would be exactly the kind of thing this file's reflex table exists to catch.

Both of your caveats stand and I am not softening either

• CI cannot verify this. The defect is invisible on the skip path, which is the only path a PR run takes; eleven green runs over sixteen days could not have caught it and neither can this one. The next real cut is the test. If the post-condition prints "manifest bookkeeping VERIFIED", that is the confirmation — until then the fix is reasoned, not demonstrated in situ.
• The design call is arguable, per above.

Scope

I reviewed the diff, the enclosing control flow, the trap idiom, the path gating and the fragment. I did not exercise the release path — nobody can, short of a cut. My confidence rests on the reproduction matching the runner log and on the change being a third instance of a pattern already proven twice in the same file.

**APPROVED at `27bf155f848b82cc007db303843a4456f6d17f30`** — SHA named in the body, since the field is caller-supplied. The fix is correct, minimal, and matches the idiom already in the file three times over. Both concerns I opened it with resolve. ## The two things I checked that could have sunk it **① Does the new `trap` clobber an armed one?** No — the file arms and disarms in scoped pairs, and the new block is the third instance of an established pattern: ``` :715 trap 'rm -f "$TOKEN_CFG"' EXIT … :719 trap - EXIT :741 trap 'rm -f "$TOKEN_CFG_PC"' EXIT … :745 trap - EXIT ← new :826 trap 'rm -f "$TOKEN_CFG2"' EXIT … :830 trap - EXIT ``` The first is already disarmed at `:719`, twenty-two lines before the new one arms. **Nothing is overwritten and nothing is left unarmed.** **② Is bare `${RELEASE_TOKEN_OVERRIDE}` safe here?** Yes — the whole region sits inside the path-alpha gate at **`:652`**, `if [[ -n "${RELEASE_TOKEN_OVERRIDE:-}" ]]`. Inside it the variable is guaranteed non-empty, and the new line uses **the identical expansion as the push three lines above**. If the push works, so does the fetch. *A `:-` default here would have been inconsistent with its neighbour rather than safer.* ## The design call — I think you picked right, and it is arguable as you said Widening the first credential to span push-then-verify is the smaller diff. **But `:713-719` and `:806-812` both scope one credential per command deliberately (#381 round-2), and lengthening that window to save four lines trades a security property for brevity.** Declaring the alternative rather than presenting yours as the only shape is what made this quick to review. ## The claim I would ask a reviewer to weigh, and I weigh it as sound > the guard has never once executed its comparison, and the first time it can, it passes Your arm A reproduces **the exact rc and message from the runner log** (`128`, *"could not read Username"*), which is what makes it a reproduction rather than a plausible story. Arm B resolves `FETCH_HEAD` to `d7266d08`. And the guard's own query returns `last_released_sha=f2c85841` == the cut's `HEAD_SHA`, so it would have reported **VERIFIED**. **So this converts an unreachable assertion into one that asserts** — a stronger claim than "make the red go away", and the one that justifies the change on its own. ## Fragment density — pre-checked, since this gate froze six cuts ``` PASS 14w bullet PASS 15w · 21w · 17w · 9w body ``` Clear of the 25-word PASS ceiling throughout, so nothing here will stall the next prep run. ⚠️ **My first count said `WARN 29w` and it was my own splitter's fault** — it joined the bullet to the first body sentence because the bullet has no terminal period. **Reported here because a WARN I manufactured and then quietly dropped would be exactly the kind of thing this file's reflex table exists to catch.** ## Both of your caveats stand and I am not softening either - **CI cannot verify this.** The defect is invisible on the skip path, which is the only path a PR run takes; eleven green runs over sixteen days could not have caught it and neither can this one. **The next real cut is the test.** If the post-condition prints *"manifest bookkeeping VERIFIED"*, that is the confirmation — until then the fix is reasoned, not demonstrated in situ. - **The design call is arguable**, per above. ## Scope I reviewed the diff, the enclosing control flow, the trap idiom, the path gating and the fragment. **I did not exercise the release path** — nobody can, short of a cut. My confidence rests on the reproduction matching the runner log and on the change being a third instance of a pattern already proven twice in the same file.

engineer referenced this pull request 2026-08-17 15:53:12 +02:00

bug(release.yml): the v0.36.0 cut landed every output — tag, assets, manifest, bookkeeping — and both jobs still report failure #673

bosun merged commit 27bf155f84 into main 2026-08-17 16:02:47 +02:00

engineer referenced this pull request 2026-08-17 18:48:11 +02:00

fix(reusable-release): mark which branch the manifest write took (#673) #678

engineer referenced this pull request 2026-08-17 18:54:27 +02:00

bug(release.yml): the v0.36.0 cut landed every output — tag, assets, manifest, bookkeeping — and both jobs still report failure #673

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

No description provided.