Run production's E2E rollback in its own job, away from npm

[Babissimo]

ClickUp: 1.4 Run the production E2E rollback in its own job, away from npm

Why

e2e-prod runs npm ci and Playwright inside the Playwright image, and then, in the same job, handed SERVER_SSH_KEY (root on production) to its rollback step. Every install script in the npm tree, and the image itself, ran first in the environment the key was later given to.

What changes

• The rollback moves to rollback-production-on-e2e-failure: its own job, on the runner, needs: e2e-prod.
• It acts on the test step's outcome, exported from e2e-prod as the job output tests, not on the job's result. A failed pull, checkout or npm ci still leaves production alone, as before.
• It is gated on !cancelled() rather than always(), so cancelling the run still stops it, as it stopped the in-job step.
• It takes production-deploy, like rollback-production-on-deploy-failure, so a following run's deploy cannot be in the deploy directory while rollback.sh is.
• Its appleboy/ssh-action is pinned by SHA (v1.2.5, what v1 points at today).
• test_deploy_workflows.py holds every workflow to one rule: a job that installs packages, runs an action other than checkout or the ssh transport, runs in a container or starts service containers cannot read a droplet key. It also evaluates the new job's if: across refs, events, test outcomes and cancellation. Against main's ci.yml the rule fails on e2e-prod, and swapping !cancelled() for always() fails the matrix.

Known limits, closed later in this stack

• production-smoke-tests and e2e-prod still hold no lock, so a following run's deploy can land while this run is still verifying, and its rollback would then act on that deploy. The next PR (1.1) runs the whole production chain under one calling job that holds production-deploy, which also removes the rollbacks' own groups and with them the one-pending-job cancellation. 1.6 adds a per-run deploy record the rollback checks.
• Actions reuses a failed job's outputs from the previous attempt on "Re-run failed jobs" (Failed job outputs from previous attempts re-used on "Re-run failed jobs" actions/runner#2598), so re-running e2e-prod after a rollback could fire a second one if the re-run never reaches the test step. The second rollback.sh restores the same snapshot.

Verification

• backend/tests/test_deploy_workflows.py, and the backend suite with CI's flags.
• actionlint on the three deploy workflows.
• Not exercised on retina-test: deploy-test.yml has no E2E stage, so it cannot drive this job, and rollback.sh is unchanged. The first real exercise is a failing production E2E.
• tower-finder-service's ci.yml has no job holding a droplet key beside an install step, so it needs no mirror.

🤖 Generated with Claude Code

[Babissimo]

On the "dead parametrization" note: the cancelled parameter isn't inert for the deploy-failure rollback. It pins that that rollback still runs when the run is cancelled, which is why it uses always() rather than !cancelled(): a cancel mid-deploy leaves production as half-deployed as a crash does.

Checked on cd278f4 by swapping that job's always() for !cancelled() and running the test. Exactly the two cancelled=True main-push cases fail:

• [True-failure-push-refs/heads/main]
• [True-cancelled-push-refs/heads/main]

With always() restored, all 72 cases pass. Without the parameter, that swap would pass unnoticed, so it stays.

[Babissimo]

Rebased onto main at ad12bbd, over #644 (the E2E jobs moved from Playwright's container to the runner's Chrome) and #651 (the Chrome report in each E2E job). Head: ebedabb.

What that changed here:

• e2e-prod keeps main's new shape: checkout, setup-node, npm ci, the Chrome launch, then the tests, with no container. The description's references to the image, to running "on the runner" by contrast with it, and to a failed pull no longer apply. The job output now separates the test step's verdict from a failed checkout, npm ci or Chrome launch, and the commit message says so.
• Read Chrome's version in the staging E2E job instead of launching it #651's reason for launching Chrome in production, keeping a Chrome that cannot start away from the rollback, holds as written: a failed launch leaves tests as skipped, so the rollback job does not run.
• The rule in test_deploy_workflows.py is unchanged. It still fails on main's ci.yml at e2e-prod, which still runs npm ci in the job that hands the rollback its key.

Verification on this head: the workflow, pin, permission and deploy-workflow tests and their neighbours pass (313), run under the shared suite lock, and pre-commit, actionlint included, is clean.

[Babissimo]

On the review of ebedabb:

1. The unlocked smoke and E2E jobs are the limit the description defers to Hold the production lock across production's verification and rollbacks #648, which runs production's whole chain under one calling job holding production-deploy.
2. The comment on the gate claims only that an operator's cancel of the run stops the rollback, which is what cancelled() reports: whether the run was cancelled. It makes no claim about e2e-prod timing out. That case is covered by the output check, as the review says: a timed-out or cancelled test step has the outcome cancelled, never failure, so the rollback stays off.

No change needed on either.

[claude]

Claude finished @jehanazad's task in 10m 31s —— View job

---

Claude's review of PR #590

• Gather context (PR diff, CLAUDE.md, ONBOARDING.md)
• Understand the request (custom instruction: run /code-review on this PR)
• Run /code-review skill against PR Run production's E2E rollback in its own job, away from npm #590
• Post findings

Summary

Reviewed ci.yml's new rollback-production-on-e2e-failure job and the accompanying test_deploy_workflows.py guard across correctness, cross-file tracing, and the repo's CI/workflow conventions. The job itself checks out as intended: it's a genuinely separate job (no container/services), gated on needs.e2e-prod.outputs.tests == 'failure' (not the job's own result, so a failed checkout/npm ci/Chrome launch in e2e-prod correctly leaves tests unset and skips the rollback), takes the production-deploy concurrency group before rollback.sh runs, and pins appleboy/ssh-action by SHA. I traced !cancelled() against both an operator cancel and an e2e-prod job-level timeout and both resolve the way the inline comment at ci.yml:453-456 claims. The new test_no_job_that_runs_third_party_code_can_read_a_droplet_key guard correctly fails against main's ci.yml (confirming the test is real, not vacuous) and I checked it against every other job holding an *SSH*KEY* secret across ci.yml, staging-deploy-verify.yml, and deploy-test.yml — none produce a false failure.

One thing worth a maintainer's attention, nothing else survived scrutiny of the diff:

Findings

1. backend/tests/test_deploy_workflows.py:220 — _allowed()'s field-boundary regex doesn't exclude a following ., so a field name that is a dotted prefix of a longer field referenced in the same condition would get corrupted instead of left alone.

   condition = re.sub(rf"(?<![\w.-]){re.escape(field)}(?![\w-])", repr(value), condition)

   The trailing lookahead (?![\w-]) blocks a following word-character or hyphen (so needs.e2e-prod.outputs.tests won't accidentally match inside ...tests_summary), but it doesn't block a following .. If some future rollback if: condition ever referenced both a field and a longer field that extends it with a dot (e.g. a nested outputs.tests.something), substituting the shorter field first would splice the substituted literal into the middle of the longer reference — producing an eval SyntaxError or a silently wrong boolean, defeating the very security-gate test this helper backs.
   This isn't live today: the only fields currently passed to _allowed() (github.ref, github.event_name, needs.deploy-production.result, needs.e2e-prod.outputs.tests) don't collide this way, so none of the 72+ parametrized cases are affected. It's a latent robustness gap in the test helper rather than a bug in this PR's shipped behavior. Tightening the lookahead to (?![\w.-]) would close it for future conditions at no cost to the current ones.
   Fix this →

The disclosed gaps in the PR description (no lock held by production-smoke-tests/e2e-prod, the actions/runner#2598 re-run caveat) are pre-existing/explicitly deferred to the next PR in the stack, not new regressions here.

Branch ·