From 00ea5fd9a3787ff0fb3724b1af29afc18104f784 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 01:10:03 +0200 Subject: [PATCH 01/28] ci: deep-link the Unity Cloud build page from CI and the PR status comment The PR status comment's build badge was a bare shields.io image (clicking it opened the image itself), and finding the actual Unity Cloud build meant going to cloud.unity.com and searching for the target and build id by hand. - build.py captures the dashboard deep link (links.dashboard_summary / dashboard_log) from the first build response that carries one, prints it as a ::notice::, adds it to the step summary, and persists it to unity_cloud_build_info.env. - build-unitycloud.yml uploads that file as a unity_build_info_* artifact. It is written as soon as the Unity-side build id is known, so it exists for failed builds too. - pr-comment-artifact-url.yml adds "Unity Cloud build (Windows/Mac)" rows linking the build id to its Unity Cloud page, on both the success and failure comments, and wraps every badge in a link to the Actions run. The info files are produced by the PR-controlled build workflow, so ids and URLs are validated (numeric id, Unity dashboard origin, conservative charset) before being rendered into the comment. - check-build-ran now also counts unity_build_info_* artifacts as evidence that a build ran, so a build that failed before producing player artifacts posts the failure comment (with the Unity Cloud link) instead of leaving the comment stuck on "Pending". Note: the dashboard URL comes from the Unity Cloud Build API response and contains the org/project slugs; it will be visible in PR comments. Co-Authored-By: Claude Fable 5 --- .github/workflows/build-unitycloud.yml | 11 ++ .github/workflows/pr-comment-artifact-url.yml | 122 +++++++++++++++++- scripts/cloudbuild/build.py | 53 +++++++- 3 files changed, 178 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build-unitycloud.yml b/.github/workflows/build-unitycloud.yml index 104e43c698a..764f1a7c0bb 100644 --- a/.github/workflows/build-unitycloud.yml +++ b/.github/workflows/build-unitycloud.yml @@ -976,6 +976,17 @@ jobs: path: unity_cloud_log.log if-no-files-found: error + # Written by build.py as soon as the Unity-side build id is known, so it exists for + # failed builds too. The PR status comment uses it to deep-link the Unity Cloud + # build page instead of asking humans to search cloud.unity.com by hand. + - name: Upload Unity Cloud build info + if: ${{ always() && hashFiles('unity_cloud_build_info.env') != '' }} + uses: actions/upload-artifact@v6 + with: + name: unity_build_info_${{ matrix.target }}_${{ needs.prebuild.outputs.install_source }} + path: unity_cloud_build_info.env + if-no-files-found: error + - name: Print cloud logs if: ${{ always() && hashFiles('unity_cloud_log.log') != '' }} run: cat unity_cloud_log.log diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 4ae385ce0f0..ce45ae97ee2 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -61,7 +61,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - ![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge) + [![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) New build in progress, come back later! @@ -80,9 +80,12 @@ jobs: REPO: ${{ github.event.repository.name }} RUN_ID: ${{ github.event.workflow_run.id }} run: | - # Check if any build artifacts exist (they only exist when Build jobs ran) + # Check if any build artifacts exist (they only exist when Build jobs ran). + # unity_build_info_* is uploaded as soon as the Unity-side build starts, so a + # build that failed before producing player artifacts still counts as "ran" + # and gets a failure comment with a Unity Cloud link instead of staying pending. ARTIFACT_COUNT=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts" \ - --jq '[.artifacts[] | select(.name | startswith("Decentraland_"))] | length') + --jq '[.artifacts[] | select((.name | startswith("Decentraland_")) or (.name | startswith("unity_build_info_")))] | length') echo "Build artifact count: $ARTIFACT_COUNT" if [ "$ARTIFACT_COUNT" -gt 0 ]; then echo "build-ran=true" >> "$GITHUB_OUTPUT" @@ -109,7 +112,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - ![Build](https://img.shields.io/badge/Build-Skipped-yellow?logo=unity&logoColor=white&style=for-the-badge) + [![Build](https://img.shields.io/badge/Build-Skipped-yellow?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) Build skipped — no changes detected under `Explorer/`. @@ -230,6 +233,54 @@ jobs: echo "SIZE_REPORT=" >> "$GITHUB_ENV" fi + - name: Fetch Unity Cloud build links + env: + GITHUB_TOKEN: ${{ github.token }} + OWNER: ${{ github.repository_owner }} + REPO: ${{ github.event.repository.name }} + run: | + set -euo pipefail + + # The info files come out of the PR-controlled build workflow, so treat them as + # untrusted input: accept only a numeric build id and a Unity dashboard URL with + # a conservative charset before letting them anywhere near the comment body. + URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&-]*$' + parse_info() { + local target="$1" + local dir="ucb_info_${target}" + REPLY_ID="" + REPLY_URL="" + if gh run download "$PREVIOUS_JOB_ID" \ + --repo "$OWNER/$REPO" \ + --name "unity_build_info_${target}_launcher" \ + --dir "$dir" 2>/dev/null; then + REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" + [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" + fi + } + + ROWS="" + parse_info windows64 + if [ -n "$REPLY_URL" ]; then + ROWS+="| Unity Cloud build (Windows) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + elif [ -n "$REPLY_ID" ]; then + ROWS+="| Unity Cloud build (Windows) | #${REPLY_ID} |"$'\n' + fi + parse_info macos + if [ -n "$REPLY_URL" ]; then + ROWS+="| Unity Cloud build (Mac) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + elif [ -n "$REPLY_ID" ]; then + ROWS+="| Unity Cloud build (Mac) | #${REPLY_ID} |"$'\n' + fi + + { + echo "UCB_ROWS<> "$GITHUB_ENV" + - name: Update build section uses: ./.github/actions/ci-status-comment with: @@ -237,7 +288,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - ![Build](https://img.shields.io/badge/Build-Success!-3fb950?logo=unity&logoColor=white&style=for-the-badge) + [![Build](https://img.shields.io/badge/Build-Success!-3fb950?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. @@ -250,6 +301,7 @@ jobs: | Download Mac | ${{ github.server_url }}/${{ github.repository }}/suites/${{ env.SUITE_ID }}/artifacts/${{ env.MAC_ARTIFACT_ID }} | | Download Mac S3 | ${{ format('{0}/{1}/Decentraland_macos.zip', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} | | Built on | ${{ env.BUILD_DATE }} | + ${{ env.UCB_ROWS }} ${{ env.SIZE_REPORT }} @@ -290,6 +342,60 @@ jobs: sparse-checkout-cone-mode: false persist-credentials: false + - name: Fetch Unity Cloud build links + env: + GITHUB_TOKEN: ${{ github.token }} + OWNER: ${{ github.repository_owner }} + REPO: ${{ github.event.repository.name }} + RUN_ID: ${{ github.event.workflow_run.id }} + run: | + set -euo pipefail + + # The info files come out of the PR-controlled build workflow, so treat them as + # untrusted input: accept only a numeric build id and a Unity dashboard URL with + # a conservative charset before letting them anywhere near the comment body. + URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&-]*$' + parse_info() { + local target="$1" + local dir="ucb_info_${target}" + REPLY_ID="" + REPLY_URL="" + if gh run download "$RUN_ID" \ + --repo "$OWNER/$REPO" \ + --name "unity_build_info_${target}_launcher" \ + --dir "$dir" 2>/dev/null; then + REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" + [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" + fi + } + + ROWS="" + parse_info windows64 + if [ -n "$REPLY_URL" ]; then + ROWS+="| Unity Cloud build (Windows) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + elif [ -n "$REPLY_ID" ]; then + ROWS+="| Unity Cloud build (Windows) | #${REPLY_ID} |"$'\n' + fi + parse_info macos + if [ -n "$REPLY_URL" ]; then + ROWS+="| Unity Cloud build (Mac) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + elif [ -n "$REPLY_ID" ]; then + ROWS+="| Unity Cloud build (Mac) | #${REPLY_ID} |"$'\n' + fi + + SECTION="" + if [ -n "$ROWS" ]; then + SECTION="| Name | Link |"$'\n'"| -------- | ----------------------- |"$'\n'"$ROWS" + fi + + { + echo "UCB_SECTION<> "$GITHUB_ENV" + - name: Update build section uses: ./.github/actions/ci-status-comment with: @@ -297,7 +403,9 @@ jobs: section: build github-token: ${{ github.token }} body: |- - ![Build](https://img.shields.io/badge/Build-Failed!-ff0000?logo=unity&logoColor=white&style=for-the-badge) + [![Build](https://img.shields.io/badge/Build-Failed!-ff0000?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) - Build failed! Check the logs to see what went wrong. + Build failed! Check the [logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) to see what went wrong. If the error repeats please consider the `clean-build` tag. + + ${{ env.UCB_SECTION }} diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index b718c28adff..06a50c19766 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -61,6 +61,13 @@ def _extract_member(self, member, targetpath, pwd): build_healthy = True +# Deep link to this build in the Unity Cloud dashboard, captured from the first build +# response that carries one. Persisted to BUILD_LINK_INFO_PATH so the workflow can +# upload it and the PR status comment can link the build directly. +BUILD_LINK_INFO_PATH = 'unity_cloud_build_info.env' +dashboard_url = None +_build_link_info_written = False + parser = argparse.ArgumentParser() parser.add_argument('--resume', help='Resume tracking a running build stored in build_info.json', action='store_true') parser.add_argument('--cancel', help='Cancel a running build stored in build_info.json', action='store_true') @@ -616,6 +623,44 @@ def try_resume_build(): return None +def record_build_link_info(id, response_json): + """Persist the Unity Cloud dashboard deep link for this build (best-effort). + + Build API responses carry dashboard links; the workflow uploads the written file + as an artifact so the PR status comment can link the build id directly instead + of telling humans to search cloud.unity.com by hand. + """ + global dashboard_url, _build_link_info_written + + links = response_json.get('links') or {} + href = None + # dashboard_summary is the build's page and dashboard_log its log tab; + # dashboard_url can be just the dashboard root, so a candidate only + # qualifies when it points at this specific build. + for key in ('dashboard_summary', 'dashboard_log', 'dashboard_url'): + candidate = (links.get(key) or {}).get('href') + if candidate and '/builds/' in candidate: + href = candidate + break + + if _build_link_info_written and not href: + return + + try: + with open(BUILD_LINK_INFO_PATH, 'w') as f: + f.write(f'BUILD_TARGET={os.getenv("TARGET")}\n') + f.write(f'BUILD_ID={id}\n') + if href: + f.write(f'DASHBOARD_URL={href}\n') + except OSError as e: + print(f'Warning: could not write {BUILD_LINK_INFO_PATH}: {e}') + + if href: + dashboard_url = href + print(f'::notice::Unity Cloud build #{id} ({os.getenv("TARGET")}): {href}') + _build_link_info_written = True + + def write_step_summary(target, build_id, final_status, phase_durations, queue_reasons, queue_elapsed, build_elapsed): """Append a phase breakdown to $GITHUB_STEP_SUMMARY (best-effort).""" summary_path = os.environ.get('GITHUB_STEP_SUMMARY') @@ -635,6 +680,8 @@ def fmt(seconds): lines.append('') lines.append(f'- Target: `{target}`') lines.append(f'- Build ID: `{build_id}`') + if dashboard_url: + lines.append(f'- Unity Cloud build page: {dashboard_url}') lines.append(f'- Final outcome: `{final_status}`') if queue_reasons: lines.append(f"- Queue reasons seen: {', '.join(f'`{r}`' for r in sorted(queue_reasons))}") @@ -711,6 +758,9 @@ 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: + record_build_link_info(id, response_json) + queued_reason = response_json.get('queuedReason') if queued_reason and status in QUEUE_STATUSES: queue_reasons.add(queued_reason) @@ -920,7 +970,8 @@ def probe_latest_build(): download_log(id) if not build_healthy: - print(f'Build unhealthy - check the downloaded logs or go to https://cloud.unity.com/ and search for target "{os.getenv('TARGET')}" and build ID "{id}"') + where = dashboard_url or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' + print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') sys.exit(1) # Cleanup (only if build is healthy and not release) From 68875b9f4f7f7809a5d6ed23991fb4e60ddc7de4 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:00:19 +0200 Subject: [PATCH 02/28] ci: address security review findings on Unity Cloud build links Addresses the security review on #9713: - check-build-ran now emits two outputs: player-artifacts (Decentraland_* only) keeps gating the success/skipped split so comment-success never interpolates missing artifact ids, while build-ran (player or unity_build_info_*) widens only the failure path. - The duplicated parse/compose logic moved into a composite action (.github/actions/ucb-build-links) used by both comment jobs, with the validation in one place, unique GITHUB_OUTPUT heredoc delimiters, no empty-label "[#](url)" rows on tampered input, and gh download errors surfaced in the log instead of swallowed. - build.py only accepts absolute https:// dashboard hrefs (matching the consumer regex) and writes the info file immediately after the build id is known, not on the first poll. - unity_build_info_* uploads use retention-days 7; URL charset allows fragments. The org/project-slug exposure in public comments (finding 1) is accepted: the ids grant no access without Unity org membership, and the deep link in the comment is the point of the feature. Co-Authored-By: Claude Fable 5 --- .github/actions/ucb-build-links/action.yml | 93 +++++++++++ .github/workflows/build-unitycloud.yml | 2 + .github/workflows/pr-comment-artifact-url.yml | 151 +++++------------- scripts/cloudbuild/build.py | 9 +- 4 files changed, 141 insertions(+), 114 deletions(-) create mode 100644 .github/actions/ucb-build-links/action.yml diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml new file mode 100644 index 00000000000..e68628c5b46 --- /dev/null +++ b/.github/actions/ucb-build-links/action.yml @@ -0,0 +1,93 @@ +name: Fetch Unity Cloud Build Links +description: >- + Download the unity_build_info_* artifacts of a Unity Cloud Build run and emit + sanitized markdown linking each build id to its Unity Cloud dashboard page: + bare table rows for appending to an existing two-column table, and a standalone + table section for comment bodies that have no table of their own. + +inputs: + run-id: + description: Workflow run id of the Unity Cloud Build run whose artifacts to read. + required: true + github-token: + description: Token used to download the run's artifacts. + required: true + +outputs: + rows: + description: >- + "| Name | Link |"-shaped rows for an existing two-column table; empty when + no valid build info was found. + value: ${{ steps.fetch.outputs.rows }} + section: + description: >- + Standalone table (header + rows); empty when no valid build info was found. + value: ${{ steps.fetch.outputs.section }} + +runs: + using: composite + steps: + - name: Download and sanitize Unity Cloud build info + id: fetch + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + RUN_ID: ${{ inputs.run-id }} + REPO_FULL: ${{ github.repository }} + run: | + set -euo pipefail + + # The info files come out of the PR-controlled build workflow, so treat them as + # untrusted input: accept only a numeric build id and a Unity dashboard URL with + # a conservative charset before letting them anywhere near a comment body. + URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&#-]*$' + parse_info() { + local target="$1" + local dir="ucb_info_${target}" + REPLY_ID="" + REPLY_URL="" + if gh run download "$RUN_ID" \ + --repo "$REPO_FULL" \ + --name "unity_build_info_${target}_launcher" \ + --dir "$dir" 2>"${dir}.err"; then + REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" + [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" + else + # Absence is normal for runs predating the info artifact; still surface the + # gh error so an auth/permission regression doesn't silently eat the rows. + echo "note: could not fetch unity_build_info_${target}_launcher: $(tr '\n' ' ' < "${dir}.err")" + fi + } + + ROWS="" + for entry in "windows64:Windows" "macos:Mac"; do + target="${entry%%:*}" + label="${entry#*:}" + parse_info "$target" + # A URL without a valid id only occurs on a tampered artifact — drop the row + # rather than render an empty "[#](...)" label. + if [ -n "$REPLY_ID" ] && [ -n "$REPLY_URL" ]; then + ROWS+="| Unity Cloud build (${label}) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + elif [ -n "$REPLY_ID" ]; then + ROWS+="| Unity Cloud build (${label}) | #${REPLY_ID} |"$'\n' + fi + done + + SECTION="" + if [ -n "$ROWS" ]; then + SECTION="| Name | Link |"$'\n'"| -------- | ----------------------- |"$'\n'"$ROWS" + fi + + # The payload derives from artifact bytes, so the heredoc delimiter must not be + # guessable content even though the validation above already forbids newlines. + DELIM="UCB_EOF_${RANDOM}${RANDOM}_$$" + { + echo "rows<<${DELIM}" + printf '%s' "$ROWS" + echo "${DELIM}" + echo "section<<${DELIM}" + printf '%s' "$SECTION" + echo "${DELIM}" + } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/build-unitycloud.yml b/.github/workflows/build-unitycloud.yml index 764f1a7c0bb..5630e6f7dfe 100644 --- a/.github/workflows/build-unitycloud.yml +++ b/.github/workflows/build-unitycloud.yml @@ -986,6 +986,8 @@ jobs: name: unity_build_info_${{ matrix.target }}_${{ needs.prebuild.outputs.install_source }} path: unity_cloud_build_info.env if-no-files-found: error + # Only consumed by the immediately-following PR status comment run. + retention-days: 7 - name: Print cloud logs if: ${{ always() && hashFiles('unity_cloud_log.log') != '' }} diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index ce45ae97ee2..1691cc1ef91 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -71,6 +71,7 @@ jobs: runs-on: ubuntu-latest outputs: build-ran: ${{ steps.check.outputs.build-ran }} + player-artifacts: ${{ steps.check.outputs.player-artifacts }} steps: - name: Check if build jobs actually ran id: check @@ -80,14 +81,24 @@ jobs: REPO: ${{ github.event.repository.name }} RUN_ID: ${{ github.event.workflow_run.id }} run: | - # Check if any build artifacts exist (they only exist when Build jobs ran). - # unity_build_info_* is uploaded as soon as the Unity-side build starts, so a - # build that failed before producing player artifacts still counts as "ran" - # and gets a failure comment with a Unity Cloud link instead of staying pending. - ARTIFACT_COUNT=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts" \ - --jq '[.artifacts[] | select((.name | startswith("Decentraland_")) or (.name | startswith("unity_build_info_")))] | length') - echo "Build artifact count: $ARTIFACT_COUNT" - if [ "$ARTIFACT_COUNT" -gt 0 ]; then + # player-artifacts: Decentraland_* zips exist. comment-success interpolates + # their artifact ids into download URLs, so the success/skipped split must + # keep gating on this and only this. + # build-ran: any evidence a Unity-side build started. unity_build_info_* is + # uploaded as soon as the build id is known, so a build that failed before + # producing player artifacts still posts a failure comment (with the Unity + # Cloud link) instead of leaving the comment stuck on "Pending". + NAMES=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts" \ + --jq '[.artifacts[].name]') + PLAYER_COUNT=$(jq 'map(select(startswith("Decentraland_"))) | length' <<< "$NAMES") + INFO_COUNT=$(jq 'map(select(startswith("unity_build_info_"))) | length' <<< "$NAMES") + echo "Player artifact count: $PLAYER_COUNT; build info artifact count: $INFO_COUNT" + if [ "$PLAYER_COUNT" -gt 0 ]; then + echo "player-artifacts=true" >> "$GITHUB_OUTPUT" + else + echo "player-artifacts=false" >> "$GITHUB_OUTPUT" + fi + if [ "$PLAYER_COUNT" -gt 0 ] || [ "$INFO_COUNT" -gt 0 ]; then echo "build-ran=true" >> "$GITHUB_OUTPUT" else echo "build-ran=false" >> "$GITHUB_OUTPUT" @@ -95,7 +106,7 @@ jobs: comment-skipped: needs: [pre-validation, check-build-ran] - if: github.event.workflow_run.conclusion == 'success' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.build-ran == 'false' + if: github.event.workflow_run.conclusion == 'success' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.player-artifacts == 'false' runs-on: ubuntu-latest steps: - name: Checkout CI status action @@ -118,13 +129,15 @@ jobs: 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.build-ran == 'true' + if: github.event.workflow_run.conclusion == 'success' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.player-artifacts == 'true' runs-on: ubuntu-latest steps: - name: Checkout CI status action uses: actions/checkout@v6 with: - sparse-checkout: .github/actions/ci-status-comment + sparse-checkout: | + .github/actions/ci-status-comment + .github/actions/ucb-build-links sparse-checkout-cone-mode: false persist-credentials: false @@ -234,52 +247,11 @@ jobs: fi - name: Fetch Unity Cloud build links - env: - GITHUB_TOKEN: ${{ github.token }} - OWNER: ${{ github.repository_owner }} - REPO: ${{ github.event.repository.name }} - run: | - set -euo pipefail - - # The info files come out of the PR-controlled build workflow, so treat them as - # untrusted input: accept only a numeric build id and a Unity dashboard URL with - # a conservative charset before letting them anywhere near the comment body. - URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&-]*$' - parse_info() { - local target="$1" - local dir="ucb_info_${target}" - REPLY_ID="" - REPLY_URL="" - if gh run download "$PREVIOUS_JOB_ID" \ - --repo "$OWNER/$REPO" \ - --name "unity_build_info_${target}_launcher" \ - --dir "$dir" 2>/dev/null; then - REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) - REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) - [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" - [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" - fi - } - - ROWS="" - parse_info windows64 - if [ -n "$REPLY_URL" ]; then - ROWS+="| Unity Cloud build (Windows) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' - elif [ -n "$REPLY_ID" ]; then - ROWS+="| Unity Cloud build (Windows) | #${REPLY_ID} |"$'\n' - fi - parse_info macos - if [ -n "$REPLY_URL" ]; then - ROWS+="| Unity Cloud build (Mac) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' - elif [ -n "$REPLY_ID" ]; then - ROWS+="| Unity Cloud build (Mac) | #${REPLY_ID} |"$'\n' - fi - - { - echo "UCB_ROWS<> "$GITHUB_ENV" + id: ucb + uses: ./.github/actions/ucb-build-links + with: + run-id: ${{ env.PREVIOUS_JOB_ID }} + github-token: ${{ github.token }} - name: Update build section uses: ./.github/actions/ci-status-comment @@ -301,7 +273,7 @@ jobs: | Download Mac | ${{ github.server_url }}/${{ github.repository }}/suites/${{ env.SUITE_ID }}/artifacts/${{ env.MAC_ARTIFACT_ID }} | | Download Mac S3 | ${{ format('{0}/{1}/Decentraland_macos.zip', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} | | Built on | ${{ env.BUILD_DATE }} | - ${{ env.UCB_ROWS }} + ${{ steps.ucb.outputs.rows }} ${{ env.SIZE_REPORT }} @@ -338,63 +310,18 @@ jobs: - name: Checkout CI status action uses: actions/checkout@v6 with: - sparse-checkout: .github/actions/ci-status-comment + sparse-checkout: | + .github/actions/ci-status-comment + .github/actions/ucb-build-links sparse-checkout-cone-mode: false persist-credentials: false - name: Fetch Unity Cloud build links - env: - GITHUB_TOKEN: ${{ github.token }} - OWNER: ${{ github.repository_owner }} - REPO: ${{ github.event.repository.name }} - RUN_ID: ${{ github.event.workflow_run.id }} - run: | - set -euo pipefail - - # The info files come out of the PR-controlled build workflow, so treat them as - # untrusted input: accept only a numeric build id and a Unity dashboard URL with - # a conservative charset before letting them anywhere near the comment body. - URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&-]*$' - parse_info() { - local target="$1" - local dir="ucb_info_${target}" - REPLY_ID="" - REPLY_URL="" - if gh run download "$RUN_ID" \ - --repo "$OWNER/$REPO" \ - --name "unity_build_info_${target}_launcher" \ - --dir "$dir" 2>/dev/null; then - REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) - REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) - [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" - [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" - fi - } - - ROWS="" - parse_info windows64 - if [ -n "$REPLY_URL" ]; then - ROWS+="| Unity Cloud build (Windows) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' - elif [ -n "$REPLY_ID" ]; then - ROWS+="| Unity Cloud build (Windows) | #${REPLY_ID} |"$'\n' - fi - parse_info macos - if [ -n "$REPLY_URL" ]; then - ROWS+="| Unity Cloud build (Mac) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' - elif [ -n "$REPLY_ID" ]; then - ROWS+="| Unity Cloud build (Mac) | #${REPLY_ID} |"$'\n' - fi - - SECTION="" - if [ -n "$ROWS" ]; then - SECTION="| Name | Link |"$'\n'"| -------- | ----------------------- |"$'\n'"$ROWS" - fi - - { - echo "UCB_SECTION<> "$GITHUB_ENV" + id: ucb + uses: ./.github/actions/ucb-build-links + with: + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} - name: Update build section uses: ./.github/actions/ci-status-comment @@ -408,4 +335,4 @@ jobs: Build failed! Check the [logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) to see what went wrong. If the error repeats please consider the `clean-build` tag. - ${{ env.UCB_SECTION }} + ${{ steps.ucb.outputs.section }} diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 06a50c19766..b62cb2509d6 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -636,10 +636,11 @@ def record_build_link_info(id, response_json): href = None # dashboard_summary is the build's page and dashboard_log its log tab; # dashboard_url can be just the dashboard root, so a candidate only - # qualifies when it points at this specific build. + # qualifies when it is an absolute link to this specific build (the + # comment workflow rejects anything else, so don't persist it either). for key in ('dashboard_summary', 'dashboard_log', 'dashboard_url'): candidate = (links.get(key) or {}).get('href') - if candidate and '/builds/' in candidate: + if candidate and candidate.startswith('https://') and '/builds/' in candidate: href = candidate break @@ -902,6 +903,10 @@ def get_clean_build_bool(): utils.persist_build_info(os.getenv('TARGET'), None) id = run_build(os.getenv('BRANCH_NAME'), get_clean_build_bool()) utils.persist_build_info(os.getenv('TARGET'), id) + # Write the link info file (target + id, no URL yet) immediately so it exists + # even if the runner dies before the first poll; the poll loop upgrades it + # with the dashboard URL once a response carries one. + record_build_link_info(id, {}) print(f'For more info and live logs, go to https://cloud.unity.com/ and search for target "{os.getenv('TARGET')}" and build ID "{id}"') final_outcome, phase_durations, queue_reasons, queue_elapsed, build_elapsed = run_poll_loop( From 26cb13e447e99a49dd5f02150f6672bc6f843900 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:04:30 +0200 Subject: [PATCH 03/28] ci: turn the CI status comment into a link hub (jobs, reports, timings, automation) Extends the unified CI status comment into a navigation hub: - Build rows now pair each target's Unity Cloud build page with its GitHub job log ("Windows build | Unity Cloud #id . GitHub job"). - Tests section: badge links the Unity Test run (where the dorny report lives), each suite links its job, a Time column shows suite duration, a collapsible "Slowest tests" top-10 is parsed from the NUnit XML, and a footer links the Test results artifacts (XML + editor logs). The extractor in test.yml now records duration and slowest tests; the trusted composer type-checks both before rendering (numeric seconds, single-line names). - Lint section: badge links the lint run; footer links the run and the csharp-lint-reports artifact (the inline findings list is capped). - New "automation" section in the status comment: defaults to an on-demand hint for /visual-tests, flips to Running when the suite dispatches, and lands on Passed/Failed with the Allure report + run links. The reusable workflow's own detailed comment is unchanged. - ci-status-comment now appends a missing section fence to existing comments instead of resetting the whole comment to the skeleton (which would have wiped the other sections' state when the automation section first writes). Co-Authored-By: Claude Fable 5 --- .github/actions/ci-status-comment/action.yml | 9 +- .../ci-status-comment/upsert-ci-status.sh | 31 ++++--- .github/actions/ucb-build-links/action.yml | 23 ++++- .../workflows/pr-comment-test-failures.yml | 62 +++++++++++-- .github/workflows/pr-comment-warnings.yml | 23 ++++- .github/workflows/test.yml | 13 +++ .github/workflows/visual-regression.yml | 91 +++++++++++++++++++ 7 files changed, 220 insertions(+), 32 deletions(-) diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 22f41506f1e..6f8e9875c4a 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.github/actions/ci-status-comment/action.yml @@ -1,16 +1,17 @@ name: Upsert CI Status Comment description: >- Create or update the single unified CI status comment on a PR, replacing only - the given section (build | lint | tests). Seeds a skeleton with all three - sections the first time it runs, and re-reads/retries so concurrent writers - (build vs. Unity Test) never clobber each other's section. + the given section (build | lint | tests | automation). Seeds a skeleton with + every section the first time it runs, appends a missing section fence to + older comments, and re-reads/retries so concurrent writers (build vs. Unity + Test) never clobber each other's section. inputs: pr-number: description: Pull request number to comment on. required: true section: - description: Which section to replace — one of build, lint, tests. + description: Which section to replace — one of build, lint, tests, automation. required: true body: description: Markdown for this section (inline badge + message). Rendered as-is between the section markers. diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 933e1b36209..8326a61b257 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -1,17 +1,18 @@ #!/usr/bin/env bash # Create or update the single unified CI status comment on a PR, replacing only -# one section (build | lint | tests). All three CI comment workflows call this -# through the ci-status-comment composite action, so the three separate bot +# one section (build | lint | tests | automation). All CI comment workflows call +# this through the ci-status-comment composite action, so the separate bot # comments collapse into one. # -# The comment is keyed by the hidden marker and holds three -# sections, each fenced by its own start/end markers: +# The comment is keyed by the hidden marker and holds one +# fenced block per section: # # # ### 🚦 CI Status -# …build… -# …lint… -# …tests… +# …build… +# …lint… +# …tests… +# …automation… # # Build and Unity Test run as independent workflows whose comment writers can # fire at the same time, so a plain read-modify-write would drop a section or @@ -33,6 +34,7 @@ section_default() { build) printf '![Build](https://img.shields.io/badge/Build-Waiting-lightgrey?logo=unity&logoColor=white&style=for-the-badge)\n\n_Waiting for the build to start…_' ;; lint) printf '![Lint](https://img.shields.io/badge/Lint-Waiting-lightgrey?logo=jetbrains&logoColor=white&style=for-the-badge)\n\n_Waiting for lint to start…_' ;; tests) printf '![Tests](https://img.shields.io/badge/Tests-Waiting-lightgrey?logo=codecov&logoColor=white&style=for-the-badge)\n\n_Waiting for tests to start…_' ;; + automation) printf '![Automation](https://img.shields.io/badge/Automation-On%%20demand-lightgrey?logo=github&logoColor=white&style=for-the-badge)\n\n_On demand — comment `/visual-tests` on this PR to run the visual regression suite against its build._' ;; esac } @@ -41,11 +43,12 @@ wrap_section() { printf '\n%s\n' "$1" "$2" # A fresh comment with every section defaulted to "waiting". skeleton() { - printf '%s\n%s\n\n%s\n\n%s\n\n%s\n' \ + printf '%s\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n' \ "$MARKER" "$HEADER" \ "$(wrap_section build "$(section_default build)")" \ "$(wrap_section lint "$(section_default lint)")" \ - "$(wrap_section tests "$(section_default tests)")" + "$(wrap_section tests "$(section_default tests)")" \ + "$(wrap_section automation "$(section_default automation)")" } # Emit the section body for this run to a file so awk can splice it verbatim, @@ -108,10 +111,14 @@ for attempt in 1 2 3 4 5; do CURRENT_BODY="" fi - # No unified comment yet, or one missing our section markers: start clean so - # all three sections are always present. - if [ -z "$CURRENT_BODY" ] || ! grep -qF "$START" <<< "$CURRENT_BODY"; then + # No unified comment yet: start from the full skeleton. A comment that exists + # but lacks our markers predates this section (e.g. it was written before the + # automation section existed) — append an empty fence for just our section + # instead of resetting the whole comment and wiping the other sections' state. + if [ -z "$CURRENT_BODY" ]; then CURRENT_BODY="$(skeleton)" + elif ! grep -qF "$START" <<< "$CURRENT_BODY"; then + CURRENT_BODY="$CURRENT_BODY"$'\n\n'"$(wrap_section "$SECTION" "$(section_default "$SECTION")")" fi NEW_BODY="$(replace_section "$CURRENT_BODY")" diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index e68628c5b46..e2b36d86fb1 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -16,7 +16,8 @@ inputs: outputs: rows: description: >- - "| Name | Link |"-shaped rows for an existing two-column table; empty when + "| Name | Link |"-shaped rows for an existing two-column table, pairing + each target's Unity Cloud build page with its GitHub job log; empty when no valid build info was found. value: ${{ steps.fetch.outputs.rows }} section: @@ -61,17 +62,31 @@ runs: fi } + # Per-target GitHub job pages, from the trusted Actions API (jobs of the + # matrix job "Build ()"), so each row pairs the Unity Cloud build + # page with the GitHub-side job log. + JOBS_JSON=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/jobs?per_page=100" 2>/dev/null || echo '{"jobs":[]}') + ROWS="" for entry in "windows64:Windows" "macos:Mac"; do target="${entry%%:*}" label="${entry#*:}" parse_info "$target" - # A URL without a valid id only occurs on a tampered artifact — drop the row + job_url=$(jq -r --arg n "Build ($target)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1) + + cell="" + # A URL without a valid id only occurs on a tampered artifact — drop the link # rather than render an empty "[#](...)" label. if [ -n "$REPLY_ID" ] && [ -n "$REPLY_URL" ]; then - ROWS+="| Unity Cloud build (${label}) | [#${REPLY_ID}](${REPLY_URL}) |"$'\n' + cell="[Unity Cloud #${REPLY_ID}](${REPLY_URL})" elif [ -n "$REPLY_ID" ]; then - ROWS+="| Unity Cloud build (${label}) | #${REPLY_ID} |"$'\n' + cell="Unity Cloud #${REPLY_ID}" + fi + if [ -n "$cell" ] && [ -n "$job_url" ]; then + cell="${cell} · [GitHub job](${job_url})" + fi + if [ -n "$cell" ]; then + ROWS+="| ${label} build | ${cell} |"$'\n' fi done diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index e8dfb4cb94b..40a6d20231a 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -72,9 +72,15 @@ jobs: run: | set -euo pipefail - # Job list of the originating "Unity Test" run, used to deep-link a - # crashed/timed-out suite straight to its job page in the warning line. - JOBS_JSON=$(gh api "/repos/$REPO/actions/runs/$WORKFLOW_RUN_ID/jobs" 2>/dev/null || echo '{"jobs":[]}') + # Job and artifact lists of the originating "Unity Test" run, used to + # deep-link each suite to its job page and the result artifacts + # (NUnit XML + editor logs) to their download links. + JOBS_JSON=$(gh api "/repos/$REPO/actions/runs/$WORKFLOW_RUN_ID/jobs?per_page=100" 2>/dev/null || echo '{"jobs":[]}') + ARTS_JSON=$(gh api "/repos/$REPO/actions/runs/$WORKFLOW_RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') + + fmt_secs() { + awk -v s="$1" 'BEGIN { s=int(s+0.5); m=int(s/60); r=s%60; if (m>0) printf("%dm %02ds", m, r); else printf("%ds", r) }' + } declare -A DISPLAY=( [editmode]=EditMode [playmode]=PlayMode ) @@ -82,6 +88,7 @@ jobs: rows="" warnings="" failed_list="" + slowest_list="" total_failed=0 for mode in editmode playmode; do @@ -89,13 +96,15 @@ jobs: [ -f "$file" ] || continue disp=${DISPLAY[$mode]} + job_url=$(jq -r --arg n "Test ($mode)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1) + if [ -n "$job_url" ]; then disp_cell="[$disp]($job_url)"; else disp_cell="$disp"; fi + # A suite that produced no result XML crashed or timed out before finishing. # Surface it as its own state instead of silently contributing 0 to a green total. if [ "$(jq -r '.hasResults' "$file")" != "true" ]; then status=incomplete - job_url=$(jq -r --arg n "Test ($mode)" '.jobs[]? | select(.name==$n) | .html_url' <<< "$JOBS_JSON" | head -1) [ -n "$job_url" ] || job_url="$WORKFLOW_RUN_URL" - rows="$rows| $disp | ⚠️ No results | — | — | — |"$'\n' + rows="$rows| $disp_cell | ⚠️ No results | — | — | — | — |"$'\n' warnings="$warnings⚠️ **$disp** produced no results — the run likely crashed or timed out before finishing. Check the [\`Unity Test / Test ($mode)\`]($job_url) job."$'\n\n' continue fi @@ -107,14 +116,24 @@ jobs: f=$(jq -r '.failed | length' "$file") s=$((t - p - f)); if [ "$s" -lt 0 ]; then s=0; fi + # Suite duration + slowest tests come from the same untrusted artifact: + # duration must be numeric before it reaches awk, and slowest entries are + # type-checked in jq with names flattened to a single line. + d=$(jq -r '.duration // empty' "$file") + if [[ "$d" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then dur=$(fmt_secs "$d"); else dur="—"; fi + slow=$(jq -r --arg mode "$mode" \ + '.slowest[]? | select((.seconds|type=="number") and (.name|type=="string")) | "- [\($mode)] \(.seconds)s \(.name | gsub("[\r\n]"; " "))"' \ + "$file" 2>/dev/null || true) + [ -n "$slow" ] && slowest_list="$slowest_list$slow"$'\n' + if [ "$f" -gt 0 ]; then if [ "$status" = "passed" ]; then status=failed; fi total_failed=$((total_failed + f)) - rows="$rows| $disp | ❌ $f failed | $p | $f | $s |"$'\n' + rows="$rows| $disp_cell | ❌ $f failed | $p | $f | $s | $dur |"$'\n' names=$(jq -r --arg mode "$mode" '.failed[] | "- [\($mode)] \(. | gsub("[\r\n]"; " "))"' "$file") failed_list="$failed_list$names"$'\n' else - rows="$rows| $disp | ✅ Passed | $p | 0 | $s |"$'\n' + rows="$rows| $disp_cell | ✅ Passed | $p | 0 | $s | $dur |"$'\n' fi done @@ -124,15 +143,24 @@ jobs: *) badge="https://img.shields.io/badge/Tests-Passed!-3fb950?logo=codecov&logoColor=white&style=for-the-badge"; headline="All Unity tests passed ✅" ;; esac + # Artifact download links (ids come from the trusted Actions API). + art_footer="" + for entry in "editmode:Test results (editmode)" "playmode:Test results (playmode)"; do + mode="${entry%%:*}" + art_name="${entry#*:}" + art_id=$(jq -r --arg n "$art_name" '.artifacts[]? | select(.name==$n) | .id // empty' <<< "$ARTS_JSON" | head -1) + [ -n "$art_id" ] && art_footer="$art_footer · [$mode](https://github.com/$REPO/actions/runs/$WORKFLOW_RUN_ID/artifacts/$art_id)" + done + DELIM="EOF_$(uuidgen)" { echo "body<<$DELIM" - echo "![Tests]($badge)" + echo "[![Tests]($badge)]($WORKFLOW_RUN_URL)" echo "" printf '%s\n' "$headline" echo "" - echo "| TESTS SUITE | Result | Passed | Failed | Skipped |" - echo "| ----------- | ------ | -----: | -----: | ------: |" + echo "| TESTS SUITE | Result | Passed | Failed | Skipped | Time |" + echo "| ----------- | ------ | -----: | -----: | ------: | ---: |" printf '%s' "$rows" if [ "$total_failed" -gt 0 ]; then echo "" @@ -142,6 +170,20 @@ jobs: echo "" echo "" fi + if [ -n "$slowest_list" ]; then + echo "" + echo "
Slowest tests" + echo "" + printf '%s' "$slowest_list" + echo "" + echo "
" + fi + echo "" + if [ -n "$art_footer" ]; then + echo "Full report: [run summary]($WORKFLOW_RUN_URL) · results + editor logs:${art_footer#" ·"}" + else + echo "Full report: [run summary]($WORKFLOW_RUN_URL)" + fi echo "$DELIM" } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/pr-comment-warnings.yml b/.github/workflows/pr-comment-warnings.yml index d55336245b1..15aa355c05c 100644 --- a/.github/workflows/pr-comment-warnings.yml +++ b/.github/workflows/pr-comment-warnings.yml @@ -62,7 +62,7 @@ jobs: section: lint github-token: ${{ github.token }} body: |- - ![Lint](https://img.shields.io/badge/Lint-Pending!-ffff00?logo=jetbrains&logoColor=white&style=for-the-badge) + [![Lint](https://img.shields.io/badge/Lint-Pending!-ffff00?logo=jetbrains&logoColor=white&style=for-the-badge)](${{ github.event.workflow_run.html_url }}) Lint in progress, come back later! @@ -123,6 +123,9 @@ jobs: ALLOW_EQUAL: ${{ steps.result.outputs.allow-equal }} CONCLUSION: ${{ github.event.workflow_run.conclusion }} RUN_URL: ${{ github.event.workflow_run.html_url }} + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + RUN_ID: ${{ github.event.workflow_run.id }} run: | DELIM="EOF_$(uuidgen)" BADGE_STYLE="?logo=jetbrains&logoColor=white&style=for-the-badge" @@ -205,12 +208,28 @@ jobs: fi fi + # Footer: the lint job run plus the full InspectCode report artifact (the + # findings list above is capped; the artifact has everything). The id comes + # from the trusted Actions API. + FOOTER="" + if [ "$FOUND" = "true" ]; then + ART_ID=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" \ + --jq '.artifacts[] | select(.name=="csharp-lint-reports") | .id' 2>/dev/null | head -1) + FOOTER="[Lint run]($RUN_URL)" + [ -n "$ART_ID" ] && FOOTER="$FOOTER · [full InspectCode report](https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$ART_ID)" + FOOTER="$FOOTER" + fi + { echo "body<<$DELIM" - echo "![Lint]($BADGE)" + echo "[![Lint]($BADGE)]($RUN_URL)" echo "" echo "$MSG" [ -n "$DETAILS" ] && cat "$DETAILS" + if [ -n "$FOOTER" ]; then + echo "" + echo "$FOOTER" + fi echo "$DELIM" } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ebe112a57cf..299dcbaa093 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -814,6 +814,8 @@ jobs: total = 0 passed = 0 failed = [] + duration = 0.0 + timings = [] for path in xml_files: try: @@ -825,6 +827,12 @@ jobs: if case_result is None: continue total += 1 + try: + seconds = float(test_case.get("duration") or 0) + except ValueError: + seconds = 0.0 + duration += seconds + timings.append((seconds, test_case.get("fullname") or test_case.get("name"))) if case_result == "Passed": passed += 1 elif case_result == "Failed": @@ -835,6 +843,11 @@ jobs: "total": total, "passed": passed, "failed": sorted(set(failed)), + "duration": round(duration, 1), + "slowest": [ + {"name": name, "seconds": round(seconds, 1)} + for seconds, name in sorted(timings, key=lambda t: -t[0])[:10] + ], } with open(f"failed-tests-{test_mode}.json", "w") as f: diff --git a/.github/workflows/visual-regression.yml b/.github/workflows/visual-regression.yml index 59aebedbf46..f2c9b8b8c8e 100644 --- a/.github/workflows/visual-regression.yml +++ b/.github/workflows/visual-regression.yml @@ -163,6 +163,33 @@ jobs: echo "::notice::No matching explorer-automation branch '${HEAD_REF}' — using default." fi + # Flip the unified CI status comment's automation section to "running" the + # moment the suite is dispatched, so the on-demand placeholder never lingers + # while a run is in flight. + automation-pending: + name: Mark automation running + needs: resolve + if: needs.resolve.outputs.authorized == 'true' && needs.resolve.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: Set automation section to running + uses: ./.github/actions/ci-status-comment + with: + pr-number: ${{ needs.resolve.outputs.pr_number }} + section: automation + github-token: ${{ github.token }} + body: |- + [![Automation](https://img.shields.io/badge/Automation-Running!-ffff00?logo=github&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) + + Visual regression suite running for commit `${{ needs.resolve.outputs.head_short_sha }}` — [watch the run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}). + run-suite: name: Run visual suite needs: resolve @@ -179,3 +206,67 @@ jobs: commit_sha: ${{ needs.resolve.outputs.head_short_sha }} branch_label: ${{ needs.resolve.outputs.head_ref }} secrets: inherit + + # Final state of the automation section: badge + Allure report + run links. + # The reusable workflow still posts its own detailed per-platform comment; + # this section is the at-a-glance summary inside the unified CI status comment. + report: + name: Update CI status comment + needs: [resolve, run-suite] + if: always() && needs.resolve.outputs.authorized == 'true' && needs.resolve.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: Compose automation section + id: compose + env: + RESULT: ${{ needs.run-suite.result }} + PR_NUMBER: ${{ needs.resolve.outputs.pr_number }} + COMMIT_SHA: ${{ needs.resolve.outputs.head_short_sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + PUBLIC_URL_PREFIX: ${{ vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + BADGE_STYLE="?logo=github&logoColor=white&style=for-the-badge" + case "$RESULT" in + success) BADGE="https://img.shields.io/badge/Automation-Passed!-3fb950${BADGE_STYLE}"; MSG="✅ Visual regression suite passed." ;; + failure) BADGE="https://img.shields.io/badge/Automation-Failed!-ff0000${BADGE_STYLE}"; MSG="❌ Visual regression suite failed." ;; + *) BADGE="https://img.shields.io/badge/Automation-Cancelled-lightgrey${BADGE_STYLE}"; MSG="Visual regression suite did not finish (\`$RESULT\`)." ;; + esac + + # Mirrors run-visual-suite.yml's "Compute identifiers" S3 path. mode=test + # and platform=macos are the reusable workflow's defaults — this dispatcher + # 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" + + DELIM="EOF_${RANDOM}${RANDOM}_$$" + { + echo "body<<$DELIM" + echo "[![Automation]($BADGE)]($RUN_URL)" + echo "" + echo "$MSG" + echo "" + echo "| Name | Link |" + echo "| -------- | ----------------------- |" + echo "| Commit | \`$COMMIT_SHA\` |" + echo "| Allure report | $REPORT_URL |" + echo "| Workflow run | $RUN_URL |" + echo "" + echo "Triggered via \`/visual-tests\` · the detailed per-platform comment is posted separately." + echo "$DELIM" + } >> "$GITHUB_OUTPUT" + + - name: Update automation section + uses: ./.github/actions/ci-status-comment + with: + pr-number: ${{ needs.resolve.outputs.pr_number }} + section: automation + github-token: ${{ github.token }} + body: ${{ steps.compose.outputs.body }} From 8ee55d7de53cefaef6e06d9cc3776227583bd01c Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:31:59 +0200 Subject: [PATCH 04/28] ci: add a performance section to the CI status comment Adds a fifth "performance" section to the unified CI status comment, covering both perf lanes: - Bare-metal benchmark (decentraland/performance-testing): when comment-success dispatches it after a successful build, the section flips to "Dispatched" linking the target workflow's run queue; the benchmark's own perf-test-summary comment (which links its run) remains the detailed result, as repository_dispatch returns no run id to link directly. - In-repo Unity Performance Test (perf_test label): new companion pr-comment-perf.yml (workflow_run, trusted context) writes Passed/Failed with links to the run summary (which renders the benchmark report) and the JSON results + PDF report artifacts. The workflow fires on every PR event but its job gates on the label, so the companion checks the job actually ran (jobs API) before touching the section - a skipped run must not overwrite the bare-metal dispatch status. - Section default documents both lanes, including that perf_test skips normal CI and blocks merge while set. Co-Authored-By: Claude Fable 5 --- .github/actions/ci-status-comment/action.yml | 10 +- .../ci-status-comment/upsert-ci-status.sh | 19 +-- .github/workflows/pr-comment-artifact-url.yml | 16 +++ .github/workflows/pr-comment-perf.yml | 120 ++++++++++++++++++ 4 files changed, 152 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/pr-comment-perf.yml diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 6f8e9875c4a..2e07f30f837 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.github/actions/ci-status-comment/action.yml @@ -1,17 +1,17 @@ name: Upsert CI Status Comment description: >- Create or update the single unified CI status comment on a PR, replacing only - the given section (build | lint | tests | automation). Seeds a skeleton with - every section the first time it runs, appends a missing section fence to - older comments, and re-reads/retries so concurrent writers (build vs. Unity - Test) never clobber each other's section. + the given section (build | lint | tests | performance | automation). Seeds a + skeleton with every section the first time it runs, appends a missing section + fence to older comments, and re-reads/retries so concurrent writers (build + vs. Unity Test) never clobber each other's section. inputs: pr-number: description: Pull request number to comment on. required: true section: - description: Which section to replace — one of build, lint, tests, automation. + description: Which section to replace — one of build, lint, tests, performance, automation. required: true body: description: Markdown for this section (inline badge + message). Rendered as-is between the section markers. diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 8326a61b257..b7baeefbece 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -1,18 +1,19 @@ #!/usr/bin/env bash # Create or update the single unified CI status comment on a PR, replacing only -# one section (build | lint | tests | automation). All CI comment workflows call -# this through the ci-status-comment composite action, so the separate bot -# comments collapse into one. +# one section (build | lint | tests | performance | automation). All CI comment +# workflows call this through the ci-status-comment composite action, so the +# separate bot comments collapse into one. # # The comment is keyed by the hidden marker and holds one # fenced block per section: # # # ### 🚦 CI Status -# …build… -# …lint… -# …tests… -# …automation… +# …build… +# …lint… +# …tests… +# …performance… +# …automation… # # Build and Unity Test run as independent workflows whose comment writers can # fire at the same time, so a plain read-modify-write would drop a section or @@ -35,6 +36,7 @@ section_default() { lint) printf '![Lint](https://img.shields.io/badge/Lint-Waiting-lightgrey?logo=jetbrains&logoColor=white&style=for-the-badge)\n\n_Waiting for lint to start…_' ;; tests) printf '![Tests](https://img.shields.io/badge/Tests-Waiting-lightgrey?logo=codecov&logoColor=white&style=for-the-badge)\n\n_Waiting for tests to start…_' ;; automation) printf '![Automation](https://img.shields.io/badge/Automation-On%%20demand-lightgrey?logo=github&logoColor=white&style=for-the-badge)\n\n_On demand — comment `/visual-tests` on this PR to run the visual regression suite against its build._' ;; + performance) printf '![Performance](https://img.shields.io/badge/Performance-Waiting-lightgrey?logo=speedtest&logoColor=white&style=for-the-badge)\n\n_Bare-metal benchmarks run automatically after each successful build; results arrive as a separate comment. Add the `perf_test` label to run the in-repo Unity performance suite instead (skips normal CI and blocks merge while set)._' ;; esac } @@ -43,11 +45,12 @@ wrap_section() { printf '\n%s\n' "$1" "$2" # A fresh comment with every section defaulted to "waiting". skeleton() { - printf '%s\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n' \ + printf '%s\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n' \ "$MARKER" "$HEADER" \ "$(wrap_section build "$(section_default build)")" \ "$(wrap_section lint "$(section_default lint)")" \ "$(wrap_section tests "$(section_default tests)")" \ + "$(wrap_section performance "$(section_default performance)")" \ "$(wrap_section automation "$(section_default automation)")" } diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 1691cc1ef91..b15011dff46 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -283,6 +283,7 @@ jobs: run: gh release view -R $GITHUB_REPOSITORY --json tagName --template "RELEASE_TAG={{.tagName}}" >> $GITHUB_ENV - name: Trigger performance test + id: perf_dispatch uses: peter-evans/repository-dispatch@v4 with: repository: decentraland/performance-testing @@ -302,6 +303,21 @@ jobs: "mac_base_build_url": "${{ vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL }}/@dcl/unity-explorer/releases/${{ env.RELEASE_TAG }}/Decentraland_macos.zip" } + # repository_dispatch is fire-and-forget (no run id comes back), so this + # links the target workflow's run list; the benchmark itself posts a + # separate perf-test-summary comment with its run link when it finishes. + - name: Mark performance section as dispatched + if: steps.perf_dispatch.outcome == 'success' + uses: ./.github/actions/ci-status-comment + with: + pr-number: ${{ needs.pre-validation.outputs.pr-number }} + section: performance + github-token: ${{ github.token }} + body: |- + [![Performance](https://img.shields.io/badge/Performance-Dispatched!-ffff00?logo=speedtest&logoColor=white&style=for-the-badge)](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml) + + 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. + comment-failed: needs: [pre-validation, check-build-ran] if: github.event.workflow_run.conclusion == 'failure' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.build-ran == 'true' diff --git a/.github/workflows/pr-comment-perf.yml b/.github/workflows/pr-comment-perf.yml new file mode 100644 index 00000000000..b0f1af69122 --- /dev/null +++ b/.github/workflows/pr-comment-perf.yml @@ -0,0 +1,120 @@ +# pr-comment-perf.yml +--- +name: Comment Performance Results on PR + +# Runs in the trusted base-repo context after the (possibly fork) "Unity +# Performance Test" run completes, and writes the "performance" section of the +# unified CI status comment. Mirrors pr-comment-test-failures.yml. +# +# "Unity Performance Test" fires on every PR event but its job gates on the +# 'perf_test' label, so most runs conclude 'success' with the job skipped — +# those must not touch the section (it belongs to the bare-metal benchmark +# status written by pr-comment-artifact-url.yml on label-less PRs). +on: + workflow_run: + types: + - "completed" + workflows: + - "Unity Performance Test" + workflow_dispatch: + +permissions: + contents: read + pull-requests: write + actions: read + +jobs: + comment: + runs-on: ubuntu-latest + steps: + - name: Resolve PR number + id: pr + env: + WORKFLOW_RUN_EVENT_OBJ: ${{ toJSON(github.event.workflow_run) }} + run: | + PR_NUMBER=$(jq -r '.pull_requests[0].number' <<< "$WORKFLOW_RUN_EVENT_OBJ") + echo "PR number: $PR_NUMBER" + if [[ -z "$PR_NUMBER" || "$PR_NUMBER" == "null" ]]; then + echo "No PR associated with this run, skipping." + echo "pr-number=" >> "$GITHUB_OUTPUT" + else + echo "pr-number=$PR_NUMBER" >> "$GITHUB_OUTPUT" + fi + + - name: Check the performance job actually ran + id: ran + if: steps.pr.outputs.pr-number != '' + env: + GITHUB_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + RUN_ID: ${{ github.event.workflow_run.id }} + run: | + set -euo pipefail + CONCLUSION=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" \ + --jq '[.jobs[] | select(.name | startswith("Performance Test"))][0].conclusion // "absent"') + echo "Performance job conclusion: $CONCLUSION" + if [ "$CONCLUSION" = "skipped" ] || [ "$CONCLUSION" = "absent" ]; then + echo "ran=false" >> "$GITHUB_OUTPUT" + else + echo "ran=true" >> "$GITHUB_OUTPUT" + fi + + - name: Compose comment body + id: body + if: steps.pr.outputs.pr-number != '' && steps.ran.outputs.ran == 'true' + env: + GITHUB_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + RUN_ID: ${{ github.event.workflow_run.id }} + RUN_URL: ${{ github.event.workflow_run.html_url }} + CONCLUSION: ${{ github.event.workflow_run.conclusion }} + run: | + set -euo pipefail + BADGE_STYLE="?logo=speedtest&logoColor=white&style=for-the-badge" + case "$CONCLUSION" in + success) BADGE="https://img.shields.io/badge/Performance-Passed!-3fb950${BADGE_STYLE}"; MSG="✅ Unity performance suite finished." ;; + failure) BADGE="https://img.shields.io/badge/Performance-Failed!-ff0000${BADGE_STYLE}"; MSG="❌ Unity performance suite failed." ;; + *) BADGE="https://img.shields.io/badge/Performance-Cancelled-lightgrey${BADGE_STYLE}"; MSG="Unity performance suite did not finish (\`$CONCLUSION\`)." ;; + esac + + # Artifact download links come from the trusted Actions API. + ARTS_JSON=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') + links="" + for entry in "results (JSON):Performance test results (JSON)" "benchmark report (PDF):Performance benchmark report (PDF)"; do + label="${entry%%:*}" + art_name="${entry#*:}" + art_id=$(jq -r --arg n "$art_name" '.artifacts[]? | select(.name==$n) | .id // empty' <<< "$ARTS_JSON" | head -1) + [ -n "$art_id" ] && links="$links · [$label](https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$art_id)" + done + + DELIM="EOF_${RANDOM}${RANDOM}_$$" + { + echo "body<<$DELIM" + echo "[![Performance]($BADGE)]($RUN_URL)" + echo "" + echo "$MSG The benchmark report is rendered on the [run summary]($RUN_URL)." + echo "" + if [ -n "$links" ]; then + echo "Download:${links#" ·"}" + else + echo "No result artifacts were produced — check the [run]($RUN_URL)." + fi + echo "$DELIM" + } >> "$GITHUB_OUTPUT" + + - name: Checkout CI status action + if: steps.body.outcome == 'success' && steps.ran.outputs.ran == 'true' + uses: actions/checkout@v6 + with: + sparse-checkout: .github/actions/ci-status-comment + sparse-checkout-cone-mode: false + persist-credentials: false + + - name: Update performance section + if: steps.body.outcome == 'success' && steps.ran.outputs.ran == 'true' + uses: ./.github/actions/ci-status-comment + with: + pr-number: ${{ steps.pr.outputs.pr-number }} + section: performance + github-token: ${{ github.token }} + body: ${{ steps.body.outputs.body }} From 3b856eb892a4d65064cf335b5c37ceaba6add1f1 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:02:52 +0200 Subject: [PATCH 05/28] ci: surface a failed performance-test dispatch in the PR status comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repository-dispatch to decentraland/performance-testing has been failing silently since ~2026-06-15 ("Repository not found, OR token has insufficient permissions" — the PERFORMANCE_TESTING_PAT secret, last rotated 2026-05-20, expired). The step turned every comment-success run red but left no trace on the PR, so nobody noticed for two months. On dispatch failure the performance section now shows a red "Dispatch failed" state pointing at the step log and naming the likely cause. Co-Authored-By: Claude Fable 5 --- .github/workflows/pr-comment-artifact-url.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index b15011dff46..2e8c01e86f2 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -318,6 +318,22 @@ jobs: 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. + # A failing dispatch (an expired PERFORMANCE_TESTING_PAT is the historical + # cause — it broke silently from 2026-06-15 to 2026-08-13) turns this job + # red but leaves no trace on the PR; surface it in the performance section + # so it cannot go unnoticed for weeks again. + - name: Mark performance section as dispatch-failed + if: failure() && steps.perf_dispatch.outcome == 'failure' + uses: ./.github/actions/ci-status-comment + with: + pr-number: ${{ needs.pre-validation.outputs.pr-number }} + section: performance + github-token: ${{ github.token }} + body: |- + [![Performance](https://img.shields.io/badge/Performance-Dispatch%20failed-ff0000?logo=speedtest&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) + + ❌ Could not dispatch the bare-metal benchmark — see the [step log](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}). If it says "Repository not found, OR token has insufficient permissions", the `PERFORMANCE_TESTING_PAT` secret has expired and needs to be rotated. + comment-failed: needs: [pre-validation, check-build-ran] if: github.event.workflow_run.conclusion == 'failure' && needs.pre-validation.outputs.pr-number != '' && needs.check-build-ran.outputs.build-ran == 'true' From 94e21fc683ad83019156caa8723295b2a8691acd Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:47:58 +0200 Subject: [PATCH 06/28] ci: warn about PERFORMANCE_TESTING_PAT expiry inside the CI status comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a successful benchmark dispatch, probe the PAT's remaining lifetime via the github-authentication-token-expiration response header and, when under 30 days, append a rotation warning to the performance section of the unified CI status comment. This replaces the idea of a separate expiry-canary workflow opening issues — the warning lives where people already look. The 2026-06..08 outage was exactly this token expiring with no warning anywhere visible. Co-Authored-By: Claude Fable 5 --- .github/workflows/pr-comment-artifact-url.yml | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 2e8c01e86f2..96fdab1dd6b 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -303,6 +303,31 @@ jobs: "mac_base_build_url": "${{ vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL }}/@dcl/unity-explorer/releases/${{ env.RELEASE_TAG }}/Decentraland_macos.zip" } + # The PAT's remaining lifetime comes back as a response header on any API + # call it makes; surfacing it in the status comment replaces a separate + # expiry canary — the 2026-06..08 outage was exactly this token expiring + # with no warning anywhere a human looks. + - name: Probe performance PAT expiry + if: steps.perf_dispatch.outcome == 'success' + env: + PAT: ${{ secrets.PERFORMANCE_TESTING_PAT }} + run: | + set -euo pipefail + exp=$(curl -sI -H "Authorization: Bearer $PAT" https://api.github.com/repos/decentraland/performance-testing \ + | tr -d '\r' | grep -i '^github-authentication-token-expiration:' | cut -d' ' -f2- || true) + msg="" + if [ -n "$exp" ]; then + exp_s=$(date -d "$exp" +%s 2>/dev/null || echo 0) + if [ "$exp_s" -gt 0 ]; then + days=$(( (exp_s - $(date +%s)) / 86400 )) + echo "PERFORMANCE_TESTING_PAT expires in $days days ($exp)" + if [ "$days" -lt 30 ]; then + msg="⚠️ \`PERFORMANCE_TESTING_PAT\` expires in **$days days** ($exp) — rotate it before benchmark dispatches start failing." + fi + fi + fi + echo "PAT_EXPIRY_WARNING=$msg" >> "$GITHUB_ENV" + # repository_dispatch is fire-and-forget (no run id comes back), so this # links the target workflow's run list; the benchmark itself posts a # separate perf-test-summary comment with its run link when it finishes. @@ -318,6 +343,8 @@ jobs: 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. + ${{ env.PAT_EXPIRY_WARNING }} + # A failing dispatch (an expired PERFORMANCE_TESTING_PAT is the historical # cause — it broke silently from 2026-06-15 to 2026-08-13) turns this job # red but leaves no trace on the PR; surface it in the performance section From 34aad8724d9dbb472b2f8294a0de7823525277b0 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:56:23 +0200 Subject: [PATCH 07/28] ci: let external callers write CI status sections (file body, no-create) Two opt-in knobs on upsert-ci-status.sh for decentraland/performance-testing, which will fold its benchmark report into the performance section instead of posting a standalone perf-test-summary comment: - SECTION_BODY_FILE: read the section body from a file (the report is too large to pass comfortably via env). - NO_CREATE=1: exit 3 instead of creating the unified comment when it does not exist - a comment created with a foreign token would not be authored by github-actions[bot], later writers would not find it, and duplicates would accumulate. The caller falls back to its standalone comment instead. Co-Authored-By: Claude Fable 5 --- .../ci-status-comment/upsert-ci-status.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index b7baeefbece..c5fe4c5ee50 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -22,6 +22,19 @@ # confirm the section landed and no duplicate slipped in — retrying otherwise. set -euo pipefail +# Optional caller knobs (used by decentraland/performance-testing, which runs +# this script directly against unity-explorer's unified comment): +# SECTION_BODY_FILE — read the body from a file instead of $SECTION_BODY, +# for bodies too large to pass comfortably via env. +# NO_CREATE=1 — never create the unified comment; exit 3 when it does +# not exist so the caller can fall back to a standalone +# comment (a foreign-token creation would not be authored +# by github-actions[bot] and later writers would not +# find it, spawning duplicates). +if [ -n "${SECTION_BODY_FILE:-}" ]; then + SECTION_BODY="$(cat "$SECTION_BODY_FILE")" +fi + MARKER="" HEADER="### 🚦 CI Status" BOT="github-actions[bot]" @@ -100,6 +113,11 @@ for attempt in 1 2 3 4 5; do while IFS= read -r line; do [ -n "$line" ] && IDS+=("$line"); done <<< "$(marker_ids "$COMMENTS")" COMMENT_ID="${IDS[0]:-}" + if [ -z "$COMMENT_ID" ] && [ -n "${NO_CREATE:-}" ]; then + echo "No unified CI status comment exists and NO_CREATE is set; leaving creation to the repo's own workflows." + exit 3 + fi + # Collapse accidental duplicates from a create race: keep the oldest, drop the rest. if [ "${#IDS[@]}" -gt 1 ]; then for extra in "${IDS[@]:1}"; do From 29b7e7b02eeaeb7204f701563f48e8070f5d029c Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:17:00 +0200 Subject: [PATCH 08/28] ci: address review findings across the status-comment pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes for the 10 open review threads: - ucb-build-links: URL_RE now requires /builds/ under the Unity hosts, mirroring build.py's producer check — query-string-only paths (open-redirect bait) no longer validate. - pr-comment-artifact-url: artifacts list fetched with per_page=100 so unity_build_info_* cannot fall off page 1 and fake build-ran=false; the PAT expiry probe moved to gh api HEAD /rate_limit (token via env, never argv), gained continue-on-error + a 15s ceiling + a loud warning when the expiration header is absent; the dispatch-failed marker now fires on outcome != success so upstream failures (Find latest release, section upserts) no longer leave the performance section silent. - pr-comment-test-failures: slowest/failed test names render inside inline code with backticks/pipes stripped — markdown-shaped names read as text instead of first-party links/images. - test.yml: duration values are clamped to finite floats (float() admits nan/inf/1e999), keeping the timings JSON valid for the consumer's jq. - visual-regression: the two comment-writing jobs drop to contents:read + pull-requests:write. Narrowing secrets:inherit needs run-visual-suite.yml to declare workflow_call secrets first (it currently instructs callers to use inherit), so that part stays. - upsert-ci-status: fence-existence check is whole-line (grep -qxF) matching the awk matchers, so an embedded marker in a body line can no longer wedge a section; file-passed bodies are capped at 20k chars with a visible truncation note (GitHub's 65536 ceiling is shared by all sections); SECTION is validated against the fence set and fails fast; duplicate-comment GC is skipped for NO_CREATE callers; NO_CREATE waits one round before falling back to a standalone comment. Committed via API because repository rules require verified signatures. Co-Authored-By: Claude Fable 5 --- .../ci-status-comment/upsert-ci-status.sh | 41 ++++++++++++++++--- .github/actions/ucb-build-links/action.yml | 5 ++- .github/workflows/pr-comment-artifact-url.yml | 27 +++++++++--- .../workflows/pr-comment-test-failures.yml | 9 ++-- .github/workflows/test.yml | 7 +++- .github/workflows/visual-regression.yml | 10 +++++ 6 files changed, 84 insertions(+), 15 deletions(-) diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index c5fe4c5ee50..fe1faeaaf98 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -33,8 +33,24 @@ set -euo pipefail # find it, spawning duplicates). if [ -n "${SECTION_BODY_FILE:-}" ]; then SECTION_BODY="$(cat "$SECTION_BODY_FILE")" + # GitHub caps an issue comment at 65536 chars across every section; keep one + # writer from consuming the whole budget and failing an unrelated section's + # PATCH with an opaque 422. Truncation is fine for a status section that + # already links out to the full report. + if [ "${#SECTION_BODY}" -gt 20000 ]; then + echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to 20000." + SECTION_BODY="${SECTION_BODY:0:20000}"$'\n\n'"_…truncated; see the linked run for the full report._" + fi fi +# Fail fast on a section name outside the fence set — an unknown name would +# append a dead fence to the shared comment and then wedge the survive check +# for 5 attempts, burning ~15 API calls per write from then on. +case "${SECTION:-}" in + build|lint|tests|performance|automation) ;; + *) echo "::error::Unknown section '${SECTION:-}'."; exit 2 ;; +esac + MARKER="" HEADER="### 🚦 CI Status" BOT="github-actions[bot]" @@ -114,12 +130,23 @@ for attempt in 1 2 3 4 5; do COMMENT_ID="${IDS[0]:-}" if [ -z "$COMMENT_ID" ] && [ -n "${NO_CREATE:-}" ]; then - echo "No unified CI status comment exists and NO_CREATE is set; leaving creation to the repo's own workflows." - exit 3 + # Lose one round before falling back: an external caller often lands here + # seconds before the build workflow seeds the comment, and the standalone + # fallback it would post instead is noise that never collapses. + if [ "$attempt" -ge 2 ]; then + echo "No unified CI status comment exists and NO_CREATE is set; leaving creation to the repo's own workflows." + exit 3 + fi + echo "No unified CI status comment yet (attempt $attempt); waiting for the repo's own workflows to seed it." + sleep $((attempt * 2)) + continue fi - # Collapse accidental duplicates from a create race: keep the oldest, drop the rest. - if [ "${#IDS[@]}" -gt 1 ]; then + # Collapse accidental duplicates from a create race: keep the oldest, drop the + # rest. Skipped for external callers — comment GC belongs to this repo's own + # workflows, which run often enough to clean up within minutes, and a misfire + # under a foreign token would delete evidence with nothing logged. + if [ "${#IDS[@]}" -gt 1 ] && [ -z "${NO_CREATE:-}" ]; then for extra in "${IDS[@]:1}"; do echo "Deleting duplicate CI status comment $extra." gh api -X DELETE "/repos/$REPO/issues/comments/$extra" >/dev/null || true @@ -138,7 +165,11 @@ for attempt in 1 2 3 4 5; do # instead of resetting the whole comment and wiping the other sections' state. if [ -z "$CURRENT_BODY" ]; then CURRENT_BODY="$(skeleton)" - elif ! grep -qF "$START" <<< "$CURRENT_BODY"; then + # -x: whole-line, matching replace_section/extract_section's $0==s exactly. A + # substring hit on a marker embedded in a body line (which the strip filter + # deliberately lets through) would skip fence creation here while the awk + # matchers see nothing — leaving the section permanently unwritable. + elif ! grep -qxF "$START" <<< "$CURRENT_BODY"; then CURRENT_BODY="$CURRENT_BODY"$'\n\n'"$(wrap_section "$SECTION" "$(section_default "$SECTION")")" fi diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index e2b36d86fb1..c898cdeef03 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -41,7 +41,10 @@ runs: # The info files come out of the PR-controlled build workflow, so treat them as # untrusted input: accept only a numeric build id and a Unity dashboard URL with # a conservative charset before letting them anywhere near a comment body. - URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&#-]*$' + # Mirrors the producer's '/builds/' requirement (build.py) so the two + # validators agree, and pins the id to digits — a query-string-only path + # under a Unity host (open-redirect bait) no longer passes. + URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&#-]*/builds/[0-9]+[A-Za-z0-9./_%~?=&#-]*$' parse_info() { local target="$1" local dir="ucb_info_${target}" diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 96fdab1dd6b..c344f8a9e76 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -88,7 +88,11 @@ jobs: # uploaded as soon as the build id is known, so a build that failed before # producing player artifacts still posts a failure comment (with the Unity # Cloud link) instead of leaving the comment stuck on "Pending". - NAMES=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts" \ + # per_page=100: the default page holds 30 and a two-target run already + # uploads ~a dozen artifacts — unity_build_info_* falling off page 1 + # would read as build-ran=false, the exact stuck-on-Pending bug the + # flag exists to prevent. + NAMES=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" \ --jq '[.artifacts[].name]') PLAYER_COUNT=$(jq 'map(select(startswith("Decentraland_"))) | length' <<< "$NAMES") INFO_COUNT=$(jq 'map(select(startswith("unity_build_info_"))) | length' <<< "$NAMES") @@ -308,13 +312,20 @@ jobs: # expiry canary — the 2026-06..08 outage was exactly this token expiring # with no warning anywhere a human looks. - name: Probe performance PAT expiry + # Purely decorative — its whole output is one optional sentence, so it + # must never gate the dispatched/dispatch-failed section writes below. if: steps.perf_dispatch.outcome == 'success' + continue-on-error: true env: PAT: ${{ secrets.PERFORMANCE_TESTING_PAT }} run: | set -euo pipefail - exp=$(curl -sI -H "Authorization: Bearer $PAT" https://api.github.com/repos/decentraland/performance-testing \ - | tr -d '\r' | grep -i '^github-authentication-token-expiration:' | cut -d' ' -f2- || true) + # The expiry header rides every authenticated call; /rate_limit spends + # no quota and names no repo that could drift. gh reads the token from + # the environment, keeping it out of any process's argv. head -1 keeps + # $GITHUB_ENV single-line even if the response ever repeats the header. + exp=$(GH_TOKEN="$PAT" timeout 15 gh api --include --method HEAD /rate_limit 2>/dev/null \ + | tr -d '\r' | grep -i '^github-authentication-token-expiration:' | head -1 | cut -d' ' -f2- || true) msg="" if [ -n "$exp" ]; then exp_s=$(date -d "$exp" +%s 2>/dev/null || echo 0) @@ -325,6 +336,8 @@ jobs: msg="⚠️ \`PERFORMANCE_TESTING_PAT\` expires in **$days days** ($exp) — rotate it before benchmark dispatches start failing." fi fi + else + echo "::warning::No github-authentication-token-expiration header returned — PAT expiry cannot be monitored." fi echo "PAT_EXPIRY_WARNING=$msg" >> "$GITHUB_ENV" @@ -349,8 +362,12 @@ jobs: # cause — it broke silently from 2026-06-15 to 2026-08-13) turns this job # red but leaves no trace on the PR; surface it in the performance section # so it cannot go unnoticed for weeks again. + # outcome != 'success' rather than == 'failure': anything failing between + # the checkout and the dispatch (Find latest release, the build-section + # upsert) skips perf_dispatch, and a skipped dispatch is the same silence + # on the PR as a failed one. - name: Mark performance section as dispatch-failed - if: failure() && steps.perf_dispatch.outcome == 'failure' + if: failure() && steps.perf_dispatch.outcome != 'success' uses: ./.github/actions/ci-status-comment with: pr-number: ${{ needs.pre-validation.outputs.pr-number }} @@ -359,7 +376,7 @@ jobs: body: |- [![Performance](https://img.shields.io/badge/Performance-Dispatch%20failed-ff0000?logo=speedtest&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) - ❌ Could not dispatch the bare-metal benchmark — see the [step log](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}). If it says "Repository not found, OR token has insufficient permissions", the `PERFORMANCE_TESTING_PAT` secret has expired and needs to be rotated. + ❌ Could not dispatch the bare-metal benchmark (the job failed before or during the dispatch) — see the [step log](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}). If it says "Repository not found, OR token has insufficient permissions", the `PERFORMANCE_TESTING_PAT` secret has expired and needs to be rotated. comment-failed: needs: [pre-validation, check-build-ran] diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index 40a6d20231a..c26a3b6c912 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -118,11 +118,14 @@ jobs: # Suite duration + slowest tests come from the same untrusted artifact: # duration must be numeric before it reaches awk, and slowest entries are - # type-checked in jq with names flattened to a single line. + # type-checked in jq with names flattened to a single line. Names render + # inside inline code (backticks/pipes stripped) so a name shaped like + # markdown — a link, an , a — reads as text instead of + # rendering as first-party comment furniture. d=$(jq -r '.duration // empty' "$file") if [[ "$d" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then dur=$(fmt_secs "$d"); else dur="—"; fi slow=$(jq -r --arg mode "$mode" \ - '.slowest[]? | select((.seconds|type=="number") and (.name|type=="string")) | "- [\($mode)] \(.seconds)s \(.name | gsub("[\r\n]"; " "))"' \ + '.slowest[]? | select((.seconds|type=="number") and (.name|type=="string")) | "- [\($mode)] \(.seconds)s `\(.name | gsub("[\r\n`|]"; " "))`"' \ "$file" 2>/dev/null || true) [ -n "$slow" ] && slowest_list="$slowest_list$slow"$'\n' @@ -130,7 +133,7 @@ jobs: if [ "$status" = "passed" ]; then status=failed; fi total_failed=$((total_failed + f)) rows="$rows| $disp_cell | ❌ $f failed | $p | $f | $s | $dur |"$'\n' - names=$(jq -r --arg mode "$mode" '.failed[] | "- [\($mode)] \(. | gsub("[\r\n]"; " "))"' "$file") + names=$(jq -r --arg mode "$mode" '.failed[] | "- [\($mode)] `\(. | gsub("[\r\n`|]"; " "))`"' "$file") failed_list="$failed_list$names"$'\n' else rows="$rows| $disp_cell | ✅ Passed | $p | 0 | $s | $dur |"$'\n' diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 299dcbaa093..a9ec4b30f4a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -805,7 +805,7 @@ jobs: TEST_MODE: ${{ matrix.testMode }} run: | python3 - <<'PY' - import glob, json, os, xml.etree.ElementTree as ET + import glob, json, math, os, xml.etree.ElementTree as ET artifacts_path = os.environ["ARTIFACTS_PATH"] test_mode = os.environ["TEST_MODE"] @@ -831,6 +831,11 @@ jobs: seconds = float(test_case.get("duration") or 0) except ValueError: seconds = 0.0 + # float() admits nan/inf/1e999 without raising, and json.dump + # would then emit bare NaN/Infinity — invalid JSON that aborts + # the consumer's very first jq read of this file. + if not math.isfinite(seconds): + seconds = 0.0 duration += seconds timings.append((seconds, test_case.get("fullname") or test_case.get("name"))) if case_result == "Passed": diff --git a/.github/workflows/visual-regression.yml b/.github/workflows/visual-regression.yml index f2c9b8b8c8e..050ca43a5aa 100644 --- a/.github/workflows/visual-regression.yml +++ b/.github/workflows/visual-regression.yml @@ -171,6 +171,11 @@ jobs: needs: resolve if: needs.resolve.outputs.authorized == 'true' && needs.resolve.outputs.pr_number != '' runs-on: ubuntu-latest + # This job only writes the status comment; the workflow-level contents:write + # ceiling exists for the reusable suite call, not for it. + permissions: + contents: read + pull-requests: write steps: - name: Checkout CI status action uses: actions/checkout@v6 @@ -215,6 +220,11 @@ jobs: needs: [resolve, run-suite] if: always() && needs.resolve.outputs.authorized == 'true' && needs.resolve.outputs.pr_number != '' runs-on: ubuntu-latest + # This job only writes the status comment; the workflow-level contents:write + # ceiling exists for the reusable suite call, not for it. + permissions: + contents: read + pull-requests: write steps: - name: Checkout CI status action uses: actions/checkout@v6 From 1c934400afa881bb037be67bfab057706de1460c Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:30:18 +0200 Subject: [PATCH 09/28] ci: clamp the duration accumulator and truncate section bodies structurally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 review fixes: - test.yml: the serialised duration is clamped too — per-case isfinite keeps each addend and the slowest list finite, but a sum of finite doubles (two 1e308s) still overflows to inf, and round(inf,1) would emit bare Infinity into the JSON. Failed test-case entries with neither fullname nor name fall back to "(unnamed)" so sorted(set(...)) cannot hit a None/str TypeError. - upsert-ci-status.sh: the 20k cap now guards both body paths (env and file), and truncation closes any code fence or
the cut severed — an unterminated construct would render the rest of the comment inside it, visually eating the neighbouring sections. Committed via API because repository rules require verified signatures. Co-Authored-By: Claude Fable 5 --- .../ci-status-comment/upsert-ci-status.sh | 28 ++++++++++++++----- .github/workflows/test.yml | 9 ++++-- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index fe1faeaaf98..58afdb745a7 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -33,14 +33,28 @@ set -euo pipefail # find it, spawning duplicates). if [ -n "${SECTION_BODY_FILE:-}" ]; then SECTION_BODY="$(cat "$SECTION_BODY_FILE")" - # GitHub caps an issue comment at 65536 chars across every section; keep one - # writer from consuming the whole budget and failing an unrelated section's - # PATCH with an opaque 422. Truncation is fine for a status section that - # already links out to the full report. - if [ "${#SECTION_BODY}" -gt 20000 ]; then - echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to 20000." - SECTION_BODY="${SECTION_BODY:0:20000}"$'\n\n'"_…truncated; see the linked run for the full report._" +fi + +# 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 +# fine for a status section that already links out to the full report. +if [ "${#SECTION_BODY}" -gt 20000 ]; then + echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to 20000." + SECTION_BODY="${SECTION_BODY:0:20000}" + # Close constructs the cut may have severed — an unterminated code fence or + #
makes GitHub render everything after it in this comment inside + # the open block, visually eating the neighbouring sections. + if [ $(( $(grep -c '^```' <<< "$SECTION_BODY") % 2 )) -ne 0 ]; then + SECTION_BODY="$SECTION_BODY"$'\n''```' fi + opens=$(grep -oi '' + closes=$((closes + 1)) + done + SECTION_BODY="$SECTION_BODY"$'\n\n'"_…truncated; see the linked run for the full report._" fi # Fail fast on a section name outside the fence set — an unknown name would diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a9ec4b30f4a..57ae1d815af 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -841,14 +841,19 @@ jobs: if case_result == "Passed": passed += 1 elif case_result == "Failed": - failed.append(test_case.get("fullname") or test_case.get("name")) + # "(unnamed)" keeps the set homogeneous — one None among + # strings makes sorted() raise and kills the whole file. + failed.append(test_case.get("fullname") or test_case.get("name") or "(unnamed)") result = { "hasResults": len(xml_files) > 0, "total": total, "passed": passed, "failed": sorted(set(failed)), - "duration": round(duration, 1), + # The per-case clamp keeps each addend (and the slowest list) + # finite, but a sum of finite doubles can still overflow to inf — + # clamp again at the one point the accumulator is serialised. + "duration": round(duration, 1) if math.isfinite(duration) else 0.0, "slowest": [ {"name": name, "seconds": round(seconds, 1)} for seconds, name in sorted(timings, key=lambda t: -t[0])[:10] From 414b0f3fc285f7848d7352be16f6b264d6bdf52b Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:50:15 +0200 Subject: [PATCH 10/28] ci: collapse the build table to one row per platform and link Unity Cloud live The build section's table becomes three rows: Build (commit, run logs, build date), Windows and Mac (GitHub job, Unity Cloud build page, Unity log artifact, zip downloads) - the per-platform links people actually follow, one click shorter. build.py now also writes the per-target Unity Cloud link into the status comment the moment the Unity-side build id is known, re-asserting a few times from its poll loop in case the Pending reset lands after it - the dashboard becomes watchable while the build runs instead of only after completion. Push/dispatch builds (no PR number) skip it. Co-Authored-By: Claude Fable 5 --- .github/actions/ucb-build-links/action.yml | 54 +++++-- .github/workflows/build-unitycloud.yml | 4 + .github/workflows/pr-comment-artifact-url.yml | 48 ++++-- scripts/cloudbuild/build.py | 144 ++++++++++++++++++ 4 files changed, 227 insertions(+), 23 deletions(-) diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index c898cdeef03..754e995cece 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -16,14 +16,22 @@ inputs: outputs: rows: description: >- - "| Name | Link |"-shaped rows for an existing two-column table, pairing - each target's Unity Cloud build page with its GitHub job log; empty when - no valid build info was found. + "| Name | Link |"-shaped rows for an existing two-column table, one per + target, each pairing the GitHub job log, the Unity Cloud build page and + the Unity log artifact; empty when no valid build info was found. value: ${{ steps.fetch.outputs.rows }} section: description: >- Standalone table (header + rows); empty when no valid build info was found. value: ${{ steps.fetch.outputs.section }} + windows-cell: + description: >- + The Windows row's link cell alone ("[GitHub job](…) · [Unity Cloud #N](…) · + [Unity log](…)"), for callers composing their own rows; empty when unknown. + value: ${{ steps.fetch.outputs.windows-cell }} + mac-cell: + description: Mac twin of windows-cell. + value: ${{ steps.fetch.outputs.mac-cell }} runs: using: composite @@ -70,27 +78,47 @@ runs: # page with the GitHub-side job log. JOBS_JSON=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/jobs?per_page=100" 2>/dev/null || echo '{"jobs":[]}') + # Suite id + artifact ids feed the per-target Unity log download links + # (artifact downloads hang off the check suite, not the run). + SUITE_ID=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID" --jq '.check_suite_id' 2>/dev/null || echo "") + [[ "$SUITE_ID" =~ ^[0-9]+$ ]] || SUITE_ID="" + ARTIFACTS_JSON=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') + + WINDOWS_CELL="" + MAC_CELL="" ROWS="" for entry in "windows64:Windows" "macos:Mac"; do target="${entry%%:*}" label="${entry#*:}" parse_info "$target" job_url=$(jq -r --arg n "Build ($target)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1) + log_id=$(jq -r --arg n "${target}_launcher_unity_log" \ + '.artifacts[]? | select(.name==$n and .expired==false) | .id' <<< "$ARTIFACTS_JSON" | head -1) + [[ "$log_id" =~ ^[0-9]+$ ]] || log_id="" - cell="" + parts=() + [ -n "$job_url" ] && parts+=("[GitHub job](${job_url})") # A URL without a valid id only occurs on a tampered artifact — drop the link # rather than render an empty "[#](...)" label. if [ -n "$REPLY_ID" ] && [ -n "$REPLY_URL" ]; then - cell="[Unity Cloud #${REPLY_ID}](${REPLY_URL})" + parts+=("[Unity Cloud #${REPLY_ID}](${REPLY_URL})") elif [ -n "$REPLY_ID" ]; then - cell="Unity Cloud #${REPLY_ID}" + parts+=("Unity Cloud #${REPLY_ID}") fi - if [ -n "$cell" ] && [ -n "$job_url" ]; then - cell="${cell} · [GitHub job](${job_url})" + if [ -n "$log_id" ] && [ -n "$SUITE_ID" ]; then + parts+=("[Unity log](${GITHUB_SERVER_URL:-https://github.com}/${REPO_FULL}/suites/${SUITE_ID}/artifacts/${log_id})") fi - if [ -n "$cell" ]; then - ROWS+="| ${label} build | ${cell} |"$'\n' + + cell="" + if [ "${#parts[@]}" -gt 0 ]; then + cell=$(printf '%s · ' "${parts[@]}") + cell="${cell% · }" + ROWS+="| ${label} | ${cell} |"$'\n' fi + case "$target" in + windows64) WINDOWS_CELL="$cell" ;; + macos) MAC_CELL="$cell" ;; + esac done SECTION="" @@ -108,4 +136,10 @@ runs: echo "section<<${DELIM}" printf '%s' "$SECTION" echo "${DELIM}" + echo "windows-cell<<${DELIM}" + printf '%s' "$WINDOWS_CELL" + echo "${DELIM}" + echo "mac-cell<<${DELIM}" + printf '%s' "$MAC_CELL" + echo "${DELIM}" } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/build-unitycloud.yml b/.github/workflows/build-unitycloud.yml index 5630e6f7dfe..20f3971fc44 100644 --- a/.github/workflows/build-unitycloud.yml +++ b/.github/workflows/build-unitycloud.yml @@ -597,6 +597,10 @@ jobs: QUEUE_TIMEOUT: 14400 BUILD_TIMEOUT: 10800 TARGET: t_${{ matrix.target }} + # For the live PR status-comment update the moment the Unity-side + # build id is known; empty PR number (push/dispatch) disables it. + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number || '' }} BRANCH_NAME: ${{ github.head_ref || github.ref_name }} COMMIT_SHA: ${{ needs.prebuild.outputs.commit_sha }} BUILD_OPTIONS: ${{ needs.prebuild.outputs.options }} diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index c344f8a9e76..e56b3945218 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -63,7 +63,7 @@ jobs: body: |- [![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) - New build in progress, come back later! + New build in progress — per-platform Unity Cloud links land here as soon as each build is created. check-build-ran: needs: pre-validation @@ -187,6 +187,7 @@ jobs: SHORT_SHA=$(echo "$HEAD_SHA" | cut -c1-7) echo "Short SHA: $SHORT_SHA" + echo "SHORT_SHA=$SHORT_SHA" >> "$GITHUB_ENV" SAFE_BRANCH_NAME=$(jq -r '.pull_requests[0].head.ref // .head_branch' <<< "$WORKFLOW_RUN_EVENT_OBJ") echo "Safe Branch Name: $SAFE_BRANCH_NAME" @@ -257,6 +258,33 @@ jobs: run-id: ${{ env.PREVIOUS_JOB_ID }} github-token: ${{ github.token }} + # One row per platform: GitHub job · Unity Cloud build · Unity log · zip + # downloads. Composed here rather than inline in the body so a link whose + # id could not be resolved is dropped instead of rendering broken. + - name: Compose platform rows + env: + WINDOWS_CELL: ${{ steps.ucb.outputs.windows-cell }} + MAC_CELL: ${{ steps.ucb.outputs.mac-cell }} + S3_BASE: ${{ format('{0}/{1}', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} + run: | + set -euo pipefail + compose_row() { + local label="$1" cell="$2" art_id="$3" file="$4" + local parts=() + [ -n "$cell" ] && parts+=("$cell") + [ -n "$art_id" ] && parts+=("[Download .zip](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/suites/${SUITE_ID}/artifacts/${art_id})") + parts+=("[.zip via S3](${S3_BASE}/${file})") + local joined + joined=$(printf '%s · ' "${parts[@]}") + printf '| %s | %s |\n' "$label" "${joined% · }" + } + { + echo "PLATFORM_ROWS<> "$GITHUB_ENV" + - name: Update build section uses: ./.github/actions/ci-status-comment with: @@ -266,18 +294,12 @@ jobs: body: |- [![Build](https://img.shields.io/badge/Build-Success!-3fb950?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) - Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. - - | Name | Link | - | -------- | ----------------------- | - | Commit | ${{ env.HEAD_SHA }} | - | Logs | ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }} | - | Download Windows | ${{ github.server_url }}/${{ github.repository }}/suites/${{ env.SUITE_ID }}/artifacts/${{ env.WINDOWS_ARTIFACT_ID }} | - | Download Windows S3 | ${{ format('{0}/{1}/Decentraland_windows64.zip', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} | - | Download Mac | ${{ github.server_url }}/${{ github.repository }}/suites/${{ env.SUITE_ID }}/artifacts/${{ env.MAC_ARTIFACT_ID }} | - | Download Mac S3 | ${{ format('{0}/{1}/Decentraland_macos.zip', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} | - | Built on | ${{ env.BUILD_DATE }} | - ${{ steps.ucb.outputs.rows }} + Windows and Mac built successfully in Unity Cloud. + + | Name | Link | + | -------- | ----------------------- | + | Build | [`${{ env.SHORT_SHA }}`](${{ github.server_url }}/${{ github.repository }}/commit/${{ env.HEAD_SHA }}) · [Logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) · built ${{ env.BUILD_DATE }} | + ${{ env.PLATFORM_ROWS }} ${{ env.SIZE_REPORT }} diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index b62cb2509d6..db1f9bd8e23 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -8,6 +8,8 @@ import requests import datetime import argparse +import subprocess +import tempfile import collections from urllib3.util.retry import Retry from requests.adapters import HTTPAdapter @@ -68,6 +70,15 @@ def _extract_member(self, member, targetpath, pwd): dashboard_url = None _build_link_info_written = False +# Live PR status-comment update: the artifact above only leaves the runner when +# this job ends, so the comment's build section is also written directly the +# moment the build id is known — the Unity Cloud link goes live while the build +# runs instead of after it. Purely cosmetic: every failure is swallowed. +CI_STATUS_SCRIPT = os.path.join('.github', 'actions', 'ci-status-comment', 'upsert-ci-status.sh') +LIVE_MARKER_PREFIX = '' in body: + start = body.find('') + end = body.find('') + return body[start:end] if 0 <= start < end else '' + if len(comments) < 100: + break + return '' + + +def _own_job_url(): + target = (os.getenv('TARGET') or '')[2:] + repo = os.getenv('GITHUB_REPOSITORY') + run_id = os.getenv('GITHUB_RUN_ID') + try: + resp = _github_api(f'/repos/{repo}/actions/runs/{run_id}/jobs?per_page=100') + if resp.status_code == 200: + for job in resp.json().get('jobs') or []: + if job.get('name') == f'Build ({target})': + return job.get('html_url') + except requests.RequestException: + pass + return None + + +def upsert_live_comment(build_id, only_if_missing=False): + """Write this target's live links into the comment's build section. + + 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. + """ + target = (os.getenv('TARGET') or '')[2:] + label = {'windows64': 'Windows', 'macos': 'Mac'}.get(target, target) + marker = f'{LIVE_MARKER_PREFIX}{target} -->' + section = _build_section_of_status_comment() + if only_if_missing and marker in section: + return False + + parts = [] + job_url = _own_job_url() + if job_url: + parts.append(f'[GitHub job]({job_url})') + parts.append(f'[Unity Cloud #{build_id}]({dashboard_url})' if dashboard_url else f'Unity Cloud #{build_id}') + own_row = f'| {label} | {" · ".join(parts)} {marker} |' + + rows = [line for line in section.splitlines() if LIVE_MARKER_PREFIX in line and marker not in line] + rows.append(own_row) + rows.sort(key=lambda row: 0 if '| Windows |' in row else 1) + + server = os.getenv('GITHUB_SERVER_URL', 'https://github.com') + run_url = f"{server}/{os.getenv('GITHUB_REPOSITORY')}/actions/runs/{os.getenv('GITHUB_RUN_ID')}" + body = '\n'.join([ + f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url}) ' + f'', + '', + 'Building in Unity Cloud — live links, one row per platform as each build is created:', + '', + '| Name | Link |', + '| -------- | ----------------------- |', + *rows, + ]) + + body_file = None + try: + with tempfile.NamedTemporaryFile('w', suffix='.md', delete=False) as f: + f.write(body) + body_file = f.name + env = dict(os.environ, + REPO=os.getenv('GITHUB_REPOSITORY') or '', + SECTION='build', + SECTION_BODY='', + SECTION_BODY_FILE=body_file) + subprocess.run(['bash', CI_STATUS_SCRIPT], env=env, timeout=180, check=False) + finally: + if body_file: + os.unlink(body_file) + return True + + +def maybe_update_live_comment(build_id, reconcile=False, force=False): + """Gate and rate-limit the live comment write; never let it fail the build.""" + global _live_comment_asserts, _live_comment_last_attempt + if not os.getenv('PR_NUMBER') or not (os.getenv('GH_TOKEN') or os.getenv('GITHUB_TOKEN')): + return + if not os.path.exists(CI_STATUS_SCRIPT): + return + if reconcile: + # Re-assert a few times only: the Pending reset (or the other target's + # first write racing ours) can land after us and drop this row. + if _live_comment_asserts == 0 or _live_comment_asserts >= 3: + return + if time.time() - _live_comment_last_attempt < 240: + return + elif _live_comment_asserts > 0 and not force: + return + try: + _live_comment_last_attempt = time.time() + if upsert_live_comment(build_id, only_if_missing=reconcile): + _live_comment_asserts += 1 + except Exception as e: + print(f'note: live status-comment update failed: {e}') def write_step_summary(target, build_id, final_status, phase_durations, queue_reasons, queue_elapsed, build_elapsed): @@ -761,6 +903,8 @@ def run_poll_loop(id, build_already_active=False, resumed_build_elapsed=0): if dashboard_url is None: record_build_link_info(id, response_json) + else: + maybe_update_live_comment(id, reconcile=True) queued_reason = response_json.get('queuedReason') if queued_reason and status in QUEUE_STATUSES: From b5186df02408c5de799f608f27f28db4c42cc573 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:23:23 +0200 Subject: [PATCH 11/28] ci: derive the live row's platform from TARGET's stable prefix build.py rewrites TARGET to the per-branch build-target name, so the live row's label, marker and job lookup keyed on the raw value produced 'ndows64-feat-...' rows with no job link. Key them on the windows64/macos prefix, which survives every rewrite. Co-Authored-By: Claude Fable 5 --- scripts/cloudbuild/build.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index db1f9bd8e23..0d3645e9d0a 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -709,15 +709,25 @@ def _build_section_of_status_comment(): return '' +def _platform_key(): + """windows64 | macos, surviving the per-branch TARGET rewrites this script does.""" + target = os.getenv('TARGET') or '' + if target.startswith('t_'): + target = target[2:] + for key in ('windows64', 'macos'): + if target.startswith(key): + return key + return target or 'unknown' + + def _own_job_url(): - target = (os.getenv('TARGET') or '')[2:] repo = os.getenv('GITHUB_REPOSITORY') run_id = os.getenv('GITHUB_RUN_ID') try: resp = _github_api(f'/repos/{repo}/actions/runs/{run_id}/jobs?per_page=100') if resp.status_code == 200: for job in resp.json().get('jobs') or []: - if job.get('name') == f'Build ({target})': + if job.get('name') == f'Build ({_platform_key()})': return job.get('html_url') except requests.RequestException: pass @@ -732,9 +742,9 @@ def upsert_live_comment(build_id, only_if_missing=False): clobbering each other; write races on the comment itself are the upsert script's problem. Returns whether a write was attempted. """ - target = (os.getenv('TARGET') or '')[2:] - label = {'windows64': 'Windows', 'macos': 'Mac'}.get(target, target) - marker = f'{LIVE_MARKER_PREFIX}{target} -->' + platform = _platform_key() + label = {'windows64': 'Windows', 'macos': 'Mac'}.get(platform, platform) + marker = f'{LIVE_MARKER_PREFIX}{platform} -->' section = _build_section_of_status_comment() if only_if_missing and marker in section: return False From 4eb8b842802200ca3eeb94a48c166bcf02d54ccb Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:58:56 +0200 Subject: [PATCH 12/28] ci: drop the live-build intro line and stop #N autolinking to issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-progress build rows' unlinked fallback rendered "Unity Cloud #2" as plain text, which GitHub autolinks to issue #2; it now reads "Unity Cloud build 2" until the dashboard deep link is available (the linked form keeps the #N label — text inside a markdown link is not autolinked). Same fix in the composite action's tampered-URL fallback. Also removes the "Building in Unity Cloud — live links..." intro sentence; the badge and table speak for themselves. --- .github/actions/ucb-build-links/action.yml | 4 +++- scripts/cloudbuild/build.py | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index 754e995cece..591f0f64958 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -103,7 +103,9 @@ runs: if [ -n "$REPLY_ID" ] && [ -n "$REPLY_URL" ]; then parts+=("[Unity Cloud #${REPLY_ID}](${REPLY_URL})") elif [ -n "$REPLY_ID" ]; then - parts+=("Unity Cloud #${REPLY_ID}") + # Unlinked fallback must not say "#N": GitHub autolinks bare #N in + # comments to issue N. + parts+=("Unity Cloud build ${REPLY_ID}") fi if [ -n "$log_id" ] && [ -n "$SUITE_ID" ]; then parts+=("[Unity log](${GITHUB_SERVER_URL:-https://github.com}/${REPO_FULL}/suites/${SUITE_ID}/artifacts/${log_id})") diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 0d3645e9d0a..2e32f2f1e3c 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -753,7 +753,9 @@ def upsert_live_comment(build_id, only_if_missing=False): job_url = _own_job_url() if job_url: parts.append(f'[GitHub job]({job_url})') - parts.append(f'[Unity Cloud #{build_id}]({dashboard_url})' if dashboard_url else f'Unity Cloud #{build_id}') + # Unlinked fallback must not say "#N": GitHub autolinks bare #N in comments + # to issue N. + parts.append(f'[Unity Cloud #{build_id}]({dashboard_url})' if dashboard_url else f'Unity Cloud build {build_id}') own_row = f'| {label} | {" · ".join(parts)} {marker} |' rows = [line for line in section.splitlines() if LIVE_MARKER_PREFIX in line and marker not in line] @@ -766,8 +768,6 @@ def upsert_live_comment(build_id, only_if_missing=False): f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url}) ' f'', '', - 'Building in Unity Cloud — live links, one row per platform as each build is created:', - '', '| Name | Link |', '| -------- | ----------------------- |', *rows, From 53dc7e35d13716593a5c67a94a7c862ad1be98e5 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:06:16 +0200 Subject: [PATCH 13/28] ci: link the Unity Cloud build log page before the API deep link arrives The build id is known before the build API returns its dashboard_summary/dashboard_log links, which left the first status-comment write with an unlinked build label. The classic dashboard log page is constructible from ORG_ID/PROJECT_ID/TARGET alone, so the live row, the build-info artifact, and the unhealthy-build message now link it immediately; the API's own deep link still replaces it on the first poll that carries one. The constructed URL passes the composite action's URL_RE (allowed origin + /builds/). --- scripts/cloudbuild/build.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 2e32f2f1e3c..f22416ad156 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -662,8 +662,9 @@ def record_build_link_info(id, response_json): with open(BUILD_LINK_INFO_PATH, 'w') as f: f.write(f'BUILD_TARGET={os.getenv("TARGET")}\n') f.write(f'BUILD_ID={id}\n') - if href: - f.write(f'DASHBOARD_URL={href}\n') + link = href or _dashboard_log_url(id) + if link: + f.write(f'DASHBOARD_URL={link}\n') except OSError as e: print(f'Warning: could not write {BUILD_LINK_INFO_PATH}: {e}') @@ -720,6 +721,19 @@ def _platform_key(): return target or 'unknown' +def _dashboard_log_url(build_id): + """Classic Unity Cloud dashboard log page for a build, constructible from + the env alone — the API's own dashboard_summary/dashboard_log deep links + replace it as soon as a poll response carries them.""" + org = os.getenv('ORG_ID') + project = os.getenv('PROJECT_ID') + target = os.getenv('TARGET') + if not (org and project and target): + return None + return (f'https://developer.cloud.unity3d.com/build/orgs/{org}/projects/{project}' + f'/buildtargets/{target}/builds/{build_id}/log/') + + def _own_job_url(): repo = os.getenv('GITHUB_REPOSITORY') run_id = os.getenv('GITHUB_RUN_ID') @@ -753,9 +767,10 @@ def upsert_live_comment(build_id, only_if_missing=False): job_url = _own_job_url() if job_url: parts.append(f'[GitHub job]({job_url})') - # Unlinked fallback must not say "#N": GitHub autolinks bare #N in comments - # to issue N. - parts.append(f'[Unity Cloud #{build_id}]({dashboard_url})' if dashboard_url else f'Unity Cloud build {build_id}') + # Unlinked last-resort must not say "#N": GitHub autolinks bare #N in + # comments to issue N. + link = dashboard_url or _dashboard_log_url(build_id) + parts.append(f'[Unity Cloud #{build_id}]({link})' if link else f'Unity Cloud build {build_id}') own_row = f'| {label} | {" · ".join(parts)} {marker} |' rows = [line for line in section.splitlines() if LIVE_MARKER_PREFIX in line and marker not in line] @@ -1129,7 +1144,7 @@ def probe_latest_build(): download_log(id) if not build_healthy: - where = dashboard_url or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' + where = dashboard_url or _dashboard_log_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') sys.exit(1) From 23e95773d4e33e5766005f0bac679ec8fe734d42 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:30:01 +0200 Subject: [PATCH 14/28] ci: point the constructed dashboard link at the cloud.unity.com build page The live-test click-through showed the classic developer.cloud.unity3d.com path does not resolve for this org; the working page is https://cloud.unity.com/home/organizations/{org}/projects/{project}/cloud-build/buildtargets/{target}/builds/{N}, and ORG_ID/PROJECT_ID already hold exactly the identifiers that page expects (verified against the live row's rendered URL). Renamed the helper to _dashboard_build_url since it now links the build page, not the log tab. --- scripts/cloudbuild/build.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index f22416ad156..273ee014dfa 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -662,7 +662,7 @@ def record_build_link_info(id, response_json): with open(BUILD_LINK_INFO_PATH, 'w') as f: f.write(f'BUILD_TARGET={os.getenv("TARGET")}\n') f.write(f'BUILD_ID={id}\n') - link = href or _dashboard_log_url(id) + link = href or _dashboard_build_url(id) if link: f.write(f'DASHBOARD_URL={link}\n') except OSError as e: @@ -721,17 +721,17 @@ def _platform_key(): return target or 'unknown' -def _dashboard_log_url(build_id): - """Classic Unity Cloud dashboard log page for a build, constructible from - the env alone — the API's own dashboard_summary/dashboard_log deep links - replace it as soon as a poll response carries them.""" +def _dashboard_build_url(build_id): + """Unity Cloud dashboard page for a build, constructible from the env + alone — the API's own dashboard_summary/dashboard_log deep links replace + it as soon as a poll response carries them.""" org = os.getenv('ORG_ID') project = os.getenv('PROJECT_ID') target = os.getenv('TARGET') if not (org and project and target): return None - return (f'https://developer.cloud.unity3d.com/build/orgs/{org}/projects/{project}' - f'/buildtargets/{target}/builds/{build_id}/log/') + return (f'https://cloud.unity.com/home/organizations/{org}/projects/{project}' + f'/cloud-build/buildtargets/{target}/builds/{build_id}') def _own_job_url(): @@ -769,7 +769,7 @@ def upsert_live_comment(build_id, only_if_missing=False): parts.append(f'[GitHub job]({job_url})') # Unlinked last-resort must not say "#N": GitHub autolinks bare #N in # comments to issue N. - link = dashboard_url or _dashboard_log_url(build_id) + link = dashboard_url or _dashboard_build_url(build_id) parts.append(f'[Unity Cloud #{build_id}]({link})' if link else f'Unity Cloud build {build_id}') own_row = f'| {label} | {" · ".join(parts)} {marker} |' @@ -1144,7 +1144,7 @@ def probe_latest_build(): download_log(id) if not build_healthy: - where = dashboard_url or _dashboard_log_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' + where = dashboard_url or _dashboard_build_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') sys.exit(1) From a94f73d7ab27901c3fccd7943e3a4ad4f6ea430d Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:42:11 +0200 Subject: [PATCH 15/28] ci: DCL logo header, and durations for builds, lint and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The comment header shows the Decentraland logo instead of the traffic-light emoji, wrapped in — the one construct GitHub's renderer leaves unlinked, so clicking it no longer opens the raw image. Existing comments' headers migrate on the next section write; the decorative logo next to the build badges is dropped in favour of the header one. - Build rows show each platform's duration: build.py rewrites the link-info file at terminal status with the queue/build split, and the composite validates the numbers and renders "⏱ 1h 12m (6m queued)" per row. - The lint footer shows the Lint job's wall time; the tests table gains a Job time column (wall time incl. setup, from the trusted Actions API) next to the existing test-sum Time column, renamed Tests time for contrast. --- .../ci-status-comment/upsert-ci-status.sh | 8 +++++++- .github/actions/ucb-build-links/action.yml | 18 +++++++++++++++++ .github/workflows/pr-comment-artifact-url.yml | 8 ++++---- .../workflows/pr-comment-test-failures.yml | 18 ++++++++++++----- .github/workflows/pr-comment-warnings.yml | 7 +++++++ scripts/cloudbuild/build.py | 20 ++++++++++++++++--- 6 files changed, 66 insertions(+), 13 deletions(-) diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 58afdb745a7..2594828e3e3 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -66,7 +66,10 @@ case "${SECTION:-}" in esac MARKER="" -HEADER="### 🚦 CI Status" +# The wrapper stops GitHub's renderer from auto-wrapping the logo in +# a link to the image itself — the one construct its sanitizer leaves unlinked. +HEADER='### CI Status' +OLD_HEADER="### 🚦 CI Status" BOT="github-actions[bot]" START="" END="" @@ -187,6 +190,9 @@ for attempt in 1 2 3 4 5; do CURRENT_BODY="$CURRENT_BODY"$'\n\n'"$(wrap_section "$SECTION" "$(section_default "$SECTION")")" fi + # Migrate comments created while the header was still the emoji variant. + CURRENT_BODY="${CURRENT_BODY/"$OLD_HEADER"/$HEADER}" + NEW_BODY="$(replace_section "$CURRENT_BODY")" if [ -z "$COMMENT_ID" ]; then diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index 591f0f64958..47e69db0b0c 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -58,14 +58,20 @@ runs: local dir="ucb_info_${target}" REPLY_ID="" REPLY_URL="" + REPLY_QUEUE="" + REPLY_BUILD="" if gh run download "$RUN_ID" \ --repo "$REPO_FULL" \ --name "unity_build_info_${target}_launcher" \ --dir "$dir" 2>"${dir}.err"; then REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + REPLY_QUEUE=$(grep -m1 '^QUEUE_SECS=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) + REPLY_BUILD=$(grep -m1 '^BUILD_SECS=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) [[ "$REPLY_ID" =~ ^[0-9]+$ ]] || REPLY_ID="" [[ "$REPLY_URL" =~ $URL_RE ]] || REPLY_URL="" + [[ "$REPLY_QUEUE" =~ ^[0-9]+$ ]] || REPLY_QUEUE="" + [[ "$REPLY_BUILD" =~ ^[0-9]+$ ]] || REPLY_BUILD="" else # Absence is normal for runs predating the info artifact; still surface the # gh error so an auth/permission regression doesn't silently eat the rows. @@ -84,6 +90,13 @@ runs: [[ "$SUITE_ID" =~ ^[0-9]+$ ]] || SUITE_ID="" ARTIFACTS_JSON=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') + fmt_dur() { + local s=$1 + if [ "$s" -ge 3600 ]; then printf '%dh %dm' $((s/3600)) $((s%3600/60)) + elif [ "$s" -ge 60 ]; then printf '%dm %ds' $((s/60)) $((s%60)) + else printf '%ds' "$s"; fi + } + WINDOWS_CELL="" MAC_CELL="" ROWS="" @@ -110,6 +123,11 @@ runs: if [ -n "$log_id" ] && [ -n "$SUITE_ID" ]; then parts+=("[Unity log](${GITHUB_SERVER_URL:-https://github.com}/${REPO_FULL}/suites/${SUITE_ID}/artifacts/${log_id})") fi + if [ -n "$REPLY_BUILD" ]; then + t="⏱ $(fmt_dur "$REPLY_BUILD")" + [ -n "$REPLY_QUEUE" ] && [ "$REPLY_QUEUE" -gt 0 ] && t+=" ($(fmt_dur "$REPLY_QUEUE") queued)" + parts+=("$t") + fi cell="" if [ "${#parts[@]}" -gt 0 ]; then diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index e56b3945218..b2ae67ba30c 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -61,7 +61,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - [![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) + [![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) New build in progress — per-platform Unity Cloud links land here as soon as each build is created. @@ -127,7 +127,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - [![Build](https://img.shields.io/badge/Build-Skipped-yellow?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) + [![Build](https://img.shields.io/badge/Build-Skipped-yellow?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) Build skipped — no changes detected under `Explorer/`. @@ -292,7 +292,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - [![Build](https://img.shields.io/badge/Build-Success!-3fb950?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) + [![Build](https://img.shields.io/badge/Build-Success!-3fb950?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) Windows and Mac built successfully in Unity Cloud. @@ -428,7 +428,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - [![Build](https://img.shields.io/badge/Build-Failed!-ff0000?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) + [![Build](https://img.shields.io/badge/Build-Failed!-ff0000?logo=unity&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) Build failed! Check the [logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) to see what went wrong. If the error repeats please consider the `clean-build` tag. diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index c26a3b6c912..e01a5e8c158 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -99,12 +99,20 @@ jobs: job_url=$(jq -r --arg n "Test ($mode)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1) if [ -n "$job_url" ]; then disp_cell="[$disp]($job_url)"; else disp_cell="$disp"; fi + # Wall time of the suite's job (checkout + Unity licensing + import + + # tests), from the trusted Actions API — pairs with the test-sum Time + # column so setup overhead is visible. + job_secs=$(jq -r --arg n "Test ($mode)" \ + '[.jobs[]? | select(.name==$n and .completed_at != null) + | ((.completed_at|fromdateiso8601) - (.started_at|fromdateiso8601))] | first // empty' <<< "$JOBS_JSON") + if [[ "$job_secs" =~ ^[0-9]+$ ]]; then job_dur=$(fmt_secs "$job_secs"); else job_dur="—"; fi + # A suite that produced no result XML crashed or timed out before finishing. # Surface it as its own state instead of silently contributing 0 to a green total. if [ "$(jq -r '.hasResults' "$file")" != "true" ]; then status=incomplete [ -n "$job_url" ] || job_url="$WORKFLOW_RUN_URL" - rows="$rows| $disp_cell | ⚠️ No results | — | — | — | — |"$'\n' + rows="$rows| $disp_cell | ⚠️ No results | — | — | — | — | $job_dur |"$'\n' warnings="$warnings⚠️ **$disp** produced no results — the run likely crashed or timed out before finishing. Check the [\`Unity Test / Test ($mode)\`]($job_url) job."$'\n\n' continue fi @@ -132,11 +140,11 @@ jobs: if [ "$f" -gt 0 ]; then if [ "$status" = "passed" ]; then status=failed; fi total_failed=$((total_failed + f)) - rows="$rows| $disp_cell | ❌ $f failed | $p | $f | $s | $dur |"$'\n' + rows="$rows| $disp_cell | ❌ $f failed | $p | $f | $s | $dur | $job_dur |"$'\n' names=$(jq -r --arg mode "$mode" '.failed[] | "- [\($mode)] `\(. | gsub("[\r\n`|]"; " "))`"' "$file") failed_list="$failed_list$names"$'\n' else - rows="$rows| $disp_cell | ✅ Passed | $p | 0 | $s | $dur |"$'\n' + rows="$rows| $disp_cell | ✅ Passed | $p | 0 | $s | $dur | $job_dur |"$'\n' fi done @@ -162,8 +170,8 @@ jobs: echo "" printf '%s\n' "$headline" echo "" - echo "| TESTS SUITE | Result | Passed | Failed | Skipped | Time |" - echo "| ----------- | ------ | -----: | -----: | ------: | ---: |" + echo "| TESTS SUITE | Result | Passed | Failed | Skipped | Tests time | Job time |" + echo "| ----------- | ------ | -----: | -----: | ------: | ---: | ---: |" printf '%s' "$rows" if [ "$total_failed" -gt 0 ]; then echo "" diff --git a/.github/workflows/pr-comment-warnings.yml b/.github/workflows/pr-comment-warnings.yml index 15aa355c05c..b76a389add4 100644 --- a/.github/workflows/pr-comment-warnings.yml +++ b/.github/workflows/pr-comment-warnings.yml @@ -215,8 +215,15 @@ jobs: if [ "$FOUND" = "true" ]; then ART_ID=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" \ --jq '.artifacts[] | select(.name=="csharp-lint-reports") | .id' 2>/dev/null | head -1) + # Wall time of the Lint job, from the trusted Actions API. + LINT_SECS=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" \ + --jq '[.jobs[] | select(.name=="Lint" and .completed_at != null) + | ((.completed_at|fromdateiso8601) - (.started_at|fromdateiso8601))] | first // empty' 2>/dev/null) FOOTER="[Lint run]($RUN_URL)" [ -n "$ART_ID" ] && FOOTER="$FOOTER · [full InspectCode report](https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$ART_ID)" + if [[ "$LINT_SECS" =~ ^[0-9]+$ ]]; then + FOOTER="$FOOTER · took $((LINT_SECS/60))m $((LINT_SECS%60))s" + fi FOOTER="$FOOTER" fi diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 273ee014dfa..16e492c2327 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -69,6 +69,7 @@ def _extract_member(self, member, targetpath, pwd): BUILD_LINK_INFO_PATH = 'unity_cloud_build_info.env' dashboard_url = None _build_link_info_written = False +_final_elapsed = None # (queue_secs, build_secs), set once when the build reaches a terminal status # Live PR status-comment update: the artifact above only leaves the runner when # this job ends, so the comment's build section is also written directly the @@ -662,9 +663,12 @@ def record_build_link_info(id, response_json): with open(BUILD_LINK_INFO_PATH, 'w') as f: f.write(f'BUILD_TARGET={os.getenv("TARGET")}\n') f.write(f'BUILD_ID={id}\n') - link = href or _dashboard_build_url(id) + link = href or dashboard_url or _dashboard_build_url(id) if link: f.write(f'DASHBOARD_URL={link}\n') + if _final_elapsed: + f.write(f'QUEUE_SECS={_final_elapsed[0]}\n') + f.write(f'BUILD_SECS={_final_elapsed[1]}\n') except OSError as e: print(f'Warning: could not write {BUILD_LINK_INFO_PATH}: {e}') @@ -677,6 +681,16 @@ def record_build_link_info(id, response_json): maybe_update_live_comment(id, force=bool(href)) +def record_final_elapsed(id, queue_secs, build_secs): + """Rewrite the link-info file with the final queue/build split, so the + comment's build rows can show how long each platform took. Runs after the + last poll; the artifact uploads when the job ends.""" + global _final_elapsed, _build_link_info_written + _final_elapsed = (max(0, int(queue_secs)), max(0, int(build_secs))) + _build_link_info_written = False # bypass the no-news guard for this rewrite + record_build_link_info(id, {}) + + def _github_api(path): token = os.getenv('GH_TOKEN') or os.getenv('GITHUB_TOKEN') base = os.getenv('GITHUB_API_URL', 'https://api.github.com') @@ -780,8 +794,7 @@ def upsert_live_comment(build_id, only_if_missing=False): server = os.getenv('GITHUB_SERVER_URL', 'https://github.com') run_url = f"{server}/{os.getenv('GITHUB_REPOSITORY')}/actions/runs/{os.getenv('GITHUB_RUN_ID')}" body = '\n'.join([ - f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url}) ' - f'', + f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url})', '', '| Name | Link |', '| -------- | ----------------------- |', @@ -1139,6 +1152,7 @@ def probe_latest_build(): utils.delete_build_info() print(f'Runner FINAL elapsed: queue {datetime.timedelta(seconds=int(queue_elapsed))} / build {datetime.timedelta(seconds=int(build_elapsed))}') +record_final_elapsed(id, queue_elapsed, build_elapsed) download_artifact(id) download_log(id) From 0532ad466d654d2605e693512912bb16b327d01f Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:21:06 +0200 Subject: [PATCH 16/28] ci: review-round fixes across the status-comment pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker: the composite's GITHUB_OUTPUT heredoc glued the delimiter onto non-empty cell values (printf without a trailing newline), failing the step whenever a cell had content; values now emit through a guarded helper that always terminates the line. Correctness: the survive check retries on the surviving oldest comment when its own write landed on a younger duplicate that GC will delete; reconcile probing stops after 3 consecutive confirmed checks instead of polling the comments API for the whole build; the link-info written-flag settles only on a successful write so an OSError no longer suppresses retries; the info/log artifact names take the install source from a new composite input instead of hardcoding launcher. Presentation: duration cells read "⏱ 1h 12m build + 6m queue"; tables headed "Platform | Links & timing"; in-flight badges use the readable named yellow; the header logo carries alt text (older header spellings migrate); the tests table explains Tests time vs Job time in a footnote; the automation table links its commit/report/run rows. Docs: workflow and action descriptions caught up with the five-section comment, the duration cell, direct script callers and body truncation. --- .github/actions/ci-status-comment/action.yml | 5 +- .../ci-status-comment/upsert-ci-status.sh | 40 +++++++++---- .github/actions/ucb-build-links/action.yml | 59 +++++++++++-------- .github/workflows/pr-comment-artifact-url.yml | 30 +++++----- .../workflows/pr-comment-test-failures.yml | 2 + .github/workflows/visual-regression.yml | 8 +-- scripts/cloudbuild/build.py | 17 +++++- 7 files changed, 103 insertions(+), 58 deletions(-) diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 2e07f30f837..72f1ff87d45 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.github/actions/ci-status-comment/action.yml @@ -14,7 +14,10 @@ inputs: description: Which section to replace — one of build, lint, tests, performance, automation. required: true body: - description: Markdown for this section (inline badge + message). Rendered as-is between the section markers. + description: >- + 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/
re-closed and a truncation note. required: true github-token: description: Token with pull-requests:write used to read and upsert the comment. diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 2594828e3e3..d8e6b362b00 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -1,14 +1,15 @@ #!/usr/bin/env bash # Create or update the single unified CI status comment on a PR, replacing only -# one section (build | lint | tests | performance | automation). All CI comment -# workflows call this through the ci-status-comment composite action, so the -# separate bot comments collapse into one. +# one section (build | lint | tests | performance | automation). CI comment +# workflows call this through the ci-status-comment composite action; build.py +# (live build rows) and decentraland/performance-testing run it directly. Either +# way the separate bot comments collapse into one. # -# The comment is keyed by the hidden marker and holds one -# fenced block per section: +# The comment is keyed by the hidden marker and holds the +# $HEADER heading plus one fenced block per section: # # -# ### 🚦 CI Status +# ### CI Status # …build… # …lint… # …tests… @@ -22,8 +23,8 @@ # confirm the section landed and no duplicate slipped in — retrying otherwise. set -euo pipefail -# Optional caller knobs (used by decentraland/performance-testing, which runs -# this script directly against unity-explorer's unified comment): +# Optional caller knobs for direct invocations (build.py, and +# decentraland/performance-testing writing against unity-explorer's comment): # SECTION_BODY_FILE — read the body from a file instead of $SECTION_BODY, # for bodies too large to pass comfortably via env. # NO_CREATE=1 — never create the unified comment; exit 3 when it does @@ -68,8 +69,12 @@ esac MARKER="" # The wrapper stops GitHub's renderer from auto-wrapping the logo in # a link to the image itself — the one construct its sanitizer leaves unlinked. -HEADER='### CI Status' -OLD_HEADER="### 🚦 CI Status" +HEADER='### Decentraland CI Status' +# Retired header spellings, migrated to $HEADER whenever a section write runs. +OLD_HEADERS=( + "### 🚦 CI Status" + '### CI Status' +) BOT="github-actions[bot]" START="" END="" @@ -190,8 +195,10 @@ for attempt in 1 2 3 4 5; do CURRENT_BODY="$CURRENT_BODY"$'\n\n'"$(wrap_section "$SECTION" "$(section_default "$SECTION")")" fi - # Migrate comments created while the header was still the emoji variant. - CURRENT_BODY="${CURRENT_BODY/"$OLD_HEADER"/$HEADER}" + # Migrate comments created under a retired header spelling. + for OLD_HEADER in "${OLD_HEADERS[@]}"; do + CURRENT_BODY="${CURRENT_BODY/"$OLD_HEADER"/$HEADER}" + done NEW_BODY="$(replace_section "$CURRENT_BODY")" @@ -212,6 +219,15 @@ for attempt in 1 2 3 4 5; do 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")") + # 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 + # survivor instead of declaring success on a comment about to be deleted. + if [ -n "${RIDS[0]:-}" ] && [ "$COMMENT_ID" != "${RIDS[0]}" ]; then + echo "Comment $COMMENT_ID lost the create race to ${RIDS[0]}; retrying on the survivor." + sleep $attempt + continue + fi + # Success means our section landed on the comment we wrote — nothing more. # Duplicate collapsing is best-effort cleanup (the DELETE above may lack # permission); a duplicate we could not remove must not block reporting that diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index 47e69db0b0c..2370618026a 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -12,22 +12,32 @@ inputs: github-token: description: Token used to download the run's artifacts. required: true + install-source: + description: >- + Install source half of the info-artifact name + (unity_build_info__), matching the build + workflow's install_source matrix value. + required: false + default: launcher outputs: rows: description: >- - "| Name | Link |"-shaped rows for an existing two-column table, one per - target, each pairing the GitHub job log, the Unity Cloud build page and - the Unity log artifact; empty when no valid build info was found. + Two-column table rows, one per target, each pairing the GitHub job log, + the Unity Cloud build page, the Unity log artifact and a "⏱" duration + cell; a row omits the parts whose data was absent or invalid, and rows + built purely from the Actions API (job/log links) still render when the + info artifact itself is missing. value: ${{ steps.fetch.outputs.rows }} section: description: >- - Standalone table (header + rows); empty when no valid build info was found. + Standalone table (header + rows); empty only when no row could be built. value: ${{ steps.fetch.outputs.section }} windows-cell: description: >- - The Windows row's link cell alone ("[GitHub job](…) · [Unity Cloud #N](…) · - [Unity log](…)"), for callers composing their own rows; empty when unknown. + The Windows row's cell alone ("[GitHub job](…) · [Unity Cloud #N](…) · + [Unity log](…) · ⏱ …"), for callers composing their own rows; empty when + unknown. value: ${{ steps.fetch.outputs.windows-cell }} mac-cell: description: Mac twin of windows-cell. @@ -43,6 +53,7 @@ runs: GH_TOKEN: ${{ inputs.github-token }} RUN_ID: ${{ inputs.run-id }} REPO_FULL: ${{ github.repository }} + INSTALL_SOURCE: ${{ inputs.install-source }} run: | set -euo pipefail @@ -62,7 +73,7 @@ runs: REPLY_BUILD="" if gh run download "$RUN_ID" \ --repo "$REPO_FULL" \ - --name "unity_build_info_${target}_launcher" \ + --name "unity_build_info_${target}_${INSTALL_SOURCE}" \ --dir "$dir" 2>"${dir}.err"; then REPLY_ID=$(grep -m1 '^BUILD_ID=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) REPLY_URL=$(grep -m1 '^DASHBOARD_URL=' "$dir/unity_cloud_build_info.env" | cut -d= -f2- || true) @@ -75,7 +86,7 @@ runs: else # Absence is normal for runs predating the info artifact; still surface the # gh error so an auth/permission regression doesn't silently eat the rows. - echo "note: could not fetch unity_build_info_${target}_launcher: $(tr '\n' ' ' < "${dir}.err")" + echo "note: could not fetch unity_build_info_${target}_${INSTALL_SOURCE}: $(tr '\n' ' ' < "${dir}.err")" fi } @@ -105,7 +116,7 @@ runs: label="${entry#*:}" parse_info "$target" job_url=$(jq -r --arg n "Build ($target)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1) - log_id=$(jq -r --arg n "${target}_launcher_unity_log" \ + log_id=$(jq -r --arg n "${target}_${INSTALL_SOURCE}_unity_log" \ '.artifacts[]? | select(.name==$n and .expired==false) | .id' <<< "$ARTIFACTS_JSON" | head -1) [[ "$log_id" =~ ^[0-9]+$ ]] || log_id="" @@ -124,8 +135,8 @@ runs: parts+=("[Unity log](${GITHUB_SERVER_URL:-https://github.com}/${REPO_FULL}/suites/${SUITE_ID}/artifacts/${log_id})") fi if [ -n "$REPLY_BUILD" ]; then - t="⏱ $(fmt_dur "$REPLY_BUILD")" - [ -n "$REPLY_QUEUE" ] && [ "$REPLY_QUEUE" -gt 0 ] && t+=" ($(fmt_dur "$REPLY_QUEUE") queued)" + t="⏱ $(fmt_dur "$REPLY_BUILD") build" + [ -n "$REPLY_QUEUE" ] && [ "$REPLY_QUEUE" -gt 0 ] && t+=" + $(fmt_dur "$REPLY_QUEUE") queue" parts+=("$t") fi @@ -143,23 +154,23 @@ runs: SECTION="" if [ -n "$ROWS" ]; then - SECTION="| Name | Link |"$'\n'"| -------- | ----------------------- |"$'\n'"$ROWS" + SECTION="| Platform | Links & timing |"$'\n'"| -------- | ----------------------- |"$'\n'"$ROWS" fi # The payload derives from artifact bytes, so the heredoc delimiter must not be # guessable content even though the validation above already forbids newlines. + # Every value is emitted with exactly one trailing newline — a value glued to + # the delimiter line would make the runner miss the terminator entirely. DELIM="UCB_EOF_${RANDOM}${RANDOM}_$$" - { - echo "rows<<${DELIM}" - printf '%s' "$ROWS" - echo "${DELIM}" - echo "section<<${DELIM}" - printf '%s' "$SECTION" - echo "${DELIM}" - echo "windows-cell<<${DELIM}" - printf '%s' "$WINDOWS_CELL" - echo "${DELIM}" - echo "mac-cell<<${DELIM}" - printf '%s' "$MAC_CELL" + emit() { + local name="$1" val="$2" + echo "${name}<<${DELIM}" + if [ -n "$val" ]; then printf '%s\n' "${val%$'\n'}"; fi echo "${DELIM}" + } + { + emit rows "$ROWS" + emit section "$SECTION" + emit windows-cell "$WINDOWS_CELL" + emit mac-cell "$MAC_CELL" } >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index b2ae67ba30c..0200f0be763 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -2,8 +2,9 @@ --- name: Comment Artifact URL on PR -# Writes only the "build" section of the unified CI status comment via the -# ci-status-comment composite action (build / lint / tests live in one comment). +# Writes the "build" section of the unified CI status comment via the +# ci-status-comment composite action, plus the "performance" section's dispatch +# status (the comment holds build / lint / tests / performance / automation). # # 'requested' -> reset the build section to "pending" the moment a new build # starts, so last build's download links never linger as stale. @@ -61,7 +62,7 @@ jobs: section: build github-token: ${{ github.token }} body: |- - [![Build](https://img.shields.io/badge/Build-Pending!-ffff00?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) + [![Build](https://img.shields.io/badge/Build-Pending!-yellow?logo=github&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}) New build in progress — per-platform Unity Cloud links land here as soon as each build is created. @@ -257,10 +258,12 @@ jobs: with: run-id: ${{ env.PREVIOUS_JOB_ID }} github-token: ${{ github.token }} + install-source: launcher - # One row per platform: GitHub job · Unity Cloud build · Unity log · zip - # downloads. Composed here rather than inline in the body so a link whose - # id could not be resolved is dropped instead of rendering broken. + # One row per platform: GitHub job · Unity Cloud build · Unity log · + # build duration · zip downloads. Composed here rather than inline in the + # body so a link whose id could not be resolved is dropped instead of + # rendering broken. - name: Compose platform rows env: WINDOWS_CELL: ${{ steps.ucb.outputs.windows-cell }} @@ -296,7 +299,7 @@ jobs: Windows and Mac built successfully in Unity Cloud. - | Name | Link | + | Name | Links & timing | | -------- | ----------------------- | | Build | [`${{ env.SHORT_SHA }}`](${{ github.server_url }}/${{ github.repository }}/commit/${{ env.HEAD_SHA }}) · [Logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ env.PREVIOUS_JOB_ID }}) · built ${{ env.BUILD_DATE }} | ${{ env.PLATFORM_ROWS }} @@ -331,8 +334,7 @@ jobs: # The PAT's remaining lifetime comes back as a response header on any API # call it makes; surfacing it in the status comment replaces a separate - # expiry canary — the 2026-06..08 outage was exactly this token expiring - # with no warning anywhere a human looks. + # expiry canary — an expiring token must warn somewhere a human looks. - name: Probe performance PAT expiry # Purely decorative — its whole output is one optional sentence, so it # must never gate the dispatched/dispatch-failed section writes below. @@ -374,16 +376,15 @@ jobs: section: performance github-token: ${{ github.token }} body: |- - [![Performance](https://img.shields.io/badge/Performance-Dispatched!-ffff00?logo=speedtest&logoColor=white&style=for-the-badge)](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml) + [![Performance](https://img.shields.io/badge/Performance-Dispatched!-yellow?logo=speedtest&logoColor=white&style=for-the-badge)](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml) 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. ${{ env.PAT_EXPIRY_WARNING }} - # A failing dispatch (an expired PERFORMANCE_TESTING_PAT is the historical - # cause — it broke silently from 2026-06-15 to 2026-08-13) turns this job - # red but leaves no trace on the PR; surface it in the performance section - # so it cannot go unnoticed for weeks again. + # A failing dispatch (e.g. an expired PERFORMANCE_TESTING_PAT) turns this + # job red but leaves no trace on the PR; surface it in the performance + # section so it cannot go unnoticed. # outcome != 'success' rather than == 'failure': anything failing between # the checkout and the dispatch (Find latest release, the build-section # upsert) skips perf_dispatch, and a skipped dispatch is the same silence @@ -420,6 +421,7 @@ jobs: with: run-id: ${{ github.event.workflow_run.id }} github-token: ${{ github.token }} + install-source: launcher - name: Update build section uses: ./.github/actions/ci-status-comment diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index e01a5e8c158..b8c8b941944 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -173,6 +173,8 @@ jobs: echo "| TESTS SUITE | Result | Passed | Failed | Skipped | Tests time | Job time |" echo "| ----------- | ------ | -----: | -----: | ------: | ---: | ---: |" printf '%s' "$rows" + echo "" + echo "Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import." if [ "$total_failed" -gt 0 ]; then echo "" echo "
Failed tests ($total_failed)" diff --git a/.github/workflows/visual-regression.yml b/.github/workflows/visual-regression.yml index 050ca43a5aa..d6750c69dc3 100644 --- a/.github/workflows/visual-regression.yml +++ b/.github/workflows/visual-regression.yml @@ -191,7 +191,7 @@ jobs: section: automation github-token: ${{ github.token }} body: |- - [![Automation](https://img.shields.io/badge/Automation-Running!-ffff00?logo=github&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) + [![Automation](https://img.shields.io/badge/Automation-Running!-yellow?logo=github&logoColor=white&style=for-the-badge)](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) Visual regression suite running for commit `${{ needs.resolve.outputs.head_short_sha }}` — [watch the run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}). @@ -265,9 +265,9 @@ jobs: echo "" echo "| Name | Link |" echo "| -------- | ----------------------- |" - echo "| Commit | \`$COMMIT_SHA\` |" - echo "| Allure report | $REPORT_URL |" - echo "| Workflow run | $RUN_URL |" + echo "| Commit | [\`$COMMIT_SHA\`](${GITHUB_SERVER_URL:-https://github.com}/${REPO}/commit/${COMMIT_SHA}) |" + echo "| Allure report | [Open report]($REPORT_URL) |" + echo "| Workflow run | [View run]($RUN_URL) |" echo "" echo "Triggered via \`/visual-tests\` · the detailed per-platform comment is posted separately." echo "$DELIM" diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 16e492c2327..5772e95b439 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -79,6 +79,7 @@ def _extract_member(self, member, targetpath, pwd): LIVE_MARKER_PREFIX = '' <<< "$BODY" || fail "create: performance fence missing" +grep -q 'alt="Decentraland"' <<< "$BODY" || fail "create: header logo missing" +pass "create seeds skeleton with all sections" + +# --- 2. section update preserves the others --------------------------------- +run_upsert tests "TESTS-CONTENT" >/dev/null +BODY="$(body_of)" +grep -q 'BUILD-CONTENT' <<< "$BODY" || fail "update: build content lost" +grep -q 'TESTS-CONTENT' <<< "$BODY" || fail "update: tests content missing" +pass "section update preserves other sections" + +# --- 3. missing fence appended, others intact -------------------------------- +reset_store "$(python3 - <<'PY' +import json +body = "\n### 🚦 CI Status\n\nOLD-BUILD\n" +print(json.dumps([{"id": 5, "user": {"login": "github-actions[bot]"}, "body": body}])) +PY +)" +run_upsert automation "AUTO-CONTENT" >/dev/null +BODY="$(body_of)" +grep -q 'OLD-BUILD' <<< "$BODY" || fail "append: existing section wiped" +grep -q 'AUTO-CONTENT' <<< "$BODY" || fail "append: new section missing" +grep -q 'alt="Decentraland"' <<< "$BODY" || fail "append: emoji header not migrated" +grep -q '🚦' <<< "$BODY" && fail "append: old emoji header still present" +pass "missing fence appended + header migrated" + +# --- 4. marker-shaped body lines are stripped -------------------------------- +reset_store +run_upsert build "$(printf 'SAFE\n\nALSO-SAFE')" >/dev/null +BODY="$(body_of)" +[ "$(grep -c '' <<< "$BODY")" = 1 ] || fail "strip: injected fence survived" +grep -q 'ALSO-SAFE' <<< "$BODY" || fail "strip: legitimate line lost" +pass "marker-shaped body lines stripped" + +# --- 5. duplicate GC keeps the oldest ---------------------------------------- +reset_store "$(python3 - <<'PY' +import json +mk = lambda i: {"id": i, "user": {"login": "github-actions[bot]"}, + "body": "\nhdr\n\nB%d\n" % i} +print(json.dumps([mk(3), mk(9)])) +PY +)" +run_upsert build "DEDUPED" >/dev/null +[ "$(count)" = 1 ] || fail "gc: duplicate not deleted" +grep -q 'DEDUPED' <<< "$(body_of)" || fail "gc: content missing on survivor" +pass "duplicate collapse keeps one comment with the write" + +# --- 6. NO_CREATE exits 3 without creating ----------------------------------- +reset_store +set +e +(cd "$WORK" && SECTION=performance SECTION_BODY=X NO_CREATE=1 bash "$UPSERT") >/dev/null 2>&1 +RC=$? +set -e +[ "$RC" = 3 ] || fail "no-create: expected exit 3, got $RC" +[ "$(count)" = 0 ] || fail "no-create: comment was created" +pass "NO_CREATE exits 3, creates nothing" + +# --- 7. unknown section exits 2 ---------------------------------------------- +set +e +(cd "$WORK" && SECTION=bogus SECTION_BODY=X bash "$UPSERT") >/dev/null 2>&1 +RC=$? +set -e +[ "$RC" = 2 ] || fail "allowlist: expected exit 2, got $RC" +pass "unknown section exits 2" + +# --- 8. oversized body truncates and re-closes constructs -------------------- +reset_store +BIG="$WORK/big-body.md" +{ + echo '
big' + echo '```' + for i in $(seq 1 3000); do echo "line $i of filler to overflow the cap"; done +} > "$BIG" +(cd "$WORK" && SECTION=tests SECTION_BODY= SECTION_BODY_FILE="$BIG" bash "$UPSERT") >/dev/null +BODY="$(body_of)" +grep -q 'truncated' <<< "$BODY" || fail "truncate: no truncation note" +[ "$(( $(grep -c '^```' <<< "$BODY") % 2 ))" = 0 ] || fail "truncate: unbalanced code fence" +pass "oversized body truncated with constructs closed" + +[ "$FAILED" = 0 ] && echo "ALL PASS" || { echo "FAILURES PRESENT"; exit 1; } diff --git a/.github/workflows/ci-scripts-tests.yml b/.github/workflows/ci-scripts-tests.yml new file mode 100644 index 00000000000..b12be320beb --- /dev/null +++ b/.github/workflows/ci-scripts-tests.yml @@ -0,0 +1,34 @@ +# ci-scripts-tests.yml +--- +name: CI Scripts Tests + +# Tests for the CI plumbing itself: build.py's pure helpers (including the +# URL_RE drift guard against ucb-build-links) and the unified status comment's +# upsert script against a stubbed gh. Cheap and network-free, so it runs on +# any PR touching these paths. +on: + pull_request: + paths: + - "scripts/cloudbuild/**" + - ".github/actions/ci-status-comment/**" + - ".github/actions/ucb-build-links/**" + - ".github/workflows/ci-scripts-tests.yml" + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + persist-credentials: false + + - name: Unit tests (build.py helpers) + run: | + pip install --quiet requests + python3 -m unittest discover -s scripts/cloudbuild -v + + - name: Functional tests (upsert-ci-status.sh, stubbed gh) + run: bash .github/actions/ci-status-comment/test-upsert-ci-status.sh diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 0200f0be763..941fb8d3114 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -380,6 +380,8 @@ jobs: 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. + Latest build wins this section — it also replaces any earlier `perf_test`-suite verdict. + ${{ env.PAT_EXPIRY_WARNING }} # A failing dispatch (e.g. an expired PERFORMANCE_TESTING_PAT) turns this diff --git a/.github/workflows/pr-comment-perf.yml b/.github/workflows/pr-comment-perf.yml index b0f1af69122..6b3f7081d2b 100644 --- a/.github/workflows/pr-comment-perf.yml +++ b/.github/workflows/pr-comment-perf.yml @@ -94,6 +94,8 @@ jobs: echo "" echo "$MSG The benchmark report is rendered on the [run summary]($RUN_URL)." echo "" + echo "Written by the \`perf_test\`-label suite; the next build's benchmark dispatch replaces it." + echo "" if [ -n "$links" ]; then echo "Download:${links#" ·"}" else diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index b8c8b941944..6e4fb0b0ae6 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -78,8 +78,11 @@ jobs: JOBS_JSON=$(gh api "/repos/$REPO/actions/runs/$WORKFLOW_RUN_ID/jobs?per_page=100" 2>/dev/null || echo '{"jobs":[]}') ARTS_JSON=$(gh api "/repos/$REPO/actions/runs/$WORKFLOW_RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') + # Same h/m/s convention as fmt_dur in ucb-build-links/action.yml and the + # lint footer in pr-comment-warnings.yml — keep the three in lockstep. fmt_secs() { - awk -v s="$1" 'BEGIN { s=int(s+0.5); m=int(s/60); r=s%60; if (m>0) printf("%dm %02ds", m, r); else printf("%ds", r) }' + awk -v s="$1" 'BEGIN { s=int(s+0.5); h=int(s/3600); m=int(s%3600/60); r=s%60; + if (h>0) printf("%dh %dm", h, m); else if (m>0) printf("%dm %ds", m, r); else printf("%ds", r) }' } declare -A DISPLAY=( [editmode]=EditMode [playmode]=PlayMode ) diff --git a/.github/workflows/pr-comment-warnings.yml b/.github/workflows/pr-comment-warnings.yml index b76a389add4..54f27e88060 100644 --- a/.github/workflows/pr-comment-warnings.yml +++ b/.github/workflows/pr-comment-warnings.yml @@ -62,7 +62,7 @@ jobs: section: lint github-token: ${{ github.token }} body: |- - [![Lint](https://img.shields.io/badge/Lint-Pending!-ffff00?logo=jetbrains&logoColor=white&style=for-the-badge)](${{ github.event.workflow_run.html_url }}) + [![Lint](https://img.shields.io/badge/Lint-Pending!-yellow?logo=jetbrains&logoColor=white&style=for-the-badge)](${{ github.event.workflow_run.html_url }}) Lint in progress, come back later! @@ -222,7 +222,11 @@ jobs: FOOTER="[Lint run]($RUN_URL)" [ -n "$ART_ID" ] && FOOTER="$FOOTER · [full InspectCode report](https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$ART_ID)" if [[ "$LINT_SECS" =~ ^[0-9]+$ ]]; then - FOOTER="$FOOTER · took $((LINT_SECS/60))m $((LINT_SECS%60))s" + # Same h/m/s convention as fmt_dur in ucb-build-links/action.yml. + if [ "$LINT_SECS" -ge 3600 ]; then took="$((LINT_SECS/3600))h $((LINT_SECS%3600/60))m" + elif [ "$LINT_SECS" -ge 60 ]; then took="$((LINT_SECS/60))m $((LINT_SECS%60))s" + else took="${LINT_SECS}s"; fi + FOOTER="$FOOTER · took $took" fi FOOTER="$FOOTER" fi diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 5772e95b439..85608546489 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -1026,156 +1026,158 @@ def run_poll_loop(id, build_already_active=False, resumed_build_elapsed=0): time.sleep(poll_interval) -args = parser.parse_args() -build_already_active = False -resumed_build_elapsed = 0 +if __name__ == '__main__': + args = parser.parse_args() -if args.delete: - delete_current_target() -elif args.resume or args.cancel: - build_info = utils.read_build_info() - if build_info is None: - sys.exit(1) + build_already_active = False + resumed_build_elapsed = 0 + + if args.delete: + delete_current_target() + elif args.resume or args.cancel: + build_info = utils.read_build_info() + if build_info is None: + sys.exit(1) + + os.environ['TARGET'] = build_info["target"] + id = build_info["id"] + + if args.cancel: + if id is None: + # The runner died between the build POST and the id write; the queued build is + # findable only as the target's latest build. Cancel it only while it is still + # in a queue status: targets are shared (release pool; consecutive runs on one + # branch), so an already-started build may belong to a concurrent run — leaving + # it is at worst one wasted build, cancelling it would kill someone else's. + # A missing/unknown status is treated as not-cancellable for the same reason. + latest = get_latest_build(os.getenv('TARGET')) + if latest and latest.get('buildStatus') in QUEUE_STATUSES: + id = latest['build'] + print(f'No build id persisted; cancelling latest queued build #{id} on {os.getenv("TARGET")}') + else: + print('No build id persisted and no queued build found; nothing to cancel.') + utils.delete_build_info() + sys.exit(0) + cancel_build(id) + utils.delete_build_info() + sys.exit(0) - os.environ['TARGET'] = build_info["target"] - id = build_info["id"] - - if args.cancel: - if id is None: - # The runner died between the build POST and the id write; the queued build is - # findable only as the target's latest build. Cancel it only while it is still - # in a queue status: targets are shared (release pool; consecutive runs on one - # branch), so an already-started build may belong to a concurrent run — leaving - # it is at worst one wasted build, cancelling it would kill someone else's. - # A missing/unknown status is treated as not-cancellable for the same reason. - latest = get_latest_build(os.getenv('TARGET')) - if latest and latest.get('buildStatus') in QUEUE_STATUSES: - id = latest['build'] - print(f'No build id persisted; cancelling latest queued build #{id} on {os.getenv("TARGET")}') - else: - print('No build id persisted and no queued build found; nothing to cancel.') - utils.delete_build_info() - sys.exit(0) - cancel_build(id) - utils.delete_build_info() - sys.exit(0) - -else: - branch_name = os.getenv('BRANCH_NAME') - validate_branch_name(branch_name) - - resumed = try_resume_build() - if resumed is not None: - target_name, id, resumed_status, resumed_elapsed = resumed - os.environ['TARGET'] = target_name - build_already_active = resumed_status in ACTIVE_STATUSES - if build_already_active: - resumed_build_elapsed = resumed_elapsed else: - try: - clone_current_target(True) - except Exception as e: - print(f"Operation failed: {e}") - - # Set parameters immediately before run_build to avoid races with concurrent - # builds on shared targets. - set_parameters(get_param_env_variables()) - - def get_clean_build_bool(): - value = os.getenv('CLEAN_BUILD', 'false').lower() - if value in ['true', '1']: - return True - elif value in ['false', '0']: - return False - else: - raise ValueError(f"Invalid boolean value for CLEAN_BUILD: {value}") - - # Persist the target before the POST: if the runner dies mid-request, --cancel can still - # find the queued build via the target's latest-build lookup. - utils.persist_build_info(os.getenv('TARGET'), None) - id = run_build(os.getenv('BRANCH_NAME'), get_clean_build_bool()) - utils.persist_build_info(os.getenv('TARGET'), id) - # Write the link info file (target + id, no URL yet) immediately so it exists - # even if the runner dies before the first poll; the poll loop upgrades it - # with the dashboard URL once a response carries one. - record_build_link_info(id, {}) - print(f'For more info and live logs, go to https://cloud.unity.com/ and search for target "{os.getenv('TARGET')}" and build ID "{id}"') - -final_outcome, phase_durations, queue_reasons, queue_elapsed, build_elapsed = run_poll_loop( - id, - build_already_active=build_already_active, - resumed_build_elapsed=resumed_build_elapsed, -) -write_step_summary(os.getenv('TARGET'), id, final_outcome, phase_durations, queue_reasons, queue_elapsed, build_elapsed) - -if final_outcome in ('queue_timeout', 'build_timeout', 'log_stall'): - if final_outcome in ('build_timeout', 'log_stall'): - # Build was cancelled; the persisted info points to a dead build. - # Delete it so the next retry creates a fresh build on a different VM. + branch_name = os.getenv('BRANCH_NAME') + validate_branch_name(branch_name) + + resumed = try_resume_build() + if resumed is not None: + target_name, id, resumed_status, resumed_elapsed = resumed + os.environ['TARGET'] = target_name + build_already_active = resumed_status in ACTIVE_STATUSES + if build_already_active: + resumed_build_elapsed = resumed_elapsed + else: + try: + clone_current_target(True) + except Exception as e: + print(f"Operation failed: {e}") + + # Set parameters immediately before run_build to avoid races with concurrent + # builds on shared targets. + set_parameters(get_param_env_variables()) + + def get_clean_build_bool(): + value = os.getenv('CLEAN_BUILD', 'false').lower() + if value in ['true', '1']: + return True + elif value in ['false', '0']: + return False + else: + raise ValueError(f"Invalid boolean value for CLEAN_BUILD: {value}") + + # Persist the target before the POST: if the runner dies mid-request, --cancel can still + # find the queued build via the target's latest-build lookup. + utils.persist_build_info(os.getenv('TARGET'), None) + id = run_build(os.getenv('BRANCH_NAME'), get_clean_build_bool()) + utils.persist_build_info(os.getenv('TARGET'), id) + # Write the link info file (target + id, no URL yet) immediately so it exists + # even if the runner dies before the first poll; the poll loop upgrades it + # with the dashboard URL once a response carries one. + record_build_link_info(id, {}) + print(f'For more info and live logs, go to https://cloud.unity.com/ and search for target "{os.getenv('TARGET')}" and build ID "{id}"') + + final_outcome, phase_durations, queue_reasons, queue_elapsed, build_elapsed = run_poll_loop( + id, + build_already_active=build_already_active, + resumed_build_elapsed=resumed_build_elapsed, + ) + write_step_summary(os.getenv('TARGET'), id, final_outcome, phase_durations, queue_reasons, queue_elapsed, build_elapsed) + + if final_outcome in ('queue_timeout', 'build_timeout', 'log_stall'): + if final_outcome in ('build_timeout', 'log_stall'): + # Build was cancelled; the persisted info points to a dead build. + # Delete it so the next retry creates a fresh build on a different VM. + utils.delete_build_info() + try: + download_log(id) + except Exception as e: + print(f'Warning: could not download log after {final_outcome}: {e}') + sys.exit(RETRYABLE_EXIT_CODE) + + if final_outcome == 'canceled': + # This run's own cancellations exit through the watchdog/timeout branches above, + # so 'canceled' here came from outside. Two different outsides, though: + # - UBA giving up on builder provisioning (observed: 9 min in sentToBuilder, then a + # platform-side cancel) — nothing else wants the target, so retry on a fresh build; + # - a concurrent run superseding us via run_build's `already a build pending` cancel. + # main/release/*/hotfix/* share one target but sit in different concurrency groups, + # so re-POSTing here would cancel *their* build and hand them the same exit 99 — + # both runs then burn a full queue+build cycle and one still ends red. + # Build numbers are monotonic per target: a newer build means we were superseded. + def probe_latest_build(): + # Fail-open: a transient socket error here must not traceback past the + # cleanup below - it degrades to the retry path, same as a non-200 probe. + try: + return get_latest_build(os.getenv('TARGET')) + except requests.exceptions.RequestException as e: + print(f'Warning: latest-build probe failed ({e})') + return None + + latest = probe_latest_build() + if latest and int(latest.get('build') or 0) <= int(id): + # run_build cancels the pending build and only re-POSTs ~30 s later, so a + # supersede can be invisible for that gap. Re-probe once past it before + # deciding to retry. + time.sleep(35) + latest = probe_latest_build() or latest utils.delete_build_info() try: download_log(id) except Exception as e: - print(f'Warning: could not download log after {final_outcome}: {e}') - sys.exit(RETRYABLE_EXIT_CODE) - -if final_outcome == 'canceled': - # This run's own cancellations exit through the watchdog/timeout branches above, - # so 'canceled' here came from outside. Two different outsides, though: - # - UBA giving up on builder provisioning (observed: 9 min in sentToBuilder, then a - # platform-side cancel) — nothing else wants the target, so retry on a fresh build; - # - a concurrent run superseding us via run_build's `already a build pending` cancel. - # main/release/*/hotfix/* share one target but sit in different concurrency groups, - # so re-POSTing here would cancel *their* build and hand them the same exit 99 — - # both runs then burn a full queue+build cycle and one still ends red. - # Build numbers are monotonic per target: a newer build means we were superseded. - def probe_latest_build(): - # Fail-open: a transient socket error here must not traceback past the - # cleanup below - it degrades to the retry path, same as a non-200 probe. - try: - return get_latest_build(os.getenv('TARGET')) - except requests.exceptions.RequestException as e: - print(f'Warning: latest-build probe failed ({e})') - return None - - latest = probe_latest_build() - if latest and int(latest.get('build') or 0) <= int(id): - # run_build cancels the pending build and only re-POSTs ~30 s later, so a - # supersede can be invisible for that gap. Re-probe once past it before - # deciding to retry. - time.sleep(35) - latest = probe_latest_build() or latest - utils.delete_build_info() - try: - download_log(id) - except Exception as e: - print(f'Warning: could not download log after external cancel: {e}') - if latest and int(latest.get('build') or 0) > int(id): - print( - f'Build {id} was superseded by #{latest["build"]} on shared target ' - f'{os.getenv("TARGET")} - not retrying (the successor owns the slot).' - ) - sys.exit(1) - print('Build was canceled outside this run - retrying with a fresh build.') - sys.exit(RETRYABLE_EXIT_CODE) + print(f'Warning: could not download log after external cancel: {e}') + if latest and int(latest.get('build') or 0) > int(id): + print( + f'Build {id} was superseded by #{latest["build"]} on shared target ' + f'{os.getenv("TARGET")} - not retrying (the successor owns the slot).' + ) + sys.exit(1) + print('Build was canceled outside this run - retrying with a fresh build.') + sys.exit(RETRYABLE_EXIT_CODE) -utils.delete_build_info() + utils.delete_build_info() -print(f'Runner FINAL elapsed: queue {datetime.timedelta(seconds=int(queue_elapsed))} / build {datetime.timedelta(seconds=int(build_elapsed))}') -record_final_elapsed(id, queue_elapsed, build_elapsed) + print(f'Runner FINAL elapsed: queue {datetime.timedelta(seconds=int(queue_elapsed))} / build {datetime.timedelta(seconds=int(build_elapsed))}') + record_final_elapsed(id, queue_elapsed, build_elapsed) -download_artifact(id) -download_log(id) + download_artifact(id) + download_log(id) -if not build_healthy: - where = dashboard_url or _dashboard_build_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' - print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') - sys.exit(1) + if not build_healthy: + where = dashboard_url or _dashboard_build_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' + print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') + sys.exit(1) -# Cleanup (only if build is healthy and not release) -# We only delete all artifacts, not the build target -if not is_release_workflow: - delete_build(id) + # Cleanup (only if build is healthy and not release) + # We only delete all artifacts, not the build target + if not is_release_workflow: + delete_build(id) -utils.delete_build_info() + utils.delete_build_info() diff --git a/scripts/cloudbuild/test_build_helpers.py b/scripts/cloudbuild/test_build_helpers.py new file mode 100644 index 00000000000..91d665d3734 --- /dev/null +++ b/scripts/cloudbuild/test_build_helpers.py @@ -0,0 +1,142 @@ +"""Unit tests for build.py's pure helpers and the link-info file writer. + +Run from anywhere: python3 -m unittest scripts.cloudbuild.test_build_helpers +(or `python3 -m unittest discover -s scripts/cloudbuild`). build.py's build +flow is under a __main__ guard, so importing it here executes nothing. +""" +import os +import re +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import build # noqa: E402 + +REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..')) +UCB_LINKS_ACTION = os.path.join(REPO_ROOT, '.github', 'actions', 'ucb-build-links', 'action.yml') + + +class EnvMixin: + def set_env(self, **pairs): + for key, value in pairs.items(): + old = os.environ.get(key) + self.addCleanup( + (lambda k, v: (os.environ.__setitem__(k, v) if v is not None else os.environ.pop(k, None))) + , key, old) + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + + +class PlatformKeyTest(EnvMixin, unittest.TestCase): + def check(self, target, expected): + self.set_env(TARGET=target) + self.assertEqual(build._platform_key(), expected) + + def test_template_targets(self): + self.check('t_windows64', 'windows64') + self.check('t_macos', 'macos') + + def test_branch_derived_targets(self): + self.check('windows64-feat-unity-cloud-build-link', 'windows64') + self.check('macos-release-epic', 'macos') + + def test_unknown(self): + self.check('linux64-foo', 'linux64-foo') + self.check('', 'unknown') + + +class DashboardUrlTest(EnvMixin, unittest.TestCase): + ENV = dict(ORG_ID='4673197905245', + PROJECT_ID='8c12744f-9e98-47b8-b40c-576d04cb8d5c', + TARGET='windows64-some-branch') + + def test_shape(self): + self.set_env(**self.ENV) + self.assertEqual( + build._dashboard_build_url(15), + 'https://cloud.unity.com/home/organizations/4673197905245' + '/projects/8c12744f-9e98-47b8-b40c-576d04cb8d5c' + '/buildtargets/windows64-some-branch/builds/15'.replace( + '/buildtargets', '/cloud-build/buildtargets')) + + def test_missing_env_returns_none(self): + for absent in ('ORG_ID', 'PROJECT_ID', 'TARGET'): + env = dict(self.ENV) + env[absent] = None + self.set_env(**env) + self.assertIsNone(build._dashboard_build_url(15), f'{absent} unset') + + def test_matches_consumer_url_re(self): + """Drift guard: the consumer drops URLs failing its allowlist silently, + so the producer's constructed URL must always pass it.""" + with open(UCB_LINKS_ACTION) as f: + match = re.search(r"URL_RE='([^']+)'", f.read()) + self.assertIsNotNone(match, 'URL_RE not found in ucb-build-links/action.yml') + url_re = re.compile(match.group(1)) + self.set_env(**self.ENV) + url = build._dashboard_build_url(42) + self.assertRegex(url, url_re) + # And the producer's own qualifying filter for API-returned links. + self.assertTrue(url.startswith('https://') and '/builds/' in url) + + +class LinkInfoFileTest(EnvMixin, unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + old_cwd = os.getcwd() + self.addCleanup(os.chdir, old_cwd) + os.chdir(tmp.name) + # 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 + build._build_link_info_written = False + build._final_elapsed = None + self.addCleanup(self._reset_module_state) + + @staticmethod + def _reset_module_state(): + build.dashboard_url = None + build._build_link_info_written = False + build._final_elapsed = None + + @staticmethod + def read_info(): + with open(build.BUILD_LINK_INFO_PATH) as f: + return dict(line.strip().split('=', 1) for line in f if '=' in line) + + def test_first_write_uses_constructed_url(self): + build.record_build_link_info(7, {}) + info = self.read_info() + self.assertEqual(info['BUILD_ID'], '7') + self.assertEqual(info['DASHBOARD_URL'], build._dashboard_build_url(7)) + self.assertNotIn('QUEUE_SECS', info) + + def test_api_href_replaces_constructed_and_survives_final_rewrite(self): + build.record_build_link_info(7, {}) + href = 'https://cloud.unity.com/some/deep/builds/7/link' + build.record_build_link_info(7, {'links': {'dashboard_summary': {'href': href}}}) + self.assertEqual(self.read_info()['DASHBOARD_URL'], href) + + build.record_final_elapsed(7, 63, 3725) + info = self.read_info() + self.assertEqual(info['DASHBOARD_URL'], href, 'final rewrite must keep the API deep link') + self.assertEqual(info['QUEUE_SECS'], '63') + self.assertEqual(info['BUILD_SECS'], '3725') + + def test_non_build_link_rejected(self): + build.record_build_link_info(7, {'links': {'dashboard_url': {'href': 'https://cloud.unity.com/'}}}) + self.assertEqual(self.read_info()['DASHBOARD_URL'], build._dashboard_build_url(7)) + + def test_final_elapsed_clamps_negative(self): + build.record_final_elapsed(7, -5, -1) + info = self.read_info() + self.assertEqual(info['QUEUE_SECS'], '0') + self.assertEqual(info['BUILD_SECS'], '0') + + +if __name__ == '__main__': + unittest.main() From 63a0cf47c937a3686f56abf6acb947bb6095639f Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:05:20 +0200 Subject: [PATCH 18/28] ci: drop the logo from the CI status comment header Back to the plain emoji header; both picture spellings join the retired list so existing comments migrate on their next section write. Co-Authored-By: Claude Fable 5 --- .github/actions/ci-status-comment/upsert-ci-status.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index d8e6b362b00..3ca0f9e84df 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -67,12 +67,10 @@ case "${SECTION:-}" in esac MARKER="" -# The wrapper stops GitHub's renderer from auto-wrapping the logo in -# a link to the image itself — the one construct its sanitizer leaves unlinked. -HEADER='### Decentraland CI Status' +HEADER='### 🚦 CI Status' # Retired header spellings, migrated to $HEADER whenever a section write runs. OLD_HEADERS=( - "### 🚦 CI Status" + '### Decentraland CI Status' '### CI Status' ) BOT="github-actions[bot]" From a9f8e935e631475da2147975a480318e013a3791 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:19:14 +0200 Subject: [PATCH 19/28] test: follow the header back to the emoji spelling The upsert tests still asserted the retired logo header; migration now runs picture->emoji, so the create case asserts the emoji header and the append case seeds a picture-headed comment and asserts it migrates. Co-Authored-By: Claude Fable 5 --- .../ci-status-comment/test-upsert-ci-status.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/actions/ci-status-comment/test-upsert-ci-status.sh b/.github/actions/ci-status-comment/test-upsert-ci-status.sh index a244b6fd3ec..d20d31005c3 100644 --- a/.github/actions/ci-status-comment/test-upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/test-upsert-ci-status.sh @@ -91,7 +91,8 @@ run_upsert build "BUILD-CONTENT" >/dev/null BODY="$(body_of)" grep -q 'BUILD-CONTENT' <<< "$BODY" || fail "create: build content missing" grep -q '' <<< "$BODY" || fail "create: performance fence missing" -grep -q 'alt="Decentraland"' <<< "$BODY" || fail "create: header logo missing" +grep -q '🚦 CI Status' <<< "$BODY" || fail "create: emoji header missing" +grep -q 'decentraland_256x256' <<< "$BODY" && fail "create: retired logo header present" pass "create seeds skeleton with all sections" # --- 2. section update preserves the others --------------------------------- @@ -104,7 +105,9 @@ pass "section update preserves other sections" # --- 3. missing fence appended, others intact -------------------------------- reset_store "$(python3 - <<'PY' import json -body = "\n### 🚦 CI Status\n\nOLD-BUILD\n" +body = ('\n### Decentraland CI Status\n' + "\nOLD-BUILD\n") print(json.dumps([{"id": 5, "user": {"login": "github-actions[bot]"}, "body": body}])) PY )" @@ -112,8 +115,8 @@ run_upsert automation "AUTO-CONTENT" >/dev/null BODY="$(body_of)" grep -q 'OLD-BUILD' <<< "$BODY" || fail "append: existing section wiped" grep -q 'AUTO-CONTENT' <<< "$BODY" || fail "append: new section missing" -grep -q 'alt="Decentraland"' <<< "$BODY" || fail "append: emoji header not migrated" -grep -q '🚦' <<< "$BODY" && fail "append: old emoji header still present" +grep -q '🚦 CI Status' <<< "$BODY" || fail "append: logo header not migrated" +grep -q 'decentraland_256x256' <<< "$BODY" && fail "append: retired logo header still present" pass "missing fence appended + header migrated" # --- 4. marker-shaped body lines are stripped -------------------------------- From 536e961a7743b6914c3239aadd6a6c549596c376 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:19:44 +0200 Subject: [PATCH 20/28] ci: close review findings 2-13 across the status-comment pipeline - warnings: LINT_SECS degrades to no-duration on an API failure; the false no-set-e comment now states the real default shell; jq guards null started_at (also in test-failures' job_secs) - ci-scripts-tests installs from requirements.txt (pinned requests) - ucb-build-links: fmt_dur forces base-10 so artifact-fed 0900 can't die as octal - build.py: API hrefs pass the same Unity-host regex the consumer enforces (shared pattern under a drift-guard test); the unhealthy-build message prints target+id instead of a secrets-masked URL; a failed first live-comment write is retried by reconcile instead of disabling live rows for the job - upsert-ci-status: doc header diagram matches the emoji header; the truncation re-cuts until closers+note fit inside the cap; two new functional tests (embedded-marker wedge, stale re-read convergence) plus a reconcile-retry unit test - artifact-url: randomized PLATFORM_ROWS heredoc delimiter Co-Authored-By: Claude Fable 5 --- .../test-upsert-ci-status.sh | 57 +++++++++++++++ .../ci-status-comment/upsert-ci-status.sh | 47 ++++++++----- .github/actions/ucb-build-links/action.yml | 4 +- .github/workflows/ci-scripts-tests.yml | 2 +- .github/workflows/pr-comment-artifact-url.yml | 5 +- .../workflows/pr-comment-test-failures.yml | 2 +- .github/workflows/pr-comment-warnings.yml | 14 ++-- scripts/cloudbuild/build.py | 22 ++++-- scripts/cloudbuild/test_build_helpers.py | 69 ++++++++++++++++++- 9 files changed, 186 insertions(+), 36 deletions(-) diff --git a/.github/actions/ci-status-comment/test-upsert-ci-status.sh b/.github/actions/ci-status-comment/test-upsert-ci-status.sh index d20d31005c3..097df314d3c 100644 --- a/.github/actions/ci-status-comment/test-upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/test-upsert-ci-status.sh @@ -47,6 +47,18 @@ path = next(a for a in args if a.startswith('/')) comments = load() if method == 'GET': + # Optional eventual-consistency simulation: while the STALE_READS_FILE + # counter is positive and a pre-PATCH snapshot exists, serve the snapshot + # instead of the live store and decrement the counter. + stale_file = os.environ.get('STALE_READS_FILE') + snapshot = store + '.prev' + if stale_file and os.path.exists(stale_file) and os.path.exists(snapshot): + remaining = int(open(stale_file).read().strip() or 0) + if remaining > 0: + with open(snapshot) as f: + comments = json.load(f) + with open(stale_file, 'w') as f: + f.write(str(remaining - 1)) # --paginate --slurp shape: array of pages. print(json.dumps([comments])) elif method == 'POST': @@ -58,6 +70,10 @@ elif method == 'POST': elif method == 'PATCH': cid = int(path.rsplit('/', 1)[1]) body = json.load(sys.stdin)['body'] + if os.environ.get('STALE_READS_FILE'): + # Snapshot the pre-PATCH store so stale GETs can serve it. + with open(store + '.prev', 'w') as f: + json.dump(comments, f) for c in comments: if c['id'] == cid: c['body'] = body @@ -170,6 +186,47 @@ BIG="$WORK/big-body.md" BODY="$(body_of)" grep -q 'truncated' <<< "$BODY" || fail "truncate: no truncation note" [ "$(( $(grep -c '^```' <<< "$BODY") % 2 ))" = 0 ] || fail "truncate: unbalanced code fence" +SECTION_CONTENT="$(awk '/^$/{grab=1;next} /^$/{grab=0} grab' <<< "$BODY")" +[ "${#SECTION_CONTENT}" -le 20000 ] || fail "truncate: section is ${#SECTION_CONTENT} chars, closers re-inflated past the cap" pass "oversized body truncated with constructs closed" +# --- 9. embedded own-section markers must not scramble the comment ------------ +# The wedge shape: the body smuggles in this section's own end marker (which +# would truncate the fence) and the top-level comment marker. The script strips +# such lines, so the write must settle on the first attempt with the structure +# intact — one marker, one end fence — and later writers must still land. +reset_store +OUT="$(run_upsert build "$(printf 'BEFORE\n\n \nAFTER')")" +grep -q 'updated (attempt 1)' <<< "$OUT" || fail "wedge: write did not settle on attempt 1" +[ "$(count)" = 1 ] || fail "wedge: expected 1 comment" +BODY="$(body_of)" +grep -q 'BEFORE' <<< "$BODY" || fail "wedge: content before marker lost" +grep -q 'AFTER' <<< "$BODY" || fail "wedge: content after marker lost" +[ "$(grep -c '' <<< "$BODY")" = 1 ] || fail "wedge: embedded end marker survived" +[ "$(grep -c '' <<< "$BODY")" = 1 ] || fail "wedge: embedded comment marker survived" +run_upsert lint "LINT-AFTER-WEDGE" >/dev/null +BODY="$(body_of)" +grep -q 'LINT-AFTER-WEDGE' <<< "$BODY" || fail "wedge: later section write lost" +grep -q 'BEFORE' <<< "$BODY" || fail "wedge: later write wiped earlier section" +pass "embedded section markers stripped, comment structure intact" + +# --- 10. stale re-read after PATCH retries and converges ---------------------- +# Create-race shape: the PATCH lands, but the confirming re-read returns a +# stale body whose section content differs from WANT. The script must treat +# that as unsettled, retry, and converge once reads are fresh again. +reset_store "$(python3 - <<'PY' +import json +body = ("\n### 🚦 CI Status\n" + "\nPRE-RACE\n") +print(json.dumps([{"id": 7, "user": {"login": "github-actions[bot]"}, "body": body}])) +PY +)" +echo 1 > "$WORK/stale-reads" +OUT="$( (cd "$WORK" && SECTION=build SECTION_BODY="RACE-CONVERGED" STALE_READS_FILE="$WORK/stale-reads" bash "$UPSERT") )" +grep -q "not settled (attempt 1)" <<< "$OUT" || fail "race: stale re-read did not trigger a retry" +grep -q 'updated (attempt 2)' <<< "$OUT" || fail "race: did not converge on attempt 2" +grep -q 'RACE-CONVERGED' <<< "$(body_of)" || fail "race: final body missing converged content" +[ "$(cat "$WORK/stale-reads")" = 0 ] || fail "race: stale read was not consumed" +pass "stale re-read retried until convergence" + [ "$FAILED" = 0 ] && echo "ALL PASS" || { echo "FAILURES PRESENT"; exit 1; } diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 3ca0f9e84df..7ecde647c22 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -9,7 +9,7 @@ # $HEADER heading plus one fenced block per section: # # -# ### CI Status +# ### 🚦 CI Status # …build… # …lint… # …tests… @@ -40,22 +40,37 @@ fi # 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 # fine for a status section that already links out to the full report. -if [ "${#SECTION_BODY}" -gt 20000 ]; then - echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to 20000." - SECTION_BODY="${SECTION_BODY:0:20000}" - # Close constructs the cut may have severed — an unterminated code fence or - #
makes GitHub render everything after it in this comment inside - # the open block, visually eating the neighbouring sections. - if [ $(( $(grep -c '^```' <<< "$SECTION_BODY") % 2 )) -ne 0 ]; then - SECTION_BODY="$SECTION_BODY"$'\n''```' - fi - opens=$(grep -oi '' - closes=$((closes + 1)) +CAP=20000 +if [ "${#SECTION_BODY}" -gt "$CAP" ]; then + echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to $CAP." + NOTE=$'\n\n'"_…truncated; see the linked run for the full report._" + # Cut with headroom for the note, then close constructs the cut may have + # severed — an unterminated code fence or
makes GitHub render + # everything after it in this comment inside the open block, visually eating + # the neighbouring sections. The appended closers count against the cap too, + # so re-cut and re-balance until the finished body fits inside it. + CUT=$((CAP - ${#NOTE})) + while :; do + TRUNCATED="${SECTION_BODY:0:CUT}" + if [ $(( $(grep -c '^```' <<< "$TRUNCATED") % 2 )) -ne 0 ]; then + TRUNCATED="$TRUNCATED"$'\n''```' + fi + opens=$(grep -oi '' + closes=$((closes + 1)) + done + TRUNCATED="$TRUNCATED$NOTE" + if [ "${#TRUNCATED}" -le "$CAP" ]; then + break + fi + CUT=$((CUT - (${#TRUNCATED} - CAP))) + if [ "$CUT" -lt 0 ]; then + CUT=0 + fi done - SECTION_BODY="$SECTION_BODY"$'\n\n'"_…truncated; see the linked run for the full report._" + SECTION_BODY="$TRUNCATED" fi # Fail fast on a section name outside the fence set — an unknown name would diff --git a/.github/actions/ucb-build-links/action.yml b/.github/actions/ucb-build-links/action.yml index 2370618026a..79f178514ed 100644 --- a/.github/actions/ucb-build-links/action.yml +++ b/.github/actions/ucb-build-links/action.yml @@ -102,7 +102,9 @@ runs: ARTIFACTS_JSON=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/artifacts?per_page=100" 2>/dev/null || echo '{"artifacts":[]}') fmt_dur() { - local s=$1 + # Digits-only input may still carry leading zeros; force base-10 so + # $(( )) can't parse a value like 0900 as (invalid) octal. + local s=$((10#$1)) if [ "$s" -ge 3600 ]; then printf '%dh %dm' $((s/3600)) $((s%3600/60)) elif [ "$s" -ge 60 ]; then printf '%dm %ds' $((s/60)) $((s%60)) else printf '%ds' "$s"; fi diff --git a/.github/workflows/ci-scripts-tests.yml b/.github/workflows/ci-scripts-tests.yml index b12be320beb..b4fd7d4e424 100644 --- a/.github/workflows/ci-scripts-tests.yml +++ b/.github/workflows/ci-scripts-tests.yml @@ -27,7 +27,7 @@ jobs: - name: Unit tests (build.py helpers) run: | - pip install --quiet requests + pip install --quiet -r scripts/cloudbuild/requirements.txt python3 -m unittest discover -s scripts/cloudbuild -v - name: Functional tests (upsert-ci-status.sh, stubbed gh) diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 941fb8d3114..0044ad28d52 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -281,11 +281,12 @@ jobs: joined=$(printf '%s · ' "${parts[@]}") printf '| %s | %s |\n' "$label" "${joined% · }" } + DELIM="PLATFORM_ROWS_EOF_${RANDOM}${RANDOM}_$$" { - echo "PLATFORM_ROWS<> "$GITHUB_ENV" - name: Update build section diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index 6e4fb0b0ae6..4507d992716 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.github/workflows/pr-comment-test-failures.yml @@ -106,7 +106,7 @@ jobs: # tests), from the trusted Actions API — pairs with the test-sum Time # column so setup overhead is visible. job_secs=$(jq -r --arg n "Test ($mode)" \ - '[.jobs[]? | select(.name==$n and .completed_at != null) + '[.jobs[]? | select(.name==$n and .completed_at != null and .started_at != null) | ((.completed_at|fromdateiso8601) - (.started_at|fromdateiso8601))] | first // empty' <<< "$JOBS_JSON") if [[ "$job_secs" =~ ^[0-9]+$ ]]; then job_dur=$(fmt_secs "$job_secs"); else job_dur="—"; fi diff --git a/.github/workflows/pr-comment-warnings.yml b/.github/workflows/pr-comment-warnings.yml index 54f27e88060..77175a1e41a 100644 --- a/.github/workflows/pr-comment-warnings.yml +++ b/.github/workflows/pr-comment-warnings.yml @@ -133,9 +133,10 @@ jobs: BLOCKED=0 # COUNT/BASELINE come from the artifact produced by the untrusted pull_request job - # (fork-controlled). This step has no `set -e`, and bash's `[` errors out on a - # non-numeric operand and falls through - never let such a value reach a comparison - # or a bash arithmetic context ($(( )) evaluates its operands as expressions). + # (fork-controlled). This step runs under Actions' default `bash -e {0}`: `[` + # errors on a non-numeric operand (falling through inside `if`, aborting the step + # elsewhere) - never let such a value reach a comparison or a bash arithmetic + # context ($(( )) evaluates its operands as expressions). [[ "$COUNT" =~ ^[0-9]+$ ]] || COUNT=0 [[ "$BASELINE" =~ ^[0-9]+$ ]] || BASELINE="" @@ -215,10 +216,11 @@ jobs: if [ "$FOUND" = "true" ]; then ART_ID=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" \ --jq '.artifacts[] | select(.name=="csharp-lint-reports") | .id' 2>/dev/null | head -1) - # Wall time of the Lint job, from the trusted Actions API. + # Wall time of the Lint job, from the trusted Actions API. An API failure + # degrades to "no duration" (empty) instead of aborting the step under -e. LINT_SECS=$(gh api "/repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" \ - --jq '[.jobs[] | select(.name=="Lint" and .completed_at != null) - | ((.completed_at|fromdateiso8601) - (.started_at|fromdateiso8601))] | first // empty' 2>/dev/null) + --jq '[.jobs[] | select(.name=="Lint" and .completed_at != null and .started_at != null) + | ((.completed_at|fromdateiso8601) - (.started_at|fromdateiso8601))] | first // empty' 2>/dev/null || true) FOOTER="[Lint run]($RUN_URL)" [ -n "$ART_ID" ] && FOOTER="$FOOTER · [full InspectCode report](https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$ART_ID)" if [[ "$LINT_SECS" =~ ^[0-9]+$ ]]; then diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 85608546489..7106d43769a 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -67,6 +67,11 @@ def _extract_member(self, member, targetpath, pwd): # response that carries one. Persisted to BUILD_LINK_INFO_PATH so the workflow can # upload it and the PR status comment can link the build directly. BUILD_LINK_INFO_PATH = 'unity_cloud_build_info.env' +# Mirror of URL_RE in .github/actions/ucb-build-links/action.yml (the consumer +# silently drops links failing it, so only persist links that will survive). +_DASHBOARD_LINK_RE = re.compile( + r'^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)' + r'/[A-Za-z0-9./_%~?=&#-]*/builds/[0-9]+[A-Za-z0-9./_%~?=&#-]*$') dashboard_url = None _build_link_info_written = False _final_elapsed = None # (queue_secs, build_secs), set once when the build reaches a terminal status @@ -649,11 +654,11 @@ def record_build_link_info(id, response_json): href = None # dashboard_summary is the build's page and dashboard_log its log tab; # dashboard_url can be just the dashboard root, so a candidate only - # qualifies when it is an absolute link to this specific build (the - # comment workflow rejects anything else, so don't persist it either). + # qualifies when it deep-links this specific build on a Unity dashboard + # host — exactly what the comment workflow accepts. for key in ('dashboard_summary', 'dashboard_log', 'dashboard_url'): candidate = (links.get(key) or {}).get('href') - if candidate and candidate.startswith('https://') and '/builds/' in candidate: + if candidate and _DASHBOARD_LINK_RE.match(candidate): href = candidate break @@ -830,8 +835,9 @@ def maybe_update_live_comment(build_id, reconcile=False, force=False): return if reconcile: # Re-assert a few times only: the Pending reset (or the other target's - # first write racing ours) can land after us and drop this row. - if _live_comment_asserts == 0 or _live_comment_asserts >= 3: + # first write racing ours) can land after us and drop this row. A zero + # count still falls through, so a failed first write gets retried. + if _live_comment_asserts >= 3: return # Every probe is a comments-API read drawn from the repo-shared rate # budget; once the row has stayed put this many consecutive checks, @@ -1171,8 +1177,10 @@ def probe_latest_build(): download_log(id) if not build_healthy: - where = dashboard_url or _dashboard_build_url(id) or f'https://cloud.unity.com/ (search for target "{os.getenv("TARGET")}" and build ID "{id}")' - print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page: {where}') + # Dashboard URLs embed ORG_ID/PROJECT_ID, which are masked to *** in + # runner logs — the PR status comment carries the clickable link. + print(f'Build unhealthy - check the downloaded logs or the Unity Cloud build page ' + f'linked from the PR status comment (target "{os.getenv("TARGET")}", build {id}).') sys.exit(1) # Cleanup (only if build is healthy and not release) diff --git a/scripts/cloudbuild/test_build_helpers.py b/scripts/cloudbuild/test_build_helpers.py index 91d665d3734..ca94c0ca41a 100644 --- a/scripts/cloudbuild/test_build_helpers.py +++ b/scripts/cloudbuild/test_build_helpers.py @@ -79,8 +79,10 @@ def test_matches_consumer_url_re(self): self.set_env(**self.ENV) url = build._dashboard_build_url(42) self.assertRegex(url, url_re) - # And the producer's own qualifying filter for API-returned links. - self.assertTrue(url.startswith('https://') and '/builds/' in url) + # The producer's API-href filter must be the same rule verbatim, or a + # link it persists could still be dropped downstream. + self.assertEqual(build._DASHBOARD_LINK_RE.pattern, match.group(1)) + self.assertRegex(url, build._DASHBOARD_LINK_RE) class LinkInfoFileTest(EnvMixin, unittest.TestCase): @@ -131,6 +133,13 @@ def test_non_build_link_rejected(self): build.record_build_link_info(7, {'links': {'dashboard_url': {'href': 'https://cloud.unity.com/'}}}) self.assertEqual(self.read_info()['DASHBOARD_URL'], build._dashboard_build_url(7)) + def test_link_failing_consumer_allowlist_rejected(self): + for href in ('https://example.com/deep/builds/7', # non-dashboard host + 'https://cloud.unity.com/deep/builds/none', # no numeric build id + 'https://cloud.unity.com/deep/builds/7?x=<'): # char outside the allowlist + build.record_build_link_info(7, {'links': {'dashboard_summary': {'href': href}}}) + self.assertEqual(self.read_info()['DASHBOARD_URL'], build._dashboard_build_url(7), href) + def test_final_elapsed_clamps_negative(self): build.record_final_elapsed(7, -5, -1) info = self.read_info() @@ -138,5 +147,61 @@ def test_final_elapsed_clamps_negative(self): self.assertEqual(info['BUILD_SECS'], '0') +class LiveCommentReconcileTest(EnvMixin, unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + old_cwd = os.getcwd() + self.addCleanup(os.chdir, old_cwd) + os.chdir(tmp.name) + # CI_STATUS_SCRIPT is cwd-relative; it must exist for the gate to pass. + os.makedirs(os.path.dirname(build.CI_STATUS_SCRIPT)) + open(build.CI_STATUS_SCRIPT, 'w').close() + self.set_env(PR_NUMBER='1', GH_TOKEN='token') + self._reset_counters() + self.addCleanup(self._reset_counters) + self.addCleanup(setattr, build, 'upsert_live_comment', build.upsert_live_comment) + + @staticmethod + def _reset_counters(): + build._live_comment_asserts = 0 + build._live_comment_last_attempt = 0.0 + build._live_comment_confirms = 0 + + def stub_upsert(self, result): + calls = [] + + def fake(build_id, only_if_missing=False): + calls.append(only_if_missing) + if isinstance(result, Exception): + raise result + return result + build.upsert_live_comment = fake + return calls + + def test_reconcile_retries_after_failed_first_write(self): + self.stub_upsert(RuntimeError('transient')) + build.maybe_update_live_comment(7) # swallowed; no row asserted + self.assertEqual(build._live_comment_asserts, 0) + + calls = self.stub_upsert(True) + build.maybe_update_live_comment(7, reconcile=True) + self.assertEqual(calls, [], 'the 240s spacing must still hold') + + build._live_comment_last_attempt -= 241 + build.maybe_update_live_comment(7, reconcile=True) + self.assertEqual(calls, [True], 'reconcile must retry the failed first write') + self.assertEqual(build._live_comment_asserts, 1) + + def test_reconcile_caps_still_hold(self): + calls = self.stub_upsert(True) + build._live_comment_asserts = 3 + build.maybe_update_live_comment(7, reconcile=True) + build._live_comment_asserts = 1 + build._live_comment_confirms = 3 + build.maybe_update_live_comment(7, reconcile=True) + self.assertEqual(calls, []) + + if __name__ == '__main__': unittest.main() From 447ab7ad2df541b86faec987137a0520f770574d Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:20:07 +0200 Subject: [PATCH 21/28] ci: give every Unity Cloud Build job a least-privilege permissions block Audited every token-consuming operation per job: prebuild needs contents:read (checkout + version composite's git fetch), statuses:write (the four createCommitStatus calls) and pull-requests:read (changed-files REST fallback); build needs contents:read, actions:read (runs/{id}/jobs) and pull-requests:write (status-comment CRUD via build.py); build-gate touches no token at all. The two workflow_call callers' build jobs get the union block so the calls keep working if the repo default token ever tightens (a caller caps its callee). Fork PRs already run with a read-only token regardless of these blocks; status/comment writes there fail today and are unchanged by this. Co-Authored-By: Claude Fable 5 --- .github/workflows/build-profile-nightly.yml | 7 +++++++ .github/workflows/build-release-main.yml | 7 +++++++ .github/workflows/build-unitycloud.yml | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/.github/workflows/build-profile-nightly.yml b/.github/workflows/build-profile-nightly.yml index e918f08ee91..494014b8972 100644 --- a/.github/workflows/build-profile-nightly.yml +++ b/.github/workflows/build-profile-nightly.yml @@ -71,6 +71,13 @@ jobs: name: Build Unity Cloud needs: [check-commits, get-info] if: needs.check-commits.outputs.should_build == 'true' + # Union of the called workflow's job-level grants — a caller caps its + # callee, so this keeps the call working if the repo default tightens. + permissions: + contents: read + statuses: write + actions: read + pull-requests: write uses: ./.github/workflows/build-unitycloud.yml with: profile: profile diff --git a/.github/workflows/build-release-main.yml b/.github/workflows/build-release-main.yml index ffece0ca056..a289ff313eb 100644 --- a/.github/workflows/build-release-main.yml +++ b/.github/workflows/build-release-main.yml @@ -38,6 +38,13 @@ jobs: build: name: Build Unity Cloud needs: get-info + # Union of the called workflow's job-level grants — a caller caps its + # callee, so this keeps the call working if the repo default tightens. + permissions: + contents: read + statuses: write + actions: read + pull-requests: write strategy: matrix: install_source: ['launcher', 'epic'] diff --git a/.github/workflows/build-unitycloud.yml b/.github/workflows/build-unitycloud.yml index 20f3971fc44..514a88a94b8 100644 --- a/.github/workflows/build-unitycloud.yml +++ b/.github/workflows/build-unitycloud.yml @@ -209,6 +209,14 @@ jobs: prebuild: name: Prebuild runs-on: ubuntu-latest + # contents: read — checkout + the version composite's `git fetch origin main:main --tags` + # over the checkout's persisted credentials. + # statuses: write — "Skip build and test checks" posts 4 commit statuses via github-script. + # pull-requests: read — step-security/changed-files' REST fallback (GET /pulls/{n}/files). + permissions: + contents: read + statuses: write + pull-requests: read timeout-minutes: 20 # Skip when PR has 'perf_test' label (only performance tests should run) if: | @@ -550,6 +558,14 @@ jobs: runs-on: ubuntu-latest needs: prebuild if: needs.prebuild.outputs.should_build == 'true' + # contents: read — checkout + the size-budget step's GET /releases/latest. + # actions: read — build.py resolves its own job URL via GET /actions/runs/{run_id}/jobs. + # pull-requests: write — live CI status comment upsert (issue-comment POST/PATCH/DELETE + # on the PR via .github/actions/ci-status-comment/upsert-ci-status.sh). + permissions: + contents: read + actions: read + pull-requests: write # Safety ceiling around the 450m retry budget + surrounding steps. timeout-minutes: 510 strategy: @@ -1018,6 +1034,8 @@ jobs: build-gate: name: Build Gate (Windows + macOS) runs-on: ubuntu-latest + # Pure bash over needs.* context — no checkout, no token use. + permissions: {} needs: [prebuild, build] if: always() && github.event_name == 'pull_request' steps: From 8f4123e534defa0c79d001a6f26bbea63651343a Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:29:08 +0200 Subject: [PATCH 22/28] 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 --- .github/actions/ci-status-comment/action.yml | 3 ++ .../test-upsert-ci-status.sh | 17 ++++++++ .../ci-status-comment/upsert-ci-status.sh | 32 +++++++++++++-- .github/workflows/pr-comment-artifact-url.yml | 35 ++++++++++++++++- .github/workflows/pr-comment-perf.yml | 13 ++++++- .../workflows/pr-comment-test-failures.yml | 7 ++++ .github/workflows/visual-regression.yml | 19 +++++++-- scripts/cloudbuild/build.py | 39 ++++++++++++------- scripts/cloudbuild/test_build_helpers.py | 6 +++ 9 files changed, 148 insertions(+), 23 deletions(-) diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 72f1ff87d45..9e68d672c07 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.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/
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. diff --git a/.github/actions/ci-status-comment/test-upsert-ci-status.sh b/.github/actions/ci-status-comment/test-upsert-ci-status.sh index 097df314d3c..2dfda544a4f 100644 --- a/.github/actions/ci-status-comment/test-upsert-ci-status.sh +++ b/.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 = ("\r\n### 🚦 CI Status\r\n" + "\r\nOLD-BUILD\r\n") +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 '' <<< "$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; } diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 7ecde647c22..b84a9d1e38c 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.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 diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 0044ad28d52..379b160419d 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.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: diff --git a/.github/workflows/pr-comment-perf.yml b/.github/workflows/pr-comment-perf.yml index 6b3f7081d2b..487fe9fd0e5 100644 --- a/.github/workflows/pr-comment-perf.yml +++ b/.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:-}" if [[ -z "$PR_NUMBER" || "$PR_NUMBER" == "null" ]]; then echo "No PR associated with this run, skipping." echo "pr-number=" >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/pr-comment-test-failures.yml b/.github/workflows/pr-comment-test-failures.yml index 4507d992716..5b107cddcbc 100644 --- a/.github/workflows/pr-comment-test-failures.yml +++ b/.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 ❌" ;; diff --git a/.github/workflows/visual-regression.yml b/.github/workflows/visual-regression.yml index d6750c69dc3..9c27914d313 100644 --- a/.github/workflows/visual-regression.yml +++ b/.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 "Triggered via \`/visual-tests\` · the detailed per-platform comment is posted separately." diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 7106d43769a..8b87984a5d2 100644 --- a/scripts/cloudbuild/build.py +++ b/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('') 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: diff --git a/scripts/cloudbuild/test_build_helpers.py b/scripts/cloudbuild/test_build_helpers.py index ca94c0ca41a..f51d34533c6 100644 --- a/scripts/cloudbuild/test_build_helpers.py +++ b/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 From e480a3ad71c28423ac0cc709b9ab4f035b42f052 Mon Sep 17 00:00:00 2001 From: Esteban Ordano <42750+eordano@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:42:35 +0200 Subject: [PATCH 23/28] fix: close review must-fix items on the Unity Cloud build-link PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live PR status-comment writer in build.py composed the matrix platforms' row union from a single read taken before handing the body to upsert-ci-status.sh, which only verifies that its own write landed — it has no visibility into a sibling platform's row arriving in the window between that read and its write, so a second writer's stale union could silently drop the first writer's row (mikhail-dcl). Fix the root cause: upsert_live_comment now loops (bounded by the new LIVE_COMMENT_WRITE_ATTEMPTS) re-reading the section, recomposing the union, and re-reading once more after the write to confirm every row it composed actually survived, retrying against a fresh read instead of trusting a stale snapshot. Added UpsertLiveCommentRaceTest, which reproduces the interleaving and pins the fix (pravusjif). Also: named the 3/3/240 reconcile thresholds in maybe_update_live_comment instead of leaving them as inline magic numbers (nickkhalow), and declared SUITE_ID/WINDOWS_ARTIFACT_ID/ MAC_ARTIFACT_ID/GITHUB_SERVER_URL/GITHUB_REPOSITORY explicitly in the "Compose platform rows" step's own env: block instead of relying on implicit $GITHUB_ENV inheritance from an earlier step in the same job (dalkia). The PR-split ask (popuz, blocking) is a submission/process concern — landing the ci-status-comment hardening, the build-link feature, and the performance/automation sections as separate sequenced PRs — not a code defect this branch can fix; see FIXNOTES.md for why it's out of reach here and the concrete split to do as a follow-up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018c638dR1vPysCMbYt2qQg5 --- .github/workflows/pr-comment-artifact-url.yml | 7 ++ scripts/cloudbuild/build.py | 111 +++++++++++------- scripts/cloudbuild/test_build_helpers.py | 71 +++++++++++ 3 files changed, 147 insertions(+), 42 deletions(-) diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 379b160419d..258175a31eb 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -301,6 +301,13 @@ jobs: WINDOWS_CELL: ${{ steps.ucb.outputs.windows-cell }} MAC_CELL: ${{ steps.ucb.outputs.mac-cell }} S3_BASE: ${{ format('{0}/{1}', vars.EXPLORER_TEAM_S3_BUCKET_PUBLIC_URL, env.ARTIFACT_S3_DESTINATION_PATH) }} + # Set by "Get Artifact and Pull request info" above via $GITHUB_ENV; listed + # here so the coupling between the two steps is visible without tracing it. + SUITE_ID: ${{ env.SUITE_ID }} + WINDOWS_ARTIFACT_ID: ${{ env.WINDOWS_ARTIFACT_ID }} + MAC_ARTIFACT_ID: ${{ env.MAC_ARTIFACT_ID }} + GITHUB_SERVER_URL: ${{ github.server_url }} + GITHUB_REPOSITORY: ${{ github.repository }} run: | set -euo pipefail compose_row() { diff --git a/scripts/cloudbuild/build.py b/scripts/cloudbuild/build.py index 8b87984a5d2..2eecc8dc9f5 100644 --- a/scripts/cloudbuild/build.py +++ b/scripts/cloudbuild/build.py @@ -82,6 +82,19 @@ def _extract_member(self, member, targetpath, pwd): # runs instead of after it. Purely cosmetic: every failure is swallowed. CI_STATUS_SCRIPT = os.path.join('.github', 'actions', 'ci-status-comment', 'upsert-ci-status.sh') LIVE_MARKER_PREFIX = '' - section = _build_section_of_status_comment() - if section is None: - return None - if only_if_missing and marker in section: - return False parts = [] job_url = _own_job_url() @@ -801,35 +814,49 @@ def upsert_live_comment(build_id, only_if_missing=False): parts.append(f'[Unity Cloud #{build_id}]({link})' if link else f'Unity Cloud build {build_id}') own_row = f'| {label} | {" · ".join(parts)} {marker} |' - rows = [line for line in section.splitlines() if LIVE_MARKER_PREFIX in line and marker not in line] - rows.append(own_row) - rows.sort(key=lambda row: 0 if '| Windows |' in row else 1) - - server = os.getenv('GITHUB_SERVER_URL', 'https://github.com') - run_url = f"{server}/{os.getenv('GITHUB_REPOSITORY')}/actions/runs/{os.getenv('GITHUB_RUN_ID')}" - body = '\n'.join([ - f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url})', - '', - '| Platform | Links & timing |', - '| -------- | ----------------------- |', - *rows, - ]) - - body_file = None - try: - with tempfile.NamedTemporaryFile('w', suffix='.md', delete=False) as f: - f.write(body) - body_file = f.name - env = dict(os.environ, - REPO=os.getenv('GITHUB_REPOSITORY') or '', - SECTION='build', - SECTION_BODY='', - SECTION_BODY_FILE=body_file) - result = subprocess.run(['bash', CI_STATUS_SCRIPT], env=env, timeout=180, check=False) - finally: - if body_file: - os.unlink(body_file) - return True if result.returncode == 0 else None + for _ in range(LIVE_COMMENT_WRITE_ATTEMPTS): + section = _build_section_of_status_comment() + if section is None: + return None + if only_if_missing and marker in section: + return False + + rows = [line for line in section.splitlines() if LIVE_MARKER_PREFIX in line and marker not in line] + rows.append(own_row) + rows.sort(key=lambda row: 0 if '| Windows |' in row else 1) + + server = os.getenv('GITHUB_SERVER_URL', 'https://github.com') + run_url = f"{server}/{os.getenv('GITHUB_REPOSITORY')}/actions/runs/{os.getenv('GITHUB_RUN_ID')}" + body = '\n'.join([ + f'[![Build](https://img.shields.io/badge/Build-In%20progress-1f6feb?logo=unity&logoColor=white&style=for-the-badge)]({run_url})', + '', + '| Platform | Links & timing |', + '| -------- | ----------------------- |', + *rows, + ]) + + body_file = None + try: + with tempfile.NamedTemporaryFile('w', suffix='.md', delete=False) as f: + f.write(body) + body_file = f.name + env = dict(os.environ, + REPO=os.getenv('GITHUB_REPOSITORY') or '', + SECTION='build', + SECTION_BODY='', + SECTION_BODY_FILE=body_file) + result = subprocess.run(['bash', CI_STATUS_SCRIPT], env=env, timeout=180, check=False) + finally: + if body_file: + os.unlink(body_file) + if result.returncode != 0: + return None + + post_section = _build_section_of_status_comment() + if post_section is not None and all(row in post_section for row in rows): + return True + + return None def maybe_update_live_comment(build_id, reconcile=False, force=False): @@ -843,14 +870,14 @@ def maybe_update_live_comment(build_id, reconcile=False, force=False): # Re-assert a few times only: the Pending reset (or the other target's # first write racing ours) can land after us and drop this row. A zero # count still falls through, so a failed first write gets retried. - if _live_comment_asserts >= 3: + if _live_comment_asserts >= MAX_LIVE_ASSERTS: return # Every probe is a comments-API read drawn from the repo-shared rate # budget; once the row has stayed put this many consecutive checks, # stop probing for the rest of the build. - if _live_comment_confirms >= 3: + if _live_comment_confirms >= MAX_LIVE_CONFIRMS: return - if time.time() - _live_comment_last_attempt < 240: + if time.time() - _live_comment_last_attempt < LIVE_RECONCILE_INTERVAL_SECS: return elif _live_comment_asserts > 0 and not force: return diff --git a/scripts/cloudbuild/test_build_helpers.py b/scripts/cloudbuild/test_build_helpers.py index f51d34533c6..acbee04259e 100644 --- a/scripts/cloudbuild/test_build_helpers.py +++ b/scripts/cloudbuild/test_build_helpers.py @@ -209,5 +209,76 @@ def test_reconcile_caps_still_hold(self): self.assertEqual(calls, []) +class UpsertLiveCommentRaceTest(EnvMixin, unittest.TestCase): + """Pins the race mikhail-dcl flagged in review: upsert-ci-status.sh's own + retry loop only confirms that *this* write's body landed, so a sibling + platform's row arriving between upsert_live_comment's read and that write + is invisible to it. upsert_live_comment must notice its composed union did + not survive and retry against a fresh read instead of reporting success on + a stale one.""" + + WIN_ROW = '| Windows | Unity Cloud build 7 |' + MAC_ROW = '| Mac | Unity Cloud build 9 |' + + def setUp(self): + self.set_env(TARGET='windows64-x', GITHUB_REPOSITORY='org/repo', GITHUB_RUN_ID='1', + GITHUB_SERVER_URL='https://github.com', ORG_ID=None, PROJECT_ID=None) + build.dashboard_url = None + self.addCleanup(setattr, build, 'dashboard_url', None) + + @staticmethod + def fake_run_capturing(bodies): + def fake_run(cmd, env, timeout, check): + with open(env['SECTION_BODY_FILE']) as f: + bodies.append(f.read()) + return mock.Mock(returncode=0) + return fake_run + + def test_sibling_row_landing_mid_write_is_recovered_on_retry(self): + # attempt 1 pre-write read: no rows yet. + # attempt 1 post-write verify: Mac's row raced in underneath us — the + # union this attempt wrote (Windows only) is now stale. The pre-fix + # code had no post-write read at all and would have reported success + # here, permanently dropping Mac's row from the next real write. + # attempt 2 pre-write read: fresh, carries Mac's row along. + # attempt 2 post-write verify: both rows confirmed present. + reads = ['', self.MAC_ROW, self.MAC_ROW, f'{self.MAC_ROW}\n{self.WIN_ROW}'] + bodies = [] + with mock.patch.object(build, '_build_section_of_status_comment', side_effect=reads), \ + mock.patch.object(build, '_own_job_url', return_value=None), \ + mock.patch.object(build.subprocess, 'run', side_effect=self.fake_run_capturing(bodies)): + result = build.upsert_live_comment(7) + + self.assertTrue(result) + self.assertEqual(len(bodies), 2, 'a stale-union write must be retried, not accepted') + self.assertIn(self.WIN_ROW, bodies[0]) + self.assertNotIn(self.MAC_ROW, bodies[0], "attempt 1's read had no sibling row yet") + self.assertIn(self.WIN_ROW, bodies[1]) + self.assertIn(self.MAC_ROW, bodies[1], "retry's union must carry the sibling row along") + + def test_gives_up_after_exhausting_attempts_instead_of_spinning(self): + # Pathological: the post-write read never reflects this attempt's own + # write (as if every attempt kept losing the race). Must terminate + # after LIVE_COMMENT_WRITE_ATTEMPTS, not retry forever. + reads = [''] * (2 * build.LIVE_COMMENT_WRITE_ATTEMPTS) + bodies = [] + with mock.patch.object(build, '_build_section_of_status_comment', side_effect=reads), \ + mock.patch.object(build, '_own_job_url', return_value=None), \ + mock.patch.object(build.subprocess, 'run', side_effect=self.fake_run_capturing(bodies)): + result = build.upsert_live_comment(7) + + self.assertIsNone(result) + self.assertEqual(len(bodies), build.LIVE_COMMENT_WRITE_ATTEMPTS) + + def test_only_if_missing_short_circuits_on_first_fresh_read(self): + with mock.patch.object(build, '_build_section_of_status_comment', return_value=self.WIN_ROW), \ + mock.patch.object(build, '_own_job_url', return_value=None), \ + mock.patch.object(build.subprocess, 'run') as run: + result = build.upsert_live_comment(7, only_if_missing=True) + + self.assertFalse(result) + run.assert_not_called() + + if __name__ == '__main__': unittest.main() From 68c4d99ef1723acd2897c7e43be1c189bf3cd4c8 Mon Sep 17 00:00:00 2001 From: Juan Molteni Date: Fri, 28 Aug 2026 15:54:31 -0300 Subject: [PATCH 24/28] ci: add an on-demand inworld section to the unified CI status comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The InWorld suite (run-inworld-suite.yml in explorer-automation) and the bare-metal benchmark (performance-testing) still post standalone PR comments; both fold into the unified comment instead. The inworld section is not seeded in the skeleton — the suite only runs on release/hotfix PRs into main, so the fence is appended the first time it reports. The seed path now appends a missing fence to a fresh skeleton too, so a non-skeleton section that has to create the comment cannot wedge the survive check. Co-Authored-By: Claude Fable 5 --- .github/actions/ci-status-comment/action.yml | 11 +++--- .../test-upsert-ci-status.sh | 23 +++++++++++- .../ci-status-comment/upsert-ci-status.sh | 35 ++++++++++++------- .github/workflows/in-world-tests.yml | 10 +++--- .github/workflows/pr-comment-artifact-url.yml | 7 ++-- 5 files changed, 61 insertions(+), 25 deletions(-) diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 9e68d672c07..732673b02f5 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.github/actions/ci-status-comment/action.yml @@ -1,17 +1,18 @@ name: Upsert CI Status Comment description: >- Create or update the single unified CI status comment on a PR, replacing only - the given section (build | lint | tests | performance | automation). Seeds a - skeleton with every section the first time it runs, appends a missing section - fence to older comments, and re-reads/retries so concurrent writers (build - vs. Unity Test) never clobber each other's section. + the given section (build | lint | tests | performance | automation | + inworld). Seeds a skeleton with every always-present section the first time + it runs, appends a missing section fence to older comments (and for the + on-demand inworld section), and re-reads/retries so concurrent writers + (build vs. Unity Test) never clobber each other's section. inputs: pr-number: description: Pull request number to comment on. required: true section: - description: Which section to replace — one of build, lint, tests, performance, automation. + description: Which section to replace — one of build, lint, tests, performance, automation, inworld. required: true body: description: >- diff --git a/.github/actions/ci-status-comment/test-upsert-ci-status.sh b/.github/actions/ci-status-comment/test-upsert-ci-status.sh index 2dfda544a4f..a32a93495d0 100644 --- a/.github/actions/ci-status-comment/test-upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/test-upsert-ci-status.sh @@ -107,9 +107,10 @@ run_upsert build "BUILD-CONTENT" >/dev/null BODY="$(body_of)" grep -q 'BUILD-CONTENT' <<< "$BODY" || fail "create: build content missing" grep -q '' <<< "$BODY" || fail "create: performance fence missing" +grep -q '' <<< "$BODY" && fail "create: on-demand inworld fence in skeleton" grep -q '🚦 CI Status' <<< "$BODY" || fail "create: emoji header missing" grep -q 'decentraland_256x256' <<< "$BODY" && fail "create: retired logo header present" -pass "create seeds skeleton with all sections" +pass "create seeds skeleton with all always-present sections" # --- 2. section update preserves the others --------------------------------- run_upsert tests "TESTS-CONTENT" >/dev/null @@ -135,6 +136,26 @@ grep -q '🚦 CI Status' <<< "$BODY" || fail "append: logo header not migrated" grep -q 'decentraland_256x256' <<< "$BODY" && fail "append: retired logo header still present" pass "missing fence appended + header migrated" +# --- 3b. on-demand section: appended to an existing comment, and on create ---- +# inworld is not in the skeleton; a write must append its fence to a comment +# seeded without it — and a write that has to create the comment must append +# the fence to the fresh skeleton too, not wedge the survive check. +reset_store +run_upsert build "BUILD-FIRST" >/dev/null +run_upsert inworld "INWORLD-CONTENT" >/dev/null +[ "$(count)" = 1 ] || fail "inworld append: expected 1 comment" +BODY="$(body_of)" +grep -q 'BUILD-FIRST' <<< "$BODY" || fail "inworld append: build content lost" +grep -q 'INWORLD-CONTENT' <<< "$BODY" || fail "inworld append: content missing" +[ "$(grep -cF '' <<< "$BODY")" = 1 ] || fail "inworld append: fence count wrong" +reset_store +OUT="$(run_upsert inworld "INWORLD-SEEDS")" +grep -q 'updated (attempt 1)' <<< "$OUT" || fail "inworld create: did not settle on attempt 1" +BODY="$(body_of)" +grep -q 'INWORLD-SEEDS' <<< "$BODY" || fail "inworld create: content missing" +grep -q '' <<< "$BODY" || fail "inworld create: skeleton sections missing" +pass "on-demand inworld section appended on update and on create" + # --- 4. marker-shaped body lines are stripped -------------------------------- reset_store run_upsert build "$(printf 'SAFE\n\nALSO-SAFE')" >/dev/null diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index b84a9d1e38c..5197877c2d9 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -1,9 +1,10 @@ #!/usr/bin/env bash # Create or update the single unified CI status comment on a PR, replacing only -# one section (build | lint | tests | performance | automation). CI comment -# workflows call this through the ci-status-comment composite action; build.py -# (live build rows) and decentraland/performance-testing run it directly. Either -# way the separate bot comments collapse into one. +# one section (build | lint | tests | performance | automation | inworld). CI +# comment workflows call this through the ci-status-comment composite action; +# build.py (live build rows), decentraland/performance-testing (benchmark +# report) and decentraland/explorer-automation (InWorld suite) run it directly. +# Either way the separate bot comments collapse into one. # # The comment is keyed by the hidden marker and holds the # $HEADER heading plus one fenced block per section: @@ -15,6 +16,7 @@ # …tests… # …performance… # …automation… +# …inworld… # # Build and Unity Test run as independent workflows whose comment writers can # fire at the same time, so a plain read-modify-write would drop a section or @@ -83,7 +85,7 @@ fi # append a dead fence to the shared comment and then wedge the survive check # for 5 attempts, burning ~15 API calls per write from then on. case "${SECTION:-}" in - build|lint|tests|performance|automation) ;; + build|lint|tests|performance|automation|inworld) ;; *) echo "::error::Unknown section '${SECTION:-}'."; exit 2 ;; esac @@ -106,14 +108,18 @@ section_default() { lint) printf '![Lint](https://img.shields.io/badge/Lint-Waiting-lightgrey?logo=jetbrains&logoColor=white&style=for-the-badge)\n\n_Waiting for lint to start…_' ;; tests) printf '![Tests](https://img.shields.io/badge/Tests-Waiting-lightgrey?logo=codecov&logoColor=white&style=for-the-badge)\n\n_Waiting for tests to start…_' ;; automation) printf '![Automation](https://img.shields.io/badge/Automation-On%%20demand-lightgrey?logo=github&logoColor=white&style=for-the-badge)\n\n_On demand — comment `/visual-tests` on this PR to run the visual regression suite against its build._' ;; - performance) printf '![Performance](https://img.shields.io/badge/Performance-Waiting-lightgrey?logo=speedtest&logoColor=white&style=for-the-badge)\n\n_Bare-metal benchmarks run automatically after each successful build; results arrive as a separate comment. Add the `perf_test` label to run the in-repo Unity performance suite instead (skips normal CI and blocks merge while set)._' ;; + performance) printf '![Performance](https://img.shields.io/badge/Performance-Waiting-lightgrey?logo=speedtest&logoColor=white&style=for-the-badge)\n\n_Bare-metal benchmarks run automatically after each successful build; results land in this section. Add the `perf_test` label to run the in-repo Unity performance suite instead (skips normal CI and blocks merge while set)._' ;; + inworld) printf '![InWorld](https://img.shields.io/badge/InWorld-Waiting-lightgrey?logo=unity&logoColor=white&style=for-the-badge)\n\n_Waiting for the InWorld suite…_' ;; esac } # One section, fenced by its start/end markers. wrap_section() { printf '\n%s\n' "$1" "$2" "$1"; } -# A fresh comment with every section defaulted to "waiting". +# A fresh comment with every always-present section defaulted to "waiting". +# inworld is deliberately absent: the suite only runs on release/hotfix PRs +# into main, and a permanent "waiting" row on every other PR would be noise — +# the append-missing-fence path below adds it the first time it reports. skeleton() { printf '%s\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n\n%s\n' \ "$MARKER" "$HEADER" \ @@ -201,17 +207,22 @@ for attempt in 1 2 3 4 5; do CURRENT_BODY="" fi - # No unified comment yet: start from the full skeleton. A comment that exists - # but lacks our markers predates this section (e.g. it was written before the - # automation section existed) — append an empty fence for just our section - # instead of resetting the whole comment and wiping the other sections' state. + # No unified comment yet: start from the full skeleton. A comment that lacks + # our markers predates this section (e.g. it was written before the automation + # section existed) or the section is not part of the skeleton (inworld) — + # append an empty fence for just our section instead of resetting the whole + # comment and wiping the other sections' state. Two independent checks, not + # if/elif: a fresh skeleton needs the fence appended too when the section is + # a non-skeleton one, or replace_section would find no fence and the survive + # check would burn all 5 attempts. if [ -z "$CURRENT_BODY" ]; then CURRENT_BODY="$(skeleton)" + fi # -x: whole-line, matching replace_section/extract_section's $0==s exactly. A # substring hit on a marker embedded in a body line (which the strip filter # deliberately lets through) would skip fence creation here while the awk # matchers see nothing — leaving the section permanently unwritable. - elif ! grep -qxF "$START" <<< "$CURRENT_BODY"; then + if ! grep -qxF "$START" <<< "$CURRENT_BODY"; then CURRENT_BODY="$CURRENT_BODY"$'\n\n'"$(wrap_section "$SECTION" "$(section_default "$SECTION")")" fi diff --git a/.github/workflows/in-world-tests.yml b/.github/workflows/in-world-tests.yml index 197ab294049..bbad189b65f 100644 --- a/.github/workflows/in-world-tests.yml +++ b/.github/workflows/in-world-tests.yml @@ -1,8 +1,9 @@ name: In-World Tests # Dispatcher for the InWorld NUnit suite. The mechanics — Explorer install, -# AltTester, `mf explorer test`, Allure upload, PR comment — live in -# decentraland/explorer-automation's `run-inworld-suite.yml`. This file only +# AltTester, `mf explorer test`, Allure upload, and reporting into the unified +# CI status comment's InWorld section (standalone comment as fallback) — live +# in decentraland/explorer-automation's `run-inworld-suite.yml`. This file only # decides when to run and against which build, and shares its build resolution # with visual-regression.yml via .github/actions/resolve-explorer-build. # @@ -188,8 +189,9 @@ jobs: # explorer-automation that touch it cannot change release validation here. uses: decentraland/explorer-automation/.github/workflows/run-inworld-suite.yml@main # Must be a superset of run-inworld-suite.yml's own `permissions:`, or the - # call dies at startup. The write is the PR comment it posts; this is the - # only job that needs one, hence per job rather than at the top of the file. + # call dies at startup. The write is the CI status comment section it + # upserts (or the standalone comment it falls back to); this is the only + # job that needs one, hence per job rather than at the top of the file. permissions: contents: read pull-requests: write diff --git a/.github/workflows/pr-comment-artifact-url.yml b/.github/workflows/pr-comment-artifact-url.yml index 258175a31eb..e1a82e78328 100644 --- a/.github/workflows/pr-comment-artifact-url.yml +++ b/.github/workflows/pr-comment-artifact-url.yml @@ -406,8 +406,9 @@ jobs: echo "PAT_EXPIRY_WARNING=$msg" >> "$GITHUB_ENV" # repository_dispatch is fire-and-forget (no run id comes back), so this - # links the target workflow's run list; the benchmark itself posts a - # separate perf-test-summary comment with its run link when it finishes. + # links the target workflow's run list; the benchmark itself rewrites + # this section with its report when it finishes (or falls back to a + # standalone perf-test-summary comment when the section write fails). - name: Mark performance section as dispatched if: steps.perf_dispatch.outcome == 'success' uses: ./.github/actions/ci-status-comment @@ -418,7 +419,7 @@ jobs: body: |- [![Performance](https://img.shields.io/badge/Performance-Dispatched!-yellow?logo=speedtest&logoColor=white&style=for-the-badge)](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml) - 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet will be posted as a separate `perf-test-summary` comment. + 🏁 Bare-metal benchmark dispatched for this build ([run queue](https://github.com/decentraland/performance-testing/actions/workflows/unity-explorer.yaml)) — results across the runner fleet land in this section when it finishes. Latest build wins this section — it also replaces any earlier `perf_test`-suite verdict. From d10978bc07f41ee272b8310a5ddb5fddb0cecd32 Mon Sep 17 00:00:00 2001 From: Juan Molteni Date: Tue, 1 Sep 2026 12:13:55 -0300 Subject: [PATCH 25/28] docs(ci): note why the build job's pull-requests:write is safe The build job runs PR-head code with a write token, which a security re-review flagged as a trust boundary. Record why it is not one: pull_request (not pull_request_target) gives forks a read-only token that permissions cannot escalate, forks cannot run the job without the Unity Cloud secrets anyway, and a same-repo branch's author already holds the capability. No behaviour change. Co-Authored-By: Claude Fable 5 --- .github/workflows/build-unitycloud.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/build-unitycloud.yml b/.github/workflows/build-unitycloud.yml index 2f0a3e2194d..dca61904c14 100644 --- a/.github/workflows/build-unitycloud.yml +++ b/.github/workflows/build-unitycloud.yml @@ -570,6 +570,10 @@ jobs: # actions: read — build.py resolves its own job URL via GET /actions/runs/{run_id}/jobs. # pull-requests: write — live CI status comment upsert (issue-comment POST/PATCH/DELETE # on the PR via .github/actions/ci-status-comment/upsert-ci-status.sh). + # Safe on this `pull_request` (not `pull_request_target`) trigger: a fork PR gets a + # read-only GITHUB_TOKEN that `permissions:` cannot escalate, and cannot run this job + # anyway (the build needs UNITY_CLOUD_API_KEY/ORG_ID/PROJECT_ID, unavailable to forks); + # a same-repo branch is pushed by someone who already holds this capability with write access. permissions: contents: read actions: read From 855249521532c5c1e7c35735f629e0d514fffd2a Mon Sep 17 00:00:00 2001 From: Juan Molteni Date: Tue, 1 Sep 2026 13:43:03 -0300 Subject: [PATCH 26/28] fix(ci): size the per-section comment cap to fit GitHub's 65536 ceiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-section CAP was 20000, but the section allowlist grew to six (build, lint, tests, performance, automation, inworld). 6 × 20000 = 120000 > 65536, so the cap could not actually prevent the 422 it exists to prevent. Drop it to 10000 (6 × 10000 = 60000, ~5.5k headroom for the header and fences) and note the invariant so a future section forces a re-derive. Test assertion updated to match. Co-Authored-By: Claude Fable 5 --- .../actions/ci-status-comment/test-upsert-ci-status.sh | 2 +- .github/actions/ci-status-comment/upsert-ci-status.sh | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/actions/ci-status-comment/test-upsert-ci-status.sh b/.github/actions/ci-status-comment/test-upsert-ci-status.sh index a32a93495d0..ee3cba29bd4 100644 --- a/.github/actions/ci-status-comment/test-upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/test-upsert-ci-status.sh @@ -208,7 +208,7 @@ BODY="$(body_of)" grep -q 'truncated' <<< "$BODY" || fail "truncate: no truncation note" [ "$(( $(grep -c '^```' <<< "$BODY") % 2 ))" = 0 ] || fail "truncate: unbalanced code fence" SECTION_CONTENT="$(awk '/^$/{grab=1;next} /^$/{grab=0} grab' <<< "$BODY")" -[ "${#SECTION_CONTENT}" -le 20000 ] || fail "truncate: section is ${#SECTION_CONTENT} chars, closers re-inflated past the cap" +[ "${#SECTION_CONTENT}" -le 10000 ] || fail "truncate: section is ${#SECTION_CONTENT} chars, closers re-inflated past the cap" pass "oversized body truncated with constructs closed" # --- 9. embedded own-section markers must not scramble the comment ------------ diff --git a/.github/actions/ci-status-comment/upsert-ci-status.sh b/.github/actions/ci-status-comment/upsert-ci-status.sh index 5197877c2d9..f75330cfc92 100755 --- a/.github/actions/ci-status-comment/upsert-ci-status.sh +++ b/.github/actions/ci-status-comment/upsert-ci-status.sh @@ -48,7 +48,13 @@ SECTION_BODY="${SECTION_BODY//$'\r'/}" # 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 # fine for a status section that already links out to the full report. -CAP=20000 +# +# The cap is per-section and blind to the others, so it only bounds the total +# if (section count × CAP) stays under 65536 with headroom for the header and +# the fences. There are 6 allowlisted sections (see below): 6 × 10000 = 60000, +# leaving ~5.5k. Re-derive this if a section is added, or the ${#NEW_BODY} +# guard below is the only thing left standing between a big comment and a 422. +CAP=10000 if [ "${#SECTION_BODY}" -gt "$CAP" ]; then echo "::warning::Section body is ${#SECTION_BODY} chars; truncating to $CAP." NOTE=$'\n\n'"_…truncated; see the linked run for the full report._" From d4eb6b2f1bae6a983664139cfdb7711c88b10f95 Mon Sep 17 00:00:00 2001 From: Juan Molteni Date: Tue, 1 Sep 2026 14:19:36 -0300 Subject: [PATCH 27/28] fix(ci): stop create-release-branch clobbering the unified CI status comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Its build-links comment finder matches any github-actions[bot] comment containing an img.shields.io/badge/Build badge — which the unified CI status comment's build section now also carries. On a release PR that has one, this either short-circuited on its "Build-Success!" badge (so the dev-build-links comment was never posted) or PATCHed the entire unified comment away with the release message. Exclude the unified comment by its marker, matching the finders in the perf and InWorld folds. Co-Authored-By: Claude Fable 5 --- .github/workflows/create-release-branch.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/create-release-branch.yml b/.github/workflows/create-release-branch.yml index 396aafeaf97..ce2831a29da 100644 --- a/.github/workflows/create-release-branch.yml +++ b/.github/workflows/create-release-branch.yml @@ -52,8 +52,14 @@ jobs: # SHA was already built by the push to dev — reuse that run's # artifacts instead of rebuilding. PR_NUMBER=$(gh pr view "$BRANCH_NAME" --json number --jq '.number') + # This standalone build-links comment predates the unified CI status + # comment, whose build section ALSO carries an img.shields.io/badge/Build + # badge under github-actions[bot]. Exclude it explicitly (by its + # marker) or a release PR that has one would either + # short-circuit here on its "Build-Success!" badge, or have its whole + # unified comment PATCHed away by post_comment below. EXISTING=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" \ - --jq '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("img.shields.io/badge/Build"))] | .[0] // empty') + --jq '[.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("img.shields.io/badge/Build")) | select(.body | contains("") | not)] | .[0] // empty') # A "Build Not Found" comment is upgraded in place on re-run once the # build exists; only a success comment short-circuits. From ba21b348d3dc6743341a14a8568b8780899e45f3 Mon Sep 17 00:00:00 2001 From: Juan Ignacio Molteni Date: Wed, 2 Sep 2026 08:13:32 -0300 Subject: [PATCH 28/28] Update .github/actions/ci-status-comment/action.yml Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Juan Ignacio Molteni --- .github/actions/ci-status-comment/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/ci-status-comment/action.yml b/.github/actions/ci-status-comment/action.yml index 732673b02f5..4d0bd9e58b0 100644 --- a/.github/actions/ci-status-comment/action.yml +++ b/.github/actions/ci-status-comment/action.yml @@ -17,7 +17,7 @@ inputs: body: description: >- Markdown for this section (inline badge + message). Rendered between the - section markers after dropping marker-shaped lines; bodies over 20000 + section markers after dropping marker-shaped lines; bodies over 10000 chars are truncated with fences/
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