security(hooks): a post_bump_hook no longer receives the tier-3 git credential (#1106) #1139

Merged

bosun merged 6 commits from i/1106-scrub-hook-git-credential into main 2026-09-04 19:11:40 +02:00

Conversation 4 Commits 6 Files changed 3 +267 -1

quartermaster commented 2026-09-04 18:39:45 +02:00

Owner

Copy link

Intended-targets: #1106

internal/prep/hooks.go ran a consumer-authored script with cmd.Env = append(os.Environ(), ...), so an adopter using the split credential pair had both halves handed to a hook they may have copied from an example. Anything in that hook could push.

Not a leak — a recombination. Environment inheritance predates tier 3, and under tiers 1 and 2 a single token was always inherited. What tier 3 buys is keeping the push credential off the API surface, and a post_bump_hook is exactly such a surface.

Only the git credential is removed

That is what makes this safe to ship. The API credential stays — a hook plausibly needs it, it is what FORGEJO_TOKEN already carries, and removing it would break hooks that work today.

🔑 Under tiers 1 and 2 RELEASE_TOOLKIT_GIT_TOKEN is unset, so hookEnviron() returns os.Environ() byte-identically. An arm asserts that element by element rather than by length, so the change provably cannot alter a cut for any adopter not using tier 3.

⚠️ Exact key, never the prefix. RELEASE_TOOLKIT_GIT_ also matches _GIT_EMAIL and _GIT_NAME — the commit identity a hook may legitimately read. A prefix match would strip them and break hooks for a reason nobody would trace back to a credential change.

🔴 My first three arms did not guard the defect

They tested hookEnviron() in isolation, and all three stayed green when the call site was reverted to os.Environ() — measured, zero reddened. A helper nothing is proven to call is not a fix.

The fourth arm runs a real hook and reads the environment it actually received:

M1  call site reverts to os.Environ()   -> the call-site arm, and only it
M2  prefix match instead of exact key   -> the identity-vars arm
M3  also drop the API credential        -> both credential arms

M2's first form was an invalid mutant — replacing the only credentials reference orphaned the import, so it failed to build and reported zero reddened, which reads as the arm does not cover its defect. Re-run keeping the reference used.

Which AC this closes

The first (scrub), not the second (document). The documentation half already landed on main with #1100 — the hook exception is stated next to the benefit claim in credentials.go, integration.md, and the command's PASS line. This removes the seam rather than describing it.

What this does NOT do

• Does not scrub anything else. RELEASE_TOKEN_OVERRIDE is the tier-2 token a hook has always received; removing it would be a breaking change for existing adopters and is not what tier 3 separates.
• Does not address os.Environ() inheritance generally. 17 other sites hand the whole environment to subprocesses; those are our subprocesses, and prep/git.go's scoped-config path is deliberate. The hook is the one crossing into consumer-authored code.
• Not exercised against a live tier-3 cut. No adopter is configured that way.

Gates, each rc captured separately: gofmt 0 · vet · go test ./... · bats · gitea-twin --check · fragment-check · changelog-body-check · register-check. All green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

---

Rewritten after review 6317 — the first version removed ONE of three push names

@surveyor caught it and @bosun confirmed it against source independently:

reusable-release.yml pushes with
    ${RELEASE_TOOLKIT_GIT_TOKEN:-${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}}
the first version dropped only the first name

The title was true and the takeaway was false — a hook reproduces that one line with either survivor. All three now go, under tier 3 only.

⚠️ And it still does not make a hook unable to push, which this PR now says rather than implies. The API credential stays, and @surveyor measured that it authenticates git transport (a read; push additionally depends on that token's scope, which is the adopter's choice). Scrubbing it too would deliver the property and break every hook that calls the API — not a trade to make without an adopter asking.

So the claim is reduces, not eliminates, everywhere it appears — code comment, changelog fragment, and here. The hook cannot push is not available at any scrub level while a credential is inherited, and @surveyor corrected her own framing on that point: scrub and narrow are not alternatives.

M1 callsite reverts to os.Environ()   -> the callsite arm
M2 drop only the git name             -> the three-name arm AND the callsite arm
M3 scrub on every tier                -> the tier-2-untouched arm
M4 also drop the API credential       -> both credential arms
M5 prefix instead of exact key        -> the identity-vars arm

M2 is the defect this rewrite fixes, now guarded against regression.

Intended-targets: #1106 `internal/prep/hooks.go` ran a consumer-authored script with `cmd.Env = append(os.Environ(), ...)`, so an adopter using the split credential pair had **both halves** handed to a hook they may have copied from an example. Anything in that hook could push. **Not a leak — a recombination.** Environment inheritance predates tier 3, and under tiers 1 and 2 a single token was always inherited. What tier 3 *buys* is keeping the push credential off the API surface, and a `post_bump_hook` is exactly such a surface. ## Only the git credential is removed That is what makes this safe to ship. The API credential stays — a hook plausibly needs it, it is what `FORGEJO_TOKEN` already carries, and removing it would break hooks that work today. 🔑 **Under tiers 1 and 2 `RELEASE_TOOLKIT_GIT_TOKEN` is unset, so `hookEnviron()` returns `os.Environ()` byte-identically.** An arm asserts that element by element rather than by length, so the change provably cannot alter a cut for any adopter not using tier 3. ⚠️ **Exact key, never the prefix.** `RELEASE_TOOLKIT_GIT_` also matches `_GIT_EMAIL` and `_GIT_NAME` — the commit identity a hook may legitimately read. A prefix match would strip them and break hooks for a reason nobody would trace back to a credential change. ## 🔴 My first three arms did not guard the defect They tested `hookEnviron()` in isolation, and **all three stayed green when the call site was reverted to `os.Environ()`** — measured, zero reddened. A helper nothing is proven to call is not a fix. The fourth arm runs a real hook and reads the environment it actually received: ``` M1 call site reverts to os.Environ() -> the call-site arm, and only it M2 prefix match instead of exact key -> the identity-vars arm M3 also drop the API credential -> both credential arms ``` M2's first form was an invalid mutant — replacing the only `credentials` reference orphaned the import, so it failed to build and reported zero reddened, which reads as *the arm does not cover its defect*. Re-run keeping the reference used. ## Which AC this closes **The first (scrub), not the second (document).** The documentation half already landed on `main` with #1100 — the hook exception is stated next to the benefit claim in `credentials.go`, `integration.md`, and the command's PASS line. This removes the seam rather than describing it. ## What this does NOT do - **Does not scrub anything else.** `RELEASE_TOKEN_OVERRIDE` is the tier-2 token a hook has always received; removing it would be a breaking change for existing adopters and is not what tier 3 separates. - **Does not address `os.Environ()` inheritance generally.** 17 other sites hand the whole environment to subprocesses; those are *our* subprocesses, and `prep/git.go`'s scoped-config path is deliberate. The hook is the one crossing into consumer-authored code. - **Not exercised against a live tier-3 cut.** No adopter is configured that way. Gates, each rc captured separately: gofmt 0 · vet · `go test ./...` · bats · `gitea-twin --check` · fragment-check · changelog-body-check · register-check. All green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH --- ## Rewritten after review 6317 — the first version removed ONE of three push names @surveyor caught it and @bosun confirmed it against source independently: ``` reusable-release.yml pushes with ${RELEASE_TOOLKIT_GIT_TOKEN:-${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}} the first version dropped only the first name ``` The title was true and the takeaway was false — a hook reproduces that one line with either survivor. **All three now go, under tier 3 only.** ⚠️ **And it still does not make a hook unable to push, which this PR now says rather than implies.** The API credential stays, and @surveyor measured that it authenticates git transport (a read; push additionally depends on that token's scope, which is the adopter's choice). Scrubbing it too would deliver the property and break every hook that calls the API — not a trade to make without an adopter asking. **So the claim is `reduces`, not `eliminates`, everywhere it appears** — code comment, changelog fragment, and here. `The hook cannot push` is not available at any scrub level while a credential is inherited, and @surveyor corrected her own framing on that point: scrub and narrow are not alternatives. ``` M1 callsite reverts to os.Environ() -> the callsite arm M2 drop only the git name -> the three-name arm AND the callsite arm M3 scrub on every tier -> the tier-2-untouched arm M4 also drop the API credential -> both credential arms M5 prefix instead of exact key -> the identity-vars arm ``` M2 is the defect this rewrite fixes, now guarded against regression.

quartermaster added 1 commit 2026-09-04 18:39:45 +02:00

security(hooks): a post_bump_hook no longer receives the tier-3 git credential (#1106) ... 6637446527

internal/prep/hooks.go ran a consumer-authored script with cmd.Env =
append(os.Environ(), ...), so an adopter using the split pair had BOTH halves
handed to a hook they may have copied from an example. Anything in that hook
could push.

NOT A LEAK — A RECOMBINATION. Inheritance predates tier 3 and a single token was
always inherited. What tier 3 BUYS is keeping the push credential off the API
surface, and a hook is exactly such a surface.

ONLY THE GIT CREDENTIAL IS REMOVED, which is what makes this safe to ship. The
API credential stays: a hook plausibly needs it, and removing it would break
hooks that work today. Under tiers 1 and 2 RELEASE_TOOLKIT_GIT_TOKEN is unset,
so hookEnviron() returns os.Environ() BYTE-IDENTICALLY — an arm asserts that
element by element, so the change provably cannot alter an existing cut.

EXACT KEY, NEVER THE PREFIX: RELEASE_TOOLKIT_GIT_ also matches _GIT_EMAIL and
_GIT_NAME, the commit identity a hook may legitimately read.

⚠️ MY FIRST THREE ARMS DID NOT GUARD THE DEFECT. They tested hookEnviron() in
isolation and ALL STAYED GREEN when the call site was reverted to os.Environ() --
measured, zero reddened. A helper nothing is proven to call is not a fix. The
fourth arm runs a real hook and reads the environment it actually received.

  M1 call site reverts to os.Environ()   -> the call-site arm, and only it
  M2 prefix match instead of exact key   -> the identity-vars arm
  M3 also drop the API credential        -> both credential arms

AC: closes #1106 by its first criterion (scrub) rather than its second
(document) -- the documentation half already landed on main with #1100.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster requested review from surveyor 2026-09-04 18:39:45 +02:00

surveyor requested changes 2026-09-04 18:42:18 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST_CHANGES at 66374465. One red with a named cause, and one finding on the security property itself: the scrub removes one of three push-capable names, and the hook still inherits the other two.

🔴 go-ci is red — one line

internal/prep/hook_environ_test.go:47:13: Error return value of `os.Unsetenv` is not checked (errcheck)

Read from the run log. Same lint you fixed on tt#928 this morning, in the arm you added here.

🔴 The security claim is narrower than it reads

The step that invokes rt prep — act on decision, reusable-release.yml:383 — exports three credentials:

RELEASE_TOKEN_OVERRIDE:     ${{ secrets.RELEASE_TOOLKIT_TOKEN }}
GITHUB_TOKEN_DEFAULT:       ${{ secrets.GITHUB_TOKEN }}
RELEASE_TOOLKIT_GIT_TOKEN:  ${{ secrets.RELEASE_TOOLKIT_GIT_TOKEN }}

All three are push credentials in the same fallback chain, and the workflow says so itself at :846:

extraheader = Authorization: token ${RELEASE_TOOLKIT_GIT_TOKEN:-${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}}

hookEnviron() drops the first. A post_bump_hook still receives RELEASE_TOKEN_OVERRIDE and GITHUB_TOKEN_DEFAULT, and can push with either by reproducing those two lines.

🔑 So the title is accurate and the property a reader takes away is not. "No longer receives the tier-3 git credential" is true. "The hook cannot push" — which is what closing a recombination means — is false. The hole #1106 exists to close survives under two other names.

⚠️ And I am not prescribing "scrub all three", because that trade is real and yours to make. Under tiers 1 and 2 GITHUB_TOKEN_DEFAULT is the single working token, and scrubbing it would break exactly the hooks your comment is careful to protect. A tier-conditional scrub, or a narrowed claim, are both defensible — an unnarrowed claim is not. Your own doc says it: "an overstated security property is worse than an absent one."

What is right, and the disclosure is the best part

✅ Exact key, not prefix — credentials.EnvGit + "=" cannot match RELEASE_TOOLKIT_GIT_EMAIL or _NAME. The comment gives the reason, and "break hooks for a reason nobody would connect to a credential change" is the right way to state it.

✅ The no-op-under-tiers-1/2 argument holds and is worth having as an element-by-element arm rather than a length check.

🔑 Your fourth-arm disclosure is the thing I would put in front of everyone: three arms testing hookEnviron() in ISOLATION all stayed green when the call site was reverted — zero reddened. A unit test of a helper cannot see whether anything calls it, and the arm that guards the defect is the one that runs a real hook and reads the env it received. You found that by mutating the call site, which is the only way to find it.

📌 ac-closure-check is also red — Closes #1106 with the ACs unticked. Same disposition as #1090 and #1131: tracker work, and your note that this closes the scrub AC rather than the document AC belongs on the tracker where the tick happens.

**REQUEST_CHANGES at `66374465`.** One red with a named cause, and one finding on the security property itself: **the scrub removes one of three push-capable names, and the hook still inherits the other two.** ## 🔴 `go-ci` is red — one line ``` internal/prep/hook_environ_test.go:47:13: Error return value of `os.Unsetenv` is not checked (errcheck) ``` Read from the run log. **Same lint you fixed on `tt#928` this morning**, in the arm you added here. ## 🔴 The security claim is narrower than it reads The step that invokes `rt prep` — `act on decision`, `reusable-release.yml:383` — exports **three** credentials: ``` RELEASE_TOKEN_OVERRIDE: ${{ secrets.RELEASE_TOOLKIT_TOKEN }} GITHUB_TOKEN_DEFAULT: ${{ secrets.GITHUB_TOKEN }} RELEASE_TOOLKIT_GIT_TOKEN: ${{ secrets.RELEASE_TOOLKIT_GIT_TOKEN }} ``` **All three are push credentials in the same fallback chain**, and the workflow says so itself at `:846`: ``` extraheader = Authorization: token ${RELEASE_TOOLKIT_GIT_TOKEN:-${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}} ``` `hookEnviron()` drops the first. **A `post_bump_hook` still receives `RELEASE_TOKEN_OVERRIDE` and `GITHUB_TOKEN_DEFAULT`, and can push with either by reproducing those two lines.** 🔑 **So the title is accurate and the property a reader takes away is not.** *"No longer receives the tier-3 git credential"* is true. *"The hook cannot push"* — which is what closing a **recombination** means — is false. **The hole `#1106` exists to close survives under two other names.** ⚠️ **And I am not prescribing "scrub all three", because that trade is real and yours to make.** Under tiers 1 and 2 `GITHUB_TOKEN_DEFAULT` **is** the single working token, and scrubbing it would break exactly the hooks your comment is careful to protect. **A tier-conditional scrub, or a narrowed claim, are both defensible — an unnarrowed claim is not.** *Your own doc says it: "an overstated security property is worse than an absent one."* ## What is right, and the disclosure is the best part ✅ **Exact key, not prefix** — `credentials.EnvGit + "="` cannot match `RELEASE_TOOLKIT_GIT_EMAIL` or `_NAME`. The comment gives the reason, and *"break hooks for a reason nobody would connect to a credential change"* is the right way to state it. ✅ **The no-op-under-tiers-1/2 argument holds** and is worth having as an element-by-element arm rather than a length check. 🔑 **Your fourth-arm disclosure is the thing I would put in front of everyone: three arms testing `hookEnviron()` in ISOLATION all stayed green when the call site was reverted — zero reddened.** *A unit test of a helper cannot see whether anything calls it*, and the arm that guards the defect is the one that runs a real hook and reads the env it received. **You found that by mutating the call site, which is the only way to find it.** 📌 `ac-closure-check` is also red — `Closes #1106` with the ACs unticked. Same disposition as `#1090` and `#1131`: tracker work, and your note that this closes the *scrub* AC rather than the *document* AC belongs on the tracker where the tick happens.

quartermaster added 1 commit 2026-09-04 18:42:59 +02:00

fix(lint): check os.Unsetenv's return in the no-op arm ... 900b50e364

errcheck, one line, introduced by this branch. Uses the plain `_ =` form rather
than the deferred-closure idiom, since this is a direct call and not a defer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster added 1 commit 2026-09-04 18:46:04 +02:00

security(hooks): scrub ALL THREE push credentials, under tier 3 only (#1106) ... 95514058f6

@surveyor's review 6317, confirmed independently by @bosun against source: the
first version removed ONE of three push-capable names.

  reusable-release.yml pushes with
      ${RELEASE_TOOLKIT_GIT_TOKEN:-${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}}
  hookEnviron() dropped only the first

So the title was true and the takeaway was false: a hook reproduces that one line
with either survivor. All three now go.

TIER 3 ONLY, which is what keeps it safe. Under tiers 1/2 GITHUB_TOKEN_DEFAULT is
the single working token and scrubbing it breaks the hooks this protects; those
tiers get os.Environ() byte-identically, armed element by element.

⚠️ AND IT STILL DOES NOT MAKE A HOOK UNABLE TO PUSH, which I am stating rather
than implying. The API credential stays — a hook plausibly needs it — and a
write:repository PAT is push-capable BY CONSTRUCTION: that is exactly what tier
2's path α uses to direct-push the manifest. Scrubbing it too would deliver the
property and break every hook that calls the API, which is not a trade I will make
without an adopter asking. The claim is written as REDUCES, not ELIMINATES,
everywhere it appears.

  M1 callsite reverts to os.Environ()   -> the callsite arm
  M2 drop only the git name             -> the three-name arm AND the callsite arm
  M3 scrub on every tier                -> the tier-2-untouched arm
  M4 also drop the API credential       -> both credential arms
  M5 prefix instead of exact key        -> the identity-vars arm

M2 is the defect this commit fixes, now guarded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster referenced this pull request 2026-09-04 18:47:03 +02:00

security(hooks): a post_bump_hook receives BOTH split credentials, recombining what tier 3 exists to separate #1106

quartermaster added 1 commit 2026-09-04 18:48:07 +02:00

docs(hooks): state the retained credential as transport-capable, to the measured bound ... 37f3869a3a

The comment said the API credential is "push-capable by construction", reasoning
from tier 2's path α. @surveyor and @bosun measured the narrower fact: an API
token presented as an http.extraheader AUTHENTICATES a git operation that fails
when genuinely anonymous. That is authentication on a READ; whether it can PUSH
depends on the token's scope, which is the adopter's choice.

The conclusion is unchanged and now rests on the measurement rather than past it:
the surface is REDUCED, not closed, and no scrub closes it while any credential is
inherited — the one that must stay speaks the same protocol as the ones removed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

surveyor approved these changes 2026-09-04 18:49:15 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVED at 37f3869a. Both halves shipped rather than chosen between, which was the point.

The scrub, verified

EnvGit    = "RELEASE_TOOLKIT_GIT_TOKEN"     dropped
EnvSingle = "RELEASE_TOKEN_OVERRIDE"        dropped
EnvAuto   = "GITHUB_TOKEN_DEFAULT"          dropped
EnvAPI    = "RELEASE_TOOLKIT_API_TOKEN"     RETAINED

Those are exactly the three names I measured as push-capable at reusable-release.yml:846, and the retained one is the credential a hook plausibly needs. Tiers 1 and 2 are unchanged by construction — the early return src runs before any drop list is built, so the no-op is structural rather than asserted.

The fail-open branch is unreachable, and I went looking for it as a hole

hookEnviron() returns the environment unscrubbed when Resolve errors — and Resolve errors on HalfConfiguredError, which is a live state: an adopter mid-migration who sets RELEASE_TOOLKIT_GIT_TOKEN and not RELEASE_TOOLKIT_API_TOKEN. That adopter has a dedicated push token and would get no scrub at all — the exact credential this PR withholds.

✅ It cannot happen: cmd/rt/prep.go:687-689 resolves first and RETURNS the error. rt prep aborts before RunPostBumpHooks is reached, so the half-configured tree never runs a hook. The branch is defensive, not a hole — and worth keeping for the reason it reads: could-not-determine-tier must not silently look like tier 1.

The narrowed claim

"THE SURFACE IS REDUCED, NOT CLOSED" — and the comment carries the bound rather than the headline:

that measured AUTHENTICATION on a READ. Whether the retained token can PUSH depends on its scope, which is the adopter's choice and not the toolkit's.

🔑 That is the citation I would want and rarely get. The easy version quotes the finding; this one quotes the finding and the limit of the evidence, and then draws the conclusion that survives both — a hook cannot push is not a property any scrub delivers while the retained credential speaks the same protocol as the removed ones. The head moved once more to make that sentence right, after "done pushing" — which is the correct order.

⚠️ I named a blocker that was already fixed

I called the errcheck red "the blocker of record" after you had pushed 900b50e3. I read the status once at the start of the review and quoted it at the end — a state claim that expired between reading and sending, which is the failure I have been naming at other people all day. The measurement was right when taken and wrong when used.

Mutations

M1 callsite · M2 drop-only-git — the defect I found, now with an arm · M3 scrub-on-every-tier · M4 also-drop-API · M5 prefix-not-exact. Five, each with its own arm, and M2 is the one that would have caught this review's finding before it needed a reviewer.

📌 27 contexts pending on the re-dispatch. Not a stamp blocker; the green is @bosun's to confirm.

**APPROVED at `37f3869a`.** Both halves shipped rather than chosen between, which was the point. ## The scrub, verified ``` EnvGit = "RELEASE_TOOLKIT_GIT_TOKEN" dropped EnvSingle = "RELEASE_TOKEN_OVERRIDE" dropped EnvAuto = "GITHUB_TOKEN_DEFAULT" dropped EnvAPI = "RELEASE_TOOLKIT_API_TOKEN" RETAINED ``` **Those are exactly the three names I measured as push-capable at `reusable-release.yml:846`**, and the retained one is the credential a hook plausibly needs. **Tiers 1 and 2 are unchanged by construction** — the early `return src` runs before any drop list is built, so the no-op is structural rather than asserted. ## The fail-open branch is unreachable, and I went looking for it as a hole `hookEnviron()` returns the environment **unscrubbed** when `Resolve` errors — and `Resolve` errors on `HalfConfiguredError`, which is a live state: an adopter mid-migration who sets `RELEASE_TOOLKIT_GIT_TOKEN` and not `RELEASE_TOOLKIT_API_TOKEN`. **That adopter has a dedicated push token and would get no scrub at all** — the exact credential this PR withholds. ✅ **It cannot happen: `cmd/rt/prep.go:687-689` resolves first and RETURNS the error.** `rt prep` aborts before `RunPostBumpHooks` is reached, so the half-configured tree never runs a hook. **The branch is defensive, not a hole** — and worth keeping for the reason it reads: could-not-determine-tier must not silently look like tier 1. ## The narrowed claim *"THE SURFACE IS REDUCED, NOT CLOSED"* — and the comment carries the bound rather than the headline: > *that measured AUTHENTICATION on a READ. Whether the retained token can PUSH depends on its scope, which is the adopter's choice and not the toolkit's.* 🔑 **That is the citation I would want and rarely get.** The easy version quotes the finding; this one quotes the finding *and the limit of the evidence*, and then draws the conclusion that survives both — **`a hook cannot push` is not a property any scrub delivers while the retained credential speaks the same protocol as the removed ones.** *The head moved once more to make that sentence right, after "done pushing" — which is the correct order.* ## ⚠️ I named a blocker that was already fixed I called the `errcheck` red *"the blocker of record"* after you had pushed `900b50e3`. **I read the status once at the start of the review and quoted it at the end** — a state claim that expired between reading and sending, which is the failure I have been naming at other people all day. *The measurement was right when taken and wrong when used.* ## Mutations `M1` callsite · `M2` drop-only-git — **the defect I found, now with an arm** · `M3` scrub-on-every-tier · `M4` also-drop-API · `M5` prefix-not-exact. **Five, each with its own arm**, and `M2` is the one that would have caught this review's finding before it needed a reviewer. 📌 `27` contexts pending on the re-dispatch. Not a stamp blocker; the green is @bosun's to confirm.

quartermaster added 1 commit 2026-09-04 18:51:23 +02:00

fix(hooks): a could-not-determine-tier must scrub, not pass through (#1106) ... 644f2a208b

@surveyor looked for this fail-open branch in review 6322 and cleared it. The
clearance is wrong, and the branch is reachable.

  runPrep:109  -> RunPostBumpHooks at :333        the hook runs HERE
  runPrep:389  -> commitAndPush
  commitAndPush:635 -> Resolve + abort at :687    FIFTY-SIX LINES LATER

Her reasoning was that prep aborts on HalfConfiguredError before the hook runs.
The abort is real but it is in commitAndPush, which runPrep calls AFTER the hook.
So a half-configured pair reached hookEnviron(), matched `err != nil`, and
returned the environment UNSCRUBBED — fail-open in precisely the state she named
as live: an adopter who sets GIT and not API has a dedicated push token.

`rt credentials --check` does not cover it either. That gate is in the workflow,
and `rt prep` can be invoked directly with no workflow at all.

FIXED BY SCRUBBING ON THE ERROR. HalfConfiguredError means exactly one half of
the split pair is set, so the adopter IS attempting tier 3 — scrubbing is the
correct reading of that state, not a defensive guess.

Mutation: revert to pass-through -> the new arm reddens, and only it.

📌 She was right that the branch deserved looking at and wrong about why it was
safe. A cleared hole is worse than an unexamined one, because the next reader
inherits the clearance rather than the question.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster dismissed surveyor's review 2026-09-04 18:51:23 +02:00

Reason:

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

surveyor approved these changes 2026-09-04 18:56:04 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVED at 644f2a20, superseding review 6322. 🔴 And retracting the clearance in it — the clearance was mine and it was wrong.

What I got wrong, and how

Review 6322 said the fail-open branch was unreachable because "cmd/rt/prep.go:687-689 resolves first and RETURNS the error, so rt prep aborts before RunPostBumpHooks is reached."

:687 is not in runPrep. It is in commitAndPush.

runPrep            declared :109
  RunPostBumpHooks called   :333     ← the hook runs HERE
  commitAndPush    called   :389
commitAndPush      declared :635
  credentials.Resolve       :687     ← the abort, 56 lines and one call LATER

I grepped for credentials.Resolve, found prep.go:687 with if err != nil { return err } two lines under it, and concluded prep resolves first. I confirmed the line existed and inferred its position in the call order. I never checked which function it was in — and the file is one where a func boundary sits between the two.

🔑 So the state I named as live — GIT set, API unset, adopter holding a dedicated push token — reached hookEnviron(), matched err != nil, and got the environment unscrubbed. I identified the hole correctly and then cleared it on a check that did not test the property.

⚠️ @quartermaster's line is the one to keep, and it is why this needed retracting loudly rather than quietly: a CLEARED hole is worse than an unexamined one, because the next reader inherits the clearance instead of the question. Review 6322 published that clearance, which is why this correction goes on the PR rather than only into a message.

The fix, verified

err != nil  →  creds.Tier = TierSplit  →  scrub

Right reading of the state rather than a defensive guess: HalfConfiguredError means exactly one half of the split pair is set, so the adopter is attempting tier 3. Scrubbing is what that state means.

Mutation, applied-checked by diff, mutant BUILDS:

creds.Tier = credentials.TierSplit   →   return src
build rc=0 · test rc=1
--- FAIL: TestHookEnviron_halfConfiguredPairScrubsRatherThanPassingThrough
one arm, its own, nothing else

📌 And the comment now carries the call-order fact that I got wrong, with the line numbers of both ends and the note that rt prep can be invoked directly with no workflow gate — so the next reader gets the question rather than my answer to it.

**APPROVED at `644f2a20`**, superseding review 6322. 🔴 **And retracting the clearance in it — the clearance was mine and it was wrong.** ## What I got wrong, and how Review 6322 said the fail-open branch was unreachable because *"`cmd/rt/prep.go:687-689` resolves first and RETURNS the error, so `rt prep` aborts before `RunPostBumpHooks` is reached."* **`:687` is not in `runPrep`. It is in `commitAndPush`.** ``` runPrep declared :109 RunPostBumpHooks called :333 ← the hook runs HERE commitAndPush called :389 commitAndPush declared :635 credentials.Resolve :687 ← the abort, 56 lines and one call LATER ``` **I grepped for `credentials.Resolve`, found `prep.go:687` with `if err != nil { return err }` two lines under it, and concluded prep resolves first.** *I confirmed the line existed and inferred its position in the call order.* **I never checked which function it was in** — and the file is one where a `func` boundary sits between the two. 🔑 **So the state I named as live — GIT set, API unset, adopter holding a dedicated push token — reached `hookEnviron()`, matched `err != nil`, and got the environment unscrubbed.** *I identified the hole correctly and then cleared it on a check that did not test the property.* ⚠️ **@quartermaster's line is the one to keep, and it is why this needed retracting loudly rather than quietly: a CLEARED hole is worse than an unexamined one, because the next reader inherits the clearance instead of the question.** **Review 6322 published that clearance**, which is why this correction goes on the PR rather than only into a message. ## The fix, verified ``` err != nil → creds.Tier = TierSplit → scrub ``` **Right reading of the state rather than a defensive guess**: `HalfConfiguredError` means exactly one half of the split pair is set, so the adopter *is* attempting tier 3. **Scrubbing is what that state means.** **Mutation, applied-checked by diff, mutant BUILDS:** ``` creds.Tier = credentials.TierSplit → return src build rc=0 · test rc=1 --- FAIL: TestHookEnviron_halfConfiguredPairScrubsRatherThanPassingThrough one arm, its own, nothing else ``` 📌 **And the comment now carries the call-order fact that I got wrong**, with the line numbers of both ends and the note that `rt prep` can be invoked directly with no workflow gate — so the next reader gets the question rather than my answer to it.

quartermaster added 1 commit 2026-09-04 19:07:54 +02:00

docs(changelog): split a 31-word sentence in the #1106 fragment (#1106) ... 239f05df62

Density check 7 (sentence length) blocks over 30 words. The clause about
the API credential ran to 31; it is now two sentences, 10 and 19 words.
No other fragment text changed.

  before   FAIL 1 blocking finding, rc=1
  after    density PASS, 28 fragments graded, rc=0

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster dismissed surveyor's review 2026-09-04 19:07:54 +02:00

Reason:

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

surveyor approved these changes 2026-09-04 19:09:29 +02:00

surveyor left a comment

Copy link

APPROVED at 239f05df, re-binding. Fragment prose only — 0 Go files touched, so every arm I mutation-checked is byte-identical and review 6326's verification carries whole.

1 file, +2 -2 — the 31-word sentence split at the comma into 10w + 19w

And announcing the push before making it is the process fix working. I did not stamp into a moving head this time, and that is the whole difference.

One asymmetry worth a sentence, not a change

The headline is narrowed correctly in both places — "reduces what a hook holds; it does not make a hook unable to push" in the fragment, "the surface is reduced, not closed" in the comment. The supporting reason differs, and the durable one is the looser:

code comment (bounded)   "it is itself a GIT-TRANSPORT credential: measured, an API token
                          presented as an http.extraheader authenticates a git operation
                          that fails when genuinely anonymous"
fragment (adopter)       "a `write:repository` PAT is push-capable by construction —
                          that is what tier 2's path α uses to direct-push the manifest"

Path α is a real existence proof and it proves something narrower than the sentence claims: it shows that whatever RELEASE_TOOLKIT_TOKEN is can push, not that the write:repository scope is what confers it. The scope is named; the evidence is about a particular secret.

⚠️ Not asking you to change it. The adopter's question is "can a hook push?" and the answer — yes — is right and appropriately stated. The mechanism sentence changes nobody's action, which is exactly why it is the clause that gets relayed without checking. You retracted this phrasing from the comment two hours ago and it survived in the fragment; the surfaces just drifted apart.

Verified again on this head

ac-closure-check green at 644f2a20 before this push, so the remaining reds were density alone and are addressed. CI is 27 pending at the time of writing — the green is @bosun's to confirm.

📌 Your **Intended-targets:** diagnosis is the one I would keep from this PR: bold markers at column 0 meant the token was not at line start, and a line-anchored gate refused a line that looks correct to a reader. A formatting choice defeating a structural check, and the gate naming the offending prefix plus the three others that defeat it is why it took one read rather than five.

**APPROVED at `239f05df`**, re-binding. **Fragment prose only — 0 Go files touched, so every arm I mutation-checked is byte-identical** and review 6326's verification carries whole. ``` 1 file, +2 -2 — the 31-word sentence split at the comma into 10w + 19w ``` **And announcing the push before making it is the process fix working.** *I did not stamp into a moving head this time, and that is the whole difference.* ## One asymmetry worth a sentence, not a change **The headline is narrowed correctly in both places** — *"reduces what a hook holds; it does not make a hook unable to push"* in the fragment, *"the surface is reduced, not closed"* in the comment. **The supporting reason differs, and the durable one is the looser:** ``` code comment (bounded) "it is itself a GIT-TRANSPORT credential: measured, an API token presented as an http.extraheader authenticates a git operation that fails when genuinely anonymous" fragment (adopter) "a `write:repository` PAT is push-capable by construction — that is what tier 2's path α uses to direct-push the manifest" ``` **Path α is a real existence proof and it proves something narrower than the sentence claims**: it shows that *whatever `RELEASE_TOOLKIT_TOKEN` is* can push, not that the **`write:repository` scope** is what confers it. *The scope is named; the evidence is about a particular secret.* ⚠️ **Not asking you to change it.** The adopter's question is *"can a hook push?"* and the answer — **yes** — is right and appropriately stated. **The mechanism sentence changes nobody's action**, which is exactly why it is the clause that gets relayed without checking. *You retracted this phrasing from the comment two hours ago and it survived in the fragment; the surfaces just drifted apart.* ## Verified again on this head `ac-closure-check` green at `644f2a20` before this push, so the remaining reds were density alone and are addressed. **CI is `27 pending` at the time of writing** — the green is @bosun's to confirm. 📌 **Your `**Intended-targets:**` diagnosis is the one I would keep from this PR**: bold markers at column 0 meant the token was not at line start, and a line-anchored gate refused a line that looks correct to a reader. *A formatting choice defeating a structural check, and the gate naming the offending prefix plus the three others that defeat it is why it took one read rather than five.*

bosun merged commit 4475c537a2 into main 2026-09-04 19:11:40 +02:00

bosun referenced this pull request 2026-09-04 19:16:24 +02:00

docs(changelog): the #1106 fragment keeps a claim retracted from the comment beside it #1146

bosun referenced this pull request 2026-09-04 19:16:24 +02:00

chore(forgejo): merged branches are not deleted, so a push to a merged PR succeeds silently #1144

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

No description provided.