From 18bd737c9bed3e2eeda21dfd669575358f403436 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 22:11:01 -0500 Subject: [PATCH 01/17] 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 5d8f62e..1aa8b4a 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 0ded64e..9b0facf 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 4e25c0c..7282abe 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 950b44a..40db248 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 bb88060..97b654d 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 0000000..b02ceb0 --- /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 02/17] 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 39531c5..b5b8f84 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 97b654d..427999a 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 03/17] 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 b5b8f84..39531c5 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 427999a..97b654d 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 b02ceb0..f787b67 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 04/17] 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 c2289cd..4ebf357 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 ca6fe893e4cf0525d992c9a959f9477edfa59f63 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 22:24:17 -0500 Subject: [PATCH 05/17] ci: wire the gitleaks, shellcheck, and script self-test gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #177 review (bonnyr-f5) — Major. `.gitleaks.toml` was a config nothing ran, `make shellcheck` had no caller, and compute_version_bump.sh's SELF_TEST was never invoked — so the release-critical shell scripts were statically unchecked and, for a public repo, the secret-scanning gate was unwired. Three new P1 jobs, all wired into the CI gate: - Secret Scan (gitleaks): `--no-git` over the tracked source with the repo's config. Extended the allowlist to cover generated build artifacts (.pyc, frontend-v2/dist) alongside the existing synthetic-key entries; a fresh checkout now scans clean (verified locally even with artifacts present). - ShellCheck: runs `make shellcheck` over the whole script corpus. Fixed the two pre-existing findings that would have blocked the gate — a missing shebang in get_dpu_pwd.sh (SC2148) and unused `for i` loop counters in ibm_cloud_bnk_forge.sh (SC2034, now `for _`). Corpus is clean at --severity=warning. - Script Self-Tests: runs `SELF_TEST=1 compute_version_bump.sh`, which now exits non-zero on failure (that change ships with the SIGPIPE-race PR), so a regression in the logic that decides the released version fails CI. The release-critical scripts (compute_version_bump, extract-breaking-changes, publish-signed-images) already pass shellcheck cleanly. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 40 ++++++++++++++++++++++++++++++++++ .gitleaks.toml | 5 +++++ scripts/get_dpu_pwd.sh | 1 + scripts/ibm_cloud_bnk_forge.sh | 6 ++--- 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1aa8b4a..e506c08 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,6 +139,40 @@ jobs: # / silent drift (#177 Blocker 2). run: bash scripts/sync-version-artifacts.sh --check + shellcheck: + name: "P1 · ShellCheck" + needs: changes + if: always() + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Install shellcheck + run: sudo apt-get update && sudo apt-get install -y shellcheck + - name: Run shellcheck + run: make shellcheck + + secret-scan: + name: "P1 · Secret Scan (gitleaks)" + needs: changes + if: always() + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: gitleaks + run: | + docker run --rm -v "$PWD:/repo" -w /repo ghcr.io/gitleaks/gitleaks:latest detect \ + --source=/repo --config=/repo/.gitleaks.toml --no-git --redact --verbose + + script-selftests: + name: "P1 · Script Self-Tests" + needs: changes + if: always() + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: compute_version_bump SELF_TEST + run: SELF_TEST=1 bash scripts/compute_version_bump.sh + lint-frontend: name: "P1 · Lint Frontend" needs: changes @@ -1077,6 +1111,9 @@ jobs: # Phase 1 - lint-backend - version-consistency + - shellcheck + - secret-scan + - script-selftests - lint-frontend - typecheck-backend - openapi-check @@ -1111,6 +1148,9 @@ jobs: for job in \ "lint-backend:${{ needs.lint-backend.result }}" \ "version-consistency:${{ needs.version-consistency.result }}" \ + "shellcheck:${{ needs.shellcheck.result }}" \ + "secret-scan:${{ needs.secret-scan.result }}" \ + "script-selftests:${{ needs.script-selftests.result }}" \ "lint-frontend:${{ needs.lint-frontend.result }}" \ "typecheck-backend:${{ needs.typecheck-backend.result }}" \ "openapi-check:${{ needs.openapi-check.result }}" \ diff --git a/.gitleaks.toml b/.gitleaks.toml index d6b977b..44e32cf 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -46,4 +46,9 @@ paths = [ '''backend/tests/component/test_agent_host_candidates\.py''', '''backend/tests/integration/test_routes_project_secrets\.py''', '''backend/tests/unit/test_infrastructure_access_service\.py''', + # Generated build artifacts, never source. The .py sources above are scanned; + # their compiled .pyc and the built frontend bundle are not credentials. + '''.*__pycache__/.*''', + '''.*\.pyc$''', + '''frontend-v2/dist/.*''', ] diff --git a/scripts/get_dpu_pwd.sh b/scripts/get_dpu_pwd.sh index 779352a..d6f0271 100644 --- a/scripts/get_dpu_pwd.sh +++ b/scripts/get_dpu_pwd.sh @@ -1,3 +1,4 @@ +#!/usr/bin/env bash docker compose exec -it backend python -c " from database import SessionLocal from models.bare_metal import BareMetalHost diff --git a/scripts/ibm_cloud_bnk_forge.sh b/scripts/ibm_cloud_bnk_forge.sh index 39d4ff5..e781fa8 100644 --- a/scripts/ibm_cloud_bnk_forge.sh +++ b/scripts/ibm_cloud_bnk_forge.sh @@ -343,7 +343,7 @@ done docker compose up -d # 9. Wait for backend health then drop a ready marker -for i in $(seq 1 60); do +for _ in $(seq 1 60); do curl -sf http://localhost:8000/api/system/health >/dev/null 2>&1 && break || sleep 5 done touch /opt/bnk-forge/.bnk-forge-ready @@ -650,7 +650,7 @@ VM_ID="$(echo "${INST_JSON}" | jq -r '.id')" [ -n "${VM_ID}" ] && [ "${VM_ID}" != "null" ] || die "Instance creation failed." log "Waiting for the VSI to reach 'running'..." -for i in $(seq 1 60); do +for _ in $(seq 1 60); do ST="$(ibmcloud is instance "${VM_ID}" --output json | jq -r '.status')" [ "${ST}" = "running" ] && break [ "${ST}" = "failed" ] && die "Instance entered 'failed' state." @@ -684,7 +684,7 @@ log "Floating IP: ${FIP}" URL="https://${FIP}" log "Installing bnk-forge on the VSI (this can take 5–10 minutes)..." READY=0 -for i in $(seq 1 90); do +for _ in $(seq 1 90); do CODE="$(curl -sk -o /dev/null -w '%{http_code}' --connect-timeout 5 "${URL}/api/system/health" 2>/dev/null || true)" if [ "${CODE}" = "200" ]; then READY=1; break; fi sleep 10 From d51d18a167433cc8cb1af4d072f28110a058e9c6 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 23:07:30 -0500 Subject: [PATCH 06/17] review fix: don't allowlist frontend-v2/dist; pin the gitleaks image mwiget's re-review of #182. Removed the `frontend-v2/dist/.*` allowlist entry. A built JS bundle is exactly where VITE_*-style build-time secret injection lands, so it's the highest-value thing to scan, not to skip; a blanket directory entry in the top-level [allowlist] also suppresses every rule (private-key, cloud creds), not just generic-api-key -- which the config's own header warns against. And it bought nothing in CI: dist is gitignored, so a fresh checkout has no tracked files there anyway. The .pyc / __pycache__ entries stay (compiled bytecode of sources that are themselves scanned) with the "reviewed, here's why" framing the other entries carry. Pinned the gitleaks image to v8.21.2 (was :latest) -- a security gate shouldn't float its scanner version. The script-selftests job becomes meaningful once #179 lands: its rebuilt Test 7 catches the SIGPIPE regression on CI's GNU grep (the old single-line Test 7 passed even with the fix reverted). Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 2 +- .gitleaks.toml | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e506c08..fe6fbbe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -160,7 +160,7 @@ jobs: - uses: actions/checkout@v6 - name: gitleaks run: | - docker run --rm -v "$PWD:/repo" -w /repo ghcr.io/gitleaks/gitleaks:latest detect \ + docker run --rm -v "$PWD:/repo" -w /repo ghcr.io/gitleaks/gitleaks:v8.21.2 detect \ --source=/repo --config=/repo/.gitleaks.toml --no-git --redact --verbose script-selftests: diff --git a/.gitleaks.toml b/.gitleaks.toml index 44e32cf..608deeb 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -46,9 +46,12 @@ paths = [ '''backend/tests/component/test_agent_host_candidates\.py''', '''backend/tests/integration/test_routes_project_secrets\.py''', '''backend/tests/unit/test_infrastructure_access_service\.py''', - # Generated build artifacts, never source. The .py sources above are scanned; - # their compiled .pyc and the built frontend bundle are not credentials. + # Compiled bytecode of the sources above, which ARE scanned. Not a blanket + # directory skip of hand-written code, and it buys a clean local run without + # blinding CI (these are gitignored -- zero tracked files -- so a fresh + # checkout has none). frontend-v2/dist is deliberately NOT allowlisted: a + # built JS bundle is where VITE_*-style build-time secret injection lands, so + # it is the highest-value thing to scan, not to skip (#182 review). '''.*__pycache__/.*''', '''.*\.pyc$''', - '''frontend-v2/dist/.*''', ] From 1c271fec427ea1c42356c5337315a3406e03a5ed Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 23:26:55 -0500 Subject: [PATCH 07/17] fix: catalogue the 13 private-key false positives the gitleaks gate surfaced The gitleaks gate this PR adds went red on its own first run: enforcing the scan surfaced 13 `private-key` matches never catalogued before, because gitleaks had never actually run in CI. All 13 are PEM headers around a placeholder body, verified individually: - the textarea hint "-----BEGIN OPENSSH PRIVATE KEY-----\n...\n-----END..." in the SSH key-input UI (SSHCredentials, DpuFormDialog, RshimInstallDialog, DpuProjectSettingsCard, NodeDiscoveryPanel), and - synthetic keys / "..." / "SECRETCONTENT" in six backend SSH tests. Added as specific-path entries to the existing allowlist, matching the convention the config already uses for the other PEM-placeholder test files. (`[[allowlists]]` with `targetRules` would scope these to the private-key rule only, but this gitleaks version ignores the plural block when the singular `[allowlist]` is present.) Verified: a clean tracked-only checkout scans with no leaks found. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .gitleaks.toml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.gitleaks.toml b/.gitleaks.toml index 608deeb..1f83cc2 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -54,4 +54,21 @@ paths = [ # it is the highest-value thing to scan, not to skip (#182 review). '''.*__pycache__/.*''', '''.*\.pyc$''', + # gitleaks was never enforced in CI before, so these private-key false + # positives were never catalogued. All are PEM headers wrapped around a + # placeholder body -- synthetic keys / "..." / "SECRETCONTENT" in tests, and + # the textarea hint "-----BEGIN OPENSSH PRIVATE KEY-----\n...\n-----END..." + # in the key-input UI. Verified 2026-08: no live key material. + '''backend/tests/component/test_ssh_credential_service\.py''', + '''backend/tests/component/test_ssh_service\.py''', + '''backend/tests/integration/test_routes_discovery\.py''', + '''backend/tests/integration/test_routes_ssh_credentials\.py''', + '''backend/tests/unit/test_proxy_translate_cis_service\.py''', + '''backend/tests/unit/test_tmos_engine\.py''', + '''frontend-v2/src/components/settings/SSHCredentials\.tsx''', + '''frontend-v2/src/components/settings/__tests__/SSHCredentials\.test\.tsx''', + '''frontend-v2/src/components/dpu/DpuFormDialog\.tsx''', + '''frontend-v2/src/components/dpu/RshimInstallDialog\.tsx''', + '''frontend-v2/src/components/dpu/DpuProjectSettingsCard\.tsx''', + '''frontend-v2/src/components/discovery/NodeDiscoveryPanel\.tsx''', ] From 06af757de30740d65b8a426a90ef94e0b4a98682 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 23:32:15 -0500 Subject: [PATCH 08/17] review fix: pin gitleaks to v8.30.1 (precise private-key rule); read-only mount mwiget's data was the key: v8.21.2 flags 13 private-key false positives on these PEM-header placeholders, but v8.30.1 flags 0 -- the rule was tightened across those minors. So the right fix isn't to catalogue the false positives against an old scanner; it's to pin the scanner that gets it right. Repinned :v8.21.2 -> :v8.30.1 and dropped the 13 allowlist entries I'd added, which over-suppressed production key-input components (all rules, not just private-key) for no benefit under v8.30.1. Verified: clean tracked-only checkout scans no leaks found. Also from mwiget's nits: the mount is now read-only (-v "$PWD:/repo:ro" -- detect only reads). `--redact` was already deliberate (redacted value + file + line), no change. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 2 +- .gitleaks.toml | 17 ----------------- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe6fbbe..53bedd0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -160,7 +160,7 @@ jobs: - uses: actions/checkout@v6 - name: gitleaks run: | - docker run --rm -v "$PWD:/repo" -w /repo ghcr.io/gitleaks/gitleaks:v8.21.2 detect \ + docker run --rm -v "$PWD:/repo:ro" -w /repo ghcr.io/gitleaks/gitleaks:v8.30.1 detect \ --source=/repo --config=/repo/.gitleaks.toml --no-git --redact --verbose script-selftests: diff --git a/.gitleaks.toml b/.gitleaks.toml index 1f83cc2..608deeb 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -54,21 +54,4 @@ paths = [ # it is the highest-value thing to scan, not to skip (#182 review). '''.*__pycache__/.*''', '''.*\.pyc$''', - # gitleaks was never enforced in CI before, so these private-key false - # positives were never catalogued. All are PEM headers wrapped around a - # placeholder body -- synthetic keys / "..." / "SECRETCONTENT" in tests, and - # the textarea hint "-----BEGIN OPENSSH PRIVATE KEY-----\n...\n-----END..." - # in the key-input UI. Verified 2026-08: no live key material. - '''backend/tests/component/test_ssh_credential_service\.py''', - '''backend/tests/component/test_ssh_service\.py''', - '''backend/tests/integration/test_routes_discovery\.py''', - '''backend/tests/integration/test_routes_ssh_credentials\.py''', - '''backend/tests/unit/test_proxy_translate_cis_service\.py''', - '''backend/tests/unit/test_tmos_engine\.py''', - '''frontend-v2/src/components/settings/SSHCredentials\.tsx''', - '''frontend-v2/src/components/settings/__tests__/SSHCredentials\.test\.tsx''', - '''frontend-v2/src/components/dpu/DpuFormDialog\.tsx''', - '''frontend-v2/src/components/dpu/RshimInstallDialog\.tsx''', - '''frontend-v2/src/components/dpu/DpuProjectSettingsCard\.tsx''', - '''frontend-v2/src/components/discovery/NodeDiscoveryPanel\.tsx''', ] From b12384b229a68ffb0a60f3cd61d7345493f66971 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 01:58:45 -0500 Subject: [PATCH 09/17] fix: secret-scan every change; make the self-test job able to fail bonnyrf5 aggregate review, #182. paths-ignore skipped secret scanning on doc-only PRs (ci.yml:24-49): gitleaks sits behind the workflow-level paths-ignore, so a doc-only PR skipped the ENTIRE workflow and never ran it -- in a public repo, where a secret lands in a .md as easily as in code. secret-scan already carries `if: always()`, but that can't override a workflow-level skip. Removed both paths-ignore blocks so secret-scan and the CI Gate see every change; expensive jobs still skip on irrelevant paths through the per-job `changes` filter, so path filtering now has one home. script-selftests couldn't fail (ci.yml:154-162): the job trusted the self-test's exit code, but the harness printed FAIL: while exiting 0, so it could not catch a broken self-test until #179's exit-code fix landed. The job now fails on a non-zero exit OR a FAIL: line, so it's effective on its own. Verified the grep catches a FAIL: line even at exit 0 and does not flag PASS output. YAML valid; actionlint clean on the changed regions. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53bedd0..9a04463 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,29 +24,16 @@ name: CI on: pull_request: branches: [main, staging, develop] - paths-ignore: - - '**.md' - - 'docs/**' - - '.agent/**' - - '.opencode/**' - - 'LICENSE' - - '.gitignore' - - '.trivyignore' - - 'USER_GUIDE.md' + # No workflow-level paths-ignore: secret scanning (gitleaks) and the CI Gate + # must see EVERY change, doc-only PRs included — a secret lands in a .md as + # easily as in code, and this is a public repo (#182 review). Expensive jobs + # still skip on irrelevant paths via the per-job `changes` filter below; path + # filtering lives there (one source of truth), not at the trigger. push: # main (deploy trigger) + staging (release-automation preflight needs a # push-triggered CI run to match by SHA — see release.yml preflight); - # develop skipped. + # develop skipped. No paths-ignore, same reason as above. branches: [main, staging] - paths-ignore: - - '**.md' - - 'docs/**' - - '.agent/**' - - '.opencode/**' - - 'LICENSE' - - '.gitignore' - - '.trivyignore' - - 'USER_GUIDE.md' concurrency: group: ci-${{ github.ref }} @@ -171,7 +158,20 @@ jobs: steps: - uses: actions/checkout@v6 - name: compute_version_bump SELF_TEST - run: SELF_TEST=1 bash scripts/compute_version_bump.sh + run: | + # Fail on a non-zero exit OR a FAIL: line. The harness historically + # printed FAIL: but still exited 0, so trusting the exit code alone + # made this job unable to catch a broken self-test until the exit-code + # fix landed (#182 review). Checking both decouples the two. + set +e + out="$(SELF_TEST=1 bash scripts/compute_version_bump.sh 2>&1)"; rc=$? + echo "$out" + if [ "$rc" -ne 0 ]; then + echo "::error::compute_version_bump self-test exited $rc"; exit "$rc" + fi + if grep -qE '(^|[[:space:]])FAIL:' <<< "$out"; then + echo "::error::compute_version_bump self-test reported FAIL: but exited 0"; exit 1 + fi lint-frontend: name: "P1 · Lint Frontend" From f13d2fc5c2c586e3fd7cc42d8ab26452e833edc4 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 19:25:45 -0500 Subject: [PATCH 10/17] fix: stop releases being starved by cancelled CI; scope gitleaks; gate self-test; full shellcheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bonnyr-f5 BLOCK review of #182. All findings reproduced and confirmed. BLOCKER — the trigger/concurrency divergence silently dropped releases. This PR removes ci.yml's push paths-ignore (correct — secret-scan must see docs), but release.yml keeps its own, and ci.yml ran cancel-in-progress: true. So a docs-only push cancelled the in-flight CI run an earlier commit's release polls by SHA, while triggering no replacement Release -> that release exited 1 "superseded" and never ran. Fix per your shape (don't restore paths-ignore): CI no longer cancels in-progress runs on main/staging, so a release's CI run can't be starved; feature branches still cancel. MAJOR — the top-level [allowlist].paths blanket-disabled EVERY rule for the four fixture files (a real AWS/GitHub key there was invisible), against the file's own header. Scoped the four files to the private-key rule instead, and dropped the .pyc/__pycache__ blanket (zero tracked files). Verified on a git-archive tree (== CI checkout): no leaks found, and a planted AKIA key in a scoped file is still caught (1 leak). MAJOR — the self-test job passed on silence (only checked for a FAIL: line); renaming the harness guard produced empty output + rc=0. Now also requires a PASS: line, so a harness that didn't run fails the job. MAJOR — make shellcheck covered 23/37 scripts (missed dist/install.sh et al.). Driven from `git ls-files '*.sh'` now; the full corpus is clean at --severity=warning. Acknowledged (documented): --no-git scans tree not history (defense-in-depth follow-up: fetch-depth:0 + --log-opts); the explicit paths-ignore-subset assertion. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 12 +++++++++++- .gitleaks.toml | 23 ++++++++--------------- Makefile | 4 +++- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9a04463..a47fd23 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,11 @@ on: concurrency: group: ci-${{ github.ref }} - cancel-in-progress: true + # bonnyr-f5 #182: never cancel an in-flight CI run on the release branches -- + # release.yml preflight polls that exact run by SHA, so a docs-only push (which + # triggers CI but not Release) would otherwise cancel it and strand the earlier + # commit's release. Feature branches still cancel to save minutes. + cancel-in-progress: ${{ github.ref_name != 'main' && github.ref_name != 'staging' }} permissions: contents: read @@ -172,6 +176,12 @@ jobs: if grep -qE '(^|[[:space:]])FAIL:' <<< "$out"; then echo "::error::compute_version_bump self-test reported FAIL: but exited 0"; exit 1 fi + # bonnyr-f5 #182: silence must not pass -- require positive evidence the + # harness actually ran (renaming its SELF_TEST guard produced empty + # output + rc=0, i.e. green with zero assertions). + if ! grep -qE '(^|[[:space:]])PASS:' <<< "$out"; then + echo "::error::self-test produced no PASS lines -- the harness did not run"; exit 1 + fi lint-frontend: name: "P1 · Lint Frontend" diff --git a/.gitleaks.toml b/.gitleaks.toml index 608deeb..bbe27e2 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -34,24 +34,17 @@ regexes = [ # "signature_here"; used to test _looks_like_jwt(). '''eyJhbGciOiJIUzI1NiJ9\.eyJzdWIiOiJ0ZXN0In0\.signature_here''', ] + +[[rules]] +id = "private-key" +# bonnyr-f5 #182: scope the fixture private keys to the private-key rule only, so +# a real AWS/GitHub/generic secret hidden in these same files is still caught -- +# a top-level [allowlist].paths would have disabled EVERY rule for them. The +# .pyc/__pycache__ blanket entries were dropped (zero tracked files). +[rules.allowlist] paths = [ - # Throwaway RSA/Ed25519 keypairs generated solely to exercise paramiko key - # parsing. They authenticate nothing — no corresponding public key is - # deployed anywhere. See the note in the file header. '''backend/tests/unit/test_paramiko_utils\.py''', - # PEM headers wrapped around placeholder bodies, not key material: - # test_agent_host_candidates.py -> "MIIEowIBAAKCAQEA000000..." - # test_routes_project_secrets.py -> "fake" - # test_infrastructure_access_service.py-> "MIIEowIBAAKCAQEAuTestKeyMaterial" '''backend/tests/component/test_agent_host_candidates\.py''', '''backend/tests/integration/test_routes_project_secrets\.py''', '''backend/tests/unit/test_infrastructure_access_service\.py''', - # Compiled bytecode of the sources above, which ARE scanned. Not a blanket - # directory skip of hand-written code, and it buys a clean local run without - # blinding CI (these are gitignored -- zero tracked files -- so a fresh - # checkout has none). frontend-v2/dist is deliberately NOT allowlisted: a - # built JS bundle is where VITE_*-style build-time secret injection lands, so - # it is the highest-value thing to scan, not to skip (#182 review). - '''.*__pycache__/.*''', - '''.*\.pyc$''', ] diff --git a/Makefile b/Makefile index 75528fe..f935175 100644 --- a/Makefile +++ b/Makefile @@ -464,7 +464,9 @@ test-upgrade: shellcheck: @echo "" @echo "=== ShellCheck: linting shell scripts ===" - @shellcheck --severity=warning upgrade.sh scripts/*.sh vm-bnk-forge/*.sh vm-bnk-forge/lib/*.sh + @# bonnyr-f5 #182: drive from git ls-files so the WHOLE corpus is gated + @# (the hardcoded globs missed 14 tracked scripts incl. dist/install.sh). + @git ls-files '*.sh' | xargs shellcheck --severity=warning # Convenience: start/stop/restart all (platform-aware) up: ensure-artifact-network From cc9a99ad1a5866b0cc5c832306c02fc26a6de9e1 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 21:09:23 -0500 Subject: [PATCH 11/17] fix: gitleaks scans the commit range; per-SHA concurrency; gate rejects skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/ci.yml | 39 ++++++++++++++++++++++++++++++++++++--- Makefile | 7 ++++++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a47fd23..8b3f776 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,7 +36,11 @@ on: branches: [main, staging] concurrency: - group: ci-${{ github.ref }} + # bonnyr-f5 #182 r2: on main/staging give every push its OWN group (append the + # SHA) so a later docs-only push can't cancel -- even as a PENDING run -- the CI + # run a release polls by SHA. Feature branches keep the per-ref group so rapid + # pushes still supersede each other and save minutes. + group: ci-${{ github.ref }}${{ (github.ref_name == 'main' || github.ref_name == 'staging') && github.sha || '' }} # bonnyr-f5 #182: never cancel an in-flight CI run on the release branches -- # release.yml preflight polls that exact run by SHA, so a docs-only push (which # triggers CI but not Release) would otherwise cancel it and strand the earlier @@ -149,10 +153,25 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + fetch-depth: 0 # full history so the range scan sees add-then-remove - name: gitleaks run: | + # bonnyr-f5 #182 r2: scan the PR/push COMMIT RANGE in git mode, not the + # working tree. --no-git misses a secret added then REMOVED within the + # branch, which stays fetchable forever from a public clone -- the main + # thing a public repo needs a history-aware scan for. + if [ "${{ github.event_name }}" = "pull_request" ]; then + RANGE="${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}" + elif [ -n "${{ github.event.before }}" ] && [ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then + RANGE="${{ github.event.before }}..${{ github.sha }}" + else + RANGE="" # first push / no base — scan all reachable history + fi + echo "gitleaks scanning range: ${RANGE:-}" docker run --rm -v "$PWD:/repo:ro" -w /repo ghcr.io/gitleaks/gitleaks:v8.30.1 detect \ - --source=/repo --config=/repo/.gitleaks.toml --no-git --redact --verbose + --source=/repo --config=/repo/.gitleaks.toml --redact --verbose \ + ${RANGE:+--log-opts=$RANGE} script-selftests: name: "P1 · Script Self-Tests" @@ -179,9 +198,15 @@ jobs: # bonnyr-f5 #182: silence must not pass -- require positive evidence the # harness actually ran (renaming its SELF_TEST guard produced empty # output + rc=0, i.e. green with zero assertions). + # bonnyr-f5 #182 r2: require the END marker AND >=1 PASS. The marker + # prints only after the LAST assertion, so an early exit (the #179 shape, + # 5 of 6 unrun) is caught without hardcoding a per-branch test count. if ! grep -qE '(^|[[:space:]])PASS:' <<< "$out"; then echo "::error::self-test produced no PASS lines -- the harness did not run"; exit 1 fi + if ! grep -qE '=== END SELF-TEST ===' <<< "$out"; then + echo "::error::self-test did not reach its END marker -- it exited early with assertions unrun"; exit 1 + fi lint-frontend: name: "P1 · Lint Frontend" @@ -1155,6 +1180,10 @@ jobs: # Collect all job results (skipped jobs are OK — they were filtered by path) failed=false + # bonnyr-f5 #182 r2: these gates run `if: always()` on every change, so + # `skipped` for them means a future path-filter silently disabled the + # check. Treat skipped as a failure for exactly these. + ALWAYS_RUN="version-consistency shellcheck secret-scan script-selftests" for job in \ "lint-backend:${{ needs.lint-backend.result }}" \ "version-consistency:${{ needs.version-consistency.result }}" \ @@ -1184,10 +1213,14 @@ jobs: ; do name="${job%%:*}" result="${job##*:}" - # 'success' and 'skipped' are both acceptable + # 'success' and 'skipped' are acceptable, EXCEPT skipped for an + # always-run gate, which means the check silently didn't execute. if [ "$result" = "failure" ] || [ "$result" = "cancelled" ]; then echo "::error::$name: $result" failed=true + elif [ "$result" = "skipped" ] && case " $ALWAYS_RUN " in *" $name "*) true;; *) false;; esac; then + echo "::error::$name was SKIPPED but is an always-run gate — the check never executed" + failed=true else echo "$name: $result" fi diff --git a/Makefile b/Makefile index f935175..5f19ffc 100644 --- a/Makefile +++ b/Makefile @@ -466,7 +466,12 @@ shellcheck: @echo "=== ShellCheck: linting shell scripts ===" @# bonnyr-f5 #182: drive from git ls-files so the WHOLE corpus is gated @# (the hardcoded globs missed 14 tracked scripts incl. dist/install.sh). - @git ls-files '*.sh' | xargs shellcheck --severity=warning + @# bonnyr-f5 #182 r2: include the (extensionless) git hooks, and fail on an + @# EMPTY list -- `xargs shellcheck` with no files exits 0 on BSD (blind). + @files="$$(git ls-files '*.sh' .githooks/pre-commit .githooks/pre-push 2>/dev/null)"; \ + n=$$(printf '%s\n' "$$files" | grep -c .); \ + [ "$$n" -ge 1 ] || { echo "::error::shellcheck found no files to lint"; exit 1; }; \ + printf '%s\n' "$$files" | xargs shellcheck --severity=warning # Convenience: start/stop/restart all (platform-aware) up: ensure-artifact-network From 83982af4fab211c24fea9f7aabd184f9c974e30a Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 22:55:25 -0500 Subject: [PATCH 12/17] ci: assert the BREAKING CHANGE detector is identical across both scripts bonnyr-f5 #179 r3 (cross-PR / INV-15). The two _is_breaking detectors in compute_version_bump.sh and extract-breaking-changes.sh must stay byte-identical -- if they drift, a major bump ships with empty notes, or a note ships with no bump. Until now only a "MUST stay identical" comment enforced that. The script-selftests job now extracts the `grep -qE '...BREAKING...CHANGE...'` detector regex from each script and asserts they match, and runs the extractor's own --self-test. The extraction is version-agnostic: it pulls whichever detector form each script uses, so parity is enforced whether the tree is pre- or post-#179 (verified: passes on both the current old form and #179's anchored form, and correctly fails on a constructed drift between them). That keeps the gate meaningful on this PR today rather than only after the stack merges. The extractor --self-test lands with #179 (it adds the flag). Until #179 is in staging this PR's base carries the older extractor, so the step warns loudly (::warning::, not a silent skip) that the self-test activates on merge while the parity assertion still runs this build. Verified: YAML + actionlint clean; parity extraction tested across old/new/drift. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8b3f776..6d34640 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -207,6 +207,44 @@ jobs: if ! grep -qE '=== END SELF-TEST ===' <<< "$out"; then echo "::error::self-test did not reach its END marker -- it exited early with assertions unrun"; exit 1 fi + - name: BREAKING CHANGE detector parity + extractor self-test + run: | + # bonnyr-f5 #179 r3 (cross-PR / INV-15): the BREAKING CHANGE detector + # MUST be byte-identical between the two scripts -- if they drift, a + # major bump ships with empty notes (or a note ships with no bump). This + # asserts identity in code, replacing the "MUST stay identical" comment. + # + # The extraction is version-agnostic on purpose: it pulls the regex out + # of whichever `grep -qE '...BREAKING...CHANGE...'` detector each script + # uses, so it enforces parity whether the tree is pre- or post-#179 + # (both scripts use the SAME form as each other in either state). That + # keeps this gate meaningful on this PR today AND on the merged stack. + set -euo pipefail + _regex() { + grep -oE "grep -qE '[^']*BREAKING[^']*CHANGE[^']*'" "$1" \ + | sed -E "s/^grep -qE '//; s/'\$//" | sort -u + } + ex="$(_regex scripts/extract-breaking-changes.sh)" + cv="$(_regex scripts/compute_version_bump.sh)" + if [ -z "$ex" ] || [ -z "$cv" ]; then + echo "::error::could not extract a BREAKING CHANGE detector regex from one of the scripts (extract='$ex' compute='$cv')"; exit 1 + fi + if [ "$ex" != "$cv" ]; then + echo "::error::INV-15 violated: the BREAKING CHANGE detector differs between the two scripts" + echo " extract-breaking-changes.sh: $ex" + echo " compute_version_bump.sh: $cv" + exit 1 + fi + echo "INV-15 OK: BREAKING CHANGE detector identical across both scripts -> $ex" + # Run the extractor's own self-test once #179's anchored extractor (which + # adds --self-test) is in the tree. Until #179 merges to staging, this + # PR's base carries the older extractor; warn LOUDLY (not a silent skip) + # so the pending activation is visible in the log. + if grep -q -- '--self-test' scripts/extract-breaking-changes.sh; then + bash scripts/extract-breaking-changes.sh --self-test + else + echo "::warning::extract-breaking-changes.sh has no --self-test yet (it lands with #179); the parity gate above is still enforced this run" + fi lint-frontend: name: "P1 · Lint Frontend" From d357e403e058e963296c39093bb54ba5164532be Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 23:21:43 -0500 Subject: [PATCH 13/17] fix: make the secret-scan gate fail when it scanned nothing The gitleaks gate ran the container but asserted nothing about what it did. On a bad revision range or a git dubious-ownership refusal (image runs as root, checkout owned by runner, no safe.directory), gitleaks prints "ERR [git] ..." + "0 commits scanned" and STILL exits 0 -- so the gate went green having scanned NOTHING. "0 commits scanned" and "11 commits scanned" were indistinguishable to it, exactly the silent-pass failure already fixed for script-selftests. Root-cause fix: whitelist /repo via GIT_CONFIG_* env inside the container (no writable HOME needed, unlike git config --global) so the dubious-ownership path cannot short-circuit the scan. Backstop: capture the output and fail on any "ERR [git]" line, on a missing "commits scanned" line, on 0 commits for a non-empty range, and on a non-zero gitleaks exit (leaks found). The assertions run even with the safe.directory fix in place, so a future breakage is caught, not masked. Reproduced against throwaway git fixtures: bad range and hostile root-mount both yield ERR + 0-commits + rc0 and are now caught; valid range (2 commits), full history (3 commits) still pass; a planted private key exits 1 and fails the gate. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 52 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d34640..500be40 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,6 +156,17 @@ jobs: with: fetch-depth: 0 # full history so the range scan sees add-then-remove - name: gitleaks + env: + # bonnyr-f5 #182 r3 BLOCKER root-cause fix: the gitleaks image runs as + # root while the checkout is owned by `runner`, so git 2.35.2+ refuses + # the mount ("detected dubious ownership"), gitleaks then scans 0 commits + # and EXITS 0 -- a silently blind green gate. Whitelisting /repo via + # GIT_CONFIG_* env (no writable HOME needed, unlike `git config --global`) + # makes git trust the mount so the scan actually runs. The assertions in + # the step STILL run, so a future regression is caught, not masked. + GIT_CONFIG_COUNT: "1" + GIT_CONFIG_KEY_0: safe.directory + GIT_CONFIG_VALUE_0: /repo run: | # bonnyr-f5 #182 r2: scan the PR/push COMMIT RANGE in git mode, not the # working tree. --no-git misses a secret added then REMOVED within the @@ -169,9 +180,46 @@ jobs: RANGE="" # first push / no base — scan all reachable history fi echo "gitleaks scanning range: ${RANGE:-}" - docker run --rm -v "$PWD:/repo:ro" -w /repo ghcr.io/gitleaks/gitleaks:v8.30.1 detect \ + + # bonnyr-f5 #182 r3 BLOCKER: the gate must FAIL when it did not actually + # scan. On a bad revision range OR a dubious-ownership refusal, gitleaks + # prints "ERR [git] ..." + "0 commits scanned" and STILL exits 0 -- so a + # bare `docker run` gate goes green having scanned NOTHING; "0 commits + # scanned" and "11 commits scanned" are indistinguishable to it. Same + # lesson already applied to script-selftests (positive evidence + no + # silent pass). We capture the output and assert on it. + set +e + out="$(docker run --rm \ + -e GIT_CONFIG_COUNT -e GIT_CONFIG_KEY_0 -e GIT_CONFIG_VALUE_0 \ + -v "$PWD:/repo:ro" -w /repo ghcr.io/gitleaks/gitleaks:v8.30.1 detect \ --source=/repo --config=/repo/.gitleaks.toml --redact --verbose \ - ${RANGE:+--log-opts=$RANGE} + ${RANGE:+--log-opts=$RANGE} 2>&1)" + rc=$? + printf '%s\n' "$out" + + # Strip ANSI so parsing is robust whether or not gitleaks colourises. + clean="$(printf '%s\n' "$out" | sed -E 's/\x1b\[[0-9;]*m//g')" + + # 1) Any git error (bad range, dubious ownership) means the scan never + # saw the repo/range -- fail even though gitleaks exited 0. + if grep -qE 'ERR \[git\]' <<< "$clean"; then + echo "::error::gitleaks hit a git error (bad revision range or dubious ownership) -- the scan did not run"; exit 1 + fi + # 2) Positive evidence the scan ran: gitleaks always prints " commits + # scanned" in git mode. No such line == silence == must not pass. + scanned="$(grep -oE '[0-9]+ commits scanned' <<< "$clean" | grep -oE '^[0-9]+' | tail -n1)" + echo "gitleaks reported commits scanned: ${scanned:-}" + if [ -z "$scanned" ]; then + echo "::error::gitleaks printed no 'commits scanned' line -- no evidence the scan ran"; exit 1 + fi + # 3) A non-empty range that scanned 0 commits scanned NOTHING. + if [ -n "$RANGE" ] && [ "$scanned" -eq 0 ]; then + echo "::error::gitleaks scanned 0 commits for range $RANGE -- the gate would have passed blind"; exit 1 + fi + # 4) Real leaks make gitleaks exit non-zero -- that must still fail here. + if [ "$rc" -ne 0 ]; then + echo "::error::gitleaks exited $rc (leaks found or scan error)"; exit "$rc" + fi script-selftests: name: "P1 · Script Self-Tests" From cf64faf00d277edb953c2a68dc9a04adecbec7b6 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Thu, 20 Aug 2026 23:53:22 -0500 Subject: [PATCH 14/17] ci: close the remaining secret-scan / gate / parity gaps from #182 r3 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 --- .githooks/pre-push | 14 ++++ .github/workflows/ci.yml | 111 +++++++++++++------------- .github/workflows/release.yml | 26 +++--- .github/workflows/secret-baseline.yml | 39 +++++++++ AGENTS.md | 9 +++ Makefile | 51 +++++++++++- scripts/lint-commit-markers.sh | 74 +++++++++++++++++ scripts/secret-scan.sh | 91 +++++++++++++++++++++ 8 files changed, 350 insertions(+), 65 deletions(-) create mode 100644 .github/workflows/secret-baseline.yml create mode 100644 scripts/lint-commit-markers.sh create mode 100644 scripts/secret-scan.sh diff --git a/.githooks/pre-push b/.githooks/pre-push index f3f50c0..5866903 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -16,6 +16,20 @@ if [ $? -ne 0 ]; then exit 1 fi +echo "" + +# ─── Commit-message marker lint (bonnyr-f5 #182 r3) ─────────────────────────── +# Fail fast, before the heavy suite, if any commit about to be pushed carries a +# CI-control marker (which would suppress the workflow run) or a spurious major- +# bump prose line. Same script the ci.yml commit-lint gate runs, so local == CI. +echo "=== Commit message marker lint (pre-push) ===" +if ! bash scripts/lint-commit-markers.sh; then + echo "" + echo "PUSH BLOCKED: a commit message carries a CI-control / spurious-major marker." + echo "Reword it (see AGENTS.md 'Commit conventions') and try again." + exit 1 +fi + echo "" echo "=========================================" echo " Pre-push: Running local checks" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 500be40..a79f3b0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,8 +131,9 @@ jobs: - 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 + # / silent drift (#177 Blocker 2). Goes through `make version-check` so + # this job and `make pre-push` run the identical command (#182 r3). + run: make version-check shellcheck: name: "P1 · ShellCheck" @@ -156,22 +157,18 @@ jobs: with: fetch-depth: 0 # full history so the range scan sees add-then-remove - name: gitleaks - env: - # bonnyr-f5 #182 r3 BLOCKER root-cause fix: the gitleaks image runs as - # root while the checkout is owned by `runner`, so git 2.35.2+ refuses - # the mount ("detected dubious ownership"), gitleaks then scans 0 commits - # and EXITS 0 -- a silently blind green gate. Whitelisting /repo via - # GIT_CONFIG_* env (no writable HOME needed, unlike `git config --global`) - # makes git trust the mount so the scan actually runs. The assertions in - # the step STILL run, so a future regression is caught, not masked. - GIT_CONFIG_COUNT: "1" - GIT_CONFIG_KEY_0: safe.directory - GIT_CONFIG_VALUE_0: /repo run: | # bonnyr-f5 #182 r2: scan the PR/push COMMIT RANGE in git mode, not the # working tree. --no-git misses a secret added then REMOVED within the # branch, which stays fetchable forever from a public clone -- the main # thing a public repo needs a history-aware scan for. + # + # All of the scan + assertion logic (the r3 BLOCKER fix, the + # dubious-ownership safe.directory fix, the archive-depth fix, and the + # digest pin) lives in scripts/secret-scan.sh so `make secret-scan` and + # this job run byte-identical commands (#166 / ci.yml header: local==CI). + # We only compute the range from the event here and hand it to the + # script; RANGE is exported (even when empty => full history). if [ "${{ github.event_name }}" = "pull_request" ]; then RANGE="${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}" elif [ -n "${{ github.event.before }}" ] && [ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then @@ -179,47 +176,36 @@ jobs: else RANGE="" # first push / no base — scan all reachable history fi - echo "gitleaks scanning range: ${RANGE:-}" - - # bonnyr-f5 #182 r3 BLOCKER: the gate must FAIL when it did not actually - # scan. On a bad revision range OR a dubious-ownership refusal, gitleaks - # prints "ERR [git] ..." + "0 commits scanned" and STILL exits 0 -- so a - # bare `docker run` gate goes green having scanned NOTHING; "0 commits - # scanned" and "11 commits scanned" are indistinguishable to it. Same - # lesson already applied to script-selftests (positive evidence + no - # silent pass). We capture the output and assert on it. - set +e - out="$(docker run --rm \ - -e GIT_CONFIG_COUNT -e GIT_CONFIG_KEY_0 -e GIT_CONFIG_VALUE_0 \ - -v "$PWD:/repo:ro" -w /repo ghcr.io/gitleaks/gitleaks:v8.30.1 detect \ - --source=/repo --config=/repo/.gitleaks.toml --redact --verbose \ - ${RANGE:+--log-opts=$RANGE} 2>&1)" - rc=$? - printf '%s\n' "$out" - - # Strip ANSI so parsing is robust whether or not gitleaks colourises. - clean="$(printf '%s\n' "$out" | sed -E 's/\x1b\[[0-9;]*m//g')" + export RANGE + make secret-scan - # 1) Any git error (bad range, dubious ownership) means the scan never - # saw the repo/range -- fail even though gitleaks exited 0. - if grep -qE 'ERR \[git\]' <<< "$clean"; then - echo "::error::gitleaks hit a git error (bad revision range or dubious ownership) -- the scan did not run"; exit 1 - fi - # 2) Positive evidence the scan ran: gitleaks always prints " commits - # scanned" in git mode. No such line == silence == must not pass. - scanned="$(grep -oE '[0-9]+ commits scanned' <<< "$clean" | grep -oE '^[0-9]+' | tail -n1)" - echo "gitleaks reported commits scanned: ${scanned:-}" - if [ -z "$scanned" ]; then - echo "::error::gitleaks printed no 'commits scanned' line -- no evidence the scan ran"; exit 1 - fi - # 3) A non-empty range that scanned 0 commits scanned NOTHING. - if [ -n "$RANGE" ] && [ "$scanned" -eq 0 ]; then - echo "::error::gitleaks scanned 0 commits for range $RANGE -- the gate would have passed blind"; exit 1 - fi - # 4) Real leaks make gitleaks exit non-zero -- that must still fail here. - if [ "$rc" -ne 0 ]; then - echo "::error::gitleaks exited $rc (leaks found or scan error)"; exit "$rc" + commit-lint: + name: "P1 · Commit Message Lint" + needs: changes + if: always() + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 # need the whole PR range of commit messages + - name: Lint commit messages for CI-control / spurious-bump markers + run: | + # bonnyr-f5 #182 r3 (Minor -> enforcement): the AGENTS.md rule against + # CI-control markers in commit messages was documentation only, and + # "documentation is not enforcement" (#166). This gate FAILS a PR/push + # whose commit range carries a marker (which would suppress CI for that + # commit -- the #179/#181 case) or an accidental line-start BREAKING + # CHANGE prose that spuriously majors a release. The .githooks/pre-push + # hook runs the SAME script locally so it is caught before push too. + if [ "${{ github.event_name }}" = "pull_request" ]; then + RANGE="${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}" + elif [ -n "${{ github.event.before }}" ] && [ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then + RANGE="${{ github.event.before }}..${{ github.sha }}" + else + RANGE="" # first push / no base — script scans just the tip commit fi + export RANGE + make commit-lint script-selftests: name: "P1 · Script Self-Tests" @@ -1234,6 +1220,7 @@ jobs: - version-consistency - shellcheck - secret-scan + - commit-lint - script-selftests - lint-frontend - typecheck-backend @@ -1266,15 +1253,31 @@ jobs: # Collect all job results (skipped jobs are OK — they were filtered by path) failed=false - # bonnyr-f5 #182 r2: these gates run `if: always()` on every change, so + + # bonnyr-f5 #182 r3 (Major): the aggregator must verify the change- + # detection job itself SUCCEEDED. If `changes` fails/cancels, ~21 of the + # gates below resolve to `skipped` (their `needs: changes` was never + # satisfied), the old loop accepted skipped as success, and the required + # check printed "CI Gate PASSED" while nothing had actually run. Same + # class as the secret-scan blocker: cannot distinguish "passed" from + # "never evaluated". A non-success `changes` fails the gate outright. + changes_result="${{ needs.changes.result }}" + echo "changes (change-detection): $changes_result" + if [ "$changes_result" != "success" ]; then + echo "::error::change-detection job did not succeed ($changes_result) -- every downstream gate was skipped, so the gate cannot certify anything. Failing." + failed=true + fi + + # bonnyr-f5 #182 r2/r3: these gates run `if: always()` on every change, so # `skipped` for them means a future path-filter silently disabled the # check. Treat skipped as a failure for exactly these. - ALWAYS_RUN="version-consistency shellcheck secret-scan script-selftests" + ALWAYS_RUN="version-consistency shellcheck secret-scan commit-lint script-selftests" for job in \ "lint-backend:${{ needs.lint-backend.result }}" \ "version-consistency:${{ needs.version-consistency.result }}" \ "shellcheck:${{ needs.shellcheck.result }}" \ "secret-scan:${{ needs.secret-scan.result }}" \ + "commit-lint:${{ needs.commit-lint.result }}" \ "script-selftests:${{ needs.script-selftests.result }}" \ "lint-frontend:${{ needs.lint-frontend.result }}" \ "typecheck-backend:${{ needs.typecheck-backend.result }}" \ diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9b0facf..60342e0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -22,9 +22,13 @@ on: branches: - staging - main - # MUST stay in sync with ci.yml's push-trigger paths-ignore list — a - # divergence lets a push trigger Release without a matching CI run, - # which then times out the preflight SHA poll below (PR #297 review). + # ci.yml has NO push paths-ignore any more: it runs CI on EVERY push to + # main/staging (ci.yml:31-36, #182). That is a strict superset of the pushes + # that reach Release here, so any push that starts a release is guaranteed a + # matching CI run for the preflight SHA poll below to find — regardless of + # what this list ignores. This paths-ignore therefore only spares docs-only + # pushes from kicking off a release at all (PR #297 review; premise updated + # for #182, which removed ci.yml's paths-ignore). paths-ignore: - '**.md' - 'docs/**' @@ -148,12 +152,16 @@ jobs: exit 0 fi if [ "$RUN_CONCLUSION" = "cancelled" ]; then - # ci.yml runs with cancel-in-progress: true, so a rapid - # follow-up push to the same branch cancels this SHA's CI - # run. That's not a CI failure for this SHA — it means a - # newer push superseded it, so fail fast instead of - # reporting a generic non-success error. - echo "::error::CI run for $SHA was cancelled — superseded by a newer push; this release attempt is stale, the newer push will release instead." + # A cancelled CI run is not a success for this SHA, so fail + # fast rather than reporting a generic non-success error. + # NOTE (premise updated for #182): ci.yml does NOT cancel + # in-progress runs on main/staging — cancel-in-progress is + # false for exactly these release branches (ci.yml:48), each + # push gets its own concurrency group. So a cancellation here + # is no longer necessarily "a newer push superseded it"; it may + # have been cancelled by other means (e.g. a manual cancel). + # Either way the run is stale — do not release on it. + echo "::error::CI run for $SHA was cancelled — this release attempt is stale (a newer push, if any, will release instead)." exit 1 fi echo "::error::CI run for commit $SHA on branch '$BRANCH' completed with conclusion '$RUN_CONCLUSION'." diff --git a/.github/workflows/secret-baseline.yml b/.github/workflows/secret-baseline.yml new file mode 100644 index 0000000..bdb01fc --- /dev/null +++ b/.github/workflows/secret-baseline.yml @@ -0,0 +1,39 @@ +# ╔══════════════════════════════════════════════════════════════════════════╗ +# ║ Secret Scan — Full-History Baseline ║ +# ║ ║ +# ║ bonnyr-f5 #182 r3 (Major): the per-PR/push gate in ci.yml scans only the ║ +# ║ COMMIT RANGE of each change. Anything already in history before that gate ║ +# ║ landed — or a secret that slips in via a path the range scan misses — is ║ +# ║ never re-examined. This workflow runs gitleaks over ALL reachable history ║ +# ║ on a weekly schedule and on demand, so the whole repo stays monitored. ║ +# ║ ║ +# ║ It reuses scripts/secret-scan.sh (the SAME scan + assertion backstop as ║ +# ║ CI and `make secret-scan`), with RANGE="" meaning "full history". The ║ +# ║ gate still FAILS on a git error or a 0-commit non-scan, so a broken ║ +# ║ baseline is caught, not silently green. ║ +# ╚══════════════════════════════════════════════════════════════════════════╝ + +name: Secret Scan Baseline + +on: + schedule: + # Mondays 06:17 UTC — weekly full-history sweep (off the hour to avoid the + # scheduler's top-of-hour congestion). + - cron: "17 6 * * 1" + workflow_dispatch: {} + +permissions: + contents: read + +jobs: + baseline: + name: "Full-history gitleaks baseline" + runs-on: ubuntu-latest + env: + RANGE: "" # explicit empty => scan all reachable history + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 # the whole history is the point + - name: gitleaks (full history) + run: make secret-scan diff --git a/AGENTS.md b/AGENTS.md index 4ebf357..972aa5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,6 +95,15 @@ major). One repo-specific trap worth stating outright: 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. + This is now **enforced**, not just documented: the `commit-lint` CI gate and the + `.githooks/pre-push` hook both run `scripts/lint-commit-markers.sh`, which fails a + push/PR whose commit range carries any CI-control marker (bonnyr-f5 #182 r3, #166: + documentation is not enforcement). +- **Declare a major bump with a real `BREAKING CHANGE: ` footer**, not a + bold `**BREAKING CHANGE**` heading or a bare colon-less line. `compute_version_bump.sh` + majors on the phrase, so a prose line that *looks* like a footer ships a spurious major + release; `commit-lint` rejects the line-start prose forms while allowing the plain + footer. --- diff --git a/Makefile b/Makefile index 5f19ffc..022347f 100644 --- a/Makefile +++ b/Makefile @@ -473,6 +473,51 @@ shellcheck: [ "$$n" -ge 1 ] || { echo "::error::shellcheck found no files to lint"; exit 1; }; \ printf '%s\n' "$$files" | xargs shellcheck --severity=warning +# ── CI-parity gates (bonnyr-f5 #182 r3, Major) ────────────────────────────── +# The four gates ci.yml added were not runnable locally: `make pre-push` ran +# none of them and `make shellcheck` had no dependents, yet ci.yml's header +# claims `make pre-push` == CI. #166: "a local gate that does not run the CI +# command is not a gate." These targets ARE the CI command (ci.yml calls the +# same `make` targets / same scripts), and `pre-push` now depends on `ci-gates`. +.PHONY: ci-gates version-check secret-scan commit-lint script-selftests + +# Helm chart tag/appVersion + frontend package.json must equal VERSION. +version-check: + @echo "" + @echo "=== Version artifacts consistency (sync-version-artifacts.sh --check) ===" + @bash scripts/sync-version-artifacts.sh --check + +# gitleaks range-aware secret scan + assertion backstop (single source of truth, +# shared with ci.yml's secret-scan job and the scheduled baseline workflow). +# Honours RANGE from the environment; unset => scan since the upstream merge-base. +secret-scan: + @echo "" + @echo "=== Secret scan (gitleaks) ===" + @bash scripts/secret-scan.sh + +# Commit-message marker enforcement (shared with ci.yml's commit-lint job and +# .githooks/pre-push). Honours RANGE; unset => @{upstream}..HEAD. +commit-lint: + @echo "" + @echo "=== Commit message marker lint ===" + @bash scripts/lint-commit-markers.sh + +# The paired self-test harnesses ci.yml's script-selftests job runs. +script-selftests: + @echo "" + @echo "=== Script self-tests ===" + @SELF_TEST=1 bash scripts/compute_version_bump.sh + @if grep -q -- '--self-test' scripts/extract-breaking-changes.sh; then \ + bash scripts/extract-breaking-changes.sh --self-test; \ + else \ + echo " (extract-breaking-changes.sh has no --self-test yet; it lands with #179)"; \ + fi + +# Aggregate: every CI gate that is not already covered by quick-check/tests. +ci-gates: shellcheck version-check commit-lint script-selftests secret-scan + @echo "" + @echo "=== CI-parity gates passed ===" + # Convenience: start/stop/restart all (platform-aware) up: ensure-artifact-network $(COMPOSE) up -d @@ -701,8 +746,10 @@ quick-check: lint typecheck-backend openapi-types-check check-migrations # ── Pre-push (~90s parallel): mirrors ALL CI jobs ─────────────────────────── # Run once before git push. Runs test suites in parallel for speed. -# Prerequisite: quick-check runs first (sequential), then tests fan out. -pre-push: quick-check +# Prerequisite: quick-check runs first (sequential), then the CI-parity gates +# (shellcheck / version-check / commit-lint / script-selftests / secret-scan -- +# bonnyr-f5 #182 r3, so `make pre-push` genuinely == CI), then tests fan out. +pre-push: quick-check ci-gates @echo "" @echo "=== Running all test suites in parallel... ===" @failed=""; \ diff --git a/scripts/lint-commit-markers.sh b/scripts/lint-commit-markers.sh new file mode 100644 index 0000000..d6610fe --- /dev/null +++ b/scripts/lint-commit-markers.sh @@ -0,0 +1,74 @@ +#!/usr/bin/env bash +# +# Enforce the AGENTS.md "Commit conventions" rule -- documentation is not +# enforcement (#166; bonnyr-f5 #182 r3). Shared by the ci.yml `commit-lint` job, +# `make commit-lint`, and .githooks/pre-push, so a local run == CI. +# +# FAILS a commit-message range on either of: +# +# 1. A CI-control marker anywhere in subject or body. GitHub scans the whole +# message, so one of these sitting even in prose SUPPRESSES the workflow run +# for that commit -- and the gates that get skipped (ShellCheck, Secret Scan, +# Script Self-Tests) are exactly the ones that matter. Bit us on #179/#181. +# +# 2. A line that STARTS a major-version-bump declaration as prose rather than a +# real Conventional Commits footer. compute_version_bump.sh bumps major on +# any `\bBREAKINGCHANGE\b`, so a bold "**BREAKING CHANGE**" heading or a +# bare colon-less "BREAKING CHANGE" line spuriously ships a major release. +# A PROPER footer -- a line of the exact form `BREAKING CHANGE: ` (or +# `BREAKING-CHANGE:`), no markdown bold -- is the intended, documented, +# self-tested mechanism and is ALLOWED. (Mid-line prose mentions are a +# separate, pre-existing greediness in the detector itself, owned by the +# version-tooling PRs #179/#180; this gate does not touch them.) +# +# RANGE (env): "base..head" to scan. If unset/empty, defaults to +# @{upstream}..HEAD, else just the tip commit. Never scans all history (old +# release-bot commits legitimately carry the deliberate skip marker). +set -uo pipefail + +# Resolve the commit list without ever falling back to full history. +commits="" +if [ -n "${RANGE:-}" ]; then + commits="$(git rev-list "$RANGE" 2>/dev/null || true)" +elif upstream="$(git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' 2>/dev/null)"; then + commits="$(git rev-list "${upstream}..HEAD" 2>/dev/null || true)" +fi +[ -z "$commits" ] && commits="$(git rev-list -1 HEAD)" + +# CI-control markers (matched case-insensitively, as fixed strings). +markers=('[skip ci]' '[ci skip]' '[no ci]' '[skip actions]' '[actions skip]') + +fail=0 +n=0 +while IFS= read -r sha; do + [ -z "$sha" ] && continue + n=$((n + 1)) + msg="$(git log -1 --format='%B' "$sha")" + subject="$(git log -1 --format='%s' "$sha")" + + for m in "${markers[@]}"; do + if grep -iqF -- "$m" <<< "$msg"; then + echo "::error::commit $sha ($subject): message contains CI-control marker \"$m\" -- it would suppress the workflow run. Refer to it indirectly (e.g. \"the skip-CI marker\") or split it across backticks." + fail=1 + fi + done + + # A line that opens with a major-bump declaration... + while IFS= read -r line; do + if grep -qE '^(\*\*)?BREAKING[ -]CHANGE' <<< "$line"; then + # ...is allowed ONLY as a bare, unbolded footer "BREAKING CHANGE: ". + if grep -qE '^BREAKING[ -]CHANGE: .' <<< "$line"; then + continue + fi + echo "::error::commit $sha ($subject): line \"$line\" starts a BREAKING CHANGE declaration that is not a plain Conventional Commits footer -- it spuriously triggers a major release. Use a real footer 'BREAKING CHANGE: ' or reword (e.g. lowercase 'breaking-change')." + fail=1 + fi + done <<< "$msg" +done <<< "$commits" + +echo "commit-lint: scanned $n commit(s) in range '${RANGE:-}'" +if [ "$fail" -ne 0 ]; then + echo "::error::commit-lint failed -- see markers above." + exit 1 +fi +echo "commit-lint: OK" diff --git a/scripts/secret-scan.sh b/scripts/secret-scan.sh new file mode 100644 index 0000000..6fd0002 --- /dev/null +++ b/scripts/secret-scan.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# +# Single source of truth for the gitleaks secret scan + its assertion backstop. +# +# Called by BOTH .github/workflows/ci.yml (the secret-scan job and the scheduled +# baseline job) AND `make secret-scan` / `make pre-push`, so a local run is +# byte-identical to CI -- #166: "a local gate that does not run the CI command is +# not a gate", and ci.yml's header claims `make pre-push` == CI. +# +# The gate must be able to tell "clean scan" from "did not run". gitleaks exits 0 +# on a bad revision range OR a git dubious-ownership refusal, printing +# "ERR [git] ..." + "0 commits scanned" -- a silently blind green gate +# (bonnyr-f5 #182 r3 BLOCKER). So we capture the output and FAIL on: any +# "ERR [git]" line, a missing "commits scanned" line, 0 commits for a non-empty +# range, or a non-zero gitleaks exit (leaks found). +# +# RANGE selection: +# * If the RANGE env var is SET (even to empty), it is used verbatim -- empty +# means "scan all reachable history" (the scheduled baseline + first push). +# CI computes it from the triggering event. +# * If RANGE is UNSET, a local default is computed: everything since HEAD +# diverged from its upstream tracking branch, falling back to full history. +set -uo pipefail + +# gitleaks v8.30.1, pinned by digest so a re-tag cannot change what runs +# (bonnyr-f5 #182 r3 nit). Update the version comment when bumping the digest. +IMAGE="ghcr.io/gitleaks/gitleaks@sha256:c00b6bd0aeb3071cbcb79009cb16a60dd9e0a7c60e2be9ab65d25e6bc8abbb7f" # v8.30.1 + +repo_dir="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" + +# Resolve the range (see header). ${RANGE+set} distinguishes unset from empty. +if [ -n "${RANGE+set}" ]; then + range="$RANGE" +else + range="" + if upstream="$(git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' 2>/dev/null)"; then + if base="$(git merge-base "$upstream" HEAD 2>/dev/null)"; then + range="${base}..HEAD" + fi + fi +fi + +echo "gitleaks scanning range: ${range:-} (repo: $repo_dir)" + +# safe.directory whitelists the mount so git 2.35.2+ does not refuse it for +# dubious ownership (the image runs as root; the checkout is owned by another +# uid). GIT_CONFIG_* needs no writable HOME, unlike `git config --global`. +# --max-archive-depth 2: without it gitleaks defaults to 0 and NEVER looks inside +# tracked archives, so a secret shipped in a tarball is invisible (bonnyr-f5 #182 +# r3 Major). Depth 2 covers e.g. a key inside a .tar.gz inside a .zip. +out="$(docker run --rm \ + -e GIT_CONFIG_COUNT=1 -e GIT_CONFIG_KEY_0=safe.directory -e GIT_CONFIG_VALUE_0=/repo \ + -v "$repo_dir:/repo:ro" -w /repo "$IMAGE" detect \ + --source=/repo --config=/repo/.gitleaks.toml --redact --verbose \ + --max-archive-depth 2 \ + ${range:+--log-opts="$range"} 2>&1)" +rc=$? +printf '%s\n' "$out" + +# Strip ANSI so parsing is robust whether or not gitleaks colourises. +clean="$(printf '%s\n' "$out" | sed -E 's/\x1b\[[0-9;]*m//g')" + +# 1) Any git error (bad range, dubious ownership) means the scan never saw the +# repo/range -- fail even though gitleaks exited 0. +if grep -qE 'ERR \[git\]' <<< "$clean"; then + echo "::error::gitleaks hit a git error (bad revision range or dubious ownership) -- the scan did not run" + exit 1 +fi + +# 2) Positive evidence the scan ran: gitleaks always prints " commits scanned" +# in git mode. No such line == silence == must not pass. +scanned="$(grep -oE '[0-9]+ commits scanned' <<< "$clean" | grep -oE '^[0-9]+' | tail -n1)" +echo "gitleaks reported commits scanned: ${scanned:-}" +if [ -z "$scanned" ]; then + echo "::error::gitleaks printed no 'commits scanned' line -- no evidence the scan ran" + exit 1 +fi + +# 3) A non-empty range that scanned 0 commits scanned NOTHING. +if [ -n "$range" ] && [ "$scanned" -eq 0 ]; then + echo "::error::gitleaks scanned 0 commits for range $range -- the gate would have passed blind" + exit 1 +fi + +# 4) Real leaks make gitleaks exit non-zero -- that must still fail here. +if [ "$rc" -ne 0 ]; then + echo "::error::gitleaks exited $rc (leaks found or scan error)" + exit "$rc" +fi + +echo "secret-scan: OK" From 46d4ec785e2ab4371cc36b36d32c4d27d4b69db0 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Fri, 21 Aug 2026 01:11:44 -0500 Subject: [PATCH 15/17] ci: parity-check the detector FUNCTIONS, not just a regex Follow-up to bonnyr-f5 #179 r4. That PR factors breaking-change detection into _is_breaking_subject and _is_breaking_body functions -- the body one is a paragraph-aware awk, not a single grep -- so the old regex-extraction parity check could not see the awk detector, and bonnyr's nit was right that compute had no function to diff. The parity step is now adaptive: when both scripts define _is_breaking_body it diffs the full bodies of both detector functions (guarding the awk too); on the pre-#179 tree (inline greps, no functions) it falls back to extracting the detector regex, so the gate stays meaningful on this PR before the stack merges. Verified across three states: old tree -> regex fallback passes; new scripts -> function diff passes and runs the extractor self-test; a one-character mutation of compute's _is_breaking_body -> exit 1 naming the differing function. YAML + actionlint clean. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- .github/workflows/ci.yml | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a79f3b0..6ca812b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -254,22 +254,40 @@ jobs: # (both scripts use the SAME form as each other in either state). That # keeps this gate meaningful on this PR today AND on the merged stack. set -euo pipefail + E=scripts/extract-breaking-changes.sh + C=scripts/compute_version_bump.sh + # Adaptive check. #179 factors detection into _is_breaking_subject and + # _is_breaking_body FUNCTIONS (the body one is a paragraph-aware awk, not + # a single grep). When those functions exist, diff their full bodies -- + # that guards the awk detector too, and answers bonnyr-f5 #179 r4's nit + # that compute had no function to diff. On the pre-#179 tree (inline + # greps, no functions) fall back to extracting the detector regex, so the + # gate stays meaningful on this PR before the stack merges. + _fn() { sed -n "/^$2()/,/^}/p" "$1"; } # print a function definition _regex() { grep -oE "grep -qE '[^']*BREAKING[^']*CHANGE[^']*'" "$1" \ | sed -E "s/^grep -qE '//; s/'\$//" | sort -u } - ex="$(_regex scripts/extract-breaking-changes.sh)" - cv="$(_regex scripts/compute_version_bump.sh)" - if [ -z "$ex" ] || [ -z "$cv" ]; then - echo "::error::could not extract a BREAKING CHANGE detector regex from one of the scripts (extract='$ex' compute='$cv')"; exit 1 - fi - if [ "$ex" != "$cv" ]; then - echo "::error::INV-15 violated: the BREAKING CHANGE detector differs between the two scripts" - echo " extract-breaking-changes.sh: $ex" - echo " compute_version_bump.sh: $cv" - exit 1 + if grep -q '^_is_breaking_body()' "$E" && grep -q '^_is_breaking_body()' "$C"; then + for fn in _is_breaking_subject _is_breaking_body; do + if [ "$(_fn "$E" "$fn")" != "$(_fn "$C" "$fn")" ]; then + echo "::error::INV-15 violated: $fn differs between the two scripts" + diff <(_fn "$E" "$fn") <(_fn "$C" "$fn") || true + exit 1 + fi + done + echo "INV-15 OK: _is_breaking_subject + _is_breaking_body are byte-identical functions in both scripts" + else + ex="$(_regex "$E")"; cv="$(_regex "$C")" + if [ -z "$ex" ] || [ -z "$cv" ]; then + echo "::error::could not extract a BREAKING CHANGE detector from one of the scripts (extract='$ex' compute='$cv')"; exit 1 + fi + if [ "$ex" != "$cv" ]; then + echo "::error::INV-15 violated: the BREAKING CHANGE detector regex differs between the two scripts" + echo " extract: $ex"; echo " compute: $cv"; exit 1 + fi + echo "INV-15 OK (pre-#179 tree): detector regex identical across both scripts -> $ex" fi - echo "INV-15 OK: BREAKING CHANGE detector identical across both scripts -> $ex" # Run the extractor's own self-test once #179's anchored extractor (which # adds --self-test) is in the tree. Until #179 merges to staging, this # PR's base carries the older extractor; warn LOUDLY (not a silent skip) From fb1e79331a8168234941e6c985f747a69e980433 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Fri, 21 Aug 2026 02:08:07 -0500 Subject: [PATCH 16/17] fix: exempt the release bot from commit-lint so promotion can't deadlock INV-4 blocker (bonnyr-f5 #182 r4): the commit-lint gate had no exemption for the release automation's own machine commits. release.yml emits promotion commits of the form "release: vX.Y.Z ", so the staging->main promotion range carried a commit the gate flagged -> CI Gate red -> release preflight refuses the SHA -> main never releases again. Reproduced: rc=1 on a release-bot commit in-range. Fix: exempt commits whose subject matches '^release: ' (the machine identity), while still catching a human who quotes a marker anywhere else. Mutation-tested: a release-bot commit carrying the skip marker now PASSES; a human body/subject with the same marker still FAILS. Also folds in the valid same-surface findings from the review: - fail closed when an explicitly-set RANGE is unresolvable, instead of silently falling back to scanning the tip while printing a range never scanned (matches secret-scan.sh; rc=1 proven). - catch GitHub's documented 'skip-checks: true' commit-check trailer, which the bracketed fixed-string list missed. - broaden the spurious-major detector to every shape the version tooling fires on (bold, bulleted, block-quoted, indented, multi-separator), not just the two it caught; the canonical column-0 footer stays allowed and lowercase stays an escape hatch. - Makefile script-selftests now mirrors ci.yml's anti-vacuity check (fail on a printed FAIL: line, not only a non-zero exit), closing the local-vs-CI gap. 21/21 mutation tests green; bash -n + shellcheck -S style clean; PR range still lints OK. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- Makefile | 12 +++++- scripts/lint-commit-markers.sh | 76 +++++++++++++++++++++++++++------- 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 022347f..727605c 100644 --- a/Makefile +++ b/Makefile @@ -503,10 +503,20 @@ commit-lint: @bash scripts/lint-commit-markers.sh # The paired self-test harnesses ci.yml's script-selftests job runs. +# The compute self-test historically prints "FAIL:" but still exits 0, so +# ci.yml's step fails on EITHER a non-zero exit OR a FAIL: line. Mirror that +# exact anti-vacuity logic here or a broken detector passes locally while CI +# goes red (bonnyr-f5 #182 r4: a local gate that diverges from the CI command +# is not a gate). script-selftests: @echo "" @echo "=== Script self-tests ===" - @SELF_TEST=1 bash scripts/compute_version_bump.sh + @set +e; out="$$(SELF_TEST=1 bash scripts/compute_version_bump.sh 2>&1)"; rc=$$?; \ + echo "$$out"; \ + if [ "$$rc" -ne 0 ]; then echo "::error::compute_version_bump self-test exited $$rc"; exit "$$rc"; fi; \ + if printf '%s\n' "$$out" | grep -qE '(^|[[:space:]])FAIL:'; then \ + echo "::error::compute_version_bump self-test reported FAIL: but exited 0"; exit 1; \ + fi @if grep -q -- '--self-test' scripts/extract-breaking-changes.sh; then \ bash scripts/extract-breaking-changes.sh --self-test; \ else \ diff --git a/scripts/lint-commit-markers.sh b/scripts/lint-commit-markers.sh index d6610fe..0b54c67 100644 --- a/scripts/lint-commit-markers.sh +++ b/scripts/lint-commit-markers.sh @@ -4,36 +4,59 @@ # enforcement (#166; bonnyr-f5 #182 r3). Shared by the ci.yml `commit-lint` job, # `make commit-lint`, and .githooks/pre-push, so a local run == CI. # -# FAILS a commit-message range on either of: +# FAILS a commit-message range on any of: # # 1. A CI-control marker anywhere in subject or body. GitHub scans the whole # message, so one of these sitting even in prose SUPPRESSES the workflow run # for that commit -- and the gates that get skipped (ShellCheck, Secret Scan, # Script Self-Tests) are exactly the ones that matter. Bit us on #179/#181. +# The `skip-checks: true` commit-check trailer is caught too (bonnyr-f5 #182 +# r4): it is GitHub's documented way to suppress ALL required checks and is +# not a bracketed token, so the fixed-string list alone would miss it. # # 2. A line that STARTS a major-version-bump declaration as prose rather than a # real Conventional Commits footer. compute_version_bump.sh bumps major on -# any `\bBREAKINGCHANGE\b`, so a bold "**BREAKING CHANGE**" heading or a -# bare colon-less "BREAKING CHANGE" line spuriously ships a major release. -# A PROPER footer -- a line of the exact form `BREAKING CHANGE: ` (or -# `BREAKING-CHANGE:`), no markdown bold -- is the intended, documented, -# self-tested mechanism and is ALLOWED. (Mid-line prose mentions are a -# separate, pre-existing greediness in the detector itself, owned by the +# any `\bBREAKING[[:space:] -]+CHANGE\b`, so a bold "**BREAKING CHANGE**" +# heading, a bullet "- BREAKING CHANGE", a block-quoted "> BREAKING CHANGE", +# an indented one, or a bare colon-less line all spuriously ship a major +# release. A PROPER footer -- a line of the exact canonical form +# `BREAKING CHANGE: ` (or `BREAKING-CHANGE: `), column 0, no +# markdown, single separator -- is the intended, documented, self-tested +# mechanism and is ALLOWED. (Mid-line prose mentions are a separate, +# pre-existing greediness in the detector itself, owned by the # version-tooling PRs #179/#180; this gate does not touch them.) # +# EXEMPT: the release bot's own commits (subject `^release: `). release.yml's +# promotion commits are of the form "release: vX.Y.Z [skip ci]" -- that marker +# is DELIBERATE (release.yml's loop-guard filters them so a release push does not +# re-trigger a release). Linting them would turn the staging->main promotion +# range red -> the CI Gate fails -> release.yml's preflight refuses that SHA -> +# main never releases again (bonnyr-f5 #182 r4, INV-4/INV-28: a gate that forbids +# a token must exempt the machine identity told to emit it). The exemption is +# scoped to the exact release-bot subject prefix, so a HUMAN quoting a marker in +# any other commit is still caught. +# # RANGE (env): "base..head" to scan. If unset/empty, defaults to # @{upstream}..HEAD, else just the tip commit. Never scans all history (old -# release-bot commits legitimately carry the deliberate skip marker). +# release-bot commits legitimately carry the deliberate skip marker). An +# explicitly-set RANGE that does not resolve is a HARD failure -- we never +# silently fall back to scanning the tip while claiming we scanned the range +# (bonnyr-f5 #182 r4; matches secret-scan.sh's fail-closed behaviour). set -uo pipefail -# Resolve the commit list without ever falling back to full history. -commits="" +# Resolve the commit list without ever falling back to full history, and fail +# closed when an explicit RANGE is unresolvable. if [ -n "${RANGE:-}" ]; then - commits="$(git rev-list "$RANGE" 2>/dev/null || true)" + if ! commits="$(git rev-list "$RANGE" 2>/dev/null)"; then + echo "::error::commit-lint: RANGE '$RANGE' is not a resolvable revision range -- the scan did not run" + exit 1 + fi elif upstream="$(git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' 2>/dev/null)"; then commits="$(git rev-list "${upstream}..HEAD" 2>/dev/null || true)" + [ -z "$commits" ] && commits="$(git rev-list -1 HEAD)" +else + commits="$(git rev-list -1 HEAD)" fi -[ -z "$commits" ] && commits="$(git rev-list -1 HEAD)" # CI-control markers (matched case-insensitively, as fixed strings). markers=('[skip ci]' '[ci skip]' '[no ci]' '[skip actions]' '[actions skip]') @@ -46,6 +69,13 @@ while IFS= read -r sha; do msg="$(git log -1 --format='%B' "$sha")" subject="$(git log -1 --format='%s' "$sha")" + # Release-bot commits are exempt (see header): machine identity told to emit + # the marker. A human quoting a marker in any non-release commit is still hit. + if grep -qE '^release: ' <<< "$subject"; then + echo "commit-lint: commit $sha ($subject) is a release-bot commit -- exempt" + continue + fi + for m in "${markers[@]}"; do if grep -iqF -- "$m" <<< "$msg"; then echo "::error::commit $sha ($subject): message contains CI-control marker \"$m\" -- it would suppress the workflow run. Refer to it indirectly (e.g. \"the skip-CI marker\") or split it across backticks." @@ -53,14 +83,28 @@ while IFS= read -r sha; do fi done - # A line that opens with a major-bump declaration... + # GitHub's documented commit-check trailer suppresses ALL required checks; it + # is a key:value trailer, not a bracketed token, so the fixed-string list above + # would miss it. + if grep -iqE '^[[:space:]]*skip-checks:[[:space:]]*true\b' <<< "$msg"; then + echo "::error::commit $sha ($subject): message carries the 'skip-checks: true' trailer -- it suppresses all required checks. Remove it or refer to it indirectly." + fail=1 + fi + + # A line that OPENS with a major-bump declaration -- in any of the shapes a + # human writes (bare, bold, bulleted, block-quoted, indented) -- spuriously + # majors a release, because the detector fires on the token anywhere. while IFS= read -r line; do - if grep -qE '^(\*\*)?BREAKING[ -]CHANGE' <<< "$line"; then - # ...is allowed ONLY as a bare, unbolded footer "BREAKING CHANGE: ". + # Peel a leading run of markdown/quote/whitespace/bold so we judge the line + # by the shape a human wrote, not just a bare column-0 token. + stripped="$(sed -E 's/^[[:space:]]*([>*+-][[:space:]]*)*//' <<< "$line")" + if grep -qE '^BREAKING[[:space:] -]+CHANGE' <<< "$stripped"; then + # ...allowed ONLY as the exact canonical footer at column 0: no leading + # prefix, no markdown, a single separator, real ": ". if grep -qE '^BREAKING[ -]CHANGE: .' <<< "$line"; then continue fi - echo "::error::commit $sha ($subject): line \"$line\" starts a BREAKING CHANGE declaration that is not a plain Conventional Commits footer -- it spuriously triggers a major release. Use a real footer 'BREAKING CHANGE: ' or reword (e.g. lowercase 'breaking-change')." + echo "::error::commit $sha ($subject): line \"$line\" starts a BREAKING CHANGE declaration that is not a plain Conventional Commits footer -- it spuriously triggers a major release. Use a real footer 'BREAKING CHANGE: ' at column 0 or reword (e.g. lowercase 'breaking-change')." fail=1 fi done <<< "$msg" From 95da587cab127e29ee45e7cda1c06b4af4ecebb8 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Fri, 21 Aug 2026 07:33:15 -0500 Subject: [PATCH 17/17] fix: exempt GitHub's squash composer from commit-lint; complete self-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 ", 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 --- .githooks/pre-push | 31 ++++++++++++++++++++++++++++++- Makefile | 23 ++++++++++++++++++----- scripts/lint-commit-markers.sh | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/.githooks/pre-push b/.githooks/pre-push index 5866903..026ea80 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -22,8 +22,37 @@ echo "" # Fail fast, before the heavy suite, if any commit about to be pushed carries a # CI-control marker (which would suppress the workflow run) or a spurious major- # bump prose line. Same script the ci.yml commit-lint gate runs, so local == CI. +# +# RANGE from the pre-push stdin protocol (bonnyr-f5 #182 r5, Minor). git feeds +# this hook one " " line per ref +# being pushed. Scanning the script's default `@{upstream}..HEAD` misses every +# non-tip commit when the branch has no upstream yet (a FIRST push) -- exactly +# when a bad commit is most likely to slip in. Deriving `..` from stdin scans precisely the commits this push introduces. A new remote +# branch reports an all-zero remote sha (no merge-base to diff against); there we +# fall back to the script's own default rather than scanning all of history. +# Deletions (all-zero local sha) contribute no commits. When stdin is empty (the +# hook run by hand, not by git) we leave RANGE unset so the script default runs. +zero="0000000000000000000000000000000000000000" +prepush_range="" +while read -r _localref localsha _remoteref remotesha; do + [ -z "${localsha:-}" ] && continue + [ "$localsha" = "$zero" ] && continue # branch deletion: nothing to lint + if [ "${remotesha:-$zero}" = "$zero" ]; then + prepush_range="__DEFAULT__" # new branch: no base -> script default + break + fi + prepush_range="${remotesha}..${localsha}" # normal update: exactly the pushed commits + break +done + echo "=== Commit message marker lint (pre-push) ===" -if ! bash scripts/lint-commit-markers.sh; then +if [ -n "$prepush_range" ] && [ "$prepush_range" != "__DEFAULT__" ]; then + lint_status() { RANGE="$prepush_range" bash scripts/lint-commit-markers.sh; } +else + lint_status() { bash scripts/lint-commit-markers.sh; } +fi +if ! lint_status; then echo "" echo "PUSH BLOCKED: a commit message carries a CI-control / spurious-major marker." echo "Reword it (see AGENTS.md 'Commit conventions') and try again." diff --git a/Makefile b/Makefile index 727605c..22a0fa4 100644 --- a/Makefile +++ b/Makefile @@ -503,11 +503,18 @@ commit-lint: @bash scripts/lint-commit-markers.sh # The paired self-test harnesses ci.yml's script-selftests job runs. -# The compute self-test historically prints "FAIL:" but still exits 0, so -# ci.yml's step fails on EITHER a non-zero exit OR a FAIL: line. Mirror that -# exact anti-vacuity logic here or a broken detector passes locally while CI -# goes red (bonnyr-f5 #182 r4: a local gate that diverges from the CI command -# is not a gate). +# ci.yml's compute step has FOUR anti-vacuity assertions and this target must +# mirror ALL of them, or a broken harness passes locally while CI goes red +# (bonnyr-f5 #182 r4/r5, Major-3: a local gate that diverges from the CI command +# is not a gate). The four (in ci.yml order): +# 1. non-zero exit -> the harness itself errored +# 2. a "FAIL:" line (rc still 0) -> an assertion failed but exit was swallowed +# 3. NO "PASS:" line -> the guard was silenced / renamed: green with +# zero assertions actually run +# 4. NO "=== END SELF-TEST ===" -> the harness exited early (deleted END marker +# or an early `exit 0`) with assertions unrun +# r5 landed 3+4 here; r4 had only 1+2, so the "silenced guard" and "early exit" +# harness-break modes were CI-red but `make`-GREEN. script-selftests: @echo "" @echo "=== Script self-tests ===" @@ -516,6 +523,12 @@ script-selftests: if [ "$$rc" -ne 0 ]; then echo "::error::compute_version_bump self-test exited $$rc"; exit "$$rc"; fi; \ if printf '%s\n' "$$out" | grep -qE '(^|[[:space:]])FAIL:'; then \ echo "::error::compute_version_bump self-test reported FAIL: but exited 0"; exit 1; \ + fi; \ + if ! printf '%s\n' "$$out" | grep -qE '(^|[[:space:]])PASS:'; then \ + echo "::error::self-test produced no PASS lines -- the harness did not run"; exit 1; \ + fi; \ + if ! printf '%s\n' "$$out" | grep -qE '=== END SELF-TEST ==='; then \ + echo "::error::self-test did not reach its END marker -- it exited early with assertions unrun"; exit 1; \ fi @if grep -q -- '--self-test' scripts/extract-breaking-changes.sh; then \ bash scripts/extract-breaking-changes.sh --self-test; \ diff --git a/scripts/lint-commit-markers.sh b/scripts/lint-commit-markers.sh index 0b54c67..442ddf7 100644 --- a/scripts/lint-commit-markers.sh +++ b/scripts/lint-commit-markers.sh @@ -36,6 +36,16 @@ # scoped to the exact release-bot subject prefix, so a HUMAN quoting a marker in # any other commit is still caught. # +# EXEMPT (2nd machine identity, bonnyr-f5 #182 r5, BLOCKER-1): GitHub's own +# squash-merge composer -- committer `GitHub `, single +# parent. Its body is machine-composed from the PR description and the commit is +# already merged (the new tip of staging/main), so it is unamendable and already +# past the pre-merge gate. On a push to staging/main the range `before..tip` +# scans this squash tip; without the exemption a BREAKING-CHANGE bullet or a +# quoted marker in the summarised body reddens the push's ci-gate and release.yml +# then refuses to release that SHA. Human commits never carry this committer +# identity, so they are still fully linted in their own PR. +# # RANGE (env): "base..head" to scan. If unset/empty, defaults to # @{upstream}..HEAD, else just the tip commit. Never scans all history (old # release-bot commits legitimately carry the deliberate skip marker). An @@ -76,6 +86,30 @@ while IFS= read -r sha; do continue fi + # GitHub's squash-merge composer is the SECOND machine identity on the + # promotion path (bonnyr-f5 #182 r5, BLOCKER-1 / INV-28). When a PR is + # squash-merged, GitHub composes the resulting commit's BODY from the PR + # description under the identity `GitHub ` with a single + # parent. That commit is (a) UNAMENDABLE -- its body is machine-composed, and + # (b) ALREADY MERGED -- it is the new tip of staging/main, so re-linting it + # serves no pre-merge purpose (the human commits it summarises were linted in + # their own PR). On a push to staging/main the range is `before..tip`, so the + # just-merged squash tip IS scanned; a stray line-start "BREAKING CHANGE" or a + # marker quoted from the summarised PR body then turns the push's ci-gate red + # and release.yml's preflight refuses that SHA -- the pipeline stops releasing. + # Exempting this machine identity (mirroring the `^release: ` exemption) means + # the gate never judges already-merged, machine-composed history, while every + # HUMAN-authored commit -- which never carries this committer identity -- is + # still linted in its own PR. This is an identity check on the committer, not a + # spoofable subject allowlist. Scoped to single-parent commits so a genuine + # non-squash merge is not blanket-exempted. + committer="$(git log -1 --format='%cn <%ce>' "$sha")" + nparents="$(git log -1 --format='%p' "$sha" | wc -w)" + if [ "$committer" = "GitHub " ] && [ "$nparents" -eq 1 ]; then + echo "commit-lint: commit $sha ($subject) is a GitHub-composed squash commit (already-merged machine identity) -- exempt" + continue + fi + for m in "${markers[@]}"; do if grep -iqF -- "$m" <<< "$msg"; then echo "::error::commit $sha ($subject): message contains CI-control marker \"$m\" -- it would suppress the workflow run. Refer to it indirectly (e.g. \"the skip-CI marker\") or split it across backticks."