From eaa71a410c3a1078e95be42f0812baf017f9128a Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 15 Sep 2026 14:34:44 +0000 Subject: [PATCH 1/2] fix: prevent false major release signoff Amp-Thread-ID: https://ampcode.com/threads/T-01a0a579-a3bf-73f5-9ed7-2eb6bd92439c --- .changeset/forty-pens-throw.md | 2 + .github/scripts/major-release-signoff.test.sh | 62 +++++++++++++++ .github/workflows/major-release-signoff.yml | 79 +++++++++++++++---- package.json | 2 +- 4 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 .changeset/forty-pens-throw.md create mode 100644 .github/scripts/major-release-signoff.test.sh diff --git a/.changeset/forty-pens-throw.md b/.changeset/forty-pens-throw.md new file mode 100644 index 00000000..a845151c --- /dev/null +++ b/.changeset/forty-pens-throw.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.github/scripts/major-release-signoff.test.sh b/.github/scripts/major-release-signoff.test.sh new file mode 100644 index 00000000..6931ccba --- /dev/null +++ b/.github/scripts/major-release-signoff.test.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# Structural regression tests for the workflow's concurrency and classification boundaries. +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +WORKFLOW="$ROOT/.github/workflows/major-release-signoff.yml" +passes=0 +failures=0 + +pass() { + printf 'ok %s\n' "$1" + passes=$((passes + 1)) +} + +fail() { + printf 'FAIL %s\n %s\n' "$1" "$2" + failures=$((failures + 1)) +} + +assert_contains() { + local name="$1" needle="$2" + if grep -Fq -- "$needle" "$WORKFLOW"; then + pass "$name" + else + fail "$name" "workflow did not contain: $needle" + fi +} + +assert_not_contains() { + local name="$1" needle="$2" + if grep -Fq -- "$needle" "$WORKFLOW"; then + fail "$name" "workflow unexpectedly contained: $needle" + else + pass "$name" + fi +} + +if pnpm exec prettier --check "$WORKFLOW" >/dev/null; then + pass "workflow YAML parses and is formatted" +else + fail "workflow YAML parses and is formatted" "prettier rejected $WORKFLOW" +fi + +assert_contains "bot comments use an isolated pre-job concurrency key" \ + "github.event.comment.user.type == 'Bot' && format('bot-{0}', github.run_id) || 'evaluation'" +assert_contains "bot issue comments do not evaluate the PR" \ + "github.event.comment.user.type != 'Bot'" +assert_contains "unevaluable previews are not classified as major" \ + 'echo "is_major=0" >> "$GITHUB_OUTPUT"' +assert_not_contains "no decision branch maps uncertainty to major" \ + 'echo "is_major=1" >> "$GITHUB_OUTPUT"' +assert_contains "malformed preview decisions fail closed" \ + "blocked=release preview returned an invalid major decision" +assert_contains "unknown impact has its own failure status" \ + "Post failure status when release impact cannot be determined" +assert_contains "breaking-change status requires a trusted decision" \ + "steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major == '1'" +assert_contains "trusted non-major evaluations remove stale instructions" \ + "Mark PRs without a breaking change as success and remove stale instructions" + +printf '\n%d passed, %d failed\n' "$passes" "$failures" +[[ "$failures" -eq 0 ]] diff --git a/.github/workflows/major-release-signoff.yml b/.github/workflows/major-release-signoff.yml index 40b81d9d..08afe534 100644 --- a/.github/workflows/major-release-signoff.yml +++ b/.github/workflows/major-release-signoff.yml @@ -14,7 +14,10 @@ on: permissions: {} concurrency: - group: major-release-signoff-${{ github.event.pull_request.number || github.event.issue.number }} + # Workflow concurrency is resolved before job `if` conditions. Isolate bot comment + # runs so comments from changeset-bot or this workflow cannot cancel an active PR + # evaluation before their jobs are skipped. + group: major-release-signoff-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.event_name == 'issue_comment' && github.event.comment.user.type == 'Bot' && format('bot-{0}', github.run_id) || 'evaluation' }} cancel-in-progress: true env: @@ -23,8 +26,9 @@ env: jobs: context: # issue_comment fires for issues AND PRs across the repo. Skip non-PR - # issues so we don't waste a run when someone comments on a plain issue. - if: ${{ github.event_name != 'issue_comment' || github.event.issue.pull_request != null }} + # issues and bot comments. Bot runs have isolated concurrency keys above, + # because this condition is not evaluated until after concurrency handling. + if: ${{ github.event_name != 'issue_comment' || (github.event.issue.pull_request != null && github.event.comment.user.type != 'Bot') }} name: Resolve PR context runs-on: ubuntu-latest permissions: @@ -70,7 +74,7 @@ jobs: preview: # Skipped for forks. A fork's code is untrusted, so rather than run it we fail closed - # in the gate below and require a signoff regardless of what the PR declares. + # in the gate below and report that its release impact could not be determined. if: needs.context.outputs.is_fork != 'true' name: Compute release preview needs: context @@ -184,20 +188,30 @@ jobs: PREVIEW_RELEASE_TYPE: ${{ needs.preview.outputs.release_type }} run: | set -euo pipefail - # Fail closed. A fork, or a preview that did not succeed, means we could not - # establish that this PR is non-breaking, so require the signoff rather than - # trusting a value computed by code we did not run in a trusted context. + # Fail closed. A fork, or a preview that did not succeed, means release impact + # is unknown. Keep the gate red, but do not classify that uncertainty as a + # confirmed breaking change or offer signoff instructions for an unknown result. if [ "$IS_FORK" = "true" ] || [ "$PREVIEW_RESULT" != "success" ]; then if [ "$IS_FORK" = "true" ]; then echo "blocked=pull request is from a fork, so its release preview is not trusted" >> "$GITHUB_OUTPUT" else echo "blocked=release preview did not succeed ($PREVIEW_RESULT)" >> "$GITHUB_OUTPUT" fi - echo "is_major=1" >> "$GITHUB_OUTPUT" + echo "is_major=0" >> "$GITHUB_OUTPUT" echo "next=$PREVIEW_NEXT" >> "$GITHUB_OUTPUT" echo "release_type=$PREVIEW_RELEASE_TYPE" >> "$GITHUB_OUTPUT" exit 0 fi + case "$PREVIEW_IS_MAJOR" in + 0|1) ;; + *) + echo "blocked=release preview returned an invalid major decision" >> "$GITHUB_OUTPUT" + echo "is_major=0" >> "$GITHUB_OUTPUT" + echo "next=$PREVIEW_NEXT" >> "$GITHUB_OUTPUT" + echo "release_type=$PREVIEW_RELEASE_TYPE" >> "$GITHUB_OUTPUT" + exit 0 + ;; + esac # The version reaches a regex below, so anything that is not a plain semver # string is recorded as unevaluable rather than aborting: the job must still # reach the step that posts a red status. @@ -205,7 +219,7 @@ jobs: ! printf '%s' "$PREVIEW_NEXT" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$'; then echo "::error::Preview reported a major but next version '$PREVIEW_NEXT' is not a plain semver string." echo "blocked=next version '$PREVIEW_NEXT' is not a plain semver string" >> "$GITHUB_OUTPUT" - echo "is_major=1" >> "$GITHUB_OUTPUT" + echo "is_major=0" >> "$GITHUB_OUTPUT" echo "next=" >> "$GITHUB_OUTPUT" echo "release_type=$PREVIEW_RELEASE_TYPE" >> "$GITHUB_OUTPUT" exit 0 @@ -215,11 +229,12 @@ jobs: echo "next=$PREVIEW_NEXT" >> "$GITHUB_OUTPUT" echo "release_type=$PREVIEW_RELEASE_TYPE" >> "$GITHUB_OUTPUT" - - name: Mark PRs without a breaking change as success and exit - if: steps.decision.outputs.is_major != '1' + - name: Mark PRs without a breaking change as success and remove stale instructions + if: steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major != '1' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} HEAD_SHA: ${{ needs.context.outputs.head_sha }} + PR_NUMBER: ${{ needs.context.outputs.pr_number }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} RELEASE_TYPE: ${{ steps.decision.outputs.release_type }} run: | @@ -230,6 +245,32 @@ jobs: -f description="No breaking changeset added (${RELEASE_TYPE:-no bump}); signoff not required." \ -f target_url="$RUN_URL" + MARKER='' + gh api --paginate \ + "repos/${{ github.repository }}/issues/$PR_NUMBER/comments" \ + --jq ".[] | select(.body | contains(\"$MARKER\")) | .id" | + while read -r COMMENT_ID; do + [ -z "$COMMENT_ID" ] && continue + gh api -X DELETE "repos/${{ github.repository }}/issues/comments/$COMMENT_ID" + echo "Removed stale blocking comment ($COMMENT_ID)." + done + + - name: Post failure status when release impact cannot be determined + if: steps.decision.outputs.blocked != '' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + HEAD_SHA: ${{ needs.context.outputs.head_sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + BLOCKED: ${{ steps.decision.outputs.blocked }} + run: | + set -euo pipefail + gh api repos/${{ github.repository }}/statuses/$HEAD_SHA \ + -f state=failure \ + -f context="$STATUS_CONTEXT" \ + -f description="Unable to determine release impact; inspect the workflow run." \ + -f target_url="$RUN_URL" + echo "Unable to determine release impact: $BLOCKED" + - name: Search PR comments for valid approver signoff id: signoff if: steps.decision.outputs.is_major == '1' && steps.decision.outputs.blocked == '' @@ -323,7 +364,7 @@ jobs: fi - name: Post success status when signoff present - if: steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off == '1' + if: steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off == '1' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} HEAD_SHA: ${{ needs.context.outputs.head_sha }} @@ -339,7 +380,7 @@ jobs: -f target_url="$RUN_URL" - name: Upsert blocking comment when signoff missing - if: always() && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1' && github.event_name == 'pull_request' + if: always() && steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1' && github.event_name == 'pull_request' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ needs.context.outputs.pr_number }} @@ -390,7 +431,7 @@ jobs: fi - name: Post failure status when signoff missing - if: always() && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1' + if: always() && steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} HEAD_SHA: ${{ needs.context.outputs.head_sha }} @@ -405,7 +446,13 @@ jobs: -f target_url="$RUN_URL" - name: Fail the job so the gate is loud in the PR UI - if: always() && steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1' + if: always() && ((steps.decision.outputs.blocked != '') || (steps.decision.outputs.is_major == '1' && steps.signoff.outputs.signed_off != '1')) + env: + BLOCKED: ${{ steps.decision.outputs.blocked }} run: | - echo "Breaking change detected; awaiting write-access collaborator signoff." + if [ -n "$BLOCKED" ]; then + echo "Unable to determine release impact: $BLOCKED" + else + echo "Breaking change detected; awaiting write-access collaborator signoff." + fi exit 1 diff --git a/package.json b/package.json index 060c6fc7..56be7e7c 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "analyze:select": "node scripts/analyze-select.mjs", "generate:i18n": "pnpm --filter @youversion/platform-react-ui generate:i18n", "check:i18n": "node scripts/check-i18n-parity.mjs", - "test:ci-scripts": "bash .github/scripts/check-locale-ownership.test.sh", + "test:ci-scripts": "bash .github/scripts/check-locale-ownership.test.sh && bash .github/scripts/major-release-signoff.test.sh", "storybook": "pnpm --filter @youversion/platform-react-ui storybook" }, "devDependencies": { From d03268c5bb42bc8e1f12e5edf04de9140f21fca2 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 15 Sep 2026 16:38:24 +0000 Subject: [PATCH 2/2] fix: clean stale signoff comments safely Amp-Thread-ID: https://ampcode.com/threads/T-01a0a579-a3bf-73f5-9ed7-2eb6bd92439c --- .github/scripts/major-release-signoff.test.sh | 20 ++++++++++-- .github/workflows/major-release-signoff.yml | 32 ++++++++++++------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/.github/scripts/major-release-signoff.test.sh b/.github/scripts/major-release-signoff.test.sh index 6931ccba..830af18d 100644 --- a/.github/scripts/major-release-signoff.test.sh +++ b/.github/scripts/major-release-signoff.test.sh @@ -35,6 +35,17 @@ assert_not_contains() { fi } +assert_before() { + local name="$1" first="$2" second="$3" first_line second_line + first_line="$(grep -nF -- "$first" "$WORKFLOW" | head -1 | cut -d: -f1)" + second_line="$(grep -nF -- "$second" "$WORKFLOW" | head -1 | cut -d: -f1)" + if [[ -n "$first_line" && -n "$second_line" && "$first_line" -lt "$second_line" ]]; then + pass "$name" + else + fail "$name" "expected '$first' before '$second'" + fi +} + if pnpm exec prettier --check "$WORKFLOW" >/dev/null; then pass "workflow YAML parses and is formatted" else @@ -55,8 +66,13 @@ assert_contains "unknown impact has its own failure status" \ "Post failure status when release impact cannot be determined" assert_contains "breaking-change status requires a trusted decision" \ "steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major == '1'" -assert_contains "trusted non-major evaluations remove stale instructions" \ - "Mark PRs without a breaking change as success and remove stale instructions" +assert_contains "unknown and non-major evaluations remove stale instructions" \ + "steps.decision.outputs.blocked != '' || steps.decision.outputs.is_major != '1'" +assert_contains "cleanup only deletes workflow-owned comments" \ + '.user.login == \"github-actions[bot]\"' +assert_before "cleanup happens before a new status is published" \ + "Remove stale workflow-owned signoff instructions" \ + "Mark PRs without a breaking change as success" printf '\n%d passed, %d failed\n' "$passes" "$failures" [[ "$failures" -eq 0 ]] diff --git a/.github/workflows/major-release-signoff.yml b/.github/workflows/major-release-signoff.yml index 08afe534..8fa62dab 100644 --- a/.github/workflows/major-release-signoff.yml +++ b/.github/workflows/major-release-signoff.yml @@ -229,32 +229,40 @@ jobs: echo "next=$PREVIEW_NEXT" >> "$GITHUB_OUTPUT" echo "release_type=$PREVIEW_RELEASE_TYPE" >> "$GITHUB_OUTPUT" - - name: Mark PRs without a breaking change as success and remove stale instructions - if: steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major != '1' + - name: Remove stale workflow-owned signoff instructions + # Unknown impact must not leave a comment claiming a confirmed breaking change. + # Trusted non-major results also retire instructions from an earlier evaluation. + if: steps.decision.outputs.blocked != '' || steps.decision.outputs.is_major != '1' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - HEAD_SHA: ${{ needs.context.outputs.head_sha }} PR_NUMBER: ${{ needs.context.outputs.pr_number }} - RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - RELEASE_TYPE: ${{ steps.decision.outputs.release_type }} run: | set -euo pipefail - gh api repos/${{ github.repository }}/statuses/$HEAD_SHA \ - -f state=success \ - -f context="$STATUS_CONTEXT" \ - -f description="No breaking changeset added (${RELEASE_TYPE:-no bump}); signoff not required." \ - -f target_url="$RUN_URL" - MARKER='' gh api --paginate \ "repos/${{ github.repository }}/issues/$PR_NUMBER/comments" \ - --jq ".[] | select(.body | contains(\"$MARKER\")) | .id" | + --jq ".[] | select(.user.login == \"github-actions[bot]\" and (.body | contains(\"$MARKER\"))) | .id" | while read -r COMMENT_ID; do [ -z "$COMMENT_ID" ] && continue gh api -X DELETE "repos/${{ github.repository }}/issues/comments/$COMMENT_ID" echo "Removed stale blocking comment ($COMMENT_ID)." done + - name: Mark PRs without a breaking change as success + if: steps.decision.outputs.blocked == '' && steps.decision.outputs.is_major != '1' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + HEAD_SHA: ${{ needs.context.outputs.head_sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + RELEASE_TYPE: ${{ steps.decision.outputs.release_type }} + run: | + set -euo pipefail + gh api repos/${{ github.repository }}/statuses/$HEAD_SHA \ + -f state=success \ + -f context="$STATUS_CONTEXT" \ + -f description="No breaking changeset added (${RELEASE_TYPE:-no bump}); signoff not required." \ + -f target_url="$RUN_URL" + - name: Post failure status when release impact cannot be determined if: steps.decision.outputs.blocked != '' env: