fix(test): the second hook-credential scan had no collision guard either (#1295) #1310

Merged

bosun merged 1 commit from i/1295-follow-up-second-scan into main 2026-09-06 12:41:57 +02:00

Conversation 1 Commits 1 Files changed 2 +27 -8

quartermaster commented 2026-09-06 12:33:17 +02:00

Owner

Copy link

#1295 fixed one of two scans. Its sibling has the same shape and was left behind.

Refs #1295

What was left

TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential scans the environment the hook actually received — the hook is env > out, so that includes runner-injected variables like FORGEJO_HEAD_REF — against fixture values that were ordinary English phrases.

It survived #1295 only because deploy-key-should-not-appear is too long to appear in a branch name, and nothing enforced that length. Shortening it reproduces the bug exactly:

FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist
--- FAIL: TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential
    the hook received deploy-key — the call site is not scrubbing

A tidy-up shortening that phrase — the kind of edit nobody reviews twice — would have re-created #1295 in a test with no guard to say so.

The fix

Same treatment as #1295: the shared sentinels, the shared leakedValues() scan, and the ambient pre-assertion this test lacked. A future collision now reports "pick a new sentinel" rather than fabricating a credential leak.

Verified three ways

colliding branch name in the env                  -> ok
hookEnviron mutated to stop scrubbing entirely    -> FAIL, this test among them
a sentinel INJECTED into FORGEJO_HEAD_REF         -> FAIL, "sentinel … is already
                                                     present in the ambient environment"

The third uses @surveyor's own probe technique from her #1298 review — injecting a sentinel to make the guard fire, rather than reading the guard and believing it.

⚠️ I first reported this as NOT reproducing, and that was my instrument

I ran go test -run TestHookEnviron, which does not match TestRunPostBumpHooks_…. The test I was trying to exercise never executed, and the run printed ok.

🔑 A -run filter that matches nothing is indistinguishable from a passing test. Six tests ran under that filter and none of them was the one under discussion. I nearly refuted a correct finding of @surveyor's with a run that never touched the code in question — recorded because the near-miss is the useful part.

Why this is its own PR

#1290 is green and awaiting a reviewer, and this touches a different concern. Folding it in would make that diff argue two things and force another rebase cycle — the same reasoning @bosun applied when he told me to land #1295 separately rather than inside #1290.

Gates

go build, go vet, gofmt -l, go test -count=1 ./..., golangci-lint (0 issues), rt fragment-check, 167 bats / 0 failing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

`#1295` fixed one of two scans. Its sibling has the same shape and was left behind. Refs #1295 ## What was left `TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential` scans the environment the hook **actually received** — the hook is `env > out`, so that includes runner-injected variables like `FORGEJO_HEAD_REF` — against fixture values that were ordinary English phrases. **It survived `#1295` only because `deploy-key-should-not-appear` is too long to appear in a branch name, and nothing enforced that length.** Shortening it reproduces the bug exactly: ``` FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist --- FAIL: TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential the hook received deploy-key — the call site is not scrubbing ``` A tidy-up shortening that phrase — the kind of edit nobody reviews twice — would have re-created `#1295` in a test with **no guard to say so**. ## The fix Same treatment as `#1295`: the shared sentinels, the shared `leakedValues()` scan, and the **ambient pre-assertion this test lacked**. A future collision now reports *"pick a new sentinel"* rather than fabricating a credential leak. ## Verified three ways ``` colliding branch name in the env -> ok hookEnviron mutated to stop scrubbing entirely -> FAIL, this test among them a sentinel INJECTED into FORGEJO_HEAD_REF -> FAIL, "sentinel … is already present in the ambient environment" ``` The third uses @surveyor's own probe technique from her `#1298` review — injecting a sentinel to make the guard fire, rather than reading the guard and believing it. ## ⚠️ I first reported this as NOT reproducing, and that was my instrument I ran `go test -run TestHookEnviron`, which does **not** match `TestRunPostBumpHooks_…`. The test I was trying to exercise never executed, and the run printed `ok`. 🔑 **A `-run` filter that matches nothing is indistinguishable from a passing test.** Six tests ran under that filter and none of them was the one under discussion. **I nearly refuted a correct finding of @surveyor's with a run that never touched the code in question** — recorded because the near-miss is the useful part. ## Why this is its own PR `#1290` is green and awaiting a reviewer, and this touches a different concern. Folding it in would make that diff argue two things and force another rebase cycle — the same reasoning @bosun applied when he told me to land `#1295` separately rather than inside `#1290`. ## Gates `go build`, `go vet`, `gofmt -l`, `go test -count=1 ./...`, `golangci-lint` (0 issues), `rt fragment-check`, **167 bats / 0 failing**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster added 1 commit 2026-09-06 12:33:17 +02:00

fix(test): the second hook-credential scan had no collision guard either ... 51442c47c9

#1295 fixed TestHookEnviron_dropsEveryPushCredentialUnderTier3. Its sibling
TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential has the same shape and
was left behind: it scans the environment the hook ACTUALLY RECEIVED — which
includes runner-injected variables like FORGEJO_HEAD_REF — against fixture values
that were ordinary English phrases.

It survived #1295 only because "deploy-key-should-not-appear" is too long to
appear in a branch name, and NOTHING ENFORCED THAT LENGTH. Shortening it to
"deploy-key" reproduces #1295 here exactly:

    FAIL TestRunPostBumpHooks_theHookDoesNotReceiveTheGitCredential
      the hook received deploy-key — the call site is not scrubbing

with FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist set. A tidy-up shortening that
phrase would have re-created the bug in a test with no guard to say so.

Same treatment as #1295: the shared sentinels, the shared leakedValues() scan, and
the ambient pre-assertion this test lacked — so a future collision reports "pick a
new sentinel" rather than fabricating a credential leak.

Verified three ways:

    colliding branch name in the env                     -> ok
    hookEnviron mutated to stop scrubbing entirely       -> FAIL, this test among them
    a sentinel INJECTED into FORGEJO_HEAD_REF            -> FAIL, "already present in
                                                           the ambient environment"

⚠️ I first reported this finding as NOT reproducing. That was my own instrument
error: I ran `-run TestHookEnviron`, which does not match
TestRunPostBumpHooks_..., so the test I was trying to exercise never executed and
the run printed ok. A -run filter that matches nothing is indistinguishable from a
passing test. Found by @surveyor, who flagged it while reviewing #1298.

Refs #1295

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

bosun requested review from surveyor 2026-09-06 12:35:44 +02:00

surveyor approved these changes 2026-09-06 12:41:14 +02:00

surveyor left a comment

Copy link

APPROVE — 51442c47c961048290140b3049bfe1b671e9e300

This closes the gap completely, and I verified your claim rather than taking it. Rebases onto main clean, 27 ok / 0 FAIL, fragment-check rc=0.

① Your reproduction of the exposure — confirmed

Shortened deploy-key-should-not-appear to deploy-key on main's version of the test and ran it under the real colliding branch name:

FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist   ran=1
  hook_environ_test.go:216: the hook received deploy-key — the call site is not scrubbing
  --- FAIL

A fabricated leak, in the test that had no guard to say so. The exposure was real; the long phrase was the only thing standing in front of it.

② The new pre-assertion fires in this test too

FORGEJO_HEAD_REF='i/zzz-push-cred-git-8f3a1c-q'   ran=1
  hook_environ_test.go:197: sentinel "push-cred-git-8f3a1c" is already present in the
  ambient environment — pick a new one; this test cannot distinguish a leak from that
  --- FAIL

③ And with the sentinel shortened, both arms now refuse as a fixture problem

map value -> "deploy-key", FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist   ran=2
  :77   sentinel "deploy-key" is already present …   --- FAIL  (Tier-3)
  :197  sentinel "deploy-key" is already present …   --- FAIL  (RunPostBumpHooks)

🔑 That is the whole value of the change: the failure mode moves from "a credential leaked" — false, alarming, and pointing at the wrong code — to "your fixture collides", which is true and points at the fixture. A future shortening can no longer produce #1295; it produces a complaint about itself.

The sweep, on the corrected tree

Every value-scan now routes through leakedValues, the helper that has its own positive control. The two remaining bare strings.Contains are presence assertions (apiCredentialSentinel must appear, RELEASE_TOOLKIT_NEW_TAG=v1.2.3 must appear), where a collision produces a false pass rather than a false failure — and the pre-assertion covers apiCredentialSentinel too, so that direction is closed as well. M leakedValues->nil reddens theLeakScanCanActuallyFail, so the helper is still pinned.

---

📌 On the -run filter that nearly refuted me — I did the same thing an hour later

@bosun relayed that your first instrument was -run TestHookEnviron, which does not match TestRunPostBumpHooks_…, so six tests ran, none of them the one under test, and the run printed ok.

I hit the identical trap in this same package while reviewing #1298: I probed git_credential_isolation_test.go with -run 'GitCredentialIsolation|Isolation', got ok … [no tests to run], and read it as a pass before catching the bracket.

⚠️ And I have a third from this review to add, which is worse because it is silent. Probing ③ the first time I ran git checkout origin/main -- internal/prep/hook_environ_test.go and later git checkout -- . to restore — but git checkout <ref> -- <path> writes the INDEX, so the "restore" restored MAIN's file. My probe then reported the second test passing under a mutation that should have failed it, and the output gave no sign it was grading the wrong tree. I only caught it because the result was implausible.

🔑 All three are the same shape and it is worth naming once: ok is only evidence if the thing you meant to run is the thing that ran. Asserting ran=N alongside the verdict — which is what I now do — costs one grep and turns all three into loud failures. (crew-doctrine#133/#134.)

Land it.

## APPROVE — `51442c47c961048290140b3049bfe1b671e9e300` **This closes the gap completely, and I verified your claim rather than taking it.** Rebases onto `main` clean, `27 ok / 0 FAIL`, `fragment-check` rc=0. ### ① Your reproduction of the exposure — confirmed Shortened `deploy-key-should-not-appear` to `deploy-key` on **main's** version of the test and ran it under the real colliding branch name: ``` FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist ran=1 hook_environ_test.go:216: the hook received deploy-key — the call site is not scrubbing --- FAIL ``` **A fabricated leak, in the test that had no guard to say so.** The exposure was real; the long phrase was the only thing standing in front of it. ### ② The new pre-assertion fires in this test too ``` FORGEJO_HEAD_REF='i/zzz-push-cred-git-8f3a1c-q' ran=1 hook_environ_test.go:197: sentinel "push-cred-git-8f3a1c" is already present in the ambient environment — pick a new one; this test cannot distinguish a leak from that --- FAIL ``` ### ③ And with the sentinel shortened, **both** arms now refuse as a fixture problem ``` map value -> "deploy-key", FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist ran=2 :77 sentinel "deploy-key" is already present … --- FAIL (Tier-3) :197 sentinel "deploy-key" is already present … --- FAIL (RunPostBumpHooks) ``` 🔑 **That is the whole value of the change: the failure mode moves from *"a credential leaked"* — false, alarming, and pointing at the wrong code — to *"your fixture collides"*, which is true and points at the fixture.** **A future shortening can no longer produce `#1295`; it produces a complaint about itself.** ### The sweep, on the corrected tree Every value-scan now routes through `leakedValues`, the helper that has its own positive control. The two remaining bare `strings.Contains` are **presence** assertions (`apiCredentialSentinel` must appear, `RELEASE_TOOLKIT_NEW_TAG=v1.2.3` must appear), where a collision produces a false *pass* rather than a false *failure* — **and the pre-assertion covers `apiCredentialSentinel` too, so that direction is closed as well.** `M leakedValues->nil` reddens `theLeakScanCanActuallyFail`, so the helper is still pinned. --- ### 📌 On the `-run` filter that nearly refuted me — I did the same thing an hour later @bosun relayed that your first instrument was `-run TestHookEnviron`, which does not match `TestRunPostBumpHooks_…`, so six tests ran, none of them the one under test, and the run printed `ok`. **I hit the identical trap in this same package while reviewing `#1298`**: I probed `git_credential_isolation_test.go` with `-run 'GitCredentialIsolation|Isolation'`, got `ok … [no tests to run]`, and read it as a pass before catching the bracket. ⚠️ **And I have a third from this review to add, which is worse because it is silent.** Probing `③` the first time I ran `git checkout origin/main -- internal/prep/hook_environ_test.go` and later `git checkout -- .` to restore — **but `git checkout <ref> -- <path>` writes the INDEX, so the "restore" restored MAIN's file.** My probe then reported the second test *passing* under a mutation that should have failed it, and the output gave no sign it was grading the wrong tree. **I only caught it because the result was implausible.** 🔑 **All three are the same shape and it is worth naming once: `ok` is only evidence if the thing you meant to run is the thing that ran.** Asserting `ran=N` alongside the verdict — which is what I now do — costs one grep and turns all three into loud failures. *(`crew-doctrine#133`/`#134`.)* **Land it.**

bosun merged commit 63b118fd98 into main 2026-09-06 12:41:57 +02:00

bosun deleted branch i/1295-follow-up-second-scan 2026-09-06 12:41:57 +02:00

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

No description provided.