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

Verdict: REQUEST_CHANGES

A careful, well-hardened CircleCI→GitHub Actions cutover that faithfully follows the Variant A family plan: least-privilege permissions, env-indirection for untrusted inputs, git-crypt ciphertext guards, a release environment gate, and thorough "why" comments. One in-scope defect blocks approval: both workflows invoke ./go spec, a rake task that does not exist in this repo (the only test task is test:unit), so the required test gate fails on every push and PR and the whole pipeline stalls. Everything else the lenses raised is a concern against a decision the plan/PR documents as deliberate — kept below for a human to revisit at the plan level; none of it blocks a plan-conformant diff.

Cross-Cutting Themes

• Non-existent spec test task (correctness, standards) — both main.yaml and pr.yaml run ./go spec; the Rakefile defines only test:unit. The plan (§2.3, §5.2) specifies test:unit, and sibling rake_slack uses it. ./go spec was a pre-existing broken invocation in the old test.sh (the spec task was removed from the Rakefile long ago), but the migration should have adopted the plan-mandated test:unit, which exists and passes. As written the test job errors with Don't know how to build task 'spec'.

Strengths

• ✅ Least-privilege permissions: contents: read with narrow per-job escalation.
• ✅ Untrusted inputs passed via env: as $VARS, never ${{ }}-interpolated into run:.
• ✅ Secret-bearing PR prerelease job guarded to same-repo, non-Dependabot PRs.
• ✅ Dependabot merge uses --match-head-commit "$HEAD_SHA" and immutable user.login.
• ✅ read_encryption_passphrase refuses git-crypt ciphertext; resolve_github_token fails fast.
• ✅ prerelease:publish restores version.rb and removes the built gem in an ensure block.

General Findings

• 🔵 code-quality (suggestion, in-scope): token/passphrase logic was extracted into top-level resolve_github_token / read_encryption_passphrase methods; the plan (§4.4 step 3) shows this inline and says "Do not restructure anything else". Functionally identical, cleaner, likely helps Metrics/BlockLength — a benign deviation, noted for transparency.

Plan Concerns (documented-deliberate — do not block; for plan revisit)

• 🟡 security: the same-repo PR prerelease job hands the RubyGems token and git-crypt secrets to PR-branch code; a write-access contributor could exfiltrate them with no maintainer approval. This is the D8 pre-merge publish, documented and guarded to same-repo human PRs. (pr.yaml:85)
• 🔵 security: Dependabot auto-merge accepts any update type — D3, documented parity.
• 🔵 security: provisioning uses the operator's ambient gh/GITHUB_TOKEN, not a stored PAT — D7, documented parity.
• 🔵 safety: ./go release publishes to RubyGems before git push — a pre-existing hazard inside untouched ./go release logic (§1, D5). (main.yaml:124)
• 🔵 standards: concurrency.queue: max — the plan §4.2 specifies it and cites the 2026-05-07 GA changelog; authoritative here. (main.yaml:54)
• 🔵 standards: notify idiom (!cancelled() + continue-on-error) differs from rake_slack's always() — plan §3 says the §4 YAML is authoritative.
• 🔵 standards: main.yaml omits the sibling's skip-ci-check job — D4 states no loop-guard job is needed.
• 🔵 code-quality: gem build duplicated across library:build / prerelease:publish; repeated arg list; magic Slack channel IDs — all plan-prescribed verbatim (§4.4).

Non-issue (verified)

• prerelease:publish's ensure restore not being SIGKILL-safe self-nullifies: the runner checkout is ephemeral, so a mutated version.rb never persists.

Review generated by /accelerator:review-pr

Code Review: #157 — Migrate pipeline from CircleCI to GitHub Actions

Verdict: REQUEST_CHANGES

A clean, faithful Variant A cutover — task wiring is sound, secrets handling is well-hardened (PR-controlled values routed through env, secret-bearing prerelease job guarded to same-repo human PRs, immutable user.login dependabot guard, least-privilege permissions:), and the decommission is complete. One in-scope critical defect blocks approval: the release job runs ./go documentation:update, but no such task exists in the Rakefile and the deleted release.sh never invoked it — plan §4.2 says this step must be omitted when release.sh does not run it. Several lens findings target the plan's own authoritative YAML/code (queue: max, gpg path, verbatim comments) and are recorded as plan concerns, not defects.

Cross-Cutting Themes

• queue: max concurrency key (correctness, safety) — both lenses believe it is an unsupported GitHub Actions concurrency key. Plan §4.2 specifies it verbatim, citing a GA feature dated 2026-05-07 → plan concern, not a defect. Worth a human confirming the feature shipped as cited.

Strengths

• ✅ Every ./go task referenced resolves to a real task (except documentation:update, below); slack:notify[outcome,type] arity matches; gpg path matches rake_git_crypt's default.
• ✅ PR title/number/SHA/URL routed through env vars, not ${{ }} shell interpolation; only ${{ job.status }} is inline (trusted enum).
• ✅ Prerelease (secret-bearing) job guarded to same-repo, non-dependabot PRs; dependabot merge guard uses immutable pull_request.user.login.
• ✅ Least-privilege permissions:; environment: release gate present; all jobs have timeout-minutes; --match-head-commit guards the merge.
• ✅ prerelease:publish restores version.rb and removes the built gem in an ensure block; requires/Gemfile stay alphabetical; helpers fail fast.

General Findings

• 🔴 Correctness (plan-conformance): release job Update documentation step references a non-existent task; release.sh never ran it. Breaks every release. See inline.
• 🔵 Code Quality: gem name / gem build command duplicated across library:build and prerelease:publish (minor).
• 🔵 Standards: helper defs placed mid-file between namespaces (suggestion; a clean divergence from the plan's inline code).
• 🔵 Code Quality: prerelease:publish validates arg presence but not numeric shape (suggestion).
• 🔵 Plan-conformance (minor): Gemfile.lock PLATFORMS still lists operator-local darwin platforms (§4.5 asks for none) — pre-existing and unchanged by this PR.

Plan Concerns (documented-deliberate; do not block)

• 🔵 queue: max on prerelease + release (correctness ×2, safety ×1) — plan §4.2 verbatim, cites GA 2026-05-07.
• 🔵 Shared main concurrency group across gated release + ungated prerelease (safety) — the plan's explicit design.
• 🔵 Compound prerelease version 0.13.0.pre.2.pr<N>... (correctness) — plan §4.4 step 8 documents this shape as valid/accepted.
• 🔵 gpg key at .github/gpg.private.enc root (standards) — plan §4.3 mandates this exact path.
• 🔵 pr.yaml comments exceed 80 cols (standards) — verbatim from the plan's YAML; YAML is not rubocop-linted.
• 🔵 String#sub replacement-string backslash handling (code-quality) — plan's verbatim code; inputs numeric, safe.

Review generated by /accelerator:review-pr