chore: unify build/lint/tests PR comments into one

[dalkia]

Pull Request Description

What does this PR change?

The three github-actions[bot] status comments on every PR — Build, Lint, and Tests — are collapsed into a single "🚦 CI Status" comment with three marker-delimited sections. Each workflow now edits only its own section when its respective action runs, so the combined status stops sprawling across three long comments.

Before: three independent comments posted by four workflows (pr-comment-delete-artifact-url + pr-comment-artifact-url → Build; pr-comment-warnings → Lint; pr-comment-test-failures → Tests), each replacing a whole comment.

After: one comment keyed by a hidden <!-- ci-status --> marker, structured as:

<!-- ci-status -->
### 🚦 CI Status
<!-- ci:build:start -->  …build…  <!-- ci:build:end -->
<!-- ci:lint:start -->   …lint…   <!-- ci:lint:end -->
<!-- ci:tests:start -->  …tests…  <!-- ci:tests:end -->

How

• New composite action .github/actions/ci-status-comment + upsert-ci-status.sh:
  • Seeds a three-section skeleton (each defaulting to "Waiting…") the first time any workflow writes.
  • Replaces only the requested section (build | lint | tests), leaving the others untouched.
  • Build and Unity Test are independent workflows whose comment writers can fire simultaneously, so the script collapses duplicate comments from a create race (keeps the oldest), then re-reads and retries if a concurrent writer clobbered its section.
• Rewired the comment workflows to call the action instead of peter-evans/create-or-update-comment. Reference-style [badge] links became inline badges, since three sections can no longer share one Markdown reference label.
• Consolidated 4 workflows → 3. pr-comment-delete-artifact-url.yml never deleted anything — it only reset the build comment to "pending" on the Unity Cloud Build requested event. It's now folded into pr-comment-artifact-url.yml as a requested job alongside the completed result jobs, mirroring how pr-comment-warnings.yml handles lint pending + result in one file.
• Each posting job gains a sparse actions/checkout (only the action folder) so the local composite action is available.
• Drive-by fix (pre-existing bug): added actions: read to pr-comment-warnings.yml. download-artifact@v4 needs it to fetch the warning-result artifact from the Unity Test run; without it the download failed with Resource not accessible by integration and the lint section silently fell back to "skipped". pr-comment-test-failures.yml already had this permission. Caught by the end-to-end validation below.

Validation

Because workflow_run workflows always run from the default branch, this PR's own comments still use the old workflows — so I validated the new plumbing end-to-end in a throwaway repo instead of relying on this PR's checks.

Throwaway lab: dalkia/ci-status-comment-lab — public repo, so you can browse the runs directly. Quick links: validation PR #1 · round-2 before (PR #2) · round-2 after (PR #3) · Actions runs.

It stubs Unity Cloud Build and Unity Test with trivial workflows that emit the exact artifacts the comment workflows parse (warning-result.json, failed-tests-{editmode,playmode}.json, a dummy Decentraland_* build artifact), then copies this branch's composite action + three comment workflows (build one trimmed of the decentraland-only perf-dispatch/S3/release steps).

Opening a PR there fired both producers' requested events at once, then both completed. Verified on the resulting PR:

• ✅ One 🚦 CI Status comment — not three.
• ✅ Independent sections: Build flipped pending → Success without touching Lint/Tests; Lint and Tests each rendered from their own artifacts (Lint "Passed — warnings 10 → 8"; Tests "Failed — EditMode 128/0, PlayMode 54/2" with a failed-test <details> list).
• ✅ Create-race dedup: two requested writers seeded a single comment, never a duplicate.
• ✅ Re-running after the actions: read fix turned Lint from "skipped" to the correct "Passed".

Also validated locally before pushing: the upsert script was exercised against a mock gh CLI (single comment accrual, independent sections, duplicate-collapse), and actionlint + shellcheck are clean on all changed workflows and the new script.

Round 2 — review fixes validated (72905d1b4)

The final review round flagged two low-severity leftovers, fixed in 72905d1b4:

1. upsert-ci-status.sh — the post-write recheck read still filtered the raw paginated comment list; it now goes through flatten_pages like the other two reads, so LIVE_BODY resolves under both flat and per-page gh api --paginate --slurp shapes.
2. pr-comment-warnings.yml — \(.line) was the only interpolated field from the fork-controlled warning-result.json not scrubbed of newlines; it now gets tostring | gsub("[\r\n]"; " ") like .file/.rule/.message.

Both were re-validated in the same lab, with the Unity Test stub extended to emit a hostile .line ("7\n<!-- ci:lint:end -->\nforged") alongside a normal numeric one:

• ❌ Before (lab PR #2 ran the pre-fix workflows from the lab's default branch): the embedded newline survived, the forged ci:lint:end marker landed on its own line, and the lint section scrambled — the injected block was duplicated 5× with stray section fences. Kept as a live repro of the failure mode.
• ✅ After (lab PR #3, fixed workflows on the lab's default branch): one clean comment, all three sections intact; the numeric .line renders normally (Foo.cs:42) and the hostile one collapses to a single inert line inside the code fence (Evil.cs:7 <!-- ci:lint:end --> forged …). The upsert log shows CI status 'lint' section updated (attempt 1) for every section writer — no retries, no jq errors on the flattened recheck read.

After merge, sanity-check on the next real PR:

1. Push a PR touching Explorer/ and a .cs file (triggers build + lint + tests).
2. Confirm one "🚦 CI Status" comment with three sections.
3. Confirm each section flips independently as its workflow finishes.

Quality Checklist

• Changes have been tested locally (mock-gh simulation + actionlint/shellcheck)
• Changes validated end-to-end in a live Actions environment (dalkia/ci-status-comment-lab)
• Documentation has been updated (workflow header comments + composite action docs)
• Performance impact has been considered (adds a sparse checkout per job; negligible)
• For SDK features: Test scene is included (N/A)

Additional Testing Notes

• Existing PRs opened before this merges keep their legacy comments (harmless); new PRs get the unified one.

Code Review Reference

Please review our Branch & PR Standards before submitting.

[github-actions]

Build skipped — no changes detected under Explorer/.

[decentraland-bot]

Review Summary

PR: #9617 — chore: unify build/lint/tests PR comments into one
Branch: chore/unify-ci-status-comment → dev

Step 2 — Root-cause check: PASS

The PR addresses comment sprawl from three independent github-actions[bot] comments per PR. The solution correctly fixes the root cause by consolidating into a single marker-keyed comment with section-delimited updates — not a symptom-level workaround.

Step 3 — Design & integration: N/A

This PR modifies only CI workflow files and introduces a new composite action (.github/actions/ci-status-comment/). No runtime code, no ECS systems, no long-lived units. The marker-based section approach is a sound pattern for concurrent partial updates within GitHub API constraints. The retry-and-verify loop correctly handles the read-modify-write race inherent in concurrent workflow runs (duplicate-collapse keeps the oldest, section verify + retry on clobber).

Steps 4–5 — Line-level findings

1 finding (P2) — see inline comment on upsert-ci-status.sh.

Security Review

Category	Status
Secrets & credentials	✅ GH_TOKEN sourced from github.token via action input — no hardcoded secrets
Input validation & injection	✅ JSON payloads constructed via jq --arg (proper escaping). Shell variables consistently quoted. One minor gap: SECTION not validated (see P2 finding)
Auth & authz	✅ workflow_run triggers run in base-repo context with pull-requests:write
Sensitive data exposure	✅ No secrets, PII, or internal details in logs or error messages
Dependencies	✅ No new packages added. Uses actions/checkout@v4

No security issues found.

Step 6 — Complexity: SIMPLE

CI-only changes (GitHub Actions YAML + bash script). No runtime code, no ECS, no Unity player impact.

Step 7 — QA: NO

All changes are in .github/ — no runtime code affected, no user-facing behavior changes.

Step 8 — Warnings

None. Main.unity not modified.

Step 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI workflow YAML and bash script changes only — no runtime or Unity code touched.
QA_REQUIRED: NO

---

Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

[github-actions]

🔒 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).

[decentraland-bot]

STEP 1 — Context

CI/CD-only PR. No runtime code, no ECS systems, no Unity player changes. Reviewed:

• .github/actions/ci-status-comment/action.yml (new composite action)
• .github/actions/ci-status-comment/upsert-ci-status.sh (new, 150 lines)
• .github/workflows/pr-comment-artifact-url.yml (modified — absorbed pending job from deleted workflow)
• .github/workflows/pr-comment-delete-artifact-url.yml (deleted)
• .github/workflows/pr-comment-test-failures.yml (modified)
• .github/workflows/pr-comment-warnings.yml (modified)

STEP 2 — Root-cause check

The PR solves "3 separate bot comments clutter every PR" by consolidating them into one marker-fenced comment with independent sections. This addresses the root cause, not a symptom. The existing peter-evans/find-comment + create-or-update-comment pattern had no coordination between workflows, so each wrote its own comment.

PASS — the approach is sound.

STEP 3 — Design & integration

N/A — purely CI/CD. No new systems, managers, services, or lifecycle units in runtime code. The composite action pattern is appropriate for shared bash logic across 3 workflow files.

The follow-up commits addressed the major design issues found in the prior review round:

• ✅ Marker injection sanitization via choke-point grep in upsert-ci-status.sh
• ✅ Decoupled success from duplicate cleanup (non-convergence fix)
• ✅ Checkout pinned to @v6 with persist-credentials: false
• ✅ Fork-controlled fields wrapped in toJSON() for the perf-test dispatch payload
• ✅ gsub extended to .file and .rule in lint output
• ✅ Workspace isolation via path: ci-status-action for warnings/test-failures checkouts

STEP 4 — Member audit

N/A — no runtime members.

STEP 5 — Line-level findings

Two P2 items remain from the prior review round (already flagged with suggestions by claude[bot]), plus one new minor finding:

Open from prior review

1. [P2] .line unscrubbed in jq output (pr-comment-warnings.yml lines 178, 197) — .file and .rule now have gsub("[\r\n]"; " ") but .line does not. Since warning-result.json is fork-controlled, a non-numeric .line could inject newlines into the comment body. The marker-strip in upsert-ci-status.sh prevents structural corruption, so this is cosmetic-forgery-only, but it's the same one-token fix as its neighbours. Already flagged with a suggestion.

2. [P2] --slurp / .[][] shape assumption (upsert-ci-status.sh lines 86–89) — gh api --paginate --slurp produces an array-of-page-arrays, so .[][] is correct, but a flatten_pages() helper would make it shape-agnostic and remove the assumption. Lab validation predates the --slurp commit and single-page PRs don't exercise the multi-page path. Already flagged with a concrete suggestion.

New

3. [P2] Stale comment (upsert-ci-status.sh line 4) — says "All four CI comment workflows" but there are now three (delete-artifact-url was folded into artifact-url).

Security review

• No secrets or tokens hardcoded or exposed in comment bodies ✅
• Marker injection sanitized at the choke point ✅
• Fork-controlled fields in client-payload properly escaped with toJSON() ✅
• persist-credentials: false on all checkouts ✅
• actions: read permission addition is correct and minimal ✅
• Comment bodies constructed via jq -n --arg (proper JSON escaping) ✅
• No command injection vectors — variables passed via env, not inline shell interpolation ✅

No security issues found.

STEP 6 — Complexity

SIMPLE — CI/CD workflows only, no runtime code.

STEP 7 — QA assessment

QA_REQUIRED: NO — no user-facing behavior or runtime code affected.

STEP 8 — Non-blocking warnings

None.

STEP 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI/CD workflow consolidation only (.github/), no runtime subsystems touched.
QA_REQUIRED: NO

---

Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack

[dalkia]

@claude re-review

[github-actions]

🤖 @claude review has been retired — AI reviews now run through Jarvis.
Request a review from decentraland-bot in the Reviewers panel instead
(use the 🔄 re-request arrow next to it for a re-review after pushing changes).

[claude]

Claude finished @dalkia's task in 2m 41s —— View job

---

Security-focused re-review — 6e857e7a4

• Determine what changed since the last pass (72905d1b4)
• Dependency review (STEP 1–3)
• Workflow review (STEP W)
• Re-check status of prior findings
• Post findings

---

This push is a pure merge of dev — provably content-neutral in both directions

6e857e7a4 is a merge commit (72905d1b4 + 00c5d07df) with no code of its own. I verified that as a property of the trees, not by trusting the label:

Check	Result
git diff 00c5d07df HEAD (vs dev)	only this PR's 6 files → every dev-side file in HEAD is byte-identical to dev; no conflict resolution altered dev content
git diff 72905d1b4 HEAD -- <the 6 PR files>	empty → the merge didn't touch this PR's own work
two-dot vs three-dot diff	now identical — 6 files, +316/−169, matching the PR header exactly

So the reviewable surface is unchanged and there is nothing new to review in the diff. What the merge does change is my confidence in last pass's open item: the branch is no longer behind dev (git merge-base --is-ancestor origin/dev HEAD → yes), which was the one thing I recommended before landing.

The #9616 interaction is now verified rather than inferred

Last pass I reasoned about #9616's test.yml changes from per-file history because there was no merge base. With dev merged I could diff it directly, and it holds:

• git diff 162f73f2b 00c5d07df -- .github/workflows/test.yml is 31 insertions, 0 deletions — purely additive.
• name: Unity Test (test.yml:1), the warning-result artifact (:517–518), and failed-tests-${{ matrix.testMode }} (:800–801) are all untouched. This PR's workflow_run.workflows: ["Unity Test"] trigger and both download-artifact-by-name lookups still resolve. No conflict.
• The second-order effect is still handled: the new scripts/generate-ignore-warnings.sh path filter (test.yml:8–9) lets Unity Test fire on a PR with no .cs, but pr-comment-warnings.yml:139–147 composes Skipped/Unavailable from FOUND != true rather than leaving the section stuck on Pending. Confirmed in the merged tree.

One small bonus the merge brings: dev's new .gitattributes adds *.sh text eol=lf, which now covers upsert-ci-status.sh itself — the script is guaranteed to check out LF on any runner. That's orthogonal to finding #4 (which was about a human editing the comment body in the web UI, not the script's own line endings), but it removes one adjacent way exact-line matching could have been surprised.

Findings

No blockers. Nothing new. All prior fixes re-verified in the merged tree, not just in the diff:

• grep -n '\.\[\]\[\]' over upsert-ci-status.sh → nothing. All three jq filters (marker_ids :88, CURRENT_BODY :106, LIVE_BODY :134) consume flatten_pages.
• .line scrubbed with | tostring | gsub("[\r\n]"; " ") at pr-comment-warnings.yml:179 and :198, matching its three neighbours.
• 7 checkouts, all actions/checkout@v6, all 7 with persist-credentials: false.
• Header comment (:2–5) parses and says "three".
• Marker-strip choke point intact at :57–58; success decoupled from duplicate cleanup at :141–148.

Status of prior findings

#	Finding	Status
1	section never validated	⚪ dismissed by author — accepted
2	Marker injection from fork-controlled fields	🟢 closed
3	Undeletable duplicate → non-convergent loop	🟢 closed
4	CRLF from a web-UI edit	⚪ dismissed by author — accepted
5	@v4 pin · persist-credentials	🟢 closed
6	--slurp shape unvalidated	🟢 closed
7	Header comment mangled by 8f65fa966	🟢 closed
—	Branch behind dev	🟢 closed by this push

Carried MEDIUMs, unchanged and none new: actions: read set top-level rather than job-level (pr-comment-warnings.yml:28); uncapped failed-test list against the 65 536-char body budget now shared by three sections; section_body.md written to the job CWD, which is a git checkout in 5 of the 7 jobs, so it dirties the tree ("$RUNNER_TEMP/section_body.md" avoids it); broad || true at :58 swallowing a real grep exit 2 or a failed write. Trivial nit while you're in there: trailing whitespace after contents: read at pr-comment-artifact-url.yml:23.

Still true and still a deliberate scope call: the legacy-comment sweep is not on this branch, so the three legacy comments on already-open PRs stay until those PRs close. Whether pull-requests: write permits the duplicate DELETE remains UNKNOWN — not a blocker, since a 403 is now a lingering duplicate plus a log line rather than a wedge.

STEP W — workflow review

No HIGH findings. No workflow edits and no permission delta in this push (git diff 72905d1b4 HEAD over the PR's workflow files is empty), so W.1–W.10 carry: no pull_request_target, no LLM invoked, nothing checks out PR-head code (head_sha appears only as data at pr-comment-artifact-url.yml:164), fork-controlled dispatch fields wrapped in toJSON, explicit permissions: blocks in all three workflows (contents: read + pull-requests: write, plus actions: read at pr-comment-warnings.yml:28 and pr-comment-test-failures.yml:22). W.2 clean — both $GITHUB_OUTPUT heredocs use DELIM="EOF_$(uuidgen)". action.yml passes everything through env:; PR_NUMBER is API-derived.

Worth naming once because it's now visible in this branch's tree: dev's rsp-drift job checks out pull_request.head.sha and runs bash scripts/generate-ignore-warnings.sh — fork-controlled code execution, but under plain pull_request with permissions: contents: read and no secrets, which is the acceptable form. It arrived via #9616 and reaches dev independently of this PR; not a finding against this PR.

STEP 1–3 — dependency review: no subjects

All six changed files are under .github/, and this push changed none of them. No Packages/ manifest or lockfile entries, no binaries, no asmdef or build-script changes in the PR's own surface, so STEP 1 / 1.5 / 2 / 3 have no subjects and no registry lookups were needed. The pre-computed new-binary list is empty. The new-dependency label remains a false positive.

Verification limits

jq, gh, shellcheck and actionlint were unavailable in this sandbox, so I read rather than executed. Diff scope, merge neutrality, ancestry, the additive-only test.yml delta, checkout pins and permissions are HIGH confidence (direct git diff / file reads). Whether pull-requests: write permits the duplicate DELETE remains UNKNOWN — worth checking post-merge.

Verdict

No blocker; the open list stays empty, and the one item I asked for before landing is now done. Every finding across seven passes is closed or explicitly dismissed by the author, and the #9616 interaction I could only infer last time now checks out against a real merge base. The remaining MEDIUMs are the same polish items — none affect correctness or the security posture.

Post-merge sanity check, unchanged: exactly one 🚦 CI Status comment on the first real PR, each section flipping independently, and the duplicate DELETE actually succeeding. Legacy comments on PRs opened before the merge will not be cleaned up.

DEPENDENCY_REVIEW: NEEDS_ATTENTION

--- · branch chore/unify-ci-status-comment