ci: close the verified review findings across the status-comment pipeline

- artifact-url grants actions:read (the composite's cross-run artifact
  reads 403 without it), gains a comment-cancelled job so a cancelled
  build can't leave the live In-progress claim up, and marks the
  link/size/compose steps continue-on-error so the status write always
  lands
- upsert-ci-status normalizes CRLF out of the body and every API read
  (a web-UI edit resubmits \r\n and defeated the whole-line marker
  matching), retries failed POST/PATCH inside the loop instead of dying
  under set -e, and warns when the whole comment nears GitHub's 65k cap;
  new functional test covers the CRLF round trip
- the composite documents the 128KiB env-transport limit and
  test-failures bounds its only unbounded list at composition
- build.py: comment reads distinguish 'absent' from 'unreadable' so a
  transient 502 can't compose a section that wipes the sibling row (page
  bound raised 3->30); failed upsert writes no longer count as asserts;
  record + reconcile both run every poll, so a missing dashboard href or
  a failed info-file write keeps retrying instead of stranding
- visual-regression orders the Running write before the suite so it can
  never overwrite the final verdict, and probes the Allure URL before
  rendering it as a link
- pr-comment-perf resolves fork-PR numbers via the commit->PRs lookup
  when workflow_run.pull_requests is empty
- the unit tests silence build.py's prints so its ::notice:: line stops
  annotating the test job's check run

Not changed: the dashboard URL's org/project ids in public comments stay
by explicit earlier decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: eordano

.github/actions/ci-status-comment/action.yml

@@ -18,6 +18,9 @@ inputs:
       Markdown for this section (inline badge + message). Rendered between the
       section markers after dropping marker-shaped lines; bodies over 20000
       chars are truncated with fences/<details> re-closed and a truncation note.
+      Callers must keep it under ~120KB: it travels as one env string, and
+      Linux rejects any single env entry over 128KiB (E2BIG) before the
+      truncation here can run.
     required: true
   github-token:
     description: Token with pull-requests:write used to read and upsert the comment.

.github/actions/ci-status-comment/test-upsert-ci-status.sh

@@ -229,4 +229,21 @@ grep -q 'RACE-CONVERGED' <<< "$(body_of)" || fail "race: final body missing conv
 [ "$(cat "$WORK/stale-reads")" = 0 ] || fail "race: stale read was not consumed"
 pass "stale re-read retried until convergence"
 
+# --- 11. CRLF body normalized in place, fences not duplicated -----------------
+reset_store "$(python3 - <<'PY'
+import json
+body = ("<!-- ci-status -->\r\n### 🚦 CI Status\r\n"
+        "<!-- ci:build:start -->\r\nOLD-BUILD\r\n<!-- ci:build:end -->")
+print(json.dumps([{"id": 9, "user": {"login": "github-actions[bot]"}, "body": body}]))
+PY
+)"
+run_upsert build "CRLF-BUILD" >/dev/null
+[ "$(count)" = 1 ] || fail "crlf: expected 1 comment"
+BODY="$(body_of)"
+[ "$(grep -cF '<!-- ci:build:start -->' <<< "$BODY")" = 1 ] || fail "crlf: build fence duplicated"
+grep -q 'CRLF-BUILD' <<< "$BODY" || fail "crlf: new content missing"
+grep -q 'OLD-BUILD' <<< "$BODY" && fail "crlf: stale content still rendered"
+grep -q $'\r' <<< "$BODY" && fail "crlf: body still carries CR"
+pass "CRLF body replaced in place, no duplicate fences"
+
 [ "$FAILED" = 0 ] && echo "ALL PASS" || { echo "FAILURES PRESENT"; exit 1; }

.github/actions/ci-status-comment/upsert-ci-status.sh

@@ -36,6 +36,12 @@ if [ -n "${SECTION_BODY_FILE:-}" ]; then
   SECTION_BODY="$(cat "$SECTION_BODY_FILE")"
 fi
 
+# Everything below matches markers as whole lines, which CRLF endings defeat —
+# and GitHub's web editor resubmits an edited comment with \r\n. Normalize the
+# body here and every API read below, so one manual edit cannot make each
+# writer append a duplicate fence beneath a stale, still-rendering one.
+SECTION_BODY="${SECTION_BODY//$'\r'/}"
+
 # GitHub caps an issue comment at 65536 chars across every section; keep one
 # writer — whichever path its body arrived by — from consuming the whole budget
 # and failing an unrelated section's PATCH with an opaque 422. Truncation is
@@ -190,6 +196,7 @@ for attempt in 1 2 3 4 5; do
 
   if [ -n "$COMMENT_ID" ]; then
     CURRENT_BODY=$(jq -r --arg id "$COMMENT_ID" '.[] | select(.id==($id|tonumber)) | .body' <<< "$(flatten_pages "$COMMENTS")")
+    CURRENT_BODY="${CURRENT_BODY//$'\r'/}"
   else
     CURRENT_BODY=""
   fi
@@ -215,13 +222,29 @@ for attempt in 1 2 3 4 5; do
 
   NEW_BODY="$(replace_section "$CURRENT_BODY")"
 
+  # Near GitHub's 65536-char comment cap the write starts 422ing; the per-
+  # section CAP cannot see the other sections, so at least say why.
+  if [ "${#NEW_BODY}" -gt 65000 ]; then
+    echo "::warning::Unified comment is ${#NEW_BODY} chars — at/over GitHub's 65536 cap; a section needs a tighter cap."
+  fi
+
+  # A failed write must land in the retry loop, not kill the script under
+  # set -e — that would fail this section's job and strand the section stale.
   if [ -z "$COMMENT_ID" ]; then
-    RESULT=$(jq -n --arg b "$NEW_BODY" '{body:$b}' \
-      | gh api -X POST "/repos/$REPO/issues/$PR_NUMBER/comments" --input -)
+    if ! RESULT=$(jq -n --arg b "$NEW_BODY" '{body:$b}' \
+      | gh api -X POST "/repos/$REPO/issues/$PR_NUMBER/comments" --input -); then
+      echo "Create failed (attempt $attempt); retrying."
+      sleep $((attempt * 2))
+      continue
+    fi
     COMMENT_ID=$(jq -r '.id' <<< "$RESULT")
   else
-    jq -n --arg b "$NEW_BODY" '{body:$b}' \
-      | gh api -X PATCH "/repos/$REPO/issues/comments/$COMMENT_ID" --input - >/dev/null
+    if ! jq -n --arg b "$NEW_BODY" '{body:$b}' \
+      | gh api -X PATCH "/repos/$REPO/issues/comments/$COMMENT_ID" --input - >/dev/null; then
+      echo "Update failed (attempt $attempt); retrying."
+      sleep $((attempt * 2))
+      continue
+    fi
   fi
 
   # Re-read and confirm our section landed on the surviving comment, and that no
@@ -231,6 +254,7 @@ for attempt in 1 2 3 4 5; do
   RIDS=()
   while IFS= read -r line; do [ -n "$line" ] && RIDS+=("$line"); done <<< "$(marker_ids "$RECHECK")"
   LIVE_BODY=$(jq -r --arg id "$COMMENT_ID" '.[] | select(.id==($id|tonumber)) | .body' <<< "$(flatten_pages "$RECHECK")")
+  LIVE_BODY="${LIVE_BODY//$'\r'/}"
 
   # A write that landed on a younger duplicate is doomed: GC keeps the oldest,
   # so this section's content would vanish with the duplicate. Retry on the

.github/workflows/pr-comment-artifact-url.yml

@@ -21,8 +21,11 @@ on:
       - "main"
   workflow_dispatch:
 permissions:
-  contents: read  
+  contents: read
   pull-requests: write
+  # ucb-build-links reads the build run's artifacts and jobs cross-run — the
+  # same reason the sibling comment workflows grant it.
+  actions: read
 
 jobs:
   pre-validation:
@@ -132,6 +135,32 @@ jobs:
 
             Build skipped — no changes detected under `Explorer/`.
 
+  # A cancelled run matches neither the success nor the failure gate, and the
+  # build job's live writer may have left an In-progress badge and rows up —
+  # without this the comment claims a build is running forever.
+  comment-cancelled:
+    needs: pre-validation
+    if: github.event.action == 'completed' && github.event.workflow_run.conclusion == 'cancelled' && needs.pre-validation.outputs.pr-number != ''
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout CI status action
+        uses: actions/checkout@v6
+        with:
+          sparse-checkout: .github/actions/ci-status-comment
+          sparse-checkout-cone-mode: false
+          persist-credentials: false
+
+      - name: Post cancelled build section
+        uses: ./.github/actions/ci-status-comment
+        with:
+          pr-number: ${{ needs.pre-validation.outputs.pr-number }}
+          section: build
+          github-token: ${{ github.token }}
+          body: |-
+            [![Build](https://img.shields.io/badge/Build-Cancelled-lightgrey?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }})
+
+            Build cancelled — push a new commit or re-run the workflow to refresh this section.
+
   comment-success:
     needs: [pre-validation, check-build-ran]
     if: github.event.workflow_run.conclusion == 'success' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.player-artifacts == 'true'
@@ -218,6 +247,7 @@ jobs:
           echo "BUILD_DATE=$BUILD_DATE" >> "$GITHUB_ENV"
 
       - name: Download size reports
+        continue-on-error: true
         env:
           GITHUB_TOKEN: ${{ github.token }}
           OWNER: ${{ github.repository_owner }}
@@ -253,6 +283,7 @@ jobs:
           fi
 
       - name: Fetch Unity Cloud build links
+        continue-on-error: true
         id: ucb
         uses: ./.github/actions/ucb-build-links
         with:
@@ -265,6 +296,7 @@ jobs:
       # body so a link whose id could not be resolved is dropped instead of
       # rendering broken.
       - name: Compose platform rows
+        continue-on-error: true
         env:
           WINDOWS_CELL: ${{ steps.ucb.outputs.windows-cell }}
           MAC_CELL: ${{ steps.ucb.outputs.mac-cell }}
@@ -419,6 +451,7 @@ jobs:
           persist-credentials: false
 
       - name: Fetch Unity Cloud build links
+        continue-on-error: true
         id: ucb
         uses: ./.github/actions/ucb-build-links
         with:

.github/workflows/pr-comment-perf.yml

@@ -31,9 +31,20 @@ jobs:
         id: pr
         env:
           WORKFLOW_RUN_EVENT_OBJ: ${{ toJSON(github.event.workflow_run) }}
+          GITHUB_TOKEN: ${{ github.token }}
+          REPO: ${{ github.repository }}
         run: |
           PR_NUMBER=$(jq -r '.pull_requests[0].number' <<< "$WORKFLOW_RUN_EVENT_OBJ")
-          echo "PR number: $PR_NUMBER"
+          # workflow_run leaves pull_requests empty for fork-origin PRs; the
+          # commit->PRs lookup still resolves those, so a fork's perf verdict
+          # is not silently dropped.
+          if [[ -z "$PR_NUMBER" || "$PR_NUMBER" == "null" ]]; then
+            HEAD_SHA=$(jq -r '.head_sha // empty' <<< "$WORKFLOW_RUN_EVENT_OBJ")
+            if [ -n "$HEAD_SHA" ]; then
+              PR_NUMBER=$(gh api "/repos/$REPO/commits/$HEAD_SHA/pulls" --jq '.[0].number // empty' 2>/dev/null || true)
+            fi
+          fi
+          echo "PR number: ${PR_NUMBER:-<none>}"
           if [[ -z "$PR_NUMBER" || "$PR_NUMBER" == "null" ]]; then
             echo "No PR associated with this run, skipping."
             echo "pr-number=" >> "$GITHUB_OUTPUT"

.github/workflows/pr-comment-test-failures.yml

@@ -151,6 +151,13 @@ jobs:
             fi
           done
 
+          # The only unbounded list in this body. The composite's env transport
+          # rejects any single env string over 128KiB (E2BIG) before its own
+          # 20k truncation can run, so bound it at composition.
+          if [ "${#failed_list}" -gt 60000 ]; then
+            failed_list="${failed_list:0:60000}"$'\n'"- …list truncated — see the run for the full set."$'\n'
+          fi
+
           case "$status" in
             incomplete) badge="https://img.shields.io/badge/Tests-Incomplete-d29922?logo=codecov&logoColor=white&style=for-the-badge"; headline="$warnings" ;;
             failed)     badge="https://img.shields.io/badge/Tests-Failed!-ff0000?logo=codecov&logoColor=white&style=for-the-badge";     headline="Some Unity tests failed ❌" ;;

.github/workflows/visual-regression.yml

@@ -197,8 +197,12 @@ jobs:
 
   run-suite:
     name: Run visual suite
-    needs: resolve
-    if: needs.resolve.outputs.authorized == 'true'
+    # automation-pending in needs: orders the two writers of the automation
+    # section — the "Running!" write must precede the suite (and so the final
+    # verdict), or a queue-delayed pending write can overwrite the verdict and
+    # stick. !cancelled() keeps the suite running when pending skips or fails.
+    needs: [resolve, automation-pending]
+    if: ${{ !cancelled() && needs.resolve.outputs.authorized == 'true' }}
     # @main pins us to the merged version of the reusable workflow so PRs to
     # explorer-automation that touch run-visual-suite.yml don't accidentally
     # affect every unity-explorer PR's visual run.
@@ -256,6 +260,15 @@ jobs:
           # passes neither, so keep the three in lockstep if that ever changes.
           REPORT_URL="${PUBLIC_URL_PREFIX}/@dcl/${REPO//\//-}/visual-regression/test/macos/${PR_NUMBER}/${COMMIT_SHA}/index.html"
 
+          # The callee only syncs a report to S3 when the suite produced one —
+          # probe before rendering the link so a dead run doesn't present a
+          # 404 as a working report.
+          if curl -sfIL --max-time 15 "$REPORT_URL" >/dev/null 2>&1; then
+            REPORT_ROW="| Allure report | [Open report]($REPORT_URL) |"
+          else
+            REPORT_ROW="| Allure report | not produced — see the workflow run |"
+          fi
+
           DELIM="EOF_${RANDOM}${RANDOM}_$$"
           {
             echo "body<<$DELIM"
@@ -266,7 +279,7 @@ jobs:
             echo "| Name | Link |"
             echo "| -------- | ----------------------- |"
             echo "| Commit | [\`$COMMIT_SHA\`](${GITHUB_SERVER_URL:-https://github.com}/${REPO}/commit/${COMMIT_SHA}) |"
-            echo "| Allure report | [Open report]($REPORT_URL) |"
+            echo "$REPORT_ROW"
             echo "| Workflow run | [View run]($RUN_URL) |"
             echo ""
             echo "<sub>Triggered via \`/visual-tests\` · the detailed per-platform comment is posted separately.</sub>"

scripts/cloudbuild/build.py

@@ -709,17 +709,20 @@ def _github_api(path):
 
 
 def _build_section_of_status_comment():
-    """Current text between the build fences of the unified CI status comment, or ''.
+    """Current text between the build fences of the unified CI status comment.
 
     Oldest marker-bearing bot comment wins, matching upsert-ci-status.sh's
-    duplicate-collapse rule, so both read the same comment.
+    duplicate-collapse rule, so both read the same comment. Returns '' when the
+    comment genuinely does not exist, and None when it could not be determined
+    (API failure, or the page bound ran out) — writing on None would compose a
+    section without the sibling platform's row and wipe it.
     """
     repo = os.getenv('GITHUB_REPOSITORY')
     pr = os.getenv('PR_NUMBER')
-    for page in (1, 2, 3):
+    for page in range(1, 31):
         resp = _github_api(f'/repos/{repo}/issues/{pr}/comments?per_page=100&page={page}')
         if resp.status_code != 200:
-            return ''
+            return None
         comments = resp.json()
         for comment in comments:
             body = comment.get('body') or ''
@@ -728,8 +731,8 @@ def _build_section_of_status_comment():
                 end = body.find('<!-- ci:build:end -->')
                 return body[start:end] if 0 <= start < end else ''
         if len(comments) < 100:
-            break
-    return ''
+            return ''
+    return None
 
 
 def _platform_key():
@@ -776,12 +779,15 @@ def upsert_live_comment(build_id, only_if_missing=False):
     Each matrix job re-reads the section and carries the other target's live
     row along, so concurrent first writes converge on both rows instead of
     clobbering each other; write races on the comment itself are the upsert
-    script's problem. Returns whether a write was attempted.
+    script's problem. Returns True when a write landed, False when the row was
+    already present, and None when the read or the write failed.
     """
     platform = _platform_key()
     label = {'windows64': 'Windows', 'macos': 'Mac'}.get(platform, platform)
     marker = f'{LIVE_MARKER_PREFIX}{platform} -->'
     section = _build_section_of_status_comment()
+    if section is None:
+        return None
     if only_if_missing and marker in section:
         return False
 
@@ -819,11 +825,11 @@ def upsert_live_comment(build_id, only_if_missing=False):
                    SECTION='build',
                    SECTION_BODY='',
                    SECTION_BODY_FILE=body_file)
-        subprocess.run(['bash', CI_STATUS_SCRIPT], env=env, timeout=180, check=False)
+        result = subprocess.run(['bash', CI_STATUS_SCRIPT], env=env, timeout=180, check=False)
     finally:
         if body_file:
             os.unlink(body_file)
-    return True
+    return True if result.returncode == 0 else None
 
 
 def maybe_update_live_comment(build_id, reconcile=False, force=False):
@@ -850,10 +856,13 @@ def maybe_update_live_comment(build_id, reconcile=False, force=False):
         return
     try:
         _live_comment_last_attempt = time.time()
-        if upsert_live_comment(build_id, only_if_missing=reconcile):
+        outcome = upsert_live_comment(build_id, only_if_missing=reconcile)
+        if outcome is True:
             _live_comment_asserts += 1
             _live_comment_confirms = 0
-        elif reconcile:
+        elif outcome is False and reconcile:
+            # Only a confirmed present row spends the probe budget; a failed
+            # read or write (None) must leave both counters for the retry.
             _live_comment_confirms += 1
     except Exception as e:
         print(f'note: live status-comment update failed: {e}')
@@ -956,10 +965,12 @@ def run_poll_loop(id, build_already_active=False, resumed_build_elapsed=0):
 
         keep_polling, status, response_json = poll_build(id)
 
-        if dashboard_url is None:
+        # Both run every poll: record keeps retrying until the info file lands
+        # AND a dashboard href arrives, and reconcile self-heals the live row
+        # whether or not an href ever qualifies (it rate-limits internally).
+        if dashboard_url is None or not _build_link_info_written:
             record_build_link_info(id, response_json)
-        else:
-            maybe_update_live_comment(id, reconcile=True)
+        maybe_update_live_comment(id, reconcile=True)
 
         queued_reason = response_json.get('queuedReason')
         if queued_reason and status in QUEUE_STATUSES:

scripts/cloudbuild/test_build_helpers.py

@@ -9,6 +9,7 @@
 import sys
 import tempfile
 import unittest
+from unittest import mock
 
 sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
 import build  # noqa: E402
@@ -92,6 +93,11 @@ def setUp(self):
         old_cwd = os.getcwd()
         self.addCleanup(os.chdir, old_cwd)
         os.chdir(tmp.name)
+        # Silence build.py's prints: its ::notice:: line is a live workflow
+        # command when the test job itself runs on the Actions runner.
+        silencer = mock.patch('builtins.print')
+        silencer.start()
+        self.addCleanup(silencer.stop)
         # PR_NUMBER unset keeps maybe_update_live_comment inert.
         self.set_env(TARGET='windows64-x', ORG_ID='org1', PROJECT_ID='proj1', PR_NUMBER=None)
         build.dashboard_url = None