fix(ci): resolve build zip artifact ids via the run-scoped endpoint - #9957
fix(ci): resolve build zip artifact ids via the run-scoped endpoint#9957dalkia wants to merge 4 commits into
Conversation
The CI status comment looked up Decentraland_windows64/Decentraland_macos artifact ids in the repo-wide /actions/artifacts list (newest-first, default page size 30, no pagination), filtering by run id client-side. When the Windows build finishes long before the Mac build on a busy CI day, its zip falls off the first page by the time the run completes, so the comment silently drops the Windows "Download .zip" link (seen on PR #9908: 45 newer artifacts landed between the Windows upload and run completion). Query the run's own artifact list instead, which never exceeds one page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚦 CI StatusBuild skipped — no changes detected under Waiting for lint to start… Waiting for tests to start… Bare-metal benchmarks run automatically after each successful build; results land in this section. Add the On demand — comment |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix(ci): resolve build zip artifact ids via the run-scoped endpoint
Step 1 — Scope
Single file changed: .github/workflows/pr-comment-artifact-url.yml (+6 −4). CI workflow only — no runtime C# or Unity code touched.
Step 2 — Root-cause check: PASS ✅
The PR correctly identifies and fixes the root cause. The comment-success job resolved Windows/Mac artifact IDs via the repo-wide /actions/artifacts endpoint, which returns a newest-first, page-limited (default 30) list. When the slow platform (Mac) finishes long after the fast one (Windows), concurrent CI runs push the earlier artifact off the first page, so the jq filter returns empty and the download link is silently dropped.
Switching to the run-scoped /actions/runs/{run_id}/artifacts?per_page=100 endpoint eliminates the race entirely — only that run's ~10 artifacts are returned, immune to repo-wide churn.
Step 3 — Design & integration: PASS ✅
The check-build-ran job in the same workflow file (line ~93) already uses the identical run-scoped pattern:
gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100"The PR description explicitly notes this mirrors the sibling ucb-build-links action. The change makes comment-success consistent with the existing pattern — no new design introduced.
Step 4 — Member audit
N/A — YAML/shell script change; no public C# members added or modified.
Step 5 — Line-level review
Two passes over the changed lines:
A. Blocking issues: None found.
- The new endpoint URL
/repos/$OWNER/$REPO/actions/runs/${PREVIOUS_JOB_ID}/artifacts?per_page=100is correctly formed.PREVIOUS_JOB_IDis set fromjq -r '.id'on theworkflow_runevent object (line 185), which is the run ID — the correct value for this endpoint. per_page=100is appropriate: a single run produces ~10 artifacts, well within the limit. This matches the siblingcheck-build-ranjob.- Removing
select(.workflow_run.id==${PREVIOUS_JOB_ID})is correct — run-scoping at the API level makes the client-side filter redundant. - The remaining
jqfilters (.expired==false,.name=="Decentraland_windows64"/"Decentraland_macos") are preserved and correct. - The added comment (lines 193-196) accurately explains the motivation.
B. Design/encapsulation smells: None — straightforward shell variable assignments in a workflow step.
Security review
No security concerns:
- No new secrets or tokens introduced
- No change to workflow permissions (already has
actions: read) - No user-controlled input flows into the API path unsanitized —
PREVIOUS_JOB_IDcomes fromgithub.event.workflow_run.id(GitHub-controlled) gh apihandles authentication via the existingGITHUB_TOKEN
Step 8 — Non-blocking warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI workflow YAML change only — no runtime, ECS, or Unity code touched
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔒 Jarvis reviewed this PR — sensitive paths modified ( |
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix(ci): resolve build zip artifact ids via the run-scoped endpoint
Step 2 — Root-cause check: PASS ✅
The PR correctly identifies and fixes the root cause: the repo-wide /actions/artifacts endpoint returns artifacts newest-first with a default page size of 30 and no pagination. When the build run completes (gated on the slowest platform), concurrent CI activity can push older artifacts from that run off the first page. The run-scoped endpoint (/actions/runs/<id>/artifacts) is immune to repo-wide churn — correct fix.
Step 3 — Design & integration: PASS ✅
No new units, systems, or state introduced. This is a targeted endpoint swap in an existing workflow step.
The check-build-ran job in the same file already uses the run-scoped pattern:
gh api "/repos/$OWNER/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100"This PR brings comment-success in line with that established pattern — good consistency improvement.
Step 4 — Member audit: N/A
YAML workflow — no public members or accessors.
Step 5 — Line-level review: No issues found
Correctness:
- The
select(.workflow_run.id==${PREVIOUS_JOB_ID})filter is correctly removed — the run-scoped endpoint already constrains results to the target run, making the client-side filter redundant. - The remaining
select(.expired==false)andselect(.name==...)filters are preserved correctly. per_page=100matchescheck-build-ran's usage and comfortably covers the ~10 artifacts a typical run produces.PREVIOUS_JOB_IDis sourced fromjq -r '.id' <<< "$WORKFLOW_RUN_EVENT_OBJ"(the trustedgithub.event.workflow_runcontext) — no injection risk.
Comment quality:
- The 4-line inline comment clearly explains why the endpoint was changed (not just what), linking the timing race to the pagination behavior. Valuable for future maintainers.
Security review: No issues found
- No secrets or credentials exposed.
PREVIOUS_JOB_ID,OWNER, andREPOall originate from trusted GitHub context (github.event.workflow_run,github.repository_owner,github.event.repository.name) — no user-controlled injection vectors.- No new permissions required —
actions: readalready covers the run-scoped endpoint. - No changes to token handling or authentication flows.
Step 6 — Complexity: SIMPLE
One YAML file, +6/−4 lines, CI-only endpoint swap with no runtime impact.
Step 7 — QA: Not required
Changes are limited to a CI workflow under .github/. No runtime code is affected.
Step 8 — Warnings: None
Main scene not modified. No non-blocking warnings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single CI workflow file change — switches artifact API endpoint from repo-scoped to run-scoped
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
Review follow-ups: the Windows and Mac lookups issued the same request twice — fetch once and extract both ids from the same snapshot. The run-scoped artifact list also spans re-run attempts, where a same-name artifact can appear once per attempt (verified on run 33657144083: two non-expired unity_build_info_macos_launcher entries); a two-line jq result would corrupt the GITHUB_ENV entry, so keep only the newest. 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.
|
Claude finished @dalkia's task in 58s —— View job Re-review after
|
| case | behaviour |
|---|---|
| one match | single id, one line ✅ |
| several matches (re-run attempts) | max_by(.created_at) picks newest; ISO-8601 UTC sorts correctly as a string ✅ |
| no match / all expired | [] → null → .id → null → // empty ⇒ empty var, compose_row drops the link by design, jq exits 0 under -eo pipefail ✅ |
created_at: null |
jq orders null below strings, so a real timestamp always wins ✅ |
| precedence | | binds looser than //, parsing as max_by(...) | (.id // empty) — the intended form ✅ |
.id // empty remains the right idiom over .id?; it's the only variant that avoids writing a literal WINDOWS_ARTIFACT_ID=null into $GITHUB_ENV and producing a broken download URL.
Security re-check on the current diff
Steps 1–3 (dependencies): N/A for this PR. One file attributable to this branch, .github/workflows/pr-comment-artifact-url.yml (+15/−12); no Packages/manifest.json, packages-lock.json, Plugins/, Assets/**, .asmdef, or build-hook changes in the three-dot diff, and no binaries. The new-dependency label fires on a .github/** path filter, not an actual dependency change.
| dependency / file | version | source | type | scope | risk | evidence confidence |
|---|---|---|---|---|---|---|
.github/workflows/pr-comment-artifact-url.yml |
n/a | in-repo | workflow YAML | CI only (not shipped) | LOW | HIGH |
Step W (W.1–W.10): unchanged. W.1/W.4/W.5/W.6 N/A (no LLM in this workflow); W.3 N/A (workflow_run checkouts carry no ref:, so they resolve to the default branch with persist-credentials: false); W.8 not used; W.10 permissions are explicit and minimal (contents: read, pull-requests: write, actions: read at :23–:28, re-verified). The only values reaching the API path are $OWNER, $REPO and $PREVIOUS_JOB_ID (a numeric run id from the trusted github.event.workflow_run payload); the artifact name travels via --arg, not the filter body. No new injection surface.
Pre-existing, untouched by this PR (re-confirmed present at the same lines after this merge; recorded, not attributed here):
- W.7a — secret-holding action on a mutable ref (HIGH class).
peter-evans/repository-dispatch@v4at:359consumessecrets.PERFORMANCE_TESTING_PAT(:363); should be a 40-char SHA.actions/checkout@v6(:55,:121,:147,:170,:456) is W.7b. Fix this → - W.2 — static heredoc delimiter (MEDIUM class, not currently exploitable).
echo "SIZE_REPORT<<EOF"at:274wraps artifact-derived content; thegrep "^|"filter makes a bareEOFline unreachable today, but the random-delimiter pattern already used forPLATFORM_ROWS(:326–:331) would be more robust. Fix this →
Verdict
Nothing to re-review substantively — 8c0afbd only brings the branch up to date with dev again, leaving the fix byte-identical to the version I confirmed correct after 552a9ce. No new HIGH or MEDIUM finding. Still ready to merge from my side.
The "not testable before merge" caveat holds: workflow_run workflows always execute the default-branch definition, so this takes effect only once it lands on dev. The one HIGH-class item in the file (unpinned secret-holding repository-dispatch@v4) is pre-existing and untouched, so it isn't a blocker for this CI fix — it should be tracked separately.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
· Branch: fix/ci-comment-artifact-lookup-pagination
Pull Request Description
What does this PR change?
Fixes the CI status comment sometimes missing the Download .zip link for Windows (observed on #9908).
The
pr-comment-artifact-url.ymlworkflow resolved theDecentraland_windows64/Decentraland_macosartifact ids from the repo-wide/actions/artifactslist — which is newest-first, capped at the default page size of 30, and not paginated — filtering by run id client-side injq. The comment fires when the whole build run completes, i.e. gated on the slowest platform. On #9908 the Windows build finished 26m in while Mac took 1h 22m; by run completion, 45 newer artifacts from concurrent runs had pushed the Windows zip off the first page, soWINDOWS_ARTIFACT_IDresolved empty andcompose_row(by design) dropped the link, leaving only the S3 one.This PR switches both lookups to the run-scoped endpoint (
/actions/runs/<run-id>/artifacts?per_page=100), which only contains that run's ~10 artifacts and is immune to repo-wide churn. This mirrors what the siblingucb-build-linksaction already does.Test Instructions
Not testable via a client build — this changes a
workflow_run-triggered workflow, which always executes the definition from the default branch, so the fix only takes effect after merge todev.Steps (standard run):
N/A — no client change;
metaforge explorer rundoes not apply.Expected result:
N/A
Steps (fresh account):
N/A
Expected result:
N/A
Automation (if applicable):
N/A
Prerequisites
Test Steps
Download .zip(GitHub artifact) and.zip via S3links, and that theDownload .ziplink points to this run'sDecentraland_windows64artifact.Additional Testing Notes
Quality Checklist
🤖 Generated with Claude Code