From 18bd737c9bed3e2eeda21dfd669575358f403436 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 22:11:01 -0500 Subject: [PATCH 1/9] fix: keep Helm chart tag, appVersion, and package.json in lockstep with VERSION MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #177 review (bonnyr-f5) — BLOCKER 2. helm/bnk-forge/values.yaml pinned image.tag: "3.0.1" and Chart.yaml appVersion: "3.0.1", and every per-service tag is "" (falls back to the global 3.0.1). The release publishes only :${VERSION} and :latest, so :3.0.1 -- which was never published on this registry -- means ImagePullBackOff across all seven services. frontend-v2/package.json had likewise drifted to 2.12.0. The release job bumped only VERSION/dist/VERSION/ CHANGELOG, so every other version-bearing artifact drifted silently. - New scripts/sync-version-artifacts.sh with --write (sets the global Helm image tag, Chart appVersion, and frontend package.json) and --check (asserts all three equal VERSION, exits 1 otherwise). Anchored seds hit only the global 2-space image tag -- postgres/redis and the "" per-service tags are untouched. - The release job (both the automated and manual paths) now runs --write after bumping VERSION and stages the three files, so a 4.0.0 release updates the chart to 4.0.0 instead of leaving it on 3.0.1. - New CI job "P1 · Version Consistency" runs --check and is wired into the CI gate, so this drift can't reappear silently -- mirroring the existing image-level VERSION assertion, but at source level on every PR. - Fixed the current drift: all three now read 3.1.6 (= VERSION), and 3.1.6 images do exist. Note: this makes frontend-v2/package.json track the product VERSION, as the review requested. If the frontend is meant to version independently, that's the one line to drop from the assertion. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 15 +++++++++ .github/workflows/release.yml | 10 +++++- frontend-v2/package.json | 2 +- helm/bnk-forge/Chart.yaml | 2 +- helm/bnk-forge/values.yaml | 2 +- scripts/sync-version-artifacts.sh | 53 +++++++++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 scripts/sync-version-artifacts.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d8f62e3..1aa8b4a9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,6 +126,19 @@ jobs: - name: Run lint run: make lint-backend + version-consistency: + name: "P1 · Version Consistency" + needs: changes + if: always() + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Assert version-bearing artifacts agree with VERSION + # Helm chart tag/appVersion and frontend package.json must equal VERSION, + # or the release (which publishes only :${VERSION}) yields ImagePullBackOff + # / silent drift (#177 Blocker 2). + run: bash scripts/sync-version-artifacts.sh --check + lint-frontend: name: "P1 · Lint Frontend" needs: changes @@ -1063,6 +1076,7 @@ jobs: - changes # Phase 1 - lint-backend + - version-consistency - lint-frontend - typecheck-backend - openapi-check @@ -1096,6 +1110,7 @@ jobs: failed=false for job in \ "lint-backend:${{ needs.lint-backend.result }}" \ + "version-consistency:${{ needs.version-consistency.result }}" \ "lint-frontend:${{ needs.lint-frontend.result }}" \ "typecheck-backend:${{ needs.typecheck-backend.result }}" \ "openapi-check:${{ needs.openapi-check.result }}" \ diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0ded64e0..9b0facff 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -355,6 +355,10 @@ jobs: if git ls-files --error-unmatch dist/VERSION 2>/dev/null; then echo "$NEW" > dist/VERSION fi + # Keep the Helm chart tag/appVersion and frontend package.json in + # lockstep so the chart never pins an image tag the release doesn't + # publish (#177 Blocker 2). + bash scripts/sync-version-artifacts.sh --write "$NEW" - name: Update CHANGELOG.md run: | @@ -413,8 +417,10 @@ jobs: NEW="${{ needs.preflight.outputs.new_version }}" BUMP="${{ needs.preflight.outputs.bump_type }}" - # Stage VERSION, dist/VERSION (if tracked), CHANGELOG + # Stage VERSION, dist/VERSION (if tracked), CHANGELOG, and the + # version-bearing artifacts synced above (#177 Blocker 2). git add VERSION CHANGELOG.md + git add helm/bnk-forge/values.yaml helm/bnk-forge/Chart.yaml frontend-v2/package.json git ls-files --error-unmatch dist/VERSION 2>/dev/null && git add dist/VERSION || true git commit -m "release: v${NEW} [skip ci] @@ -515,6 +521,7 @@ jobs: if git ls-files --error-unmatch dist/VERSION 2>/dev/null; then echo "${{ needs.preflight.outputs.new_version }}" > dist/VERSION fi + bash scripts/sync-version-artifacts.sh --write "${{ needs.preflight.outputs.new_version }}" - name: Update changelog run: | @@ -548,6 +555,7 @@ jobs: - name: Commit and tag run: | git add VERSION CHANGELOG.md + git add helm/bnk-forge/values.yaml helm/bnk-forge/Chart.yaml frontend-v2/package.json git ls-files --error-unmatch dist/VERSION 2>/dev/null && git add dist/VERSION || true git commit -m "release: v${{ needs.preflight.outputs.new_version }} [skip ci] diff --git a/frontend-v2/package.json b/frontend-v2/package.json index 4e25c0cf..7282abed 100644 --- a/frontend-v2/package.json +++ b/frontend-v2/package.json @@ -1,7 +1,7 @@ { "name": "frontend-v2", "private": true, - "version": "2.12.0", + "version": "3.1.6", "type": "module", "sideEffects": [ "*.css" diff --git a/helm/bnk-forge/Chart.yaml b/helm/bnk-forge/Chart.yaml index 950b44a2..40db248d 100644 --- a/helm/bnk-forge/Chart.yaml +++ b/helm/bnk-forge/Chart.yaml @@ -3,7 +3,7 @@ name: bnk-forge description: BNK-Forge — F5 BNK lifecycle / deployment platform (api, workers, beat, frontend, proxy, mcp) type: application version: 0.1.0 -appVersion: "3.0.1" +appVersion: "3.1.6" home: https://github.com/f5devcentral/bnk-forge maintainers: - name: BNK Forge Maintainers diff --git a/helm/bnk-forge/values.yaml b/helm/bnk-forge/values.yaml index bb880602..97b654da 100644 --- a/helm/bnk-forge/values.yaml +++ b/helm/bnk-forge/values.yaml @@ -16,7 +16,7 @@ global: image: pullPolicy: IfNotPresent - tag: "3.0.1" + tag: "3.1.6" # Generated/explicit secrets. If left empty, helm generates random values on # first install and reuses them on upgrade (lookup-based). diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh new file mode 100644 index 00000000..b02ceb01 --- /dev/null +++ b/scripts/sync-version-artifacts.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# Keep every version-bearing artifact in lockstep with VERSION. +# +# The release job bumps VERSION but historically nothing else, so the Helm chart +# pinned an image tag the release never publishes (:3.0.1) -> ImagePullBackOff, +# and frontend package.json drifted (PR #177 review, Blocker 2). This is the one +# place that writes them, and the same code checks them in CI so drift can't +# reappear silently. +# +# Usage: +# sync-version-artifacts.sh --write # set all artifacts to +# sync-version-artifacts.sh --check # verify all == VERSION; exit 1 if not +set -euo pipefail + +ROOT="$(cd "$(dirname "$0")/.." && pwd)" +VALUES="$ROOT/helm/bnk-forge/values.yaml" +CHART="$ROOT/helm/bnk-forge/Chart.yaml" +PKG="$ROOT/frontend-v2/package.json" + +# Read each artifact's current version. +_helm_tag() { grep -m1 -E '^ tag: ' "$VALUES" | sed -E 's/^ tag: "?([^"]*)"?.*/\1/'; } +_appversion() { grep -m1 -E '^appVersion:' "$CHART" | sed -E 's/^appVersion: "?([^"]*)"?.*/\1/'; } +_pkg_version() { grep -m1 -E '^ "version":' "$PKG" | sed -E 's/^ "version": "([^"]*)".*/\1/'; } + +case "${1:-}" in + --write) + V="${2:?usage: sync-version-artifacts.sh --write }" + # The global image tag (2-space indent) — per-service tags are 4-space and + # fall back to it; postgres/redis tags are external and left alone. + sed -i -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" + sed -i -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" + sed -i -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" + echo "synced helm tag, appVersion, frontend package.json -> ${V}" + ;; + --check) + EXPECTED="$(cat "$ROOT/VERSION")" + rc=0 + for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)"; do + name="${pair%%:*}"; got="${pair#*:}" + if [ "$got" = "$EXPECTED" ]; then + echo " OK $name = $got" + else + echo "::error::$name is '$got' but VERSION is '$EXPECTED' — the release publishes only :\${VERSION}, so a mismatch means ImagePullBackOff / drift. Run scripts/sync-version-artifacts.sh --write $EXPECTED" + rc=1 + fi + done + exit "$rc" + ;; + *) + echo "usage: sync-version-artifacts.sh --write | --check" >&2 + exit 2 + ;; +esac From 64a37f98e64c3dd7413adc8b2605a469559e6486 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 23:34:03 -0500 Subject: [PATCH 2/9] fix: generate mcp-password instead of shipping "changeme" in the public chart PR #177 nit (bonnyr-f5): helm/bnk-forge/values.yaml shipped `mcpPassword: changeme` -- a known default password now that the chart is the public distribution path. The other four secrets (postgres/redis/jwt/encryption) are generated with randAlphaNum when left empty and reused across upgrades via the existing-secret lookup, but mcp-password had no such generation and used the raw value directly. Added the same lookup-then-randAlphaNum(24) logic for mcp-password and blanked the default in values.yaml, with a comment on how to retrieve the generated value (kubectl get secret ... | base64 -d). `helm template` confirms a random mcp-password is rendered, not "changeme". Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- helm/bnk-forge/templates/secrets.yaml | 8 +++++++- helm/bnk-forge/values.yaml | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/helm/bnk-forge/templates/secrets.yaml b/helm/bnk-forge/templates/secrets.yaml index 39531c55..b5b8f844 100644 --- a/helm/bnk-forge/templates/secrets.yaml +++ b/helm/bnk-forge/templates/secrets.yaml @@ -25,6 +25,12 @@ {{- end -}} {{- if not $enc -}}{{- $enc = randAlphaNum 32 -}}{{- end -}} +{{- $mcpPass := .Values.secrets.mcpPassword -}} +{{- if and (not $mcpPass) $existing -}} +{{- $mcpPass = (index $existing.data "mcp-password" | b64dec) -}} +{{- end -}} +{{- if not $mcpPass -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}} + apiVersion: v1 kind: Secret metadata: @@ -38,4 +44,4 @@ stringData: jwt-secret-key: {{ $jwt | quote }} encryption-key: {{ $enc | quote }} mcp-username: {{ .Values.secrets.mcpUsername | quote }} - mcp-password: {{ .Values.secrets.mcpPassword | quote }} + mcp-password: {{ $mcpPass | quote }} diff --git a/helm/bnk-forge/values.yaml b/helm/bnk-forge/values.yaml index 97b654da..427999a7 100644 --- a/helm/bnk-forge/values.yaml +++ b/helm/bnk-forge/values.yaml @@ -26,7 +26,9 @@ secrets: jwtSecretKey: "" encryptionKey: "" mcpUsername: admin - mcpPassword: changeme + # Empty -> generated on first install and reused on upgrade, like the secrets + # above. Never ship a known default in the public chart (#177 review). Retrieve + # it with: kubectl get secret -secrets -o jsonpath='{.data.mcp-password}' | base64 -d # Common pod settings # fsGroup=1000 so PVC-backed shared volumes are writable by the bnkforge user (UID 1000). From 2b01fb4a6b2e0615494abfa297027cc7d6b27a48 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 01:45:37 -0500 Subject: [PATCH 3/9] fix: sync --write fails closed; drop the half-fix MCP secret (defer to #188) bonnyrf5 aggregate review, #180. --write fails open (scripts/sync-version-artifacts.sh): a sed whose pattern matched nothing no-ops silently, so a format change to any artifact left it unchanged while the script still reported success -- and the release job commits that with CI suppressed. Now re-reads all three with the same helpers --check trusts and exits 1 if any didn't take ${V}. Verified: happy path passes, a package.json whose "version" line no longer matches makes it exit 1. MCP secret (secrets.yaml / values.yaml): this PR generated mcp-password as a chart-owned secret with mcpUsername: admin, which breaks MCP auth on every fresh install -- it's a client credential the MCP server must also read, not a chart-owned value. That's the half-fix bonnyrf5 flagged. The complete fix (point the chart at the mcp service account, wire MCP_SERVICE_PASSWORD into the backend so ensure_service_user reconciles the hash, rotate the shipped default on upgrade, checksum/secret roll) lives in #188. Reverted the MCP edits here so this PR stays scoped to version-artifact consistency; its MCP diff vs staging is now empty, so it no longer overlaps #188. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- helm/bnk-forge/templates/secrets.yaml | 8 +------- helm/bnk-forge/values.yaml | 4 +--- scripts/sync-version-artifacts.sh | 13 +++++++++++++ 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/helm/bnk-forge/templates/secrets.yaml b/helm/bnk-forge/templates/secrets.yaml index b5b8f844..39531c55 100644 --- a/helm/bnk-forge/templates/secrets.yaml +++ b/helm/bnk-forge/templates/secrets.yaml @@ -25,12 +25,6 @@ {{- end -}} {{- if not $enc -}}{{- $enc = randAlphaNum 32 -}}{{- end -}} -{{- $mcpPass := .Values.secrets.mcpPassword -}} -{{- if and (not $mcpPass) $existing -}} -{{- $mcpPass = (index $existing.data "mcp-password" | b64dec) -}} -{{- end -}} -{{- if not $mcpPass -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}} - apiVersion: v1 kind: Secret metadata: @@ -44,4 +38,4 @@ stringData: jwt-secret-key: {{ $jwt | quote }} encryption-key: {{ $enc | quote }} mcp-username: {{ .Values.secrets.mcpUsername | quote }} - mcp-password: {{ $mcpPass | quote }} + mcp-password: {{ .Values.secrets.mcpPassword | quote }} diff --git a/helm/bnk-forge/values.yaml b/helm/bnk-forge/values.yaml index 427999a7..97b654da 100644 --- a/helm/bnk-forge/values.yaml +++ b/helm/bnk-forge/values.yaml @@ -26,9 +26,7 @@ secrets: jwtSecretKey: "" encryptionKey: "" mcpUsername: admin - # Empty -> generated on first install and reused on upgrade, like the secrets - # above. Never ship a known default in the public chart (#177 review). Retrieve - # it with: kubectl get secret -secrets -o jsonpath='{.data.mcp-password}' | base64 -d + mcpPassword: changeme # Common pod settings # fsGroup=1000 so PVC-backed shared volumes are writable by the bnkforge user (UID 1000). diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index b02ceb01..f787b67b 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -30,6 +30,19 @@ case "${1:-}" in sed -i -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" sed -i -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" sed -i -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" + # Fail closed: a sed whose pattern matched nothing no-ops silently, and the + # caller commits the unchanged file [skip ci] believing it synced (#180 + # review). Re-read each artifact with the same helpers --check trusts and + # confirm it actually took ${V}. + rc=0 + for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)"; do + name="${pair%%:*}"; got="${pair#*:}" + if [ "$got" != "$V" ]; then + echo "::error::--write did not take on $name: it is '$got', expected '$V' (the sed pattern matched nothing — the artifact's format changed)" >&2 + rc=1 + fi + done + [ "$rc" -eq 0 ] || exit 1 echo "synced helm tag, appVersion, frontend package.json -> ${V}" ;; --check) From 99a286fce5392dccdd91ea19e8da9cfed75bf9a2 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 07:06:34 -0500 Subject: [PATCH 4/9] docs(AGENTS): never put a CI-control marker in a commit message body mwiget's note on #180: a commit whose body quoted the CI-skip marker in prose had its whole run suppressed (GitHub scans the entire message), and because the change was a shell script the skipped gates were exactly the relevant ones. Documented the rule and the indirect phrasings so it does not recur. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- AGENTS.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index c2289cd1..4ebf357c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,5 +82,19 @@ Strong success criteria let you loop independently. Weak criteria ("make it work **These guidelines are working if:** fewer unnecessary changes in diffs, fewer rewrites due to overcomplication, and clarifying questions come before implementation rather than after mistakes. +## Commit conventions + +Conventional Commits (`type: subject`, optional body, `BREAKING CHANGE:` footer for a +major). One repo-specific trap worth stating outright: + +- **Never write a CI-control marker as literal text anywhere in a commit message — + subject *or* body — even when quoting it in prose.** GitHub scans the whole message, + so a `[skip ci]` / `[ci skip]` sitting in a sentence suppresses the run for that + commit. This has bitten us twice, most recently on a shell-script change where the + gates that got skipped (ShellCheck, Script Self-Tests, Secret Scan) were exactly the + ones that mattered. Refer to it indirectly instead: "CI suppressed", "the skip-CI + marker", or split it across backticks. The release job's *deliberate* skip is the + only legitimate use, and it lands on the subject line where the release loop reads it. + --- From 5d25b31e3f2baaec4ba64529daf4bf6046f30d8d Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 07:46:38 -0500 Subject: [PATCH 5/9] docs: note that Chart.yaml version is deliberately not synced mwiget nit on #180: a line in the sync-version-artifacts.sh header saying the chart's own `version:` is intentionally left out of the sync (Helm treats chart version and appVersion independently, and release.yml doesn't package/push the chart) so the next person doesn't 'fix' it to match VERSION. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- scripts/sync-version-artifacts.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index f787b67b..4ce0d7a6 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -7,6 +7,12 @@ # place that writes them, and the same code checks them in CI so drift can't # reappear silently. # +# Synced: the Helm image tag (values.yaml), Chart `appVersion`, and frontend +# package.json. NOT synced, deliberately: Chart.yaml's own `version:` — Helm +# treats the chart version and appVersion as independent, and release.yml neither +# packages nor pushes the chart, so a static chart version publishes nothing +# wrong. Leave it alone rather than "fixing" it to match VERSION. +# # Usage: # sync-version-artifacts.sh --write # set all artifacts to # sync-version-artifacts.sh --check # verify all == VERSION; exit 1 if not From 96eb0495d63450eb3ea0b20583c7e9dbc4970346 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 19:30:39 -0500 Subject: [PATCH 6/9] fix: sibling operator chart 404 pin; portable sed; verify every tag line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bonnyr-f5 REVISE review of #180. All findings reproduced and confirmed. MAJOR — the bnk-operator chart pinned f5/bnk-operator:1.2.0, a 404 (the release publishes ghcr.io/f5devcentral/bnk-forge-operator). Fixed the repository + tag to a real published image. Narrowed the script header's "every version-bearing artifact" claim to what it actually syncs (the bnk-forge chart + frontend); the operator chart carries its own version line and isn't swept here. MAJOR — --write's fail-closed guarantee had a hole: sed rewrites EVERY 2-space `^ tag:` line but the verify helper read only grep -m1 (the first), so a second tag line could be clobbered while --check stayed green. Now verifies every 2-space tag line equals ${V}. MAJOR — `sed -i -E` isn't BSD-portable (BSD swallows -E as the -i suffix and litters *-E files); --check's error message tells developers to run exactly that command. Switched to the attached-suffix form both seds accept, removing the backups. Verified: --write leaves no stray files, --check passes. Acknowledged (documented): CRLF self-contradiction, --write input validation, package-lock cosmetic drift, and the missing --self-test/CI job (follow-up). Merge #180 as a MERGE COMMIT (not squash) so #182's shared prefix stays intact. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- bnk-operator/charts/bnk-operator/values.yaml | 4 ++-- scripts/sync-version-artifacts.sh | 24 ++++++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/bnk-operator/charts/bnk-operator/values.yaml b/bnk-operator/charts/bnk-operator/values.yaml index 80603000..9d26b3ac 100644 --- a/bnk-operator/charts/bnk-operator/values.yaml +++ b/bnk-operator/charts/bnk-operator/values.yaml @@ -45,8 +45,8 @@ cwc: # Operator image image: - repository: f5/bnk-operator - tag: "1.2.0" + repository: ghcr.io/f5devcentral/bnk-forge-operator + tag: "3.1.6" pullPolicy: IfNotPresent # Image pull secrets (if using private registry) diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index 4ce0d7a6..8d191162 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -1,5 +1,7 @@ #!/usr/bin/env bash -# Keep every version-bearing artifact in lockstep with VERSION. +# Keep the bnk-forge chart's image tag + Chart appVersion and the frontend +# package.json in lockstep with VERSION. (The separate bnk-operator chart has its +# own version line and is not synced here -- bonnyr-f5 #180.) # # The release job bumps VERSION but historically nothing else, so the Helm chart # pinned an image tag the release never publishes (:3.0.1) -> ImagePullBackOff, @@ -33,9 +35,13 @@ case "${1:-}" in V="${2:?usage: sync-version-artifacts.sh --write }" # The global image tag (2-space indent) — per-service tags are 4-space and # fall back to it; postgres/redis tags are external and left alone. - sed -i -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" - sed -i -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" - sed -i -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" + # -i.bak (attached suffix) is the one in-place form both GNU and BSD sed + # accept; the plain `-i -E` used before makes BSD swallow -E as the backup + # suffix and litter *-E files (bonnyr-f5 #180). Backups removed after. + sed -i.syncbak -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" + sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" + sed -i.syncbak -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" + rm -f "${VALUES}.syncbak" "${CHART}.syncbak" "${PKG}.syncbak" # Fail closed: a sed whose pattern matched nothing no-ops silently, and the # caller commits the unchanged file [skip ci] believing it synced (#180 # review). Re-read each artifact with the same helpers --check trusts and @@ -48,6 +54,16 @@ case "${1:-}" in rc=1 fi done + # grep -m1 above only reads the FIRST `^ tag:`; the sed rewrote EVERY one. + # Fail if any 2-space tag line disagrees, so a second one can't be silently + # clobbered while --check stays green (bonnyr-f5 #180). + while IFS= read -r line; do + t=$(sed -E 's/^ tag: "?([^"]*)"?.*/\1/' <<< "$line") + if [ "$t" != "$V" ]; then + echo "::error::--write left a 2-space 'tag:' at '$t', expected '$V' (multiple tag lines)" >&2 + rc=1 + fi + done < <(grep -E '^ tag: ' "$VALUES") [ "$rc" -eq 0 ] || exit 1 echo "synced helm tag, appVersion, frontend package.json -> ${V}" ;; From 8f96a0a350a903d90ac86963f3924914239bfb43 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 20:46:21 -0500 Subject: [PATCH 7/9] fix: sync the operator chart too; --check reads every tag and asserts non-vacuity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bonnyr-f5 round-2 REVISE of #180. All reproduced. MAJOR 1 — round 1 pinned the operator chart to 3.1.6 but left it OUTSIDE the syncer, so at 3.1.7 it would pin 3.1.6 again — the ImagePullBackOff class this PR exists to close, reintroduced. The operator image publishes at :${VERSION}, so the chart belongs on the VERSION train: added its values.yaml image.tag and Chart appVersion to both --write and --check. Header comment now matches the code. MAJOR 2 — --check read only the first `^ tag:` (grep -m1), blind to a drifted second tag that --write would clobber. --check now verifies EVERY 2-space tag across both values.yaml files, not just the first. MAJOR 3 (INV-16) — the gate could pass vacuously (an empty comparison list exits 0 with drift present). --check now counts what it compared and fails if it checked fewer than the expected artifacts. Verified: --write syncs all five artifacts + the operator chart, --check reports all five OK, shellcheck clean. Acknowledged: CRLF, --write input validation, package-lock (minors); --self-test + gated CI job (follow-up). Merge #180 as a MERGE COMMIT so #182's prefix is intact. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- bnk-operator/charts/bnk-operator/Chart.yaml | 2 +- scripts/sync-version-artifacts.sh | 28 ++++++++++++++++----- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/bnk-operator/charts/bnk-operator/Chart.yaml b/bnk-operator/charts/bnk-operator/Chart.yaml index eefd4535..285cb08c 100644 --- a/bnk-operator/charts/bnk-operator/Chart.yaml +++ b/bnk-operator/charts/bnk-operator/Chart.yaml @@ -3,7 +3,7 @@ name: bnk-operator description: BNK Operator — lightweight agent that connects K8s clusters to BNK-Forge type: application version: 1.1.0 -appVersion: "1.1.0" +appVersion: "3.1.6" keywords: - f5 - bnk diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index 8d191162..8ebe63ff 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -24,11 +24,17 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd)" VALUES="$ROOT/helm/bnk-forge/values.yaml" CHART="$ROOT/helm/bnk-forge/Chart.yaml" PKG="$ROOT/frontend-v2/package.json" +# bonnyr-f5 #180 r2: the sibling operator chart is on the VERSION train (its +# image publishes at :${VERSION}), so sync it here too instead of pinning it. +OPVALUES="$ROOT/bnk-operator/charts/bnk-operator/values.yaml" +OPCHART="$ROOT/bnk-operator/charts/bnk-operator/Chart.yaml" # Read each artifact's current version. _helm_tag() { grep -m1 -E '^ tag: ' "$VALUES" | sed -E 's/^ tag: "?([^"]*)"?.*/\1/'; } _appversion() { grep -m1 -E '^appVersion:' "$CHART" | sed -E 's/^appVersion: "?([^"]*)"?.*/\1/'; } _pkg_version() { grep -m1 -E '^ "version":' "$PKG" | sed -E 's/^ "version": "([^"]*)".*/\1/'; } +_op_tag() { grep -m1 -E '^ tag: ' "$OPVALUES" | sed -E 's/^ tag: "?([^"]*)"?.*/\1/'; } +_op_appversion() { grep -m1 -E '^appVersion:' "$OPCHART" | sed -E 's/^appVersion: "?([^"]*)"?.*/\1/'; } case "${1:-}" in --write) @@ -41,13 +47,15 @@ case "${1:-}" in sed -i.syncbak -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" sed -i.syncbak -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" - rm -f "${VALUES}.syncbak" "${CHART}.syncbak" "${PKG}.syncbak" + sed -i.syncbak -E "s|^ tag: .*| tag: \"${V}\"|" "$OPVALUES" + sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$OPCHART" + rm -f "${VALUES}.syncbak" "${CHART}.syncbak" "${PKG}.syncbak" "${OPVALUES}.syncbak" "${OPCHART}.syncbak" # Fail closed: a sed whose pattern matched nothing no-ops silently, and the # caller commits the unchanged file [skip ci] believing it synced (#180 # review). Re-read each artifact with the same helpers --check trusts and # confirm it actually took ${V}. rc=0 - for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)"; do + for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)" "operator image.tag:$(_op_tag)" "operator appVersion:$(_op_appversion)"; do name="${pair%%:*}"; got="${pair#*:}" if [ "$got" != "$V" ]; then echo "::error::--write did not take on $name: it is '$got', expected '$V' (the sed pattern matched nothing — the artifact's format changed)" >&2 @@ -63,15 +71,15 @@ case "${1:-}" in echo "::error::--write left a 2-space 'tag:' at '$t', expected '$V' (multiple tag lines)" >&2 rc=1 fi - done < <(grep -E '^ tag: ' "$VALUES") + done < <(grep -hE '^ tag: ' "$VALUES" "$OPVALUES") [ "$rc" -eq 0 ] || exit 1 echo "synced helm tag, appVersion, frontend package.json -> ${V}" ;; --check) EXPECTED="$(cat "$ROOT/VERSION")" - rc=0 - for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)"; do - name="${pair%%:*}"; got="${pair#*:}" + rc=0; checked=0 + for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)" "operator image.tag:$(_op_tag)" "operator appVersion:$(_op_appversion)"; do + name="${pair%%:*}"; got="${pair#*:}"; checked=$((checked+1)) if [ "$got" = "$EXPECTED" ]; then echo " OK $name = $got" else @@ -79,6 +87,14 @@ case "${1:-}" in rc=1 fi done + # bonnyr-f5 #180 r2 (INV-16): assert the gate actually compared something -- + # an empty list would exit 0 with real drift present. Also catch a drifted + # SECOND 2-space tag that grep -m1 above can't see. + while IFS= read -r t; do + t=$(sed -E 's/^ tag: "?([^"]*)"?.*/\1/' <<< "$t"); checked=$((checked+1)) + [ "$t" = "$EXPECTED" ] || { echo "::error::a 2-space 'tag:' reads '$t' but VERSION is '$EXPECTED'"; rc=1; } + done < <(grep -hE '^ tag: ' "$VALUES" "$OPVALUES") + [ "$checked" -ge 5 ] || { echo "::error::--check compared only $checked artifacts (expected >=5) — vacuous"; exit 1; } exit "$rc" ;; *) From 35e7975cec462980a15ba791df244df174c6f4ac Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 23:29:28 -0500 Subject: [PATCH 8/9] fix: stage every synced artifact, and make --check unable to pass on empty data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bonnyr-f5 round-3 BLOCK on #180. Both blockers reproduced and fixed, each with a red-green test that proves the guard now fails on the exact input it missed. BLOCKER 1 — writer/stager divergence. --write rewrote five artifacts (incl. the two operator-chart files this round added) but release.yml's "Commit and tag" step hard-coded `git add` for three, so the operator chart was rewritten on the runner and never staged -> next PR's version-consistency job red-lined, blocking every subsequent release. Fixed structurally: the script owns a canonical SYNCED_FILES list exposed via a new --list mode, and both "Commit and tag" steps stage exactly `sync-version-artifacts.sh --list` -> writer and stager cannot diverge. Added an INDEX check (git diff --quiet per file) so a synced-but-unstaged artifact fails the release rather than the next PR (bonnyr: "verify the index, not the files"). Verified: all five stage; unstaging one is caught. BLOCKER 2 — --check green on a tree with no version data. The non-vacuity guard counted loop iterations over a literal 5-item list, so `checked>=5` was always true and its guard unreachable; empty compared equal to empty five times, rc 0. Rewritten to assert each version line is NON-EMPTY and equals VERSION, that VERSION itself is non-empty, and that every artifact contributed >=1 MATCHED line (total counts matched lines, not iterations). Verified against bonnyr's exact repro (empty VERSION + every key deleted) -> now rc 1. Majors (same review): - Every reader now reads EVERY matching line (was grep -m1 first-match against a global write), so a second appVersion/tag can't drift unseen. Verified: a drifted second 2-space tag is caught. - The image-tag writer is bounded to the top-level `image:` block via a sed range instead of a bare `^ tag:`; a future unrelated 2-space tag is left alone. Verified. appVersion/version writes are already key-anchored (single-occurrence top-level keys). Docs: header, the "Synced" list and the success echo now name all five artifacts (they claimed three while five were written, and one line still said the operator chart was not synced while the code synced it). Confirmed: operator image IS on the release train (docker-bake.hcl `default` group builds it), so syncing its chart to VERSION is correct. shellcheck -S style clean; real-tree --check green; full red-green matrix in the PR comment. Merge-order note for the record: merge #180 before #182, or with a merge commit — #182 carries a pre-round-2 copy of this script, and a squash would arrive as an add/add conflict on it. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/release.yml | 26 ++++- scripts/sync-version-artifacts.sh | 180 +++++++++++++++++++----------- 2 files changed, 139 insertions(+), 67 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9b0facff..930203b8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -420,7 +420,18 @@ jobs: # Stage VERSION, dist/VERSION (if tracked), CHANGELOG, and the # version-bearing artifacts synced above (#177 Blocker 2). git add VERSION CHANGELOG.md - git add helm/bnk-forge/values.yaml helm/bnk-forge/Chart.yaml frontend-v2/package.json + # Stage EXACTLY the artifacts sync-version-artifacts.sh owns, from its + # own --list, so a newly-synced file can never be left unstaged and die + # with the runner (bonnyr-f5 #180 r3, BLOCKER 1: --write rewrote five + # files, the hard-coded `git add` staged three). + while IFS= read -r f; do git add "$f"; done < <(bash scripts/sync-version-artifacts.sh --list) + # Verify the INDEX, not the files: --write's post-write check re-reads + # the files (correct on disk even when unstaged), so assert each synced + # artifact has no unstaged residue — i.e. the sync is actually in the + # commit we are about to make. + while IFS= read -r f; do + git diff --quiet -- "$f" || { echo "::error::$f was synced but is not fully staged"; exit 1; } + done < <(bash scripts/sync-version-artifacts.sh --list) git ls-files --error-unmatch dist/VERSION 2>/dev/null && git add dist/VERSION || true git commit -m "release: v${NEW} [skip ci] @@ -555,7 +566,18 @@ jobs: - name: Commit and tag run: | git add VERSION CHANGELOG.md - git add helm/bnk-forge/values.yaml helm/bnk-forge/Chart.yaml frontend-v2/package.json + # Stage EXACTLY the artifacts sync-version-artifacts.sh owns, from its + # own --list, so a newly-synced file can never be left unstaged and die + # with the runner (bonnyr-f5 #180 r3, BLOCKER 1: --write rewrote five + # files, the hard-coded `git add` staged three). + while IFS= read -r f; do git add "$f"; done < <(bash scripts/sync-version-artifacts.sh --list) + # Verify the INDEX, not the files: --write's post-write check re-reads + # the files (correct on disk even when unstaged), so assert each synced + # artifact has no unstaged residue — i.e. the sync is actually in the + # commit we are about to make. + while IFS= read -r f; do + git diff --quiet -- "$f" || { echo "::error::$f was synced but is not fully staged"; exit 1; } + done < <(bash scripts/sync-version-artifacts.sh --list) git ls-files --error-unmatch dist/VERSION 2>/dev/null && git add dist/VERSION || true git commit -m "release: v${{ needs.preflight.outputs.new_version }} [skip ci] diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index 8ebe63ff..79c8b0ed 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -1,104 +1,154 @@ #!/usr/bin/env bash -# Keep the bnk-forge chart's image tag + Chart appVersion and the frontend -# package.json in lockstep with VERSION. (The separate bnk-operator chart has its -# own version line and is not synced here -- bonnyr-f5 #180.) +# Keep the version-bearing release artifacts in lockstep with VERSION: +# - the bnk-forge Helm chart image tag (values.yaml) and Chart `appVersion` +# - the frontend package.json version +# - the sibling bnk-operator chart image tag (values.yaml) and `appVersion` # -# The release job bumps VERSION but historically nothing else, so the Helm chart -# pinned an image tag the release never publishes (:3.0.1) -> ImagePullBackOff, -# and frontend package.json drifted (PR #177 review, Blocker 2). This is the one -# place that writes them, and the same code checks them in CI so drift can't +# All five publish at :${VERSION} on the release train — docker-bake.hcl's +# `default` group builds the operator image alongside the rest — so any drift +# means an image tag the release never publishes -> ImagePullBackOff. This is the +# one place that writes them, and --check verifies them in CI so drift can't # reappear silently. # -# Synced: the Helm image tag (values.yaml), Chart `appVersion`, and frontend -# package.json. NOT synced, deliberately: Chart.yaml's own `version:` — Helm -# treats the chart version and appVersion as independent, and release.yml neither -# packages nor pushes the chart, so a static chart version publishes nothing -# wrong. Leave it alone rather than "fixing" it to match VERSION. +# Synced: the bnk-forge image tag + appVersion, the frontend package.json, AND +# the bnk-operator image tag + appVersion. NOT synced, deliberately: each +# Chart.yaml's own `version:` — Helm treats the chart version and appVersion as +# independent, and release.yml neither packages nor pushes the chart, so a static +# chart version publishes nothing wrong. Leave it alone rather than "fixing" it. # # Usage: # sync-version-artifacts.sh --write # set all artifacts to # sync-version-artifacts.sh --check # verify all == VERSION; exit 1 if not +# sync-version-artifacts.sh --list # print artifact paths (repo-relative) set -euo pipefail ROOT="$(cd "$(dirname "$0")/.." && pwd)" VALUES="$ROOT/helm/bnk-forge/values.yaml" CHART="$ROOT/helm/bnk-forge/Chart.yaml" PKG="$ROOT/frontend-v2/package.json" -# bonnyr-f5 #180 r2: the sibling operator chart is on the VERSION train (its -# image publishes at :${VERSION}), so sync it here too instead of pinning it. +# The sibling operator chart is on the VERSION train (its image publishes at +# :${VERSION}), so it is synced here too rather than pinned. OPVALUES="$ROOT/bnk-operator/charts/bnk-operator/values.yaml" OPCHART="$ROOT/bnk-operator/charts/bnk-operator/Chart.yaml" -# Read each artifact's current version. -_helm_tag() { grep -m1 -E '^ tag: ' "$VALUES" | sed -E 's/^ tag: "?([^"]*)"?.*/\1/'; } -_appversion() { grep -m1 -E '^appVersion:' "$CHART" | sed -E 's/^appVersion: "?([^"]*)"?.*/\1/'; } -_pkg_version() { grep -m1 -E '^ "version":' "$PKG" | sed -E 's/^ "version": "([^"]*)".*/\1/'; } -_op_tag() { grep -m1 -E '^ tag: ' "$OPVALUES" | sed -E 's/^ tag: "?([^"]*)"?.*/\1/'; } -_op_appversion() { grep -m1 -E '^appVersion:' "$OPCHART" | sed -E 's/^appVersion: "?([^"]*)"?.*/\1/'; } +# Canonical artifact list. --write, --list, and the release job's `git add` all +# derive the file set from HERE, so the writer and its stager cannot diverge and +# leave a synced-but-unstaged file behind (bonnyr-f5 #180 r3, BLOCKER 1). +SYNCED_FILES=("$VALUES" "$CHART" "$PKG" "$OPVALUES" "$OPCHART") + +# ── Value readers ───────────────────────────────────────────────────────────── +# Each reads EVERY matching version line (not grep -m1), so a second occurrence +# can't drift unseen behind a global write (bonnyr-f5 #180 r3). `^ tag: ` (two +# spaces) matches only the top-level image tag — the postgres/redis/per-service +# tags are 4-space and never match. +TAG_RE='^ tag: ' +TAG_SED='s/^ tag: "?([^"]*)"?.*/\1/' +APPVER_RE='^appVersion:' +APPVER_SED='s/^appVersion: "?([^"]*)"?.*/\1/' +PKGVER_RE='^ "version":' +PKGVER_SED='s/^ "version": "([^"]*)".*/\1/' case "${1:-}" in --write) V="${2:?usage: sync-version-artifacts.sh --write }" - # The global image tag (2-space indent) — per-service tags are 4-space and - # fall back to it; postgres/redis tags are external and left alone. - # -i.bak (attached suffix) is the one in-place form both GNU and BSD sed - # accept; the plain `-i -E` used before makes BSD swallow -E as the backup - # suffix and litter *-E files (bonnyr-f5 #180). Backups removed after. - sed -i.syncbak -E "s|^ tag: .*| tag: \"${V}\"|" "$VALUES" + # Anchor every substitution to its key PATH, not a bare 2-space `tag:`. The + # image-tag writes are scoped to the top-level `image:` block via a sed range + # (`/^image:/` to the next column-0 key) so a future unrelated 2-space `tag:` + # elsewhere is never repinned to VERSION (bonnyr-f5 #180 r3, unbounded writer). + # -i.syncbak (attached suffix) is the one in-place form both GNU and BSD sed + # accept; `-i -E` makes BSD swallow -E as the suffix and litter *-E files. + sed -i.syncbak -E "/^image:/,/^[^[:space:]]/ s|^ tag: .*| tag: \"${V}\"|" "$VALUES" sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" sed -i.syncbak -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" - sed -i.syncbak -E "s|^ tag: .*| tag: \"${V}\"|" "$OPVALUES" + sed -i.syncbak -E "/^image:/,/^[^[:space:]]/ s|^ tag: .*| tag: \"${V}\"|" "$OPVALUES" sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$OPCHART" - rm -f "${VALUES}.syncbak" "${CHART}.syncbak" "${PKG}.syncbak" "${OPVALUES}.syncbak" "${OPCHART}.syncbak" + for f in "${SYNCED_FILES[@]}"; do rm -f "${f}.syncbak"; done + # Fail closed: a sed whose pattern matched nothing no-ops silently, and the - # caller commits the unchanged file [skip ci] believing it synced (#180 - # review). Re-read each artifact with the same helpers --check trusts and - # confirm it actually took ${V}. + # caller would commit the unchanged file believing it synced (#177 review). + # Re-read every version line in every artifact with the SAME readers --check + # uses and confirm each one actually took ${V} — and that at least one line + # matched per artifact, so a renamed key can't pass as "nothing to change". rc=0 - for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)" "operator image.tag:$(_op_tag)" "operator appVersion:$(_op_appversion)"; do - name="${pair%%:*}"; got="${pair#*:}" - if [ "$got" != "$V" ]; then - echo "::error::--write did not take on $name: it is '$got', expected '$V' (the sed pattern matched nothing — the artifact's format changed)" >&2 - rc=1 - fi - done - # grep -m1 above only reads the FIRST `^ tag:`; the sed rewrote EVERY one. - # Fail if any 2-space tag line disagrees, so a second one can't be silently - # clobbered while --check stays green (bonnyr-f5 #180). - while IFS= read -r line; do - t=$(sed -E 's/^ tag: "?([^"]*)"?.*/\1/' <<< "$line") - if [ "$t" != "$V" ]; then - echo "::error::--write left a 2-space 'tag:' at '$t', expected '$V' (multiple tag lines)" >&2 + _verify_file() { # label, file, grep-ERE, extract-sed + local label="$1" file="$2" gre="$3" ext="$4" n=0 line val + while IFS= read -r line; do + val=$(sed -E "$ext" <<< "$line"); n=$((n + 1)) + if [ "$val" != "$V" ]; then + echo "::error::--write did not take on $label: it is '$val', expected '$V' (the sed pattern matched nothing — the artifact's format changed)" >&2 + rc=1 + fi + done < <(grep -E "$gre" "$file" || true) + if [ "$n" -eq 0 ]; then + echo "::error::--write found no '$label' line in $file (key renamed/removed?) — nothing was synced" >&2 rc=1 fi - done < <(grep -hE '^ tag: ' "$VALUES" "$OPVALUES") + } + _verify_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" + _verify_file "Chart appVersion" "$CHART" "$APPVER_RE" "$APPVER_SED" + _verify_file "frontend version" "$PKG" "$PKGVER_RE" "$PKGVER_SED" + _verify_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" + _verify_file "operator appVersion" "$OPCHART" "$APPVER_RE" "$APPVER_SED" [ "$rc" -eq 0 ] || exit 1 - echo "synced helm tag, appVersion, frontend package.json -> ${V}" + echo "synced bnk-forge tag+appVersion, frontend package.json, operator tag+appVersion -> ${V}" >&2 ;; + --check) EXPECTED="$(cat "$ROOT/VERSION")" - rc=0; checked=0 - for pair in "helm image.tag:$(_helm_tag)" "Chart appVersion:$(_appversion)" "frontend package.json:$(_pkg_version)" "operator image.tag:$(_op_tag)" "operator appVersion:$(_op_appversion)"; do - name="${pair%%:*}"; got="${pair#*:}"; checked=$((checked+1)) - if [ "$got" = "$EXPECTED" ]; then - echo " OK $name = $got" - else - echo "::error::$name is '$got' but VERSION is '$EXPECTED' — the release publishes only :\${VERSION}, so a mismatch means ImagePullBackOff / drift. Run scripts/sync-version-artifacts.sh --write $EXPECTED" - rc=1 + # VERSION itself must be non-empty, or every artifact would "match" an empty + # string and the gate would pass on a tree with no version data at all + # (bonnyr-f5 #180 r3, BLOCKER 2). + if [ -z "$EXPECTED" ]; then + echo "::error::VERSION is empty — refusing to validate artifacts against nothing" >&2 + exit 1 + fi + rc=0; total=0 + # Assert EVERY version line is NON-EMPTY and equals VERSION, and that each + # artifact contributed at least one matched line. `total` counts MATCHED + # LINES, never loop iterations — an artifact whose key vanished contributes + # zero and both trips its own error and lowers the vacuity floor (bonnyr-f5 + # #180 r3: the old `checked` counted a literal 5-item list, so its >=5 guard + # was unreachable and --check was green on an empty tree). + _check_file() { # label, file, grep-ERE, extract-sed + local label="$1" file="$2" gre="$3" ext="$4" n=0 line val + while IFS= read -r line; do + val=$(sed -E "$ext" <<< "$line"); n=$((n + 1)); total=$((total + 1)) + if [ -z "$val" ]; then + echo "::error::$label in $file has an empty version — expected '$EXPECTED'"; rc=1 + elif [ "$val" != "$EXPECTED" ]; then + echo "::error::$label is '$val' but VERSION is '$EXPECTED' — the release publishes only :\${VERSION}, so a mismatch means ImagePullBackOff / drift. Run scripts/sync-version-artifacts.sh --write $EXPECTED"; rc=1 + else + echo " OK $label = $val" + fi + done < <(grep -E "$gre" "$file" || true) + if [ "$n" -eq 0 ]; then + echo "::error::$label: no version line matched in $file (key renamed/removed?) — vacuous check"; rc=1 fi - done - # bonnyr-f5 #180 r2 (INV-16): assert the gate actually compared something -- - # an empty list would exit 0 with real drift present. Also catch a drifted - # SECOND 2-space tag that grep -m1 above can't see. - while IFS= read -r t; do - t=$(sed -E 's/^ tag: "?([^"]*)"?.*/\1/' <<< "$t"); checked=$((checked+1)) - [ "$t" = "$EXPECTED" ] || { echo "::error::a 2-space 'tag:' reads '$t' but VERSION is '$EXPECTED'"; rc=1; } - done < <(grep -hE '^ tag: ' "$VALUES" "$OPVALUES") - [ "$checked" -ge 5 ] || { echo "::error::--check compared only $checked artifacts (expected >=5) — vacuous"; exit 1; } + } + _check_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" + _check_file "Chart appVersion" "$CHART" "$APPVER_RE" "$APPVER_SED" + _check_file "frontend version" "$PKG" "$PKGVER_RE" "$PKGVER_SED" + _check_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" + _check_file "operator appVersion" "$OPCHART" "$APPVER_RE" "$APPVER_SED" + # Backstop: five artifacts, each with >=1 version line, is the minimum a + # healthy tree yields. Fewer means a key vanished — treat as vacuous. + if [ "$total" -lt 5 ]; then + echo "::error::--check matched only $total version lines (expected >=5) — vacuous" >&2 + exit 1 + fi exit "$rc" ;; + + --list) + # Print the canonical artifact paths (repo-relative) so the release job stages + # EXACTLY what --write touches. Adding an artifact above updates all three. + for f in "${SYNCED_FILES[@]}"; do + printf '%s\n' "${f#"$ROOT"/}" + done + ;; + *) - echo "usage: sync-version-artifacts.sh --write | --check" >&2 + echo "usage: sync-version-artifacts.sh --write | --check | --list" >&2 exit 2 ;; esac From 88cca2b2adb07b4b52153c63d06217e18ee39425 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Fri, 21 Aug 2026 07:24:21 -0500 Subject: [PATCH 9/9] fix: scope the tag reader to the writer's image block; close release-staging vacuity gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review (bonnyr-f5 #180). F1 (Major) — writer/checker asymmetry on the image tag. The --write sed scopes its tag substitution to the top-level image: block (/^image:/,/^[^[:space:]]/), but both readers (--check and --write's post-write verify) grepped `^ tag:` file-global. The two site sets could diverge: a column-0 comment closing the block early gave a GREEN --check while the next --write HARD-FAILED the release; a stray 2-space tag: under another key gave a RED --check on a line --write can never fix. Fixed by lifting the range into one shared IMG_RANGE expression used by the writer's sed AND both readers (via a new _version_lines helper), so the tag reader sees exactly the site set the writer touches. Now symmetric: - col0-comment shape: BOTH fail (check vacuous-red, write no-op-red) - stray-tag shape: BOTH ignore the out-of-block tag (check green, write green) - drifted image tag: BOTH catch it (check red -> write fixes -> check green) F2 (Minor) — release.yml "not fully staged" guard is tautological (git diff after git add is always clean) and had no vacuity floor, so an empty --list would silently commit a bare VERSION bump with every image pin unsynced (BLOCKER-1 class). Added a `staged >= 5` floor to both Commit-and-tag steps, mirroring the script's own `total < 5` guard. F3 (Minor) — the CI version-consistency gate was unreachable from `make pre-push`, which ci.yml promises is CI-equivalent. Added a `version-check` target and pulled it into `quick-check` (a pre-push prerequisite). F4 (Minor) — the header's "the one place that writes them" over-claimed. Narrowed it: this owns the five release-train image-pin artifacts, NOT frontend-v2/ package-lock.json's root version (npm-owned, desyncs harmlessly) nor the dist/ doc copies (PR #183). Nits: validate the --write arg against [A-Za-z0-9._+-] (fail fast instead of corrupting sed); gitignore *.syncbak; correct the AGENTS.md claim that the release loop reads the skip marker on the subject line (its grep is line-oriented over the whole message). F1-b (cross-PR, documented not fixed): #182 carries this PR's first four commits (the pre-INV-19 66-line script); with squash enabled, squash-merging #180 first conflicts. Belongs to #182's merge order — land #180 first and rebase #182, or merge both as merge-commits. Left for #182. Reproduced red, fixed, mutation-tested green; shellcheck -S style clean; --check green on the real tree. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/release.yml | 26 +++++++++++- .gitignore | 4 ++ AGENTS.md | 4 +- Makefile | 14 ++++++- scripts/sync-version-artifacts.sh | 67 ++++++++++++++++++++++++------- 5 files changed, 96 insertions(+), 19 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 930203b8..e3a95c16 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -424,7 +424,18 @@ jobs: # own --list, so a newly-synced file can never be left unstaged and die # with the runner (bonnyr-f5 #180 r3, BLOCKER 1: --write rewrote five # files, the hard-coded `git add` staged three). - while IFS= read -r f; do git add "$f"; done < <(bash scripts/sync-version-artifacts.sh --list) + staged=0 + while IFS= read -r f; do git add "$f"; staged=$((staged + 1)); done < <(bash scripts/sync-version-artifacts.sh --list) + # Vacuity floor mirroring the script's own `--check` `total < 5` guard: + # if --list ever yields fewer paths (script broke / was truncated) the + # add + verify loops both go silent and we would commit a bare VERSION + # bump with every image pin left unsynced — the exact BLOCKER-1 failure + # the staging logic exists to prevent (bonnyr-f5 #180 r5, F2). The + # per-file "not fully staged" check below cannot catch this: it runs the + # same possibly-empty --list, so an empty list makes it vacuously pass. + if [ "$staged" -lt 5 ]; then + echo "::error::sync-version-artifacts.sh --list yielded only $staged path(s) (expected >=5) — refusing to commit an unsynced release"; exit 1 + fi # Verify the INDEX, not the files: --write's post-write check re-reads # the files (correct on disk even when unstaged), so assert each synced # artifact has no unstaged residue — i.e. the sync is actually in the @@ -570,7 +581,18 @@ jobs: # own --list, so a newly-synced file can never be left unstaged and die # with the runner (bonnyr-f5 #180 r3, BLOCKER 1: --write rewrote five # files, the hard-coded `git add` staged three). - while IFS= read -r f; do git add "$f"; done < <(bash scripts/sync-version-artifacts.sh --list) + staged=0 + while IFS= read -r f; do git add "$f"; staged=$((staged + 1)); done < <(bash scripts/sync-version-artifacts.sh --list) + # Vacuity floor mirroring the script's own `--check` `total < 5` guard: + # if --list ever yields fewer paths (script broke / was truncated) the + # add + verify loops both go silent and we would commit a bare VERSION + # bump with every image pin left unsynced — the exact BLOCKER-1 failure + # the staging logic exists to prevent (bonnyr-f5 #180 r5, F2). The + # per-file "not fully staged" check below cannot catch this: it runs the + # same possibly-empty --list, so an empty list makes it vacuously pass. + if [ "$staged" -lt 5 ]; then + echo "::error::sync-version-artifacts.sh --list yielded only $staged path(s) (expected >=5) — refusing to commit an unsynced release"; exit 1 + fi # Verify the INDEX, not the files: --write's post-write check re-reads # the files (correct on disk even when unstaged), so assert each synced # artifact has no unstaged residue — i.e. the sync is actually in the diff --git a/.gitignore b/.gitignore index 4959da45..82f0f7bd 100644 --- a/.gitignore +++ b/.gitignore @@ -278,3 +278,7 @@ next-session-prompt agent-selection handoffs/ *.code-workspace + +# Transient sed backup files from scripts/sync-version-artifacts.sh --write +# (removed on success; gitignored so an interrupted run leaves no tracked litter) +*.syncbak diff --git a/AGENTS.md b/AGENTS.md index 4ebf357c..9d3678ea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -94,7 +94,9 @@ major). One repo-specific trap worth stating outright: gates that got skipped (ShellCheck, Script Self-Tests, Secret Scan) were exactly the ones that mattered. Refer to it indirectly instead: "CI suppressed", "the skip-CI marker", or split it across backticks. The release job's *deliberate* skip is the - only legitimate use, and it lands on the subject line where the release loop reads it. + only legitimate use — and the release loop's own skip-detection grep is + line-oriented over the whole message (`^\[skip ci\]` / `\[skip ci\]$` anchored + per line), so it matches the marker on any line, not just the subject. --- diff --git a/Makefile b/Makefile index 75528feb..9aefe799 100644 --- a/Makefile +++ b/Makefile @@ -84,7 +84,7 @@ AWSBNKCTL_STAMP := bin/.awsbnkctl-$(AWSBNKCTL_VERSION).stamp test test-backend test-backend-unit test-backend-component test-backend-legacy test-frontend \ test-proxy test-operator test-db test-contracts test-e2e test-e2e-tier1 test-e2e-tier2 \ test-integration test-integration-full build-frontend-check smoke-mcp-live mcp-readiness mcp-recreate \ - lint lint-backend lint-frontend shellcheck coverage quick-check pre-push push install-hooks setup-hooks \ + lint lint-backend lint-frontend shellcheck coverage quick-check version-check pre-push push install-hooks setup-hooks \ dev-setup security-audit docker-check docker-verify docker-validate \ openapi openapi-types openapi-check openapi-types-check typecheck-backend typecheck-frontend \ build build-retry build-backend build-frontend build-worker build-agent build-all \ @@ -684,9 +684,19 @@ check-migrations: @echo "=== Migration Chain Validator ===" @python3 scripts/check-migrations.py +# ── Version-artifact consistency ───────────────────────────────────────────── +# Mirror of CI's "P1 · Version Consistency" job. ci.yml promises `make pre-push` +# ≡ CI, so the gate must be reachable from the documented local target or drift +# is undetectable until the release job dies (bonnyr-f5 #180 r5, F3). Pulled in +# by quick-check (a pre-push prerequisite). +version-check: + @echo "" + @echo "=== Version Artifact Consistency (Helm tag/appVersion, frontend, operator) ===" + @bash scripts/sync-version-artifacts.sh --check + # ── Quick check (~15s): lint + types + contracts ──────────────────────────── # Run before every commit. Catches most CI failures instantly. -quick-check: lint typecheck-backend openapi-types-check check-migrations +quick-check: lint typecheck-backend openapi-types-check check-migrations version-check @echo "" @echo "=========================================" @echo " Quick check passed (~15s)" diff --git a/scripts/sync-version-artifacts.sh b/scripts/sync-version-artifacts.sh index 79c8b0ed..ff27b040 100644 --- a/scripts/sync-version-artifacts.sh +++ b/scripts/sync-version-artifacts.sh @@ -7,8 +7,15 @@ # All five publish at :${VERSION} on the release train — docker-bake.hcl's # `default` group builds the operator image alongside the rest — so any drift # means an image tag the release never publishes -> ImagePullBackOff. This is the -# one place that writes them, and --check verifies them in CI so drift can't -# reappear silently. +# one place that writes THESE FIVE, and --check verifies them in CI so drift +# can't reappear silently. +# +# Scope, precisely: this owns the five release-train image-pin artifacts above — +# NOT every version string in the repo. Deliberately out of scope, and NOT +# claimed here: frontend-v2/package-lock.json's root `version` (npm owns it; it +# desyncs harmlessly — `npm ci` tolerates it), and the dist/ documentation +# copies (dist/.env.example, dist/README.md), which are packaged separately and +# tracked under PR #183. Don't read "the one place" as "every version site." # # Synced: the bnk-forge image tag + appVersion, the frontend package.json, AND # the bnk-operator image tag + appVersion. NOT synced, deliberately: each @@ -48,19 +55,49 @@ APPVER_SED='s/^appVersion: "?([^"]*)"?.*/\1/' PKGVER_RE='^ "version":' PKGVER_SED='s/^ "version": "([^"]*)".*/\1/' +# The image tag lives inside the top-level `image:` block. The WRITER scopes its +# substitution to that block (sed range below); the READERS (--check and --write's +# post-write verify) MUST use the SAME range, or writer and checker diverge: +# a `tag:` the writer can't reach (a column-0 comment closing the block early) or +# a stray 2-space `tag:` under another key would be read by a file-global checker +# but never written — CI green while the next release hard-fails, or CI red on a +# line --write can't fix (bonnyr-f5 #180 r5, F1). One expression, used by both. +IMG_RANGE='/^image:/,/^[^[:space:]]/' + +# Emit the candidate version lines for a reader. When RANGE is given, the grep is +# scoped to that sed address range (the image-tag case) so the reader sees EXACTLY +# the site set the writer's ranged sed touches; otherwise it is file-global. +_version_lines() { # file, grep-ERE, range(optional) + local file="$1" gre="$2" range="${3:-}" + if [ -n "$range" ]; then + sed -nE "${range}{/${gre}/p;}" "$file" + else + grep -E "$gre" "$file" || true + fi +} + case "${1:-}" in --write) V="${2:?usage: sync-version-artifacts.sh --write }" + # V is interpolated into sed replacement strings, so a `|`/`&`/`\`/`"` would + # corrupt the substitution. It only fails-closed at the post-write verify + # today (bonnyr-f5 #180 r5 nit) — reject metacharacters up front with a clear + # message. The class is permissive enough for full semver incl. prerelease and + # build metadata (e.g. 1.2.3-rc.1+build.5). + if ! printf '%s' "$V" | grep -qE '^[A-Za-z0-9._+-]+$'; then + echo "::error::--write version '$V' contains characters outside [A-Za-z0-9._+-] — refusing (would corrupt the sed substitution)" >&2 + exit 2 + fi # Anchor every substitution to its key PATH, not a bare 2-space `tag:`. The # image-tag writes are scoped to the top-level `image:` block via a sed range # (`/^image:/` to the next column-0 key) so a future unrelated 2-space `tag:` # elsewhere is never repinned to VERSION (bonnyr-f5 #180 r3, unbounded writer). # -i.syncbak (attached suffix) is the one in-place form both GNU and BSD sed # accept; `-i -E` makes BSD swallow -E as the suffix and litter *-E files. - sed -i.syncbak -E "/^image:/,/^[^[:space:]]/ s|^ tag: .*| tag: \"${V}\"|" "$VALUES" + sed -i.syncbak -E "${IMG_RANGE} s|^ tag: .*| tag: \"${V}\"|" "$VALUES" sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$CHART" sed -i.syncbak -E "s|^ \"version\": \"[^\"]*\"| \"version\": \"${V}\"|" "$PKG" - sed -i.syncbak -E "/^image:/,/^[^[:space:]]/ s|^ tag: .*| tag: \"${V}\"|" "$OPVALUES" + sed -i.syncbak -E "${IMG_RANGE} s|^ tag: .*| tag: \"${V}\"|" "$OPVALUES" sed -i.syncbak -E "s|^appVersion: .*|appVersion: \"${V}\"|" "$OPCHART" for f in "${SYNCED_FILES[@]}"; do rm -f "${f}.syncbak"; done @@ -70,24 +107,25 @@ case "${1:-}" in # uses and confirm each one actually took ${V} — and that at least one line # matched per artifact, so a renamed key can't pass as "nothing to change". rc=0 - _verify_file() { # label, file, grep-ERE, extract-sed - local label="$1" file="$2" gre="$3" ext="$4" n=0 line val + _verify_file() { # label, file, grep-ERE, extract-sed, range(optional) + local label="$1" file="$2" gre="$3" ext="$4" range="${5:-}" n=0 line val while IFS= read -r line; do val=$(sed -E "$ext" <<< "$line"); n=$((n + 1)) if [ "$val" != "$V" ]; then echo "::error::--write did not take on $label: it is '$val', expected '$V' (the sed pattern matched nothing — the artifact's format changed)" >&2 rc=1 fi - done < <(grep -E "$gre" "$file" || true) + done < <(_version_lines "$file" "$gre" "$range") if [ "$n" -eq 0 ]; then echo "::error::--write found no '$label' line in $file (key renamed/removed?) — nothing was synced" >&2 rc=1 fi } - _verify_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" + # range ↓ (tag only: same scope as the writer) + _verify_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" "$IMG_RANGE" _verify_file "Chart appVersion" "$CHART" "$APPVER_RE" "$APPVER_SED" _verify_file "frontend version" "$PKG" "$PKGVER_RE" "$PKGVER_SED" - _verify_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" + _verify_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" "$IMG_RANGE" _verify_file "operator appVersion" "$OPCHART" "$APPVER_RE" "$APPVER_SED" [ "$rc" -eq 0 ] || exit 1 echo "synced bnk-forge tag+appVersion, frontend package.json, operator tag+appVersion -> ${V}" >&2 @@ -109,8 +147,8 @@ case "${1:-}" in # zero and both trips its own error and lowers the vacuity floor (bonnyr-f5 # #180 r3: the old `checked` counted a literal 5-item list, so its >=5 guard # was unreachable and --check was green on an empty tree). - _check_file() { # label, file, grep-ERE, extract-sed - local label="$1" file="$2" gre="$3" ext="$4" n=0 line val + _check_file() { # label, file, grep-ERE, extract-sed, range(optional) + local label="$1" file="$2" gre="$3" ext="$4" range="${5:-}" n=0 line val while IFS= read -r line; do val=$(sed -E "$ext" <<< "$line"); n=$((n + 1)); total=$((total + 1)) if [ -z "$val" ]; then @@ -120,15 +158,16 @@ case "${1:-}" in else echo " OK $label = $val" fi - done < <(grep -E "$gre" "$file" || true) + done < <(_version_lines "$file" "$gre" "$range") if [ "$n" -eq 0 ]; then echo "::error::$label: no version line matched in $file (key renamed/removed?) — vacuous check"; rc=1 fi } - _check_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" + # range ↓ (tag only: same scope as the writer) + _check_file "helm image.tag" "$VALUES" "$TAG_RE" "$TAG_SED" "$IMG_RANGE" _check_file "Chart appVersion" "$CHART" "$APPVER_RE" "$APPVER_SED" _check_file "frontend version" "$PKG" "$PKGVER_RE" "$PKGVER_SED" - _check_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" + _check_file "operator image.tag" "$OPVALUES" "$TAG_RE" "$TAG_SED" "$IMG_RANGE" _check_file "operator appVersion" "$OPCHART" "$APPVER_RE" "$APPVER_SED" # Backstop: five artifacts, each with >=1 version line, is the minimum a # healthy tree yields. Fewer means a key vanished — treat as vacuous.