feat(deploy): scope rollback to failed stacks + image quarantine skill - #82
Merged
Conversation
Decides per-stack vs whole-tree rollback from the changed-file list. Uncertainty always resolves to whole-tree.
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).
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.
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.
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
<sha> -- <stack>/` 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.
Task 7 live validation declined; per-stack path ships enabled. Documents what remains unverified and the accepted swallowed-failure risk.
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 <sha> -- <dir>/` 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.
Reviewer's GuideThe PR limits rollback to failed, deploy-touched stacks only when scope classification and all input validation are unambiguous; otherwise it retains the existing whole-tree recovery path. It also adds classifier tests, dirty-tree recovery handling, richer notifications, and an external quarantine skill that prevents Renovate from repeatedly proposing a bad image. Sequence diagram for scoped failed-stack rollbacksequenceDiagram
participant Deploy
participant HealthCheck
participant Rollback
participant LiveRepo
participant OpDocker
Deploy->>HealthCheck: health-check
HealthCheck-->>Rollback: failed_stacks
Deploy-->>Rollback: rollback_scope and changed_stacks
Rollback->>Rollback: Resolve rollback plan
alt per-stack and validated culprits
loop each culprit
Rollback->>LiveRepo: git checkout PREVIOUS_SHA -- stack/
Rollback->>OpDocker: op run -- docker compose up -d --wait
OpDocker-->>Rollback: recovery result
end
else whole-tree or degraded input
Rollback->>LiveRepo: git reset --hard PREVIOUS_SHA
Rollback->>OpDocker: redeploy stacks at previous SHA
end
Flow diagram for conservative rollback scope selectionflowchart TD
A["Deploy fails or health check fails"] --> B["classify-rollback-scope.sh"]
B --> C{"All changed paths are known stack paths?"}
C -- No --> W["whole-tree"]
C -- Yes --> D["Resolve rollback plan"]
D --> E{"Inputs valid and culprit list non-empty?"}
E -- No --> W
E -- Yes --> F{"Every culprit is in changed_stacks?"}
F -- No --> W
F -- Yes --> P["per-stack"]
W --> R["Reset tree to PREVIOUS_SHA and redeploy"]
P --> S["Roll back failed stacks only"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes the production recovery policy from a whole-tree reset to selectively reverting and restarting individual stacks. If the classification or culprit attribution is wrong, shared configuration or dependencies can leave services broken while the rollback reports success, and the resulting production impact can span the fleet; reverting the workflow cannot undo an incident that already occurred.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
…nt (#83) * 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A bad upstream image (
ghcr.io/lukegus/termix:release-2.7.1) took down every subsequent deployment. Two independent defects combined:git reset --hard $PREVIOUS_SHAover the whole live tree, so one stack's failure reverted all ~15 stacks.main. The next commit re-applied the bad image, failed, and rolled back again — indefinitely.git revertalone cannot fix #2: theall-depsgroup automerges minor/patch/digest after a 1h minimum release age, so Renovate re-proposes the reverted version within the hour. Renovate has no memory of a version being bad.Changes
A — scoped rollback (
deploy.yml)Rollback gains a per-stack path, used only when all three hold:
classify-rollback-scope.sh),changed_stacks).Everything else keeps today's whole-tree reset, byte-identical.
preparerollback_scope+changed_stacksoutputs;escape_json: falseonchanged-fileshealth-checkfailed_stacksoutput (emitted beforeexit 1)rollbackResolve rollback plan+Roll back failed stacks only; three existing steps gated onmode == 'whole-tree'notifyB —
quarantine-imageskillLives in
~/.claude/skills/(not in this repo). Reverts a bad image to its last-known-good tag+digest and adds a negated-regexallowedVersionsblock in one commit, making the version invisible to Renovate rather than merely un-automerged.Governing invariants
whole-tree, neverper-stack. A wrong whole-tree is wasteful; a wrong per-stack leaves a broken deploy partially un-rolled-back.docker compose upisop run-wrapped. Without it every${VAR}resolves to empty and the stack comes up misconfigured while reporting success.Notable fixes found while building this
escape_jsondefaulted totrue, emitting[\"a/b.yaml\"]— unparseable byjq. Withoutescape_json: falsethe classifier would have rejected every input and returnedwhole-treeforever: the feature would have been silently inert. This also reviveddetect_removed_stacks_discovery, dead since it was written (a strict duplicate ofgitdiff, so no behavior change — verified across all 8 transition cases).existing_stacksis the whole fleet, not the diff (detect-stack-changes.sh:401). An earlier version of the culprit guard used it and would have been vacuous. Hence the newchanged_stacksoutput.git checkout <sha> -- dir/moves the worktree but notHEAD, soHEAD == TARGET_REFand the cleanupreset --hardnever ran. Now the gate treats a dirty tree as a reason to deploy.Testing
test-classify-rollback-scope.sh); 8 pre-existing transition tests still passdocker/opstubbedyamllint --strictclean repo-wide;shellcheck -xclean;actionlintshows only the pre-existingjob.workflow_shawarningNo live-host test was performed — see the plan's Task 7 section for what that leaves unverified and the accepted risk (a failed per-stack
upwarns but leaves the job green).Rollout
Caller repos pin this workflow by SHA, so merging does not activate anything. The per-stack path goes live when Renovate bumps each caller's pin.
Docs
docs/superpowers/specs/2026-08-24-scoped-rollback-and-image-quarantine-design.mddocs/superpowers/plans/2026-08-24-scoped-rollback-and-image-quarantine.mdSummary by Sourcery
Make deployment recovery safer by rolling back only affected stacks when confidence is high and quarantining known-bad images from future automation.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Documentation:
Tests: