Skip to content

fix: externalize production deploy script - #107

Merged
protostatis merged 2 commits into
mainfrom
fix/deploy-script-expression-limit
Aug 14, 2026
Merged

fix: externalize production deploy script#107
protostatis merged 2 commits into
mainfrom
fix/deploy-script-expression-limit

Conversation

@protostatis

Copy link
Copy Markdown
Owner

Summary

  • move the production cutover from the SSH action input into a versioned deploy/release.sh
  • keep only a 1,289-character bootstrap in the workflow, below GitHub's 21,000-character action-input expression limit
  • pass deployment inputs through the SSH action environment and avoid putting tokens in Docker command arguments
  • add regression tests for bootstrap length, external script use, shell parsing, and expression isolation

Incident context

GitHub rejected the v1.37.0 workflow before any jobs ran with Exceeded max expression length 21000. Production was not modified and remains on v1.36.0. v1.37.0 is marked not deployed/superseded; the corrected release will be v1.37.1.

Verification

  • pytest tests/ -q — 266 passed, 1 skipped
  • actionlint .github/workflows/deploy.yml
  • shellcheck deploy/release.sh
  • bash -n deploy/release.sh
  • short-bootstrap regression reports 1,289 characters

@protostatis protostatis left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sky's Code Review

This PR externalizes the ~550-line production cutover from the SSH action's inline script into a versioned deploy/release.sh, leaving a short bootstrap in the workflow to resolve the GitHub 'Exceeded max expression length 21000' rejection. The refactor is directionally sound: it moves secrets out of Docker command arguments into passthrough environment variables (GHCR_TOKEN via docker login --password-stdin, TELEGRAM_BOT_TOKEN via docker run -e KEY passthrough), fixes unquoted variable references, keeps the checkout-then-run ordering correct (git checkout before bash deploy/release.sh so the versioned script self-runs), and adds meaningful regression tests for bootstrap length, expression isolation, and shell parseability. No blocking security or correctness bugs found. Main suggestions: standardize on set -uo pipefail (the current set -e alone can mask pipeline failures in the heartbeat-watermark and disk-space checks), and note that REGISTRY/IMAGE_PREFIX are now silently required by release.sh but still sourced from repo/environment-level vars not visible in this diff. Also worth flagging: GITHUB_TOKEN and TELEGRAM_BOT_TOKEN now sit in the remote shell environ (visible via /proc//environ) for the duration of the SSH session, which is acceptable but a tradeoff to acknowledge, and docker system prune -af remains aggressively destructive.

Verdict: Comment

Comments

  • Removing the actions/checkout@v4 step is correct here — the runner-side checkout was never used by the SSH step, which operates on the EC2 host's own git repository and performs its own git fetch --tags origin + git checkout.
  • The checkout-then-run ordering (git checkout "$RELEASE_TAG" followed by bash deploy/release.sh) is exactly right: the freshly checked-out, tag-versioned script self-locates via BASH_SOURCE and runs against that tag. This achieves the stated goal of versioning the deploy logic.
  • The regression tests are a good addition, but the length assertion (< 20_000) does not match the reported 1,289-byte bound and GitHub's documented 21,000 limit; consider asserting against a tighter constant (e.g. < 5_000) so future bootstrap growth toward the real ceiling is caught earlier.
  • Consider adding a test that asserts release.sh never embeds ${{ secrets. / GITHUB_TOKEN literals (the ${{ guard already covers the expression syntax) to lock in the no-secret-in-script invariant going forward.

Reviewed by Sky — Unchained Sky engineering agent

Comment thread deploy/release.sh Outdated
#!/usr/bin/env bash
# Run the fail-closed production cutover after the release tag is checked out.

set -e

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use set -euo pipefail instead of set -e. Without pipefail, a failing upstream command in a pipeline is masked by the last command's exit status. Concretely, the heartbeat-watermark extraction (printf ... | python3 -c ...) and disk-space check (df / | tail -1 | awk '{print $4}') will not abort if docker run or df fails but python3/awk still exits 0.

Comment thread deploy/release.sh
docker system prune -af || true
docker builder prune -af || true

echo "Post-cleanup disk usage:"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AVAIL_KB=$(df / | tail -1 | awk '{print $4}') relies on the last-column field of df, which is fragile across filesystems (varies with SOURCE column width / mount names). Prefer df --output=avail / | tail -1 (GNU coreutils) or df -kP / | awk 'NR==2 {print $4}'. Combined with missing pipefail, a df failure here would silently produce an empty value that fails the [ "$AVAIL_KB" -lt ... ] comparison rather than aborting cleanly.

Comment thread deploy/release.sh

# ========== DISK SPACE CHECK & CLEANUP ==========
echo "Pre-cleanup disk usage:"
df -h /

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docker system prune -af removes all unused images and is destructive; if a concurrent process or a prior failed deploy holds the only known-good image tag, it is gone. This behavior is pre-existing (moved verbatim), but since the script is now versioned it may be worth gating the -a flag or preserving the :previous tags before pruning.

Comment thread deploy/release.sh
fi
echo "Disk space OK: ${AVAIL_KB}KB available"

# ========== DOCKER LOGIN & PULL ==========

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good — piping the token via printf '%s' "$GHCR_TOKEN" | docker login --password-stdin keeps it out of argv. Just confirm that docker login itself is not logged with the secret echoed; consider 2>&1 redirection if login noise ends up in the SSH action logs.

RELEASE_TAG: ${{ github.ref_name }}
RELEASE_IMAGE_TAG: ${{ needs.build-and-push.outputs.version }}
GHCR_ACTOR: ${{ github.actor }}
GHCR_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GHCR_TOKEN and TELEGRAM_BOT_TOKEN are now forwarded via the SSH envs list, which places them in the remote shell environment (visible in /proc/<pid>/environ for the lifetime of the SSH session). This is the intended improvement over inline argv, but note it means any process on the EC2 host during the deploy can read the Telegram token. Acceptable, but worth a comment acknowledging the tradeoff vs. a dedicated secrets store.

username: ${{ secrets.EC2_USERNAME }}
key: ${{ secrets.EC2_SSH_KEY }}
command_timeout: 45m
envs: RELEASE_TAG,RELEASE_IMAGE_TAG,REGISTRY,IMAGE_PREFIX,GHCR_ACTOR,GHCR_TOKEN,TELEGRAM_BOT_TOKEN

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

envs: RELEASE_TAG,RELEASE_IMAGE_TAG,REGISTRY,IMAGE_PREFIX,GHCR_ACTOR,GHCR_TOKEN,TELEGRAM_BOT_TOKEN forwards REGISTRY and IMAGE_PREFIX, but neither is defined in this job's env: block. They must resolve from workflow-level or repository/environment variables; if either is missing, release.sh will now fail at its required-vars guard. Confirm these are defined at a scope this diff doesn't show (the old inline code referenced ${{ env.REGISTRY }} so it likely already is, but the new hard requirement is worth verifying).

@protostatis

Copy link
Copy Markdown
Owner Author

Review follow-up in d1bd517:

  • enabled set -euo pipefail in both bootstrap and release script
  • switched disk parsing to df -kP with numeric validation
  • made intentionally empty retention/cron pipelines explicit under pipefail
  • tightened the bootstrap regression budget to 5,000 characters and asserted no secret expressions appear in the bootstrap

REGISTRY and IMAGE_PREFIX are workflow-level environment values at lines 7–9 and are forwarded by envs. GHCR login uses --password-stdin and does not print the token. The temporary SSH environment exposure is accepted on this single-user production host; the session ends immediately after deployment. Pre-pull pruning remains safe because the currently running five-container set references the known-good images, and workflow concurrency prevents another release deploy from racing it.

@protostatis protostatis left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sky's Code Review

The PR externalizes a ~550-line deployment cutover from an inline GitHub Actions SSH script into a versioned deploy/release.sh, leaving a short 1,289-character bootstrap in the workflow to stay under GitHub's 21,000-char action-input expression limit. This is a sound remediation for the v1.37.0 expression-length rejection. The refactor also improves secret handling (GHCR_TOKEN/TELEGRAM_BOT_TOKEN now flow through the SSH action env instead of appearing inline in command args) and adds regression tests for bootstrap length, external-script use, and expression isolation. The logic is a faithful extraction of the prior inline script with set -euo pipefail and array-based env-arg handling. I found no production-blocking issues, but several correctness and safety observations merit attention before the v1.37.1 cutover.

Verdict: Comment

Comments

  • The removal of actions/checkout@v4 combined with the bootstrap relying on git fetch --tags origin and git checkout $RELEASE_TAG is correct only because the deployment runs on the remote EC2 worktree (cd /home/ec2-user/crypto_sentiment_crawler), not on the runner. Worth confirming the EC2 worktree has origin/upstream configured for tag fetch, otherwise git fetch --tags origin will fail and release.sh won't run.
  • REGISTRY and IMAGE_PREFIX are referenced in release.sh but are absent from the workflow's envs: list and from the env: block. The old inline script used ${{ env.REGISTRY }} and ${{ env.IMAGE_PREFIX }}; ensure these are defined either at workflow/job level (and reachable) or added to the ssh-action envs so the required-variable guard doesn't fail-closed on a real deploy.
  • The ls -t ... | tail -n +15 | xargs -r rm pruning now uses || true at the pipeline end and shellcheck disable=SC2012; the xargs -r rm under old code was not guarded but also tolerated failures. This is a reasonable hardening, but note set -o pipefail still applies to the ls pipeline, so the || true only guards the final rm stage — a missing directory in ls could still trip pipefail. Confirm this is intended.

Reviewed by Sky — Unchained Sky engineering agent

echo "========== DEPLOYMENT START: ${{ github.ref_name }} =========="

set -euo pipefail
echo "========== DEPLOYMENT BOOTSTRAP: $RELEASE_TAG =========="

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bootstrap drops the explicit set -euo pipefail failure semantics on several critical checks. The old inline script wrapped the direct-egress probes in if ! ... ; then echo ERROR; exit 1; fi; the new bootstrap runs timeout 15 getent ahostsv4 github.com and a bare curl -fsS with no guard, so a DNS/egress failure after WireGuard teardown no longer aborts the deployment. Because release.sh presumably starts only after git checkout, a failure here means the deployment proceeds on broken egress and can fail later with a less actionable error. Consider restoring if ! guarded failure for these two probes.

Comment thread deploy/release.sh
fi
docker pull "$remote_image"
docker tag "$remote_image" "$local_image:current"
docker image rm "$remote_image" >/dev/null || true

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docker login uses printf '%s' "$GHCR_TOKEN" | docker login ... --password-stdin under set -o pipefail. This is good, but note that GHCR_TOKEN is now broughyt into the remote shell as a plain environment variable (via ssh-action envs), so it is visible in the remote process list/environment for the lifetime of the deploy job on the EC2 host. This is still better than the old inline echo ${{ secrets.GITHUB_TOKEN }} | ... (which leaked it into the workflow expression/args), so it is an improvement; just be aware the token materializes in the EC2 environment rather than being consumed directly by the action.

Comment thread deploy/release.sh

ROLLBACK_ACTIVE=0
for service in $PREVIOUS_SERVICES; do
docker rm "$service-previous" >/dev/null 2>&1 || true

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signals-bot container is launched with -e TELEGRAM_BOT_TOKEN (the pass-through form) which exposes the token from the deploy shell environment into the container. Verify this is intended versus -e TELEGRAM_BOT_TOKEN="$TELEGRAM_BOT_TOKEN", since the pass-through form only works because docker forwards the existing env var and would silently start the bot with an empty value if the env is not actually propagated through ssh-action.

Comment thread deploy/release.sh
set -euo pipefail

for required_name in \
RELEASE_TAG RELEASE_IMAGE_TAG REGISTRY IMAGE_PREFIX GHCR_ACTOR GHCR_TOKEN \

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required-variable guard uses if [ -z "${!required_name:-}" ] with indirect expansion under set -u. The ${!name:-} default handles an unset variable, but note REGISTRY and IMAGE_PREFIX are not listed in the workflow's envs: line (only RELEASE_TAG, RELEASE_IMAGE_TAG, GHCR_ACTOR, GHCR_TOKEN, TELEGRAM_BOT_TOKEN are exported). If REGISTRY/IMAGE_PREFIX were previously sourced from ${{ env.REGISTRY }} at the job level, confirm they are actually set in the remote environment or the guard will correctly fail-closed — which is safe but would break the deploy if they rely on job-level env: that was not exported.

@protostatis

Copy link
Copy Markdown
Owner Author

Final review clarification: REGISTRY and IMAGE_PREFIX are defined at workflow scope (lines 7–9) and explicitly present in the SSH action envs list. The production EC2 origin fetch was exercised during preflight. Under Bash, pipeline || true guards the status of the complete pipeline after pipefail, including a no-match ls.

@protostatis
protostatis merged commit 4aa1d32 into main Aug 14, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant