fix(ci): prevent transitive skip propagation in release pipeline (#266)

polaz · web-flow · commit f809a7ae7296 · 2026-02-02T13:17:19.000+02:00
* fix(ci): prevent transitive skip propagation in release pipeline - Make gate job always run instead of skipping on non-PR events - Add early exit for non-PR events with allow_heavy=true output - Simplify docker-build condition to check gate output only - Revert !cancelled() approach from #264 (insufficient fix) Fixes #265 * fix(ci): move gate check into docker-build, remove standalone gate job - Delete separate gate job that broke needs chain on push to main - Add gate step as first step of docker-build (runs on pull requests only) - docker-build now depends only on quality-checks - Simplify summary job status reporting Fixes #265 * fix(ci): fail-closed on incomplete pagination, rename quality-checks job - Fail with exit 1 on inconsistent pagination (empty page or missing cursor with hasNextPage=true) instead of silently breaking the loop - Rename quality-checks display name to "Quality Checks" to avoid collision with legacy "Lint, Test & Build" expected-status job * fix(ci): fix off-by-one in pagination guard, remove unused output - Only fail on MAX_PAGES when hasNextPage is still true (avoids false positive when scan completes exactly on the last page) - Remove unused image-built output from docker-build job - Add comment explaining intentional fail-on-unresolved design * fix(ci): add timeout-minutes to docker-build job - Set 30-minute timeout for docker-build (multi-arch build ~8min + margin for cache misses and gate check) * fix(ci): document always() behavior on summary job - Add comment explaining why needs lists all jobs (output access) and why always() guarantees execution despite skipped dependencies
diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml
@@ -31,7 +31,7 @@ env:
 jobs:
   # Job 1: Quality checks (lint, test, build)
   quality-checks:
-    name: Lint, Test & Build
+    name: Quality Checks
     runs-on: ubuntu-latest
 
     steps:
@@ -94,43 +94,55 @@ jobs:
           path: dist/
           retention-days: 1
 
-  review-thread-gate:
-    name: Review Threads Gate
+  # Legacy required status check - reports quality-checks result for branch protection
+  # Added to support migration from old "Lint, Test & Build" ruleset requirement
+  expected-status:
+    name: Lint, Test & Build
+    runs-on: ubuntu-latest
+    needs: [quality-checks]
+    if: always()
+
+    steps:
+      - name: Report status
+        run: |
+          if [ "${{ needs.quality-checks.result }}" = "success" ]; then
+            echo "Lint/Test/Build passed."
+            exit 0
+          fi
+          echo "Lint/Test/Build failed."
+          exit 1
+
+  # Job 2: Build and test Docker image
+  docker-build:
+    name: Build Docker Image
     runs-on: ubuntu-latest
-    timeout-minutes: 5
-    # Only relevant for PRs; non-PR events skip the gate entirely.
-    if: github.event_name == 'pull_request'
+    timeout-minutes: 30
+    needs: [quality-checks]
 
     permissions:
-      pull-requests: read
       contents: read
-
-    outputs:
-      allow_heavy: ${{ steps.gate.outputs.allow_heavy }}
+      pull-requests: read  # For review thread check on PRs
 
     steps:
-      - name: Check unresolved review threads
-        id: gate
+      - name: Check for unresolved review threads
+        if: github.event_name == 'pull_request'
         env:
           GH_TOKEN: ${{ github.token }}
           PR_NUMBER: ${{ github.event.pull_request.number }}
           REPO: ${{ github.repository }}
         run: |
           if ! command -v gh >/dev/null 2>&1; then
-            echo "gh CLI not available; allowing heavy jobs to proceed (fail-open)." >> "$GITHUB_STEP_SUMMARY"
-            echo "allow_heavy=true" >> "$GITHUB_OUTPUT"
+            echo "gh CLI not available; skipping thread check (fail-open)." >> "$GITHUB_STEP_SUMMARY"
             exit 0
           fi
           if [ -z "$PR_NUMBER" ]; then
-            echo "allow_heavy=true" >> "$GITHUB_OUTPUT"
-            echo "PR_NUMBER is empty; allowing heavy jobs to proceed (fail-open)." >> "$GITHUB_STEP_SUMMARY"
+            echo "PR_NUMBER is empty; skipping thread check (fail-open)." >> "$GITHUB_STEP_SUMMARY"
             exit 0
           fi
           OWNER="${REPO%/*}"
           NAME="${REPO#*/}"
           UNRESOLVED=0
           AFTER=""
-          # Hard cap to avoid unbounded pagination; 100 pages * 100 threads = 10,000 threads max.
           MAX_PAGES=100
           PAGE_COUNT=0
           while [ "$PAGE_COUNT" -lt "$MAX_PAGES" ]; do
@@ -151,9 +163,7 @@ jobs:
                 -f owner="$OWNER" \
                 -f name="$NAME" \
                 -F number="$PR_NUMBER") || {
-                  # Fail-open on API errors to avoid blocking contributor workflows.
-                  echo "GitHub GraphQL API query for review threads failed; allowing heavy jobs to proceed (fail-open)." >> "$GITHUB_STEP_SUMMARY"
-                  echo "allow_heavy=true" >> "$GITHUB_OUTPUT"
+                  echo "GraphQL query failed; skipping thread check (fail-open)." >> "$GITHUB_STEP_SUMMARY"
                   exit 0
                 }
             else
@@ -173,103 +183,41 @@ jobs:
                 -f name="$NAME" \
                 -F number="$PR_NUMBER" \
                 -f after="$AFTER") || {
-                  # Fail-open on API errors to avoid blocking contributor workflows.
-                  echo "GitHub GraphQL API query for review threads failed; allowing heavy jobs to proceed (fail-open)." >> "$GITHUB_STEP_SUMMARY"
-                  echo "allow_heavy=true" >> "$GITHUB_OUTPUT"
+                  echo "GraphQL query failed; skipping thread check (fail-open)." >> "$GITHUB_STEP_SUMMARY"
                   exit 0
                 }
             fi
-            # Count only active, unresolved, non-outdated review threads.
-            # GitHub marks a thread as "outdated" when the underlying code has changed
-            # (e.g., after new commits). For this gate, we deliberately ignore outdated
-            # threads so that only current, actionable discussions block heavy jobs.
-            # This keeps the gate aligned to actionable feedback and avoids blocking
-            # heavy jobs on stale discussions that no longer apply to the diff.
-            #
-            # If you want unresolved outdated threads to also block heavy jobs, remove
-            # "and .isOutdated == false" from the jq filter below.
+            # Count only active, unresolved, non-outdated threads.
+            # Outdated threads (code changed since comment) are excluded.
             PAGE_UNRESOLVED=$(echo "$RESP" | jq '[.data.repository.pullRequest.reviewThreads.nodes // [] | .[] | select(.isResolved == false and .isOutdated == false)] | length')
             PAGE_NODE_COUNT=$(echo "$RESP" | jq '(.data.repository.pullRequest.reviewThreads.nodes // []) | length')
-            if ! [[ "$PAGE_UNRESOLVED" =~ ^[0-9]+$ ]]; then
-              PAGE_UNRESOLVED=0
-            fi
-            if ! [[ "$PAGE_NODE_COUNT" =~ ^[0-9]+$ ]]; then
-              PAGE_NODE_COUNT=0
-            fi
+            if ! [[ "$PAGE_UNRESOLVED" =~ ^[0-9]+$ ]]; then PAGE_UNRESOLVED=0; fi
+            if ! [[ "$PAGE_NODE_COUNT" =~ ^[0-9]+$ ]]; then PAGE_NODE_COUNT=0; fi
             UNRESOLVED=$((UNRESOLVED + PAGE_UNRESOLVED))
-            # Only fetch the fields we need to evaluate the gate, to reduce payload size.
             HAS_NEXT=$(echo "$RESP" | jq -r '.data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage')
-            if [ "$HAS_NEXT" != "true" ]; then
-              break
-            fi
+            if [ "$HAS_NEXT" != "true" ]; then break; fi
             AFTER=$(echo "$RESP" | jq -r '.data.repository.pullRequest.reviewThreads.pageInfo.endCursor')
-            if [ "$AFTER" = "null" ]; then
-              AFTER=""
-            fi
-            # If endCursor is missing despite hasNextPage=true, stop to avoid looping.
+            if [ "$AFTER" = "null" ]; then AFTER=""; fi
             if [ "$PAGE_NODE_COUNT" -eq 0 ] && [ "$HAS_NEXT" = "true" ]; then
-              echo "Warning: empty page while hasNextPage=true; stopping pagination to avoid loops." >> "$GITHUB_STEP_SUMMARY"
-              break
+              echo "::error::Thread scan incomplete: empty page with hasNextPage=true. Blocking build for safety."
+              exit 1
             fi
             if [ "$HAS_NEXT" = "true" ] && [ -z "$AFTER" ]; then
-              echo "Warning: missing endCursor while hasNextPage=true; stopping pagination to avoid infinite loop." >> "$GITHUB_STEP_SUMMARY"
-              break
+              echo "::error::Thread scan incomplete: missing endCursor with hasNextPage=true. Blocking build for safety."
+              exit 1
             fi
           done
-          if [ "$PAGE_COUNT" -ge "$MAX_PAGES" ]; then
-            # Fail-closed when pagination is incomplete: we cannot guarantee the thread count.
-            echo "Warning: reached maximum page limit while scanning review threads." >> "$GITHUB_STEP_SUMMARY"
-            echo "Review thread scan incomplete; blocking heavy jobs for safety." >> "$GITHUB_STEP_SUMMARY"
-            echo "allow_heavy=false" >> "$GITHUB_OUTPUT"
-            exit 0
+          if [ "$PAGE_COUNT" -ge "$MAX_PAGES" ] && [ "$HAS_NEXT" = "true" ]; then
+            echo "::error::Thread scan incomplete (exceeded $MAX_PAGES pages). Blocking build for safety."
+            exit 1
           fi
+          # Intentionally fail (not skip) — PRs with unresolved threads should not show green CI
           if [ "$UNRESOLVED" -gt 0 ]; then
-            echo "allow_heavy=false" >> "$GITHUB_OUTPUT"
-            echo "Unresolved review threads: $UNRESOLVED" >> "$GITHUB_STEP_SUMMARY"
-            exit 0
+            echo "::error::$UNRESOLVED unresolved review thread(s). Resolve them before building."
+            exit 1
           fi
-          echo "allow_heavy=true" >> "$GITHUB_OUTPUT"
           echo "Unresolved review threads: 0" >> "$GITHUB_STEP_SUMMARY"
 
-  # Legacy required status check - reports quality-checks result for branch protection
-  # Added to support migration from old "Lint, Test & Build" ruleset requirement
-  expected-status:
-    name: Lint, Test & Build
-    runs-on: ubuntu-latest
-    needs: [quality-checks]
-    if: always()
-
-    steps:
-      - name: Report status
-        run: |
-          if [ "${{ needs.quality-checks.result }}" = "success" ]; then
-            echo "Lint/Test/Build passed."
-            exit 0
-          fi
-          echo "Lint/Test/Build failed."
-          exit 1
-
-  # Job 2: Build and test Docker image
-  docker-build:
-    name: Build Docker Image
-    runs-on: ubuntu-latest
-    needs: [quality-checks, review-thread-gate]
-    # For PRs, only run heavy jobs if the gate succeeded and explicitly allowed them.
-    # Non-PR events (push/workflow_dispatch) run unconditionally.
-    # Note: !cancelled() is required because review-thread-gate is skipped on non-PR events,
-    # and GitHub Actions skips dependent jobs by default when any needs job is skipped.
-    if: |
-      !cancelled() &&
-      needs.quality-checks.result == 'success' &&
-      (github.event_name != 'pull_request' || (needs.review-thread-gate.result == 'success' && needs.review-thread-gate.outputs.allow_heavy == 'true'))
-
-    permissions:
-      contents: read
-
-    outputs:
-      image-built: ${{ steps.build.outputs.digest }}
-
-    steps:
       - name: Checkout code
         uses: actions/checkout@v6
 
@@ -534,7 +482,9 @@ jobs:
     name: Pipeline Summary
     runs-on: ubuntu-latest
     permissions: {}
-    needs: [quality-checks, review-thread-gate, docker-build, semantic-release, mcpb-bundle, docker-publish, mcp-registry-publish]
+    # All jobs listed so summary can read their outputs/results.
+    # if: always() ensures this runs even when needs jobs are skipped/failed.
+    needs: [quality-checks, docker-build, semantic-release, mcpb-bundle, docker-publish, mcp-registry-publish]
     if: always()
 
     steps:
@@ -551,12 +501,6 @@ jobs:
 
           if [[ "${{ needs.docker-build.result }}" == "success" ]]; then
             echo "✅ Docker build: Passed" >> $GITHUB_STEP_SUMMARY
-          elif [[ "${{ needs.docker-build.result }}" == "skipped" && "${{ needs.review-thread-gate.result }}" == "skipped" ]]; then
-            echo "⏭️ Docker build: Skipped (review gate not applicable)" >> $GITHUB_STEP_SUMMARY
-          elif [[ "${{ needs.docker-build.result }}" == "skipped" && "${{ needs.review-thread-gate.result }}" == "failure" ]]; then
-            echo "⏭️ Docker build: Skipped (review gate failed)" >> $GITHUB_STEP_SUMMARY
-          elif [[ "${{ needs.docker-build.result }}" == "skipped" && "${{ needs.review-thread-gate.outputs.allow_heavy }}" == "false" ]]; then
-            echo "⏭️ Docker build: Skipped (unresolved review threads)" >> $GITHUB_STEP_SUMMARY
           elif [[ "${{ needs.docker-build.result }}" == "skipped" ]]; then
             echo "⏭️ Docker build: Skipped" >> $GITHUB_STEP_SUMMARY
           else
