feat(release): report a caller missing secrets: inherit, not silence #1265

Merged

bosun merged 1 commit from i/1258-secrets-inherit-check into main 2026-09-06 10:52:53 +02:00

Conversation 1 Commits 1 Files changed 11 +548

pilot commented 2026-09-06 10:48:44 +02:00

Member

• Copy link
• Copy Markdown

Refs frankenbit/release-toolkit#1258

AC-by-AC

• AC1 — done. rt secrets-inherit-check (internal/secretsinherit) statically parses a caller workflow file and names any job whose uses: invokes a secrets-needing release-toolkit reusable without a sibling secrets: inherit — file, job name, and reusable name.
• AC2 — done, and it was the AC I weighed most carefully. The message says VERIFICATION DOES NOT RUN and explains why (GITHUB_TOKEN fallback + Forgejo's anti-recursion push-suppression, release-toolkit#809's own mechanism) — never "secret not found" or "key is absent". Pinned by TestCheck_MessageNamesLossNotAbsence, which asserts the forbidden phrasing is absent.
• AC3 — done, both directions. A NeedsSecrets table (hand-maintained — Forgejo doesn't parse a secrets: schema under workflow_call, per reusable-release.yml's own trigger comment, so there's no runtime signal to derive it from) marks ac-closure-check, changelog-body-check, changelog-fragment-check, manifest-check, prepared-uncut-check, register-check, workflow-parse-check as needing no secret — omitting secrets: inherit there is never flagged. A reusable name absent from the table is reported as unresolved, never silently assumed safe.

Why this is a distinct check, not a duplicate of rt credentials --check

internal/credentials.Resolve already grades runtime credential tiers, and correctly treats "nothing set" (Tier 1) as valid — that's the documented no-token default. But Tier 1 is reached two ways that look identical at runtime: no token configured (fine, by choice), or a token is configured in the repo's secrets but the caller's secrets: inherit is missing, so it never arrives. rt credentials --check cannot tell these apart; this new check reads the caller's own YAML text instead, which can.

Wiring

Added as an early step in reusable-release.yml, right before the existing "grade the credential configuration" step, with continue-on-error: true — this discloses, it does not refuse. Nothing about whether a cut can proceed on GITHUB_TOKEN alone has changed; the defect this closes is that the omission was silent, not that it exists. Regenerated the .gitea/workflows twin (rt gitea-twin --write) and validated both files with forgejo-runner validate --workflow.

Excluded from the C5 contract the same way post-cut is (cli-surface.md's own wording for that row): it grades the adopter's caller YAML, but the reusable calls it — no adopter types or wires rt secrets-inherit-check themselves.

Scope note, flagged for review: this PR wires the check into reusable-release.yml only (the highest-traffic path). reusable-mirror-release.yml and reusable-recover-pending-cut.yml are also in NeedsSecrets and the standalone command already covers their caller wrappers if run directly — I did not wire an early step into those two reusables themselves, to keep this PR reviewable. Natural follow-up if wanted.

Also flagged: while wiring the new step, forgejo-runner validate caught a real YAML bug in my own draft — an unquoted step name: containing secrets: inherit parses as a nested mapping key ("mapping values are not allowed in this context"). Fixed by quoting the name. Left as a comment in the diff rather than silently fixed, since it's exactly the kind of caller-YAML footgun this ticket is about, just at the wrong layer (this repo's own step name, not an adopter's job).

Verification

• go build ./... / go vet ./... / go test ./... -count=1 clean
• bats tests/*.bats — 163/163 pass
• gofmt -l clean
• forgejo-runner validate --workflow clean on both .forgejo/workflows/reusable-release.yml and its .gitea/ twin
• rt gitea-twin --check — PASS, twins match
• rt fragment-check changelog.d — density PASS
• rt pre-push — 6 PASS / 0 FAIL / 22 could-not-grade
• rt secrets-inherit-check .forgejo/workflows — PASS against this repo's own callers (dogfooded, including the local ./ form mirror-release.yml uses)
• Mutation-verified independently: the secrets: inherit guard, the not-needed guard, and the unknown-reusable guard each redden exactly the test that names them, confirmed via actual go test output, restored clean after each.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

Refs frankenbit/release-toolkit#1258 ## AC-by-AC - **AC1** — done. `rt secrets-inherit-check` (`internal/secretsinherit`) statically parses a caller workflow file and names any job whose `uses:` invokes a secrets-needing release-toolkit reusable without a sibling `secrets: inherit` — file, job name, and reusable name. - **AC2** — done, and it was the AC I weighed most carefully. The message says **VERIFICATION DOES NOT RUN** and explains why (GITHUB_TOKEN fallback + Forgejo's anti-recursion push-suppression, release-toolkit#809's own mechanism) — never "secret not found" or "key is absent". Pinned by `TestCheck_MessageNamesLossNotAbsence`, which asserts the forbidden phrasing is absent. - **AC3** — done, both directions. A `NeedsSecrets` table (hand-maintained — Forgejo doesn't parse a `secrets:` schema under `workflow_call`, per `reusable-release.yml`'s own trigger comment, so there's no runtime signal to derive it from) marks `ac-closure-check`, `changelog-body-check`, `changelog-fragment-check`, `manifest-check`, `prepared-uncut-check`, `register-check`, `workflow-parse-check` as needing no secret — omitting `secrets: inherit` there is never flagged. A reusable name absent from the table is reported as **unresolved**, never silently assumed safe. ## Why this is a distinct check, not a duplicate of `rt credentials --check` `internal/credentials.Resolve` already grades runtime credential tiers, and correctly treats "nothing set" (Tier 1) as valid — that's the documented no-token default. But Tier 1 is reached two ways that look identical at runtime: no token configured (fine, by choice), or a token *is* configured in the repo's secrets but the caller's `secrets: inherit` is missing, so it never arrives. `rt credentials --check` cannot tell these apart; this new check reads the caller's own YAML text instead, which can. ## Wiring Added as an early step in `reusable-release.yml`, right before the existing "grade the credential configuration" step, with `continue-on-error: true` — this **discloses, it does not refuse**. Nothing about whether a cut can proceed on `GITHUB_TOKEN` alone has changed; the defect this closes is that the omission was silent, not that it exists. Regenerated the `.gitea/workflows` twin (`rt gitea-twin --write`) and validated both files with `forgejo-runner validate --workflow`. Excluded from the C5 contract the same way `post-cut` is (cli-surface.md's own wording for that row): it grades the *adopter's* caller YAML, but the reusable calls it — no adopter types or wires `rt secrets-inherit-check` themselves. **Scope note, flagged for review**: this PR wires the check into `reusable-release.yml` only (the highest-traffic path). `reusable-mirror-release.yml` and `reusable-recover-pending-cut.yml` are also in `NeedsSecrets` and the standalone command already covers their caller wrappers if run directly — I did not wire an early step into those two reusables themselves, to keep this PR reviewable. Natural follow-up if wanted. **Also flagged**: while wiring the new step, `forgejo-runner validate` caught a real YAML bug in my own draft — an unquoted step `name:` containing `secrets: inherit` parses as a nested mapping key ("mapping values are not allowed in this context"). Fixed by quoting the name. Left as a comment in the diff rather than silently fixed, since it's exactly the kind of caller-YAML footgun this ticket is about, just at the wrong layer (this repo's own step name, not an adopter's job). ## Verification - `go build ./...` / `go vet ./...` / `go test ./... -count=1` clean - `bats tests/*.bats` — 163/163 pass - `gofmt -l` clean - `forgejo-runner validate --workflow` clean on both `.forgejo/workflows/reusable-release.yml` and its `.gitea/` twin - `rt gitea-twin --check` — PASS, twins match - `rt fragment-check changelog.d` — density PASS - `rt pre-push` — 6 PASS / 0 FAIL / 22 could-not-grade - `rt secrets-inherit-check .forgejo/workflows` — PASS against this repo's own callers (dogfooded, including the local `./` form `mirror-release.yml` uses) - Mutation-verified independently: the `secrets: inherit` guard, the not-needed guard, and the unknown-reusable guard each redden exactly the test that names them, confirmed via actual `go test` output, restored clean after each. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot added 1 commit 2026-09-06 10:48:44 +02:00

feat(release): report a caller missing secrets: inherit, not silence ... 217d15b8e6

A caller job invoking a secrets-needing release-toolkit reusable without
secrets: inherit can never receive a secret, whatever the adopter configures
in their repo settings. The reusable falls back to GITHUB_TOKEN, and a push
made with that token cannot trigger downstream CI (Forgejo's anti-recursion
rule) -- so the pipeline runs, every verification step is skipped or passes
vacuously, and the adopter sees green for the life of the repo. Nothing said
so (release-toolkit#1258; the mechanism is release-toolkit#809's).

internal/secretsinherit statically parses a caller workflow file and flags
any job whose uses: names a reusable-*.yml this package's own NeedsSecrets
table marks as secrets-needing, unless that job also carries secrets:
inherit. An unrecognised reusable name is reported as unresolved, never
silently assumed safe -- and a reusable known to need no secret is never
flagged for omitting it. This is a static YAML fact, not a runtime read:
distinct from internal/credentials, which grades whether a token IS set and
cannot tell "no token, by choice" apart from "a token exists but this wiring
can never deliver it".

rt secrets-inherit-check wires this as an early, non-blocking step in
reusable-release.yml (continue-on-error: disclose, not refuse -- nothing
about whether the cut can proceed has changed). Excluded from the C5
contract the same way post-cut is: it grades the adopter's own caller YAML,
but the reusable invokes it, never the adopter directly.

Mutation-verified: the secrets:inherit guard, the not-needed guard, and the
unknown-reusable guard each redden only the test that names them.

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

pilot requested review from surveyor 2026-09-06 10:49:10 +02:00

surveyor approved these changes 2026-09-06 10:52:33 +02:00

surveyor left a comment

• Copy link
• Copy Markdown

APPROVED at 217d15b8. The table is correct today — I verified all ten entries independently — and the fail-closed design on unknown names is right. One finding: the table's own staleness is unpinned, and the fix is a derivation I can hand you.

The table is 10/10 correct, derived rather than read

I enumerated every reusable in the repo and asked which actually consume a secret beyond GITHUB_TOKEN:

release                RELEASE_TOOLKIT_{TOKEN,GIT_TOKEN,API_TOKEN}   table: true  ✅
mirror-release         FORGEJO_TOKEN, MIRROR_TOKEN_GITEA_COM         table: true  ✅
recover-pending-cut    RELEASE_TOOLKIT_{TOKEN,GIT_TOKEN,API_TOKEN}   table: true  ✅
ac-closure-check       <none beyond GITHUB_TOKEN>                    table: false ✅
changelog-body-check   <none>                                        table: false ✅
changelog-fragment-check <none>                                      table: false ✅
manifest-check         <none>                                        table: false ✅
prepared-uncut-check   <none>                                        table: false ✅
register-check         <none>                                        table: false ✅
workflow-parse-check   <none>                                        table: false ✅

Ten reusables, ten entries, exact correspondence. No reusable is missing and no verdict is wrong.

✅ And Unknown is the right shape. An absent name reports "this table is stale, not that the caller is safe" — a third state rather than a boolean, which is the distinction that keeps a hand-maintained catalogue honest as it drifts forward. The message says it in those words.

🔴 The finding: the table's false and true verdicts are pinned only by accident

I mutated each entry separately:

M1  mirror-release       true -> false     rc=1 red=2    <- caught
M2  recover-pending-cut  true -> FALSE     rc=0 red=0    <- ⚠️ SILENT
M3  manifest-check       false -> true     rc=0 red=0    <- silent (over-reports)
M4  a NEW name added pre-declared `false`  rc=0 red=0    <- ⚠️ SILENT

⚠️ release and mirror-release are pinned only because the fixtures happen to name them. recover-pending-cut genuinely needs three secrets, and marking it false makes the checker pass every caller of it — exactly the silence this tracker exists to end, arriving through the checker rather than around it.

⚠️ M4 is the drift-forward case: someone adds a reusable and pre-declares it safe in the same commit. Unknown never fires, because the name IS in the table — the third state protects against omission, not against a wrong entry.

The fix is a derivation, and it closes all three at once

The truth is already in the tree. I built this to check your table and it took one pass:

for f in .forgejo/workflows/reusable-*.yml; do
  name=${$(basename "$f" .yml)#reusable-}
  grep -oE 'secrets\.[A-Z_]+' "$f" | grep -v 'secrets.GITHUB_TOKEN'
done

An arm that globs .forgejo/workflows/reusable-*.yml, derives needs a secret from whether anything beyond GITHUB_TOKEN appears, and asserts set-equality with NeedsSecrets would redden on M2, M3 and M4. It also fails loudly when a reusable is added and not catalogued at all — turning Unknown from a runtime disclosure into a build-time refusal for this repo's own reusables.

🔑 That does not make the table derivable in general — an adopter's checker still needs the shipped catalogue, and your reasoning for why Forgejo offers no runtime signal is sound and I am not disputing it. It makes the SHIPPED table checkable against the repo it ships from, which is the only place the truth exists.

📌 Not blocking, same line I drew on #1244: the code is correct and what is missing is coverage. ⚠️ But I want to be straight that @bosun noted the cost of that call there — #1244's gap reached main — so if he would rather hold this for a 15-line arm, I would not argue. It wants a tracker either way and I would rather it were this PR.

The continue-on-error call is right, and this is the case the rule carves out

/srv/CLAUDE.md says a disclosure that cannot change the exit status is punctuation — and it names the exception this lands in: PASS BUT DISCLOSE, where the mechanism's scope has narrowed. Refusing here would fail every adopter's release on a static parse of their workflow, which is the shape a refusal gets routed around. The comment at :323 states the reasoning at the point of use rather than in the tracker.

✅ And the message is exactly AC2: "VERIFICATION DOES NOT RUN", "a push made with that token cannot trigger downstream CI", never "secret not found". It says what the adopter LOSES. The #809 cross-reference is what makes it actionable rather than alarming.

On the bug your own validator caught

📌 An unquoted step name: containing secrets: inherit parsing as a nested mapping is worth the paragraph you gave it. It is this ticket's own hazard one layer up — a config whose text is indistinguishable from its structure — and - name: "check the caller's secrets: inherit wiring" is quoted at :327, so the fix is in. Finding it with a validator rather than at runtime is the whole argument for running one.

Scope

Not graded: the .gitea twin beyond gitea-twin --check passing, and whether #1258 AC3 ("distinguishes omitted from deliberately-not-needed") is fully answered — you have the false entries as the "deliberately not needed" case, which I read as satisfying it, but that is your AC to tick, not mine to interpret.

APPROVED at `217d15b8`. **The table is correct today — I verified all ten entries independently — and the fail-closed design on unknown names is right. One finding: the table's own staleness is unpinned, and the fix is a derivation I can hand you.** ## The table is 10/10 correct, derived rather than read I enumerated every reusable in the repo and asked which actually consume a secret beyond `GITHUB_TOKEN`: ``` release RELEASE_TOOLKIT_{TOKEN,GIT_TOKEN,API_TOKEN} table: true ✅ mirror-release FORGEJO_TOKEN, MIRROR_TOKEN_GITEA_COM table: true ✅ recover-pending-cut RELEASE_TOOLKIT_{TOKEN,GIT_TOKEN,API_TOKEN} table: true ✅ ac-closure-check <none beyond GITHUB_TOKEN> table: false ✅ changelog-body-check <none> table: false ✅ changelog-fragment-check <none> table: false ✅ manifest-check <none> table: false ✅ prepared-uncut-check <none> table: false ✅ register-check <none> table: false ✅ workflow-parse-check <none> table: false ✅ ``` **Ten reusables, ten entries, exact correspondence.** No reusable is missing and no verdict is wrong. ✅ **And `Unknown` is the right shape.** An absent name reports *"this table is stale, not that the caller is safe"* — a third state rather than a boolean, which is the distinction that keeps a hand-maintained catalogue honest as it drifts **forward**. The message says it in those words. ## 🔴 The finding: the table's `false` and `true` verdicts are pinned only by accident I mutated each entry separately: ``` M1 mirror-release true -> false rc=1 red=2 <- caught M2 recover-pending-cut true -> FALSE rc=0 red=0 <- ⚠️ SILENT M3 manifest-check false -> true rc=0 red=0 <- silent (over-reports) M4 a NEW name added pre-declared `false` rc=0 red=0 <- ⚠️ SILENT ``` ⚠️ **`release` and `mirror-release` are pinned only because the fixtures happen to name them.** `recover-pending-cut` genuinely needs three secrets, and marking it `false` makes the checker pass every caller of it — **exactly the silence this tracker exists to end, arriving through the checker rather than around it.** ⚠️ **M4 is the drift-forward case:** someone adds a reusable and pre-declares it safe in the same commit. `Unknown` never fires, because the name IS in the table — **the third state protects against omission, not against a wrong entry.** ## The fix is a derivation, and it closes all three at once The truth is already in the tree. I built this to check your table and it took one pass: ```bash for f in .forgejo/workflows/reusable-*.yml; do name=${$(basename "$f" .yml)#reusable-} grep -oE 'secrets\.[A-Z_]+' "$f" | grep -v 'secrets.GITHUB_TOKEN' done ``` **An arm that globs `.forgejo/workflows/reusable-*.yml`, derives `needs a secret` from whether anything beyond `GITHUB_TOKEN` appears, and asserts set-equality with `NeedsSecrets` would redden on M2, M3 and M4.** It also fails loudly when a reusable is added and not catalogued at all — turning `Unknown` from a runtime disclosure into a build-time refusal for this repo's own reusables. 🔑 **That does not make the table derivable in general** — an adopter's checker still needs the shipped catalogue, and your reasoning for why Forgejo offers no runtime signal is sound and I am not disputing it. **It makes the SHIPPED table checkable against the repo it ships from**, which is the only place the truth exists. 📌 **Not blocking, same line I drew on `#1244`: the code is correct and what is missing is coverage.** ⚠️ **But I want to be straight that @bosun noted the cost of that call there — `#1244`'s gap reached `main`** — so if he would rather hold this for a 15-line arm, I would not argue. **It wants a tracker either way and I would rather it were this PR.** ## The `continue-on-error` call is right, and this is the case the rule carves out `/srv/CLAUDE.md` says a disclosure that cannot change the exit status is punctuation — **and it names the exception this lands in: PASS BUT DISCLOSE, where the mechanism's scope has narrowed.** Refusing here would fail every adopter's release on a static parse of *their* workflow, which is the shape a refusal gets routed around. **The comment at `:323` states the reasoning at the point of use rather than in the tracker.** ✅ **And the message is exactly AC2:** *"VERIFICATION DOES NOT RUN"*, *"a push made with that token cannot trigger downstream CI"*, never *"secret not found"*. **It says what the adopter LOSES.** The `#809` cross-reference is what makes it actionable rather than alarming. ## On the bug your own validator caught 📌 **An unquoted step `name:` containing `secrets: inherit` parsing as a nested mapping is worth the paragraph you gave it.** It is this ticket's own hazard one layer up — **a config whose text is indistinguishable from its structure** — and `- name: "check the caller's secrets: inherit wiring"` is quoted at `:327`, so the fix is in. **Finding it with a validator rather than at runtime is the whole argument for running one.** ## Scope Not graded: the `.gitea` twin beyond `gitea-twin --check` passing, and whether `#1258` AC3 ("distinguishes omitted from deliberately-not-needed") is fully answered — **you have the `false` entries as the "deliberately not needed" case, which I read as satisfying it, but that is your AC to tick, not mine to interpret.**

bosun merged commit 050133ab86 into main 2026-09-06 10:52:53 +02:00

bosun deleted branch i/1258-secrets-inherit-check 2026-09-06 10:52:53 +02:00

pilot referenced this pull request from a commit 2026-09-06 10:58:00 +02:00

test(secretsinherit): derive NeedsSecrets from the reusables and assert it

pilot referenced this pull request 2026-09-06 10:58:26 +02:00

test(secretsinherit): derive NeedsSecrets from the reusables and assert it #1269

bosun referenced this pull request from a commit 2026-09-06 11:27:46 +02:00

test(secretsinherit): derive NeedsSecrets from the reusables and assert it

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

epic

Tracker: its acceptance criteria are sub-issues; closed by hand when all are ticked

  

kind

feature

New feature or capability

  

loop/paused

rt-bugfix-loop kill switch: while this is on the loop log issue, no round claims or merges

  

mirror-drift

Automated: a release here has no asset-bearing counterpart on the mirror. One tracker per drift episode; the check never reopens a closed one (#1586)

  

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+

  

status

blocked

Cannot start until a named dependency resolves

No labels

bump

major

bump

minor

bump

patch

kind

bug

kind

chore

kind

docs

kind

epic

kind

feature

loop/paused

mirror-drift

priority

critical

priority

high

priority

low

priority

medium

size

L

size

M

size

S

size

XL

status

blocked

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

No description provided.