Migrate pipeline from CircleCI to GitHub Actions#215
Conversation
16036d7 to
60d5331
Compare
phelma
left a comment
There was a problem hiding this comment.
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
activesupport8.1.3 (which declaresrequired_ruby_version >= 3.2.0) while.tool-versionspinsruby 3.1.1, concluding that "every job fails at toolchain bootstrap, so the migrated pipeline cannot run at all". Thecheckjob passed on a real runner at this exact SHA in 43s (run 29500096772), completingbundle installover 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-pendingtestjob uses the same bundle and will confirm. - 🔍 Confirmed and sharpened — the standards lens flagged an
arm64-darwinplatform leak inGemfile.lockat 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
mainconcurrency 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 thereleaseenvironment approval holdsgroup: main, so withcancel-in-progress: false+queue: maxevery later push tomainqueues behind it for up to the 30-day approval window. Since the modal push tomainis one nobody releases, this is arguably the normal case, not the edge case — and it is a regression from CircleCI, where theholdjob 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 installof docker-compose, the mutableasdf_install@v1tag, 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 holdsENCRYPTION_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 (
dynamiccredentials rescue,github_tokenresolution) 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 oncheck/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-checkpasses the untrusted commit message viaenv:rather than interpolating${{ github.event.head_commit.message }}into the shell — the correct, non-obvious idiom, applied without prompting. A nullhead_commitdegrades to the safe "run CI" default. - ✅
pull_request(notpull_request_target), workflow-levelcontents: read, and narrow per-job escalation (contents: writeonly on prerelease/release,pull-requests: writeonly on the merge job) — proper least privilege. - ✅
fetch-depth: 0applied to exactly the tag-resolving jobs (test, prerelease, release) and correctly omitted fromcheck. - ✅ Slack routing verified against the
rake_slacksource: first-match-wins resolves correctly for every reachable outcome/actor/type combination, with a terminal catch-all soNoMatchingRulecannot 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
ghauth.keys:gpgcorrectly 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 pullparity note, thequeue: maxchangelog link, the "Actions store only" note ont.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-minutesoncheck/test(both workflows) — they fall back to the 360-minute default, and a hungtestonmainholds the concurrency group for six hours. Plan's exact YAML sets timeouts only on prerelease/release. - 🔵 Correctness: the
merge-pull-requestjob drops the CircleCImerge_pull_requestjob's Slack notify step, so a failed auto-merge is silent. Low impact (dependabot runs have noSLACK_BOT_TOKENuntil issue 19 lands); worth folding into that follow-up. - 🔵 Security:
git-crypt unlockimports 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 sha1recipe (pre-existing; plan says leave untouched) uses the legacyEVP_BytesToKeyKDF to protect a GPG key whose ciphertext is public.-pbkdf2 -iter 600000is the fleet-wide fix; ciphertext and recipe must change in lockstep. - 🔵 Security: dependabot-triggered
pull_requestruns get a restrictedGITHUB_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 topull_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:ensureare destroy-then-provision in rake_github, so re-runningpipeline:prepare(documented as idempotent) while a release awaits approval would silently cancel that pending deployment. Consider theprovisionvariants, 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 holdstep relies on GHA's implicitif: success()default while every neighbouring notify step has an explicitif:. The default is load-bearing — it is the only thing preventing an "on hold" message after a failedPrereleasestep. Anyone "tidying up" for consistency would break it. - 🔵 Code quality: the two Slack fall-through rules hard-code
format: :failurewhile 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
dynamicwrapper is inaccurate. Tracingrake_factory,resolve_blockdefers the config block into the rake action, so the eagerYAML.load_filenever fired at Rakefile load time — it fired whenimage:buildran. 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
| unicode-emoji (4.0.4) | ||
| uri (0.13.2) | ||
| uri (1.1.1) | ||
|
|
||
| PLATFORMS | ||
| arm64-darwin-21 |
There was a problem hiding this comment.
🔵 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
PLATFORMSsection retains the genericrubyandx86_64-linuxplatforms and contains no operator-local platforms (e.g.arm64-darwin-*): a barebundle lock/bundle updateon a Mac can addarm64-darwin-25and dropruby.
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.
| concurrency: | ||
| group: main | ||
| cancel-in-progress: false | ||
| queue: max |
There was a problem hiding this comment.
🟡 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.
| 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 |
There was a problem hiding this comment.
🟡 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 |
There was a problem hiding this comment.
🟡 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.
| 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 |
There was a problem hiding this comment.
🟡 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):
- Detect lock state directly rather than by exception type — git-crypt ciphertext starts with a
\0GITCRYPTmagic prefix, sonext nil if File.binread(path, 9) == "\x00GITCRYPT"states the intent precisely and sidesteps a robustness question worth confirming on the runner:YAML.load_fileover binary ciphertext may raise an encodingArgumentErrorrather thanPsych::SyntaxError, which the current rescue would not catch at all. - Have the
prerelease/releasejobs 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.)
| env: | ||
| PR_URL: ${{ github.event.pull_request.html_url }} | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🔵 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 |
There was a problem hiding this comment.
🔵 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.
| merge-pull-request: | ||
| needs: [check, test] | ||
| runs-on: ubuntu-latest | ||
| if: github.actor == 'dependabot[bot]' |
There was a problem hiding this comment.
🔵 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.
| 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 |
There was a problem hiding this comment.
🔵 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'
endMinor 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.
| circle_ci:checkout_keys:ensure | ||
| circle_ci:ssh_keys:ensure | ||
| github:deploy_keys:ensure | ||
| github:secrets:ensure |
There was a problem hiding this comment.
🔵 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.)
There was a problem hiding this comment.
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.
| - 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 }} |
| - 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 |
| - 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
left a comment
There was a problem hiding this comment.
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 resolvesactivesupport 8.1.3(required_ruby_version >= 3.2.0) andpublic_suffix 7.0.5(>= 3.2) while.tool-versionspinsruby 3.1.1, concluding every job would fail atbundle install. The gemspec metadata is accurate (verified against the RubyGems API), and this is a change frommain(which heldactivesupport 7.1.3.4). But the conclusion is wrong: run 29500096772 at SHAea9b1aarancheckandtestto 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-darwinplatform 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.lockhasPLATFORMSof exactlyruby+x86_64-linux, andgrep -i darwinover that blob returns nothing. The local tree is clean and identical to the pushed SHA. §4.5's platform requirement is satisfied. - 🔍 Disproven —
Layout/ExtraSpacingon the Slack routing rules' trailing comments (standards lens, low confidence, self-flagged for confirmation).bundle exec rubocop Rakefilereports1 file inspected, no offenses detected.
Plan conformance (independent §4 check, both directions)
Verified directly rather than via the lenses:
.circleci/andscripts/ci/gone;git grep -i circleci -- ':!Gemfile.lock'matches nothing.config/secrets/ci/retains exactlyencryption.passphrase,gpg.private,gpg.public; nossh.*.config/secrets/circle_ci/andconfig/secrets/github/are gone.config/secrets/dockerhub/untouched.- GPG key move is a pure rename (
git log --followtraces 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_extensionscorrectly slotted. spec/dependencies.ymlis 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
dynamiccredentialsrescue Psych::SyntaxErrortreats 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+testgreen 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-checkpasses the untrusted commit message viaenv:rather than interpolating${{ github.event.head_commit.message }}into the shell — the correct, non-obvious idiom. - ✅
pull_request(notpull_request_target), workflow-levelcontents: read, narrow per-job escalation — proper least privilege. - ✅
fetch-depth: 0on exactly the tag-resolving jobs (test, prerelease, release), correctly omitted fromcheck. - ✅ Net credential reduction: SSH deploy key, stored CircleCI token and stored PAT all go, provisioning moves to short-lived ambient
ghauth. - ✅ 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_github0.17 addsrbnacl (7.1.2)→require 'rbnacl'insecrets/provision.rb, which raisesLoadErrorwithout 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.yamlis proven.main.yaml'sprerelease/releasejobs — git-crypt unlock, tagging, Docker Hub publish, thereleaseenvironment 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
mainconcurrency 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/latestURL with no checksum,sudo-installed to/usr/local/bin. Plan §4.1/§4.2 verbatim. - 🔵
git pullruns beforeSet CI git authorinrelease; benign for fast-forwards, fails if a merge commit is ever needed. Plan §4.2 ordering. - 🔵
rescue Psych::SyntaxError → nilconflates "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 sha1without-pbkdf2protects a GPG private key committed to a public repo. Plan §4.6 says leave the openssl commands untouched. - 🔵 No
timeout-minutesoncheck/test(6-hour default) or on the dependabot merge job;prerelease/releasecorrectly 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 revertwhile 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)
| t.secrets = [ | ||
| { name: 'ENCRYPTION_PASSPHRASE', | ||
| value: File.read('config/secrets/ci/encryption.passphrase').chomp } | ||
| ] |
There was a problem hiding this comment.
🔵 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'
endFleet-wide concern, so possibly better fixed once in the family plan than here.
phelma
left a comment
There was a problem hiding this comment.
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::SyntaxErrorblock, so it is plan-conformant. All three lenses independently reached the same conclusion: the rescue is unconditional and so also applies toprerelease/release, where nil credentials are always wrong; and it is narrower than the failure set it implies (Errno::ENOENTon 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-composeguard fetchesreleases/latestunpinned 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 viasudowith no version pin and no checksum, onmain.yamlrunners that holdENCRYPTION_PASSPHRASEand 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 behinddependencies:test:provision. - Concurrency group scope (flagged by: correctness, safety) —
group: mainspans the whole workflow including the approval-gatedreleasejob. Two distinct consequences: a queued run'sprerelease(deliberately un-pulled) would hit a non-fast-forward onrepo.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 circlecisweep is clean and the GPG move is a true rename. - ✅
github.event.head_commit.messageis bound viaenv:rather than interpolated into therun: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, withcontents: writeelevated only onprerelease/releaseandpull-requests: writeonly 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
ghcredential. - ✅
ENCRYPTION_PASSPHRASEis deliberately kept out ofpr.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_crypt0.4.0's defaultencrypted_key_pathis.github/gpg.private.enc, exactly matching the file's new location — verified, no override needed. - ✅ The
dynamicblock is mechanically correct:rake_factoryresolves parameters at task-run time, andRakeDocker's Build/Push both guard withDocker.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: maxrationale 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:checkas drift from the family-standard./go library:check. Disproven: plan §4.2 parameterises this step to mirror the repo's ownscripts/ci/steps/test.sh, and I confirmed againstmain:scripts/ci/steps/test.shthat it ran./go test:code:checkfollowed by./go test:integration. The workflows are correct as written. (The PR description's mention oflibrary:checkrefers 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
releasejob publishesmainat approval time rather than the tested SHA (documented parity with the oldrelease.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
CircleCIdeploy 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 storedENCRYPTION_PASSPHRASEremains a live copy of a secret that still unlocks this tree. - 🔵 Security:
actions/checkout@v4andasdf_install@v1track mutable tags (documented deliberate forasdf_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
mainconcurrency 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 justprerelease/releasewould keep tag-race protection while lettingcheck/testflow. - 🔵 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/*.shis right (they were CircleCI-shaped), but the prerelease/release sequences they encoded now live only as ordered step lists in YAML. Compositepipeline:prerelease/pipeline:releaserake 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 onlyterraform_pkl_type_gen(which predates the current convention): this PR is an essentially exact match to current house style. Separately,Gemfile.lock'sPLATFORMShandling 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
| 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 |
There was a problem hiding this comment.
🔵 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| - 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 |
There was a problem hiding this comment.
🔵 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 chain — releases/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):
- Point
dependencies:test:provision/destroyat thedocker composeplugin already present onubuntu-latest— the guard step and its download disappear entirely. - Failing that, factor the guard into a composite action in
infrablocks/github-actionsalongsideasdf_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| concurrency: | ||
| group: main | ||
| cancel-in-progress: false | ||
| queue: max |
There was a problem hiding this comment.
🔵 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 |
There was a problem hiding this comment.
🔵 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.
| env: | ||
| SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} |
There was a problem hiding this comment.
🔵 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.
| if: github.actor == 'dependabot[bot]' | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write |
There was a problem hiding this comment.
🔵 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.repositorySeparately — 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.
| - 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 |
There was a problem hiding this comment.
🔵 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.
| t.secrets = [ | ||
| { name: 'ENCRYPTION_PASSPHRASE', | ||
| value: File.read('config/secrets/ci/encryption.passphrase').chomp } | ||
| ] |
There was a problem hiding this comment.
🔵 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.
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.
Cutover to GitHub Actions per the Variant B family plan (docker image).
Includes decommission — merging this PR completes the repo's migration.
releaseenvironment gate.github/CircleCI's
setup_remote_docker+ install scripts); the integration suitebuilds the image and runs its docker-compose dependencies on the runner
dynamicblock, nil when the treeis locked) so PR checks run without unlocking git-crypt — the one
deliberate change inside the
imagenamespacerake_slack; dependabot auto-merge jobrake_githubsecrets/environments;rake_circle_cidropped.circleci/,scripts/ci/, the CI SSH deploykey pair and its
keys:deploy/deploy_keysprovisioning, and the storedCircleCI/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:releasetag and push beforeimage:publishruns — a failed publish leaves a version tag with no imagebehind it. Pre-existing ordering inside the untouched release logic.
mainwith noapproval gate; only full releases are gated (
environment: release).version:bump[rc]on a released version increments minor and startsrc.1— every release cycle is a minor bump. Pre-existinglib/version.rblogic.merge does not trigger a release build — on CircleCI the merge commit
carried
[skip ci], so this matches. Updates ship with the nexthuman-triggered release.
mainanddependabot/*branches;pr.yamlrunning on every pull request is a deliberate family-uniformimprovement.
releasejob pullsmainat approval time, so a delayed approvalreleases main as it stands then, not the SHA this run tested — parity with
the old
release.sh(which also pulled;prerelease.shdid not, so theprerelease job has no pull).
push-back commit and nothing for the loop-guard to catch;
skip-ci-checkstays as family-uniform belt-and-braces.
asdf_install@v1is our own action (infrablocks/github-actions); we arehappy tracking its major version tag.
test,prerelease,release) check outwith
fetch-depth: 0— the Rakefile'slatest_tagneeds the full tag set.testjobs carry a guard step installing standalonedocker-composeonly 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.
build system (
./go/rake) and CI stays lean — it just triggers tasks andsupplies secrets/context.
Gemfile.lockcarries transitive major bumps — the unavoidable resolutionof the targeted
bundle lock --update, not scope creep.autocorrects existing code — required by the
library:checkverificationgate, not drive-by refactoring.
pipeline:prepare) authenticates with the operator's ambientghlogin (GITHUB_TOKENfallback) instead of a stored PAT — a deliberateparity deviation; the stored token in
config/secrets/github/config.yamlis 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
CircleCIdeploy key aredeferred to the end-of-migration sweep.