Skip to content

Migrate pipeline from CircleCI to GitHub Actions#215

Closed
phelma wants to merge 5 commits into
mainfrom
gha-migration
Closed

Migrate pipeline from CircleCI to GitHub Actions#215
phelma wants to merge 5 commits into
mainfrom
gha-migration

Conversation

@phelma

@phelma phelma commented Jul 16, 2026

Copy link
Copy Markdown

Cutover to GitHub Actions per the Variant B family plan (docker image).
Includes decommission — merging this PR completes the repo's migration.

  • main + pr workflows: loop-guard, check/test, prerelease, release environment gate
  • git-crypt unlock on the runner; encrypted CI GPG key moved to .github/
  • Docker builds/publishes use the runner's native docker daemon (replaces
    CircleCI's setup_remote_docker + install scripts); the integration suite
    builds the image and runs its docker-compose dependencies on the runner
  • Docker Hub credentials load made lazy (dynamic block, nil when the tree
    is locked) so PR checks run without unlocking git-crypt — the one
    deliberate change inside the image namespace
  • Slack notifications via rake_slack; dependabot auto-merge job
  • Rakefile provisioning swapped to rake_github secrets/environments; rake_circle_ci dropped
  • CircleCI pipeline removed: .circleci/, scripts/ci/, the CI SSH deploy
    key pair and its keys:deploy/deploy_keys provisioning, and the stored
    CircleCI/GitHub API credentials (config/secrets/{circle_ci,github}/)

Deliberate decisions (not defects)

This cutover reproduces the CircleCI pipeline's behaviour, warts included;
fixing inherited hazards is post-migration work. In particular:

  • version:bump[rc]/version:release tag and push before
    image:publish runs — a failed publish leaves a version tag with no image
    behind it. Pre-existing ordering inside the untouched release logic.
  • An rc image is published to Docker Hub on every push to main with no
    approval gate; only full releases are gated (environment: release).
  • version:bump[rc] on a released version increments minor and starts
    rc.1 — every release cycle is a minor bump. Pre-existing
    lib/version.rb logic.
  • Dependabot auto-merge accepts any update type that passes checks, and the
    merge does not trigger a release build — on CircleCI the merge commit
    carried [skip ci], so this matches. Updates ship with the next
    human-triggered release.
  • The old pipeline ran CI only on main and dependabot/* branches;
    pr.yaml running on every pull request is a deliberate family-uniform
    improvement.
  • The release job pulls main at approval time, so a delayed approval
    releases main as it stands then, not the SHA this run tested — parity with
    the old release.sh (which also pulled; prerelease.sh did not, so the
    prerelease job has no pull).
  • Version tags live in git only — there is no version-bump commit, so no
    push-back commit and nothing for the loop-guard to catch; skip-ci-check
    stays as family-uniform belt-and-braces.
  • asdf_install@v1 is our own action (infrablocks/github-actions); we are
    happy tracking its major version tag.
  • Jobs that resolve version tags (test, prerelease, release) check out
    with fetch-depth: 0 — the Rakefile's latest_tag needs the full tag set.
  • The test jobs carry a guard step installing standalone docker-compose
    only if the runner image lacks it (the Rakefile shells out to the
    standalone binary); it is a no-op where compose is present or unused.
  • Job scaffolding is repeated flat per job by design: the logic lives in the
    build system (./go/rake) and CI stays lean — it just triggers tasks and
    supplies secrets/context.
  • Gemfile.lock carries transitive major bumps — the unavoidable resolution
    of the targeted bundle lock --update, not scope creep.
  • Small hunks may appear where the refreshed toolchain's rubocop
    autocorrects existing code — required by the library:check verification
    gate, not drive-by refactoring.
  • Provisioning (pipeline:prepare) authenticates with the operator's ambient
    gh login (GITHUB_TOKEN fallback) instead of a stored PAT — a deliberate
    parity deviation; the stored token in config/secrets/github/config.yaml
    is deleted with the rest of the CircleCI-era credentials.

Do not merge manually — the pipeline merges once checks are green.
Disabling the CircleCI project and deleting the CircleCI deploy key are
deferred to the end-of-migration sweep.

@phelma
phelma force-pushed the gha-migration branch 2 times, most recently from 16036d7 to 60d5331 Compare July 16, 2026 12:44

@phelma phelma left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Code Review: #215 - Migrate pipeline from CircleCI to GitHub Actions

Verdict: COMMENT

Reviewed through five lenses (correctness, security, safety, standards, code quality) against the Variant B family plan. This is a near-verbatim implementation of plan §4: both workflow files match the specified YAML step-for-step, the Slack channel IDs and first-match rule ordering are correct, the Rakefile require/Gemfile lists stay alphabetical, the GPG key move is byte-identical (R100), and the decommission is complete and internally consistent (git grep -i circleci matches nothing outside the lockfile). I verified that no file outside the plan's allowed set was touched. One in-scope defect (a minor plan §4.5 lockfile non-conformance, already fixed locally but unpushed) and one suggestion; everything else the lenses raised challenges a decision the plan or PR description documents as deliberate, and is recorded below as a plan concern rather than a blocker.

Evidence-based corrections to lens findings

Two findings were checked against reality rather than taken at face value:

  • 🔍 Disproven (false positive) — the correctness lens reported a critical: the regenerated lockfile resolves activesupport 8.1.3 (which declares required_ruby_version >= 3.2.0) while .tool-versions pins ruby 3.1.1, concluding that "every job fails at toolchain bootstrap, so the migrated pipeline cannot run at all". The check job passed on a real runner at this exact SHA in 43s (run 29500096772), completing bundle install over the full lockfile graph and running rubocop clean (6 files inspected, no offenses detected). The claim is disproven and is not counted. The underlying version-declaration mismatch is real but demonstrably does not bite; the still-pending test job uses the same bundle and will confirm.
  • 🔍 Confirmed and sharpened — the standards lens flagged an arm64-darwin platform leak in Gemfile.lock at medium confidence, unsure whether it was already fixed. It is present on the pushed head and absent from the local tree; see the inline comment.

Cross-Cutting Themes

  • The main concurrency group is held during release approval (flagged by: correctness, safety) — the strongest signal in this review, and the one most worth taking back to the plan before fleet rollout. Both lenses independently concluded that a run parked on the release environment approval holds group: main, so with cancel-in-progress: false + queue: max every later push to main queues behind it for up to the 30-day approval window. Since the modal push to main is one nobody releases, this is arguably the normal case, not the edge case — and it is a regression from CircleCI, where the hold job blocked only its own workflow. The plan mandates this block verbatim (§4.2), so it is a plan concern, not a diff defect.
  • Trust boundary at release time (flagged by: security, safety, code quality) — the unpinned curl | sudo install of docker-compose, the mutable asdf_install@v1 tag, and the unfiltered dependabot auto-merge are each individually accepted by the plan, but they compose: all three are routes by which new code enters the one job that holds ENCRYPTION_PASSPHRASE, contents: write, and the decrypted Docker Hub credentials. Worth considering as a set in the post-migration hardening pass rather than three separate items.
  • Error handling infers one cause from a broad signal (flagged by: correctness, safety, code quality) — both new Ruby blocks (dynamic credentials rescue, github_token resolution) collapse several distinct failure modes into a single assumed cause. Both shapes are mandated by plan §4.4, so this is feedback for the plan.

Tradeoff Analysis

  • Parity vs hardening: the great majority of findings below are the lenses correctly identifying real hazards that the plan has already considered and deliberately accepted for parity. The plan is the spec here, and a plan-conformant diff should not be blocked by them — but they are worth preserving, because this shape is about to be replicated across ~103 repos, and the cost of revisiting a decision is much lower now than after the fleet rollout.
  • Verbatim plan conformance vs local improvement: several suggestions (explicit if: success(), comments on the Slack fall-through rules, timeouts on check/test) would improve readability but would deviate from the plan's exact YAML. Recommendation: leave this diff conformant and raise them against the plan, so the family stays uniform.

Strengths

  • ✅ Faithful, disciplined execution of the plan: workflows match §4.1/§4.2 essentially line-for-line, and nothing outside the plan's allowed file set was touched — genuinely rare for a migration of this size.
  • skip-ci-check passes the untrusted commit message via env: rather than interpolating ${{ github.event.head_commit.message }} into the shell — the correct, non-obvious idiom, applied without prompting. A null head_commit degrades to the safe "run CI" default.
  • pull_request (not pull_request_target), workflow-level contents: read, and narrow per-job escalation (contents: write only on prerelease/release, pull-requests: write only on the merge job) — proper least privilege.
  • fetch-depth: 0 applied to exactly the tag-resolving jobs (test, prerelease, release) and correctly omitted from check.
  • ✅ Slack routing verified against the rake_slack source: first-match-wins resolves correctly for every reachable outcome/actor/type combination, with a terminal catch-all so NoMatchingRule cannot occur.
  • ✅ The decommission is genuinely credential-reducing: the CI SSH deploy key, the stored CircleCI API token, and the stored GitHub PAT all go, with provisioning moving to short-lived ambient gh auth. keys:gpg correctly retained for git-crypt.
  • ✅ Ten shell scripts collapse into repository:set_ci_author — moving logic into the build system rather than porting bash into YAML.
  • ✅ Comments explain why, not what (the git pull parity note, the queue: max changelog link, the "Actions store only" note on t.secrets).
  • ✅ The PR description's deliberate-decisions list is exemplary and made this review substantially faster and more accurate.

Plan concerns (not blockers)

These challenge documented deliberate decisions. Keeping them visible so a human can revisit the plan; none should block this PR. Those not posted inline:

  • 🔵 Safety: no timeout-minutes on check/test (both workflows) — they fall back to the 360-minute default, and a hung test on main holds the concurrency group for six hours. Plan's exact YAML sets timeouts only on prerelease/release.
  • 🔵 Correctness: the merge-pull-request job drops the CircleCI merge_pull_request job's Slack notify step, so a failed auto-merge is silent. Low impact (dependabot runs have no SLACK_BOT_TOKEN until issue 19 lands); worth folding into that follow-up.
  • 🔵 Security: git-crypt unlock imports the CI GPG key into the runner keyring and decrypts credentials to the working tree, so the trust boundary for Docker Hub credentials is "the entire gem graph at release time". Inherent to D1; native GHA secrets/OIDC is the right long-term fix and is already a listed follow-up.
  • 🔵 Security: the README's openssl aes-256-cbc -md sha1 recipe (pre-existing; plan says leave untouched) uses the legacy EVP_BytesToKey KDF to protect a GPG key whose ciphertext is public. -pbkdf2 -iter 600000 is the fleet-wide fix; ciphertext and recipe must change in lockstep.
  • 🔵 Security: dependabot-triggered pull_request runs get a restricted GITHUB_TOKEN, so auto-merge may fail closed (a 403). Worth validating on the pilot — and worth recording in the plan that the common fix found by searching, switching to pull_request_target, would be seriously wrong here: it would run the PR's own Rakefile with the base branch's write token and secrets.
  • 🔵 Correctness: github:environments:ensure / github:secrets:ensure are destroy-then-provision in rake_github, so re-running pipeline:prepare (documented as idempotent) while a release awaits approval would silently cancel that pending deployment. Consider the provision variants, or note it in the runbook.
  • 🔵 Code quality: the docker-compose guard is installation logic, not scaffolding — the plan's "repeat scaffolding flat per job" rationale arguably doesn't cover it. infrablocks/github-actions (D6) is the natural home; that would also give the version pin somewhere to live.
  • 🔵 Code quality: the Notify Slack of release hold step relies on GHA's implicit if: success() default while every neighbouring notify step has an explicit if:. The default is load-bearing — it is the only thing preventing an "on hold" message after a failed Prerelease step. Anyone "tidying up" for consistency would break it.
  • 🔵 Code quality: the two Slack fall-through rules hard-code format: :failure while matching on conditions that say nothing about outcome — correct only positionally. A one-line comment on each would make the ordering dependency explicit.
  • 🔵 Correctness / documentation: the plan's stated rationale for the dynamic wrapper is inaccurate. Tracing rake_factory, resolve_block defers the config block into the rake action, so the eager YAML.load_file never fired at Rakefile load time — it fired when image:build ran. The change is still necessary and correct; only the plan's wording needs fixing, since it will be applied to ~103 repos and could misdirect future debugging.

Review generated by /accelerator:review-pr

Comment thread Gemfile.lock Outdated
Comment on lines 1824 to 1828
unicode-emoji (4.0.4)
uri (0.13.2)
uri (1.1.1)

PLATFORMS
arm64-darwin-21

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan conformance (§4.5) — the one in-scope defect in this diff.

The pushed lockfile's PLATFORMS section is missing the generic ruby platform and carries four operator-local ones (arm64-darwin-21/22/23/25). Plan §4.5 requires, unconditionally after any lockfile update:

ensure the PLATFORMS section retains the generic ruby and x86_64-linux platforms and contains no operator-local platforms (e.g. arm64-darwin-*): a bare bundle lock/bundle update on a Mac can add arm64-darwin-25 and drop ruby.

In fairness: I verified this state is identical on origin/main — this PR did not introduce it. But §4.5 makes the check-and-remediate step a required part of any lockfile update, and this PR updates the lockfile.

Impact — minor, not breaking: x86_64-linux is present, so the runner resolves fine; the check job passing at this SHA confirms it. The cost is a non-reproducible lockfile across the team, and ffi (1.17.4-arm64-darwin) (line 1664) has no generic ffi sibling to fall back on.

Already fixed locally, just not pushed: your unpushed local commit ea9b1aa has exactly the right thing — PLATFORMS: ruby, x86_64-linux. The pushed PR head is 60d5331. Pushing that commit resolves this finding; it also reorders the Rakefile's slack/repository blocks, which is cosmetic and equally plan-conformant.

Per §4.5 the remediation is bundle lock --add-platform ruby x86_64-linux plus bundle lock --remove-platform arm64-darwin-{21,22,23,25}, re-verified with the §5.4 clean-env install.

Comment on lines +15 to +18
concurrency:
group: main
cancel-in-progress: false
queue: max

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 PLAN CONCERN (flagged independently by the correctness and safety lenses) — this block is mandated verbatim by plan §4.2, so it is a plan-level issue, not a defect of this diff. Not a blocker for this PR, but the single item most worth resolving before the fleet rollout.

concurrency is declared at workflow level, so the group is held for the whole run — including while release sits in waiting on its environment: release approval. A release is only approved when someone deliberately wants to ship, so the modal outcome of a push to main is that the run parks in waiting and holds group: main until the approval expires (default 30 days).

With cancel-in-progress: false and queue: max, every subsequent push to main queues behind it and never starts: no tests, no rc image, no tag — silently, with no failure signal. The comment's framing ("a slow release approval delays later prereleases instead of cancelling them") describes a bounded delay; the unbounded case looks like the normal one.

This is also a regression from CircleCI rather than parity: CircleCI doesn't serialise workflows, so its hold job blocked only its own workflow — pushes 2, 3, 4 each ran test + prerelease independently.

Safety lens adds the 3am version: a release proposed on Friday that nobody approves means main has no CI feedback by Monday, plus a queue of runs that will each publish an rc image and push a tag the moment someone approves or cancels.

Suggested fix (for the plan): scope serialisation to the jobs that actually race on version tags, so the approval wait can't hold it — drop the workflow-level block and put concurrency: {group: main-prerelease, cancel-in-progress: false} on prerelease. Alternatively split release into a workflow_dispatch workflow so the approval wait lives outside the group entirely.

Comment thread .github/workflows/pr.yaml
Comment on lines +34 to +39
if ! command -v docker-compose >/dev/null 2>&1; then
sudo curl -fsSL \
"https://github.com/docker/compose/releases/latest/download/docker-compose-linux-x86_64" \
-o /usr/local/bin/docker-compose
sudo chmod +x /usr/local/bin/docker-compose
fi

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 PLAN CONCERN — plan §4.1/§4.2 mandate this snippet verbatim (and main.yaml carries it identically), so not a defect of this diff.

The guard downloads from releases/latest/download/... — a mutable pointer — and sudo-installs the result into /usr/local/bin with no checksum or signature verification, then executes it. The pipeline it replaces installed docker-compose from Debian's signed, version-pinned apt repository, so this is a supply-chain regression relative to CircleCI rather than parity.

Impact: a compromised or mis-published docker/compose release asset yields root-level arbitrary code execution on the runner. In main.yaml's test job that reaches the Slack bot token; normalised across ~103 repos it becomes a single fleet-wide point of failure. latest also means two runs a week apart can silently test against different compose versions — a reproducibility problem independent of the security one.

Suggested fix (for the plan): pin an explicit version and verify — COMPOSE_VERSION=v2.x.y in the URL plus sha256sum -c against the published checksum. Better still, drop the step if dependencies:test:* can be pointed at the docker compose plugin the runner already ships, which removes the download rather than hardening it. If the step stays, infrablocks/github-actions/docker_compose_install@v1 (D6) is the natural home, per the code-quality lens.

# as of approval time, not the tested SHA — parity with the old
# release.sh, which also pulled. prerelease.sh never pulled, so the
# prerelease job deliberately has no pull.
run: git pull

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 PLAN CONCERN (flagged by both security and safety) — documented as deliberate parity with the old release.sh, which also pulled. Not a blocker.

ref: main + git pull means the approver approves run N (built from SHA X), but what gets tagged and published is main as of approval time. The environment: release reviewer is the only human gate on an irreversible Docker Hub publish plus a pushed release tag — and what they're approving isn't pinned to anything they can inspect.

The security framing worth adding to the plan's note: this is a time-of-check/time-of-use gap in the only human authorisation control on the release path. Anyone who can land on main between approval request and approval — including via the unfiltered dependabot auto-merge, which merges without triggering a build — gets that code published under a release tag with a maintainer's approval attached. The audit trail then attributes the release to an approver who never saw the content.

The workflow-level concurrency: group: main does mitigate the common case today (later pushes queue behind the pending release), so this mainly bites on a direct push during the approval wait — but note it stops mitigating if the group is re-scoped to fix the stall issue flagged above. The two interact.

Suggested fix (post-migration): ref: ${{ github.sha }} with no git pull, so the approval gates the exact tested tree.

Comment thread Rakefile
Comment on lines +219 to +223
t.credentials = dynamic do
YAML.load_file('config/secrets/dockerhub/credentials.yaml')
rescue Psych::SyntaxError
nil # tree locked: skip Docker Hub auth (base image is public)
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 PLAN CONCERN (safety + code quality, merged) — plan §4.4 step 2a mandates this exact snippet, so not a defect of this diff.

The rescue infers "tree is locked" from a Psych::SyntaxError, but that exception has more than one cause, and the rescue applies to every invocation — including image:publish in the unlocked prerelease/release jobs. If the unlock ever half-fails (rotated GPG key, partially applied filters, a future .gitattributes change) or the file is simply malformed, the publish silently proceeds unauthenticated rather than failing loudly.

Impact: combined with the documented tag-then-publish ordering, that failure lands after version:bump[rc]/version:release has already pushed an immutable tag — so the repo ends up tagged with no image behind it, and the diagnostic is an opaque Docker denied rather than "secrets are locked". Recovery means hand-deleting a pushed tag.

The narrowness of the rescue is right and should not be broadened to StandardError — that would mask genuine malformed-credentials errors in exactly the job where the file is meant to be readable.

Two suggested refinements (for the plan):

  1. Detect lock state directly rather than by exception type — git-crypt ciphertext starts with a \0GITCRYPT magic prefix, so next nil if File.binread(path, 9) == "\x00GITCRYPT" states the intent precisely and sidesteps a robustness question worth confirming on the runner: YAML.load_file over binary ciphertext may raise an encoding ArgumentError rather than Psych::SyntaxError, which the current rescue would not catch at all.
  2. Have the prerelease/release jobs assert the tree is unlocked right after the unlock step, so a locked tree fails before any tag is pushed.

(Encouraging data point: the check job passed at this SHA in a locked tree, confirming the dynamic deferral itself works as intended.)

Comment thread .github/workflows/pr.yaml
Comment on lines +59 to +61
env:
PR_URL: ${{ github.event.pull_request.html_url }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Suggestion (safety lens) — in-scope, cheap, and doesn't deviate from the plan's behaviour.

The old pipeline made dependabot merges non-releasing explicitly, via the "%s [skip ci]" merge commit message. Here the same property holds only implicitly: pushes made with the default GITHUB_TOKEN don't trigger workflows, so the merge commit never starts main.yaml. Note skip-ci-check would not catch it either — the merge commit carries no skip marker.

This is correct today (and D3 documents it), but the safeguard is invisible at the point where someone would break it. Swapping GH_TOKEN for a PAT or GitHub App token — a common, innocuous-looking change, e.g. to satisfy branch protection — would immediately make every dependabot merge publish rc images and push version tags with no approval. That's a silent, irreversible behaviour change from a one-line edit.

Suggestion: record the dependency inline so a future editor sees it:

# Merges with the default GITHUB_TOKEN deliberately do not trigger
# main.yaml (GitHub does not re-trigger workflows for GITHUB_TOKEN
# pushes). Swapping this for a PAT/App token would start
# auto-releasing dependabot merges.

with:
fetch-depth: 0
- name: Install tools
uses: infrablocks/github-actions/asdf_install@v1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 PLAN CONCERN — §1/D6 explicitly accept tracking the moving v1 tag, so not a blocker. Recorded only because it composes with the other two items in the "trust boundary at release time" theme.

infrablocks/github-actions/asdf_install@v1 is a mutable major-version tag, and it runs in prerelease and release — the two jobs holding ENCRYPTION_PASSPHRASE, contents: write, and (post-unlock) the decrypted Docker Hub credentials.

A push to the v1 tag in the actions repo — malicious or accidental by anyone with write access there — silently executes new code in every InfraBlocks release job across the fleet. First-party ownership reduces likelihood but not blast radius; a first-party repo with broad write access is often the softer target.

Suggested follow-up: pin to a commit SHA (asdf_install@<sha> # v1.2.3) at least in the secret-bearing jobs, and protect the v1 tag in infrablocks/github-actions. Dependabot's github-actions ecosystem keeps SHA pins current, so the maintenance cost is near zero.

Comment thread .github/workflows/pr.yaml
merge-pull-request:
needs: [check, test]
runs-on: ubuntu-latest
if: github.actor == 'dependabot[bot]'

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 PLAN CONCERN — D3 explicitly accepts no update-type filter as CircleCI parity. Not a blocker.

The job gates only on github.actor == 'dependabot[bot]' and green checks, then merges with contents: write + pull-requests: write. No dependabot/fetch-metadata, no update-type filter — so any update dependabot proposes, including a new major of a gem that runs code at bundle install/Rakefile-load time, lands on main unreviewed.

The actor gate itself is sound: github.actor on pull_request is not attacker-spoofable. The concern is the trust placed in upstreams — a compromised gem becomes trusted code on main, then gets executed by the very prerelease job holding the passphrase and Docker Hub credentials.

Suggested follow-up (already anticipated by the plan's "post-migration hardening may add one"): dependabot/fetch-metadata restricting auto-merge to version-update:semver-patch/semver-minor, leaving majors for human review.

Comment thread Rakefile
Comment on lines +173 to +180
github_token = ENV.fetch('GITHUB_TOKEN') do
`gh auth token`.strip
rescue Errno::ENOENT
''
end
if github_token.empty?
raise 'No GitHub token available: set GITHUB_TOKEN or run `gh auth login`'
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 PLAN CONCERN — plan §4.4 step 3 specifies this snippet verbatim, so not a defect of this diff.

This folds three distinct failure modes into one empty string and one message: gh not installed (Errno::ENOENT''), gh installed but not logged in (non-zero exit → ''), and GITHUB_TOKEN set but empty. The message then advises run `gh auth login` — impossible advice in the first case, which is precisely the case the rescue was added to handle. The fail-fast intent is good; it just misdirects.

Suggested fix (for the plan) — distinguish the case you're already rescuing specifically:

github_token = ENV.fetch('GITHUB_TOKEN') do
  `gh auth token`.strip
rescue Errno::ENOENT
  raise 'No GitHub token: set GITHUB_TOKEN, or install the gh CLI'
end

Minor accuracy point on the comment: "Resolve once" reads as though this evaluates a single time, but rake_factory invokes a TaskSet config block per task invocation — pipeline:prepare runs two github:* tasks and so shells out to gh auth token twice. Harmless, but "Resolve up front and fail fast" would be truer.

Comment thread Rakefile
circle_ci:checkout_keys:ensure
circle_ci:ssh_keys:ensure
github:deploy_keys:ensure
github:secrets:ensure

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 PLAN CONCERN — plan §4.4 step 6 specifies the ensure spelling. Not a blocker; worth a runbook note.

github:environments:ensure is implemented in rake_github as destroy-then-provision (Tasks::Environments::Ensure invokes destroy then provision, and Environments::Destroy calls client.delete_environment). Deleting a GitHub environment also discards its pending deployments and approval state. github:secrets:ensure likewise briefly deletes ENCRYPTION_PASSPHRASE before re-adding it.

Impact: pipeline:prepare is documented in §7 as idempotent and safe to re-run, but re-running it while a release job is parked awaiting approval would silently cancel that pending deployment; a concurrently starting prerelease/release could also fail its git-crypt unlock during the secret-deletion window.

Suggestion: either note in the runbook that pipeline:prepare is not safe to run while a release awaits approval or a main run is in flight, or — if the plan intends re-runs to be genuinely non-destructive — use github:secrets:provision / github:environments:provision, which are create-or-update.

(Verified positively: both ensure tasks do exist in rake_github 0.17, so pipeline:prepare resolves, and Environments::Destroy rescues Octokit::NotFound — so first-run provisioning against a repo with no release environment won't fail.)

@phelma
phelma marked this pull request as ready for review July 16, 2026 13:04
Copilot AI review requested due to automatic review settings July 16, 2026 13:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Migrates the repository’s CI/CD from CircleCI to GitHub Actions, including pipeline provisioning updates and removal of CircleCI-era scripts/config and credentials handling.

Changes:

  • Add GitHub Actions workflows for PR checks, main branch CI, prerelease, and gated release.
  • Replace CircleCI provisioning/tasks with GitHub-centric provisioning and Slack notifications in the Rakefile.
  • Remove CircleCI configuration and CI shell scripts; update Ruby dependencies and docs for the new CI key location.

Reviewed changes

Copilot reviewed 19 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scripts/ci/steps/test.sh Removed CircleCI test step script (superseded by GitHub Actions jobs).
scripts/ci/steps/release.sh Removed CircleCI release step script (superseded by GitHub Actions release job).
scripts/ci/steps/prerelease.sh Removed CircleCI prerelease step script (superseded by GitHub Actions prerelease job).
scripts/ci/steps/merge-pull-request.sh Removed CircleCI dependabot merge helper (superseded by gh pr merge in PR workflow).
scripts/ci/common/install-slack-deps.sh Removed CircleCI runner dependency install script.
scripts/ci/common/install-gpg-key.sh Removed CircleCI GPG import/decrypt script (replaced by ./go git_crypt:unlock_with_encrypted_gpg_key).
scripts/ci/common/install-git-crypt.sh Removed CircleCI git-crypt installation script (replaced by apt install in Actions jobs).
scripts/ci/common/install-docker.sh Removed CircleCI docker installation script (Actions uses runner docker).
scripts/ci/common/install-docker-compose.sh Removed CircleCI docker-compose installation script (Actions now ensures availability).
scripts/ci/common/install-asdf.sh Removed CircleCI asdf install script (replaced by asdf_install action).
scripts/ci/common/install-asdf-dependencies.sh Removed CircleCI asdf dependencies install script (replaced by asdf_install action).
scripts/ci/common/configure-git.sh Removed CircleCI git author config script (replaced by repository:set_ci_author task).
scripts/ci/common/configure-asdf.sh Removed CircleCI asdf plugin configuration script.
README.md Updated CI key encryption instructions to point to .github/gpg.private.enc.
Rakefile Switched provisioning from CircleCI to GitHub; added Slack notification tasks; updated Docker Hub credentials loading behavior.
Gemfile.lock Updated locked dependencies reflecting CI/tooling changes (e.g., rake_slack, octokit, etc.).
Gemfile Removed CircleCI-related gems; added rake_slack.
.github/workflows/pr.yaml Added PR workflow for check/test and Dependabot auto-merge.
.github/workflows/main.yaml Added main workflow with skip guard, check/test, prerelease, and gated release.
.github/gpg.private.enc Added encrypted GPG private key blob under .github/ for CI unlock flow.
.circleci/config.yml Removed CircleCI pipeline configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/pr.yaml
Comment on lines +57 to +61
- name: Merge pull request
run: gh pr merge --merge "$PR_URL"
env:
PR_URL: ${{ github.event.pull_request.html_url }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Comment thread .github/workflows/pr.yaml
Comment on lines +32 to +39
- name: Ensure docker-compose is available
run: |
if ! command -v docker-compose >/dev/null 2>&1; then
sudo curl -fsSL \
"https://github.com/docker/compose/releases/latest/download/docker-compose-linux-x86_64" \
-o /usr/local/bin/docker-compose
sudo chmod +x /usr/local/bin/docker-compose
fi
Comment on lines +65 to +72
- name: Ensure docker-compose is available
run: |
if ! command -v docker-compose >/dev/null 2>&1; then
sudo curl -fsSL \
"https://github.com/docker/compose/releases/latest/download/docker-compose-linux-x86_64" \
-o /usr/local/bin/docker-compose
sudo chmod +x /usr/local/bin/docker-compose
fi

@phelma phelma left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Code Review: #215 - Migrate pipeline from CircleCI to GitHub Actions

Verdict: COMMENT

Second-pass review through five lenses (correctness, security, safety, standards, code quality) against the Variant B family plan. The diff is a faithful, near-verbatim implementation of plan §4 in both directions — required changes present, nothing beyond the plan touched — and the Pull Request workflow is empirically green at this exact SHA, including the test job that builds the image and runs the compose-backed integration suite. No in-scope blocker. Three findings that earlier passes treated as real defects are disproven below; the substantive new finding is that the decommission deletes credentials without revoking them.

Evidence-based corrections

Three findings were checked against reality rather than taken at face value. All three are false positives and are counted nowhere:

  • 🔍 Disproven — the activesupport / Ruby floor "critical". The correctness lens reported that the refreshed lockfile resolves activesupport 8.1.3 (required_ruby_version >= 3.2.0) and public_suffix 7.0.5 (>= 3.2) while .tool-versions pins ruby 3.1.1, concluding every job would fail at bundle install. The gemspec metadata is accurate (verified against the RubyGems API), and this is a change from main (which held activesupport 7.1.3.4). But the conclusion is wrong: run 29500096772 at SHA ea9b1aa ran check and test to success on /home/runner/.asdf/installs/ruby/3.1.1/bin/ruby — bundler installed the full graph and rspec ran the suite against a real Docker daemon. The version-declaration mismatch is real but demonstrably does not bite.
  • 🔍 Disproven — the arm64-darwin platform leak reported as the one in-scope defect in review 1 (Gemfile.lock:1828, plan §4.5 non-conformance). It does not exist on the pushed head: git show ea9b1aa:Gemfile.lock has PLATFORMS of exactly ruby + x86_64-linux, and grep -i darwin over that blob returns nothing. The local tree is clean and identical to the pushed SHA. §4.5's platform requirement is satisfied.
  • 🔍 Disproven — Layout/ExtraSpacing on the Slack routing rules' trailing comments (standards lens, low confidence, self-flagged for confirmation). bundle exec rubocop Rakefile reports 1 file inspected, no offenses detected.

Plan conformance (independent §4 check, both directions)

Verified directly rather than via the lenses:

  • .circleci/ and scripts/ci/ gone; git grep -i circleci -- ':!Gemfile.lock' matches nothing.
  • config/secrets/ci/ retains exactly encryption.passphrase, gpg.private, gpg.public; no ssh.*. config/secrets/circle_ci/ and config/secrets/github/ are gone. config/secrets/dockerhub/ untouched.
  • GPG key move is a pure rename (git log --follow traces through; 0 insertions, 0 deletions, Bin 4016 bytes).
  • Both workflows parse under YAML.load_file.
  • Slack channel IDs match §3's table exactly; first-match ordering correct with a terminal catch-all.
  • Require and gem lists stay alphabetical; rake_factory/kernel_extensions correctly slotted.
  • spec/dependencies.yml is present, so the compose guard step is load-bearing here rather than a no-op.

No plan violations found in either direction.

Cross-Cutting Themes

  • Deleting a credential is not revoking it (flagged by: security, safety) — the strongest new signal. The decommission removes the CircleCI API token, the stored GitHub PAT, and the CI SSH deploy key pair from the tree, but all three remain live at their providers, and the plan's host-side sweep list covers only "disable the CircleCI project" and "delete the deploy key" — not revocation of the two tokens. This is a gap in something the plan does not specify, and it repeats across ~103 repos.
  • Error handling infers one cause from a broad signal (flagged by: safety, code quality) — the dynamic credentials rescue Psych::SyntaxError treats any parse failure as "tree locked". Plan-mandated (§4.4 step 2a), so a plan concern, but worth revisiting before fleet rollout.

Tradeoff Analysis

  • Parity vs hardening: most lens findings correctly identify real hazards the plan has already considered and deliberately accepted. The plan is the spec; a plan-conformant diff should not be blocked by them. They are preserved below because this shape is about to be replicated fleet-wide, and revisiting is cheaper now than after rollout.

Strengths

  • ✅ Empirically validated: check + test green on a real runner at this SHA, including the compose-backed integration suite — the D8 watch-item (native docker daemon vs CircleCI's remote docker) is now evidenced, not assumed.
  • ✅ Faithful plan execution: workflows match §4.1/§4.2 essentially line-for-line, and nothing outside the plan's allowed file set was touched.
  • skip-ci-check passes the untrusted commit message via env: rather than interpolating ${{ github.event.head_commit.message }} into the shell — the correct, non-obvious idiom.
  • pull_request (not pull_request_target), workflow-level contents: read, narrow per-job escalation — proper least privilege.
  • fetch-depth: 0 on exactly the tag-resolving jobs (test, prerelease, release), correctly omitted from check.
  • ✅ Net credential reduction: SSH deploy key, stored CircleCI token and stored PAT all go, provisioning moves to short-lived ambient gh auth.
  • ✅ The PR description's deliberate-decisions list is exemplary and made this review faster and more accurate.

General Findings

  • 🟡 Security / Safety: Deleted credentials are not revoked — CircleCI API token and GitHub PAT remain live (see Cross-Cutting Themes). Add revocation to the end-of-migration sweep.
  • 🔵 Correctness: rake_github 0.17 adds rbnacl (7.1.2)require 'rbnacl' in secrets/provision.rb, which raises LoadError without native libsodium. Scoped to the host-side ./go pipeline:prepare (plan §7); runners unaffected. Confirm libsodium before provisioning.
  • 🔵 Residual risk (not a defect): only pr.yaml is proven. main.yaml's prerelease/release jobs — git-crypt unlock, tagging, Docker Hub publish, the release environment gate — have never executed and remain unvalidated until the first post-merge run.

Plan concerns (not blockers)

These challenge decisions the plan or PR description documents as deliberate. None should block this PR; recorded so a human can revisit the plan before fleet rollout. Most were posted inline on review 1 and are not re-posted here.

  • 🟡 The single main concurrency group is held during release approval, so an unapproved run stalls all main CI (check/test included), not just prereleases — up to the 30-day approval window. Plan §4.2 verbatim. The most valuable item to revisit.
  • 🔵 docker-compose installed from an unpinned releases/latest URL with no checksum, sudo-installed to /usr/local/bin. Plan §4.1/§4.2 verbatim.
  • 🔵 git pull runs before Set CI git author in release; benign for fast-forwards, fails if a merge commit is ever needed. Plan §4.2 ordering.
  • 🔵 rescue Psych::SyntaxError → nil conflates "tree locked" with "credentials corrupt"; on the release path a half-failed unlock publishes unauthenticated. Plan §4.4 step 2a verbatim.
  • 🔵 openssl aes-256-cbc -md sha1 without -pbkdf2 protects a GPG private key committed to a public repo. Plan §4.6 says leave the openssl commands untouched.
  • 🔵 No timeout-minutes on check/test (6-hour default) or on the dependabot merge job; prerelease/release correctly bounded at 30.
  • 🔵 'Resolve once' comment does not match rake_factory's per-task-run block evaluation. Plan §4.4 step 3 verbatim.
  • 🔵 All github:* tasks now require both a resolvable token and a readable passphrase file, because both resolve in the config block. Plan-mandated shape.
  • 🔵 Rollback is a clean single git revert while the sweep is pending; worth noting in the plan that the sweep is the point of no easy return.

Review generated by /accelerator:review-pr (round 1, pass 2 — supersedes the earlier automated review on this PR)

Comment thread Rakefile
Comment on lines +158 to 161
t.secrets = [
{ name: 'ENCRYPTION_PASSPHRASE',
value: File.read('config/secrets/ci/encryption.passphrase').chomp }
]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Code Quality — in-scope (minor)

On a git-crypt locked clone this File.read succeeds and returns ciphertext, which github:secrets:ensure then uploads as the ENCRYPTION_PASSPHRASE Actions secret with no error. Plan §7 states an unlocked clone as an operator precondition, but nothing enforces it.

Impact: the mis-provision is silent at the point of the mistake and only surfaces much later as an opaque GPG import failure in prerelease/release, with nothing tying it back to pipeline:prepare having been run in a locked tree.

Suggestion: guard using the marker this Rakefile already establishes at Rakefile:105-106:

unless File.exist?('config/secrets/.unlocked')
  raise 'Secrets are locked: run `./go git_crypt:unlock` before provisioning'
end

Fleet-wide concern, so possibly better fixed once in the family plan than here.

@phelma phelma left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Code Review: #215 - Migrate pipeline from CircleCI to GitHub Actions

Verdict: COMMENT — no blocking findings.

This is not an approval. I am running as the same GitHub identity that authored this PR, so GitHub correctly refuses a formal approval from me, and a comment review is not a substitute for one. This posts the lens findings only; the merge decision needs a second party who did not write the diff.

This cutover is a faithful, high-fidelity execution of the Variant B family plan. I checked the diff against the plan's §4 in both directions — every required change is present, and nothing beyond the plan has been touched. The two workflow files and the Rakefile changes match the plan's specified shape essentially verbatim; src/, spec/, lib/version.rb, go, .tool-versions, .envrc, .gitattributes and .github/dependabot.yml are all untouched; the GPG key move is byte-identical (R100); config/secrets/ ends up with exactly the intended contents (ssh.*, circle_ci/ and github/ gone; dockerhub/ untouched); git grep -i circleci is clean outside Gemfile.lock; and the lockfile carries rake_slack 0.3.0, rake_git_crypt 0.4.0, rake_github 0.17.0 with PLATFORMS correctly at ruby + x86_64-linux.

Five lenses (correctness, security, safety, standards, code quality) found no in-scope defects — no deviation from the plan, and no genuine defect in anything the plan leaves unspecified. Every substantive finding below challenges something the plan or PR description documents as deliberate. These are recorded as plan concerns: they are valuable input for post-migration hardening and for the family plan itself, but they must not block a plan-conformant diff. They are posted inline so a human can revisit them at the family level rather than per-repo.

Cross-Cutting Themes

  • Docker Hub credentials rescue infers "locked tree" from a parse error (flagged by: correctness, safety, code quality) — the plan's §4.4 step 2a prescribes this exact rescue Psych::SyntaxError block, so it is plan-conformant. All three lenses independently reached the same conclusion: the rescue is unconditional and so also applies to prerelease/release, where nil credentials are always wrong; and it is narrower than the failure set it implies (Errno::ENOENT on a missing file escapes). Combined with the documented tag-before-publish ordering, a silent degradation to an unauthenticated push surfaces after the tag is already public. The repo already has an explicit lock marker (config/secrets/.unlocked) that would make the intent stated rather than inferred. This is the single strongest candidate for a family-plan amendment.
  • The docker-compose guard fetches releases/latest unpinned and unverified (flagged by: security, safety, standards, code quality) — again exactly as the plan's §4.1/§4.2 YAML specifies. Four lenses converged: it is root-installed via sudo with no version pin and no checksum, on main.yaml runners that hold ENCRYPTION_PASSPHRASE and Docker Hub push credentials; it is the one CI dependency whose version is declared nowhere (everything else goes through .tool-versions + asdf_install); and it is duplicated verbatim across both workflows. Worth a family-level decision on pinning + sha256sum, or on moving the guard behind dependencies:test:provision.
  • Concurrency group scope (flagged by: correctness, safety) — group: main spans the whole workflow including the approval-gated release job. Two distinct consequences: a queued run's prerelease (deliberately un-pulled) would hit a non-fast-forward on repo.push('origin', 'main', tags: true), orphaning the tag it just created; and while an approval is pending, no checks or tests run on main at all, for up to 30 days. Both follow from the plan's exact YAML.

Tradeoff Analysis

  • Parity vs hardening: this is the defining tension of the whole review, and the plan resolves it explicitly — the cutover reproduces CircleCI's behaviour, warts included, and inherited hazards are documented rather than fixed. Every lens pushed toward hardening (pin the binary, tighten the merge gate, scope the passphrase to an environment, assert the release gate exists). The plan's position is the correct one for a fleet migration: uniform, reviewable, low-variance cutovers now; hardening as a deliberate fleet-wide sweep later. Recommendation: merge as-is; route the concerns above to the post-migration backlog.
  • Fail-fast vs fail-soft: the Rakefile fails fast on a missing GitHub token (raises with an actionable message) but fails soft on unreadable Docker Hub credentials (rescues to nil). Both behaviours are plan-specified, but they are opposite instincts a few lines apart in the same file, and the fail-soft one sits on the more consequential path.

Strengths

  • ✅ Textbook plan conformance: workflows, Rakefile, Gemfile, README and the decommission deletions all match §4 exactly, with no scope creep. The git grep -i circleci sweep is clean and the GPG move is a true rename.
  • github.event.head_commit.message is bound via env: rather than interpolated into the run: script — the correct mitigation for GitHub Actions script injection, and the single easiest thing to get wrong in this diff.
  • ✅ Least-privilege permissions: workflow default contents: read, with contents: write elevated only on prerelease/release and pull-requests: write only on the merge job.
  • ✅ Real reduction in stored long-lived secret material: the CircleCI API token and the GitHub PAT are deleted, and provisioning moves to the operator's ambient short-lived gh credential.
  • ENCRYPTION_PASSPHRASE is deliberately kept out of pr.yaml, so untrusted PR runs never receive the git-crypt passphrase — and the lazy credentials load exists to support that boundary rather than to work around it.
  • rake_git_crypt 0.4.0's default encrypted_key_path is .github/gpg.private.enc, exactly matching the file's new location — verified, no override needed.
  • ✅ The dynamic block is mechanically correct: rake_factory resolves parameters at task-run time, and RakeDocker's Build/Push both guard with Docker.authenticate!(t.credentials) if t.credentials, so nil credentials cleanly skip auth.
  • ✅ Slack notify steps are continue-on-error: true + if: ${{ !cancelled() }} — a Slack outage degrades gracefully and can never block the dependabot auto-merge.
  • ✅ Comments explain why, not what: the release-pull-vs-prerelease-no-pull parity note, the queue: max rationale with its GA date, and the Slack failure-routing choice all pre-empt exactly the questions the next maintainer would ask.
  • ✅ The PR description is exemplary — it enumerates the deliberate decisions precisely enough that this review could separate defects from plan concerns without guesswork.

False Positives (verified, no action)

  • 🔵 Standards flagged ./go test:code:check as drift from the family-standard ./go library:check. Disproven: plan §4.2 parameterises this step to mirror the repo's own scripts/ci/steps/test.sh, and I confirmed against main:scripts/ci/steps/test.sh that it ran ./go test:code:check followed by ./go test:integration. The workflows are correct as written. (The PR description's mention of library:check refers to the plan's §5 self-verification gate, which is a different thing.)

General Findings

All findings are plan concerns; none is an in-scope defect. Counts: 0 critical, 0 major, 0 minor, 0 suggestion; 19 plan concerns; 1 false positive.

  • 🔵 Security: The release job publishes main at approval time rather than the tested SHA (documented parity with the old release.sh). From a threat perspective this is a TOCTOU on the pipeline's one human control — the audit trail records an approval for a tree the approver never saw, and the window is as long as the approval takes.
  • 🔵 Security: The CircleCI deploy key stays live on the repo until the end-of-migration sweep (explicitly out of scope per plan §1). Please make sure the sweep issue lists both revoking that key and disabling the CircleCI project, whose stored ENCRYPTION_PASSPHRASE remains a live copy of a secret that still unlocks this tree.
  • 🔵 Security: actions/checkout@v4 and asdf_install@v1 track mutable tags (documented deliberate for asdf_install). If the family ever adopts SHA pinning, do it fleet-wide rather than per-repo — the value is in uniformity.
  • 🔵 Safety: An un-actioned release approval occupies the main concurrency group for up to 30 days, queuing all subsequent main pushes. Quiet failure mode (runs show as queued, not failed), fast recovery once noticed. Scoping serialisation to just prerelease/release would keep tag-race protection while letting check/test flow.
  • 🔵 Code Quality: The repeated per-job scaffolding is explicitly a §1 parity decision and is self-consistent today. Noted only as a watch-point: a future change to the notify contract touches five places.
  • 🔵 Code Quality: Deleting scripts/ci/steps/*.sh is right (they were CircleCI-shaped), but the prerelease/release sequences they encoded now live only as ordered step lists in YAML. Composite pipeline:prerelease/pipeline:release rake tasks would restore local reproducibility — post-migration, consistent with the PR's own "logic lives in the build system" principle.
  • 🔵 Standards: Cross-checked against five already-migrated siblings (ruby_terraform, lino, rake_git_crypt, rake_dependencies, rspec-terraform) rather than only terraform_pkl_type_gen (which predates the current convention): this PR is an essentially exact match to current house style. Separately, Gemfile.lock's PLATFORMS handling is genuinely inconsistent across the family, so there is no convention to enforce — but this sweep is the natural moment to set one if the team wants it.

Review generated by /accelerator:review-pr

Comment thread Rakefile
Comment on lines +219 to +223
t.credentials = dynamic do
YAML.load_file('config/secrets/dockerhub/credentials.yaml')
rescue Psych::SyntaxError
nil # tree locked: skip Docker Hub auth (base image is public)
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — plan §4.4 step 2a prescribes this exact block) | Flagged independently by: correctness, safety, code quality

Three lenses converged here, which is why it leads the review. Two properties of this rescue are worth taking back to the family plan:

1. The rescue is unconditional, so it applies to prerelease/release too, where nil credentials are always wrong. If git-crypt ever fails to decrypt this particular file while the unlock step still reports success (e.g. a rotated passphrase), RakeDocker::Tasks::Push skips Docker.authenticate! and attempts an unauthenticated push to infrablocks/prometheus-aws. That surfaces as an opaque Docker denied, not "the tree is locked". Because version:bump[rc]/version:release push the tag before image:publish (the documented pre-existing ordering), the failure lands after the tag is already public — leaving a version tag with no image and requiring manual tag cleanup.

2. The rescue is narrower than the failure set it implies: a missing credentials.yaml raises Errno::ENOENT and escapes uncaught, and a real YAML typo in an unlocked tree is silently swallowed as "locked". A reader of nil cannot tell which of the three states produced it.

This is also the one place in the file that fails soft, a few lines below the github_token resolution that deliberately fails fast with an actionable message — opposite instincts on the more consequential path.

Suggested plan amendment (family-wide, not this PR): the repo already has an explicit lock marker, used by secrets:directory:ensure. Gating on it makes the intent stated rather than inferred, and keeps genuine parse/IO errors loud:

t.credentials = dynamic do
  next nil unless File.exist?('config/secrets/.unlocked')

  YAML.load_file('config/secrets/dockerhub/credentials.yaml')
end

Comment on lines +65 to +72
- name: Ensure docker-compose is available
run: |
if ! command -v docker-compose >/dev/null 2>&1; then
sudo curl -fsSL \
"https://github.com/docker/compose/releases/latest/download/docker-compose-linux-x86_64" \
-o /usr/local/bin/docker-compose
sudo chmod +x /usr/local/bin/docker-compose
fi

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — plan §4.1/§4.2 specify this exact step) | Flagged independently by: security, safety, standards, code quality

Four lenses converged. The guard is documented as deliberate; the concerns are about properties of the guard that the plan doesn't address:

Supply chainreleases/latest is a mutable pointer resolved at run time. The bytes are never pinned, checksummed or signed, yet are installed to /usr/local/bin with sudo and executed. On main.yaml the surrounding jobs hold ENCRYPTION_PASSPHRASE, the unlocked secrets tree, and Docker Hub push credentials — so a compromised upstream artefact could exfiltrate the git-crypt passphrase or tamper with the published infrablocks/prometheus-aws image, with no commit in this repo to correlate against.

Reproducibility — every other tool in this pipeline is pinned via .tool-versions + asdf_install@v1. This is the one CI dependency whose version is declared nowhere, so a breaking compose release changes behaviour fleet-wide simultaneously and bisects to nothing.

Duplication — the block is repeated verbatim in pr.yaml. The PR description justifies repeating scaffolding on the grounds that logic lives in the build system; this is imperative provisioning logic in CI YAML, so it's the one spot that cuts against that principle.

Suggested plan amendments (in preference order):

  1. Point dependencies:test:provision/destroy at the docker compose plugin already present on ubuntu-latest — the guard step and its download disappear entirely.
  2. Failing that, factor the guard into a composite action in infrablocks/github-actions alongside asdf_install, with a pinned version and a checksum:
env:
  COMPOSE_VERSION: v2.32.4
  COMPOSE_SHA256: <sha>
run: |
  if ! command -v docker-compose >/dev/null 2>&1; then
    curl -fsSL "https://github.com/docker/compose/releases/download/${COMPOSE_VERSION}/docker-compose-linux-x86_64" -o /tmp/docker-compose
    echo "${COMPOSE_SHA256}  /tmp/docker-compose" | sha256sum -c -
    sudo install -m 0755 /tmp/docker-compose /usr/local/bin/docker-compose
  fi

Comment on lines +15 to +18
concurrency:
group: main
cancel-in-progress: false
queue: max

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — this is the plan's exact YAML) | Flagged independently by: correctness, safety

The main group spans the whole workflow, including the approval-gated release job. Two distinct consequences follow, and the plan's D4 note ("nothing lands on main at all") doesn't quite cover either:

1. Queued prereleases can fail on a rejected push. With queue: max, a run whose release job awaits approval holds the group; subsequent pushes queue behind it. When a queued run starts, actions/checkout fetches at github.sha (the old commit), leaving local main behind origin/main — and prerelease deliberately has no git pull. version:bump[rc] then calls repo.push('origin', 'main', tags: true), which the git gem implements as two commands: git push origin main, then git push --tags origin. The first is a non-fast-forward and is rejected — so the job fails and the tag repo.add_tag just created is never pushed, orphaned in the workspace. In the normal case local main matches origin and the push is a harmless no-op, which is why D4's framing holds day-to-day; serialisation makes the divergent case the expected outcome whenever an approval is slow, rather than a rare race.

2. A pending approval freezes all main-branch CI. While release waits (GitHub holds approvals pending for up to 30 days), the group is occupied, so no check or test runs on main at all — not just no prereleases. queue: max retains every queued run, so the backlog grows. One forgotten approval silently stops main-branch feedback for the whole team; runs show as queued, not failed, so it's easy to miss for days. Recovery is fast once diagnosed.

Suggested plan amendment: scope serialisation to the jobs that actually race on version tags rather than the whole workflow — drop the workflow-level concurrency and put concurrency: { group: main-publish, cancel-in-progress: false, queue: max } on prerelease and release. check/test mutate no shared state and are safe to run in parallel across main pushes. This preserves the tag-race protection while keeping feedback flowing. Giving prerelease a git pull would separately address (1).

needs: [prerelease]
runs-on: ubuntu-latest
timeout-minutes: 30
environment: release

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — plan §7 covers provisioning) | Lens: safety

The approval gate depends entirely on the release environment existing with its reviewer protection rule. GitHub auto-creates an environment on first reference if it doesn't exist — and the auto-created environment has no protection rules. The environment is only created out-of-band by ./go pipeline:prepare, a manual operator step nothing in the workflow verifies.

Impact: if pipeline:prepare hasn't run against a repo before its first push to main (or if the environment is later deleted), release runs unapproved and publishes a full release image plus a release tag with no human in the loop — and the workflow reports success, so there is no signal the gate was absent. That's a fail-open default on the pipeline's most consequential job, at exactly the moment in a fleet migration when the provisioning step is most likely to be missed on some repo.

The plan's §7 verification (gh api .../environments/release --jq '.protection_rules') catches this if the operator runs it. Worth considering whether the family plan should make the gate self-verifying rather than checklist-verified:

      - name: Assert release was approved
        env:
          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: |
          rules=$(gh api "repos/${{ github.repository }}/environments/release" \
            --jq '.protection_rules | length')
          [ "$rules" -gt 0 ] || { echo "release environment has no protection rules"; exit 1; }

For this PR specifically: please confirm §7 provisioning was run and that release shows a maintainers required reviewer before merging.

Comment thread .github/workflows/pr.yaml
Comment on lines +21 to +22
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — plan §7 documents this as accepted and deferred to issue 19) | Lens: correctness

Recording the precise mechanism, since the plan states the consequence but not the why: workflows triggered by Dependabot on pull_request resolve secrets.* from the Dependabot store, not the Actions store — repository and organisation Actions secrets arrive as empty strings. So SLACK_BOT_TOKEN is unset in exactly the runs the Rakefile's dependabot[bot] routing rules exist to serve.

RakeSlack::Tasks::Notify declares parameter :bot_token, required: true, so the task raises RakeFactory::RequiredParameterUnset — and continue-on-error: true swallows it, so the failure is silent.

Net effect until issue 19 lands: the CircleCI config explicitly notified builds-dependabot on both pass and fail; after cutover those notifications stop, and the two dependabot[bot] routing rules in the Rakefile are dead code. Nothing surfaces the loss. This matches the plan's expectation ("Dependabot-triggered runs will fail their notify steps until that lands — accepted"), so no action for this PR — noted so the scope of the gap is on record: it's a silent regression against CircleCI behaviour, not merely a missing new feature.

Comment thread .github/workflows/pr.yaml
Comment on lines +52 to +55
if: github.actor == 'dependabot[bot]'
permissions:
contents: write
pull-requests: write

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — D3 specifies this gate) | Lens: security

The auto-merge job's only authorisation gate is github.actor == 'dependabot[bot]', and it grants itself contents: write + pull-requests: write. github.actor reflects whoever triggered the most recent event — not who authored the PR, and not where the head branch lives.

Impact: a single-condition gate on the one job in this repo that writes to main without human review, feeding straight into the release pipeline. Defence in depth here is free:

if: >-
  github.actor == 'dependabot[bot]' &&
  github.event.pull_request.user.login == 'dependabot[bot]' &&
  github.event.pull_request.head.repo.full_name == github.repository

Separately — a mechanism worth pinning down in the plan. The PR description says the merge doesn't trigger a release build because "on CircleCI the merge commit carried [skip ci], so this matches". The outcome matches, but the mechanism differs: gh pr merge --merge produces a default merge commit with no [skip ci] marker, so skip-ci-check never fires on it. The property holds solely because pushes authenticated with secrets.GITHUB_TOKEN don't trigger workflow runs — which D3 does state correctly. That safeguard is real but load-bearing and invisible: if GH_TOKEN is ever swapped for a PAT or App token (a common change, e.g. to satisfy branch protection), every dependabot merge would silently start tagging a version and publishing an rc image with no human in the loop. Passing a marked subject would make the intent explicit and restore literal parity with the old merge-pull-request.sh ("%s [skip ci]"), giving skip-ci-check something to catch.

Comment on lines +128 to +143
- name: Pull latest main
# Approval can land long after the run starts; release publishes main
# as of approval time, not the tested SHA — parity with the old
# release.sh, which also pulled. prerelease.sh never pulled, so the
# prerelease job deliberately has no pull.
run: git pull
- name: Install tools
uses: infrablocks/github-actions/asdf_install@v1
- name: Install secrets tools
run: sudo apt-get update && sudo apt-get install -y git-crypt gnupg
- name: Unlock git-crypt
run: ./go git_crypt:unlock_with_encrypted_gpg_key
env:
ENCRYPTION_PASSPHRASE: ${{ secrets.ENCRYPTION_PASSPHRASE }}
- name: Set CI git author
run: ./go repository:set_ci_author

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — this step order is the plan's exact YAML) | Lens: correctness

git pull runs before ./go repository:set_ci_author. It fast-forwards in the common case, but if it needs to create a merge commit, git aborts with "Committer identity unknown / Please tell me who you are" — no user.name/user.email is configured yet.

This inverts the old CircleCI ordering, where configure_tools (including configure-git.sh, which set the identity) ran before release.sh performed its git pull. So it's a small, probably unintended parity break rather than a carried-over wart.

Impact: a rare but real failure mode where the release job dies on the pull with a confusing identity error — precisely when main has diverged, which is when the release matters most.

Suggested plan amendment: move Set CI git author above Pull latest main to restore the original ordering. Alternatively git pull --ff-only, so a diverging main fails loudly and unambiguously instead of attempting a merge it cannot author.

Comment thread Rakefile
Comment on lines +158 to 161
t.secrets = [
{ name: 'ENCRYPTION_PASSPHRASE',
value: File.read('config/secrets/ci/encryption.passphrase').chomp }
]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔵 Plan concern (not a defect — plan §4.4 step 3 specifies this exactly) | Lens: security

The comment is accurate — pr.yaml never unlocks git-crypt, so dependabot runs genuinely don't need the passphrase. The concern is that this is a property of today's workflow files, not an enforced boundary: a repository-level Actions secret is readable by any workflow run from any in-repo branch.

Impact: anyone who can push a branch to this repo can author a workflow that echoes the git-crypt passphrase, which decrypts the CI GPG key and the Docker Hub push credentials. The environment: release gate protects the release action, not this secret.

Scoping the passphrase to an environment would enforce the boundary rather than document it. That needs a little thought, since prerelease is intentionally ungated — it would want a non-reviewer-gated environment (e.g. build) with a branch policy restricting it to main, referenced by both prerelease and release. Given the passphrase is only ever used on main workflows, that shape would fit.

Raising for the family plan to rule on — if repo-level scope is an accepted trade-off for the migration, no change needed here.

phelma added 4 commits July 16, 2026 15:01
Gemfile.lock is resolved under ruby 3.2 (activesupport 8.1.3 requires
>= 3.2); under ruby 3.1.1 bundler re-resolves and rewrites the lockfile,
leaving the tree dirty and blocking git-crypt unlock in CI.
The lock recorded nokogiri 1.18.8-x86_64-linux, a platform variant that
does not exist on rubygems (1.18.x ships -gnu/-musl variants), so bundle
install fails under the now-honoured lockfile.
@phelma phelma closed this Jul 17, 2026
@phelma
phelma deleted the gha-migration branch July 17, 2026 13:57
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.

2 participants