chore: gate release and hotfix PRs on the InWorld suite - #9694
Conversation
visual-regression.yml carried the only copy of "find the Unity Cloud Build
artifact for this commit": the build-workflow run lookup, the event->prefix
table that mirrors build-unitycloud.yml, and the explorer-automation branch
match. A second dispatcher for the InWorld suite needs all three, so move
them into a composite action instead of copying them.
Two things the extracted version does that the inline copy did not:
- waits, optionally. A caller triggered by a human can require the build to
already exist; a caller that fires when a PR opens has to wait out the
build that same event started. `wait-minutes` defaults to 0, so
visual-regression.yml keeps failing fast.
- HEAD-checks the artifact. A successful build run does not guarantee a
macOS zip — build-unitycloud.yml takes a `platforms` input — and the
same failure inside the suite costs a whole macOS runner slot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a dispatcher for explorer-automation's run-inworld-suite.yml, the InWorld counterpart to visual-regression.yml. Both share the build resolver, so this file is a trigger, a call, and a gate. It runs on pull_request rather than on the build finishing. A workflow_run reports its check against the default branch's SHA, so it never joins the PR's checks and can never block a merge; the requirement here is that a red suite does block one. The cost is that the first job waits out the ~40-minute build the same event started, which is cheap on a public repo's runners and shows up honestly as a pending check meanwhile. `InWorld suite result` is the job to add to main's required status checks. It is stable across renames inside the reusable workflow, it fails on a cancelled run rather than going green, and it passes for PRs into main that are not release or hotfix — so requiring it blocks nothing else. Drafts are skipped, matching build-unitycloud.yml, which does not build them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same behaviour, minus the 55 lines the composite now owns. The resolve job gains a sparse checkout, because `uses: ./…` resolves against the workspace and this job never had one. Three deliberate differences: an unreachable artifact now fails here in seconds instead of on the macOS runner; `actions: read` is stated explicitly rather than relying on GITHUB_TOKEN's read access to a public repo's data; and the header names the EXPLORER_TEAM_* secrets this repo actually needs. It listed the DEV_ family, which went stale when explorer-automation#51 made the reusable pick a family by caller repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚦 CI Status
Build skipped — no changes detected under Waiting for lint to start… Waiting for tests to start… |
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: ci: gate release and hotfix PRs on the InWorld suite
STEP 2 — Root-cause check
PASS. This PR adds CI infrastructure to gate release/hotfix merges on the InWorld test suite. No bug being fixed; the change addresses a gap in release validation coverage.
STEP 3 — Design & integration
PASS. CI-only change — no ECS systems, components, or runtime code. The composite action (resolve-explorer-build/) correctly deduplicates ~55 lines of build-resolution logic shared between two dispatchers. The 3-job structure (resolve → run-suite → result) cleanly separates the stable gate name from the reusable workflow's internal job names — exactly the right pattern for a required-check target. The gate job correctly treats cancelled as failure, and skipped (non-release/hotfix PR) as a pass, so requiring it on main blocks nothing it shouldn't.
The refactoring of visual-regression.yml preserves behavior: output wiring maps correctly (steps.resolve.outputs.build-url → build_url, steps.resolve.outputs.tests-ref → tests_ref), and the wait-minutes default of 0 is correct for slash-command-triggered runs where the build should already exist. The BUILD_PREFIX case statement matches build-unitycloud.yml's exactly.
The comment correction from DEV_EXPLORER_TEAM_* → EXPLORER_TEAM_* in visual-regression.yml aligns the documentation with the actual secret names used by build-unitycloud.yml.
STEP 4 — Member audit
N/A — no C# members introduced.
STEP 5 — Line-level review
Two P1 findings (see inline comments below): self-acknowledged TEMP code that must be stripped before merge.
Four prior findings were already posted on this PR as inline review comments and are not repeated here:
- Output-injection hardening for
BUILD_URL_OVERRIDEnewline trick (action.yml:80) conclusion == "success"not guaranteeing a macOS artifact exists (action.yml:96)secrets: inheritblast radius with mutable@mainref (in-world-tests.yml:154)- Over-broad top-level permissions vs per-job scoping (
in-world-tests.yml:80)
All four are hardening improvements, not live holes. Combined with the two TEMP entries, nothing blocks the merge once the TEMP lines are removed — the prior findings are worth addressing but are not themselves merge blockers.
Security review: No secrets committed, no user-controlled values interpolated into run: blocks (inputs are passed via env:), permissions are minimal, fork PRs are excluded, and BUILD_URL_OVERRIDE is validated against the expected domain. No security blockers found.
STEP 6 — Complexity assessment
SIMPLE — 3 CI workflow files, no runtime code, no ECS/async/rendering changes.
STEP 7 — QA assessment
NO — CI/CD infrastructure only; no user-facing behavior affected.
STEP 8 — Non-blocking warnings
None. No Main.unity or .meta changes.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI-only workflow and action changes — no runtime code, ECS, or player-facing systems
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
InWorld smoke (macOS)❌ InWorld suite failed.
GitHub-hosted |
Security review — CI workflow changes (re-run on
|
This comment has been minimized.
This comment has been minimized.
InWorld suite❌ InWorld suite failed on 1 of 3 legs.
Commit macOS on |
Measured on run 31582719577: Build (macos) finishes at 09:52:15, Build (windows64) at 10:01:18, and the run concludes at 10:01:23. Waiting for the run therefore costs ~9 minutes on every release PR for a Windows build the InWorld suite never touches. Watch the macOS matrix leg instead. One extra API call per poll, and the resolver stops caring about anything else the run does. This also relaxes /visual-tests in a useful direction: a run whose Windows leg failed but whose macOS leg is green is now usable, where before the run's conclusion disqualified it. Newest-first with a fallback over the five most recent runs, because a commit can have a skipped run in front of a real one — that is what happens when a `force-build` label starts a second run after the first declined to build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
explorer-automation#55 added a Windows leg to run-inworld-suite.yml and defaulted `windows` to true, so pinning @main silently opted this repo in. It cannot work from here yet, for three separate reasons: - the Windows workflow's checkout is bare. In a workflow_call run that fetches the caller — this repo — and explorer/ci/*.ps1 is not here. The macOS leg names decentraland/explorer-automation explicitly; the Windows one has not been given the same treatment. - it is passed no build_url, so it resolves the newest dev build instead of the commit on the PR. A release gate that reports on a different commit is worse than no gate. The artifact is also a different file — Decentraland_windows64.zip sits beside the macOS zip under the same prefix, so the resolver here can supply it once an input exists. - it runs on `win-gpu-t4-explorer`. A called workflow's jobs use runners the caller can reach, and this repo registers no self-hosted runners. Pass `windows: false` explicitly rather than inheriting a default that changes underneath us. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
86dca33 to
6f8362c
Compare
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.
Review finding on #9694: absorbing API failures fixed the 90-minute wait and broke the two paths that do not get a second poll. `wait-minutes: 0` decides on one answer. `visual-regression.yml` takes that path on every /visual-tests, and create-release-branch.yml's dispatched gate takes it on every release cut — so one 5xx reported a build that is very likely fine as missing, and reds a required check on a brand-new release PR. `gh` has no retry of its own; three attempts five seconds apart cover it. The same absorption also gave `none` back a second meaning one level up: an empty listing produced TOTAL=0 and no candidate loop, so "no run exists for this SHA" and "every listing call failed" arrived at the same message — the one telling the operator to label a fully-built commit `force-build`. The comment above it asserted that could not happen. Two flags, not one, because the scenario that reaches this is rate-limit exhaustion, and this loop is what spends the budget: it starts partway through a wait, when a listing has already succeeded. `LISTED` alone would still print the force-build message. `LISTED_NOW` says whether the last poll answered, `LISTED` whether any poll ever did, and between them the timeout names which of the three happened. The jobs call keeps one flag: its failure lands on `pending`, whose message asks rather than asserts and never points at `force-build`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1fc33ec to
1f659a0
Compare
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.
STEP 1 — Scope
CI/CD-only PR: 4 files under .github/, no runtime C# code. Changes add InWorld test gating for release/hotfix PRs and refactor build resolution into a shared composite action.
.github/actions/resolve-explorer-build/action.yml— new shared composite action (383 lines).github/workflows/in-world-tests.yml— new InWorld test workflow (243 lines).github/workflows/create-release-branch.yml— +65 lines, dispatch gate on bot-created release PRs.github/workflows/visual-regression.yml— refactored onto shared action (−55 net)
STEP 2 — Root-cause check
✅ PASS — This PR adds a new capability (InWorld test gating for release/hotfix merge gates) and deduplicates build resolution. It is not patching a symptom.
STEP 3 — Design & integration
✅ PASS — No runtime code; analysis covers CI/CD architecture:
- Trigger choice:
pull_request(notworkflow_run) correctly anchors checks to the PR SHA so branch protection can require them. The trade-off — an idleubuntu-latestrunner polling during the build — is documented and appropriate. - Job-level
if:instead of trigger filter: EnsuresInWorld suite resultalways reports on every PR intomain, so a required status check never hangs unreported on non-release PRs. - Shared composite action: Properly deduplicates the build-URL resolution and explorer-automation branch matching that was previously inline in
visual-regression.yml. - Non-fatal dispatch in
create-release-branch.yml: Avoids the force-push-on-rerun hazard (re-running that workflow re-cuts the release from a newerdevtip). The annotation makes the missing gate visible. - Gate job (
result): Handles all edge cases — skipped (non-release PR → pass), both success → pass, cancelled → fail. Theif: always()ensures it always reports.
STEP 4 — Member audit
N/A — no C# types or members changed.
STEP 5 — Line-level review
See inline comment. One P2 finding.
Additional notes (not blocking):
-
secrets: inherit(line 212) passes all repo secrets to the reusable workflow, not just the 6 documented ones. Explicitly listing secrets would be more restrictive but harder to maintain. Risk is bounded by the@mainpin on a same-org repo — only org members can push to that branch. -
Reusable workflow pinned to
@main(line 175) rather than a commit SHA. This is a deliberate, documented trade-off — SHA pinning would be more secure but creates churn every time the reusable workflow is updated. The risk surface is limited to same-org pushers toexplorer-automation/main. -
Build-leg job name coupling —
create-release-branch.yml(line 181) hardcodesstartswith("Build (macos")andstartswith("Build (windows64"), while the composite action parameterizes the same check via$BUILD_LEGS. Ifbuild-unitycloud.ymlrenames its matrix jobs, both locations need updating. Low risk since job names change rarely, but worth noting.
STEP 6 — Complexity
COMPLEX — 4 files, ~720 lines of additions, non-trivial shell logic (polling loop with state machine, retry, multi-leg build resolution), cross-workflow dispatch, and shared composite action.
STEP 7 — QA assessment
QA_REQUIRED: NO — Purely CI/CD workflow changes. No runtime code, no user-facing behavior affected. The author has validated the full flow with real runs (linked in the PR description).
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed files.
Security review
✅ No P0/P1 security issues. Key positives:
- Expression injection prevention: All user-controllable inputs (
build_url,filter,tests_ref,head-ref,head-sha) are passed to shell viaenv:blocks — never interpolated inrun:with${{ }}. This eliminates the most common GitHub Actions injection vector. - URL override validation (action.yml lines 114–134): Newline/CR checked first (blocks
$GITHUB_OUTPUTinjection), then host pinned tohttps://explorer-artifacts.decentraland.org/. The trailing/in the prefix blocks subdomain and userinfo tricks. - Fork exclusion (in-world-tests.yml line 567):
head.repo.full_name == github.repositoryprevents fork PRs from accessing secrets. - Permission scoping: Per-job, minimal —
resultjob haspermissions: {},resolveis read-only, onlyrun-suitehas write access (for the PR comment). - Graceful token degradation (create-release-branch.yml line 458): Dispatch failure is non-fatal with actionable error message, avoiding the re-run hazard.
One P2 finding in inline comment regarding concurrency group susceptibility to fork-based cancellation.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Non-trivial CI/CD infrastructure with polling loops, cross-workflow dispatch, state machine logic, and shared composite action across 4 files.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
This comment has been minimized.
This comment has been minimized.
…y the guard Review findings on #9694. Keying the group on the branch name alone let a fork PR whose head branch is named `release/2026-08-10` join the same group as the real release PR. With cancel-in-progress that fork run kills the legitimate one *before any job's `if:` is evaluated*, so the fork exclusion in `resolve` never gets to matter and the gate reports failure on a cancelled run, exactly as designed. A merge gate an outsider can red at will. The head repo now leads the key: `head.repo.full_name` is the fork on a `pull_request` and absent otherwise, so a same-repo PR and a dispatch still agree — which is what put both triggers in one group in the first place — while a fork gets its own. Everything before the first `/` is the head owner, so no fork name can collide. The release-cut guard had the same one-answer shape 1fc33ec fixed in the resolver, and a worse remedy: both its calls fell back to empty, and "nothing found" skips the dispatch, so one blip meant the gate silently never started on a cut nobody is watching. It now shares the resolver's retry, and no longer reports an unanswered API as "no build". The comment justifying that retry was wrong even though the code was right: it named rate-limit exhaustion as the realistic outage, which resets on an hour boundary and so is not rescued by three attempts ten seconds apart. Corrected to claim only what is true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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: gate release and hotfix PRs on the InWorld suite
STEP 1 — Context & scope
| File | Change |
|---|---|
.github/actions/resolve-explorer-build/action.yml |
New — composite action (404 lines): build-URL resolution, artifact verification, explorer-automation branch matching |
.github/workflows/in-world-tests.yml |
New — InWorld gate workflow (257 lines): trigger, dispatch, gate job |
.github/workflows/create-release-branch.yml |
+125 lines — dispatch InWorld gate on bot-created release PRs |
.github/workflows/visual-regression.yml |
+33/−67 — refactored onto shared composite action |
All changes are CI/CD (.github/). No C# code, no runtime code, no Unity scenes.
Prior review findings (P1 ×3, P2 ×1) from the previous pass have all been addressed — see verification in Step 5.
STEP 2 — Root-cause check
PASS. The PR adds a new capability — automated InWorld test validation as a merge gate for release/hotfix PRs into main. This directly addresses the absence of pre-merge integration testing. Not a symptom fix.
STEP 3 — Design & integration
No runtime code touched — ECS lifecycle analysis N/A. Evaluating CI/CD architecture:
- Composite action extraction — Build resolution (polling, per-leg job inspection, artifact verification, event-to-prefix mapping) and branch matching are correctly deduplicated from
visual-regression.ymlinto.github/actions/resolve-explorer-build/. Both dispatchers consume it with differentbuild-legsandwait-minutesparameters. Sound. - Trigger design —
pull_request(notworkflow_run) is correct:workflow_runattaches checks to the default branch SHA, not the PR's, so it cannot gate a merge. The polling cost (idleubuntu-latest) is documented and free on a public repo. - Gate pattern — The
resultjob usesif: always()with job-levelif:onresolve, so the required check reports on every PR intomain(green passthrough for non-release) while only running the suite for release/hotfix. Cancelled runs correctly fail the gate. - Fork protection — Concurrency group includes
head.repo.full_nameto prevent a fork from cancelling a same-repo gate.resolveexcludes forks viahead.repo.full_name == github.repository. TheisCrossRepository == falsefilter on the PR lookup step prevents matching fork PRs. - Bot-created release PRs —
create-release-branch.ymldispatches the gate viaORG_ACCESS_TOKEN(sinceGITHUB_TOKENevents start no workflows). Failures are non-fatal (exit 0) with::error::annotations because re-running would force-push the release branch. Well-considered. - Per-candidate state tracking —
CAND_STATEcorrectly prioritizesfailed>nomatrix>pending>nonewithin each candidate, and globalSTATEtakes only the newest candidate's verdict. TheLISTED/LISTED_NOWflags correctly distinguish API outages from missing builds.
PASS.
STEP 4 — Member audit
N/A — no C# classes, properties, or accessors.
STEP 5 — Line-level findings
Two passes over all 819 added and 67 removed lines.
Prior P1 findings — verification:
STATE=pendingoverwritesSTATE=failedCAND_STATEwith guarded assignments (if [ "$CAND_STATE" = none ]on pending; unguarded on failed) ensures correct priority. GlobalSTATEtakes only the newest candidate's verdict viaif [ "$STATE" = none ].Concurrency group mismatch— Fixed. Both triggers now key onhead_ref || ref_name(branch name), putting PR and dispatch runs in one group.Dispatch guard checks run success— Fixed. Now checks per-leg job conclusions (select(.conclusion == "success") | .name) and verifies bothBuild (macosandBuild (windows64names exist among successful jobs.Newline injection in— Fixed. Newline check is the firstbuild-url-overridecasearm (line 125), before the host check.
Pass A — No blocking issues found. Shell scripts use set -euo pipefail consistently; all ${{ }} expressions pass through env: (no workflow injection); error handling is thorough with distinct diagnostics for each failure mode.
Pass B — P2 findings. See inline comments below.
| # | Sev | File | Line | Finding |
|---|---|---|---|---|
| 1 | P2 | action.yml |
87 | Empty build-legs silently skips all job checks |
| 2 | P2 | action.yml |
283 | wait-minutes: 0 callers get generic error when specific diagnostics are available |
| 3 | P2 | action.yml |
398 | tests-ref lookup treats transient 5xx same as 404 (no retry) |
Security review
- Workflow injection: All
${{ }}values inrun:blocks pass throughenv:— no direct interpolation of untrusted input. Clean. - Secret exposure: No secrets logged or passed to untrusted contexts. Clean.
- Fork security: Fork PRs excluded from secret-bearing jobs. Concurrency group prevents fork-based cancellation of same-repo gates. Clean.
- Input validation:
build-url-overridevalidated for newlines (line 125) and host prefix (line 129). Clean. - Permissions: Minimal per-job:
resolvehas read-only;run-suitehaspull-requests: writefor the PR comment;resulthaspermissions: {}. Clean. - Supply chain:
@mainpin ondecentraland/explorer-automationreusable workflow — same-org, only merged code runs.actions/checkout@v6consistent with repo conventions.secrets: inheritis the pre-existing pattern; the header documents the six secrets consumed.
No security vulnerabilities found.
STEP 6 — Complexity
COMPLEX — 4 files, 819 lines of additions with polling state machine, cross-repo dispatch, and shared composite action extraction.
STEP 7 — QA
QA_REQUIRED: NO — all changes under .github/. No runtime code modified; no user-facing behavior affected.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Cross-workflow CI gate with polling state machine, shared composite action, and two-trigger concurrency management
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
test multi-line
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: chore: gate release and hotfix PRs on the InWorld suite
STEP 1 — Context & scope
| File | Change |
|---|---|
.github/actions/resolve-explorer-build/action.yml |
New — composite action (404 lines): build-URL resolution, artifact verification, explorer-automation branch matching |
.github/workflows/in-world-tests.yml |
New — InWorld gate workflow (257 lines): trigger, dispatch, gate job |
.github/workflows/create-release-branch.yml |
+125 lines — dispatch InWorld gate on bot-created release PRs |
.github/workflows/visual-regression.yml |
+33/−67 — refactored onto shared composite action |
All changes are CI/CD (.github/). No C# code, no runtime code, no Unity scenes.
Prior review findings (P1 ×3, P2 ×1) from the previous pass have all been addressed — see verification in Step 5.
STEP 2 — Root-cause check
PASS. The PR adds a new capability — automated InWorld test validation as a merge gate for release/hotfix PRs into main. This directly addresses the absence of pre-merge integration testing. Not a symptom fix.
STEP 3 — Design & integration
No runtime code touched — ECS lifecycle analysis N/A. Evaluating CI/CD architecture:
- Composite action extraction — Build resolution (polling, per-leg job inspection, artifact verification, event-to-prefix mapping) and branch matching are correctly deduplicated from
visual-regression.ymlinto.github/actions/resolve-explorer-build/. Both dispatchers consume it with differentbuild-legsandwait-minutesparameters. Sound. - Trigger design —
pull_request(notworkflow_run) is correct:workflow_runattaches checks to the default branch SHA, not the PR's, so it cannot gate a merge. The polling cost (idleubuntu-latest) is documented and free on a public repo. - Gate pattern — The
resultjob usesif: always()with job-levelif:onresolve, so the required check reports on every PR intomain(green passthrough for non-release) while only running the suite for release/hotfix. Cancelled runs correctly fail the gate. - Fork protection — Concurrency group includes
head.repo.full_nameto prevent a fork from cancelling a same-repo gate.resolveexcludes forks viahead.repo.full_name == github.repository. TheisCrossRepository == falsefilter on the PR lookup step prevents matching fork PRs. - Bot-created release PRs —
create-release-branch.ymldispatches the gate viaORG_ACCESS_TOKEN(sinceGITHUB_TOKENevents start no workflows). Failures are non-fatal (exit 0) with::error::annotations because re-running would force-push the release branch. Well-considered. - Per-candidate state tracking —
CAND_STATEcorrectly prioritizesfailed>nomatrix>pending>nonewithin each candidate, and globalSTATEtakes only the newest candidate's verdict. TheLISTED/LISTED_NOWflags correctly distinguish API outages from missing builds.
PASS.
STEP 4 — Member audit
N/A — no C# classes, properties, or accessors.
STEP 5 — Line-level findings
Two passes over all 819 added and 67 removed lines.
Prior P1 findings — verification:
STATE=pendingoverwritesSTATE=failedCAND_STATEwith guarded assignments (if [ "$CAND_STATE" = none ]on pending; unguarded on failed) ensures correct priority. GlobalSTATEtakes only the newest candidate's verdict viaif [ "$STATE" = none ].Concurrency group mismatch— Fixed. Both triggers now key onhead_ref || ref_name(branch name), putting PR and dispatch runs in one group.Dispatch guard checks run success— Fixed. Now checks per-leg job conclusions (select(.conclusion == "success") | .name) and verifies bothBuild (macosandBuild (windows64names exist among successful jobs.Newline injection in— Fixed. Newline check is the firstbuild-url-overridecasearm (line 125), before the host check.
Pass A — No blocking issues found. Shell scripts use set -euo pipefail consistently; all ${{ }} expressions pass through env: (no workflow injection); error handling is thorough with distinct diagnostics for each failure mode.
Pass B — P2 findings. See inline comments below.
| # | Sev | File | Line | Finding |
|---|---|---|---|---|
| 1 | P2 | action.yml |
87 | Empty build-legs silently skips all job checks |
| 2 | P2 | action.yml |
283 | wait-minutes: 0 callers get generic error when specific diagnostics are available |
| 3 | P2 | action.yml |
398 | tests-ref lookup treats transient 5xx same as 404 (no retry) |
Security review
- Workflow injection: All
${{ }}values inrun:blocks pass throughenv:— no direct interpolation of untrusted input. Clean. - Secret exposure: No secrets logged or passed to untrusted contexts. Clean.
- Fork security: Fork PRs excluded from secret-bearing jobs. Concurrency group prevents fork-based cancellation of same-repo gates. Clean.
- Input validation:
build-url-overridevalidated for newlines (line 125) and host prefix (line 129). Clean. - Permissions: Minimal per-job:
resolvehas read-only;run-suitehaspull-requests: writefor the PR comment;resulthaspermissions: {}. Clean. - Supply chain:
@mainpin ondecentraland/explorer-automationreusable workflow — same-org, only merged code runs.actions/checkout@v6consistent with repo conventions.secrets: inheritis the pre-existing pattern; the header documents the six secrets consumed.
No security vulnerabilities found.
STEP 6 — Complexity
COMPLEX — 4 files, 819 lines of additions with polling state machine, cross-repo dispatch, and shared composite action extraction.
STEP 7 — QA
QA_REQUIRED: NO — all changes under .github/. No runtime code modified; no user-facing behavior affected.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Cross-workflow CI gate with polling state machine, shared composite action, and two-trigger concurrency management
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
This comment has been minimized.
This comment has been minimized.
The wait-minutes: 0 exit sat above the `failed` and `nomatrix` fast-exits, so a caller that gets one answer always got the generic line — even when the loop had already decided a leg was red or was never in the matrix. Those are the two callers that cannot recover by waiting: visual-regression.yml on every /visual-tests, and create-release-branch.yml's dispatched gate on every release cut. The specific messages existed precisely so nobody has to guess, and the ordering kept them from the paths that needed them most. Hoisted rather than duplicated inside the branch, so each message has one copy. The outage message still wins where it should: a listing that never answers leaves TOTAL=0, so no candidate is examined and STATE is necessarily `none` — neither hoisted check can match. The waiting path is untouched, since a branch guarded on WAIT_MINUTES -eq 0 cannot fire when it is not. Also dropped "or did it fail?" from the generic line. With `failed` exiting above it, only pending/none reach it, so the clause asked about a case that can no longer arrive and would send an operator to re-check what the loop had already ruled out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @mikhail-dcl's task in 1m 57s —— View job Security review — CI workflow changes (re-run on
|
| file | change | source | type | scope | risk | confidence |
|---|---|---|---|---|---|---|
.github/actions/resolve-explorer-build/action.yml |
+425 (new) | in-repo, composite | shell | CI only | LOW | HIGH |
.github/workflows/in-world-tests.yml |
+257 (new) | in-repo | workflow | CI only | LOW | HIGH |
.github/workflows/create-release-branch.yml |
+125 | in-repo | workflow | CI only | MEDIUM | HIGH |
.github/workflows/visual-regression.yml |
+33/−67 | in-repo | workflow | CI only | LOW | HIGH |
decentraland/explorer-automation/…/run-inworld-suite.yml |
@main (mutable) |
same-org | reusable workflow | CI only + self-hosted | MEDIUM | MEDIUM |
actions/checkout |
@v6 (mutable tag) |
GitHub | action | CI only | LOW | HIGH |
Verdict
Clean pass, and a small one. cb358849 changes no control flow that decides a check's colour — every path it touches already exited 1 — so its whole risk surface is whether the hoist can hide the outage message, and it can't. What's left is one new low note on a message that outruns its evidence in a rare corner, one upstream contract I have no way to query from here, and the hardening the author deferred with a stated reason. No security blockers, and nothing here blocks the merge.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
--- · branch feat/ci-in-world-suite
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: chore: gate release and hotfix PRs on the InWorld suite
STEP 2 — Root-cause check: PASS
Problem: Release and hotfix PRs into main lack an InWorld test gate, so test-suite regressions can ship without blocking the merge.
Fix: The diff adds the gate mechanism directly — a new dispatcher workflow, a shared composite action for build resolution (deduplicated from visual-regression.yml), and a dispatch step in create-release-branch.yml for bot-created PRs. This is a direct solution, not a workaround.
STEP 3 — Design & integration: PASS
This is a CI/CD-only change (GitHub Actions YAML + bash). No ECS systems, lifecycle owners, or runtime code are involved, so the ECS design checks are not applicable.
The design decisions are sound:
- Composite action extraction —
resolve-explorer-buildcleanly encapsulates build-URL resolution and branch matching behind a well-defined input/output interface. Bothvisual-regression.ymlandin-world-tests.ymlconsume it identically, and the visual-regression refactoring is a net -34 lines. pull_requesttrigger vsworkflow_run— Correct. Aworkflow_runreports checks against the default branch's SHA, which never joins the PR's check list and so can never gate a merge. The cost (an idleubuntu-latestjob waiting for the build) is acceptable for a public repo.resultgate job — Theif: always()+ explicit state checks pattern is the correct way to implement a required check that must report even when upstream jobs are skipped or cancelled. The job passes for non-release/hotfix PRs (whenresolveisskipped), which prevents a required check from blocking unrelated PRs.gh_api_retryduplication — Justified. Composite actions cannot export shell functions to their callers. The function is 15 lines, stable, and unlikely to diverge. Extracting it into a standalone script or a second composite action would be over-engineering.exit 0increate-release-branch.yml— Correct. Re-running that workflow force-pushes the release branch fromdev's current tip, silently re-cutting the release. Error annotations make failures visible; the non-fatal exit prevents an operator from accidentally re-cutting by re-running.- Concurrency group with fork protection — Sound. The
head.repo.full_namesegment prevents a fork PR namedrelease/2026-08-10from cancelling the real gate viacancel-in-progress. The fallback togithub.repositoryonworkflow_dispatchkeeps the dispatch andpull_requesttriggers in the same group.
STEP 5 — Line-level review: No blocking issues
Security audit — no issues found:
- All
github.*context values flow throughenv:blocks, never interpolated directly inrun:scripts. No script injection vectors. build-url-overridevalidates newlines first (preventsGITHUB_OUTPUTinjection), then allowlistshttps://explorer-artifacts.decentraland.org/*. The ordering is correct — a multiline payload could bypass the host check on line 1 while injecting arbitrary outputs on line 2.- Fork PRs are explicitly excluded (
head.repo.full_name == github.repository) and get no secrets. - Token permissions are minimally scoped per-job:
resolvehas read-only; onlyrun-suitegetspull-requests: write(for the PR comment);resulthaspermissions: {}. secrets: inheriton the reusable workflow call is appropriate — the call targets@mainof a trusted org repo (decentraland/explorer-automation).BRANCH_NAMEincreate-release-branch.ymlis derived fromrelease/$RELEASE_DATE(a controlled input), not from untrusted user data.
Correctness audit — no issues found:
- The polling loop's state machine correctly handles: both legs green, one leg pending, a leg failed, no matrix expansion, API outages (transient and sustained), rate-limit exhaustion mid-wait, and timeout. Each failure path gives an actionable remedy.
- The
LISTED/LISTED_NOWflag pair distinguishes "no build exists" from "the API didn't answer" — a critical distinction for an operator deciding betweenforce-buildand "wait for the rate limit to reset." - Candidate examination is newest-first with
STATEpromotion guarded byif [ "$STATE" = none ], so an older run's verdict cannot overwrite a newer, more decisive one. - The
verify()function correctly handles curl's behaviour:-woutput prints even on non-zero exit, so|| truecaptures the code and${http:-000}normalizes the empty case. - The
visual-regression.ymlrefactoring correctly maps the old inline outputs to the composite action's outputs (steps.resolve.outputs.build-url,steps.resolve.outputs.tests-ref). - The
scopeexpression${{ (inputs.filter || '') == '' && 'ALL' || '' }}correctly sendsALLon PR triggers (whereinputs.filteris undefined) and defers to the planner on manual runs with a custom filter.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: 4 files, 840+ lines of changes spanning cross-workflow interactions, a build-polling state machine with 8 distinct failure paths, concurrency-group fork protection, and a dual-trigger (pull_request + workflow_dispatch) design.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — chore: gate release and hotfix PRs on the InWorld suite
STEP 2 — Root-cause check: PASS
This PR adds the InWorld NUnit suite as a merge gate on release/** and hotfix/** PRs into main. It is a proactive CI improvement — no bug, no workaround, no symptom-masking. The build resolution logic was previously inlined in visual-regression.yml; this PR lifts it into a shared composite action and adds a second consumer.
STEP 3 — Design & integration: PASS
This is purely CI/CD infrastructure (GitHub Actions YAML + bash). No ECS systems, components, runtime code, or persistent state. The design decisions are sound:
Shared composite action — resolve-explorer-build extracts ~425 lines of build-resolution logic (poll for green build legs, verify artifact URLs, match explorer-automation branch) used by both visual-regression.yml and in-world-tests.yml. Eliminates duplication and gives both dispatchers consistent behavior.
pull_request trigger instead of workflow_run — Correctly chosen: workflow_run reports against the default branch SHA and can never gate a PR merge. The cost (an idle ubuntu-latest runner waiting for the build) is negligible on a public repo.
Gate job pattern — The result job with if: always() is the only name worth requiring as a status check. It passes for non-release/hotfix PRs (so the required check doesn't block normal development), fails on cancellation (so a superseded run doesn't accidentally go green), and is stable across reusable-workflow job name changes.
exit 0 with ::error:: in create-release-branch.yml — Well-reasoned: exit 1 would invite a re-run that force-pushes the release branch from a newer dev tip, silently re-cutting the release. The ::error:: annotation makes the issue visible, and the unreported required check keeps the release blocked until the gate is started manually.
gh_api_retry duplication — Present in both the composite action and create-release-branch.yml. This is a known GitHub Actions limitation (composite actions cannot export shell functions to callers). Both copies are identical and the duplication is acknowledged.
STEP 4 — Member audit: N/A
No classes, properties, or methods — this is YAML + bash.
STEP 5 — Line-level review: No blocking issues
A. Blocking-issue scan — clean.
No bugs, logic errors, security vulnerabilities, or resource leaks identified.
B. Design & pattern review — clean.
The polling loop, retry logic, state tracking (LISTED/LISTED_NOW/STATE), and error messages are well-structured and handle edge cases thoroughly (API outages vs. missing builds vs. failed builds vs. missing matrix legs).
STEP 3 (Security review) — No security issues found
| Area | Status |
|---|---|
| Secrets | All secrets accessed via ${{ secrets.* }} — none hardcoded, none leaked in error messages |
| URL validation | BUILD_URL_OVERRIDE checked for newline/CRLF injection first, then validated against https://explorer-artifacts.decentraland.org/ prefix. Sibling URL derived rather than user-supplied |
| Fork protection | Fork PRs excluded via head.repo.full_name == github.repository check. Concurrency key includes head repo full name to prevent fork-based cancellation of legitimate gate runs |
| Permissions | Least-privilege per job: resolve is read-only, run-suite adds only pull-requests: write (for PR comments), result has empty permissions |
| Shell safety | set -euo pipefail throughout. Variables properly quoted. No uncontrolled expansion of user input into shell commands |
GITHUB_OUTPUT injection |
Newline check on BUILD_URL_OVERRIDE prevents multi-line injection into the output file |
STEP 6 — Complexity
COMPLEX — 4 files, ~840 lines of additions, significant bash logic in the build resolver and polling loop.
STEP 7 — QA assessment
NO — Changes are limited to .github/ CI/CD workflows and actions. No runtime code, no user-facing behavior affected.
STEP 8 — Non-blocking warnings
None. Main.unity not modified.
Notes
- The secret name correction in
visual-regression.ymlcomments (DEV_EXPLORER_TEAM_*→EXPLORER_TEAM_*) fixes stale documentation that was already wrong, confirmed by the validation run. - The
actions: readpermission added tovisual-regression.ymlis a hardening measure — it works today because the repo is public, but the grant should not hinge on that. - The validation evidence is strong: a full green
pull_requestpath (31717537499), a cross-repo call exercising a real release target (31589604419), and a cancellation test confirming failure reporting.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: 4 files with ~840 lines of CI infrastructure (composite action + workflow + release-branch dispatch + visual-regression refactor), significant bash logic in build resolution polling
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
|
PR #9694, run #31806460851 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
|
/visual-tests |
Visual regression tests (macos)✅ All visual tests passed.
Triggered via |
pravusjif
left a comment
pravusjif
left a comment
There was a problem hiding this comment.
LGTM, also visual tests seem to keep working manually triggered as well: #9694 (comment)
mikhail-dcl
added
the
no QA needed
3 participants
Runs explorer-automation's InWorld suite as a merge gate on every
release/**andhotfix/**PR intomain, on macOS and Windows, and manually from the Actions tab against any ref.The suite mechanics live in
decentraland/explorer-automation(run-inworld-suite.yml), the same wayvisual-regression.ymlcallsrun-visual-suite.yml. This PR is the dispatcher — plus the deduplication that a second dispatcher makes worth doing..github/actions/resolve-explorer-build/visual-regression.yml, which held the only copy.github/workflows/in-world-tests.yml.github/workflows/create-release-branch.yml.github/workflows/visual-regression.ymlThe flow
release/**orhotfix/**PR opens againstmain→InWorld suite resultappears as a pending check.Wait for the buildpolls for that commit'sBuild (macos)andBuild (windows64)jobs, and verifies both zips are reachable. It fails early if either leg goes red, and gives up after 90 minutes with a message naming the likely cause.macos-14, Windows split across self-hosted GPU runners — and the result is commented on the PR.InWorld suite resultgoes red if anything above failed, or was cancelled.Drafts are skipped, matching
build-unitycloud.yml, which doesn't build them either.ready_for_reviewpicks the PR up the moment it leaves draft.To actually block the merge
Add
InWorld suite resulttomain's required status checks — it currently requiresTest (editmode)andTest (playmode), which work the same way: they run on the PR, not onmain, and gate the merge into it.That job is the one worth requiring. It stays stable when job names change inside the reusable workflow, it fails rather than going green on a cancelled run, and it passes for PRs into
mainthat aren't release or hotfix — which is why the branch filtering is a job-levelif:rather than a trigger filter. A required check that never reports blocks a merge forever, so the gate reports on every PR intomainwhile only running the suite for the ones that need it.I haven't touched branch protection — that's a repo setting and yours to make.
Design notes
Why
pull_requestand notworkflow_runon the build. Aworkflow_runworkflow reports its check against the default branch's SHA. It never joins the PR's check list, so branch protection can't require it and it can't block a merge. The check has to be on the PR from the moment it opens. The cost is an idleubuntu-latestjob waiting out the build — free on a public repo, and visible as an honest pending check meanwhile.Why not a job inside
build-unitycloud.ymlwithneeds:, which would need no polling at all. It wouldn't be faster:needs: [build]waits for the whole matrix anyway. And it would couple release testing to build discovery — a red InWorld job makes the build run's conclusionfailure, and bothcreate-release-branch.ymland the resolver/visual-testsuses look builds up bystatus=success. One flaky test would make a release PR lose its build links and break/visual-testsfor that commit.Bot-created release PRs.
create-release-branch.ymlopens the PR withGITHUB_TOKEN, whose events start no workflow runs — the same suppression its build-link reuse already works around. So it now dispatches the gate explicitly withORG_ACCESS_TOKEN. Aworkflow_dispatchrun attaches its checks to the dispatched ref's tip, which is the PR head, so the gate lands on the PR. The alternative — a PAT on the checkout andgh pr create— would trigger a full rebuild of a commitdevalready built, which is the 40 minutes that workflow exists to avoid.Validation
31717537499 — the full
pull_requestpath on this PR, both platforms, sharded. Green:Wait for the buildInWorld suite (macOS)Windows / Plan shardsWindows InWorld tests 1/2Windows InWorld tests 2/2InWorld suite resultBuild (macos)finished 16:15:36 andBuild (windows64)16:24:37; the wait released 38s after the later one, so both-leg waiting works. Both Windows shards finished before the macOS leg, so Windows costs nothing in the suite phase — its cost is the ~9 minutes of extra build wait. Whole gate: ~55 minutes from push.31589604419 — the first cross-repo call into
run-inworld-suite.ymlfrom this repo, pinned torelease/2026-08-10's build to exercise a real release target. 65 passed, 1 failed; the failure wasNavbarTests.TestSidebarShowsAllNavigationButtonson a feature flag, fixed since by explorer-automation#69. It also confirmed theEXPLORER_TEAM_*secret family reaches this repo, correcting a stale header this PR fixes.A superseded run confirmed the gate reports failure on cancellation, rather than going green.
The resolver's shell is unit-tested against a stubbed
ghfor: both legs green, one leg still building, a leg failed, newest run skipped with an older green one, never queued, timeout, bad override host, and override sibling derivation.actionlintclean throughout.Not yet exercised: the dispatched path —
create-release-branch.yml's step and the PR lookup it feeds — can't run untilin-world-tests.ymlis ondev, sincegh workflow runresolves workflows from the default branch. The check-attachment mechanism it relies on was verified separately against a real dispatch run.Upstream
Depends on explorer-automation#73 (cross-repo Windows leg) and #72 (sharding from one planner), both merged.