From cce1ad909a272f4e5a163bc6db61f50e5750fdbe Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 18:45:58 -0500 Subject: [PATCH 01/13] docs: add scoped rollback and image quarantine design spec --- ...ed-rollback-and-image-quarantine-design.md | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md diff --git a/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md b/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md new file mode 100644 index 0000000..bc449a5 --- /dev/null +++ b/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md @@ -0,0 +1,264 @@ +# Scoped rollback and image quarantine + +**Status:** Approved (design) +**Date:** 2026-08-24 +**Scope:** `compose-workflow` reusable workflow (`deploy.yml`); new user skill `quarantine-image`; caller-repo `.github/renovate.json` + +## Problem + +A bad upstream image release (`ghcr.io/lukegus/termix:release-2.7.1`) could not start. That single stack +failure took down every subsequent deployment in `docker-piwine`. Two independent defects combined to +produce the outage, and neither one is fixed by fixing the other. + +### Defect 1 — unbounded blast radius + +`deploy.yml` rolls back by resetting the entire live tree: + +``` +rollback: + - git -C "$LIVE_REPO_PATH" reset --hard "$PREVIOUS_SHA" # deploy.yml:719 + - docker compose up ... for every existing + removed stack # deploy.yml:736-748 +``` + +One stack's failure therefore reverts the configuration of all fifteen stacks in the repo. Stacks that +deployed successfully moments earlier are silently rolled backwards for a reason unrelated to them. + +### Defect 2 — the failure is self-perpetuating + +Rollback changes the live tree; it does not change `main`. The bad `image:` line is still committed. +Every deploy begins with `git reset --hard "$TARGET_REF"` (`deploy.yml:283`), so the next commit to land +— any unrelated Renovate PR — re-applies the bad image, fails again, and rolls back again. The pipeline +stays red indefinitely and no further changes reach the host. + +The obvious manual fix, `git revert` of the bad bump, does not hold. `compose-workflow/default.json` +groups every `minor`/`patch`/`digest` update into `all-deps` with `automerge: true` and +`minimumReleaseAge: "1 hour"`. Reverting makes the old version current again; Renovate observes a newer +tag available and re-proposes and re-merges it within the hour. **Revert alone is structurally incapable +of holding**, because Renovate has no memory of a version being bad. + +## Design + +Two independent changes that compose. Change A makes a bad release *survivable*. Change B makes it +*stop recurring*. Neither is sufficient alone. + +--- + +## Change A — scoped rollback in `deploy.yml` + +Rollback gains a per-stack path used when the failure is provably confined to stack directories. The +existing whole-tree path is retained unchanged as the fallback for every other case. + +### A1. `prepare` classifies rollback scope + +New step after `Get changed files` (`deploy.yml:120`). It reads the `all_changed_files` JSON already +produced by `tj-actions/changed-files` and takes the first path segment of each changed path. + +| Condition | `rollback_scope` | +|---|---| +| Every changed path's first segment is a known stack directory | `per-stack` | +| Any changed path is a root file (`compose.env`, `.github/**`, `README.md`, …) | `whole-tree` | +| `changed-files` step was skipped (`previous_sha == target-ref`, e.g. `force-deploy`) | `whole-tree` | + +New `prepare` job output: `rollback_scope`. + +The `whole-tree` default on uncertainty is deliberate. `compose.env` is repo-root and shared by every +stack via `op run --env-file="$LIVE_REPO_PATH/compose.env"`. A failure caused by a renamed variable or a +dead 1Password reference lives *outside* any stack directory, so reverting one stack directory would fix +nothing. Whole-tree reset is the correct behavior for that case and must remain reachable. + +### A2. `health-check` names its casualties + +`health-check` builds a `failed=()` array but currently exposes only `status`. Add a `failed_stacks` +JSON output alongside it. + +Without this, a *health* failure (as distinct from a *deploy* failure) has no stack list to scope +against, and every health failure would silently degrade to whole-tree rollback — losing most of the +benefit of this change, since health failures are the common shape of a bad image release. + +### A3. `rollback` gains a per-stack branch + +A new first step unions the culprit lists: + +``` +culprits = deploy.outputs.existing_failed_stacks + ∪ deploy.outputs.new_failed_stacks + ∪ health-check.outputs.failed_stacks +``` + +Then two mutually exclusive branches: + +**Per-stack** — when `rollback_scope == 'per-stack'` AND `culprits` is non-empty. For each culprit: + +| Culprit type | Action | +|---|---| +| new stack | `docker compose down`; stays gone (matches current behavior) | +| existing stack | `git checkout $PREVIOUS_SHA -- /`, then `op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- docker compose up -d --quiet-pull --wait --remove-orphans` | + +No `--pull always` and no `--build`, matching the current rollback step: the stack lands on the +locally-tagged previous image kept on disk by the docker-prune policy, so recovery does not depend on a +registry being reachable mid-incident. Healthy stacks are never touched. + +**Whole-tree** — every other case. Today's exact behavior, unchanged: tear down new stacks, +`git reset --hard "$PREVIOUS_SHA"`, re-up existing + removed. + +### A4. Partial checkout leaves the live tree dirty — and that is safe + +`git checkout $PREVIOUS_SHA -- /` leaves the live tree with `HEAD` at the target SHA but one +directory's content staged at the previous SHA. No cleanup step is required: every deploy begins with +`git -C "$LIVE_REPO_PATH" reset --hard "$TARGET_REF"` (`deploy.yml:283`), which restores a clean tree +before anything else runs. No drift accumulates across runs. + +### A5. `notify` reports which path ran + +The Discord message currently renders a pipeline line such as `✅ Deploy → ❌ Health → ✅ Rollback`. +Extend the rollback segment with the scope and culprit list: + +``` +🔄 Rollback: per-stack (termix) +🔄 Rollback: whole-tree +``` + +Without this, a narrowly-scoped rollback is indistinguishable in the alert from a full fleet revert — +which is precisely the distinction the on-call reader needs first. + +### What Change A explicitly does not fix + +With no auto-quarantine, the bad `image:` line remains on `main`. The next deploy re-applies it, fails +on that stack, and rolls that stack back again. Unrelated updates in the same commit still land +successfully — that is the win — but the run is still red and still pages. Stopping recurrence is +Change B. + +--- + +## Change B — the `quarantine-image` skill + +### B1. The blocking mechanism + +Renovate's `allowedVersions` accepts either a version *range* interpreted by the active versioning +scheme, or a *regex* delimited by forward slashes, with `!/…/` as the negated (exclude) form. + +**The negated regex is the correct mechanism here, and range syntax such as `!=release-2.7.1` is not.** +Ranges are interpreted by the versioning scheme, and these repos pin several images to custom `regex:` +versioning schemes where range operators are not dependable: + +```json +{ "matchPackageNames": ["ghcr.io/lukegus/termix"], + "versioning": "regex:^release-(?\\d+)\\.(?\\d+)\\.(?\\d+)$" } +``` + +A negated regex filters the **raw version string** before any versioning scheme parses it. One mechanism +therefore covers every tag format in the fleet — `release-2.7.1`, LSIO's `4.2.1-ls286`, homebridge's +`2026-08-24`: + +```json +{ "matchPackageNames": ["ghcr.io/lukegus/termix"], + "allowedVersions": "!/^release-2\\.7\\.1$/" } +``` + +Because `allowedVersions` filters at lookup time, a blocked version is not merely un-automerged — it is +invisible to Renovate. Paired with reverting the compose file, that is what makes a revert hold. + +### B2. Shape and invocation + +A single `~/.claude/skills/quarantine-image/SKILL.md`, matching the existing `renovate-trigger` and +`renovate-dashboard-triage` convention: prose with inline bash recipes, no helper scripts, no plugin +packaging. + +``` +/quarantine-image "" +/quarantine-image --unblock +``` + +The stack name locates the repo by globbing `docker-piwine*//compose.yaml`. A name matching more +than one repo is an error, not a guess. + +### B3. Quarantine steps + +1. **Resolve the image.** Read the `image:` line from the stack's compose file; split package name + (`ghcr.io/lukegus/termix`) from tag and digest. For multi-service stacks, the supplied bad version + identifies which service. + +2. **Find the last-known-good line.** Walk `git log -p -- /compose.yaml` back to the commit + before the bad version landed and take that `image:` line **verbatim** — tag and digest together, + exactly as Renovate originally wrote them. The skill never composes a `@sha256:` by hand; it restores + a tag/digest pair a real Renovate run already verified. + +3. **Write the block.** Add or extend a `packageRules` entry in the **consuming repo's** + `.github/renovate.json` — never the shared `compose-workflow/default.json`, since a bad `termix` + build is not `docker-piwine-office`'s problem. + + | Case | Action | + |---|---| + | No existing block for the package | Append a new rule object with the negated-regex `allowedVersions` | + | A block already exists | Widen the alternation in place: `!/^release-(2\.7\.1\|2\.8\.0)$/` | + + Extending in place rather than appending matters: two `packageRules` entries matching the same + package both apply, and the later `allowedVersions` silently wins, discarding the earlier block. + + This rule is kept **separate** from the package's existing `versioning` rule + (`docker-piwine/.github/renovate.json:19-23`). Renovate merges matching rules in order, so the two + coexist, and keeping them apart lets `--unblock` delete a whole object instead of surgically editing + a shared one. + +4. **Commit both files together, atomically.** The ordering is load-bearing: the compose revert makes + `2.7.0` current *in the same commit* that makes `2.7.1` invisible. Split across two commits, there is + a window in which the pinned-current version is also the blocked version — a state Renovate handles + badly. The commit message records the reason and the failing run. + +5. **Report and stop.** Print the diff and push. The skill does **not** trigger a Renovate run; it + points at the existing `renovate-trigger` skill rather than reimplementing it. + +### B4. `--unblock` + +Removes the package's rule object and its comment from `.github/renovate.json`, commits, and notes that +Renovate will offer the previously-blocked version again on its next run. It does not touch the compose +file: by the time you unblock, you want the newest version, not the one you blocked. + +### B5. Block lifecycle + +Blocks are single-version and therefore self-expiring in effect. When upstream ships `2.7.2`, Renovate +offers it normally; the stale rule sits inert and documented until pruned. `--unblock` is the manual +prune. No scheduled audit and no expiry metadata — that machinery would cost more than the stale JSON +objects it removes. + +### B6. Cases the skill must refuse rather than guess + +| Case | Behavior | +|---|---| +| Stack name matches zero or multiple repos | Error; ask the user to disambiguate | +| No prior version in history (bad version was the first ever pinned) | Error; nothing to revert to, tell the user to fix forward | +| `image:` uses a floating tag (`:latest`) | Error; there is no version to block — the answer is a digest pin, not a quarantine | + +--- + +## Worked example: the termix incident under this design + +1. Renovate merges `release-2.7.1`; the image cannot start. +2. Deploy fails on `termix` only. `rollback_scope` is `per-stack` (only `termix/compose.yaml` changed), + so `termix/` reverts to `release-2.7.0` and comes back up. The other fourteen stacks keep their new + configuration. Discord reports `🔄 Rollback: per-stack (termix)`. +3. Operator runs `/quarantine-image termix release-2.7.1 "container exits on boot"`. One commit reverts + the compose line and adds `"allowedVersions": "!/^release-2\\.7\\.1$/"`. +4. Subsequent deploys are green. Renovate never re-proposes `2.7.1`. +5. Upstream ships `2.7.2`; Renovate offers it through the normal `all-deps` path. The stale block + remains until `/quarantine-image --unblock termix`. + +## Testing + +| Change | Verification | +|---|---| +| A1 scope classifier | Unit-test the path-segment logic against fixture file lists: stack-only, root-only, mixed, empty | +| A2 health-check output | Assert `failed_stacks` JSON is well-formed and matches `status=failed` | +| A3 per-stack branch | Deploy a deliberately-broken image to one non-critical stack on the office Pi; assert only that stack reverts and the others stay at the new SHA | +| A3 whole-tree fallback | Same, with a `compose.env` edit in the commit; assert whole-tree reset runs | +| A5 notify | Inspect the rendered Discord payload for both scopes | +| B | `npx --yes renovate-config-validator` on the edited `.github/renovate.json`; confirm on the Dependency Dashboard that the blocked version no longer appears | + +## Out of scope + +- Automatic quarantine from the deploy workflow. The workflow cannot distinguish a bad image from a bad + healthcheck, a bad config change, or a transient registry blip, and a false positive would block a + good version. +- Changes to `minimumReleaseAge` or automerge policy in `default.json`. Preventing bad releases from + being adopted at all is a separate trade-off from recovering when one is. +- Expiry metadata or scheduled auditing of blocks (see B5). From 701e8d462b17a47e4cbe5d94d29c1d9c6e912e16 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 18:50:30 -0500 Subject: [PATCH 02/13] docs: add scoped rollback and image quarantine implementation plan --- ...24-scoped-rollback-and-image-quarantine.md | 798 ++++++++++++++++++ 1 file changed, 798 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md diff --git a/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md b/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md new file mode 100644 index 0000000..1632ae8 --- /dev/null +++ b/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md @@ -0,0 +1,798 @@ +# Scoped Rollback and Image Quarantine Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Confine a failed stack's rollback to that stack alone, and give the operator a one-command way to permanently block a known-bad image version from being re-proposed by Renovate. + +**Architecture:** Change A adds a scope classifier to `deploy.yml`'s `prepare` job that decides between a new per-stack rollback path and the existing whole-tree reset, plus the plumbing (a `failed_stacks` output on `health-check`) needed to know which stacks to roll back. Change B adds a standalone user skill that reverts a bad image bump and writes a negated-regex `allowedVersions` block into the consuming repo's Renovate config, in one atomic commit. + +**Tech Stack:** Bash 5, GitHub Actions reusable workflows, `jq`, `git`, Renovate config JSON, shellcheck/yamllint. + +**Spec:** `docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md` + +--- + +## Context for the implementer + +You are working in `~/Git/Compose/compose-workflow`, a repository of **reusable GitHub Actions workflows** consumed by two private repos (`docker-piwine`, `docker-piwine-office`). Those repos each hold a set of Docker Compose "stacks" — one directory per stack, each containing a `compose.yaml`. + +Facts you need that are not obvious from the code: + +- **Deploys run on a self-hosted runner** that owns a persistent clone of the caller repo at `inputs.live-repo-path`. The workflow mutates that clone directly. There is no ephemeral checkout for the deploy itself. +- **Every deploy starts with `git -C "$LIVE_REPO_PATH" reset --hard "$TARGET_REF"`** (`.github/workflows/deploy.yml:283`). This is why leaving the live tree dirty after a partial rollback is safe — the next run cleans it. Do not add a cleanup step for this. +- **Secrets come from 1Password at runtime.** Every `docker compose up` is wrapped in `op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- …`. **A `compose up` without that wrapper comes up with empty environment variables and appears to succeed.** This is the single easiest way to break this change. Non-`up` compose calls (`down`, `ps`) are deliberately *not* wrapped. +- **Unit tests are local-only.** `scripts/testing/*.sh` are not invoked by any workflow. Run them by hand. Do not wire them into CI as part of this plan — that is out of scope. +- **`jq` is available on the runner** and used throughout the workflow. + +### Deviation from the spec, and why + +The spec (§A1) places the scope classifier "after `Get changed files` (`deploy.yml:120`)". **Implement it after `Detect stack changes` instead.** + +Reason: the classifier decides whether a changed path's first segment names a stack directory. It therefore needs the full set of known stack directories — and a *removed* stack's directory no longer exists in the target tree, so it is absent from `discover-stacks`'s output. Classifying against active stacks alone would misfile every stack deletion as a root-file change and force whole-tree rollback on routine removals. The `removed_stacks` list only exists after `Detect stack changes` runs, so the classifier must follow it. + +This is a placement change only. The classification rules in the spec's table are unchanged. + +--- + +## File Structure + +| File | Status | Responsibility | +|---|---|---| +| `scripts/deployment/classify-rollback-scope.sh` | Create | Pure function: changed-file list + known stack dirs → `rollback_scope`. No git, no docker, no network. | +| `scripts/testing/test-classify-rollback-scope.sh` | Create | Unit tests for the classifier. Mirrors `test-detect-stack-changes.sh` harness style. | +| `.github/workflows/deploy.yml` | Modify | Wire the classifier into `prepare`; add `failed_stacks` to `health-check`; add the per-stack branch to `rollback`; report scope in `notify`. | +| `~/.claude/skills/quarantine-image/SKILL.md` | Create | The `/quarantine-image` skill. Lives outside this repo (user skills dir), matching `renovate-trigger`. | + +The classifier is a separate script rather than inline YAML because inline `run:` blocks cannot be unit-tested, and this is the one piece of new logic with enough branches to be worth testing. Everything else in Change A is plumbing that is only meaningfully verified end-to-end on a real host. + +--- + +## Task 1: Rollback scope classifier + +**Files:** +- Create: `scripts/deployment/classify-rollback-scope.sh` +- Test: `scripts/testing/test-classify-rollback-scope.sh` + +- [ ] **Step 1: Write the failing test** + +Create `scripts/testing/test-classify-rollback-scope.sh`: + +```bash +#!/usr/bin/env bash +# Unit tests for scripts/deployment/classify-rollback-scope.sh +# Pure input/output tests — no git repos, no docker. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" +CLASSIFY="$REPO_ROOT/scripts/deployment/classify-rollback-scope.sh" + +TMPROOT=$(mktemp -d -t classify-tests.XXXXXX) +trap 'rm -rf "$TMPROOT"' EXIT + +PASS=0 +FAIL=0 +FAILURES=() + +# expect_scope +expect_scope() { + local name="$1" expected="$2" changed="$3" dirs="$4" + local out actual + out=$(mktemp -p "$TMPROOT") + GITHUB_OUTPUT="$out" "$CLASSIFY" \ + --changed-files "$changed" \ + --stack-dirs "$dirs" \ + >/dev/null 2>&1 || true + actual=$(grep '^rollback_scope=' "$out" 2>/dev/null | cut -d= -f2- || echo "") + if [[ "$actual" == "$expected" ]]; then + PASS=$((PASS + 1)) + echo " ✅ $name" + else + FAIL=$((FAIL + 1)) + FAILURES+=("$name: expected '$expected', got '$actual'") + echo " ❌ $name: expected '$expected', got '$actual'" + fi +} + +STACKS='["termix","monitoring","swag"]' + +echo "== classify-rollback-scope ==" + +# Stack-scoped: every changed path lives under a known stack dir. +expect_scope "single stack file" \ + "per-stack" '["termix/compose.yaml"]' "$STACKS" +expect_scope "two stacks" \ + "per-stack" '["termix/compose.yaml","monitoring/compose.yaml"]' "$STACKS" +expect_scope "nested file under stack" \ + "per-stack" '["swag/config/nginx.conf"]' "$STACKS" + +# Root-scoped: anything outside a known stack dir forces whole-tree. +expect_scope "compose.env at root" \ + "whole-tree" '["compose.env"]' "$STACKS" +expect_scope "mixed stack and root" \ + "whole-tree" '["termix/compose.yaml","compose.env"]' "$STACKS" +expect_scope "workflow file" \ + "whole-tree" '[".github/workflows/deploy.yml"]' "$STACKS" +expect_scope "unknown top-level dir" \ + "whole-tree" '["newstack/compose.yaml"]' "$STACKS" + +# Degenerate inputs default to the safe path. +expect_scope "empty changed list" \ + "whole-tree" '[]' "$STACKS" +expect_scope "empty string changed list" \ + "whole-tree" '' "$STACKS" +expect_scope "empty stack dirs" \ + "whole-tree" '["termix/compose.yaml"]' '[]' + +# A stack dir name that is a prefix of another must not match loosely. +expect_scope "prefix collision is not a match" \ + "whole-tree" '["termix-old/compose.yaml"]' "$STACKS" + +echo +echo "Passed: $PASS Failed: $FAIL" +if [[ $FAIL -gt 0 ]]; then + printf ' - %s\n' "${FAILURES[@]}" + exit 1 +fi +``` + +Make it executable: + +```bash +chmod +x scripts/testing/test-classify-rollback-scope.sh +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `./scripts/testing/test-classify-rollback-scope.sh` + +Expected: every case fails with `got ''`, because `classify-rollback-scope.sh` does not exist yet. Exit code 1. + +- [ ] **Step 3: Write the implementation** + +Create `scripts/deployment/classify-rollback-scope.sh`: + +```bash +#!/usr/bin/env bash +# Script Name: classify-rollback-scope.sh +# Purpose: Decide whether a failed deploy can be rolled back per-stack or +# requires the whole-tree reset. +# Usage: ./classify-rollback-scope.sh \ +# --changed-files '["termix/compose.yaml"]' \ +# --stack-dirs '["termix","monitoring"]' +# +# Emits: rollback_scope=per-stack|whole-tree (to $GITHUB_OUTPUT) +# +# A deploy is per-stack rollbackable only when EVERY changed path's first +# segment names a known stack directory. Anything else — compose.env, +# .github/**, a README, an unrecognised top-level dir — is repo-wide and +# cannot be undone by reverting one stack directory, so it falls back to the +# whole-tree reset. +# +# compose.env is the motivating case: it lives at the repo root and is passed +# to every stack via `op run --env-file`. A bad edit there (renamed var, dead +# 1Password reference) breaks stacks whose own directories never changed. +# +# Uncertainty always resolves to whole-tree. Never the reverse. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/common.sh +source "$SCRIPT_DIR/lib/common.sh" + +CHANGED_FILES="[]" +STACK_DIRS="[]" + +while [[ $# -gt 0 ]]; do + case $1 in + --changed-files) CHANGED_FILES="${2:-[]}"; shift 2 ;; + --stack-dirs) STACK_DIRS="${2:-[]}"; shift 2 ;; + *) + log_error "Unknown argument: $1" + exit 1 + ;; + esac +done + +# Normalise empty/absent inputs to empty JSON arrays. +[[ -z "$CHANGED_FILES" ]] && CHANGED_FILES="[]" +[[ -z "$STACK_DIRS" ]] && STACK_DIRS="[]" + +emit_whole_tree() { + log_info "Rollback scope: whole-tree ($1)" + set_github_output "rollback_scope" "whole-tree" + exit 0 +} + +# Malformed JSON from an upstream step must not crash the deploy; fall back. +if ! jq -e 'type == "array"' <<<"$CHANGED_FILES" >/dev/null 2>&1; then + emit_whole_tree "changed-files was not a JSON array" +fi +if ! jq -e 'type == "array"' <<<"$STACK_DIRS" >/dev/null 2>&1; then + emit_whole_tree "stack-dirs was not a JSON array" +fi + +changed_count=$(jq 'length' <<<"$CHANGED_FILES") +if [[ "$changed_count" -eq 0 ]]; then + emit_whole_tree "no changed-file list available" +fi + +dirs_count=$(jq 'length' <<<"$STACK_DIRS") +if [[ "$dirs_count" -eq 0 ]]; then + emit_whole_tree "no known stack directories" +fi + +# Split each path on the first "/" and require the segment to be an exact +# member of the stack-dir set. Exact membership (not prefix matching) is what +# keeps "termix-old/…" from being mistaken for the "termix" stack. +# +# A path with no "/" is a root-level file: its first segment is the whole +# path, which will not match any stack dir, so it correctly forces whole-tree. +outside=$(jq -r --argjson dirs "$STACK_DIRS" ' + [ .[] | select((split("/")[0]) as $seg | ($dirs | index($seg)) == null) ] + | .[]' <<<"$CHANGED_FILES") + +if [[ -n "$outside" ]]; then + log_info "Paths outside known stack directories:" + while IFS= read -r p; do + [[ -n "$p" ]] && log_info " - $p" + done <<<"$outside" + emit_whole_tree "changes touch repo-root or unknown paths" +fi + +log_success "Rollback scope: per-stack (all changes confined to stack directories)" +set_github_output "rollback_scope" "per-stack" +``` + +Make it executable: + +```bash +chmod +x scripts/deployment/classify-rollback-scope.sh +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `./scripts/testing/test-classify-rollback-scope.sh` + +Expected: `Passed: 11 Failed: 0`, exit code 0. + +- [ ] **Step 5: Run shellcheck** + +Run: `shellcheck -x scripts/deployment/classify-rollback-scope.sh scripts/testing/test-classify-rollback-scope.sh` + +Expected: no output, exit code 0. This is the same invocation `.github/workflows/workflow-lint.yml:71` uses, so a clean run here means CI will be clean. + +- [ ] **Step 6: Commit** + +```bash +git add scripts/deployment/classify-rollback-scope.sh scripts/testing/test-classify-rollback-scope.sh +git commit -m "feat(deploy): add rollback scope classifier + +Decides per-stack vs whole-tree rollback from the changed-file list. +Uncertainty always resolves to whole-tree." +``` + +--- + +## Task 2: Wire the classifier into `prepare` + +**Files:** +- Modify: `.github/workflows/deploy.yml` (job outputs at `:65-80`; new step after `Detect stack changes` at `:165-186`) + +- [ ] **Step 1: Add the job output** + +In the `prepare` job's `outputs:` block, after the `critical_stacks` line, add: + +```yaml + rollback_scope: ${{ steps.classify-scope.outputs.rollback_scope }} +``` + +- [ ] **Step 2: Add the classifier step** + +Insert this step **after** the `Detect stack changes` step and **before** `Detect critical stacks`: + +```yaml + - name: Classify rollback scope + id: classify-scope + # Runs after detect-changes because it needs removed_stacks: a removed + # stack's directory is gone from the target tree, so discover-stacks + # never sees it. Without it in the known-dirs set, every stack deletion + # would look like a root-level change and force whole-tree rollback. + env: + CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files != '' && steps.changed-files.outputs.all_changed_files || '[]' }} + ACTIVE_STACKS: ${{ steps.discover-stacks.outputs.stacks }} + DISABLED_STACKS: ${{ steps.discover-stacks.outputs.disabled_stacks }} + REMOVED_STACKS: ${{ steps.detect-changes.outputs.removed_stacks || '[]' }} + run: | + set -euo pipefail + stack_dirs=$(jq -cn \ + --argjson a "$ACTIVE_STACKS" \ + --argjson b "$DISABLED_STACKS" \ + --argjson c "$REMOVED_STACKS" \ + '($a + $b + $c) | unique') + ./.compose-workflow/scripts/deployment/classify-rollback-scope.sh \ + --changed-files "$CHANGED_FILES" \ + --stack-dirs "$stack_dirs" +``` + +Note the `./.compose-workflow/` prefix: `prepare` checks the workflow's own repo out to that path (`deploy.yml:90-94`) pinned to `job.workflow_sha`. Every other script call in this job uses the same prefix — match it. + +- [ ] **Step 3: Verify `all_changed_files` is the right output name** + +Run: `grep -n "changed-files.outputs" .github/workflows/deploy.yml` + +Expected: existing uses are `deleted_files` and `added_files`. Confirm `tj-actions/changed-files@v47` also exposes `all_changed_files` with `json: true` set (it does — the action's `json: true` input applies to every file-list output). If the JSON array does not materialise at runtime, the classifier's array-type guard emits `whole-tree` and the deploy behaves exactly as it does today. The failure mode is safe. + +- [ ] **Step 4: Lint the workflow** + +Run: +```bash +yamllint --strict --config-file .yamllint .github/workflows/deploy.yml +actionlint .github/workflows/deploy.yml +``` + +Expected: no output from either, exit code 0. + +- [ ] **Step 5: Commit** + +```bash +git add .github/workflows/deploy.yml +git commit -m "feat(deploy): expose rollback_scope from prepare" +``` + +--- + +## Task 3: `health-check` reports which stacks failed + +**Files:** +- Modify: `.github/workflows/deploy.yml` (`health-check` outputs block; the `failed=()` handling at the end of step `h`) + +Why this is its own task: `health-check` currently emits only `status`. A bad image release most often fails *health*, not *deploy* — so without this output the per-stack path would have an empty culprit list on exactly the case this whole change exists to handle, and would silently degrade to whole-tree. + +- [ ] **Step 1: Add the output declaration** + +In the `health-check` job's `outputs:` block, alongside `status`: + +```yaml + failed_stacks: ${{ steps.h.outputs.failed_stacks }} +``` + +- [ ] **Step 2: Emit the list before exiting** + +Replace the tail of step `h` (currently the `if [[ ${#failed[@]} -gt 0 ]]` block) with: + +```bash + if [[ ${#failed[@]} -gt 0 ]]; then + # Emit the culprit list BEFORE `exit 1` — a step that exits + # non-zero still has its already-written $GITHUB_OUTPUT honoured, + # but nothing written after the exit would be. + json=$(printf '"%s",' "${failed[@]}" | sed 's/,$//') + echo "failed_stacks=[$json]" >> "$GITHUB_OUTPUT" + echo "status=failed" >> "$GITHUB_OUTPUT" + exit 1 + fi + echo "failed_stacks=[]" >> "$GITHUB_OUTPUT" + echo "status=healthy" >> "$GITHUB_OUTPUT" +``` + +The `printf`/`sed` JSON construction is copied verbatim from the `deploy-existing` step (`deploy.yml:424-425`) so both jobs build their arrays identically. Stack names are already constrained to `^[a-zA-Z0-9._-]+$` upstream, so no escaping is needed. + +- [ ] **Step 3: Lint** + +Run: +```bash +yamllint --strict --config-file .yamllint .github/workflows/deploy.yml +actionlint .github/workflows/deploy.yml +``` + +Expected: clean. + +- [ ] **Step 4: Commit** + +```bash +git add .github/workflows/deploy.yml +git commit -m "feat(deploy): emit failed_stacks from health-check" +``` + +--- + +## Task 4: Per-stack rollback branch + +**Files:** +- Modify: `.github/workflows/deploy.yml` (`rollback` job, currently `:684-748`) + +This is the highest-risk task in the plan. The rollback job is what runs when things are *already* broken; a bug here turns a one-stack outage into a fleet outage. + +- [ ] **Step 1: Extend the job's `needs` and `env`** + +The `rollback` job currently declares `needs: [prepare, deploy, health-check]` — unchanged. Add to its `env:` block: + +```yaml + ROLLBACK_SCOPE: ${{ needs.prepare.outputs.rollback_scope || 'whole-tree' }} + DEPLOY_EXISTING_FAILED: ${{ needs.deploy.outputs.existing_failed_stacks || '[]' }} + DEPLOY_NEW_FAILED: ${{ needs.deploy.outputs.new_failed_stacks || '[]' }} + HEALTH_FAILED: ${{ needs.health-check.outputs.failed_stacks || '[]' }} + NEW_STACKS: ${{ needs.prepare.outputs.new_stacks || '[]' }} +``` + +The `|| 'whole-tree'` default matters: if `prepare` is ever changed such that the classifier step is skipped, the expression yields empty and this restores today's behavior rather than an empty scope string that matches neither branch. + +- [ ] **Step 2: Add the culprit-resolution step as the job's first step** + +```yaml + - name: Resolve rollback plan + id: plan + run: | + set -euo pipefail + culprits=$(jq -cn \ + --argjson a "$DEPLOY_EXISTING_FAILED" \ + --argjson b "$DEPLOY_NEW_FAILED" \ + --argjson c "$HEALTH_FAILED" \ + '($a + $b + $c) | unique') + count=$(jq 'length' <<<"$culprits") + + # Per-stack requires BOTH a stack-confined change set AND a known + # culprit. An empty culprit list means the failure was not attributed + # to any specific stack (a teardown failure, an infrastructure error, + # a timeout before any stack was named) — there is nothing to scope + # to, so the whole-tree reset is the only correct response. + if [[ "$ROLLBACK_SCOPE" == "per-stack" && "$count" -gt 0 ]]; then + mode="per-stack" + else + mode="whole-tree" + fi + + { + echo "mode=$mode" + echo "culprits=$culprits" + } >> "$GITHUB_OUTPUT" + echo "🔄 Rollback mode: $mode (culprits: $culprits, scope: $ROLLBACK_SCOPE)" +``` + +- [ ] **Step 3: Add the per-stack rollback step** + +Insert after `Resolve rollback plan`, before the existing `Tear down new stacks` step: + +```yaml + - name: Roll back failed stacks only + if: steps.plan.outputs.mode == 'per-stack' + env: + CULPRITS: ${{ steps.plan.outputs.culprits }} + run: | + set -euo pipefail + for stack in $(jq -r '.[]' <<<"$CULPRITS"); do + [[ "$stack" =~ ^[a-zA-Z0-9._-]+$ ]] || { + echo "::error::invalid stack name: $stack"; exit 1; } + + # Was this stack introduced by the failing deploy? If so it has no + # previous state to restore — tear it down and leave it gone, the + # same disposition the whole-tree path gives new stacks. + if jq -e --arg s "$stack" 'index($s) != null' <<<"$NEW_STACKS" >/dev/null; then + echo "🛑 Tearing down failed new stack $stack" + if [[ -f "$LIVE_REPO_PATH/$stack/compose.yaml" || -f "$LIVE_REPO_PATH/$stack/compose.yml" ]]; then + (cd "$LIVE_REPO_PATH/$stack" && docker compose down) \ + || echo "::warning::down failed for $stack" + fi + continue + fi + + echo "⏪ Reverting $stack to $PREVIOUS_SHA" + # Partial checkout: only this stack's directory moves back. HEAD + # stays at the target SHA and the tree is left dirty — that is + # fine, the next deploy's `git reset --hard $TARGET_REF` + # (deploy.yml:283) cleans it before anything else runs. + git -C "$LIVE_REPO_PATH" checkout "$PREVIOUS_SHA" -- "$stack/" + + [[ -f "$LIVE_REPO_PATH/$stack/compose.yaml" || -f "$LIVE_REPO_PATH/$stack/compose.yml" ]] || { + echo "::warning::no compose file for $stack after revert; skipping up" + continue + } + + # No --pull always and no --build: land on the locally-tagged + # previous image kept on disk by the prune policy, so recovery does + # not depend on a registry being reachable mid-incident. + # + # The `op run` wrapper is REQUIRED. Without it every ${VAR} in the + # compose file resolves to empty and the stack comes up + # misconfigured while reporting success. + (cd "$LIVE_REPO_PATH/$stack" && \ + op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ + docker compose up -d --quiet-pull --wait --remove-orphans) \ + || echo "::warning::per-stack rollback up failed for $stack" + done +``` + +- [ ] **Step 4: Gate the three existing whole-tree steps** + +Add `if: steps.plan.outputs.mode == 'whole-tree'` to each of the existing steps: + +| Step | Current `if:` | New `if:` | +|---|---|---| +| `Tear down new stacks (will not exist after reset)` | `needs.prepare.outputs.has_new_stacks == 'true'` | `steps.plan.outputs.mode == 'whole-tree' && needs.prepare.outputs.has_new_stacks == 'true'` | +| `Reset live tree to previous SHA` | *(none)* | `steps.plan.outputs.mode == 'whole-tree'` | +| `Redeploy stacks at previous SHA` | *(none)* | `steps.plan.outputs.mode == 'whole-tree'` | + +Change **only** the `if:` conditions. Leave the bodies of these three steps exactly as they are — they are the tested status quo and the fallback for every case the new path declines to handle. + +- [ ] **Step 5: Lint** + +Run: +```bash +yamllint --strict --config-file .yamllint .github/workflows/deploy.yml +actionlint .github/workflows/deploy.yml +``` + +Expected: clean. + +- [ ] **Step 6: Re-read the diff against the two invariants** + +Run: `git diff` + +Confirm by eye: +1. Every `docker compose up` you added is wrapped in `op run --no-masking --env-file=…`. +2. The three original whole-tree steps have gained an `if:` and nothing else. + +- [ ] **Step 7: Commit** + +```bash +git add .github/workflows/deploy.yml +git commit -m "feat(deploy): scope rollback to failed stacks when safe + +Per-stack rollback runs only when the change set is confined to stack +directories AND a culprit stack was identified. Every other case keeps +the existing whole-tree reset." +``` + +--- + +## Task 5: Report rollback scope in the Discord notification + +**Files:** +- Modify: `.github/workflows/deploy.yml` (`notify` job `env:` block at `:805-811`; pipeline line construction at `:897-902`) + +- [ ] **Step 1: Pass the plan into `notify`** + +Add to the `notify` job's `env:` block for the summary step: + +```yaml + ROLLBACK_MODE: ${{ needs.rollback.outputs.mode }} + ROLLBACK_CULPRITS: ${{ needs.rollback.outputs.culprits }} +``` + +This requires the `rollback` job to expose them. Add to the `rollback` job: + +```yaml + outputs: + mode: ${{ steps.plan.outputs.mode }} + culprits: ${{ steps.plan.outputs.culprits }} +``` + +- [ ] **Step 2: Extend the pipeline line** + +Replace the `rollback_line` construction: + +```bash + rollback_line="" + if [[ "$rollback_status" != "skipped" ]]; then + rb_icon="✅"; [[ "$rollback_status" != "success" ]] && rb_icon="❌" + rb_detail="" + if [[ "$ROLLBACK_MODE" == "per-stack" ]]; then + rb_names=$(jq -r '. | join(", ")' <<<"${ROLLBACK_CULPRITS:-[]}" 2>/dev/null || echo "") + rb_detail=" (per-stack: ${rb_names:-unknown})" + elif [[ "$ROLLBACK_MODE" == "whole-tree" ]]; then + rb_detail=" (whole-tree)" + fi + rollback_line=" → $rb_icon Rollback$rb_detail" + fi +``` + +A whole-tree rollback and a one-stack rollback are currently indistinguishable in the alert. That distinction is the first thing the reader needs: it is the difference between "one app is down" and "the entire fleet just moved backwards". + +- [ ] **Step 3: Lint** + +Run: +```bash +yamllint --strict --config-file .yamllint .github/workflows/deploy.yml +actionlint .github/workflows/deploy.yml +``` + +Expected: clean. + +- [ ] **Step 4: Commit** + +```bash +git add .github/workflows/deploy.yml +git commit -m "feat(deploy): report rollback scope and culprits in notification" +``` + +--- + +## Task 6: The `quarantine-image` skill + +**Files:** +- Create: `~/.claude/skills/quarantine-image/SKILL.md` + +This file lives **outside this repository**, in the user's skills directory, matching `~/.claude/skills/renovate-trigger/SKILL.md`. It is a single markdown file: frontmatter, prose, inline bash recipes. No helper scripts, no plugin packaging. Read `renovate-trigger/SKILL.md` first and match its voice and structure. + +- [ ] **Step 1: Read the reference skill** + +Run: `cat ~/.claude/skills/renovate-trigger/SKILL.md` + +Note the conventions: `name` + `description` frontmatter where the description enumerates trigger phrases; `## Prerequisites`; one `##` section per operation; explicit guidance on what to do when a step finds nothing. + +- [ ] **Step 2: Write the skill** + +Create `~/.claude/skills/quarantine-image/SKILL.md` covering, in order: + +**Frontmatter.** `name: quarantine-image`. The `description` must enumerate trigger phrases: "quarantine an image", "block a bad image version", "this update broke the deploy", "stop Renovate from re-proposing", "revert and block", "unblock an image version". + +**Why revert alone does not work.** State the mechanism up front, because it is the reason the skill exists: `compose-workflow/default.json` groups minor/patch/digest updates into `all-deps` with `automerge: true` and `minimumReleaseAge: "1 hour"`. A `git revert` makes the old version current again, Renovate sees the newer tag as available, and re-proposes and re-merges it within the hour. + +**The blocking mechanism, with the trap called out.** `allowedVersions` accepts a version *range* interpreted by the active versioning scheme, or a *regex* in forward slashes, negated as `!/…/`. Use the negated regex. **`!=release-2.7.1` will not work** — it is range syntax, and these repos pin images to custom `regex:` versioning schemes where range operators are not dependable. A negated regex filters the raw version string before any versioning scheme parses it, so one form covers `release-2.7.1`, `4.2.1-ls286`, and `2026-08-24` alike: + +```json +{ "matchPackageNames": ["ghcr.io/lukegus/termix"], + "allowedVersions": "!/^release-2\\.7\\.1$/" } +``` + +**Prerequisites.** Run from `~/Git/Compose`. `git` and `jq` available. `npx` available for config validation. + +**Locating the stack:** + +```bash +matches=$(ls -d docker-piwine*/"$STACK"/ 2>/dev/null) +count=$(wc -l <<<"$matches") +``` + +Zero matches or more than one is an error — ask the user which repo, never guess. + +**Finding the last-known-good image line.** Walk the file's history and take the previous `image:` line verbatim, tag *and* digest together: + +```bash +git -C "$REPO" log --format=%H -- "$STACK/compose.yaml" \ + | while read -r sha; do + line=$(git -C "$REPO" show "$sha:$STACK/compose.yaml" | grep -m1 "image: $PKG:") + case "$line" in *"$BAD_VERSION"*) continue ;; esac + echo "$line"; break + done +``` + +Emphasise: **never compose a `@sha256:` digest by hand.** Restoring a tag/digest pair that a real Renovate run already wrote is what keeps this consistent with the project's "Renovate owns digest pinning" rule. + +**Writing the block.** Edit the consuming repo's `.github/renovate.json` — never `compose-workflow/default.json`, since a bad `termix` build is not `docker-piwine-office`'s problem. Two cases, and the second is the one that bites: + +- No existing block for the package → append a new rule object. +- A block already exists → **widen the alternation in place**: `!/^release-(2\.7\.1|2\.8\.0)$/`. Do not append a second rule. Two `packageRules` entries matching the same package both apply and the later `allowedVersions` silently wins, discarding the earlier block. + +Keep the block rule **separate** from the package's existing `versioning` rule (see `docker-piwine/.github/renovate.json:19-23` for termix). Renovate merges matching rules in order so the two coexist, and separation lets `--unblock` delete a whole object rather than surgically editing a shared one. + +**Committing.** Both files in **one commit**. State why the ordering is load-bearing: the compose revert makes `2.7.0` current in the same commit that makes `2.7.1` invisible. Split across two commits there is a window where the pinned-current version is also the blocked version, which Renovate handles badly. + +**Validating before pushing:** + +```bash +npx --yes --package renovate -- renovate-config-validator "$REPO/.github/renovate.json" +``` + +**After pushing.** Do not trigger a Renovate run — point at the existing `renovate-trigger` skill. + +**`--unblock `.** Remove the package's rule object and its comment, commit, note that Renovate will offer the version again on its next run. Do not touch the compose file: by unblock time you want the newest version, not the one you blocked. + +**Cases to refuse rather than guess at:** + +| Case | Behavior | +|---|---| +| Stack name matches zero or multiple repos | Error; ask the user to disambiguate | +| No prior version in history | Error; nothing to revert to — tell the user to fix forward | +| `image:` uses a floating tag (`:latest`) | Error; no version to block — the answer is a digest pin, not a quarantine | + +- [ ] **Step 3: Verify the skill is discoverable** + +Run: `ls ~/.claude/skills/quarantine-image/SKILL.md && head -5 ~/.claude/skills/quarantine-image/SKILL.md` + +Expected: the file exists and the frontmatter parses (opening `---`, `name:`, `description:`, closing `---`). The skill will be listed in a new Claude Code session. + +- [ ] **Step 4: Dry-run the recipes against the real termix history** + +Do not commit anything. Verify each recipe returns what the skill claims: + +```bash +cd ~/Git/Compose +# Stack resolution finds exactly one repo +ls -d docker-piwine*/termix/ 2>/dev/null +# The previous image line is recoverable verbatim, with its digest +git -C docker-piwine log --format=%H -- termix/compose.yaml | head -5 +git -C docker-piwine show 65606eb:termix/compose.yaml | grep -m1 'image: ghcr.io/lukegus/termix' +``` + +Expected: one directory; a list of SHAs; an `image:` line pinned to `release-2.7.0` **with** an `@sha256:` digest. If the digest is absent, the recipe in Step 2 needs adjusting before the skill is trusted. + +- [ ] **Step 5: Verify a block validates** + +Build a throwaway copy and run the real validator against it: + +```bash +cp docker-piwine/.github/renovate.json /tmp/rv.json +jq '.packageRules += [{"matchPackageNames":["ghcr.io/lukegus/termix"],"allowedVersions":"!/^release-2\\.7\\.1$/"}]' \ + /tmp/rv.json > /tmp/rv-blocked.json +npx --yes --package renovate -- renovate-config-validator /tmp/rv-blocked.json +``` + +Expected: the validator reports the config is valid. If it rejects the `allowedVersions` string, stop — the escaping in the skill is wrong and must be fixed before Task 6 is considered done. + +- [ ] **Step 6: Commit (this repo's docs only)** + +The skill file lives outside this repo and is not committed here. Nothing to commit for this task unless the dry-run revealed a spec inaccuracy — in which case amend the spec and commit that. + +--- + +## Task 7: End-to-end validation on a real host + +**Files:** none — this is a live exercise of the deployed workflow. + +Everything before this is unit-tested or lint-clean, but the per-stack path has never actually run. It touches a live Docker host during an incident, which is not a state to first exercise during a real incident. + +**Caveat on where to test.** The spec suggested `docker-piwine-office`, but that repo runs only `dozzle` and `portainer` — with `auto-detect-critical` both are likely critical, so there is no safe stack to break there. Prefer a genuinely non-critical stack on `docker-piwine`. Confirm which stacks are critical first: + +```bash +cd ~/Git/Compose/compose-workflow +./scripts/deployment/detect-critical-stacks.sh --stacks "$(ls -d ../docker-piwine/*/ | xargs -n1 basename | tr '\n' ' ')" --repo-dir ../docker-piwine +``` + +Pick a stack that is **not** in the resulting list. Confirm the choice with the user before breaking anything. + +- [ ] **Step 1: Merge this branch so the runner picks up the new workflow** + +Callers pin `owine/compose-workflow/.github/workflows/deploy.yml@main`, so the change must be on `main` to take effect. Open a PR, let `workflow-lint` pass, merge. + +- [ ] **Step 2: Exercise the per-stack path** + +In `docker-piwine`, on a branch, point the chosen non-critical stack's `image:` at a tag that cannot start (a nonexistent tag is the cleanest — it fails fast at pull). Change **nothing else** — the commit must touch only that stack's directory, or the classifier will correctly choose whole-tree and you will not be testing the new path. + +Merge it and watch the deploy run. + +Expected: +- `prepare` logs `Rollback scope: per-stack`. +- `rollback` logs `Rollback mode: per-stack` with the broken stack as the sole culprit. +- The three whole-tree steps show as **skipped**. +- `git -C status` on the runner shows only that stack's directory modified. +- Every other stack is still running at the new SHA — verify at least two by hand. +- Discord shows `❌ Rollback (per-stack: )`. + +- [ ] **Step 3: Exercise the whole-tree fallback** + +Repeat, but include a trivial `compose.env` edit (add a comment line) in the same commit alongside the broken image. + +Expected: +- `prepare` logs `Rollback scope: whole-tree` and names `compose.env` as the path outside stack directories. +- `rollback` logs `Rollback mode: whole-tree`; the per-stack step is **skipped**; the reset runs. +- Behavior is identical to today. +- Discord shows `❌ Rollback (whole-tree)`. + +- [ ] **Step 4: Restore** + +Revert both test commits in `docker-piwine` and confirm a clean green deploy before walking away. + +- [ ] **Step 5: Exercise the skill for real** + +With a broken version still on `main` from Step 2 (or by re-landing it), run: + +``` +/quarantine-image "e2e validation of quarantine-image" +``` + +Expected: one commit touching exactly two files; the compose line back at the previous tag **and** digest; a negated-regex `allowedVersions` rule in `docker-piwine/.github/renovate.json`; the config validator passing. Then confirm on the Dependency Dashboard that the blocked version no longer appears as an available update. + +Finish with `/quarantine-image --unblock ` and confirm the rule is gone. + +--- + +## Out of scope + +Named here so they are not quietly added: + +- Automatic quarantine from the deploy workflow. The workflow cannot distinguish a bad image from a bad healthcheck, a bad config change, or a transient registry blip, and a false positive would block a good version. +- Changes to `minimumReleaseAge` or automerge policy in `default.json`. +- Expiry metadata or scheduled auditing of accumulated blocks. +- Wiring `scripts/testing/*.sh` into CI. Worth doing; not part of this change. From 88c62e4c858f44d560b04104341aa8db8128c7b9 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 18:55:17 -0500 Subject: [PATCH 03/13] feat(deploy): add rollback scope classifier Decides per-stack vs whole-tree rollback from the changed-file list. Uncertainty always resolves to whole-tree. --- scripts/deployment/classify-rollback-scope.sh | 90 +++++++++++++++++++ .../testing/test-classify-rollback-scope.sh | 76 ++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100755 scripts/deployment/classify-rollback-scope.sh create mode 100755 scripts/testing/test-classify-rollback-scope.sh diff --git a/scripts/deployment/classify-rollback-scope.sh b/scripts/deployment/classify-rollback-scope.sh new file mode 100755 index 0000000..c1effa8 --- /dev/null +++ b/scripts/deployment/classify-rollback-scope.sh @@ -0,0 +1,90 @@ +#!/usr/bin/env bash +# Script Name: classify-rollback-scope.sh +# Purpose: Decide whether a failed deploy can be rolled back per-stack or +# requires the whole-tree reset. +# Usage: ./classify-rollback-scope.sh \ +# --changed-files '["termix/compose.yaml"]' \ +# --stack-dirs '["termix","monitoring"]' +# +# Emits: rollback_scope=per-stack|whole-tree (to $GITHUB_OUTPUT) +# +# A deploy is per-stack rollbackable only when EVERY changed path's first +# segment names a known stack directory. Anything else — compose.env, +# .github/**, a README, an unrecognised top-level dir — is repo-wide and +# cannot be undone by reverting one stack directory, so it falls back to the +# whole-tree reset. +# +# compose.env is the motivating case: it lives at the repo root and is passed +# to every stack via `op run --env-file`. A bad edit there (renamed var, dead +# 1Password reference) breaks stacks whose own directories never changed. +# +# Uncertainty always resolves to whole-tree. Never the reverse. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/common.sh +source "$SCRIPT_DIR/lib/common.sh" + +CHANGED_FILES="[]" +STACK_DIRS="[]" + +while [[ $# -gt 0 ]]; do + case $1 in + --changed-files) CHANGED_FILES="${2:-[]}"; shift 2 ;; + --stack-dirs) STACK_DIRS="${2:-[]}"; shift 2 ;; + *) + log_error "Unknown argument: $1" + exit 1 + ;; + esac +done + +# Normalise empty/absent inputs to empty JSON arrays. +[[ -z "$CHANGED_FILES" ]] && CHANGED_FILES="[]" +[[ -z "$STACK_DIRS" ]] && STACK_DIRS="[]" + +emit_whole_tree() { + log_info "Rollback scope: whole-tree ($1)" + set_github_output "rollback_scope" "whole-tree" + exit 0 +} + +# Malformed JSON from an upstream step must not crash the deploy; fall back. +if ! jq -e 'type == "array"' <<<"$CHANGED_FILES" >/dev/null 2>&1; then + emit_whole_tree "changed-files was not a JSON array" +fi +if ! jq -e 'type == "array"' <<<"$STACK_DIRS" >/dev/null 2>&1; then + emit_whole_tree "stack-dirs was not a JSON array" +fi + +changed_count=$(jq 'length' <<<"$CHANGED_FILES") +if [[ "$changed_count" -eq 0 ]]; then + emit_whole_tree "no changed-file list available" +fi + +dirs_count=$(jq 'length' <<<"$STACK_DIRS") +if [[ "$dirs_count" -eq 0 ]]; then + emit_whole_tree "no known stack directories" +fi + +# Split each path on the first "/" and require the segment to be an exact +# member of the stack-dir set. Exact membership (not prefix matching) is what +# keeps "termix-old/…" from being mistaken for the "termix" stack. +# +# A path with no "/" is a root-level file: its first segment is the whole +# path, which will not match any stack dir, so it correctly forces whole-tree. +outside=$(jq -r --argjson dirs "$STACK_DIRS" ' + [ .[] | select((split("/")[0]) as $seg | ($dirs | index($seg)) == null) ] + | .[]' <<<"$CHANGED_FILES") + +if [[ -n "$outside" ]]; then + log_info "Paths outside known stack directories:" + while IFS= read -r p; do + [[ -n "$p" ]] && log_info " - $p" + done <<<"$outside" + emit_whole_tree "changes touch repo-root or unknown paths" +fi + +log_success "Rollback scope: per-stack (all changes confined to stack directories)" +set_github_output "rollback_scope" "per-stack" diff --git a/scripts/testing/test-classify-rollback-scope.sh b/scripts/testing/test-classify-rollback-scope.sh new file mode 100755 index 0000000..11b29fb --- /dev/null +++ b/scripts/testing/test-classify-rollback-scope.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# Unit tests for scripts/deployment/classify-rollback-scope.sh +# Pure input/output tests — no git repos, no docker. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" +CLASSIFY="$REPO_ROOT/scripts/deployment/classify-rollback-scope.sh" + +TMPROOT=$(mktemp -d -t classify-tests.XXXXXX) +trap 'rm -rf "$TMPROOT"' EXIT + +PASS=0 +FAIL=0 +FAILURES=() + +# expect_scope +expect_scope() { + local name="$1" expected="$2" changed="$3" dirs="$4" + local out actual + out=$(mktemp -p "$TMPROOT") + GITHUB_OUTPUT="$out" "$CLASSIFY" \ + --changed-files "$changed" \ + --stack-dirs "$dirs" \ + >/dev/null 2>&1 || true + actual=$(grep '^rollback_scope=' "$out" 2>/dev/null | cut -d= -f2- || echo "") + if [[ "$actual" == "$expected" ]]; then + PASS=$((PASS + 1)) + echo " ✅ $name" + else + FAIL=$((FAIL + 1)) + FAILURES+=("$name: expected '$expected', got '$actual'") + echo " ❌ $name: expected '$expected', got '$actual'" + fi +} + +STACKS='["termix","monitoring","swag"]' + +echo "== classify-rollback-scope ==" + +# Stack-scoped: every changed path lives under a known stack dir. +expect_scope "single stack file" \ + "per-stack" '["termix/compose.yaml"]' "$STACKS" +expect_scope "two stacks" \ + "per-stack" '["termix/compose.yaml","monitoring/compose.yaml"]' "$STACKS" +expect_scope "nested file under stack" \ + "per-stack" '["swag/config/nginx.conf"]' "$STACKS" + +# Root-scoped: anything outside a known stack dir forces whole-tree. +expect_scope "compose.env at root" \ + "whole-tree" '["compose.env"]' "$STACKS" +expect_scope "mixed stack and root" \ + "whole-tree" '["termix/compose.yaml","compose.env"]' "$STACKS" +expect_scope "workflow file" \ + "whole-tree" '[".github/workflows/deploy.yml"]' "$STACKS" +expect_scope "unknown top-level dir" \ + "whole-tree" '["newstack/compose.yaml"]' "$STACKS" + +# Degenerate inputs default to the safe path. +expect_scope "empty changed list" \ + "whole-tree" '[]' "$STACKS" +expect_scope "empty string changed list" \ + "whole-tree" '' "$STACKS" +expect_scope "empty stack dirs" \ + "whole-tree" '["termix/compose.yaml"]' '[]' + +# A stack dir name that is a prefix of another must not match loosely. +expect_scope "prefix collision is not a match" \ + "whole-tree" '["termix-old/compose.yaml"]' "$STACKS" + +echo +echo "Passed: $PASS Failed: $FAIL" +if [[ $FAIL -gt 0 ]]; then + printf ' - %s\n' "${FAILURES[@]}" + exit 1 +fi From 7c3d3003dabdacb95fa977e0dc45df2c0ecffad5 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 19:00:43 -0500 Subject: [PATCH 04/13] fix(deploy): close 3 unsafe-fallthrough gaps in rollback classifier Uncertainty must always resolve to whole-tree, never per-stack: a wrong whole-tree is merely wasteful, a wrong per-stack leaves a broken deploy partially un-rolled-back. Three input shapes violated that asymmetry: - Non-string array elements (numbers, null, nested arrays) reached the unguarded split()/index() jq pipeline and crashed the script under set -euo pipefail before any output was written (rc=5, no output). - An empty-string element (e.g. from a trailing-newline git diff pipeline) vacuously satisfied the "no paths outside a stack dir" check, landing on the unsafe per-stack side instead of whole-tree. - A flag given as the final argument (no value) tripped set -e in the shift 2 parsing and exited 1 with no output, instead of falling through to whole-tree. Tighten both array guards to require every element be a non-empty string, make argument parsing tolerate a missing trailing value, and correct a comment that inaccurately described the root-level-file fallthrough behavior. Added 6 test cases (17 total, 11 original unchanged). --- scripts/deployment/classify-rollback-scope.sh | 27 +++++++++++----- .../testing/test-classify-rollback-scope.sh | 32 +++++++++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/scripts/deployment/classify-rollback-scope.sh b/scripts/deployment/classify-rollback-scope.sh index c1effa8..20c8017 100755 --- a/scripts/deployment/classify-rollback-scope.sh +++ b/scripts/deployment/classify-rollback-scope.sh @@ -31,8 +31,8 @@ STACK_DIRS="[]" while [[ $# -gt 0 ]]; do case $1 in - --changed-files) CHANGED_FILES="${2:-[]}"; shift 2 ;; - --stack-dirs) STACK_DIRS="${2:-[]}"; shift 2 ;; + --changed-files) CHANGED_FILES="${2:-}"; shift; [[ $# -gt 0 ]] && shift ;; + --stack-dirs) STACK_DIRS="${2:-}"; shift; [[ $# -gt 0 ]] && shift ;; *) log_error "Unknown argument: $1" exit 1 @@ -51,11 +51,17 @@ emit_whole_tree() { } # Malformed JSON from an upstream step must not crash the deploy; fall back. -if ! jq -e 'type == "array"' <<<"$CHANGED_FILES" >/dev/null 2>&1; then - emit_whole_tree "changed-files was not a JSON array" +# Every element must be a non-empty string: a non-string element (number, +# null, nested array) would blow up the split()/index() pipeline below under +# `set -euo pipefail`, killing the script before any output is written — and +# an empty-string element would vacuously satisfy the "no paths outside a +# stack dir" check further down, silently landing on the unsafe per-stack +# side. Both must be caught here, before they reach the pipeline. +if ! jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' <<<"$CHANGED_FILES" >/dev/null 2>&1; then + emit_whole_tree "changed-files was not an array of non-empty strings" fi -if ! jq -e 'type == "array"' <<<"$STACK_DIRS" >/dev/null 2>&1; then - emit_whole_tree "stack-dirs was not a JSON array" +if ! jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' <<<"$STACK_DIRS" >/dev/null 2>&1; then + emit_whole_tree "stack-dirs was not an array of non-empty strings" fi changed_count=$(jq 'length' <<<"$CHANGED_FILES") @@ -72,8 +78,13 @@ fi # member of the stack-dir set. Exact membership (not prefix matching) is what # keeps "termix-old/…" from being mistaken for the "termix" stack. # -# A path with no "/" is a root-level file: its first segment is the whole -# path, which will not match any stack dir, so it correctly forces whole-tree. +# A path with no "/" is a root-level file, so its first segment is the whole +# path. If that path happens to be spelled identically to a known stack +# directory name (e.g. a root-level file literally named "termix"), it WILL +# match and count as per-stack — this is the same exact first-segment +# membership rule applied uniformly, not a special case. In practice repo +# root files (compose.env, README, .github/**) don't collide with stack dir +# names, but this is not a semantic guarantee. outside=$(jq -r --argjson dirs "$STACK_DIRS" ' [ .[] | select((split("/")[0]) as $seg | ($dirs | index($seg)) == null) ] | .[]' <<<"$CHANGED_FILES") diff --git a/scripts/testing/test-classify-rollback-scope.sh b/scripts/testing/test-classify-rollback-scope.sh index 11b29fb..7a72374 100755 --- a/scripts/testing/test-classify-rollback-scope.sh +++ b/scripts/testing/test-classify-rollback-scope.sh @@ -68,6 +68,38 @@ expect_scope "empty stack dirs" \ expect_scope "prefix collision is not a match" \ "whole-tree" '["termix-old/compose.yaml"]' "$STACKS" +# Non-string / empty-string elements must not crash the script or silently +# fall through to the unsafe per-stack side. +expect_scope "non-string element" \ + "whole-tree" '[1]' "$STACKS" +expect_scope "null element" \ + "whole-tree" '[null]' "$STACKS" +expect_scope "empty-string element" \ + "whole-tree" '[""]' "$STACKS" +expect_scope "nested-array element" \ + "whole-tree" '[["a"]]' "$STACKS" +expect_scope "non-string stack dir" \ + "whole-tree" '["termix/compose.yaml"]' '[1]' + +# A trailing flag with no value must not crash the script (defeats the +# shift-2 default-value bug) — it must fall through to whole-tree, rc=0. +trailing_flag_case() { + local name="trailing flag with no value" out actual rc + out=$(mktemp -p "$TMPROOT") + rc=0 + GITHUB_OUTPUT="$out" "$CLASSIFY" --changed-files >/dev/null 2>&1 || rc=$? + actual=$(grep '^rollback_scope=' "$out" 2>/dev/null | cut -d= -f2- || echo "") + if [[ "$actual" == "whole-tree" && "$rc" -eq 0 ]]; then + PASS=$((PASS + 1)) + echo " ✅ $name" + else + FAIL=$((FAIL + 1)) + FAILURES+=("$name: expected 'whole-tree' rc=0, got '$actual' rc=$rc") + echo " ❌ $name: expected 'whole-tree' rc=0, got '$actual' rc=$rc" + fi +} +trailing_flag_case + echo echo "Passed: $PASS Failed: $FAIL" if [[ $FAIL -gt 0 ]]; then From 50757618035730583962f9fcdc8c862aa888e440 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 19:08:15 -0500 Subject: [PATCH 05/13] fix(deploy): rollback classifier code-quality review fixes Addresses 6 findings from code-quality review of 7c3d300: - Comment both empty-list guards explicitly as load-bearing vs cosmetic: deleting the changed-files-empty guard inverts the safe default (jq's filter is vacuously true over []), the stack-dirs-empty guard only buys a clearer reason string. - Close the mid-argv missing-value hole for both flags (--changed-files --stack-dirs '[...]' no longer exits 1) by checking $# instead of shift 2, which also removes the `shift; [[ $# -gt 0 ]] && shift` construct a maintainer could "simplify" back into the exact bug already fixed. Distinguishing "missing value" from "explicit empty string" value requires checking argument count, not ${2:-} content -- the latter can't tell unset from empty. - Rework the test harness so every case asserts an exit code via a new expect_case helper; expect_scope becomes a thin 2-flag wrapper so all existing call sites stay unchanged. Verified the new rc assertion can actually fail: temporarily injected `exit 3` on the per-stack success path, confirmed 3 cases went red, reverted. - Add coverage for the unknown-flag exit-1 path, the most opinionated behavior in the file (invocation errors fail loudly; malformed data degrades to whole-tree) and previously untested. - Extract the duplicated is_string_array predicate so tightening one guard can't accidentally miss its twin. - Use log_warning (not log_info) for genuine data anomalies, with a truncated echo of the offending input so an operator doesn't have to dig through the upstream step's output. 19 test cases total (11 original unchanged, 8 new). Uncertainty still always resolves to whole-tree, never the reverse. --- scripts/deployment/classify-rollback-scope.sh | 59 ++++++++++++++--- .../testing/test-classify-rollback-scope.sh | 64 ++++++++++--------- 2 files changed, 86 insertions(+), 37 deletions(-) diff --git a/scripts/deployment/classify-rollback-scope.sh b/scripts/deployment/classify-rollback-scope.sh index 20c8017..42359ae 100755 --- a/scripts/deployment/classify-rollback-scope.sh +++ b/scripts/deployment/classify-rollback-scope.sh @@ -19,6 +19,13 @@ # 1Password reference) breaks stacks whose own directories never changed. # # Uncertainty always resolves to whole-tree. Never the reverse. +# +# Invocation errors (unknown flag) are distinct from malformed runtime data: +# an unknown flag means the caller is wired up wrong and exits 1 loudly, so +# the mistake surfaces immediately instead of quietly deploying with the +# wrong scope every time. Malformed *data* (bad JSON, non-string elements, +# a missing value) instead degrades safely to whole-tree, rc=0 — the deploy +# should not fail just because the classifier couldn't classify. set -euo pipefail @@ -31,8 +38,21 @@ STACK_DIRS="[]" while [[ $# -gt 0 ]]; do case $1 in - --changed-files) CHANGED_FILES="${2:-}"; shift; [[ $# -gt 0 ]] && shift ;; - --stack-dirs) STACK_DIRS="${2:-}"; shift; [[ $# -gt 0 ]] && shift ;; + --changed-files) + # A missing value — flag at end of argv, or immediately followed by + # another flag — leaves this empty, which normalises to [] -> whole-tree. + # Never consume the next flag, never `shift 2` past the end. Check $# + # (not the value of $2) so an explicit empty-string argument is still + # consumed as a value — ${2:-} can't tell "unset" from "set to ''". + CHANGED_FILES="" + if [[ $# -ge 2 && $2 != --* ]]; then CHANGED_FILES=$2; shift; fi + shift + ;; + --stack-dirs) + STACK_DIRS="" + if [[ $# -ge 2 && $2 != --* ]]; then STACK_DIRS=$2; shift; fi + shift + ;; *) log_error "Unknown argument: $1" exit 1 @@ -44,31 +64,54 @@ done [[ -z "$CHANGED_FILES" ]] && CHANGED_FILES="[]" [[ -z "$STACK_DIRS" ]] && STACK_DIRS="[]" +# emit_whole_tree [severity] +# severity defaults to "info" for expected/normal degradations (empty lists, +# no known stacks, changes outside a stack dir). Pass "warn" for genuine +# anomalies in the upstream data (malformed JSON) so an operator notices +# without having to go dig through the upstream step's output. emit_whole_tree() { - log_info "Rollback scope: whole-tree ($1)" + local reason="$1" severity="${2:-info}" + if [[ "$severity" == "warn" ]]; then + log_warning "Rollback scope: whole-tree ($reason)" + else + log_info "Rollback scope: whole-tree ($reason)" + fi set_github_output "rollback_scope" "whole-tree" exit 0 } +# is_string_array : true iff is a JSON array whose every element +# is a non-empty string. Shared by both input guards below so tightening the +# predicate can't be done to one and forgotten on the other. +is_string_array() { + jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' <<<"$1" >/dev/null 2>&1 +} + # Malformed JSON from an upstream step must not crash the deploy; fall back. # Every element must be a non-empty string: a non-string element (number, # null, nested array) would blow up the split()/index() pipeline below under # `set -euo pipefail`, killing the script before any output is written — and # an empty-string element would vacuously satisfy the "no paths outside a # stack dir" check further down, silently landing on the unsafe per-stack -# side. Both must be caught here, before they reach the pipeline. -if ! jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' <<<"$CHANGED_FILES" >/dev/null 2>&1; then - emit_whole_tree "changed-files was not an array of non-empty strings" +# side. Both must be caught here, before they reach the pipeline. These are +# genuine anomalies (not a normal empty-list degradation), hence "warn". +if ! is_string_array "$CHANGED_FILES"; then + emit_whole_tree "changed-files was not an array of non-empty strings: ${CHANGED_FILES:0:200}" warn fi -if ! jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' <<<"$STACK_DIRS" >/dev/null 2>&1; then - emit_whole_tree "stack-dirs was not an array of non-empty strings" +if ! is_string_array "$STACK_DIRS"; then + emit_whole_tree "stack-dirs was not an array of non-empty strings: ${STACK_DIRS:0:200}" warn fi +# An empty changed-file list must NOT be read as "nothing outside a stack dir": +# the jq filter below is vacuously true over [], which yields per-stack. This +# guard is load-bearing — deleting it inverts the safe default. Do not remove. changed_count=$(jq 'length' <<<"$CHANGED_FILES") if [[ "$changed_count" -eq 0 ]]; then emit_whole_tree "no changed-file list available" fi +# Empty stack-dirs would already fall out as whole-tree (index() over [] is null +# for every path). This guard exists only for the clearer reason string. dirs_count=$(jq 'length' <<<"$STACK_DIRS") if [[ "$dirs_count" -eq 0 ]]; then emit_whole_tree "no known stack directories" diff --git a/scripts/testing/test-classify-rollback-scope.sh b/scripts/testing/test-classify-rollback-scope.sh index 7a72374..0dfa99d 100755 --- a/scripts/testing/test-classify-rollback-scope.sh +++ b/scripts/testing/test-classify-rollback-scope.sh @@ -14,26 +14,36 @@ PASS=0 FAIL=0 FAILURES=() -# expect_scope -expect_scope() { - local name="$1" expected="$2" changed="$3" dirs="$4" - local out actual +# expect_case "> -- +# Runs the classifier with an arbitrary argv and asserts BOTH the emitted +# rollback_scope output AND the exit code. Asserting rc matters: a case that +# only checks output would miss a regression that writes the right value and +# then exits nonzero. +expect_case() { + local name="$1" expected="$2" expected_rc="$3" + shift 4 # drop name, expected, expected_rc, and the "--" separator + local out actual rc=0 out=$(mktemp -p "$TMPROOT") - GITHUB_OUTPUT="$out" "$CLASSIFY" \ - --changed-files "$changed" \ - --stack-dirs "$dirs" \ - >/dev/null 2>&1 || true + GITHUB_OUTPUT="$out" "$CLASSIFY" "$@" >/dev/null 2>&1 || rc=$? actual=$(grep '^rollback_scope=' "$out" 2>/dev/null | cut -d= -f2- || echo "") - if [[ "$actual" == "$expected" ]]; then + if [[ "$actual" == "$expected" && "$rc" == "$expected_rc" ]]; then PASS=$((PASS + 1)) echo " ✅ $name" else FAIL=$((FAIL + 1)) - FAILURES+=("$name: expected '$expected', got '$actual'") - echo " ❌ $name: expected '$expected', got '$actual'" + FAILURES+=("$name: expected scope='$expected' rc=$expected_rc, got scope='$actual' rc=$rc") + echo " ❌ $name: expected scope='$expected' rc=$expected_rc, got scope='$actual' rc=$rc" fi } +# expect_scope +# Thin two-flag wrapper over expect_case, always asserting rc=0. All existing +# call sites below stay textually unchanged and now also assert exit code. +expect_scope() { + local name="$1" expected="$2" changed="$3" dirs="$4" + expect_case "$name" "$expected" 0 -- --changed-files "$changed" --stack-dirs "$dirs" +} + STACKS='["termix","monitoring","swag"]' echo "== classify-rollback-scope ==" @@ -81,24 +91,20 @@ expect_scope "nested-array element" \ expect_scope "non-string stack dir" \ "whole-tree" '["termix/compose.yaml"]' '[1]' -# A trailing flag with no value must not crash the script (defeats the -# shift-2 default-value bug) — it must fall through to whole-tree, rc=0. -trailing_flag_case() { - local name="trailing flag with no value" out actual rc - out=$(mktemp -p "$TMPROOT") - rc=0 - GITHUB_OUTPUT="$out" "$CLASSIFY" --changed-files >/dev/null 2>&1 || rc=$? - actual=$(grep '^rollback_scope=' "$out" 2>/dev/null | cut -d= -f2- || echo "") - if [[ "$actual" == "whole-tree" && "$rc" -eq 0 ]]; then - PASS=$((PASS + 1)) - echo " ✅ $name" - else - FAIL=$((FAIL + 1)) - FAILURES+=("$name: expected 'whole-tree' rc=0, got '$actual' rc=$rc") - echo " ❌ $name: expected 'whole-tree' rc=0, got '$actual' rc=$rc" - fi -} -trailing_flag_case +# Missing-value invocation shapes must not crash the script — they normalise +# to [] and fall through to whole-tree, rc=0. +expect_case "trailing flag with no value" \ + "whole-tree" 0 -- --changed-files +expect_case "missing value before next flag" \ + "whole-tree" 0 -- --changed-files --stack-dirs "$STACKS" + +# Invocation errors (unknown flag) are deliberately NOT degraded to +# whole-tree: they mean the caller is wired up wrong, and should fail loudly +# (rc=1, no output) rather than silently deploy with a scope decision nobody +# intended. This is the most opinionated behavior in the script — cover it so +# a well-meaning "make everything fail-safe" edit can't remove it unnoticed. +expect_case "unknown flag fails loudly" \ + "" 1 -- --bogus echo echo "Passed: $PASS Failed: $FAIL" From 1709915d33f0a5197740e7a98a6cb5131136f3e7 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 19:12:05 -0500 Subject: [PATCH 06/13] feat(deploy): expose rollback_scope from prepare Wire classify-rollback-scope.sh into the prepare job and expose its result as a job output for the (not-yet-wired) rollback job to consume. Also set escape_json: false on the tj-actions/changed-files step. That input defaults to true, which backslash-escapes every quote in the JSON outputs (e.g. all_changed_files becomes [\"x\"] instead of ["x"]) -- invalid JSON that jq can't parse. Every consumer of these outputs in this job (detect-stack-changes.sh and the new classifier step) pipes them through jq, so the escaped form silently broke input validation: the classifier's strict is_string_array guard rejected the malformed value and fell back to whole-tree every time, with no error surfaced anywhere. --- .github/workflows/deploy.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 8add820..b0ef8c2 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -78,6 +78,7 @@ jobs: disabled_stacks: ${{ steps.discover-stacks.outputs.disabled_stacks }} has_disabled_stacks: ${{ steps.discover-stacks.outputs.has_disabled_stacks }} critical_stacks: ${{ steps.detect-critical.outputs.critical_stacks }} + rollback_scope: ${{ steps.classify-scope.outputs.rollback_scope }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -123,6 +124,10 @@ jobs: uses: tj-actions/changed-files@9426d40962ed5378910ee2e21d5f8c6fcbf2dd96 # v47.0.6 with: json: true + # escape_json defaults to true, which backslash-escapes every quote + # and makes the output unparseable by jq. Every consumer here pipes + # these outputs into jq, so the escaped form is useless to us. + escape_json: false sha: ${{ inputs.target-ref }} base_sha: ${{ steps.previous-sha.outputs.previous_sha }} @@ -182,6 +187,28 @@ jobs: --removed-files "$REMOVED_FILES" \ --added-files "$ADDED_FILES" + - name: Classify rollback scope + id: classify-scope + # Runs after detect-changes because it needs removed_stacks: a removed + # stack's directory is gone from the target tree, so discover-stacks + # never sees it. Without it in the known-dirs set, every stack deletion + # would look like a root-level change and force whole-tree rollback. + env: + CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files != '' && steps.changed-files.outputs.all_changed_files || '[]' }} + ACTIVE_STACKS: ${{ steps.discover-stacks.outputs.stacks }} + DISABLED_STACKS: ${{ steps.discover-stacks.outputs.disabled_stacks }} + REMOVED_STACKS: ${{ steps.detect-changes.outputs.removed_stacks || '[]' }} + run: | + set -euo pipefail + stack_dirs=$(jq -cn \ + --argjson a "$ACTIVE_STACKS" \ + --argjson b "$DISABLED_STACKS" \ + --argjson c "$REMOVED_STACKS" \ + '($a + $b + $c) | unique') + ./.compose-workflow/scripts/deployment/classify-rollback-scope.sh \ + --changed-files "$CHANGED_FILES" \ + --stack-dirs "$stack_dirs" + - name: Detect critical stacks id: detect-critical if: inputs.auto-detect-critical From 9f2a8ae3e92d0bd23670ec75dbbbb07335fb14ec Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 19:24:30 -0500 Subject: [PATCH 07/13] feat(deploy): emit failed_stacks from health-check --- .github/workflows/deploy.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index b0ef8c2..56e8276 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -612,6 +612,7 @@ jobs: timeout-minutes: 5 outputs: status: ${{ steps.h.outputs.status }} + failed_stacks: ${{ steps.h.outputs.failed_stacks }} env: LIVE_REPO_PATH: ${{ inputs.live-repo-path }} CRITICAL_STACKS: ${{ inputs.auto-detect-critical && needs.prepare.outputs.critical_stacks || inputs.critical-services }} @@ -671,9 +672,15 @@ jobs: fi done if [[ ${#failed[@]} -gt 0 ]]; then + # Emit the culprit list BEFORE `exit 1` — a step that exits + # non-zero still has its already-written $GITHUB_OUTPUT honoured, + # but nothing written after the exit would be. + json=$(printf '"%s",' "${failed[@]}" | sed 's/,$//') + echo "failed_stacks=[$json]" >> "$GITHUB_OUTPUT" echo "status=failed" >> "$GITHUB_OUTPUT" exit 1 fi + echo "failed_stacks=[]" >> "$GITHUB_OUTPUT" echo "status=healthy" >> "$GITHUB_OUTPUT" docker-prune: From 159438fba7e4463ce980663ffa476e99a7f1d751 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 19:57:42 -0500 Subject: [PATCH 08/13] feat(deploy): scope rollback to failed stacks when safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-stack rollback runs only when the change set is confined to stack directories AND a culprit stack was identified. Every other case keeps the existing whole-tree reset. The governing principle for this job: in a recovery job, malformed input changes the SCOPE of the rollback, never whether one happens. `Resolve rollback plan` is the job's first step, so any hard failure there skips every subsequent step and no rollback runs at all — neither per-stack nor whole-tree — leaving production broken until a human intervenes. The tradeoff is therefore not "loud failure vs. silently wrong rollback" but "loud failure with production still down vs. whole-tree rollback with production restored". So every unusable input forces the conservative whole-tree path and raises a ::error:: plus a step-summary entry: recovery still runs, the regression still screams. That covers three classes of bad input, all validated in the plan step rather than at their point of use, because a list we cannot trust should keep us off the per-stack path entirely: - a stack list that is not an array of non-empty strings - NEW_STACKS specifically, since without it we cannot tell a new stack from an existing one - a culprit name failing the stack-name pattern. Degrading is also the safer security response: the whole-tree path never uses these names (it resets the tree and iterates prepare's own existing/removed lists, a different producer), so it discards the poisoned name instead of acting on it. The per-stack loop also tolerates an unrevertable culprit. `git checkout -- /` for a directory absent at that SHA is an unmatched pathspec and exits non-zero, which under `set -e` aborted the whole loop and stranded every remaining culprit. It now warns and continues, as does its defence-in-depth name check. --- .github/workflows/deploy.yml | 155 ++++++++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 56e8276..12f521b 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -723,13 +723,164 @@ jobs: && (needs.deploy.result == 'failure' || needs.health-check.result == 'failure') runs-on: [self-hosted, "${{ inputs.runner-label }}"] timeout-minutes: 15 + outputs: + mode: ${{ steps.plan.outputs.mode }} + culprits: ${{ steps.plan.outputs.culprits }} env: LIVE_REPO_PATH: ${{ inputs.live-repo-path }} PREVIOUS_SHA: ${{ needs.prepare.outputs.previous_sha }} OP_SERVICE_ACCOUNT_TOKEN: ${{ secrets.OP_SERVICE_ACCOUNT_TOKEN }} + ROLLBACK_SCOPE: ${{ needs.prepare.outputs.rollback_scope || 'whole-tree' }} + DEPLOY_EXISTING_FAILED: ${{ needs.deploy.outputs.existing_failed_stacks || '[]' }} + DEPLOY_NEW_FAILED: ${{ needs.deploy.outputs.new_failed_stacks || '[]' }} + HEALTH_FAILED: ${{ needs.health-check.outputs.failed_stacks || '[]' }} + NEW_STACKS: ${{ needs.prepare.outputs.new_stacks || '[]' }} steps: + - name: Resolve rollback plan + id: plan + run: | + set -euo pipefail + + # Validate each stack list independently. A malformed value means an + # upstream output regressed or a `|| '[]'` default failed to apply — + # a real bug worth shouting about. But this is the FIRST step of the + # recovery job: aborting here skips every rollback step and leaves + # production broken. So degrade to the conservative whole-tree path + # and raise a ::error:: annotation, which surfaces in the run log and + # the step summary without preventing recovery. + sanitize() { + local name="$1" value="$2" + if jq -e 'type == "array" and all(.[]; type == "string" and length > 0)' \ + <<<"$value" >/dev/null 2>&1; then + printf '%s' "$value" + return 0 + fi + echo "::error::$name was not a valid stack array (got: ${value:0:200}); forcing whole-tree rollback" >&2 + printf '[]' + return 1 + } + + # `var=$(f)` exposes f's exit status, but assignments inside f run in + # the command-substitution subshell and would not propagate — so the + # degraded flag is set here in the parent from that status. + degraded="" + a=$(sanitize DEPLOY_EXISTING_FAILED "$DEPLOY_EXISTING_FAILED") || degraded="yes" + b=$(sanitize DEPLOY_NEW_FAILED "$DEPLOY_NEW_FAILED") || degraded="yes" + c=$(sanitize HEALTH_FAILED "$HEALTH_FAILED") || degraded="yes" + # NEW_STACKS is validated here rather than in the per-stack step: if + # it is unusable we cannot tell a new stack from an existing one, and + # the safe response is to not take the per-stack path at all. + sanitize NEW_STACKS "$NEW_STACKS" >/dev/null || degraded="yes" + + culprits=$(jq -cn --argjson a "$a" --argjson b "$b" --argjson c "$c" \ + '($a + $b + $c) | unique') + count=$(jq 'length' <<<"$culprits") + + # Any culprit name that fails the stack-name pattern means an + # upstream output is corrupt. Do not act on it — but do not abort + # either: the whole-tree path never uses these names (it resets the + # tree and iterates prepare's own existing/removed lists, a different + # producer), so degrading discards the bad name and still recovers. + # Aborting here would leave production broken. + # Fed by process substitution, not a pipe, so the loop runs in this + # shell and `degraded` propagates. + while IFS= read -r name; do + [[ -z "$name" ]] && continue + if [[ ! "$name" =~ ^[a-zA-Z0-9._-]+$ ]]; then + echo "::error::invalid stack name in culprit list: ${name:0:100}; forcing whole-tree rollback" >&2 + degraded="yes" + fi + done < <(jq -r '.[]' <<<"$culprits") + + # Per-stack requires BOTH a stack-confined change set AND a known + # culprit. An empty culprit list means the failure was not attributed + # to any specific stack (a teardown failure, an infrastructure error, + # a timeout before any stack was named) — there is nothing to scope + # to, so the whole-tree reset is the only correct response. + if [[ -n "$degraded" ]]; then + mode="whole-tree" + elif [[ "$ROLLBACK_SCOPE" == "per-stack" && "$count" -gt 0 ]]; then + mode="per-stack" + else + mode="whole-tree" + fi + + { + echo "mode=$mode" + echo "culprits=$culprits" + } >> "$GITHUB_OUTPUT" + echo "🔄 Rollback mode: $mode (culprits: $culprits, scope: $ROLLBACK_SCOPE)" + + if [[ -n "$degraded" ]]; then + { + echo "## Rollback input degraded" + echo "One or more stack lists were malformed; forced a whole-tree" + echo "rollback. See the \`::error::\` annotations on the" + echo "\`Resolve rollback plan\` step for the offending value." + } >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Roll back failed stacks only + if: steps.plan.outputs.mode == 'per-stack' + env: + CULPRITS: ${{ steps.plan.outputs.culprits }} + run: | + set -euo pipefail + for stack in $(jq -r '.[]' <<<"$CULPRITS"); do + # Unreachable by construction: this step only runs when mode is + # per-stack, and `Resolve rollback plan` forces whole-tree if any + # culprit name fails this same pattern. Kept as defence in depth, + # and skipping rather than exiting so a surprise cannot strand the + # remaining culprits mid-rollback. + [[ "$stack" =~ ^[a-zA-Z0-9._-]+$ ]] || { + echo "::error::invalid stack name: $stack; skipping"; continue; } + + # Was this stack introduced by the failing deploy? If so it has no + # previous state to restore — tear it down and leave it gone, the + # same disposition the whole-tree path gives new stacks. + if jq -e --arg s "$stack" 'index($s) != null' <<<"$NEW_STACKS" >/dev/null; then + echo "🛑 Tearing down failed new stack $stack" + if [[ -f "$LIVE_REPO_PATH/$stack/compose.yaml" || -f "$LIVE_REPO_PATH/$stack/compose.yml" ]]; then + (cd "$LIVE_REPO_PATH/$stack" && docker compose down) \ + || echo "::warning::down failed for $stack" + fi + continue + fi + + echo "⏪ Reverting $stack to $PREVIOUS_SHA" + # Partial checkout: only this stack's directory moves back. HEAD + # stays at the target SHA and the tree is left dirty — that is + # fine, the next deploy's `git reset --hard $TARGET_REF` cleans it + # before anything else runs. Do NOT add a cleanup step here. + # + # A stack absent at PREVIOUS_SHA makes this an unmatched pathspec, + # which exits non-zero and under `set -e` would abort the loop and + # strand every remaining culprit. Warn and move on instead. + if ! git -C "$LIVE_REPO_PATH" checkout "$PREVIOUS_SHA" -- "$stack/"; then + echo "::warning::cannot revert $stack to $PREVIOUS_SHA (not present at that SHA); skipping" + continue + fi + + [[ -f "$LIVE_REPO_PATH/$stack/compose.yaml" || -f "$LIVE_REPO_PATH/$stack/compose.yml" ]] || { + echo "::warning::no compose file for $stack after revert; skipping up" + continue + } + + # No --pull always and no --build: land on the locally-tagged + # previous image kept on disk by the prune policy, so recovery does + # not depend on a registry being reachable mid-incident. + # + # The `op run` wrapper is REQUIRED. Without it every ${VAR} in the + # compose file resolves to empty and the stack comes up + # misconfigured while reporting success. + (cd "$LIVE_REPO_PATH/$stack" && \ + op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ + docker compose up -d --quiet-pull --wait --remove-orphans) \ + || echo "::warning::per-stack rollback up failed for $stack" + done + - name: Tear down new stacks (will not exist after reset) - if: needs.prepare.outputs.has_new_stacks == 'true' + if: steps.plan.outputs.mode == 'whole-tree' && needs.prepare.outputs.has_new_stacks == 'true' env: NEW_STACKS: ${{ needs.prepare.outputs.new_stacks }} run: | @@ -750,9 +901,11 @@ jobs: done - name: Reset live tree to previous SHA + if: steps.plan.outputs.mode == 'whole-tree' run: git -C "$LIVE_REPO_PATH" reset --hard "$PREVIOUS_SHA" - name: Redeploy stacks at previous SHA + if: steps.plan.outputs.mode == 'whole-tree' # Only the stacks this deploy actually touched need reverting: # - existing: roll their config back to the previous SHA # - removed: deleted this deploy (torn down already), so they From f02f25d9cc8313f0ae38f86ad2e252e0f9011427 Mon Sep 17 00:00:00 2001 From: owine Date: Mon, 24 Aug 2026 20:07:29 -0500 Subject: [PATCH 09/13] feat(deploy): report rollback scope and culprits in notification --- .github/workflows/deploy.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 12f521b..b33e439 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -997,6 +997,8 @@ jobs: HAS_DISABLED: ${{ needs.prepare.outputs.has_disabled_stacks }} DISABLED_STACKS: ${{ needs.prepare.outputs.disabled_stacks }} FORCE_DEPLOY: ${{ inputs.force-deploy }} + ROLLBACK_MODE: ${{ needs.rollback.outputs.mode }} + ROLLBACK_CULPRITS: ${{ needs.rollback.outputs.culprits }} run: | set -euo pipefail @@ -1085,7 +1087,14 @@ jobs: rollback_line="" if [[ "$rollback_status" != "skipped" ]]; then rb_icon="✅"; [[ "$rollback_status" != "success" ]] && rb_icon="❌" - rollback_line=" → $rb_icon Rollback" + rb_detail="" + if [[ "$ROLLBACK_MODE" == "per-stack" ]]; then + rb_names=$(jq -r '. | join(", ")' <<<"${ROLLBACK_CULPRITS:-[]}" 2>/dev/null || echo "") + rb_detail=" (per-stack: ${rb_names:-unknown})" + elif [[ "$ROLLBACK_MODE" == "whole-tree" ]]; then + rb_detail=" (whole-tree)" + fi + rollback_line=" → $rb_icon Rollback$rb_detail" fi pipeline="$deploy_icon Deploy → $health_icon Health$rollback_line" { From 1e65147ea2ba8be18333ed94c7be0466416acd62 Mon Sep 17 00:00:00 2001 From: owine Date: Tue, 25 Aug 2026 09:23:33 -0500 Subject: [PATCH 10/13] docs: record no-live-test decision and residual risk Task 7 live validation declined; per-stack path ships enabled. Documents what remains unverified and the accepted swallowed-failure risk. --- ...24-scoped-rollback-and-image-quarantine.md | 80 ++++++++----------- 1 file changed, 32 insertions(+), 48 deletions(-) diff --git a/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md b/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md index 1632ae8..88f0f32 100644 --- a/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md +++ b/docs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.md @@ -727,66 +727,50 @@ The skill file lives outside this repo and is not committed here. Nothing to com --- -## Task 7: End-to-end validation on a real host +## Task 7: Live-host validation — NOT PERFORMED (decision, 2026-08-25) -**Files:** none — this is a live exercise of the deployed workflow. +The user declined a live test. Deliberately breaking a stack on `docker-piwine` +to exercise both rollback paths was offered and turned down, and the per-stack +path was shipped **enabled** rather than behind an opt-in flag. -Everything before this is unit-tested or lint-clean, but the per-stack path has never actually run. It touches a live Docker host during an incident, which is not a state to first exercise during a real incident. +**Decision:** ship active; the first real stack failure exercises the new path. -**Caveat on where to test.** The spec suggested `docker-piwine-office`, but that repo runs only `dozzle` and `portainer` — with `auto-detect-critical` both are likely critical, so there is no safe stack to break there. Prefer a genuinely non-critical stack on `docker-piwine`. Confirm which stacks are critical first: +### What this leaves unverified -```bash -cd ~/Git/Compose/compose-workflow -./scripts/deployment/detect-critical-stacks.sh --stacks "$(ls -d ../docker-piwine/*/ | xargs -n1 basename | tr '\n' ' ')" --repo-dir ../docker-piwine -``` - -Pick a stack that is **not** in the resulting list. Confirm the choice with the user before breaking anything. - -- [ ] **Step 1: Merge this branch so the runner picks up the new workflow** - -Callers pin `owine/compose-workflow/.github/workflows/deploy.yml@main`, so the change must be on `main` to take effect. Open a PR, let `workflow-lint` pass, merge. - -- [ ] **Step 2: Exercise the per-stack path** - -In `docker-piwine`, on a branch, point the chosen non-critical stack's `image:` at a tag that cannot start (a nonexistent tag is the cleanest — it fails fast at pull). Change **nothing else** — the commit must touch only that stack's directory, or the classifier will correctly choose whole-tree and you will not be testing the new path. - -Merge it and watch the deploy run. +Everything logic-level is covered: 19 classifier unit tests, the 8 existing +transition tests, and stubbed dry runs of the rollback loop against real +throwaway git repos. Four things are not, and unit tests structurally cannot +reach them: -Expected: -- `prepare` logs `Rollback scope: per-stack`. -- `rollback` logs `Rollback mode: per-stack` with the broken stack as the sole culprit. -- The three whole-tree steps show as **skipped**. -- `git -C status` on the runner shows only that stack's directory modified. -- Every other stack is still running at the new SHA — verify at least two by hand. -- Discord shows `❌ Rollback (per-stack: )`. +1. `op run` + `docker compose up` against a rolled-back tree — no 1Password or + Docker daemon in the dev environment. +2. `git checkout $PREVIOUS_SHA -- /` against the runner's *persistent* + clone (tested only against throwaway repos). +3. GitHub Actions expression evaluation of the new outputs — specifically the + skipped-job empty-string cases and the `|| '[]'` defaults. +4. The classifier receiving a real `all_changed_files` value post-`escape_json` + fix (verified against the action's source, not a live run). -- [ ] **Step 3: Exercise the whole-tree fallback** +### Known accepted risk -Repeat, but include a trivial `compose.env` edit (add a comment line) in the same commit alongside the broken image. +The per-stack step swallows `up` failures as warnings: -Expected: -- `prepare` logs `Rollback scope: whole-tree` and names `compose.env` as the path outside stack directories. -- `rollback` logs `Rollback mode: whole-tree`; the per-stack step is **skipped**; the reset runs. -- Behavior is identical to today. -- Discord shows `❌ Rollback (whole-tree)`. - -- [ ] **Step 4: Restore** - -Revert both test commits in `docker-piwine` and confirm a clean green deploy before walking away. - -- [ ] **Step 5: Exercise the skill for real** - -With a broken version still on `main` from Step 2 (or by re-landing it), run: - -``` -/quarantine-image "e2e validation of quarantine-image" +```bash +op run ... docker compose up ... || echo "::warning::per-stack rollback up failed for $stack" ``` -Expected: one commit touching exactly two files; the compose line back at the previous tag **and** digest; a negated-regex `allowedVersions` rule in `docker-piwine/.github/renovate.json`; the config validator passing. Then confirm on the Dependency Dashboard that the blocked version no longer appears as an available update. +This matches the pre-existing whole-tree step's behavior, so it is consistent — +but it means a per-stack rollback that fails to bring the stack back up still +reports the job **green**, and the Discord line shows a success icon. Without a +live test, the first occurrence will be during a real incident. -Finish with `/quarantine-image --unblock ` and confirm the rule is gone. +Mitigating factor: every malformed-input path in `Resolve rollback plan` +degrades to `whole-tree`, so the *scope* decision fails safe. The residual risk +is concentrated in the docker/`op`/Actions layer, not in the classification +logic. ---- +If this proves noisy in practice, the fix is to collect failed stacks in the +loop and `exit 1` at the end, so the job result reflects reality. ## Out of scope From e9faa717f00b552b3b2bea113f9216c6f456b8f8 Mon Sep 17 00:00:00 2001 From: owine Date: Tue, 25 Aug 2026 09:42:00 -0500 Subject: [PATCH 11/13] fix(deploy): close 4 gaps in the per-stack rollback path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 — culprits were never intersected with this deploy's change set. health-check iterates the *critical* stacks (detected from labels across all discovered stacks), not the changed ones, so failed_stacks could name a stack that is byte-identical at PREVIOUS_SHA and TARGET_REF. A commit touching only termix/ that knocked swag over produced mode=per-stack culprits=[swag]; the `git checkout $PREVIOUS_SHA -- swag/` was a no-op, swag stayed broken, the ::warning:: was swallowed, the job went green — and termix, the only thing that actually changed, was never reverted. Whole-tree would have caught it. prepare now emits `changed_stacks` (first path segment of every changed file that names a known stack dir) and `Resolve rollback plan` requires every culprit to be a member. A culprit outside that set means the failure cannot be attributed to a stack this deploy touched, so the plan degrades to whole-tree. Note this deliberately does NOT use `existing_stacks`: detect-stack-changes.sh defines it as (all discovered stacks - new stacks), so it names the whole fleet on every run and the check would be vacuous. I2a — the skip-gate left the live tree dirty indefinitely. `git checkout -- /` moves index and worktree but not HEAD, so after a per-stack rollback HEAD still equals TARGET_REF with a dirty tree. The skip-gate's SHA comparison saw equality and set skipped=true, so the `git reset --hard "$TARGET_REF"` never ran. "Re-run failed jobs" at the same target-ref reported a green "Repository already at target commit" while a stack sat pinned at the previous SHA. The gate now checks `git status --porcelain` ahead of the SHA comparison and forces a deploy on a dirty tree. This is what makes the deliberate absence of a cleanup step in the rollback job safe — the dirt survives for an operator to inspect, and is cleared by the next deploy's reset rather than by the recovery job. I3 — the per-stack `up` had no timeout, unlike every deploy-path `up`. `docker compose up --wait` waits indefinitely on a container stuck in `starting`, which is exactly what a bad image produces. The job's timeout-minutes then cancelled the run, stranding every remaining culprit un-rolled-back — and a cancelled job takes no whole-tree fallback. Wrapped in `timeout "$SERVICE_STARTUP_TIMEOUT"` to match the deploy path. Minors: - M8: the classifier wrapper's `jq -cn --argjson` aborted `prepare` on a malformed upstream list, where the script it calls would have degraded. Falls back to `[]`, which reaches the script's dirs_count guard and yields whole-tree — the same disposition the script itself would pick. - M9: the new-stack teardown branch skipped a missing compose file silently; now warns, matching the pre-existing whole-tree teardown step. - M10: the per-stack `up` failure warning now carries the manual recovery command, since this is the one path where the job still goes green. Every degradation added here routes to whole-tree; none exits non-zero. A recovery job must never abort and leave production down. Design doc §A4 rewrote: it asserted "no cleanup step is required ... no drift accumulates across runs", which was false — it did not account for the skip-gate, and it did not account for the re-`up` needed after the reset. Both dependencies are now stated explicitly, including the warning that the second one rests on `existing_stacks` naming the whole fleet. --- .github/workflows/deploy.yml | 92 +++++++++++++++++-- ...ed-rollback-and-image-quarantine-design.md | 66 ++++++++++++- 2 files changed, 145 insertions(+), 13 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index b33e439..16547db 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -79,6 +79,7 @@ jobs: has_disabled_stacks: ${{ steps.discover-stacks.outputs.has_disabled_stacks }} critical_stacks: ${{ steps.detect-critical.outputs.critical_stacks }} rollback_scope: ${{ steps.classify-scope.outputs.rollback_scope }} + changed_stacks: ${{ steps.classify-scope.outputs.changed_stacks }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -200,11 +201,38 @@ jobs: REMOVED_STACKS: ${{ steps.detect-changes.outputs.removed_stacks || '[]' }} run: | set -euo pipefail + # `|| echo '[]'` rather than letting jq abort the job: a malformed + # upstream list is exactly the case the classifier is built to + # survive. `[]` hits its `dirs_count` guard and degrades to + # whole-tree — the same disposition the script would pick itself. + # Aborting `prepare` instead would skip the deploy entirely. stack_dirs=$(jq -cn \ --argjson a "$ACTIVE_STACKS" \ --argjson b "$DISABLED_STACKS" \ --argjson c "$REMOVED_STACKS" \ - '($a + $b + $c) | unique') + '($a + $b + $c) | unique' 2>/dev/null || echo '[]') + + # The exact set of stack directories this deploy touched: the first + # path segment of every changed file that names a known stack dir. + # + # This is NOT the same thing as `existing_stacks`. detect-stack-changes.sh + # defines existing_stacks as (all discovered stacks - new stacks), so it + # names the whole fleet on every run and cannot tell a touched stack from + # an untouched one. `rollback` needs the touched set to check that a + # culprit is a stack whose directory actually differs between + # PREVIOUS_SHA and TARGET_REF — reverting one that does not is a no-op. + # + # Degrades to `[]` on any malformed input, which makes every non-empty + # culprit list fail the membership check in `Resolve rollback plan` and + # forces whole-tree. Uncertainty resolves to whole-tree, never per-stack. + changed_stacks=$(jq -cn \ + --argjson f "$CHANGED_FILES" \ + --argjson d "$stack_dirs" \ + '[ $f[] | split("/")[0] | select(. as $s | $d | index($s) != null) ] | unique' \ + 2>/dev/null || echo '[]') + echo "changed_stacks=$changed_stacks" >> "$GITHUB_OUTPUT" + echo "📦 Stack dirs touched by this deploy: $changed_stacks" + ./.compose-workflow/scripts/deployment/classify-rollback-scope.sh \ --changed-files "$CHANGED_FILES" \ --stack-dirs "$stack_dirs" @@ -255,7 +283,15 @@ jobs: run: | set -euo pipefail CURRENT_SHA=$(git -C "$LIVE_REPO_PATH" rev-parse HEAD) - if [[ "$CURRENT_SHA" == "$TARGET_REF" && "$FORCE_DEPLOY" != "true" ]]; then + DIRTY="$(git -C "$LIVE_REPO_PATH" status --porcelain)" + if [[ -n "$DIRTY" ]]; then + # A dirty live tree almost always means a prior per-stack rollback + # left a stack pinned at the previous SHA (git checkout -- dir/ + # moves the worktree but not HEAD, so the SHA comparison below + # cannot see it). Deploy anyway so the reset restores a clean tree. + echo "::warning::live tree is dirty (likely a prior per-stack rollback); forcing deploy" + echo "skipped=false" >> "$GITHUB_OUTPUT" + elif [[ "$CURRENT_SHA" == "$TARGET_REF" && "$FORCE_DEPLOY" != "true" ]]; then echo "skipped=true" >> "$GITHUB_OUTPUT" echo "ℹ️ Already at $TARGET_REF; skipping deploy phase" else @@ -735,6 +771,8 @@ jobs: DEPLOY_NEW_FAILED: ${{ needs.deploy.outputs.new_failed_stacks || '[]' }} HEALTH_FAILED: ${{ needs.health-check.outputs.failed_stacks || '[]' }} NEW_STACKS: ${{ needs.prepare.outputs.new_stacks || '[]' }} + CHANGED_STACKS: ${{ needs.prepare.outputs.changed_stacks || '[]' }} + SERVICE_STARTUP_TIMEOUT: ${{ inputs.service-startup-timeout }} steps: - name: Resolve rollback plan id: plan @@ -792,6 +830,24 @@ jobs: fi done < <(jq -r '.[]' <<<"$culprits") + # C1: every culprit must be a stack whose directory this deploy + # actually changed. health-check iterates the *critical* stacks, not + # the changed ones, so `failed_stacks` can name a stack that is + # byte-identical at PREVIOUS_SHA and TARGET_REF — reverting it is a + # no-op that leaves the genuinely-changed stack un-rolled-back while + # the job reports success. If any culprit is outside the touched set + # we cannot tell which stack to scope to, so take the whole tree. + # + # `all(.[]; …)` is vacuously true over an empty culprit list, so this + # never fires on []; the `count > 0` test below already forces + # whole-tree in that case. + changed_set=$(sanitize CHANGED_STACKS "$CHANGED_STACKS") || degraded="yes" + if ! jq -e --argjson c "$changed_set" \ + 'all(.[]; . as $x | $c | index($x) != null)' <<<"$culprits" >/dev/null 2>&1; then + echo "::warning::culprit list includes stacks this deploy did not change; forcing whole-tree rollback" + degraded="yes" + fi + # Per-stack requires BOTH a stack-confined change set AND a known # culprit. An empty culprit list means the failure was not attributed # to any specific stack (a teardown failure, an infrastructure error, @@ -843,15 +899,28 @@ jobs: if [[ -f "$LIVE_REPO_PATH/$stack/compose.yaml" || -f "$LIVE_REPO_PATH/$stack/compose.yml" ]]; then (cd "$LIVE_REPO_PATH/$stack" && docker compose down) \ || echo "::warning::down failed for $stack" + else + # Parity with the whole-tree teardown step's annotation: a + # missing compose file means containers may still be running + # with nothing left to address them by, which an operator needs + # to see rather than have silently skipped. + echo "::warning::compose file missing for failed new stack $stack" fi continue fi echo "⏪ Reverting $stack to $PREVIOUS_SHA" # Partial checkout: only this stack's directory moves back. HEAD - # stays at the target SHA and the tree is left dirty — that is - # fine, the next deploy's `git reset --hard $TARGET_REF` cleans it - # before anything else runs. Do NOT add a cleanup step here. + # stays at the target SHA and the tree is left dirty. That dirt is + # the record of the rollback and must survive for an operator to + # inspect, so there is deliberately no cleanup step here — the next + # deploy's `git reset --hard $TARGET_REF` clears it. + # + # That guarantee depends on the deploy job's skip-gate treating a + # dirty tree as a reason to deploy. Without that branch a re-run at + # the same target-ref short-circuits on the HEAD==TARGET_REF test, + # the reset never runs, and the stack stays pinned here forever + # while the run reports success. Do not remove it. # # A stack absent at PREVIOUS_SHA makes this an unmatched pathspec, # which exits non-zero and under `set -e` would abort the loop and @@ -873,10 +942,17 @@ jobs: # The `op run` wrapper is REQUIRED. Without it every ${VAR} in the # compose file resolves to empty and the stack comes up # misconfigured while reporting success. + # + # `timeout` matches every deploy-path `up`. Without it a container + # stuck in `starting` — exactly what a bad image produces — hangs + # `--wait` until the job's timeout-minutes cancels it, stranding + # every remaining culprit un-rolled-back with no whole-tree fallback + # (a cancelled job takes no fallback path at all). (cd "$LIVE_REPO_PATH/$stack" && \ - op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ - docker compose up -d --quiet-pull --wait --remove-orphans) \ - || echo "::warning::per-stack rollback up failed for $stack" + timeout "$SERVICE_STARTUP_TIMEOUT" \ + op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ + docker compose up -d --quiet-pull --wait --remove-orphans) \ + || echo "::warning::per-stack rollback up failed for $stack; recover manually with: cd $LIVE_REPO_PATH/$stack && op run --no-masking --env-file=$LIVE_REPO_PATH/compose.env -- docker compose up -d --wait" done - name: Tear down new stacks (will not exist after reset) diff --git a/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md b/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md index bc449a5..a94e359 100644 --- a/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md +++ b/docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.md @@ -101,12 +101,68 @@ registry being reachable mid-incident. Healthy stacks are never touched. **Whole-tree** — every other case. Today's exact behavior, unchanged: tear down new stacks, `git reset --hard "$PREVIOUS_SHA"`, re-up existing + removed. -### A4. Partial checkout leaves the live tree dirty — and that is safe +### A4. Partial checkout leaves the live tree dirty — safe, but only because of two other things -`git checkout $PREVIOUS_SHA -- /` leaves the live tree with `HEAD` at the target SHA but one -directory's content staged at the previous SHA. No cleanup step is required: every deploy begins with -`git -C "$LIVE_REPO_PATH" reset --hard "$TARGET_REF"` (`deploy.yml:283`), which restores a clean tree -before anything else runs. No drift accumulates across runs. +`git checkout $PREVIOUS_SHA -- /` moves the index and the worktree but **not `HEAD`**. The live +tree is left with `HEAD` at `TARGET_REF` and one directory's content at `PREVIOUS_SHA` — a dirty tree +that persists after the job ends: + +``` +$ git checkout $T0 -- termix/ +$ git rev-parse HEAD # 7db462a… — still T1, unchanged +$ git status --porcelain # M termix/compose.yaml +``` + +That dirt is deliberate. It is the on-disk record of which stack was rolled back, and it must survive +for an operator to inspect, so there is no cleanup step in the `rollback` job. It is cleared by the +*next* deploy's `git reset --hard "$TARGET_REF"`. + +The original version of this section claimed that reset alone made the state safe, and that "no drift +accumulates across runs". That was wrong on both counts. Two separate mechanisms are actually required, +and each is load-bearing. + +**1. The skip-gate must treat a dirty tree as a reason to deploy.** + +The `deploy` job's skip-gate compares `git rev-parse HEAD` against `TARGET_REF` and skips the whole +deploy phase when they match. After a per-stack rollback they *do* match — `HEAD` never moved. So a +re-run at the same `target-ref` (the on-call's first instinct: "Re-run failed jobs") short-circuited +before the reset, reported a green *"Repository already at target commit"*, and left the stack pinned at +the previous SHA indefinitely. The reset that this section relied on never ran. + +The skip-gate therefore tests `git status --porcelain` first and forces `skipped=false` when the tree is +dirty, ahead of the SHA comparison. Removing that branch reopens the hole. + +**2. The next deploy must re-`up` the rolled-back stack, not merely restore its files.** + +`reset --hard "$TARGET_REF"` puts the bad line back on disk. If nothing then runs `docker compose up` +for that stack, the containers keep serving the *previous* SHA's images while the tree claims the new +one — silent drift, invisible until that stack changes again. + +Nothing does re-`up` it explicitly. It works because of an incidental property of +`detect-stack-changes.sh`: `existing_stacks` is computed as *(all discovered stacks − new stacks)*, not +as the `PREVIOUS_SHA..TARGET_REF` diff. It therefore names the entire fleet on every run, and +`Deploy existing stacks` brings every stack up on every deploy. A previously rolled-back stack is +re-`up`ed along with everything else, fails again on the still-bad image, and is rolled back again — +which is exactly the fallback described under "What Change A explicitly does not fix". + +This is a real guarantee but a fragile one, because it rests on a definition that reads like a bug. **If +`existing_stacks` is ever narrowed to the actual change set, this section's guarantee breaks and a +rolled-back stack will drift.** The fix at that point is to record the rolled-back stack names +(an untracked marker file in the live tree, unioned into `existing_stacks` by `prepare`) so they are +re-deployed explicitly rather than incidentally. Note that no code in this repository runs `git clean` +on the live tree, so an untracked marker would survive `reset --hard` — verified by +`grep -rn "git clean" .github/ scripts/`, which matches nothing. + +**Scope of the culprit list.** Related, and fixed alongside: `health-check` iterates the *critical* +stacks, which are detected from labels across all discovered stacks, not from this deploy's change set. +Its `failed_stacks` output could therefore name a stack that is byte-identical at `PREVIOUS_SHA` and +`TARGET_REF`, making `git checkout $PREVIOUS_SHA -- /` a no-op while the stack that *did* change +was never reverted. `prepare` now also emits `changed_stacks` — the first path segment of every changed +file that names a known stack directory — and `Resolve rollback plan` requires every culprit to be a +member of it. A culprit outside that set means the failure cannot be attributed to a stack this deploy +touched, so the rollback degrades to whole-tree. `changed_stacks` is used rather than `existing_stacks` +precisely because, per the above, `existing_stacks` names the whole fleet and would make the check +vacuous. ### A5. `notify` reports which path ran From a5ab6ac80916b34b74f23df5e7e9825c78c37fcf Mon Sep 17 00:00:00 2001 From: owine Date: Tue, 25 Aug 2026 09:53:58 -0500 Subject: [PATCH 12/13] fix(deploy): quote paths in the per-stack rollback recovery hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning emitted when a per-stack rollback up fails prints a command for an operator to copy and paste. With an unquoted $LIVE_REPO_PATH, a deploy path containing whitespace produced a command that word-splits: cd /opt/my compose/termix && op run --env-file=/opt/my compose/compose.env so pasting it fails instead of recovering the stack. Since this is the one rollback path that leaves the job green, the hint is likely to be the operator's first action during an incident — it needs to work. Now emits quoted components, verified to parse as valid shell: cd "/opt/my compose/termix" && op run ... --env-file="/opt/my compose/compose.env" ... Found by Sourcery on the stacked PR #83; the same pattern was present here and is fixed in each PR separately. --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 16547db..c24469b 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -952,7 +952,7 @@ jobs: timeout "$SERVICE_STARTUP_TIMEOUT" \ op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ docker compose up -d --quiet-pull --wait --remove-orphans) \ - || echo "::warning::per-stack rollback up failed for $stack; recover manually with: cd $LIVE_REPO_PATH/$stack && op run --no-masking --env-file=$LIVE_REPO_PATH/compose.env -- docker compose up -d --wait" + || echo "::warning::per-stack rollback up failed for $stack; recover manually with: cd \"$LIVE_REPO_PATH/$stack\" && op run --no-masking --env-file=\"$LIVE_REPO_PATH/compose.env\" -- docker compose up -d --wait" done - name: Tear down new stacks (will not exist after reset) From 6fddd75baba1b8148ab8f623b39837fdfdeb8810 Mon Sep 17 00:00:00 2001 From: owine Date: Tue, 25 Aug 2026 10:00:46 -0500 Subject: [PATCH 13/13] fix(deploy): add timeout to whole-tree rollback up; correct its comment (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(deploy): add timeout to whole-tree rollback up; correct its comment Two pre-existing issues in `Redeploy stacks at previous SHA`, left untouched by the scoped-rollback work because that change deliberately did not modify this step's body. 1. Missing `timeout`. Every other `docker compose up` in this workflow is wrapped in `timeout "$SERVICE_STARTUP_TIMEOUT"`; this one was not. `--wait` blocks indefinitely on a container stuck in `starting` — exactly what a bad image produces — until the job's timeout-minutes cancels the whole job, stranding every stack after it with no further fallback. Verified with exec-able stubs: a 30s hang now aborts at the 2s budget, emits a warning, and the loop continues. The failure message also now carries the manual recovery command, since a failed rollback up leaves the job green. 2. Inaccurate comment. It claimed this step reverts "only the stacks this deploy actually touched" and that skipping untouched stacks "avoids needlessly recreating the whole fleet". Both are false: detect-stack-changes.sh:401 computes existing_stacks as (all discovered stacks - new stacks), so this loop covers the whole fleet on every run. The comment now says so, and records that the fleet-wide scope is load-bearing — it is what pulls a stack pinned by a prior per-stack rollback back into line with the tree. * fix(deploy): quote paths in the whole-tree rollback recovery hint Same fix as the per-stack hint on the base branch, applied to the whole-tree step's warning. An unquoted $LIVE_REPO_PATH produced a copy-paste command that word-splits on a deploy path containing whitespace, so pasting it fails instead of recovering the stack. Reported by Sourcery on this PR. --- .github/workflows/deploy.yml | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index c24469b..6f4128d 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -982,14 +982,22 @@ jobs: - name: Redeploy stacks at previous SHA if: steps.plan.outputs.mode == 'whole-tree' - # Only the stacks this deploy actually touched need reverting: - # - existing: roll their config back to the previous SHA + # Which stacks this covers: + # - existing: NOT the changed set. detect-stack-changes.sh computes + # existing_stacks as (all discovered stacks - new stacks), + # so this is effectively the whole fleet on every run. # - removed: deleted this deploy (torn down already), so they # reappear after the reset and must be brought back up - # `new` stacks were torn down above and no longer exist post-reset, and - # untouched stacks are byte-identical before/after the reset — skipping - # them avoids needlessly recreating the whole fleet on a single-stack - # failure. No `--pull always`/`--build`: roll back onto the + # `new` stacks were torn down above and no longer exist post-reset. + # + # The fleet-wide scope is load-bearing, not an oversight: it is what + # brings a stack pinned by a prior per-stack rollback back into line + # with the tree. Narrowing existing_stacks to the real change set would + # let such a stack drift — its containers on the old image while the + # tree claims the new one — until it next changed. See the design doc, + # "Scoped rollback and image quarantine", section A4. + # + # No `--pull always`/`--build`: roll back onto the # locally-tagged previous images (kept on disk by the docker-prune # policy) so rollback is fast and doesn't depend on a registry being # reachable mid-incident; compose still builds on demand if an image is @@ -1006,9 +1014,15 @@ jobs: stack_dir="$LIVE_REPO_PATH/$stack" [[ -f "$stack_dir/compose.yaml" || -f "$stack_dir/compose.yml" ]] || continue cd "$stack_dir" - op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ + # `timeout` matches every other `up` in this workflow. Without it a + # container stuck in `starting` makes `--wait` block until the job's + # timeout-minutes cancels the whole job, stranding every stack after + # this one with no further fallback — the exact state a bad image + # tends to produce. + timeout "$SERVICE_STARTUP_TIMEOUT" \ + op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \ docker compose up -d --quiet-pull --wait --remove-orphans \ - || echo "::warning::rollback up failed for $stack" + || echo "::warning::rollback up failed for $stack; recover manually with: cd \"$LIVE_REPO_PATH/$stack\" && op run --no-masking --env-file=\"$LIVE_REPO_PATH/compose.env\" -- docker compose up -d --wait" done notify: