Skip to content

fix(ci): resolve build zip artifact ids via the run-scoped endpoint - #9957

Open
dalkia wants to merge 4 commits into
devfrom
fix/ci-comment-artifact-lookup-pagination
Open

fix(ci): resolve build zip artifact ids via the run-scoped endpoint#9957
dalkia wants to merge 4 commits into
devfrom
fix/ci-comment-artifact-lookup-pagination

Conversation

@dalkia

@dalkia dalkia commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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.yml workflow resolved the Decentraland_windows64 / Decentraland_macos artifact ids from the repo-wide /actions/artifacts list — which is newest-first, capped at the default page size of 30, and not paginated — filtering by run id client-side in jq. 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, so WINDOWS_ARTIFACT_ID resolved empty and compose_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 sibling ucb-build-links action 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 to dev.

Steps (standard run):
N/A — no client change; metaforge explorer run does not apply.

Expected result:
N/A

Steps (fresh account):
N/A

Expected result:
N/A

Automation (if applicable):
N/A

Prerequisites

  • None — verification happens on PRs opened after this merges

Test Steps

  1. After merge, pick any PR whose Windows Unity Cloud build finishes well before the Mac one (the common case) on a day with concurrent CI activity.
  2. Wait for the build run to complete and the 🚦 CI Status comment's build section to update.
  3. Verify the Windows row contains both Download .zip (GitHub artifact) and .zip via S3 links, and that the Download .zip link points to this run's Decentraland_windows64 artifact.
  4. Verify the Mac row is unchanged (both links present).

Additional Testing Notes

  • The equivalent dry-run check was done against PR fix: bugsweep week 2026-W34 #9908's build run 33652525633: the run-scoped query returns both zip artifact ids correctly, while the old repo-wide query missed the Windows one.
  • The Mac lookup had the same flaw but rarely lost the race (its artifact is always fresh when the run ends); it is fixed the same way.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

🤖 Generated with Claude Code

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>
@dalkia
dalkia requested a review from a team as a code owner September 3, 2026 11:15
@github-actions
github-actions Bot requested a review from DafGreco September 3, 2026 11:15
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

Build skipped — no changes detected under Explorer/.

Lint

Waiting for lint to start…

Tests

Waiting for tests to start…

Performance

Bare-metal benchmarks run automatically after each successful build; results land in this section. Add the perf_test label to run the in-repo Unity performance suite instead (skips normal CI and blocks merge while set).

Automation

On demand — comment /visual-tests on this PR to run the visual regression suite against its build.

@claude

This comment has been minimized.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=100 is correctly formed. PREVIOUS_JOB_ID is set from jq -r '.id' on the workflow_run event object (line 185), which is the run ID — the correct value for this endpoint.
  • per_page=100 is appropriate: a single run produces ~10 artifacts, well within the limit. This matches the sibling check-build-ran job.
  • Removing select(.workflow_run.id==${PREVIOUS_JOB_ID}) is correct — run-scoping at the API level makes the client-side filter redundant.
  • The remaining jq filters (.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_ID comes from github.event.workflow_run.id (GitHub-controlled)
  • gh api handles authentication via the existing GITHUB_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

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🔒 Jarvis reviewed this PR — sensitive paths modified (.github/prompts, .github/workflows, or CODEOWNERS). Jarvis will not auto-approve these PRs — human DEV review is required. No QA needed (Jarvis reported QA_REQUIRED: NO).

@github-actions github-actions Bot added the no QA needed Used to tag pull requests that does not require QA validation label Sep 3, 2026

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and select(.name==...) filters are preserved correctly.
  • per_page=100 matches check-build-ran's usage and comfortably covers the ~10 artifacts a typical run produces.
  • PREVIOUS_JOB_ID is sourced from jq -r '.id' <<< "$WORKFLOW_RUN_EVENT_OBJ" (the trusted github.event.workflow_run context) — 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, and REPO all 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: read already 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>
@claude

This comment has been minimized.

@dalkia dalkia self-assigned this Sep 3, 2026
@claude

This comment has been minimized.

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 58s —— View job


Re-review after 8c0afbd — second catch-up merge of dev, no code change

  • Diff the branch against origin/dev
  • Check whether the merge altered the effective diff
  • Re-run the workflow/dependency security checklist
  • Post verdict

What changed since the last review

8c0afbd Merge branch 'dev' into fix/ci-comment-artifact-lookup-pagination is another pure catch-up merge. The PR's own contribution is untouched:

$ git diff origin/dev...HEAD --stat
 .github/workflows/pr-comment-artifact-url.yml | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

$ git rev-parse HEAD:.github/workflows/pr-comment-artifact-url.yml
e5097096b4322e9fb8aedec5fee41385b5cf1b3c   # identical to 4cbe407, and to 552a9ce before it

The file blob is byte-for-byte the same one I confirmed correct after 552a9ce and again after 4cbe407. The merge resolved cleanly — dev still hasn't touched pr-comment-artifact-url.yml since 8f5cca8, which is already in the base.

What 8c0afbd pulls in from dev (git diff 4cbe407 8c0afbd) is #9940 (ANR pulse host resolution) — 5 files, +75/−13:

Systems/LoadSceneDefinitionListSystem.cs
Connections/Pulse/ENet/ENetTransport.cs
Connections/Pulse/PulseHostResolutionException.cs (+ .meta)
Connections/Pulse/PulseMultiplayerService.cs

That's dev's content flowing into the branch, not this PR's work — it doesn't appear in the three-dot diff and was reviewed on its own PR. Out of scope here.


Verification of the fix (carried forward, re-confirmed against the current tree)

.github/workflows/pr-comment-artifact-url.yml:198:207:

RUN_ARTIFACTS=$(gh api "/repos/$OWNER/$REPO/actions/runs/${PREVIOUS_JOB_ID}/artifacts?per_page=100")
newest_artifact_id() {
  jq -r --arg name "$1" \
    '[.artifacts[] | select(.expired==false and .name==$name)] | max_by(.created_at) | .id // empty' \
    <<< "$RUN_ARTIFACTS"
}
WINDOWS_ARTIFACT_ID=$(newest_artifact_id "Decentraland_windows64")
MAC_ARTIFACT_ID=$(newest_artifact_id "Decentraland_macos")

All paths still behave as reviewed:

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):

  1. W.7a — secret-holding action on a mutable ref (HIGH class). peter-evans/repository-dispatch@v4 at :359 consumes secrets.PERFORMANCE_TESTING_PAT (:363); should be a 40-char SHA. actions/checkout@v6 (:55, :121, :147, :170, :456) is W.7b. Fix this →
  2. W.2 — static heredoc delimiter (MEDIUM class, not currently exploitable). echo "SIZE_REPORT<<EOF" at :274 wraps artifact-derived content; the grep "^|" filter makes a bare EOF line unreachable today, but the random-delimiter pattern already used for PLATFORM_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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-dependency no QA needed Used to tag pull requests that does not require QA validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants