fix: address CR findings — explicit pr_opened output + safe JSON + Slack fallback

jpr5 · jpr5 · commit 143b3eb41bd2 · 2026-04-16T14:12:15.000-07:00
Fixes 3 HIGH findings from R1 review on CopilotKit#3988: 1. pr_url empty was used as a proxy for "no PR opened because clean-transform was empty", but it's also empty on every error path (bot-token failure, gh pr create failure, push failure). Replace with an explicit pr_opened=true/ false output from the push step — true only after the PR URL is captured, false only on the deliberate CHANGED=0 path. Error paths leave it unset so alerts fall through to the failure() handler. 2. review_items_json was string-interpolated raw into a JSON payload inside triple-backticks. Any filename containing ", \\, or a control character would break the payload. Moved to a jq-based payload-file-path pattern: a dedicated Build Slack payloads step writes each payload to disk with jq --arg, so all values are safely JSON-escaped regardless of content. Slack steps consume the tmpfiles via payload-file-path. 3. If a notify-* step itself fails (webhook 5xx, rate limit, malformed JSON), the review-needed alert was silently lost — the existing failure() alert was gated on pr_url == '' and would not fire. Added an unconditional fallback step that posts a plain-text "alert machinery failed" message via curl when any notify-* step's outcome is failure, so we never lose a review-needed or failure notification.
diff --git a/.github/workflows/showcase_docs-sync.yml b/.github/workflows/showcase_docs-sync.yml
@@ -65,9 +65,10 @@ jobs:
               echo "::warning::review-items.txt not found despite exit code 3 — sync script may have a bug"
               REVIEW_ITEMS="(review-items.txt not found — check sync script output)"
             fi
-            # JSON-escape review items for embedding in Slack payload
-            REVIEW_ITEMS_JSON=$(echo "$REVIEW_ITEMS" | jq -Rs '.' | sed 's/^"//;s/"$//')
-            echo "review_items_json=${REVIEW_ITEMS_JSON}" >> "$GITHUB_OUTPUT"
+            # Persist review items to a file for downstream jq-based payload building
+            # (avoids unsafe string interpolation of untrusted filenames into JSON).
+            printf '%s' "$REVIEW_ITEMS" > review-items-raw.txt
+            echo "review_items_file=review-items-raw.txt" >> "$GITHUB_OUTPUT"
           else
             echo "Sync failed with exit code $EXIT_CODE"
             exit 1
@@ -100,8 +101,11 @@ jobs:
           CHANGED=$(git diff --cached --name-only | wc -l | tr -d ' ')
 
           if [ "$CHANGED" = "0" ]; then
-            echo "Nothing to commit"
+            echo "Nothing to commit — no PR will be opened (deliberate)"
             echo "files_changed=0" >> "$GITHUB_OUTPUT"
+            # Explicit signal: no PR opened, and this is the intended outcome
+            # (not an error path). Downstream alerts key on this.
+            echo "pr_opened=false" >> "$GITHUB_OUTPUT"
             exit 0
           fi
 
@@ -120,54 +124,162 @@ jobs:
           echo "Created PR: $PR_URL"
           echo "files_changed=${CHANGED}" >> "$GITHUB_OUTPUT"
           echo "pr_url=${PR_URL}" >> "$GITHUB_OUTPUT"
+          # Only set pr_opened=true after the PR URL is captured. Any earlier
+          # failure (push, gh pr create) will leave this unset so downstream
+          # alerts fall through to failure() instead of a falsely-benign path.
+          echo "pr_opened=true" >> "$GITHUB_OUTPUT"
 
           gh pr merge "$PR_URL" --merge
 
+      # Build all Slack payloads via jq into tmpfiles. This guarantees any
+      # review-item filename containing ", \, or control chars is safely
+      # JSON-escaped (never string-interpolated into a JSON literal).
+      - name: Build Slack payloads
+        id: payloads
+        if: always()
+        env:
+          RUN_URL: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}
+          PR_URL: ${{ steps.push.outputs.pr_url }}
+          FILES_CHANGED: ${{ steps.push.outputs.files_changed }}
+          REVIEW_ITEMS_FILE: ${{ steps.sync.outputs.review_items_file }}
+          SYNC_OUTCOME: ${{ steps.sync.outcome }}
+          BOT_TOKEN_OUTCOME: ${{ steps.bot-token.outcome }}
+          PUSH_OUTCOME: ${{ steps.push.outcome }}
+        run: |
+          set -euo pipefail
+          mkdir -p slack-payloads
+
+          # auto-sync success (PR merged)
+          jq -n \
+            --arg pr_url "${PR_URL:-}" \
+            --arg files "${FILES_CHANGED:-?}" \
+            --arg run_url "$RUN_URL" \
+            '{text: (":arrows_counterclockwise: *Docs sync*: auto-merged " + $files + " file(s)\n" + $pr_url + "\n<" + $run_url + "|View run>")}' \
+            > slack-payloads/auto-sync.json
+
+          # merge failed (PR exists but gh pr merge failed)
+          jq -n \
+            --arg pr_url "${PR_URL:-}" \
+            --arg run_url "$RUN_URL" \
+            '{text: (":warning: *Docs sync*: PR created but auto-merge FAILED — needs manual merge\n" + $pr_url + "\n<" + $run_url + "|View run>")}' \
+            > slack-payloads/merge-failed.json
+
+          # review-needed payloads: read items from file, let jq handle escaping
+          REVIEW_ITEMS=""
+          if [ -n "${REVIEW_ITEMS_FILE:-}" ] && [ -f "$REVIEW_ITEMS_FILE" ]; then
+            REVIEW_ITEMS=$(cat "$REVIEW_ITEMS_FILE")
+          fi
+
+          jq -n \
+            --arg items "$REVIEW_ITEMS" \
+            --arg pr_url "${PR_URL:-}" \
+            --arg run_url "$RUN_URL" \
+            '{text: (":warning: *Docs sync*: files needing manual review\n```" + $items + "```\nReview: " + $pr_url + "\n<" + $run_url + "|View run>")}' \
+            > slack-payloads/review-with-pr.json
+
+          jq -n \
+            --arg items "$REVIEW_ITEMS" \
+            --arg run_url "$RUN_URL" \
+            '{text: (":warning: *Docs sync*: files needing manual review (no auto-PR opened)\n```" + $items + "```\n<" + $run_url + "|View run>")}' \
+            > slack-payloads/review-no-pr.json
+
+          # failure alert
+          FAILED_STEP="unknown"
+          if [ "${SYNC_OUTCOME:-}" = "failure" ]; then
+            FAILED_STEP="sync-docs script"
+          elif [ "${BOT_TOKEN_OUTCOME:-}" = "failure" ]; then
+            FAILED_STEP="bot token generation (check DEVOPS_BOT_PRIVATE_KEY secret)"
+          elif [ "${PUSH_OUTCOME:-}" = "failure" ]; then
+            FAILED_STEP="push/PR creation"
+          fi
+          jq -n \
+            --arg failed_step "$FAILED_STEP" \
+            --arg run_url "$RUN_URL" \
+            '{text: (":x: *Docs sync*: workflow failed\n*Failed step:* " + $failed_step + " | <" + $run_url + "|View run>")}' \
+            > slack-payloads/failure.json
+
+          # plain-text fallback used only when notify-* steps themselves fail
+          # (webhook 5xx, rate limit, etc). Keeps us from silently losing alerts.
+          jq -n \
+            --arg run_url "$RUN_URL" \
+            '{text: (":rotating_light: *Docs sync*: review/alert machinery failed — check Actions UI\n<" + $run_url + "|View run>")}' \
+            > slack-payloads/notify-fallback.json
+
       - name: Notify Slack (auto-sync)
+        id: notify-auto-sync
         if: always() && steps.push.outcome == 'success' && steps.push.outputs.files_changed != '0'
         uses: slackapi/slack-github-action@v2.1.0
         with:
           webhook: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
           webhook-type: incoming-webhook
-          payload: |
-            { "text": ":arrows_counterclockwise: *Docs sync*: auto-merged ${{ steps.push.outputs.files_changed || '?' }} file(s)\n${{ steps.push.outputs.pr_url || '' }}\n<https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View run>" }
+          payload-file-path: slack-payloads/auto-sync.json
 
       - name: Notify Slack (merge failed)
-        if: failure() && steps.push.outputs.pr_url != ''
+        id: notify-merge-failed
+        if: failure() && steps.push.outputs.pr_opened == 'true'
         uses: slackapi/slack-github-action@v2.1.0
         with:
           webhook: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
           webhook-type: incoming-webhook
-          payload: |
-            { "text": ":warning: *Docs sync*: PR created but auto-merge FAILED — needs manual merge\n${{ steps.push.outputs.pr_url }}\n<https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View run>" }
+          payload-file-path: slack-payloads/merge-failed.json
 
       # Review items = files the sync script flagged but did NOT write to disk
-      # (local modifications, files deleted on main). A PR may have been opened
-      # for the clean-transform portion — link it so reviewers can click through.
-      - name: Notify Slack (review needed)
-        if: always() && steps.sync.outputs.action == 'push_and_pr' && steps.push.outputs.pr_url != ''
+      # (local modifications, files deleted on main). A PR was opened for the
+      # clean-transform portion — link it so reviewers can click through.
+      # Gated on pr_opened == 'true' so it only fires when we actually have a
+      # PR URL (not on any error path that leaves pr_url empty).
+      - name: Notify Slack (review needed, with PR)
+        id: notify-review-with-pr
+        if: always() && steps.sync.outputs.action == 'push_and_pr' && steps.push.outputs.pr_opened == 'true'
         uses: slackapi/slack-github-action@v2.1.0
         with:
           webhook: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
           webhook-type: incoming-webhook
-          payload: |
-            { "text": ":warning: *Docs sync*: files needing manual review\n```${{ steps.sync.outputs.review_items_json }}```\nReview: ${{ steps.push.outputs.pr_url }}\n<https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View run>" }
+          payload-file-path: slack-payloads/review-with-pr.json
 
-      # Fallback: review items but no PR was opened (clean-transform portion was empty).
+      # Review items but the clean-transform portion was empty so no PR was
+      # opened. Gated on pr_opened == 'false' (deliberate no-PR path) — not
+      # empty pr_url, which would also match error paths.
       - name: Notify Slack (review needed, no PR)
-        if: always() && steps.sync.outputs.action == 'push_and_pr' && steps.push.outputs.pr_url == ''
+        id: notify-review-no-pr
+        if: always() && steps.sync.outputs.action == 'push_and_pr' && steps.push.outputs.pr_opened == 'false'
         uses: slackapi/slack-github-action@v2.1.0
         with:
           webhook: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
           webhook-type: incoming-webhook
-          payload: |
-            { "text": ":warning: *Docs sync*: files needing manual review (no auto-PR opened)\n```${{ steps.sync.outputs.review_items_json }}```\n<https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View run>" }
+          payload-file-path: slack-payloads/review-no-pr.json
 
       - name: Notify Slack (failure)
-        if: failure() && steps.push.outputs.pr_url == ''
+        id: notify-failure
+        if: failure() && steps.push.outputs.pr_opened != 'true'
         uses: slackapi/slack-github-action@v2.1.0
         with:
           webhook: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
           webhook-type: incoming-webhook
-          payload: |
-            { "text": ":x: *Docs sync*: workflow failed\n*Failed step:* ${{ steps.sync.outcome == 'failure' && 'sync-docs script' || steps.bot-token.outcome == 'failure' && 'bot token generation (check DEVOPS_BOT_PRIVATE_KEY secret)' || steps.push.outcome == 'failure' && 'push/PR creation' || 'unknown' }} | <https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}|View run>" }
+          payload-file-path: slack-payloads/failure.json
+
+      # Unconditional fallback: if any notify-* step above failed (webhook 5xx,
+      # rate limit, malformed payload), fire a plain-text alert so we never
+      # silently lose a review-needed or failure notification. Uses curl
+      # directly so it doesn't share failure modes with the slackapi action.
+      - name: Notify Slack (alert machinery failed)
+        if: >-
+          always() && (
+            steps.notify-auto-sync.outcome == 'failure' ||
+            steps.notify-merge-failed.outcome == 'failure' ||
+            steps.notify-review-with-pr.outcome == 'failure' ||
+            steps.notify-review-no-pr.outcome == 'failure' ||
+            steps.notify-failure.outcome == 'failure'
+          )
+        env:
+          SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_OSS_ALERTS }}
+        run: |
+          set -eu
+          if [ -z "${SLACK_WEBHOOK:-}" ]; then
+            echo "::warning::SLACK_WEBHOOK_OSS_ALERTS not set — cannot post fallback alert"
+            exit 0
+          fi
+          curl -sS -X POST \
+            -H "Content-Type: application/json" \
+            --data @slack-payloads/notify-fallback.json \
+            "$SLACK_WEBHOOK" || echo "::warning::fallback Slack post also failed"
