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..e3a95c16 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,32 @@ 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 + # 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). + 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 + # 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] @@ -515,6 +543,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 +577,29 @@ jobs: - name: Commit and tag run: | git add VERSION CHANGELOG.md + # 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). + 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 + # 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/.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 c2289cd1..9d3678ea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,5 +82,21 @@ 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 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/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/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/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..ff27b040 --- /dev/null +++ b/scripts/sync-version-artifacts.sh @@ -0,0 +1,193 @@ +#!/usr/bin/env bash +# 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` +# +# 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 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 +# 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" +# 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" + +# 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/' + +# 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 "${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 "${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 + + # Fail closed: a sed whose pattern matched nothing no-ops silently, and the + # 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 + _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 < <(_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 + } + # 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" "$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 + ;; + + --check) + EXPECTED="$(cat "$ROOT/VERSION")" + # 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, 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 + 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 < <(_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 + } + # 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" "$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. + 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 | --list" >&2 + exit 2 + ;; +esac