Skip to content

ci: open the SPM checksum pin as a pull request - #8

Merged
erubboli merged 53 commits into
mainfrom
fix/release-pr-flow
Nov 15, 2026
Merged

erubboli merged 53 commits into
mainfrom
fix/release-pr-flow

Conversation

@nullPointerEnjoyer

@nullPointerEnjoyer nullPointerEnjoyer commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Branch protection rejects the release workflow's direct push to main (GH013, observed on the v0.1.0 release run 43435024739: both build jobs succeeded, the checksum-pin push was rejected, and the release had to be completed manually). The checksum-pin step now creates a pin-spm-checksum-v branch and opens a PR that a maintainer merges to activate SPM resolution.

Hardening from review:

  • `pull-requests: write` on the release job (gh pr calls would otherwise 403)
  • The logic lives in `scripts/release/open-spm-pr.sh` (shellcheck-clean), following the repo's scripts-from-workflows convention
  • Idempotent re-runs: guarded commit (already-pinned exits 0), force-push of the disposable bot branch after the existence decision, open-PR guard filtered to this repository's owner (a fork PR with a colliding head-branch name cannot suppress the bot PR)
  • Tag-ancestry check fails loudly (with %-escaped workflow annotation) when the tag is not on the default branch
  • The PR body embeds the released checksum for reviewer verification
  • New `lint.yml` runs shellcheck + actionlint on workflow/script changes (ci.yml ignores .github paths)

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 12 issue(s) in this PR.

  • ✅ Successfully posted inline: 3 comment(s)
  • 📝 In summary (no line info): 1 comment(s)
  • 📋 Routed to summary by policy: 5 comment(s)
  • ⏭️ Skipped (overlap with history): 3 comment(s)

⚠️ 1 warning(s) occurred during review.


maintainability · low

📄 scripts/release/open-spm-pr.sh

⚠️ GitHub could not post this as an inline comment: No line information provided

This file ends without a trailing newline, which produces noisy diffs on every future edit and can trip shell lint checks (the new lint.yml shellcheck job included). Add a final newline.


maintainability · low

📄 .gitignore (L25-L26)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

Removing the .orc/ ignore entry means any local .orc/ development-scripts directory will no longer be excluded and could be accidentally committed. If .orc/ is still used locally, consider keeping it ignored (e.g., under the new 'Local generated output' section); if it's genuinely obsolete, the removal is fine.


maintainability · low

📄 scripts/release/open-spm-pr.sh (L282-L282)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

Force-pushing a shared (namespace-collidable) branch without --force-with-lease is safe under the release.yml concurrency group, but a manual/CLI invocation of this script bypasses that group: two concurrent runs on the same version can interleave push/PR-create and the last push silently wins over the other's commit while both PRs reference it. Consider using git push --force-with-lease=refs/heads/${BRANCH} (expecting a non-existent ref) so a concurrent creation is detected, or rejecting manual runs (e.g. require a workflow-provided env marker).

💡 Suggested Change

Before:

git push --force origin "${BRANCH}"

After:

git push --force-with-lease="refs/heads/${BRANCH}:" origin "${BRANCH}"

other · low

📄 .github/workflows/ci.yml (L8-L12)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

Removing paths-ignore: [".github/**"] means workflow-file-only edits now run the full fmt/clippy/test/doc/npm/license matrix. Since lint.yml already covers .github/** and scripts/** with the fast actionlint/shellcheck/test jobs, consider a cheaper filter (e.g. restore paths-ignore for the heavyweight jobs, or scope the crate jobs with paths filters) to avoid multi-minute matrix runs for CI-config-only changes.


other · low

📄 .github/workflows/ci.yml (L18-L19)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

This workflow (and lint.yml) has no concurrency group with cancel-in-progress, so rapid pushes to the same branch queue redundant full-matrix runs. Adding e.g. concurrency: { group: ci-${{ github.ref }}, cancel-in-progress: true } would cancel superseded runs. (release.yml correctly serializes with cancel-in-progress: false; the CI-side workflows want the opposite.)

💡 Suggested Change

Before:

env:
  CARGO_TERM_COLOR: always

After:

concurrency:
  group: ci-${{ github.ref }}
  cancel-in-progress: true

env:
  CARGO_TERM_COLOR: always

other · low

📄 .github/workflows/lint.yml (L35-L36)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

The actionlint download has no --retry/--connect-timeout; a transient GitHub network hiccup fails the whole lint workflow (which also gates release-script unit tests). Consider curl --retry 3 --connect-timeout 10 ... for resilience. Note the hardcoded SHA-256 pin itself is good practice.

💡 Suggested Change

Before:

          curl --proto '=https' --tlsv1.2 -fsSL -o actionlint.tar.gz \
            https://github.com/rhysd/actionlint/releases/download/v1.7.7/actionlint_1.7.7_linux_amd64.tar.gz

After:

          curl --proto '=https' --tlsv1.2 --retry 3 --connect-timeout 10 -fsSL -o actionlint.tar.gz \
            https://github.com/rhysd/actionlint/releases/download/v1.7.7/actionlint_1.7.7_linux_amd64.tar.gz

⚠️ Warnings:

  • .github/workflows/ci.yml,.github/workflows/lint.yml,.github/workflows/release.yml (review_limit_reached): reached the aggregate token budget before finishing; the grace round completed the task

Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round-4 review dispositions:

  • Fixed (critical): the checksum-manifest step is rebuilt from scratch — no filtering grep -v (which would exit 1 on the zip-only artifact manifest and abort the release), and the Swift wrapper is hashed by its bare asset name so sha256sum -c matches the downloaded release files.
  • Rejected (empty-diff early exit "does not ensure a PR"): unreachable via the workflow. Every run checks out the tag fresh, and the Pin step re-applies the sed in the same run before the script executes, so the script always sees a non-empty diff. An empty diff can only occur when the tag commit itself is already pinned — and the ancestry check guarantees the tag is on the default branch, so the pin is already merged; exiting 0 is correct. Restructuring to "ensure a PR" on that path would attempt gh pr create with no commits and fail.
  • Fixed: --paginate replaces --limit 200 (no visibility window); concurrency group release-<tag> added (no interleaved force-pushes/release TOCTOU); tag-regex validation gates added to the AAR/XCFramework build jobs (values via env indirection); TAG/XCSUM regexes mirrored in the script for manual invocations; persist-credentials: false on the lint checkout; tests extended to 22 assertions (mirror drift, invalid default branch, invalid checksum, explicit --base/--head on create).
  • Accepted risk (documented): --force (not --force-with-lease) on the reserved pin-spm-checksum-* namespace — the local clone never fetches the bot branch, so lease semantics have no baseline; the namespace reservation is documented in the script header, and the pin PR diff is what a maintainer reviews before merging.

Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round-8 review dispositions:

  • Fixed (critical/major): the rewind guard now semver-compares (sort -V) — it fails closed only when the default branch pins a NEWER-or-equal release; forward releases over older pins proceed, and an unparseable origin pin tag fails closed with esc() applied.
  • Fixed (bug): short-circuit ordering — the already-merged check now runs first; an empty staged diff with a differing default branch (pin reverted on main) fails closed instead of falsely reporting success.
  • Fixed (bug): lint.yml trigger paths widened to scripts/** to match the shellcheck scope.
  • Fixed (bug): gh release create is now re-run safe — an existing release is reused with gh release upload --clobber instead of hard-failing.
  • Fixed: repo-root CWD guard; merge-base exit-code disambiguation (git errors ≠ non-ancestor); $declared control-stripped before compare (CRLF-safe).
  • Fixed (docs): dropped the incorrect rust-std claim for x86_64-apple-ios-sim (the omission is for Xcode runtime reasons only).
  • Tests: 49 assertions — added forward-release-over-older-pin, same-version-conflict, reverted-main, reserved-namespace, shallow/plain fetch, and negative-mutation coverage.
  • Accepted: the pin PR carries the tag's whole Package.swift (pin-fields-only gating noted; the maintainer diff review is the control).

Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread .github/workflows/lint.yml Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 41 dispositions: security review returned [] (eleventh consecutive clean round). Three OCR items fixed and pushed (suite at 152 assertions, shellcheck/actionlint clean):

  • bug: origin-pin tag extraction in open-spm-pr.sh used a weaker `v[0-9]...` pattern than the gate — a hand-mangled base tag like `wallet-bindings-vv1.2.3` parsed as 1.2.3 at release time while the merge gate refused it (divergent verdicts, green release with a dead PR). Extraction now uses the whole tag token plus the full version regex, identical to the gate.
  • bug: the release's base-pin counts guard was anchored-only, tolerating extra pin-bearing lines (broken anchors) the gate refuses. It now also counts pin-bearing occurrences and requires exactly one canonical assignment — same invariant as the gate.
  • nit: duplicate case number resolved; 77 assertion cases renumbered sequentially. New fixtures 59d/59e pin both divergent-verdict shapes.

Comment thread .github/workflows/ci.yml Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 42 dispositions: security review returned [] (twelfth consecutive clean round). Three OCR items fixed and pushed (suite at 154 assertions, shellcheck/actionlint clean):

  • bug: release-time base counts used prefix-only anchors, so trailing garbage on the canonical line itself (...v1.2.3"x) passed at release time while the merge gate refused it. The canonical count now uses the gate's whole-line anchor — trailing garbage refuses identically at both times. Fixture 59f pins it.
  • test-gap: the release-side trailing-garbage fixture added (was only covered merge-side).
  • nit: the second_pr_list_empty stub knob now keys on the --head-list counter (post-push re-lookup only), matching the relookup_fails pattern, and the pinned-lines log echo strips control bytes consistent with esc()/safe_tag.

@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 43 dispositions: security review returned [] (thirteenth consecutive clean round). Two OCR items fixed and pushed (suite at 154 assertions, shellcheck/actionlint clean):

  • test-gap: guard cases 61-63 were passing their deactivated-pin fixtures into package_swift (a file the guard never reads), so all three ran the identical older-marker allow under three names. The literals now go into origin_swift: 61 (newer marker) genuinely allows with the upstream-refusal note; 62 (equal marker) now asserts the guard's actual same-tag repair refusal ("do not match the pin") instead of a contradicting allow; 63 covers the genuine older path.
  • nit: the dead pr_list_calls stub instrumentation (write + both rm calls) from the pre-round-42 detection was removed.

Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/pin-order-gate.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 44 dispositions: security review returned [] (fourteenth consecutive clean round). Seven OCR nits fixed and pushed (suite at 157 assertions, shellcheck/actionlint clean):

  • case 42's comment corrected (the fixture exercises a terminated block, not an unterminated one);
  • the empty-value checksum shape now gets a precise diagnostic ("checksum line with an empty value") instead of the misleading "without a checksum line", with fixture 59g;
  • the guard's same-tag refusal prints a <non-hex/mangled> placeholder instead of empty parentheses when the base pin checksum is malformed;
  • semver.sh documents the empty-prerelease-identifier admission alongside the leading-zero deviation;
  • the release concurrency comment now reflects reality (the merge-time gate refuses rewinds; serialization prevents interleaved supersede/publish races);
  • lint.yml documents its hardcoded-main trigger assumption like ci.yml;
  • new gate case: a failed PR-head fetch refuses before any ordering decision.

Comment thread scripts/release/pin-order-gate.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 45 dispositions: security review returned [] (fifteenth consecutive clean round). Two OCR items fixed and pushed (suite at 158 assertions, shellcheck clean):

  • maintainability: the guard's checksum extraction (and open-spm-pr.sh's base-side raw extraction) were unanchored at the end — trailing garbage on a base checksum line still yielded the hex, so a same-tag refresh could succeed against a corrupted pin and the three scripts used three different anchors for the same grammar. Both now use the $-anchored 64-hex shape, matching check-pin-order.sh; guard fixture 63b pins the refusal.
  • test-gap: the documented empty-prerelease-identifier deviation is unit-pinned (a..b vs a.b ordering both directions, plus an empty-vs-nonempty tail).

Comment thread scripts/release/check-pin-order.sh Resolved
Comment thread scripts/release/check-pin-order.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 46 dispositions: security review returned [] (sixteenth consecutive clean round). Two OCR items fixed and pushed (suite at 158 assertions, shellcheck clean):

  • maintainability (ocr) + nitpick (sec), same root: the $-anchoring round left the counts guard classifying every non-well-formed checksum as "empty value" (misleading for mangled values like deadbeef), and made the dedicated mangled-checksum elif unreachable dead code. Restructured: the counts guard now verifies exactly one canonical tag, exactly one $-anchored 64-hex checksum, exactly one checksum-shaped line, and no extra pin-bearing lines, with a triaged refusal message (missing line / mangled-or-empty value / count mismatch). The dead elif and unreachable inner branch are removed; origin_pin_sum derives directly from the guaranteed-well-formed line. Fixtures 59g (empty value) and 60 (missing line) assert the new messages.

Comment thread scripts/release/check-pin-order.sh Resolved
Comment thread scripts/release/check-pin-order.sh Resolved
Comment thread scripts/release/pin-order-gate.sh Resolved
Comment thread scripts/release/pin-order-gate.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 47 dispositions: security review returned [] (seventeenth consecutive clean round). Two OCR items fixed and pushed (suite at 158 assertions, shellcheck/actionlint clean):

  • maintainability: the reserved-namespace + charset validation of branch inputs moved into one canonical pinlib.sh helper (validate_branch_input), now used by open-spm-pr.sh, guard-release-refresh.sh, check-pin-order.sh and pin-order-gate.sh — one diagnostic granularity for all four (split format vs namespace messages).
  • maintainability: test 67's stale comment (left by the round-46 refactor, describing a removed defense-in-depth branch) corrected to match the triaged counts guard.

Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/release/pin-order-gate.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
Comment thread scripts/release/test-open-spm-pr.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 48 dispositions: security review returned [] (eighteenth consecutive clean round; the reviewer also walked the full current state of all in-scope files given its no-shell fallback). Three OCR items fixed and pushed (suite at 159 assertions, shellcheck clean):

  • maintainability: the inline duplicate reserved-namespace check in open-spm-pr.sh (dead after the round-47 canonicalization) removed — validate_branch_input is the single gate.
  • test-gap: the merge gate's base-version validation now has its own case (origin wallet-bindings-v + valid checksum → 'unparseable current pin version'), matching the guard and release-side counterparts.
  • nit: case 58 and the check-pin-order comment no longer claim the fixture byte-mirrors the production URL assignment (the real file splits it across two lines).

Comment thread .github/workflows/lint.yml Resolved
Comment thread .github/workflows/release.yml Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Round 49 dispositions: OCR returned [] for the first time; security returned one major, fixed and pushed (suite at 159 assertions, shellcheck/actionlint clean):

  • major: the diff-applicability check used printf | grep -q — grep -q exits at the first match without draining the pipe, so on a changed-file list larger than one pipe buffer (trivially arranged by touching thousands of files) the producer dies of EPIPE, pipefail makes the pipeline non-zero, and the leading inversion turned that into "Package.swift untouched; nothing to check" — skipping both the SHA256SUMS binding and the ordering enforcement for a diff that DOES touch Package.swift. Replaced with a path-limited git diff --quiet ... -- Package.swift: no pipe, exit codes triaged (0 = untouched skip, 1 = enforce, >1 = fail closed).

Comment thread .github/workflows/ci.yml Resolved
Comment thread scripts/release/semver.sh Resolved
@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Convergence reached — round 50 returned [] from BOTH reviewers (OCR and security), on the full origin/main...HEAD diff.

Final state of this branch:

  • 159 assertions in the stub harness, all green; shellcheck (all release scripts) and actionlint (SHA-pinned, checksum-verified) clean
  • Release/checksum-pin flow: tag-triggered release → serialized via a concurrency group → guard refuses clobbering a merged pin → AAR/XCFramework artifacts + SHA256SUMS → automated pin PR (idempotent, superseding older pins) → merge-time gate (diff-driven applicability, SHA256SUMS checksum binding, whole-line mangle refusal, ordering enforcement) → post-push re-lookup guards the reuse path
  • Supply-chain hardening: every action SHA-pinned, every linter checksum-verified, no persisted credentials, gate code always from a trusted checkout, annotation-safe log handling, validated inputs before every sink
  • Security reviewer: [] in 15 of the last 16 rounds (the single exception was a real SIGPIPE bypass, found and fixed same-round)
  • 30 review-fix rounds total; every severity class fixed and re-verified locally before push

Ready to merge.

@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator Author

Self-bootstrap fix (found by watching this PR's own CI): the pin-order job checks out the base branch so PR content can never alter enforcement — but scripts/release/pin-order-gate.sh is a file introduced by this very PR, so the base checkout couldn't execute it (exit 127) and the "Pin ordering" check went red. This only ever fires for the gate's own introduction PR; removing the script from main post-merge would itself be a reviewed change. The run block now treats a missing gate on the base as the documented bootstrap case: warning annotation + green skip; enforcement begins the moment this PR merges.

Comment thread .github/workflows/ci.yml Resolved
Comment thread scripts/release/open-spm-pr.sh Resolved
Comment thread scripts/release/pin-order-gate.sh Resolved
@erubboli
erubboli merged commit 409890c into main Nov 15, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants