chore(ci): link Unity Cloud builds, test reports, timings, performance and automation from the CI status comment - #9713
chore(ci): link Unity Cloud builds, test reports, timings, performance and automation from the CI status comment#9713eordano wants to merge 23 commits into
Conversation
…mment 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 <noreply@anthropic.com>
decentraland-bot
added
the
ext-contribution
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Waiting for lint to start… Waiting for tests to start… 🏁 Bare-metal benchmark finished — run #31746714469.
Full report (per-runner tables)PR #9713, run #31746714469 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
Exception breakdown
|
|
Slack notification sent to #explorer-ext-contributions for external review. |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
CI Review — ci: deep-link the Unity Cloud build page from CI and the PR status comment
STEP 2 — Root-cause check: PASS
The PR status comment's build badge was a dead-end image (clicking opened shields.io), and build logs told humans to search cloud.unity.com by hand. This PR fixes the root cause by capturing the dashboard deep link from the Unity Cloud Build API response, persisting it as an artifact, and rendering it — validated — in the PR comment. No symptom-masking.
STEP 3 — Design & integration: PASS
This is a CI-only change (no ECS systems, no runtime code). The data flow is sound:
build.py captures URL from Unity Cloud API → writes .env file → uploaded as artifact → downloaded by comment workflow → validated against conservative regex → rendered in PR comment.
Key design decisions reviewed:
- Artifact-based communication — follows the existing pattern between the build and comment workflows. The
workflow_runevent trigger means these run in separate jobs/workflows, so artifacts are the correct communication channel. - Untrusted input boundary — artifacts from the PR-controlled build workflow are correctly treated as untrusted in the comment workflow. Validation is applied before rendering.
- Graceful degradation — if no dashboard link is found (older branches, API changes), rows simply don't render and the comment layout is unchanged.
check-build-ranfix — countingunity_build_info_*artifacts as evidence a build ran is a correct fix for builds that fail before producingDecentraland_*player artifacts (which previously left comments stuck on "Pending" forever).
STEP 4 — Member audit: N/A
CI/workflow code — no C# classes or properties to audit.
STEP 5 — Line-level review
Security analysis — No security issues found
- Input validation:
BUILD_IDvalidated with^[0-9]+$,DASHBOARD_URLvalidated with^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&-]*$. The charset excludes),],<,>,",', newlines, and#— no markdown link breakout, no fragment injection, no HTML injection. ✅ - Heredoc delimiter safety:
UCB_ROWS_EOF/UCB_SECTION_EOFdelimiters are hardcoded and cannot appear in the validated content (restricted charset, no underscores in sequence). No delimiter injection. ✅ - Secrets: No hardcoded secrets.
GITHUB_TOKENpassed via${{ github.token }}. ✅ - Badge link expressions:
${{ github.server_url }},${{ github.repository }},${{ github.event.workflow_run.id }},${{ env.PREVIOUS_JOB_ID }}— all GitHub-controlled or previously-validated values. No injection surface. ✅ - Information disclosure: Dashboard URLs expose Unity Cloud org/project slugs in public PR comments (author acknowledged this). Access requires Unity org membership. Acceptable trade-off. ✅
Code quality observations (non-blocking)
-
parse_info()duplication — The shell function appears identically in both the success job and failure job, differing only in the run-ID variable (PREVIOUS_JOB_IDvsRUN_ID). This is inherent to GitHub Actions' job isolation (jobs can't share inline shell functions). A reusable composite action could deduplicate this, but that would be over-engineering for a self-contained helper. Noting for future maintenance — if the validation logic ever needs updating, both copies must change in sync. -
Python code consistency — The
idparameter inrecord_build_link_info(id, response_json)shadows Python's built-inid(), but this is consistent with the existing codebase (cancel_build(id),poll_build(id),download_artifact(id),download_log(id),delete_build(id),get_log_byte_count(id)). Theos.getenv('TARGET')call without a default is also consistent with 10+ existing uses in the file. -
Call-site guard logic — The interaction between the call-site guard (
if dashboard_url is None) and the internal early-return (if _build_link_info_written and not href: return) is correct and complementary: the file is written on first poll (with or without URL), updated if a later response carries a URL, and the call site skips entirely once a URL is captured. Well-designed. -
Success vs. failure comment format — The success path appends
UCB_ROWSto an existing table (header already present), while the failure path builds a standaloneUCB_SECTIONwith its own table header. This correctly accounts for the different comment structures. ✅
STEP 6 — Complexity: SIMPLE
STEP 7 — QA: NO
CI-only changes to GitHub Actions workflows and a Python build script. No runtime code, no user-facing behavior changes.
STEP 8 — Non-blocking warnings
None. Main.unity not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI-only changes to GitHub Actions workflows and a Python build script; no runtime Unity code touched.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by eordano via Slack
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # 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: |
There was a problem hiding this comment.
The '/builds/' in candidate test correctly rejects a bare dashboard root, but it also accepts a relative href. Several Unity Cloud Build API links.*.href values are API-relative paths (/api/v1/orgs/…/builds/42), and such a value contains /builds/ so it wins the loop and gets written as DASHBOARD_URL=/api/v1/….
Downstream that produces a broken ::notice:: "link", a bare relative path in the step summary, and a DASHBOARD_URL the comment workflow silently rejects (its URL_RE requires an absolute https:// Unity host) — so the row degrades to a plain #id with no explanation. Cheap to make the two validators agree:
| if candidate and '/builds/' in candidate: | |
| if candidate and candidate.startswith('https://') and '/builds/' in candidate: |
| { | ||
| echo "UCB_ROWS<<UCB_ROWS_EOF" | ||
| printf '%s' "$ROWS" | ||
| echo "UCB_ROWS_EOF" | ||
| } >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Static heredoc delimiter on $GITHUB_ENV with artifact-derived content. This is not exploitable as written — $ROWS is built only from REPLY_ID (^[0-9]+$) and REPLY_URL (charset with no newline), so no line can equal UCB_ROWS_EOF — but the safety depends entirely on the validation two dozen lines up staying exactly as strict. A random delimiter makes it hold regardless:
| { | |
| echo "UCB_ROWS<<UCB_ROWS_EOF" | |
| printf '%s' "$ROWS" | |
| echo "UCB_ROWS_EOF" | |
| } >> "$GITHUB_ENV" | |
| DELIM="UCB_ROWS_EOF_$(uuidgen)" | |
| { | |
| echo "UCB_ROWS<<$DELIM" | |
| printf '%s' "$ROWS" | |
| echo "$DELIM" | |
| } >> "$GITHUB_ENV" |
Same applies to the UCB_SECTION block in comment-failed (L393-397).
| 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 |
There was a problem hiding this comment.
Branching on REPLY_URL first means a file with a valid DASHBOARD_URL but a missing/non-numeric BUILD_ID renders [#](https://…) — an empty link label. Reachable only with a tampered artifact (build.py always writes both), which is exactly the input this block is defending against.
| 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 | |
| if [ -n "$REPLY_ID" ] && [ -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 |
Same in the three sibling blocks (L272-276, L376-380, L382-386).
| 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') |
There was a problem hiding this comment.
build-ran is consumed by three jobs, and widening it here changes the meaning for all of them, not just the failure path this PR is targeting:
comment-failed— the intended fix. ✅comment-skipped(L98,build-ran == 'false') andcomment-success(L121,== 'true') — these split on the same flag.comment-successthen looks upDecentraland_windows64/Decentraland_macosids and interpolates them into download URLs. Before this change,build-ran == 'true'guaranteed at least oneDecentraland_*artifact existed; now a run whose conclusion issuccesswith onlyunity_build_info_*present posts "Windows and Mac build successful!" with…/artifacts/(empty id) links.
For an honest in-repo build that combination is hard to reach, but a fork PR controls its own copy of build-unitycloud.yml and can upload an arbitrarily-named artifact, so it's reachable on demand.
Suggest emitting two outputs and keeping the success/skipped split on the narrower one:
PLAYER=$(… startswith("Decentraland_") … | length)
INFO=$(… startswith("unity_build_info_") … | length)
echo "player-artifacts=$([ "$PLAYER" -gt 0 ] && echo true || echo false)" >> "$GITHUB_OUTPUT"
echo "build-ran=$([ $((PLAYER+INFO)) -gt 0 ] && echo true || echo false)" >> "$GITHUB_OUTPUT"
…with comment-success/comment-skipped gating on player-artifacts and comment-failed on build-ran.
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
| # 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./_%~?=&#-]*$' |
There was a problem hiding this comment.
The producer now requires '/builds/' in candidate (build.py:641) but the consumer still accepts any path under the three hosts, and the charset admits ?, =, & and %. On the tampered-artifact path that leaves one narrow primitive: a fork can upload DASHBOARD_URL=https://cloud.unity.com/<something>?next=https%3A%2F%2Fevil.example, which passes validation and renders as [#123](…) in the comment — a maintainer-facing link that reads as first-party. It only goes anywhere if Unity has an open redirect, so this is speculative, not a known bug.
Cheap to close by mirroring the producer's own constraint, which also keeps the two validators in sync:
| URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&#-]*$' | |
| URL_RE='^https://(cloud\.unity\.com|developer\.cloud\.unity3d\.com|dashboard\.unity3d\.com)/[A-Za-z0-9./_%~?=&#-]*/builds/[0-9]+[A-Za-z0-9./_%~?=&#-]*$' |
(Worth a quick check against a real dashboard URL first — if the id segment is followed by nothing, the trailing class still matches empty.)
| # 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" \ |
There was a problem hiding this comment.
Nit, pre-existing but this PR adds two more artifacts per run: gh api without --paginate returns only the first page (30 items) of /artifacts. A full two-target run currently uploads ~10–12 artifacts, so there's headroom — but build-ran silently going false because unity_build_info_* fell off page 1 would reproduce exactly the stuck-on-Pending bug this PR is fixing, and it'd be a puzzling one to diagnose.
| NAMES=$(gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts" \ | |
| NAMES=$(gh api --paginate "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" \ |
Note --paginate emits one JSON object per page, so the --jq '[.artifacts[].name]' would need --slurp or a flattening filter — ?per_page=100 alone is the smaller change if you'd rather not touch the jq.
…s, 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 <noreply@anthropic.com>
eordano
changed the title
This comment has been minimized.
This comment has been minimized.
| 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]"; " "))"' \ |
There was a problem hiding this comment.
The type-check + newline flattening stops the value from breaking the heredoc or the section fence, but the name still reaches the comment as raw markdown. failed_list has always had this property; the difference is that slowest_list renders on every run, including green ones, so the channel is now always open rather than only on failures.
A fork controls test.yml and the NUnit XML, so a test named [Download the fix](https://evil.example) renders in the bot's CI status comment as a plain, first-party-looking link. GitHub's sanitizer keeps <img> too (that's how the badges in this file work), and </details> in a name closes the block early and spills the rest into the comment body. Nothing escapes the `` fence — upsert-ci-status.sh:60 only strips whole-line markers and `awk` matches whole lines — so this is presentation/phishing surface, not structural.
Wrapping the name in inline code (after stripping backticks and pipes alongside the newlines) neutralises all of it in one place:
| '.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`|]"; " "))`"' \ |
Worth giving failed_list (L133) the same treatment while you're here — same input, same rendering path.
| try: | ||
| seconds = float(test_case.get("duration") or 0) | ||
| except ValueError: | ||
| seconds = 0.0 |
There was a problem hiding this comment.
float() accepts nan, inf and overflowing literals like 1e999 without raising — only malformed strings hit the ValueError branch. Any of those makes duration non-finite, and json.dump then writes bare NaN / Infinity, which is not valid JSON.
The consumer reads this file with jq under set -euo pipefail, and the very first read (jq -r '.hasResults' "$file", pr-comment-test-failures.yml:107) would abort the compose step — so one odd duration= attribute silently costs the whole tests status comment, not just the Time column. Before this commit the JSON held only ints and strings, so the failure mode is new.
| try: | |
| seconds = float(test_case.get("duration") or 0) | |
| except ValueError: | |
| seconds = 0.0 | |
| try: | |
| seconds = float(test_case.get("duration") or 0) | |
| except ValueError: | |
| seconds = 0.0 | |
| if not math.isfinite(seconds): | |
| seconds = 0.0 |
(needs math on the import at L808)
| @@ -179,3 +206,67 @@ jobs: | |||
| commit_sha: ${{ needs.resolve.outputs.head_short_sha }} | |||
| branch_label: ${{ needs.resolve.outputs.head_ref }} | |||
| secrets: inherit | |||
There was a problem hiding this comment.
Pre-existing, and I'm not counting it against this PR — but this commit adds two jobs to this file, so it's the natural moment to look at it.
secrets: inherit hands every secret on unity-explorer to a workflow resolved at @main in another repository. The header comment enumerates the six the suite actually needs (ALTTESTER_LICENSE, REPOS_READ_ONLY_TOKEN, the four DEV_EXPLORER_TEAM_*), so the blast radius is already documented — passing them explicitly costs four lines and drops the rest of the vault out of reach:
secrets:
ALTTESTER_LICENSE: ${{ secrets.ALTTESTER_LICENSE }}
REPOS_READ_ONLY_TOKEN: ${{ secrets.REPOS_READ_ONLY_TOKEN }}
# …the four DEV_EXPLORER_TEAM_* valuesSHA-pinning the reusable workflow isn't the right answer here — the @main rationale in the comment above is sound, and a pin would freeze the suite. Narrowing inherit is the part that's free.
Related, on the same file: top-level permissions: contents: write (L38-40) is inherited by this call as the ceiling for the callee's GITHUB_TOKEN, and neither of the new jobs needs it — they only comment. Job-level permissions: {contents: read, pull-requests: write} on automation-pending and report would at least keep the new surface minimal, even if the top-level block has to stay for the callee.
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 <noreply@anthropic.com>
eordano
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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/<digits>).
This comment has been minimized.
This comment has been minimized.
… 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.
- The comment header shows the Decentraland logo instead of the traffic-light emoji, wrapped in <picture> — 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.
This comment has been minimized.
This comment has been minimized.
eordano
changed the title
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…bel the perf lanes - build.py's build flow moves under a __main__ guard (top-level if, so module-global semantics are unchanged), making the helpers importable; scripts/cloudbuild/test_build_helpers.py covers _platform_key, _dashboard_build_url (including a drift guard asserting the constructed URL passes ucb-build-links' URL_RE), and the link-info writer's href/elapsed/rewrite semantics. - test-upsert-ci-status.sh exercises upsert-ci-status.sh against a stubbed gh: skeleton creation, section isolation, fence append, header migration, marker stripping, duplicate GC, NO_CREATE exit 3, section allowlist exit 2, and truncation re-closing constructs. - New ci-scripts-tests.yml runs both suites on PRs touching these paths. - Durations render with one h/m/s convention everywhere (fmt_secs gains hours and drops zero-padding; the lint footer follows suit), with lockstep notes at each copy. - The shared performance section's two writers now label their lanes in the rendered body, so a dispatch status replacing a suite verdict is legible; the lint Pending badge joins the readable named yellow.
This comment has been minimized.
This comment has been minimized.
eordano
added
force-build
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…line - artifact-url grants actions:read (the composite's cross-run artifact reads 403 without it), gains a comment-cancelled job so a cancelled build can't leave the live In-progress claim up, and marks the link/size/compose steps continue-on-error so the status write always lands - upsert-ci-status normalizes CRLF out of the body and every API read (a web-UI edit resubmits \r\n and defeated the whole-line marker matching), retries failed POST/PATCH inside the loop instead of dying under set -e, and warns when the whole comment nears GitHub's 65k cap; new functional test covers the CRLF round trip - the composite documents the 128KiB env-transport limit and test-failures bounds its only unbounded list at composition - build.py: comment reads distinguish 'absent' from 'unreadable' so a transient 502 can't compose a section that wipes the sibling row (page bound raised 3->30); failed upsert writes no longer count as asserts; record + reconcile both run every poll, so a missing dashboard href or a failed info-file write keeps retrying instead of stranding - visual-regression orders the Running write before the suite so it can never overwrite the final verdict, and probes the Allure URL before rendering it as a link - pr-comment-perf resolves fork-PR numbers via the commit->PRs lookup when workflow_run.pull_requests is empty - the unit tests silence build.py's prints so its ::notice:: line stops annotating the test job's check run Not changed: the dashboard URL's org/project ids in public comments stay by explicit earlier decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018c638dR1vPysCMbYt2qQg5
|
Claude finished @eordano's task in 7m 11s —— View job Security-focused review — round 21 (
|
| From | Finding | Status |
|---|---|---|
| r20 #1 | bare gh api --paginate reads at upsert L168/L253 die under set -e |
open — now also caps finding 4 |
| r20 #3 | 60k cap bounds failed_list only; slowest_list names unbounded in length (L157) |
open |
| r20 #4 | run-suite's !cancelled() widens past resolve failing (visual-regression.yml:204-205) |
open |
| r20 #5 | timed_out / stale / neutral / failure-without-artifacts still strand "New build in progress" |
open |
| r20 #6 | single unretried curl -sfIL gates the Allure link (L266) |
open |
| r20 #7 | ${failed_list:0:60000} can sever an inline-code pair |
open |
| r19 #2 | _DASHBOARD_LINK_RE.match() admits a trailing \n; the shell twin doesn't (build.py:674) |
open |
| r19 #3 | static SIZE_REPORT<<EOF beside the randomized PLATFORM_ROWS delimiter |
open |
STEP W — workflow surface
pr-comment-artifact-url.yml is the only workflow touched, and the change adds five env: entries to one step. No new trigger, no new uses:, no permissions: change, no ${{ }} interpolated into a run: body, no prompt file. The five values are GitHub-API-derived (workflow_run.check_suite_id, artifact ids) or context literals, and step-level env: values are not shell-evaluated. Nothing in W.1–W.10 is newly triggered.
Dependency review (STEP 1–3)
git diff 8f4123e53..HEAD touches three files: one workflow, build.py, test_build_helpers.py. No Packages/manifest.json, no packages-lock.json, no .asmdef, no .dll/.so/.dylib, no requirements.txt, no install hooks. Across the whole PR, git diff origin/dev...HEAD -- Packages/ Explorer/ scripts/cloudbuild/requirements.txt '*.asmdef' '*.dll' '*.so' '*.dylib' is still empty. Nothing ships to the desktop client; STEP 1.5 registry lookups N/A.
| dependency / file | version | source | type | scope | risk | confidence |
|---|---|---|---|---|---|---|
requests (scripts/cloudbuild/requirements.txt, unchanged) |
==2.32.2 |
PyPI | source (pure Python) | CI build job only | LOW | HIGH |
| (no manifest / lockfile / plugin / asmdef / binary changes anywhere in the PR) | — | — | — | — | — | HIGH |
The new-dependency label remains a labeler false positive.
Verdict
The mechanical asks — named thresholds, explicit env: — are done cleanly. The race fix delivers real value in one direction and is documented and tested for the other; that mismatch is finding 1 and it's the only thing I'd want the author to look at before merge, and even then as a comment/test correction plus an optional root-cause move of the merge into the shell. Findings 2–6 are LOW-and-below.
I still can't execute here — python3 -m unittest discover -s scripts/cloudbuild and bash .github/actions/ci-status-comment/test-upsert-ci-status.sh were both denied this round as in rounds 17–20, so everything above is from reading. ci-scripts-tests.yml runs both on any push touching scripts/cloudbuild/** or .github/actions/ci-status-comment/**, which is what keeps the three new test cases honest.
DEPENDENCY_REVIEW: NEEDS_ATTENTION — unchanged in substance from rounds 19–20. The sole item driving it is still pre-existing and already triaged: visual-regression.yml:213 calls decentraland/explorer-automation/.github/workflows/run-visual-suite.yml@main with secrets: inherit (W.7a-shaped). The author's rationale is on record and I agree with it — the callee declares no workflow_call secrets, so explicit passing is a syntax error until that repo changes first. Two-repo migration, not this PR's work. None of findings 1–6 block the merge.
|
PR #9713, run #32033813439 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
2 participants
Pull Request Description
What does this PR change?
Navigating from a PR to what CI actually did is currently manual: the status comment's badges are bare shields.io images (clicking one opens the image), the Unity Cloud build behind a run must be found by searching cloud.unity.com by hand, and the tests/lint sections link to nothing — the dorny test report, the NUnit XMLs with per-test timings, the InspectCode report, and the visual-regression Allure report are all unreachable from the comment. This PR turns the unified CI status comment into a link hub:
Build section
scripts/cloudbuild/build.pycaptures the Unity Cloud dashboard deep link (links.dashboard_summary/dashboard_log) from the build-API response, prints it as a::notice::, adds it to the step summary, and persists it tounity_cloud_build_info.envthe moment the build id is known — so it exists for failed builds too.build-unitycloud.ymluploads it as aunity_build_info_<target>_<source>artifact (7-day retention).Windows build | Unity Cloud #4242 · GitHub job— on the success and failure comment. All badges link the Actions run.check-build-ranemits two outputs:player-artifactskeeps gating the success/skipped split (success comments never interpolate missing artifact ids), whilebuild-ran(widened byunity_build_info_*) gates the failure path — a build that fails before producing player artifacts now posts the failure comment with its Unity Cloud link instead of sticking on "Pending".Tests section (
pr-comment-test-failures.yml+ the extractor intest.yml)Test (editmode/playmode)job.Timecolumn with suite duration and a collapsible Slowest tests top-10 per suite, parsed from the NUnit XML by the extractor and type-checked by the trusted composer (numeric seconds, single-line names) before rendering.Test results (…)artifacts (full NUnit XML + Unity editor logs).Lint section (
pr-comment-warnings.yml)csharp-lint-reportsartifact (the inline findings list is capped, the artifact has everything).Performance section (new,
pr-comment-artifact-url.yml+ newpr-comment-perf.yml)comment-successdispatches the bare-metal benchmark (decentraland/performance-testing) after a successful build, the section flips to Dispatched linking that workflow's run queue (repository_dispatch returns no run id); the benchmark's ownperf-test-summarycomment stays the detailed result.perf_test-label lane, the new companionpr-comment-perf.ymlwrites Passed/Failed with links to the run summary (which renders the generated benchmark report) and thePerformance test results (JSON)/Performance benchmark report (PDF)artifacts. "Unity Performance Test" fires on every PR event but gates on the label at job level, so the companion checks via the jobs API that the perf job actually ran before touching the section — a skipped run never overwrites the bare-metal dispatch status.perf_testskips normal CI and blocks merge while set).Automation section (new,
visual-regression.yml+ci-status-comment)/visual-tests. When the suite dispatches it flips to Running (linked to the run), and lands on Passed/Failed with the Allure report and run links. The reusable workflow's own detailed per-platform comment is unchanged.upsert-ci-status.shnow appends a missing section fence to existing comments instead of resetting the whole comment to the skeleton — without this, the automation section's first write would have wiped the build/lint/tests state on every open PR.Security notes
unity_build_info_*files are produced inside the PR-controlled build workflow, so the consumer (composite action.github/actions/ucb-build-links, shared by the success/failure jobs) treats them as untrusted: numeric build id, Unity-dashboard-origin URL regex, unique heredoc delimiters, tampered rows dropped,gh downloaderrors surfaced instead of swallowed. Same treatment for the duration/slowest fields added to the failed-tests artifact.vars.Test Instructions
This is a CI-only change (no client code);
metaforge explorer rundoes not apply.Steps (standard run):
# CI-only change — N/AExpected result: N/A
Steps (fresh account):
# CI-only change — N/AExpected result: N/A
Automation (if applicable): N/A
Prerequisites
force-buildlabel to this PR (it touches noExplorer/**files, so prebuild would otherwise skip the build), or run Unity Cloud Build viaworkflow_dispatchon this branch.Test Steps
::notice::Unity Cloud build #<id> …annotation opens the build's Unity Cloud page, the step summary carries the link, andunity_build_info_*artifacts exist.dev(all comment workflows run from the default branch, so the new comment layout appears for runs after the merge):Windows/Mac buildrows with Unity Cloud + GitHub job links on success and failure; badges link the run.Timecolumn,Slowest testsdetails, artifact footer.perf_test-labeled PR it shows Passed/Failed with the report + artifact links./visual-testshint; commenting/visual-testsflips it to Running and then to Passed/Failed with the Allure report link.Additional Testing Notes
ghand fixture artifacts: build-row pairing, tests-section composer (incl. rejection of a tamperedslowestentry), and the upsert append-section path (existing sections preserved, automation fence appended).unity_build_info_*/duration data; rows and columns degrade to today's rendering.run-visual-suite.yml's S3 path derivation (mode=test,platform=macosdefaults) — noted in-code to keep them in lockstep.Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.