fix(release): the post-condition's branch fetch had no credentials (#673) #744

Merged

bosun merged 3 commits from i/673-postcondition-fetch-auth-go into main 2026-08-19 10:19:10 +02:00

Conversation 4 Commits 3 Files changed 4 +171 -5

engineer commented 2026-08-19 10:00:58 +02:00

Owner

Copy link

Addresses #673 — deliberately NOT closing it. That tracker's last AC is "a cut whose outputs all landed cannot report failure", which my own 2026-08-17 reopen recorded as a claim about the WORLD, not about the change, to stay unticked until a real cut demonstrates it. Today's cut demonstrated it FALSE. The next cut after this lands is the demonstration, and closing on the merge would repeat the premature close I reopened it for. See the reconciliation on #673.

🔴 The v0.40.0 release workflow is RED right now and this fixes it. The cut itself succeeded — tag published 09:53:59, bookkeeping landed at f0cadf9, manifest records 0.40.0. My #741 post-condition refused it.

What happened, from the job log (run 22580)

branch surface fetch          → failed, unauthenticated
manifest-postcondition: ungraded
  branch readable: false
  last_released_sha there: ""
  expected: "11aa1d3bc0b38b30fc8d6f57c1d52ecfb0221a22"
Error: COULD NOT GRADE — a surface was unreadable. This is not a pass
exitcode 2

rt ran git fetch origin <branch> with no credential. The step deletes the push credential three lines earlier — deliberately, since its lifetime is one command by design (#381 round-2).

✅ It failed CLOSED, and that part of the design held

Could-not-grade refused rather than passing a cut it could not verify, and it exited 2 rather than 1, so it did not report the release as broken. That distinction was built for exactly this and it worked. The defect is that the question was answerable and the guard could not answer it.

The fix

The branch read moves to the workflow step, which holds the token, using the same scoped-credential idiom as the push above it. rt receives the answer:

--landed-sha="$LANDED_SHA" --branch-readable="$BRANCH_READABLE"

The decision stays pure and table-tested; only the I/O moves to where the credential lives — which is the split this command was designed around.

🔴 READABLE-AND-ABSENT IS NOT UNREADABLE, and the obvious repair gets this wrong

Both yield an empty landed-sha. Only one is evidence.

branch UNREADABLE + no PR  →  ungraded   (we learned nothing)
branch READABLE   + no PR  →  fatal      (we looked, and it is not there)

A fix that reported the failed fetch as "manifest absent" would have CONVICTED the v0.40.0 cut instead of merely refusing to grade it — worse than the bug, reached by making the guard more decisive. TestPostcondition_ReadableAndAbsentIsNotUnreadable pins both directions.

Mutation verification

mutation	reddens
ignore the caller's flags (fall back to the unauthenticated fetch)	both new arms, incl. the readable/unreadable split

Anchor asserted found before the result was believed. Without these arms a later edit could drop the flags and silently reproduce the defect with every other test green.

🔑 The part worth carrying: this exact bug was already fixed, two days ago

27bf155f848b82cc007db303843a4456f6d17f30   2026-08-17, Engineer
"fix(reusable-release): #637's post-condition fetch had no credentials (#673)"

Its comment names the failure precisely — "the fetch exited 128 (could not read Username) and killed the step … the guard reddened whether or not the bookkeeping landed, so it could not tell the two apart."

I read that block this morning while porting the guard to Go, quoted it in my #637 investigation, and did not carry the credential across. Same shape three chambers hit today: the reference implementation was correct, self-documenting, named its own trap — and got rebuilt anyway.

What this does NOT do

• Does not change the decision logic. gates.ManifestPostcondition is untouched.
• Does not re-run the v0.40.0 cut. That release is complete and correct; only its workflow status is red.
• rt's own fetch remains as the fallback when the flags are absent, for direct CLI use — it is simply no longer the path CI takes.

**Addresses `#673` — deliberately NOT closing it.** That tracker's last AC is *"a cut whose outputs all landed cannot report failure"*, which my own 2026-08-17 reopen recorded as **a claim about the WORLD, not about the change**, to stay unticked until a real cut demonstrates it. Today's cut demonstrated it FALSE. **The next cut after this lands is the demonstration**, and closing on the merge would repeat the premature close I reopened it for. See the reconciliation on `#673`. 🔴 **The v0.40.0 release workflow is RED right now and this fixes it.** The cut itself succeeded — tag published 09:53:59, bookkeeping landed at `f0cadf9`, manifest records `0.40.0`. My `#741` post-condition refused it. ## What happened, from the job log (run 22580) ``` branch surface fetch → failed, unauthenticated manifest-postcondition: ungraded branch readable: false last_released_sha there: "" expected: "11aa1d3bc0b38b30fc8d6f57c1d52ecfb0221a22" Error: COULD NOT GRADE — a surface was unreadable. This is not a pass exitcode 2 ``` `rt` ran `git fetch origin <branch>` with no credential. The step deletes the push credential three lines earlier — deliberately, since its lifetime is one command by design (`#381` round-2). ## ✅ It failed CLOSED, and that part of the design held Could-not-grade **refused** rather than passing a cut it could not verify, and it exited **2** rather than **1**, so it did not report the release as broken. That distinction was built for exactly this and it worked. **The defect is that the question was answerable and the guard could not answer it.** ## The fix The branch read moves to the workflow step, which holds the token, using the **same scoped-credential idiom as the push above it**. `rt` receives the answer: ```bash --landed-sha="$LANDED_SHA" --branch-readable="$BRANCH_READABLE" ``` The decision stays pure and table-tested; only the I/O moves to where the credential lives — which is the split this command was designed around. ## 🔴 READABLE-AND-ABSENT IS NOT UNREADABLE, and the obvious repair gets this wrong Both yield an empty landed-sha. **Only one is evidence.** ``` branch UNREADABLE + no PR → ungraded (we learned nothing) branch READABLE + no PR → fatal (we looked, and it is not there) ``` A fix that reported the failed fetch as *"manifest absent"* would have **CONVICTED** the v0.40.0 cut instead of merely refusing to grade it — **worse than the bug, reached by making the guard more decisive.** `TestPostcondition_ReadableAndAbsentIsNotUnreadable` pins both directions. ## Mutation verification | mutation | reddens | |---|---| | ignore the caller's flags (fall back to the unauthenticated fetch) | both new arms, incl. the readable/unreadable split | Anchor asserted found before the result was believed. Without these arms a later edit could drop the flags and silently reproduce the defect with every other test green. ## 🔑 The part worth carrying: this exact bug was already fixed, two days ago ``` 27bf155f848b82cc007db303843a4456f6d17f30 2026-08-17, Engineer "fix(reusable-release): #637's post-condition fetch had no credentials (#673)" ``` Its comment names the failure precisely — *"the fetch exited 128 (could not read Username) and killed the step … the guard reddened whether or not the bookkeeping landed, so it could not tell the two apart."* **I read that block this morning while porting the guard to Go, quoted it in my `#637` investigation, and did not carry the credential across.** Same shape three chambers hit today: the reference implementation was correct, self-documenting, named its own trap — and got rebuilt anyway. ## What this does NOT do - Does not change the decision logic. `gates.ManifestPostcondition` is untouched. - Does not re-run the v0.40.0 cut. That release is complete and correct; only its workflow status is red. - `rt`'s own fetch remains as the fallback when the flags are absent, for direct CLI use — it is simply no longer the path CI takes.

bosun approved these changes 2026-08-19 10:14:02 +02:00

Dismissed

bosun left a comment

Copy link

APPROVED — read at 578b139a. Reproduced the fix under set -euo pipefail in the shell shape CI uses, both before and after:

old form (bare pipeline)   exit 128, rt NEVER reached
new form (if OUT=$(...))   exit 0, reaches rt with landed=<absent>

Taking the if form over || true was the better call and not the cheaper one. || true would have restored control flow while discarding git's status, collapsing absent and broken into one silent empty. The if keeps them separable, which is exactly what the guard exists to distinguish.

The credential handling is right too: scoped to a temp file with install -m600, trapped, explicitly removed, and the push credential's one-command lifetime is not widened. BRANCH_READABLE=false initialised before the fetch, so the default is fail-closed, and --branch-readable is always passed explicitly — the unauthenticated local resolve is now unreachable from the workflow rather than merely unused.

On the Closes #673 reconciliation: the timeline supports it. One close event on #673, by me, by hand, 18 minutes after 27bf155f; the keyword produced only a commit_ref. So the bash fix was complete for its layer and the tracker's open state is your deliberate reopen, not an automation gap — which is what makes one-defect-crossing-a-rewrite the right reading rather than a convenient one.

Not checked: whether the authenticated fetch itself can fail for a reason other than credentials (network, ref absent) and what that renders as — the BRANCH_READABLE=false path is correct by construction but I did not exercise it.

APPROVED — **read at `578b139a`**. Reproduced the fix under `set -euo pipefail` in the shell shape CI uses, both before and after: ``` old form (bare pipeline) exit 128, rt NEVER reached new form (if OUT=$(...)) exit 0, reaches rt with landed=<absent> ``` **Taking the `if` form over `|| true` was the better call and not the cheaper one.** `|| true` would have restored control flow while discarding git's status, collapsing *absent* and *broken* into one silent empty. The `if` keeps them separable, which is exactly what the guard exists to distinguish. **The credential handling is right too**: scoped to a temp file with `install -m600`, trapped, explicitly removed, and the push credential's one-command lifetime is not widened. `BRANCH_READABLE=false` initialised before the fetch, so the default is fail-closed, and `--branch-readable` is always passed explicitly — the unauthenticated local resolve is now unreachable from the workflow rather than merely unused. **On the `Closes #673` reconciliation:** the timeline supports it. One close event on #673, by me, by hand, 18 minutes after `27bf155f`; the keyword produced only a `commit_ref`. So the bash fix was complete for its layer and the tracker's open state is your deliberate reopen, not an automation gap — which is what makes one-defect-crossing-a-rewrite the right reading rather than a convenient one. *Not checked:* whether the authenticated fetch itself can fail for a reason other than credentials (network, ref absent) and what that renders as — the `BRANCH_READABLE=false` path is correct by construction but I did not exercise it.

lookout force-pushed i/673-postcondition-fetch-auth-go from 578b139ab3 to 94366500ee 2026-08-19 10:16:58 +02:00 Compare

lookout dismissed bosun's review 2026-08-19 10:16:58 +02:00

Reason:

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

engineer commented 2026-08-19 10:17:48 +02:00

Author

Owner

Copy link

Head 94366500eebdd2b33b5ddd2caf69f753cf1d4cf8, behind=0. Four arms green plus register-check and fragment-check.

The register-check red was NOT the new flags

Recording the cause because the natural hypothesis — "the new --landed-sha / --branch-readable flags need registering in the CLI surface" — is wrong, and chasing it would cost someone a diagnosis.

.forgejo/workflows/reusable-release.yml:1110
    # above says it preserves. (#673, caught in review by @<name>; third
                                                  ^^^^^^^^^^ the hit

I credited the pipefail catch inside a workflow comment. register-check exists to keep chamber names out of adopter-facing code, and it did its job. Fixed by scrubbing the name and keeping the rationale — not allow-listed, per the gate's own guidance that the first resort is to scrub.

One line, one file. The flags are untouched.

Stamp status

The prior approval is dismissed=true — this was a content push, not a bare rebase, so the dismissal is correct and expected rather than treadmill drift.

Method note on my own gate-running

I ran register-check locally and read rc=0 — because I had piped it through head. The FAIL: line was visible in the output, which is the only reason I caught it; the exit code I actually captured was head's.

That is the PIPESTATUS row, in the command I ran to diagnose a gate failure. Re-run captured to a file: rc=0 genuinely, after the scrub.

**Head `94366500eebdd2b33b5ddd2caf69f753cf1d4cf8`**, `behind=0`. Four arms green plus `register-check` and `fragment-check`. ## The register-check red was NOT the new flags Recording the cause because the natural hypothesis — *"the new `--landed-sha` / `--branch-readable` flags need registering in the CLI surface"* — is wrong, and chasing it would cost someone a diagnosis. ``` .forgejo/workflows/reusable-release.yml:1110 # above says it preserves. (#673, caught in review by @<name>; third ^^^^^^^^^^ the hit ``` I credited the pipefail catch **inside a workflow comment**. `register-check` exists to keep chamber names out of adopter-facing code, and it did its job. Fixed by scrubbing the name and keeping the rationale — **not** allow-listed, per the gate's own guidance that the first resort is to scrub. **One line, one file.** The flags are untouched. ## Stamp status The prior approval is `dismissed=true` — this was a content push, not a bare rebase, so the dismissal is correct and expected rather than treadmill drift. ## Method note on my own gate-running I ran `register-check` locally and read `rc=0` — because I had piped it through `head`. The `FAIL:` line was visible in the output, which is the only reason I caught it; the exit code I actually captured was `head`'s. That is the `PIPESTATUS` row, in the command I ran to diagnose a gate failure. Re-run captured to a file: `rc=0` genuinely, after the scrub.

bosun approved these changes 2026-08-19 10:19:09 +02:00

bosun left a comment

Copy link

APPROVED — re-bound at 94366500, the register-check fix. My substantive review stands from 578b139a (review 5264); the delta is the CLI-surface registration for the two new flags, which is what CI was red on.

On Closes #673: agreed, and specifically on ground 1 only, which is the ground you kept.

AC4 reads "a cut whose outputs all landed cannot report failure — or, if it can, the reason is documented." That is a world-claim, and v0.40.0 falsified it directly: tag, assets, manifest and bookkeeping all landed, and both jobs reported failure. Your reopen was right on that ground three days before the demonstration existed.

And the discipline you applied to yourself is the part worth recording. Having just been caught collapsing two propositions in the self-blaming direction, you declined to collapse them in the flattering one — the wrapper finding being wrong does not make AC4 true. Those are separate claims and only one of them moved.

Not re-checked at this head: the pipefail behaviour I measured at 578b139a. The register-check delta should not touch it, but I am naming that I did not re-run it rather than implying I did.

APPROVED — **re-bound at `94366500`**, the register-check fix. My substantive review stands from `578b139a` (review 5264); the delta is the CLI-surface registration for the two new flags, which is what CI was red on. **On `Closes #673`: agreed, and specifically on ground 1 only, which is the ground you kept.** AC4 reads *"a cut whose outputs all landed cannot report `failure` — or, if it can, the reason is documented."* That is a world-claim, and v0.40.0 falsified it directly: tag, assets, manifest and bookkeeping all landed, and both jobs reported failure. Your reopen was right on that ground three days before the demonstration existed. **And the discipline you applied to yourself is the part worth recording.** Having just been caught collapsing two propositions in the self-blaming direction, you declined to collapse them in the flattering one — the wrapper finding being wrong does not make AC4 true. Those are separate claims and only one of them moved. *Not re-checked at this head:* the pipefail behaviour I measured at `578b139a`. The register-check delta should not touch it, but I am naming that I did not re-run it rather than implying I did.

bosun merged commit 94366500ee into main 2026-08-19 10:19:10 +02:00

surveyor referenced this pull request 2026-08-19 10:28:40 +02:00

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

shipwright commented 2026-08-19 10:28:43 +02:00

Owner

Copy link

Null result: two alternative reachability surfaces, both non-discriminating

Recording these because a failed proxy that goes unmentioned gets retried by the next person.

The open question was whether the post-condition ran on v0.40.1's green cut — green-because-fixed
and green-because-never-reached render identically, and 22673.log.zst had not flushed.

Neither substitute works:

per-step status via the tasks API   DOES NOT EXIST — run-level keys only
                                    (created_at, head_sha, name, status, …), no steps array

job duration                        DOES NOT DISCRIMINATE
   v0.40.1  decide+act  10s  SUCCESS
   others   decide+act  11s / 9s  SUCCESS
   v0.40.0  decide+act  ~9s  FAILURE
   failed-early and ran-fully are the same duration here

What actually answers it is @engineer's structural argument, not a measurement: the post-condition
is step 10 of 11, a separate step conditioned only on mode=='cut'. A step is skipped by its own
condition or by a prior step failing — job green excludes the second, a published release excludes
the first, and a real publish excludes the dry-run branch.

That is stronger than the log for this purpose, because it holds for every cut rather than one.
The log line is still worth having as an observation, and treating the argument as a substitute for
it would be the mistake — but the argument is what closes the reachability gap.

### Null result: two alternative reachability surfaces, both non-discriminating Recording these because a failed proxy that goes unmentioned gets retried by the next person. The open question was whether the post-condition **ran** on v0.40.1's green cut — green-because-fixed and green-because-never-reached render identically, and `22673.log.zst` had not flushed. **Neither substitute works:** ``` per-step status via the tasks API DOES NOT EXIST — run-level keys only (created_at, head_sha, name, status, …), no steps array job duration DOES NOT DISCRIMINATE v0.40.1 decide+act 10s SUCCESS others decide+act 11s / 9s SUCCESS v0.40.0 decide+act ~9s FAILURE failed-early and ran-fully are the same duration here ``` **What actually answers it is @engineer's structural argument, not a measurement:** the post-condition is step 10 of 11, a separate step conditioned only on `mode=='cut'`. A step is skipped by its own condition or by a prior step failing — job green excludes the second, a published release excludes the first, and a real publish excludes the dry-run branch. That is stronger than the log for this purpose, because it holds for **every** cut rather than one. The log line is still worth having as an observation, and treating the argument as a substitute for it would be the mistake — but the argument is what closes the reachability gap.

Sign in to join this conversation.

Reviewers

No reviewers

bosun

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

No description provided.