Release workflow: republish recovery path + RC/loop-guard robustness — PR #177 Blocker 3 - #181
Release workflow: republish recovery path + RC/loop-guard robustness — PR #177 Blocker 3#181jgruberf5 wants to merge 11 commits into
Conversation
…kflow PR #177 review (bonnyr-f5) — BLOCKER 3 (workflow side; the main-branch lock itself is a repo-config change handled separately). Republish recovery: if publishing failed after release-final succeeded (bake error, ghcr 5xx, Sigstore outage, the 90-min timeout), the state was tag + GitHub Release + no images, with no recovery — re-running on main hits the `release:` loop guard, and workflow_dispatch bumps to the NEXT version instead of republishing the existing one. Added a `publish_only: <tag>` dispatch input: it skips derivation/bump/CI-poll/tag-creation, asserts the tag EXISTS, and runs release-publish against it. Purely additive — normal rc/final/manual paths are unchanged (new branch/conditions only). Also from the review: - RC numbering is now max-based, not count-based (release.yml). Counting via `wc -l` breaks the moment any rc tag is deleted: the count drops and the next staging push recomputes an existing tag, which then fails to create. Now takes the highest existing rc number + 1. - The loop guard emits a `::warning::` when it suppresses a release, naming the offending commit. Before, a normal commit beginning `release: ` or ending `[skip ci]` silently skipped publication while the job reported success. YAML validates; the guard's grep is also converted to a pipe-free here-string. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
mwiget
left a comment
mwiget
left a comment
There was a problem hiding this comment.
The recovery path is well built and I checked the parts that could have quietly been wrong -- all four came out right. One thing needs fixing before this lands, and it is in the other half of the PR: the new loop-guard warning fires on the happy path and gives misleading advice there.
Verified correct
publish_onlyrepublishes the tagged tree, not current HEAD.release-publishalready checks outref: v${{ needs.preflight.outputs.new_version }}withfetch-depth: 0, so a recovery run rebuilds the exact commit the tag points at. This was the thing most likely to be subtly wrong in a "republish" path and it isn't.- The loop guard doesn't eat the recovery run.
HEAD_COMMIT_MSGcomes fromgithub.event.head_commit.message, which is null onworkflow_dispatch->MSG=""-> no match ->should_run=true. I ran the guard body with an empty message to confirm. Worth stating explicitly, because the scenariopublish_onlyexists for is precisely "therelease: vX [skip ci]commit is HEAD of main", and if the guard had keyed offgit loginstead of the event payload the feature would have been dead on arrival. - Job wiring is clean. Every other job is keyed on an exact
release_kind, sopublish_onlyrunsguard -> preflight -> release-publishand nothing else:e2e-gatewantsmanual,release-rcwantsrc,release-finalwantsfinal. Noalways()job picks it up by accident. - Max-based RC numbering is right and safe.
grep -E '^[0-9]+$'returning 1 when there are no rc tags does not sink the pipeline: this step has noset -o pipefail(GitHub's default shell isbash -e {0}), so the substitution takestail's status and${RC_MAX:-0}yields 1 as intended. Numericsort -nalso gets rc.10 right, which the oldwc -ldid too but only by accident.
Blocking
The new ::warning:: fires on every successful release, and what it says there is wrong. release-final commits release: v${NEW} [skip ci] and pushes to main; that push touches VERSION, so it is not filtered by paths-ignore and it re-triggers Release; the guard matches and now annotates the run with "nothing was published -- rename the commit or re-run the workflow." On the happy path the release was published, by the previous run, and neither piece of advice applies. Every release will carry a yellow annotation telling the operator to do something harmful, which is a reliable way to teach people to ignore the annotation that matters. Details and a suggested split inline.
Same line also emits '\'' verbatim -- that idiom escapes a quote inside a single-quoted string; inside double quotes it is just four literal characters.
Notes, non-blocking
release_notesisrequired: trueandversion_bumphas a default, so apublish_onlyrecovery still makes the operator fill in two fields that are then ignored. GitHub has no conditional-required inputs, so this is probably just worth a word in thepublish_onlydescription.- No CI has run on this PR at all (zero workflow runs against
4e3ae14b, ~5 min after opening). Since only.github/workflows/release.ymlchanged and that path isn't in ci.yml'spaths-ignore, I'd expect a run. I'll re-check; if CI genuinely never reports here, the required-check config is worth a look separately.
Re-review as soon as the guard message is sorted -- the publish_only half I'm happy with as it stands.
| if echo "$MSG" | grep -qE '^release: |^\[skip ci\]|\[skip ci\]$'; then | ||
| echo "Skipping: head commit is a release commit or has [skip ci]" | ||
| if grep -qE '^release: |^\[skip ci\]|\[skip ci\]$' <<< "$MSG"; then | ||
| echo "::warning::Release suppressed by the loop guard: head commit '\''${MSG%%$'\n'*}'\'' begins with 'release: ' or carries [skip ci]. If this was a normal change (not an automated release commit), nothing was published -- rename the commit or re-run the workflow." |
There was a problem hiding this comment.
Two problems on this line, one of them behavioural.
1. It fires on the happy path with advice that is wrong there. release-final does git commit -m "release: v${NEW} [skip ci]" and pushes to main. That commit touches VERSION, which is not in this workflow's paths-ignore, so the push re-triggers Release, the guard matches, and the run is annotated:
Release suppressed by the loop guard: head commit ... nothing was published -- rename the commit or re-run the workflow.
But on that path everything was published, by the run that created the commit. So every single successful release ends with a yellow annotation telling the operator to rename a commit or re-run a workflow, both of which would be actively wrong to do. The signal you actually want -- "a hand-written commit accidentally tripped the guard" -- is then buried in a stream of identical false ones.
The two cases are cheap to tell apart, since our own commits have a fixed shape:
MSG="$HEAD_COMMIT_MSG"
if grep -qE '^release: |^\[skip ci\]|\[skip ci\]$' <<< "$MSG"; then
FIRST="${MSG%%$'\n'*}"
if grep -qE '^release: v[0-9]+\.[0-9]+\.[0-9]+' <<< "$FIRST"; then
echo "::notice::Loop guard: skipping our own release commit \"$FIRST\" (expected)."
else
echo "::warning::Release suppressed by the loop guard: head commit \"$FIRST\" begins with 'release: ' or carries [skip ci], so nothing was published. If this was a normal change, rename the commit or dispatch the workflow manually."
fi
echo "should_run=false" >> "$GITHUB_OUTPUT"2. '\'' is the wrong escape here. That idiom ends a single-quoted string, emits an escaped quote and reopens -- it only means anything inside single quotes. This string is double-quoted, so bash passes all four characters through. Run as written:
::warning::Release suppressed by the loop guard: head commit '\''release: v4.0.0 [skip ci]'\'' begins with ...
\" (or nothing) is what you want. The ${MSG%%$'\n'*} first-line trim itself is fine -- I checked it against a multi-line release commit and it strips correctly.
| description: "Release notes (one-line summary)" | ||
| required: true | ||
| type: string | ||
| publish_only: |
There was a problem hiding this comment.
Non-blocking: release_notes is required: true and version_bump carries a default, so a recovery dispatch still forces the operator to fill in two fields this path ignores. GitHub has no conditional-required inputs, so the fix is probably just a sentence here -- e.g. "... Leave empty for a normal release. When set, version_bump, run_e2e and release_notes are ignored." -- so nobody agonises over what release notes to type while chasing a failed publish.
| echo "bump_type=$BUMP" >> "$GITHUB_OUTPUT" | ||
| echo "Version: $CURRENT -> $NEW (bump: $BUMP)" | ||
|
|
||
| - name: Check republish tag exists |
There was a problem hiding this comment.
Good that this asserts existence rather than trusting the input, and preflight checks out with fetch-depth: 0, so git tag -l actually sees the tags.
Worth knowing (no change needed, it works either way): TAG here is rebuilt as v${{ steps.version.outputs.new }} where new is ${TAG#v}, so both v4.0.0 and 4.0.0 normalise correctly, and anything that isn't a tag -- a branch name, a typo -- lands in the error branch rather than silently republishing something else.
| # count-based (#177 review): counting breaks if any rc tag is ever | ||
| # deleted -- the count drops and the next push recomputes an existing | ||
| # tag, which then fails to create. | ||
| RC_MAX=$(git tag -l "v${TARGET}-rc.*" \ |
There was a problem hiding this comment.
Correct fix, and it survives the grep returning 1 on a version that has no rc tags yet: this step doesn't set -o pipefail (GitHub's default is bash -e {0}), so the substitution takes tail's status and ${RC_MAX:-0} gives 1. Worth pinning that in the comment -- if anyone later adds set -euo pipefail to this step for tidiness, the first RC of every version starts failing here, which is an unpleasant thing to debug.
sort -n also orders rc.10 after rc.9 properly.
…e escaping mwiget's re-review of #181. The new loop-guard warning fired on every successful release: release-final pushes `release: vX [skip ci]` to main, that push touches VERSION (not in paths-ignore) so it re-triggers Release, the guard matches, and the run was annotated "nothing was published -- rename the commit or re-run the workflow." But it WAS published, by the run that made the commit, so both pieces of advice are wrong there -- and a yellow annotation on every release trains operators to ignore the one that matters. Now: our own `release: vX.Y.Z` commit gets a ::notice:: (expected, already published); only a hand-written commit that trips the guard gets the ::warning::. Also fixed the `'\''` escaping -- that idiom only means something inside single quotes; this string is double-quoted, so it printed the four characters literally. Uses `\"` now. Non-blocking, also addressed: - publish_only description states version_bump / run_e2e / release_notes are ignored on a recovery dispatch. - Pinned in a comment that the RC-number step must not gain `set -o pipefail` (grep returns 1 with no rc tags yet; without pipefail ${RC_MAX:-0} yields 1). Guard split verified: release commit -> notice, hand-written [skip ci] -> warning, normal commit -> should_run=true. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
Fixed. Our own Non-blocking: the |
mwiget
left a comment
mwiget
left a comment
There was a problem hiding this comment.
Approving. All three points addressed, and I ran the new guard logic through the cases that matter:
| head commit | result |
|---|---|
release: v4.0.0 [skip ci] (+ body) |
::notice:: -- expected, published by the previous run |
release: prep notes for the 4.0 launch |
::warning:: -- nothing published, rename or dispatch |
docs: tweak readme [skip ci] |
::warning:: |
"" (workflow_dispatch) |
should_run=true |
fix: something |
should_run=true |
That's exactly the split I was after: the happy path no longer tells an operator to undo a release that worked, and the case worth noticing still shouts. The "$FIRST" quoting renders correctly now too -- no more literal '\'' -- and the first-line trim still works on a multi-line message.
The pipefail comment on the RC step is the right kind of note to leave: it records why the absence of a setting is load-bearing, which is the sort of thing someone tidies away otherwise.
Standing observation, not a blocker and not yours to fix: this PR still has zero workflow runs -- no CI, no Integration Branch Rebuild, nothing, across both pushes. A PR touching only .github/workflows/release.yml appears to trigger nothing, while #180 and #182 (which also touch workflows, alongside other paths) run the full suite. If CI Gate is a required check, that means release-workflow-only changes can't satisfy branch protection -- worth a maintainer looking at separately from this PR.
Everything I checked on the substance holds up: the republish path builds from the tagged tree, the guard doesn't eat a dispatch, and no other job picks up publish_only by accident.
|
Non-blocking follow-up on your CI observation: I confirmed it — this branch still reports no CI checks at all, even though |
Review: BLOCKReviewed under the review-discipline pipeline (invariant sweep + context-isolated cold audit at Blocker 1 —
|
bonnyrf5 aggregate review, #181. Injection (release.yml:117,205): raw ${{ inputs.publish_only }} — free-text workflow_dispatch input — was interpolated straight into two run: scripts, so a crafted value ran as shell in a job that carries a token. Both sites now read it from env (PUBLISH_ONLY), the same indirection the CI-status step already uses, so the value is never part of the script text. Republish guardrails (release.yml republish path): repointing :latest across all seven public images accepted any existing tag. Added, in the publish_only branch: - shape guard: ^v?[0-9]+\.[0-9]+\.[0-9]+$ — refuses rc/pre-release and arbitrary strings, so :latest can only track a final release. - recency guard: sort -V against VERSION — refuses a tag older than the current release, so a republish can't move :latest backward. Verified: v4.0.0 accepts, v4.0.0-rc.1 rejects on shape, v3.1.6 rejects on recency, "; rm -rf /" rejects on shape. actionlint clean on the changed lines (the 4 remaining findings are pre-existing style nits in unrelated steps). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
mwiget
left a comment
mwiget
left a comment
There was a problem hiding this comment.
Re-approving at d45dc87b — 25/25 green, and I exercised the new guard rather than reading it.
The env indirection is the right shape, and its reach is bigger than the two lines it touches. publish_only was flowing into steps.version.outputs.new, which is then interpolated raw at release.yml:251 (TAG="v${{ steps.version.outputs.new }}") and at the publish job's ref: (release.yml:692). Validating the shape before it becomes an output sanitizes every one of those consumers at the source — that's a stronger fix than env-quoting the two original sites would have been, and worth noting because a reader may see the two env: blocks and assume the blast radius was two run: steps.
I extracted both guards verbatim and ran the matrix (CURRENT=4.0.0 unless shown):
| input | result |
|---|---|
v4.0.0 |
accept → 4.0.0 (republish same version — the recovery case) |
4.0.0 (no v) |
accept → 4.0.0 |
v4.0.1 |
accept |
v10.0.0 |
accept — sort -V, not lexical, so 10 > 4 |
v4.0.0-rc.1 |
reject (shape) |
v3.1.6 |
reject (recency) |
v3.9.9 |
reject (recency) |
v4.0.0 with CURRENT=4.0.1 |
reject (recency) |
; rm -rf / |
reject (shape) |
v4.0.0$(id) |
reject (shape) |
The equal case accepting is the important one — republishing the current version is the recovery path this feature exists for, and an off-by-one there would have made the whole input useless. sort -V also gets the 10-vs-4 case right, which a lexical compare wouldn't.
Non-existent-but-newer tags (v4.0.1 when nothing is tagged) still pass these two guards, but the existing Check republish tag exists step at release.yml:248 catches them before anything publishes, so the three checks compose to: valid shape → not older → actually exists.
One follow-up, out of scope for this PR — the same injection class remains on release_notes. It's also free-text type: string, required: true, and it's interpolated raw into five run: blocks: release.yml:580 (NOTES="${{ inputs.release_notes }}"), 612 (the git commit -m body), 620 (git tag -m), 631 (gh release create --notes), and 647 (the step summary). All five are pre-existing on staging — git diff against the merge base shows this PR doesn't touch any of them — so I'm not blocking on it. But publish_only was hardened precisely because a dispatch input reaching a run: in a token-carrying job is a bad idea, and release_notes reaches five of them on the manual path that actually gets used. No open issue covers it (I checked; #124 is the closest and is about shellcheck/branch-protection/migration-collision, not this). Same env: treatment would close it in one pass.
…invariant mwiget non-blocking note on #181: the comment told the next editor the list 'MUST stay in sync with ci.yml's push-trigger paths-ignore list' — but #182 removes that ci.yml list, so the instruction would have them re-add exactly what was removed. Reworded to state the actual invariant the preflight SHA poll depends on (CI must be a superset of Release — every SHA that triggers Release must also have a CI run to match), and to say explicitly not to re-add a ci.yml paths-ignore. The phrasing holds regardless of the #181/#182 merge order. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
Two follow-ups from your notes:
|
mwiget
left a comment
mwiget
left a comment
There was a problem hiding this comment.
Re-approving at 9aa2130f — 25/25 green.
This is a better fix than the one I asked for. I'd suggested the comment be reworded because it now describes a sync that #182 deliberately breaks; instead of describing the new state, 9aa2130f names the invariant underneath it — CI ⊇ Release, because the preflight SHA poll needs every Release-triggering SHA to have a matching CI run — and then states the operational rule that follows: keep this list a subset of whatever ci.yml runs on. That survives the next change to either file, which the original "MUST stay in sync" phrasing did not.
The parenthetical is the part that actually prevents the bug: "do NOT re-add a ci.yml paths-ignore to 'match' this list — that would break the invariant." Someone reading only release.yml would otherwise see an apparent divergence and 'fix' it in exactly the direction that reintroduces the timeout — and re-adding it would also undo #182's secret-scanning-on-doc-only-PRs change as collateral. Calling that out by name is worth more than the reword.
Everything from my previous pass stands: the shape/recency guards verified across ten inputs, the equal-version accept that makes the recovery path usable, and the env indirection sanitizing steps.version.outputs.new for its downstream consumers at release.yml:251 and :692.
The inputs.release_notes follow-up is still open and still out of scope here — five raw interpolations into run: blocks, all pre-existing on staging, no issue tracking them yet.
Review: REVISEReviewed under the review-discipline pipeline at The injection question on
|
… on preflight bonnyr-f5 REVISE review of #181. All findings reproduced and confirmed. BLOCKER — release_notes was inlined raw into 5 run: sites in release-manual (contents: write + token), the class sibling of the publish_only injection this PR fixes. Proven: `"; touch /tmp/PWNED; #` executes. Added a job-level RELEASE_NOTES env and reference "$RELEASE_NOTES" at every site — both free-text inputs are now indirected, not one. MAJOR — the recency guard compared publish_only against `cat VERSION` on the dispatch ref, but :latest is set by whatever release-publish last ran, so a stale main VERSION let an older tag repoint :latest backward. Now compares against the highest final `v*` tag (the idiom already used elsewhere in the file). MAJOR — the publish job's publish_only disjunct ran under always() with no preflight-success check, so the new validators weren't load-bearing. Gated it on `needs.preflight.result == 'success'`, matching release-manual. MAJOR — my earlier reworded comment claimed "ci.yml runs on every push", which is false until #182 lands (ci.yml still has a byte-identical push paths-ignore). Reworded to state the CI ⊇ Release invariant without the false claim, and to point at #182's subset check. Acknowledged (documented): the ::notice:: dead-code arm, republish-without- existence-check, v04.0.0 shape gap, missing publish_only docs, and the absent actionlint gate. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 — all valid, reproduced. Fixed in
Acknowledged: |
Review: REVISERound 2, cold re-audit of The env-indirection fix is complete where it counts: zero Major 1 — the
|
…ts skipped bonnyr-f5 round-2 REVISE of #182. Both majors reproduced and fixed. MAJOR 1 — gitleaks --no-git scanned the worktree, missing a secret added then REMOVED within the branch (permanently fetchable from a public clone). Verified: --no-git says "no leaks" on such history; git mode + --log-opts catches it. Now checks out fetch-depth: 0 and scans the PR/push COMMIT RANGE in git mode (pull_request base..head; push before..sha; first push -> all history). MAJOR 2 — cancel-in-progress: false does not stop GitHub cancelling a PENDING run in the same per-ref group, so a docs-only push could still starve a release's CI run. On main/staging the concurrency group now includes the SHA, so every push gets its own group and nothing cancels; feature branches keep the per-ref group. MINOR — the CI Gate accepted `skipped` for the four always()-run gates, so a future path-filter would go green with the check never run. Skipped is now a failure for version-consistency / shellcheck / secret-scan / script-selftests. MINOR — the self-test gate asserted >=1 PASS but not completion; an early exit after case 1 would pass. Now also requires the END-SELF-TEST marker (branch- independent; catches the #179 early-exit shape). MINOR — make shellcheck: `xargs shellcheck` on an empty list exits 0 on BSD. Now fails on an empty list and includes the (extensionless) .githooks; full corpus clean. Acknowledged: the generic-api-key rule's test-dir allowlist (same fixture tension as private-key; content-scoping is a follow-up); the release.yml paths-ignore comment lives in #181 and is handled there. Merge #180 first (merge commit). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 — thanks for the round-2 cold audit. Every finding was reproduced before touching code; dispositions and fixes below. All in Majors — all VALID, all fixedMajor 1 — Major 2 — loop guard read the whole message, reported the subject ( Major 3 — INV-14: CI⊇Release coupling still prose ( Minors
Nits
Commit: |
Majors:
- Hoist the :latest recency guard out of the publish_only branch so it
covers every path that reaches release-publish (final, manual,
publish_only); rc is exempt. release-manual from a maintenance branch
could otherwise tag+publish a version below the highest final tag and
drag :latest backward.
- Loop guard now matches the SUBJECT line only, not the whole commit
message: a skip-ci marker line in a commit body no longer suppresses a
legitimate release while the warning quotes a marker-free subject.
- paths-ignore comment reworded to state the CI ⊇ Release invariant in
the correct direction ("this list ⊇ ci.yml's push paths-ignore"),
fixing the inverted clause; the automated check stays deferred to #182.
Minors:
- Add a least-privilege top-level permissions: contents: read; preflight
gets actions: read for its gh run list poll. Higher-privilege jobs keep
their own blocks.
- Env-indirect github.ref_name across all run: blocks (REF_NAME), same
class as the release_notes/publish_only hardening.
- Validate the VERSION file is MAJOR.MINOR.PATCH before it reaches
$GITHUB_OUTPUT (a newline would inject a second output key).
- Serialize the :latest writer (release-publish) on one global
concurrency group so republish + final from different refs cannot push
:latest concurrently.
Nits:
- release_notes / run_e2e no longer required (publish_only ignores them).
- Escape TARGET's dots in the RC-number sed pattern.
Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
af682d3 to
a11ce5e
Compare
bonnyr-f5 round-3 BLOCK on #179. The round-2 blockers stay fixed (--first-parent reverted, SIGPIPE deterministic-fix, detector pair identical); this addresses the predicate itself, which round 2 did not examine. BLOCKER 1 — the marker regex was a WORD BOUNDARY, not a line anchor, so it fired on uppercase prose anywhere in a body while the comment claimed "footer only". Over the live v3.1.6..origin/main range the extractor shipped a fragment of this script's OWN changelog prose (#178) to operators as migration guidance. Both detectors are now anchored to the spec footer position, `^(**)?BREAKING[ -]CHANGE` (markdown-bold allowed), and are byte-identical across the two scripts (INV-15). Re-running the extractor over that range: the #178 prose bullet is gone; the real #2 container-hardening break (its only breaking signal in the whole range) is still detected, so the 4.0.0 major derivation is unchanged. I deliberately did NOT require a trailing colon: #2 declares its break as a line-start marker with no colon and no type!: subject, so a colon rule would UNDER-detect and silently ship 4.0.0 as a patch — a worse failure than prose. Also fixed from the same review: - _breaking_note emits the anchored footer paragraph(s), not the first prose match; captures EVERY footer (a second one was dropped) with no line cap (n>=40 truncated silently). - extract-breaking-changes.sh now fails CLOSED on an unresolvable range (validates both refs) instead of 2>/dev/null||true -> empty output rc 0, which fooled the reviewer mid-audit. Matches compute's fail-closed behaviour. - The consistency guard read %b only while the loop reads %s AND %b: it was blind to subject-declared breaks. It now re-derives from BOTH subjects and bodies, so dropping the loop's subject detector can no longer leave feat!: as a silent patch. - Self-test hardening (INV-16): the extractor's _expect_nonempty could never fail (it checked the note only inside a failure conjunct) -> rewritten to assert each detector fires POSITIVELY, with mid-line-prose / lowercase / indented negatives and a two-footer case. compute gains an assertion counter that fails on zero assertions, coverage for the unresolvable-since-tag guard, and a robust BASH_SOURCE self-invocation ($OLDPWD/$0 broke any non-cwd-relative call). - Nits: case-insensitive bang so `Feat!:` bumps major; `local _b`; harmonized the determinism comments (the bug is deterministic past the ~64 KB pipe buffer, not a race). Handed to sibling PRs (bonnyr flagged, out of this diff): the release.yml call sites' `|| true` and the head -40/-50 note truncation live in #181; wiring the extractor --self-test into CI plus a byte-identical-function assertion lands in #182's script-selftests job. I'll push both. Verified: shellcheck -S style clean on both; compute self-test 9/9 rc 0; extractor self-test 12/12 rc 0; detectors byte-identical; extractor re-run over the real release range shows the prose bullet gone and the real break retained. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
…ating bonnyr-f5 #179 r3, the two items handed to this PR. 1) The three extract-breaking-changes.sh call sites wrapped the script in `|| true`, which re-masked the fail-closed exit the script just gained: an unresolvable range would yield empty output and a release would publish with no breaking-change section and no signal. Dropped `|| true` at all three sites; GitHub Actions runs `run:` under `bash -eo pipefail`, so a bad range now aborts the release step. It cannot abort spuriously -- LAST_FINAL is always either empty (else-branch, no call) or a real tag from `git tag -l`. 2) The commit list was cut with a bare `head -40` / `head -50`, silently dropping 17 of 67 commits from published notes. It now caps at 300 (generous enough that real ranges are complete) and appends an explicit "… and N more commit(s)" line when it truncates. The `|| true` added on the `grep -v "^- release: "` filter guards ONLY the filter (an all-release range leaves it with no output, rc 1 under pipefail) -- it does not touch the breaking-change detector, which stays fail-closed. Verified: YAML + actionlint clean (only the pre-existing SC2129 summary-step style nits remain); cap+notice logic unit-tested (caps and appends the remainder line); empty-filtered range yields empty with no abort. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
Cross-PR items from @bonnyr-f5's #179 r3 that live in this PR — both fixed in
Verified: YAML + actionlint clean (only the pre-existing SC2129 summary-step nits remain); cap+notice logic unit-tested; empty-filtered range yields empty with no abort. This is on top of the round-2 |
Review: BLOCKRound 3, cold re-audit of Round 2's findings are genuinely closed, and I checked them rather than taking the commit message for BLOCKER 1 — the republish path overwrites an artifact it already published, and it is not idempotent
But "org.opencontainers.image.created" = timestamp()so every rebuild produces a different digest, deterministically — not occasionally. Every bake Nothing distinguishes the intended case ("publish never ran") from the destructive one ("images already BLOCKER 2 — this PR's own commit bodies contain literal
|
…tract Addresses bonnyr-f5 round-3 BLOCK on PR #181. BLOCKER 1 - republish overwrote the tag it calls immutable and was not idempotent. docker-bake.hcl stamped org.opencontainers.image.created with timestamp(), so every rebuild produced a different digest (reproduced: two identical bakes gave ...36Z then ...40Z), and every bake target pushes :VERSION unconditionally. Fixes: - docker-bake.hcl: replace timestamp() with an injectable CREATED variable, empty by default so a plain bake of a given tree is byte-reproducible; CI sets it to the release commit's committer date. release-publish also passes SOURCE_DATE_EPOCH to normalise layer mtimes, so the same tag rebuilds to the same digest and its cosign/SBOM/SLSA attestations still apply. - release.yml: before pushing, probe the registry with docker manifest inspect and refuse by default when the :VERSION manifest already exists; a new force dispatch input overrides deliberately. The normal recovery case (a tag whose publish never completed) has no manifest yet and passes. Major findings also fixed: - Makefile push-images now runs the same recency guard before baking with --push ROLLING_TAG=latest, so a stale local tree cannot repoint :latest backward (override with FORCE_LATEST=1). - workflow_dispatch of a manual release is now gated to main, or a ref already merged into main, so a side branch cannot publish an unreviewed tree as :latest. - The recency check is re-run inside the release-publish concurrency-guarded step immediately before the push, closing the TOCTOU where preflight's check (outside the critical section) let a lower version write :latest last. - The loop guard now fails the run on an unexpected suppression instead of reporting a silent green; our own release commit still ends green as a notice. A release that published nothing is no longer green. - Corrected the paths-ignore comment: #182 removes ci.yml's push paths-ignore entirely (it does not add a subset check), so CI is a strict superset of Release; fixed the two inverted remediations. - Rewrote the loop-guard header comment to describe the real mechanism - GitHub's native marker handling drops our release commits before the workflow starts; the guard is a backstop. Verification: python yaml parse OK; actionlint reports no new findings (the 4 pre-existing SC2129 style notes are unchanged); docker buildx bake --print confirms CREATED is deterministic; guard, recency, probe and Makefile-guard logic unit-tested. Note for merge: this branch's commit bodies quote a CI-control marker as literal text, so it MUST be merged with a merge commit (not squashed) or the squash body will carry the marker and suppress CI and Release. Documented on the PR. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 Round-3 addressed in BLOCKER 1 — republish overwrote the "immutable" tag and was not idempotentReproduced the non-determinism directly: Two identical bakes, two different Both halves of your class fix are in:
BLOCKER 2 — this branch's commit bodies quote a CI-control markerConfirmed: Important This PR MUST be merged with a merge commit — do NOT squash. GitHub's default squash body concatenates the branch's commit messages, so a squash head commit would carry the literal skip-ci marker and silently suppress both CI and Release (the workflow never starts, so there is no annotation anywhere). Merge with a merge commit, or hand-edit the squash body to strip the marker, before merging. (My own commit Major —
|
Addresses every non-blocker bonnyr-f5 raised in round 3. Archives (Major): gitleaks defaulted to --max-archive-depth 0, so a secret shipped inside a tracked tarball was invisible. The scan now runs with --max-archive-depth 2. Proven with a synthetic fixture: a private key inside a .tar.gz -> "no leaks found" at depth 0, "leaks found: 1" (secrets.tar.gz!id_rsa) at depth 2. Baseline (Major): the per-push gate only scanned each change's commit range, so anything already in history was never re-examined. Added secret-baseline.yml -- a weekly schedule plus workflow_dispatch that runs gitleaks over full history with the same assertion backstop. CI Gate (Major): the aggregator never checked needs.changes.result. If change detection failed, ~21 gates resolved to skipped, the loop accepted skipped, and the required check printed PASSED. It now fails when changes did not succeed. Mutation-tested: with change detection failed and test jobs skipped, the old gate went green, the new gate goes red. Local == CI (Major, #166): the four gates ci.yml added were unrunnable locally. Added make targets (version-check, secret-scan, commit-lint, script-selftests) aggregated as ci-gates, and made pre-push depend on it. The secret scan + its whole assertion backstop now live in scripts/secret-scan.sh, called identically by the CI job, the baseline workflow, and make -- one source of truth. Marker enforcement (Minor -> real): the skip-CI-marker rule was documentation only. Added scripts/lint-commit-markers.sh plus a commit-lint CI gate and a pre-push hook step that fail a range carrying a skip-CI marker, or a line-start prose form that would spuriously trigger a major release. A genuine conventional footer still passes. Mutation-tested across eight cases; passes on this PR's 13-commit range. Digest pin (Nit): the movable v8.30.1 tag is replaced by ghcr.io/gitleaks/gitleaks@sha256:c00b6bd0... (v8.30.1 kept in a comment). release.yml stale comments (Major): the ":23-26" note referenced a ci.yml paths-ignore that no longer exists, and the ":150-157" note claimed cancel-in-progress: true for main/staging where it is now false. Comment text corrected to match reality; no release logic touched (that is #181's domain). Cross-PR items are intentionally left to merge order: the sync-version-artifacts second-tag reproduction is fixed in #180's head, and compute_version_bump's exit-0-on-fail in #179's head. Merge #180 first, merge-commit not squash. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
Review: BLOCKRound 4, cold re-audit of BLOCKER — the republish idempotency probe reads any error as "tag is free" (INV-24)
Major findings (all execution-proven unless noted)
Genuinely fixed, and verified by executionINV-21 clean (no Cross-PR (for the integration owner)#181 and #182 now both edit Review Assessment
Findings & Action Items
|
The "refuse to overwrite an already-published tag" guard probed the registry with `docker manifest inspect "$REF" >/dev/null 2>&1` and read any non-zero exit as "manifest absent -> safe to publish". But a non-zero exit is ambiguous: auth expiry, a GHCR 5xx/429, a DNS/network blip, or a malformed ref all exit non-zero too. So a transient error made the guard fail OPEN and let bake overwrite the immutable :VERSION tag, orphaning the digest-bound cosign / SBOM / SLSA attestations (bonnyr-f5 #181 round 4, INV-24). Capture the probe output and treat ONLY a definitive registry not-found signal (no such manifest / manifest unknown / name unknown) as safe to publish. Every other failure is "unknown" and now fails CLOSED: refuse unless force=true is passed as an explicit override. Mutation-tested with a stubbed docker: not-found -> publish proceeds; auth error and network error -> refuse (rc 1); existing manifest -> refuse (rc 1). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 Round-4 BLOCKER (INV-24) fixed in The fail-open, reproduced firstI ran the round-3 step body verbatim against a stubbed Confirmed: The fix (
|
| Scenario | Stub | Expected | Result |
|---|---|---|---|
not-found (no such manifest) |
rc 1 | publish proceeds | rc 0 — "safe to publish" |
auth error (unauthorized) |
rc 1 | refuse | rc 1 — fail closed |
network error (connection refused) |
rc 1 | refuse | rc 1 — fail closed |
| manifest exists | rc 0 | refuse | rc 1 — "already exist" |
All four pass.
Verification
- YAML parses (
yaml.safe_load). bash -non the modified step: OK.actionlint(rhysd/actionlint:latest): the file reports 4 pre-existingSC2129style warnings on>> $GITHUB_OUTPUTredirects (lines 489/666/774/970) — identical count on the pre-change baseline (verified viagit stash); my block adds zero new findings (it writes only to stdout).- Commit body contains no
[skip ci]/[ci skip]/[skip actions]and noBREAKING CHANGEline; CI triggered on the new head (ca36182), runs > 0.
Non-blocking Major items
The other findings are classified Minor / Non-blocking in your action-item list. They are architectural (staging-ancestor exemption, publish_only recency stranding), comment-accuracy (docker-bake reproducibility claim, loop-guard advice), or need their own reproduction + guard redesign (silent-green release: regex; the extract-breaking-changes.sh || true making the bad-range guard vacuous). I've kept this commit scoped to the sole blocker to keep the release-critical change reviewable; happy to take the non-blocking set in a follow-up so each gets its own reproduction and test.
…randing, reproducibility) Addresses bonnyr-f5 round-4 major findings on the release workflow: - Loop guard: exempt only the release bot's own commits, identified by the version subject AND the marker we append, not any human 'release:' subject. A human 'release: v3.2.0 notes' no longer skips green; it fails loud. - Manual dispatch: require the dispatched ref to be refs/heads/main itself. The old ancestor-of-main exemption admitted staging (0-ahead/1-behind main), which would ship an unreviewed tree as :latest. - publish_only: stop stranding a republish behind a newer tag. It now emits its immutable :VERSION tags regardless of recency; only the floating-tag move is gated, decided in the publish critical section via ROLLING_TAG. - docker-bake.hcl / release.yml: correct the false byte-reproducibility claim. Pinned CREATED and SOURCE_DATE_EPOCH reduce variance but do not guarantee an identical digest; a republish can move it, which is why the existence probe refuses by default and gates overwrite behind force. - CI-status: fail fast on a marker-carrying head instead of polling 2700s, since no CI run is ever created for it; point to the real recovery paths. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 Round-4 major findings addressed in 1. Silent-green loop-guard regex — FIXEDThe exempt (green-skip) branch matched
2. Staging admitted by ancestor exemption — FIXED
3.
|
Review: REVISERound 5, cold re-audit of The round-4 BLOCKER is genuinely closedThe probe no longer reads any error as "tag free". Auth / 429 / 5xx / network / missing- No BLOCKER this round: every failure mode I could construct goes red, not silently green. Merges standalone: yes (rc=0; all three scripts it invokes exist at base). Three caveats: it F1 · Major — the existence probe can't distinguish "package absent" from "no permission"
To be fair on severity: this is fail-closed, not silent-green, and it is not live upstream — all F2 · Major — the republish path's granularity doesn't cover the failures it exists forBake pushes all 7 targets as one group and signing is a separate step, but the guard refuses if any F3 · Major — the class is fixed at one call site only
Meanwhile Minors
F3 is the one I'd prioritise — it's the difference between "the immutable tag is protected" being |
|
Cross-PR merge-order constraints for this series are now tracked in #192. Relevant here: this PR conflicts with #182 on |
Round-6 review remediation for the conventional-commit breaking-change
detectors shared by compute_version_bump.sh and extract-breaking-changes.sh.
F1 (blocker) + F2 + F7: rewrite _is_breaking_body (byte-identical in both
scripts) around two anchors. A marker after a blank line is accepted with or
without a colon (keeps the real no-colon paragraph break). A marker in the
trailer block -- preceded by another trailer, or folded directly onto a
conventional-commit subject -- is accepted only with a colon. An is_subject
flag on line 1 arms that path for SCOPED subjects too (fix(core): x), which the
r5 trailer regex could not reach because "(" broke the run before the colon, so
a scoped folded footer silently shipped as a patch (F1). The colon requirement
in the trailer block rejects a prose header (Before:/Note:) followed by
colon-less prose (F2). A widened separator class and an optional bullet accept
double-space and "- " bulleted markers (F7).
_breaking_note uses the same start rule so trigger and note never disagree, and
now stops at the first real git-trailer line (capitalized Word(-Word): key)
instead of stripping only a trailing trailer run -- a trailer block followed by
prose no longer leaks a Co-Authored-By address, while a lowercase-prose colon
continuation (migration:) is kept (F5). Marker lines are excluded from the stop
so a hyphen-form marker is never mistaken for a trailer. A trailing CR is
stripped from the note and subject so CRLF messages do not reach CHANGELOG.md.
F3: document the merge-order dependency at the range guard -- its rc=1 is only
effective once #181 drops the call-site "|| true" in release.yml (not owned by
this PR). F4: reword the byte-identity comments to state the property as an
invariant these two files uphold, with the enforcing CI job landing in #182
rather than asserting a job this tree does not contain. F6: remove the
consistency guard as provably dead code (same range, order, predicate and
first-hit break as the bump loop, so its condition is unsatisfiable).
Adds red-green fixtures for F1/F2/F7/F5. Both self-tests pass under mawk 1.3.4
and gawk 5.3.2; detector copies remain byte-identical; shellcheck -S style
clean; SIGPIPE tail fixture shrunk to ~400 lines while still clearing the 64KB
pipe buffer.
Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
…test parity Round-5 review (bonnyr-f5 #182). Three isolable fixes; cross-PR items left to merge-order per the review. BLOCKER-1 -- commit-lint rejected the current tip of staging (a GitHub-composed squash commit whose machine-authored body carries a line-start bump declaration). On a push to staging/main the range is before..tip, so that already-merged tip was scanned, the ci-gate went red, and release.yml refused to release the SHA -- the pipeline stopped releasing. Adds a second machine-identity exemption (committer "GitHub <noreply@github.com>", single parent) mirroring the existing release-bot exemption, so the gate never judges already-merged, machine-composed history. Human commits never carry that committer identity and are still fully linted in their own PR. Reproduced (before..tip scan rc=1 -> rc=0) and mutation-tested: human marker in a PR commit still fails; the squash tip is exempt. Major-3 -- make script-selftests mirrored only 2 of ci.yml's 4 anti-vacuity assertions. Adds the missing two (no PASS line -> silenced/renamed guard; missing END marker -> early exit / deleted marker). Mutation-tested all three harness-break modes: each is now make-RED, matching CI. Minor -- .githooks/pre-push scanned the script default (upstream..HEAD) and so missed non-tip commits on a first push. Now derives the exact pushed range from git's pre-push stdin protocol (remote..local), falling back to the default for a brand-new branch or a manual run. Tested all stdin cases. Cross-PR (documented, not forced): sibling #181's own commits carry markers -- caught in #181's PR; once squash-merged the new exemption stops the gate re-scanning them (Major-1). The duplicate scripts/sync-version-artifacts.sh and its sed -i -E are #180's file under merge-order, not duplicated here (Major-2). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
…y recovery, operator path) F1 (Major): the immutable-tag existence probe classified by grepping the docker CLI's error text, which cannot separate "package does not exist yet" (first release in a fork/mirror namespace — safe) from "no permission" (must fail closed): both print `denied`. Replace it with scripts/registry-tag-probe.sh, which classifies each image by the registry's HTTP status (200 exists, 404 absent, 401/403 auth, else transient). Only a definitive 404 is "safe to publish"; auth/network/rate-limit/5xx fail closed. A broken/partial probe run now also fails closed instead of reading as "no manifests found". F2 (Major): add a sign_only recovery mode (workflow_dispatch input, requires publish_only). It re-runs cosign sign + SBOM + provenance against the already-pushed :VERSION digests without rebuilding or re-pushing, so the most likely recovery — signing failed after all images were pushed — no longer forces the operator into force=true, which rebuilds to possibly-different digests and moves the immutable tag. F3 (Major): the operator `make push-images` path had a recency guard but no existence probe, so VERSION == highest tag (a fresh main right after a release) silently overwrote the signed release images. It now runs the same registry-tag-probe.sh and refuses on exists/inconclusive unless FORCE_LATEST=1, fixing the class at every call site. F5 (Minor): the publish-critical-section recency guard failed OPEN when the tag fetch failed — it moved :latest on an unverified guess. Capture the fetch result; on failure a final/manual release goes red and a publish_only republish emits :VERSION without moving :latest. F6 (Minor): single-source the 7-image list in registry-tag-probe.sh; the CI probe and the publish summary now read it, and a self-test asserts parity with the docker-bake.hcl default group. F7 (Minor): docker-bake.hcl stamped an empty, spec-invalid image.created label when CREATED was unset (a plain make push-images). Emit the label only when CREATED is set, matching the ROLLING_TAG conditional; docs/DOCKER.md updated. F4 (cross-PR, documented): softened the release-notes comment that claimed extract-breaking-changes.sh stays fail-closed. On this ref that script still ends its range query with `|| true`; the fail-closed fix lands with #179, which owns the script (this PR does not touch it). F8/F9 (cross-PR/documented): the release.yml conflict with #182 and the absence of workflow-shell linting in CI are noted for the reviewer; #182's paths-ignore characterization in the existing comment is accurate (verified against pr182). Tests: scripts/tests/registry-tag-probe.test.sh mutation-tests the four required outcomes (absent->publish, no-permission->refuse, network->refuse, exists->refuse) plus first-publish and image-list parity. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
@bonnyr-f5 — round-5 addressed in F1 (Major) — probe couldn't tell "package absent" from "no permission"Confirmed your finding: the round-4 classifier grepped the docker CLI's error text, and a nonexistent repo's Fix: F2 (Major) — republish granularity didn't cover sign-after-push failureAdded a F3 (Major) — class fixed at one call site only
Minors
Cross-PR (documented, not forced)
VerificationYAML parses; all 34 |
Consolidated landing of seven interdependent PRs whose shared credential and release/CI surfaces prevented merging in any order (see issue #192's conflict matrix). Merged in dependency order 179, 180, 181, 188, 186, 183, 182; the #186/#188 credential surface was reconciled once (single reserved-name guard; provenance + migrations + stale-disable combined with rotation + backend MCP wiring + threadpool). Squashed to one commit; per-PR history retained on the seven archived branches. Validated on the merged tree: ruff + mypy clean; 172 auth/credential/startup/ migration tests pass; single alembic head v2_155; openapi + frontend types fresh; helm lint/template and docker compose config green on all modes; version and detector self-tests green; commit-message lint clean. Closes #192. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4
|
Superseded by the consolidated integration PR #193 (branch Closing unmerged, not abandoning: the branch is retained and all review history stays on this page for reference. See #193 for the integrated, validated result and #192 for the cross-PR conflict analysis. |
) * Integrate the #177 follow-up stack (#179 #180 #181 #182 #183 #186 #188) Consolidated landing of seven interdependent PRs whose shared credential and release/CI surfaces prevented merging in any order (see issue #192's conflict matrix). Merged in dependency order 179, 180, 181, 188, 186, 183, 182; the #186/#188 credential surface was reconciled once (single reserved-name guard; provenance + migrations + stale-disable combined with rotation + backend MCP wiring + threadpool). Squashed to one commit; per-PR history retained on the seven archived branches. Validated on the merged tree: ruff + mypy clean; 172 auth/credential/startup/ migration tests pass; single alembic head v2_155; openapi + frontend types fresh; helm lint/template and docker compose config green on all modes; version and detector self-tests green; commit-message lint clean. Closes #192. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193): reconcile the merged unset-MCP-password model and harden the credential seed Addresses mwiget's three CHANGES_REQUESTED blockers on the #177 integration PR (#193), plus follow-up findings from a max-effort review of the same credential surface. The merge chose #188's "unset MCP_SERVICE_PASSWORD -> disable" over #186's "unset -> generate", so the generate/rotate-on-unset code was left unreachable but still documented, and the release-notes footer was missing. BLOCKING 2 (core/config.py): the MCP_SERVICE_PASSWORD comment claimed ensure_service_user "generates a random secret and surfaces it once" when unset, contradicting the merged behaviour. Rewrote it to state the truth: when unset (or a known default) seed_auth_step calls disable_stale_service_user, leaving the mcp account disabled/unavailable until an operator configures a real password; a published default is refused and rotated out; the backend receives MCP_SERVICE_PASSWORD on every deploy mode and reconciles to it when set. Also removed the duplicate #186 block that sat above the wrong field. BLOCKING 3 (services/auth_service.py): removed the unreachable usable_password is None branches from ensure_service_user (the generate-on-create and rotate-on-unset paths). seed_auth_step only calls it under the _mcp_pw_usable gate, so password is never None/default in production. ensure_service_user now requires a usable password and only creates/reconciles with it (failing closed and loudly if handed an unusable one); the unset case is owned entirely by disable_stale_service_user. Dropped the now-dead _log_generated_service_password helper and the service-account token_urlsafe/_persist_generated_password calls (_persist_generated_password is still used by the admin seed). Kept the reserved-name guard, the provenance check, the adopt-a-published-default remediation, and disable_stale_service_user fully intact. Updated the affected unit tests (published-default/None now refused; added a reachable adopt-and-reconcile test; stale-row setup builds the legacy row directly) and fixed scripts/mcp_live_smoke.py, which pointed operators at /app/keys/initial_mcp_password, a file no reachable path writes. CR-1 (services/auth_service.py, seed_admin_user): fixed a concurrent-first-boot admin lockout. With DEFAULT_ADMIN_PASSWORD unset and 2+ api replicas, both generated different passwords and the loser overwrote the keys file while its INSERT rolled back, so the file and the committed row disagreed. The fresh seed now creates+flushes first (the loser's INSERT raises IntegrityError -> rollback, no file write) and persists the keys file only after winning but before commit, so the file can only ever hold the committed row's password. Added a losing-replica test. CR-5 (services/auth_service.py, _persist_generated_password): os.open's 0600 mode only applies on create, so a pre-existing 0644 file was truncated in place and kept 0644, writing the secret world-readable. Added os.fchmod(fd, 0o600) and a test that a pre-existing 0644 file is tightened to 0600. CR-2 (routes/k8s_websocket.py, dpus_websocket.py, benchmarks.py): the WS auth validators called the blocking sync token_user_state directly on the event loop. Moved it off the loop via run_in_threadpool, matching core/auth_middleware.py. Validation: make lint-backend clean; mypy core/ schemas/ unchanged; auth (57) + ws/benchmark/startup (79) suites pass; alembic heads single v2_155; helm lint/template OK and --set secrets.mcpPassword=changeme fails the render; docker compose config OK on all modes; extract-breaking-changes and compute_version_bump self-tests pass; lint-commit-markers clean. BREAKING CHANGE: MCP_SERVICE_PASSWORD is now required — the backend SystemExits under ENVIRONMENT=staging|production when it is unset or a known default; the shipped admin/changeme default is removed and rotated out on upgrade; the dist bundle renames MCP_USERNAME/MCP_PASSWORD to MCP_SERVICE_USERNAME/MCP_SERVICE_PASSWORD; and the Helm chart ships mcpUsername=mcp with a generated mcpPassword instead of admin/changeme. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193): close bonnyr-f5's credential/backend/helm/dist/docs blockers Follow-up to the #177 integration on pr177-integration, addressing the CREDENTIAL/BACKEND/HELM/DIST/DOCS half of bonnyr-f5's BLOCK on PR #193. B1 (SECURITY): ensure_service_user no longer adopts any human account whose password is a known default. The adoption exception is now scoped to v2_155's exact backfill fingerprint (username=='mcp' AND email=='mcp@bnk-forge.local'), matching the migration's own conservative rule, and must_change_password is no longer cleared on an adopted row. Adds tests proving a human operator/changeme row (and a wrong-email mcp row) is REFUSED, not taken over. B2/B2b: all five compose files (root + dist docker-compose{,.local}.yml and the IBM embedded compose) honor legacy MCP_USERNAME/MCP_PASSWORD as aliases for MCP_SERVICE_USERNAME/MCP_SERVICE_PASSWORD on both backend and mcp env, so an existing customer .env keeps working after upgrade. Docs (dist/README.md, dist/.env.example, user-pack/install-guide.html) document MCP_SERVICE_PASSWORD as canonical with MCP_PASSWORD honored as a legacy alias. B3: ENVIRONMENT is plumbed to the backend in every compose file, so an operator who sets ENVIRONMENT=staging|production actually reaches config.py's MCP fail-fast. Helm already routes ENVIRONMENT=production onto api/worker/beat. M1: the unset-MCP behavior stays "disabled" (#188 over #186); added an explicit deliberate-consolidation comment at the decision point. M2: disable_stale_service_user skips the about-to-be-reconciled row and the "no usable MCP_SERVICE_PASSWORD" warning is conditional on a configured password, so a correctly-configured install no longer logs a false warning or commits an inactive MCP window on every boot. M7: the Helm mcp deployment and the IBM installer's mcp service now run the exec auth-probe healthcheck (python -m bnk_forge_mcp.healthcheck) instead of a bare tcpSocket probe. M8: the chart fails the render for a reserved mcpUsername (admin), mirroring the mcpPassword guard, and NOTES.txt/values.yaml call it out. Minors: deterministic checksum/secret via a shared helper (stable across renders, identical across api/worker/beat/mcp); vestigial _persist_generated_password filename docstring; false "backend generates its own secret" rationale corrected in compose/helpers/ibm; mcp_live_smoke.py #186/#188 attribution; e2e/config.py changeme default note; .env.example :58/:73/:94 fixes; unified BNK_FORGE_VERSION to latest across dist. Validation: ruff clean; typecheck-backend (core/ schemas/) Success 38 files; 199 auth/credential/startup/ws/migration tests pass incl. new B1 tests; helm lint + template stable checksums, --set secrets.mcpUsername=admin and secrets.mcpPassword=changeme both FAIL; docker compose config on all five modes shows the backend receiving MCP_SERVICE_PASSWORD (via either alias) and ENVIRONMENT. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193): close bonnyr-f5 CI/RELEASE/SCRIPTS blockers, majors, and minors Blockers: - B4 registry-tag-probe.sh: replace GNU-only BRE alternation (\|) with portable `sed -nE (access_token|token)` so the token parse works on BSD/ macOS sed; on BSD the empty token classified every image `unknown` and routed the operator into FORCE_LATEST=1, overwriting immutable :VERSION manifests (INV-24). Reproduced under `sed --posix`, fix proven to parse T2. - B5 registry-tag-probe.test.sh had no caller and was red on BSD. Both the Makefile script-selftests target and ci.yml's script-selftests job now enumerate and run every scripts/tests/*.test.sh, failing on an empty enumeration or any non-zero rc. - B6 lint-commit-markers.sh: replace the spoofable committer-identity exemption (GitHub <noreply@github.com> + single parent) with an unspoofable "already reachable from origin/main|origin/staging" check; lint the PR title (PR_TITLE via env) on pull_request events; split the rules so machine/already-merged is exempt for the marker rule but the spurious-major rule always applies. Majors: - M3 release.yml overwrite guard: derive the vacuity floor from an independent source (docker-bake.hcl default group, sourced from the workflow-ref tooling) and assert the probe's exit status before trusting its output, so an unavailable probe fails closed instead of "safe". - M4 (INV-31): generate release notes and run the registry existence-probe BEFORE the irreversible push in release-final/release-manual (new shared scripts/registry-overwrite-guard.sh); release-publish keeps its own in-critical-section re-check. - M5 make script-selftests now runs the INV-15 detector-parity diff (extracted to scripts/tests/detector-parity.test.sh) so local == CI. - M6 extractor self-test runs unconditionally with anti-vacuity assertions (ok lines + END marker), no longer gated on grepping its own --self-test. Minors: stale cross-PR comments in release.yml and extract-breaking-changes.sh; removed the duplicate Makefile version-check target; `git add dist/VERSION` no longer swallows failures; first-ever-release notes range fixed; CHANGELOG insertion asserts a non-no-op before committing; refreshed .trivyignore CVE-2026-7598 review deadline; removed e2e-tests.yml dead `|| true`; documented the new Docker dependency in the pre-push hook. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r2): close credential/backend/helm/dist blockers — drop the username alias, make ENVIRONMENT=production satisfiable, and fix the mcp liveness probe bonnyr-f5 round-2 BLOCK, credential/backend/helm/dist/docs half. B-1 (INV-12): the compose files aliased the SERVICE username (MCP_SERVICE_USERNAME:-${MCP_USERNAME:-mcp}); a legacy .env with MCP_USERNAME=admin resolved it to `admin`, and against the guardless image `latest` still points at, the old ensure_service_user rewrites the human admin row to `changeme` every boot. Drop the username alias across all five compose files + the ibm embedded compose (keep the harmless password alias), and default BNK_FORGE_VERSION to 4.0.0 (the release this tree becomes, first image with the guards) instead of `latest`, so a compose file can never hand the new credential contract to a pre-guard image. B-2: ENVIRONMENT=production reaches validate_production, which also gates on JWT_SECRET_KEY / ENCRYPTION_KEY / ALLOWED_ORIGINS — none of which were deliverable from a compose install, so the switch bricked the backend. Plumb all three into every x-backend-env anchor (four compose files + ibm) and document them in the env examples; treat an empty ("" from ${VAR:-}) key as unset in config.py so the plumbed empty default auto-generates rather than passing as a real empty key. _persist_or_load_key now flags only keys WE generated as auto_generated (sidecar .autogen marker), so an operator-provisioned key on the volume validates while a fresh prod boot still fail-fasts permanently. M-4: mcp liveness returned non-zero when the BACKEND was unreachable, so k8s restarted the pod for a dependency outage. Move the auth-probe to readiness only; liveness is tcpSocket. Fix mcp-server/README + mcp_live_smoke hints to name only the vars each process actually reads (container: BNK_FORGE_*, backend: MCP_SERVICE_*). Minors: refuse a known-default DEFAULT_ADMIN_PASSWORD on fresh seed + helm adminPassword fail-guard; guard secrets.yaml mcpUsername nil with kindIs "invalid"; make the Python reserved-name check case-insensitive/trim to match Helm; neutralise the hash when disabling a stale service account; correct the benchmarks.py JWT-gate comment; surface an empty MCP_SERVICE_PASSWORD in install.sh; fix the .env.example "No .env file is needed!" contradiction. Tests: config B-2 satisfiability + provenance-marker tests; seed_auth_step against an admin-still-holds-changeme upgrade DB; case-insensitive reserved-name and disable-hash-neutralisation cases. ruff clean, mypy clean, 4831 unit pass, helm lint/template green, docker compose config verified on all modes. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193): close r2 CI/RELEASE blockers B-3, M-1..M-3 + scripts/release minors B-3 (commit-lint exemptions): key the already-merged exemption on the range BASE (github.event.before), not the post-push tip, so a new skip-CI marker on a push to main/staging is caught while a genuinely-already-merged base commit stays exempt; replace the self-settable `^release: ` subject exemption with release.yml's own version+trailing-skip fingerprint. M-1 (spurious-major rule): redefine rule 2 as the exact complement of the detectors, sourced from the shared predicate, so it flags only a marker the detectors would MISS (never dash-bullet, markdown-bold or indented shapes); give it the same already-merged exemption; and lint inputs.release_notes through the script before it becomes a release commit/tag. M-2 (overwrite guard floor): derive the vacuity floor from `docker buildx bake --print default | jq '.group.default.targets | length'`, scoped to the default group, so a second bake group no longer wedges the release; separate bake-file parse failures from registry-unreachable in the messaging. Single-source the policy: release-publish and make push-images now call the one guard. M-3 (portability): drop bash-4 mapfile from the probe test; rebuild the compute self-test newline expansion with awk to avoid the bash-3.2 parameter-expansion cliff, so make script-selftests runs under stock macOS bash 3.2. Detector single-sourced into scripts/lib/breaking-change-detect.sh (compute, extract, lint all source it); detector-parity test asserts the wiring; added mutation tests for the lint rules and the overwrite guard. Minors: fix version-consistency misdiagnosis of a column-0 YAML comment; correct the compute/extract parity docstrings and the docker-bake four-push-paths note; wire artifact-network-self-test into ci-gates; make the pre-push hook migration message reachable under set -e; omit the false provenance buildStartedOn; filter the release CI-status poll by commit SHA. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193): seed the re-enable-guard test's default-hash row directly The round-2 disable_stale_service_user hash-scrub (bonnyr-f5 #193 minor) collided with the re-enable guard's own regression test: _seed_disabled_default_mcp built its "disabled while holding the published default" state BY CALLING disable_stale, which now scrubs the hash -- so holds_known_default_password was false and the PUT re-enable was allowed (200) instead of refused (400). The guard defends a row taken inactive by a path that LEAVES the credential intact (a manual operator PUT), not one disable_stale scrubbed. Seed that state directly (set is_active=False on the default-hash row) so the guard's real scenario is exercised; assert the default hash survives the seed. Corrected the now-stale guard comment in routes/auth.py that still claimed disable "only flips is_active". Neutralisation and its asserting tests are unchanged. Verified: TestServiceAccountReEnableGuard 2/2 pass; the three affected auth files (test_startup_seed_auth, component/test_auth_service, integration/test_routes_auth) 97/97 pass; ruff clean. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r3): fail-close key provenance (B-2) + credential/auth minors Own the round-3 CREDENTIAL/AUTH findings. B-2 (BLOCKER): _persist_or_load_key classified a marker-less key file as operator-provided, so every upgrade keys volume (key present, no marker) let SEC-006's fail-fast pass OPEN on an auto-generated JWT/ENCRYPTION secret in production. Invert the sentinel: a marker-less key now classifies AUTO-GENERATED (fail closed); an operator asserts provenance with an explicit <filename>.operator opt-out marker. No marker is written on generation, which also removes the second trigger (a partial marker write can no longer downgrade provenance). Regression tests seed the PREVIOUS-RELEASE on-disk shape and assert validate_production raises under ENVIRONMENT=production. Minors: - Write the key via os.open(0o600)+fchmod so the secret is never briefly 0644. - Single-source the MCP known-default denylist: delete the local tuple in auth_service and use core.config.MCP_KNOWN_DEFAULT_PASSWORDS (comment notes the helm copy is deploy-owned). - Correct holds_known_default_password docstring (disable_stale now scrubs the hash; this guard covers the other disable paths). - Reconcile ENCRYPTION_KEY docs with reality: the keys-file is the source of truth for at-rest crypto; the env var only drives the production gate (encryption.py comment + .env.example). - Clarify the v2_155 custom-username remedy in disable_stale docstring. Test-gaps: - Normalise the service username (trim/casefold) at the reconcile lookup and the disable skip filter, so " mcp "/"MCP" reconciles the existing mcp row instead of minting a second service account and disabling the live one. - Honest seed_admin_user log/logic under DEFAULT_ADMIN_MUST_CHANGE=false (no more "must change on first login" when no gate was applied). - disable_stale_service_user(skip_username=...) leaves the live row wholly untouched (no inactive window), variant included. - db.commit() failure after the keys file is written leaves a retriable state (published default still authenticates, orphan file password does not). All owned-suite tests pass; ruff clean. Each fix reproduced then mutation-tested. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r3): close the release/CI blocker + major + every release/CI minor M-6 (blocker): commit-lint no longer reds unamendable merge history. - rule 2 now flags ONLY a mis-anchored DECLARATIVE marker (the colon form); a colonless marker-shaped PROSE line the detectors treat as inert (an already-merged body such as "- <MARKER> footer in the body ...") is no longer flagged, so the push-to-main range (before..head, which INCLUDES the PR merge-base) goes green without a history rewrite. Detection of a real mis-anchored marker is unchanged. - deleted the already-merged exemption as dead code: base..head excludes the base by construction, so no scanned commit can ever be an ancestor of it. Removed the BEFORE derivation, _already_merged, tests B3.2/M1.5, and the ~20-line header claim. The release-bot exemption stays. - rule 2 now scans the whole body via _under_detected_markers and reports EVERY mis-anchored marker, not just the first. M-7 (major): secret-scan no longer false-fails a delete-only range. A delete-only commit has rev-list count > 0 but gitleaks scans 0 (it scans added content), so the count-based backstop is replaced by a range- resolvability check plus gitleaks' exit status. Release/CI minors: - release-bot fingerprint single-sourced to scripts/lib/is-release-bot-subject.sh; release.yml's inline copy byte-locked by a parity self-test; dropped the false unforgeability claim and documented the residual honestly. - registry-overwrite-guard: added a fail-closed default arm for an unrecognised/empty probe status (+ malformed/empty test scenarios). - Makefile push-images: FORCE_LATEST now overrides ONLY the recency guard; a new FORCE_OVERWRITE overrides ONLY the immutable-tag guard; fixed the missing-jq remediation text. - registry-tag-probe: the network arm now matches the real doubled "000000" curl-failure shape (was dead code); test fixture reproduces it. - INV-15: single-sourced the marker regex (one canonical value + a detector-parity assertion that every embedded copy is byte-identical). - release.yml Publish summary counts what buildx actually pushed (bake --metadata-file), not the static target list. - registry-tag-probe test enumerates the bake DEFAULT group (scoped), matching the guard's enumeration. - added scripts/tests/secret-scan.test.sh (fake-docker mutation suite). release.yml: added a post-push step running scripts/verify-image-pins.sh so a release cannot complete while shipping an unpublished image pin (script owned by the deploy agent; referenced by path from .release-tooling). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r3): single-source every deploy version pin + close deploy majors/minors B-1/B-1b: dist/docker-compose{,.local}.yml, dist/.env.example and ibm_cloud_bnk_forge.sh hard-pinned 4.0.0, a tag that has never been published, while Helm pinned 3.1.6 -- two shipped paths naming two versions, one of which does not exist. Bring all of them under sync-version-artifacts.sh (new PIN/ DISTENV readers+writers, --check, --list) so every pin derives from VERSION (3.1.6, which exists) and the release re-stamps them atomically via the existing --write $NEW; drop the dist/ disclaimer. Add scripts/verify-image-pins.sh (+ selftest) that resolves every shipped compose image: pin against the registry and fails on manifest unknown, wired post-push in the release job. M-1..M-5: reword NOTES/compose/chart comments that asserted post-guard behaviour as already-true on the pre-guard pinned image (they land with the guard-carrying release); NOTES now leads with the required ALLOWED_ORIGINS override; dist/README and the install guide stop recommending latest/3.1.6 and the keys-file cat the pinned image does not write; install.sh strips quotes and rejects the known- default MCP passwords so the "MCP not active" warning fires instead of a green lie; add deploy-version-lockstep + helm-known-defaults-lockstep selftests. Deploy minors: MCP_USERNAME "do not set" made consistent across docs/Makefile; chart ENCRYPTION_KEY now emits a valid Fernet key; DEFAULT_ADMIN_* added to the ibm P3 backend env; secrets.mcpUsername defaults to "mcp" on null; ibm mapfile -> portable while-read. Verified: sync --check exit 0; --write round-trip moves every pin and restores; helm lint/template clean (default + origin override); script selftests green; bash -n + shellcheck clean. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r4): unify the at-rest encryption key (B-3), fail-close key marker (M-1), row-matches-client (M-2) + credential minors B-3: validate_production gated ENCRYPTION_KEY while a second, independent generator in core/encryption.py produced the real at-rest Fernet key unchecked — setting ENCRYPTION_KEY (the documented remedy: token_hex(16), not even a Fernet key) turned the gate green while encryption auto-generated a different key. Unify: one key file (_encryption_key_path == core.encryption.ENCRYPTION_KEY_FILE), one generator. When ENCRYPTION_KEY is set it is VALIDATED as a real Fernet key (fail clearly if not), written to that file with a .operator marker, and consumed by core.encryption and services.backup_service; the provenance flag reflects the value that actually protects data. Never clobber an operator-marked key on a mismatch. config.py:319 and .env.example now print the Fernet recipe. M-1: _persist_or_load_key required a regular-FILE marker (os.path.isfile, not os.path.exists — a directory no longer counts) and treats "marker present, key file absent" as a provisioning error: generate but do NOT persist, so the stale-marker rotation gesture can never heal into auto=False on the next boot. M-2 (regression this PR introduced): ensure_service_user normalised the username before lookup/create, so MCP_SERVICE_USERNAME=MCP seeded 'mcp' while the client sends the raw 'MCP' and authenticate_user matched exactly -> login denied. Create/ reconcile under the RAW value (what the client sends); the disable_stale skip keys on the same raw value; only the reserved-name guard normalises. Fixed the false "Matches the Helm chart lower|trim" docstring. Credential minors: non-vacuous localhost-CORS test (valid MCP password so only the CORS branch fails) + wildcard is now an exact origin-list entry, not a substring; new DPU-websocket must-change/unresolvable-user tests mirroring the k8s twin; middleware unresolvable-JWT-subject-refused and exact-vs-suffix exempt-path tests; corrected the denylist copy count (4th copy in dist/install.sh); corrected v2_155's rationale (v2_154 is new in this diff, not "already shipped"); documented why ensure_service_user's adoption branch is kept. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r4): close deploy-surface blocker + majors (B1, M7-M12) and deploy minors B-1: a fresh install of the pinned 3.1.6 bundle seeded an admin nobody could log in as. `${DEFAULT_ADMIN_PASSWORD:-}` delivers the var present-and-empty; 3.1.6's `DEFAULT_ADMIN_PASSWORD: str = "changeme"` is then overridden by "" and the login schema rejects "" (422). The prescribed `${VAR}` form does NOT omit on this compose (map interpolation still renders ""); the working omit-when-unset form is a map entry with NO value (passthrough / `docker run -e KEY` semantics). Converted DEFAULT_ADMIN_PASSWORD to passthrough across all four compose paths (dist base+local, root base+local, IBM embedded). Swept the class: JWT_SECRET_KEY/ENCRYPTION_KEY also converted (3.1.6 uses `if KEY is None`, so "" is used literally, not auto-generated); MCP_SERVICE_PASSWORD kept as `${...:-...}` because omitting it restores 3.1.6's known default "mcp-service-changeme". Added a commented DEFAULT_ADMIN_PASSWORD block to dist/.env.example. Verified via `docker compose config` + real container env both directions (unset -> omitted; set in .env -> forwarded). M-7: chart checksum/secret did not change on mcp-password rotation (3 renders, 3 generated passwords, one identical checksum). Made all generate/rotate fallbacks deterministic (deriveSecret, release-seeded) so the Secret is stable across renders and includes, and hash the RENDERED Secret so the annotation tracks every resolved value. Now stable across renders, identical across the 4 deployments, and it flips when any resolved value changes. M-8: dist/install.sh credential guard failed open on `changeme ` (whitespace). Trim leading/trailing whitespace around the quote-strip before the known-default compare. M-9: removed newly-added forward-dated 4.0.0 prose (install-guide.html x2, DOCKER.md). M-10: default helm install crashlooped (production + localhost). Added a render-time guard mirroring backend validate_production (fail on wildcard under staging/production, localhost under production); defaulted ALLOWED_ORIGINS to empty so the bare render boots (empty is neither wildcard nor localhost). `helm lint` and bare `helm template` stay green; the guard fires with a clear message on a real fatal posture. M-11: portable in-place sed in the IBM installer (`sed -i.bak … && rm`), both sites. M-12: dist/ no longer ships published default DB/redis creds on host networking. install.sh generates strong POSTGRES_PASSWORD/REDIS_PASSWORD on the fresh .env (like Helm/IBM); .env.example ships them empty; a pre-existing default triggers a warning. Deploy minors: extended deploy-version-lockstep.test.sh to the bnk-operator chart (appVersion + image.tag) and dist/VERSION; brought dist/VERSION under sync-version-artifacts.sh (--check/--list/--write green); reworded install.sh's MCP UNHEALTHY assertion to match what the pinned image actually reports; added scripts/tests/ibm-compose-drift.test.sh freezing the credential/hardening env so the IBM embedded compose and dist/docker-compose.yml cannot silently diverge. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r4): make the pin verifier reachable pre-push, de-vacuum the parity/self-test harnesses, and close the release minors B-2 (blocker): scripts/verify-image-pins.sh could never pass where the release job runs it — from the 4-file sparse .release-tooling checkout that holds no compose file (ROOT resolved there), with REGISTRY/VERSION handed in as env vars the script reads only as flags, and the whole step wired AFTER the tag/Release/push/signing. Fixes, end to end: - add a consistency mode (--expect-version) that asserts every shipped first-party pin already renders to $NEW without a registry probe, and run it as the PRIMARY PRE-push gate in release-final and release-manual (before anything irreversible); - fix the post-push existence step to pass REGISTRY/VERSION as FLAGS and the compose files explicitly by --file (they live at the tag checkout at the workspace root), keeping it as a secondary confirmation; - widen the default file set to include the IBM Cloud installer's embedded compose; - add a dryrun-release-tooling job that rebuilds the exact publish-job layout and exercises both invocations against a fake probe, and gate release-publish on it, so a step that cannot execute is caught before it is wired ahead of a signature. The sibling registry-tag-probe.sh / registry-overwrite-guard.sh read nothing $ROOT-relative outside the sparse set, so they are unaffected. M-3: detector-parity.test.sh enumerated the marker copies with the very token that drifts, so a copy that drifted in the token vanished from enumeration (drifting :96-97 dropped the count 5->3 yet stayed green). Enumerate by position/count instead: an exact per-file canonical count plus a stable-anchor site scan that flags any drifted site even under a compensating add. M-4: the filesystem self-test loop checked only a non-empty enumeration and each file's exit 0, so a test gutted to a no-op passed and deleting 7 of 8 stayed green. It now requires each file to emit PASS lines, no FAIL line, and an ALL PASS terminal marker, plus a count floor derived from git's tracked *.test.sh set (detector-parity was conformed to that output convention). M-5: release-rc created and pushed the RC tag before the fail-closed notes step; the tag is now created locally, notes generated, then the tag pushed. M-6: added mutation-tested coverage for this PR's four previously-uncovered lint fixes (the PR-title lint, the pending-message lint, the RANGE fail-closed branch, and the skip-checks trailer rule). LEAD: the anti-vacuity staging floor derived the count from a stale literal while --list grew to 8 paths; both sites now derive it from --list and require every listed path to stage, and the stale comments are corrected. Release minors: scope the release-bot commit-lint exemption to the range tip (a forged release subject buried mid-range is no longer exempt) and add a REACHABLE published-history exemption anchored to the last release tag so a mis-anchored marker in unamendable history cannot red the release; add fixtures for the untested registry probe/guard arms (5xx, unexpected code, unknown status, bake-parse failure); ancestry-filter the LAST_FINAL tag queries so a tag on another branch cannot skew the notes range; reconcile the empty-RANGE handling (commit-lint now fails closed on an explicit empty RANGE, ci.yml leaves it unset); give the pre-push hook a clear message and a fetch fallback when the remote tip is absent locally; derive the cosign verify-identity org from REGISTRY instead of hardcoding it. Flagged: gate Makefile push-customer-build through registry-overwrite-guard.sh, the last documented push path that was still unguarded. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r4): keep M-7 checksum-tracks-rotation WITHOUT predictable secrets The r4 deploy fix closed M-7 (checksum did not move on a secret rotation, so pods never rolled) by making the generated fallbacks deterministic -- deriveSecret = sha256(Release.Name|Namespace|fullname|purpose). Every input is public (they appear in resource labels and the chart source), so that made the JWT signing key, the at-rest Fernet key and the admin/mcp passwords COMPUTABLE by anyone who can read a label -- a forge-any-token / decrypt-all-secrets exposure, strictly worse than the cosmetic churn it fixed. Revert generation to randAlphaNum (unpredictable) and instead hash the DETERMINISTIC inputs that determine the Secret -- values.secrets, the persisted .data (reused via lookup), plus a per-credential "rotating-from-default" marker for admin/mcp whose persisted value is a known published default. That tracks every rotation (operator edit, persisted-value change, rotate-away-from-default) so the pods roll, is stable across renders including a bare no-cluster `helm template` (the hashed inputs carry no randomness), and never derives a secret from public identity. deriveSecret removed. New scripts/tests/helm-secret-checksum.test.sh locks all three: stable-across-renders, changes-on-rotation, and generated-value-is-random -- so the determinism cannot return. Verified: helm lint 0-failed; bare + override template OK; the M-10 render guard still fires on production+localhost; the new selftest ALL PASS. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r4): self-review — close the encryption-key data-loss BLOCKER + deploy minors A cold adversarial self-review (three auditors mirroring the reviewer's method) of the r4 changeset found a data-loss BLOCKER we introduced this round, plus deploy minors. Fixing before it ships. B-3 (BLOCKER, data loss): the r4 encryption-key unification made ENCRYPTION_KEY env OVERWRITE an existing at-rest key file on boot. Traced only to the "operator freshly sets the key" consumer, never to (a) backup_service restore, which writes the backup's key to that file, or (b) the r3->r4 upgrade population, whose data was encrypted under a marker-less auto-gen FILE key (r3 core.encryption used the file regardless of env). The first r4 boot clobbered those -> restored/existing data undecryptable (or bricked the boot on a marker mismatch). A new test even LOCKED the clobber with a false premise. Fix: the at-rest key FILE is the single source of truth. ENCRYPTION_KEY only SEEDS the file when it is ABSENT and never overwrites an existing one; the gate reads the FILE's .operator provenance. backup restore now drops the .operator marker so a restored key passes the gate without a clobber. Rewrote the clobber-locking tests to lock the no-clobber invariant; added upgrade-shape, production-fail-without-data-loss, and restore-marker tests. M-7 (deploy self-review): removed the rotation-marker from secretsChecksum — the auditor proved it redundant (the same .data change already moves the digest; deleting it left the test green) and its admin branch dead. Kept the input-hash; documented the genuine trilemma (cluster-less-template-stable / tracks-generated-rotation / unpredictable-secrets — pick two; determinism is the predictable-secret hole). M-10: the render guard's wildcard check is now an exact comma-split entry, matching the backend's `"*" in cors_origins` since r4, so a legitimate `https://*.example.com` is no longer blocked; the localhost check stays a substring to match the backend. .env.example: the admin-password template was an empty assignment that uncomments into a lockout; it now carries a replace-me placeholder. Verified: backend 8082 passed; config/encryption 66, backup 13; script-selftests all pass; helm lint/template clean (subdomain-wildcard passes, bare '*'/prod+localhost fail); ruff clean. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 * fix(#193 r5): fail closed instead of regenerating over an existing at-rest key (I-1) bonnyr-f5 #193 round-5 closure blocker. B-3 wrote down the invariant "the at-rest key file is the single source of truth; nothing overwrites it once it holds bytes" and config.py honoured it -- but core.encryption.get_encryption_key() did not. A file present but under 32 bytes (truncated / partial write / disk full / bad restore) logged "Invalid key, regenerating" and OVERWROTE it with a fresh key, permanently destroying the key any existing data was encrypted under -- silently, on a GREEN production boot, because the intact .operator marker keeps validate_production passing. Fix: the read path now fails CLOSED. A genuinely absent (or 0-byte) file still generates and persists a new key. A file that HOLDS bytes is validated as a real Fernet key: valid -> returned untouched; unusable -> SystemExit with a clear message, never regenerated. A crashloop is recoverable; an overwritten key is not. This also closes r5 note #3 -- the old `len >= 32` check accepted any blob and surfaced a mis-shaped key as a later cipher error; it now Fernet-validates and says so plainly. Locked by TestAtRestKeyFileNeverRegeneratedOverBytes: a 30-byte truncated key -> SystemExit AND the original bytes survive on disk (recoverable); a valid key -> returned untouched; an absent file -> generates a valid key. Verified: backend 8086 passed; encryption unit tests 19 passed; ruff clean. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --------- Co-authored-by: John Gruber <john.t.gruber@gmail.com>
4 participants
Addresses the workflow side of Blocker 3 from @bonnyr-f5's #177 review. (The
main-branchlock_branchitself is a repo-config change handled separately.)Republish recovery (the main ask)
If publishing failed after
release-finalsucceeded (bake error, ghcr 5xx, Sigstore outage, the 90-min timeout), the state was tag + GitHub Release + no images, with no way to recover — re-running onmainhits therelease:loop guard, andworkflow_dispatchbumps to the next version rather than republishing the existing one.Added a
publish_only: <tag>dispatch input: skips derivation/bump/CI-poll/tag-creation, asserts the tag exists, and runsrelease-publishagainst it. Purely additive — the rc/final/manual paths are unchanged (only a newkind=publish_onlybranch and an|| kind == 'publish_only'on the publish job).Robustness nits (same review)
wc -lbreaks the moment any rc tag is deleted — the count drops and the next staging push recomputes an existing tag, which fails to create. Now: highest existing rc number + 1.::warning::when it suppresses a release, naming the commit. Before, a normal commit beginningrelease:or ending[skip ci]silently skipped publication while the job reported success. (Its grep is now a pipe-free here-string too.)Verified
release.ymlYAML validates. The change is additive; existing release kinds keep their exact conditions.https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4