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 31 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
Build failed! Check the logs to see what went wrong. 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}
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.
…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.
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.
…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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
PR #9713, run #33200618068 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
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.
PR Review: chore(ci): link Unity Cloud builds, test reports, timings, performance and automation from the CI status comment
Step 2 — Root-cause check: PASS
The PR solves the correct problem: CI status comment badges were unlinked images, Unity Cloud builds had to be found by hand-searching cloud.unity.com, and test/lint/automation reports were unreachable from the comment. The diff adds links at their source (workflow output, API response, artifact ID) and composes them into the comment. This addresses the cause — missing linkage — not a symptom.
Step 3 — Design & integration: PASS
This is CI infrastructure (GitHub Actions YAML, bash, Python). No ECS systems, components, or runtime code.
Concurrent-write model — upsert-ci-status.sh uses read-modify-write with a 5-attempt verify-and-retry loop that re-reads after each write to confirm the section landed and no duplicate slipped in. The new on-demand section approach (inworld fence appended on first report, not in the skeleton) avoids permanent "waiting" noise on PRs that never run the suite. The NO_CREATE flag for external callers prevents foreign-token comment creation that would spawn unfindable duplicates.
Live comment updates from build.py — Race detection is well-designed: reads the section, composes a row union preserving any sibling platform's row, writes, re-reads to verify the composed union survived, and retries on mismatch (bounded by LIVE_COMMENT_WRITE_ATTEMPTS=3). Rate-limited with MAX_LIVE_ASSERTS=3, MAX_LIVE_CONFIRMS=3, and LIVE_RECONCILE_INTERVAL_SECS=240 to avoid burning the comments-API budget.
Two-tier gating — player-artifacts gates the success comment (which interpolates artifact IDs that must exist), while build-ran (widened by unity_build_info_*) gates the failure path so builds failing before producing player artifacts still post a failure comment with the Unity Cloud link. Clean separation.
comment-cancelled job — Correctly handles the gap where a cancelled build leaves an "In progress" badge stale. Fires for any cancelled run regardless of whether a build started — the right tradeoff ("Cancelled" is accurate and avoids stuck state).
Step 4 — Member audit: No issues
All new Python functions have clear responsibilities and appropriate consumer counts:
record_build_link_info— 2 callers (poll loop,record_final_elapsed). Persists dashboard link + timing to file._github_api— 2 callers (_build_section_of_status_comment,_own_job_url). Shared HTTP helper._build_section_of_status_comment— 1 caller (upsert_live_comment). Extracts current build section from the live comment.upsert_live_comment— 1 caller (maybe_update_live_comment). Distinct responsibility from the rate-limiting/budget logic.maybe_update_live_comment— 2 callers (record_build_link_info, poll loop). Gate and rate-limiter.
No single-use merge candidates or derived-predicate smell.
Step 5 — Line-level review: No blocking issues
Security review: No security issues found
- Untrusted artifact data is validated: build IDs as
^[0-9]+$, URLs against a compiled allowlist regex (_DASHBOARD_LINK_RE/URL_RE— drift-guarded bytest_matches_consumer_url_re), durations as numeric-only, test names sanitized (gsub("[\r\n\x60|]"; " ")). - Marker-shaped lines stripped from section bodies to prevent comment structure injection (tested in scenario 4 and 9 of
test-upsert-ci-status.sh). - PAT expiry probe (
HEAD /rate_limit) keeps the token in env (out of argv viaGH_TOKEN="$PAT") and surfaces only the expiry date, not the token. - Permissions explicitly scoped per job —
pull-requests: writeonly where the comment upsert runs,{}on the pure-bash gate job. - Oversized body truncation re-closes severed code fences and
<details>blocks to prevent rendering bleed into neighbouring sections. - CRLF normalization (
${SECTION_BODY//$'\r'/}) applied to both incoming bodies and API reads, preventing duplicate fences from a GitHub web-editor round-trip.
P2 observations (non-blocking)
-
Duration formatter duplication —
fmt_dur/fmt_secsis implemented three times (ucb-build-links, pr-comment-test-failures, pr-comment-warnings). The PR description notes "keep the three in lockstep" and each uses a slightly different shell dialect (bash arithmetic vs. awk). Acceptable for independent CI scripts that run in different job contexts. -
math.isfinite()belt-and-suspenders intest.yml— Guarding both individualfloat()results and the accumulated sum againstnan/inf/1e999is correct:float()admits non-finite values without raising, andjson.dumpwould emit bareNaN/Infinity— invalid JSON that breaks the consumer's very firstjqread. -
Backward compatibility — Old PRs without
unity_build_info_*or duration data gracefully degrade: rows omit the parts whose data was absent, and thecontinue-on-error: trueon the fetch/compose steps means a missing artifact silently produces empty cells rather than failing the job.
Tests: Comprehensive
test-upsert-ci-status.sh (11 scenarios): skeleton creation, section update preservation, missing fence append + header migration, marker-shaped body stripping, duplicate GC, NO_CREATE exit code, unknown section rejection, oversized body truncation with construct re-closing, embedded own-section marker wedge, stale re-read convergence, CRLF normalization.
test_build_helpers.py: platform key extraction (template targets, branch-derived, unknown), dashboard URL construction + consumer-regex drift guard, link info file lifecycle (first write, API href replacement, final elapsed rewrite, rejection of non-build links and consumer-failing links, negative elapsed clamping), live comment reconcile budgets (failed first write retry, caps), and the sibling-row race recovery with a pinned multi-attempt scenario.
Step 6 — Complexity: COMPLEX
17 files, +2021/−209 lines across GitHub Actions workflows, bash CI scripts, and the Python build orchestrator. Introduces concurrent-write handling, race detection, live comment updates, new workflow jobs, and comprehensive test coverage.
Step 7 — QA: NO
Changes are limited to CI/CD workflows (.github/), bash scripts, and Python build scripts. No runtime code is affected — no Unity player code, no user-facing behavior changes.
Step 8 — Non-blocking warnings: None
Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches 17 CI infrastructure files across GitHub Actions workflows, bash scripts, and the Python build orchestrator with concurrent-write handling and race detection.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
There was a problem hiding this comment.
Re-review
STEP 1 — Load context & set scope
Read CLAUDE.md, docs/README.md, and docs/build-and-ci.md; checked out PR #9713 locally and reviewed the diff against origin/dev. Changed files are CI workflows/actions plus scripts/cloudbuild/build.py and its Python tests. I also ran the shell functional tests and Python helper tests locally.
STEP 2 — Root-cause check
The PR is solving a real navigation gap: CI status comments did not link Unity Cloud builds, detailed reports, timings, performance runs, or automation outputs. The artifact-based workflow_run sections address the root cause. The new live build-comment path, however, moves part of comment writing into PR-head build code instead of keeping writes in the trusted post-run comment workflows.
STEP 3 — Design & integration
Relevant owners searched/read: .github/actions/ci-status-comment/upsert-ci-status.sh owns the unified comment upsert; .github/actions/ci-status-comment/action.yml wraps it; .github/workflows/pr-comment-artifact-url.yml, pr-comment-test-failures.yml, pr-comment-warnings.yml, pr-comment-perf.yml, and visual-regression.yml are the trusted section writers; scripts/cloudbuild/build.py owns Unity Cloud build lifecycle. No new long-lived runtime/ECS unit is introduced. The artifact-based design fits the existing owner model, but the live comment writer in build.py duplicates the trusted comment-writer responsibility and requires a write-capable token in code checked out from the PR head.
Teardown/consumption trace: Unity build link artifacts are consumed by .github/actions/ucb-build-links/action.yml; the build-status section is consumed by upsert-ci-status.sh; temporary files in build.py live-comment writes are unlinked in finally; downloaded artifacts are job-local. No subscription/resource leak found.
STEP 4 — Member audit
No public C# members/accessors were added or changed. New Python helpers are module-private workflow helpers and are covered by scripts/cloudbuild/test_build_helpers.py.
STEP 5 — Line-level review
[P1] The build job grants pull-requests: write and runs PR-head scripts/cloudbuild/build.py with GH_TOKEN. That breaks the trust boundary already used by the CI comment workflows: PR-controlled code should produce artifacts, while trusted default-branch workflows should write comments. See inline suggestions.
Security review: one security issue found — the PR-head build job receives a PR-write token. No hardcoded secrets or shell-injection issues found in the new artifact parser path; untrusted Unity build-info artifacts are validated before rendering by .github/actions/ucb-build-links/action.yml.
Sub-agents run: security-sentinel, architecture-strategist, pattern-recognition-specialist, code-simplicity-reviewer, kieran-python-reviewer.
STEP 6 — Complexity assessment
COMPLEX: 17 files and ~2k LOC changed across GitHub Actions orchestration, shell comment upsert logic, and the Unity Cloud Python build handler.
STEP 7 — QA assessment
QA_REQUIRED: NO — changes are limited to CI/workflow/scripts and do not affect Unity player runtime behavior.
STEP 8 — Non-blocking warnings
No main scene changes. Local checks run: python -m py_compile scripts/cloudbuild/build.py scripts/cloudbuild/test_build_helpers.py, python scripts/cloudbuild/test_build_helpers.py, and bash .github/actions/ci-status-comment/test-upsert-ci-status.sh all passed. GitHub checks currently show the Windows build/build gate failing and enforce-approvals failing.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: CI orchestration, comment-writing workflows/actions, and the Unity Cloud Python build handler are all changed.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
This comment has been minimized.
This comment has been minimized.
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
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 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…comment 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 <!-- ci-status --> marker, matching the finders in the perf and InWorld folds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Claude finished @dalkia's task in 1m 48s —— View job Security-focused review — round 29 (
|
| Check | Result |
|---|---|
git diff d4eb6b2f1 b738c353b -- .github/ scripts/ |
empty — every PR-owned file is unchanged from round 28 |
git diff --name-only ee7128d62 b738c353b -- Explorer/ unity-shared-dependencies/ avatar-preview-renderer/ |
empty — the merge added nothing on top of dev outside the PR surface |
git rev-list --count HEAD..origin/dev |
0 — the branch is fully current with dev; no stale-base drift left |
So git diff --name-only ee7128d62 b738c353b is exactly the 18 PR files, and git diff --stat origin/dev...HEAD is unchanged at 18 files, +2038/−210. There is no new code to review; nothing I verified in rounds 1–28 can have regressed here, and I re-read the three values my open findings anchor on to confirm the line references still hold (CAP=10000 at :57, the six-entry SECTION allowlist at :94, the two bare --paginate --slurp calls at :180/:270).
STEP W — nothing to bind to
No workflow or prompt file changed in this commit. Across the whole PR the trigger surface is unchanged since round 28: no pull_request_target anywhere (the only grep hit is the explanatory comment at build-unitycloud.yml:823 recording why the pull_request trigger makes the build job's pull-requests: write safe), no new ${{ github.event.* }} interpolated into a run: body, no heredoc regressions, no LLM invocation, no allowedTools, no retention change, .github/prompts/** untouched.
STEP 1–3 — N/A
The dependency-shaped glob over Packages/**, Explorer/** plugins, *.asmdef, *.dll/.so/.dylib/.bundle, **/requirements.txt, **/package.json returns nothing for origin/dev...HEAD. scripts/cloudbuild/requirements.txt is untouched by this PR and still the single pinned requests==2.32.2. The new-dependency label remains a labeler false positive.
Carried findings — all unchanged (same file bytes)
| From | Finding | Status |
|---|---|---|
| r23 #1 | CAP × 6 sections over the 65536 ceiling |
✅ fixed in 855249521 |
| r28 #1 | create-release-branch.yml has no permissions: block at all while it force-pushes, opens a PR and PATCHes comments |
open — NIT |
| r27 #1 | CAP=10000 sits below MAX_FINDINGS: 50's rendered size and the 60000 failed_list bound, so specific artifact-pointer notes get replaced by the generic one |
open — NIT |
| r23 #2 / #3 | section_default inworld never rendered; automation default describes only the /visual-tests lane |
open — NIT |
| r22 #1–#3 | !cancelled() && …authorized lets a failed resolve dispatch an empty build_url; resolve has no job-level permissions:; "(unnamed)" on failed but not timings |
open |
| r21 #1–#4 | post-write verify checks "my rows present", not "I dropped a sibling's row"; _live_comment_last_attempt stamped before a ≤540s call; only_if_missing short-circuits every retry; returncode != 0 → return None spends no retry |
open |
| r20 #1 | bare gh api … --paginate --slurp dies under set -e |
open |
| r20 #5–#7 | timed_out/stale/neutral strand the build section; single unretried curl -sfIL gates the Allure link; ${failed_list:0:60000} can sever an inline-code pair |
open |
| r19 #2/#3 | _DASHBOARD_LINK_RE.match() $ admits a trailing \n; static SIZE_REPORT<<EOF beside the randomized PLATFORM_ROWS delimiter |
open |
None of these block. Every one is a NIT or a bounded-degradation case in a status-comment path that already degrades gracefully, and each has been triaged in an earlier round.
Test suites
CI Scripts Tests is green on this merge head (run 33541059567) — the 12-case test-upsert-ci-status.sh suite and test_build_helpers.py both pass at CAP=10000.
As in rounds 17–28, I could not execute either suite locally: bash .github/actions/ci-status-comment/test-upsert-ci-status.sh was denied by the tool policy. Everything above is from git plumbing and reading the tree; CI's green run is the executable evidence.
Verdict
A clean, up-to-date merge with no functional delta on the reviewed surface. Nothing outstanding blocks the merge from my side. The only red check is Enforce QA and DEV Approvals — an approvals gate, not a code finding — and dalkia has since approved.
DEPENDENCY_REVIEW: NEEDS_ATTENTION — unchanged in substance since round 19 and untouched by this commit. Still driven by the same pre-existing, already-triaged W.7a item: visual-regression.yml:183 and in-world-tests.yml:190 both call decentraland/explorer-automation reusables at @main with secrets: inherit. Neither line is this PR's, and both need the callee to declare workflow_call secrets before explicit passing is syntactically possible.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Final Review — chore(ci): link Unity Cloud builds, test reports, timings, performance and automation from the CI status comment
STEP 1 — Load context & set scope
Read CLAUDE.md, docs/README.md. Loaded the diff (18 files, +2038/−210). Changed files are CI-only: GitHub Actions workflows/actions, bash scripts (upsert-ci-status.sh, test-upsert-ci-status.sh), Python (build.py, test_build_helpers.py), and a new composite action (ucb-build-links). No runtime Unity code is touched.
Reviewed inline comments from previous reviews (20+ findings from Aug 12–13, two full reviews from Aug 31). Confirmed that prior P1 findings (URL validation alignment, heredoc delimiters, player-artifacts/build-ran split, grep -qxF whole-line matching, failure() && dispatch guard, PAT probe continue-on-error, NO_CREATE delay, truncation with fence re-closing, math.isfinite() guards, test name sanitization, _platform_key() prefix match) were addressed in subsequent commits.
Ran parallel sub-agents: security-sentinel, architecture-strategist, code-quality-reviewer.
STEP 2 — Root-cause check: PASS
The PR solves a real navigation gap: CI status comment badges were dead-end images, Unity Cloud builds had to be found by hand-searching cloud.unity.com, and test/lint/performance/automation reports were unreachable from the comment. The diff adds links at their source (workflow output, API response, artifact ID) and composes them into the comment. This addresses the cause — missing linkage — not a symptom.
STEP 3 — Design & integration: PASS
No ECS, runtime, or scene code. The section-fence model with concurrent-write safety is well-designed:
- Skeleton vs on-demand sections: 5 always-present sections seeded in the skeleton, inworld appended lazily. The
grep -qxFwhole-line match prevents embedded marker-shaped content from being mistaken for a real fence. - Concurrent writer safety:
upsert-ci-status.sh's 5-attempt verify-and-retry loop guarantees a fixed body lands in a section. Where content is a union from multiple concurrent writers (live build rows from windows64 + macos),build.py'supsert_live_commentlayers its own read-compose-write-verify on top — two different problems at two different layers, with a dedicated regression test (UpsertLiveCommentRaceTest) pinning the exact race. - Two-tier gating:
player-artifactsgates success/skipped (which interpolate artifact IDs that must exist),build-rangates failure (any evidence of a build). Clean split. - External caller contract:
NO_CREATEprevents foreign-token orphan comments;SECTION_BODY_FILEfor bodies too large for env. - Trust boundary (re-verified): The
buildjob runs PR-head code withpull-requests: writeon apull_requesttrigger. Forks get a read-only token regardless ofpermissions:and can't run the job without Unity Cloud secrets; same-repo branch authors already hold write access. No escalation path. Rationale correctly documented inbuild-unitycloud.yml.
Teardown trace: temp files in upsert_live_comment cleaned in finally; downloaded artifacts job-local. No resource leak.
STEP 4 — Member audit: N/A
No public C# members added/changed. Python helpers are module-private and covered by test_build_helpers.py.
STEP 5 — Line-level review
Security review: Ran the security-review skill checklist. Untrusted artifact data validated (numeric IDs, URL regex with drift guard, test name sanitization). CRLF normalization applied consistently. Marker-shaped lines stripped. PAT stays in env (not argv). secrets: inherit on visual-regression.yml's suite call is pre-existing and unchanged — the two new jobs scope down to contents: read + pull-requests: write. No security issues found in new code.
Three P2 findings — see inline comments with suggestion blocks.
STEP 6 — Complexity: COMPLEX
18 files, +2038/−210 across GitHub Actions workflows, bash CI scripts, Python build orchestrator, and two new test suites. Concurrent-write handling, race detection, live comment updates.
STEP 7 — QA: NO
CI-only changes. No runtime code, no user-facing behavior.
STEP 8 — Non-blocking warnings: None
Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: CI orchestration, comment-writing workflows/actions, and the Unity Cloud Python build handler changed across 18 files.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
| 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 |
There was a problem hiding this comment.
[P2] Documentation says "over 20000" but the actual CAP in upsert-ci-status.sh is 10000 (6 × 10000 = 60000, with headroom under 65536). External callers relying on the documented threshold will hit truncation at half the expected size.
| section markers after dropping marker-shaped lines; bodies over 20000 | |
| section markers after dropping marker-shaped lines; bodies over 10000 |
| while :; do | ||
| TRUNCATED="${SECTION_BODY:0:CUT}" | ||
| if [ $(( $(grep -c '^```' <<< "$TRUNCATED") % 2 )) -ne 0 ]; then | ||
| TRUNCATED="$TRUNCATED"$'\n''```' | ||
| fi |
There was a problem hiding this comment.
[P2] Two improvements for the truncation loop:
-
Iteration cap — the loop terminates (CUT strictly decreases to 0, body degenerates to just NOTE) but has no safety valve on iteration count. A section body with many
<details>tags from adversarial test names could force many iterations, each forking grep/wc. A cap of 10 is generous and makes the bound explicit. -
Single-backtick rebalancing — fences and
<details>are rebalanced but inline backticks are not. Test names render in`pairs; a cut between the pair leaves a dangling backtick that swallows subsequent sections as monospace — the same rendering-bleed this block exists to prevent, just for a different construct.
| while :; do | |
| TRUNCATED="${SECTION_BODY:0:CUT}" | |
| if [ $(( $(grep -c '^```' <<< "$TRUNCATED") % 2 )) -ne 0 ]; then | |
| TRUNCATED="$TRUNCATED"$'\n''```' | |
| fi | |
| for _rebalance in 1 2 3 4 5 6 7 8 9 10; do | |
| TRUNCATED="${SECTION_BODY:0:CUT}" | |
| if [ $(( $(grep -c '^```' <<< "$TRUNCATED") % 2 )) -ne 0 ]; then | |
| TRUNCATED="$TRUNCATED"$'\n''```' | |
| fi | |
| # Close a dangling inline backtick — test names arrive in `…` pairs, | |
| # and a cut between the pair eats everything after it as monospace. | |
| ticks=$(grep -o '`' <<< "$TRUNCATED" | wc -l || true) | |
| if [ $(( ticks % 2 )) -ne 0 ]; then | |
| TRUNCATED="$TRUNCATED"'`' | |
| fi |
| 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}') |
There was a problem hiding this comment.
[P2] Defense-in-depth: the artifact-based path (ucb-build-links) validates URLs against URL_RE, but the live-comment path interpolates dashboard_url or the constructed URL directly into a markdown link without re-checking _DASHBOARD_LINK_RE. Today this is safe because TARGET is sanitized upstream in clone_current_target(), but the safety depends on that unrelated sanitizer — there's no drift guard here the way test_matches_consumer_url_re pins the artifact path.
| 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}') | |
| link = dashboard_url or _dashboard_build_url(build_id) | |
| if link and not _DASHBOARD_LINK_RE.match(link): | |
| link = None | |
| parts.append(f'[Unity Cloud #{build_id}]({link})' if link else f'Unity Cloud build {build_id}') |
3 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 itself then rewrites this section with its full report (performance-testing#19, approved), falling back to the classic standaloneperf-test-summarycomment only when the section write fails.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.InWorld section (new, on-demand)
inworldsection that is not part of the skeleton: the suite only runs on release/hotfix PRs into main, so the fence is appended the first time it reports instead of showing a permanent "waiting" row on every PR. 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 (covered by a new functional test).decentraland/explorer-automation'srun-inworld-suite.ymlfolds its results table into this section when unity-explorer is the caller (explorer-automation#86), retiring the standalone## InWorld suitecomment; other callers and any failed section write keep the standalone path.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.create-release-branch.ymlfinds its standalone "latest dev build" comment bygithub-actions[bot]+ animg.shields.io/badge/Buildbadge — which the unified comment's build section now also carries. Its finder is scoped to exclude the<!-- ci-status -->comment, so on a release PR it no longer short-circuits on the unified badge or PATCHes the whole unified comment away. (Fully collapsing release-PR comments into one is tracked as a follow-up in #9936.)Rollout / merge order
This PR is the keystone of a three-repo change; the other two check out this repo's
ci-status-commentaction fromdevat runtime and depend on theinworld/performancesections plus theNO_CREATE+SECTION_BODY_FILEexternal-caller contract introduced here.devfirst. The comment workflows run from the default branch, so the new unified layout appears for runs after the merge.The two companions are independent repos with no shared merge gate, and both degrade to today's standalone-comment behaviour when
dev's action does not yet support them (#19 greps the script forSECTION_BODY_FILE/NO_CREATEand refuses; #86 falls back on theinworldallowlist'sexit 2). So merging a companion early is harmless — it keeps posting its own comment until this PR lands, then silently switches to folding. There is no window where a companion duplicates or corrupts the unified comment.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.buildjob runs PR-head code while holdingpull-requests: write(for the live comment upsert). A re-review flagged this as a trust boundary; it is not one, and the rationale is now recorded atbuild-unitycloud.yml: the trigger ispull_request(notpull_request_target), so a fork gets a read-only token thatpermissions:cannot escalate and cannot run the job without the Unity Cloud secrets anyway, and a same-repo branch's author already holds the capability with write access.SECTIONallowlist grew to six, so the oldCAP=20000(6 × 20000 = 120000) could exceed GitHub's 65,536-char comment ceiling — the very 422 it exists to prevent. NowCAP=10000(6 × 10000 = 60000, with headroom for the header/fences), with the invariant noted in-code so a seventh section forces a re-derive; the${#NEW_BODY} > 65000guard remains the runtime backstop.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.## InWorld suitecomment is retired to a one-line pointer.Additional Testing Notes
NO_CREATE=1+SECTION_BODY_FILE).performancesection, rendering the linked Regression badge (red, report auto-expanded because a regression is present) with the report'sOverall: 🔴 …headline and per-runner tables — the badge is sourced from that same headline, so it can't contradict the report. Flip the stub'sBADGE_STATEfor theNo regressions(green) /Partial(amber) /Failed!(red) variants.ghand fixture artifacts: build-row pairing, tests-section composer (incl. rejection of a tamperedslowestentry), and the upsert append-section path (existing sections preserved, automation + inworld fences appended); thetest-upsert-ci-status.shsuite (12 cases, incl. the on-demand inworld create/append case) passes.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.