chore: require admin approval to merge the main→dev sync PR

[dalkia]

Pull Request Description

What does this PR change?

The Create PR for syncing main to dev workflow (.github/workflows/auto-sync-main-to-dev.yml) only opened the chore/sync PR and left the merge method up to whoever clicked the button. That PR has been getting squash-merged, which is what caused the release conflicts in #9490.

Why squash breaks the sync: squashing rewrites the synced commits into new SHAs on dev, so Git no longer recognizes that dev already contains hotfixes applied directly to main. The originals stop being ancestors of dev, and the next release → main merge re-conflicts on those exact same changes.

Concrete evidence from #9490 (release/2026-07-27 → main):

• The security hardening fix: harden decentraland:// deep-link & external-URL handling (SEC-003/004/005/008/019/028) #9466 (SEC-003/004/005/008/019/028) was on main but missing from dev/release, so all 6 conflicts were its files (including an add/add on DeepLinkAllowlist.cs).
• fix: gltf container dereference destroyed root #9460 appeared with two different SHAs — 537f9fcf0e on main vs 481ff99a23 on release — the fingerprint of a squashed sync rewriting shared commits.

The strategy: keep it simple — admin approval + explicit instructions

The manual admin merge-commit procedure is not a new invention: it kept main and dev in sync for years while admins were aware of it (#9433, #9458, #9504, #9523 were all merged correctly by hand). What failed in #9479 was awareness in the moment, not the mechanism. So this PR restores and reinforces exactly that, without adding new machinery to the repo:

1. enforce-group-approvals.yml — the enforce-approvals check (already required on dev) now fails on PRs whose head branch is chore/sync until at least one approver has admin permission (checked live via the collaborators API; no team to maintain). All other PRs keep the existing QA/DEV logic untouched, including the auto-pr skip. The approver scan now tolerates API failures: a 404 on the permission lookup (bot or offboarded reviewer) or a transient failure fetching reviews no longer aborts the step with a misleading "Waiting on: QA approval, DEV approval" status.
2. auto-sync-main-to-dev.yml — the sync PR body now states the procedure explicitly (admin merges as a merge commit, never squash, and why), and links the escalation plan ([TECH DEBT] CI/Branch protection | Enforce merge-commit syncs via ruleset with bot bypass (if squash recurs) #9566) so future admins have the context in front of them. gh pr create is now guarded by an explicit open-PR check instead of || echo, so a bad token or a failed create fails the run instead of logging a reassuring message.

No repo settings changes are required — this works with dev's branch protection exactly as configured today.

What this deliberately is — and isn't

This is an accountability speed bump, not mechanical enforcement: GitHub cannot restrict merge methods per-PR, admins bypass required checks (enforce_admins: false), and the gate would have been green on #9479 itself (same admin approved and squashed). We know that and accept it — the gate guarantees an aware admin is in the loop, and the instructions make the right action unambiguous at the moment of merge.

An auto-merge bot was prototyped on this branch and reverted after verification: ORG_ACCESS_TOKEN belongs to decentraland-bot (permission: write), which cannot create merge commits under dev's linear-history protection — and the alternative, an admin-owned token, would bypass all of dev's protection (checks, reviews), an over-powered credential reachable by any workflow in the repo.

If a squash slips through again, the escalation is designed, pre-analyzed, and tracked in #9566: move required_linear_history into a repository ruleset with a narrowly-scoped bypass (the sync bot, PR-merges only, still bound by checks and the admin-approval gate) and re-land the reverted auto-merge workflow. We're not applying it preemptively — we don't want to complicate the repo's governance more than it already is for a failure mode the simple procedure historically prevented.

Test Instructions

This is a CI workflow change; there is no in-app behavior to run.

Steps:

1. Review the diff of both workflow files.
2. The enforce-approvals change applies to sync PRs once this lands on dev. On the next chore/sync PR, verify the check stays red with only non-admin approvals and flips green after an admin approves.
3. The auto-sync-main-to-dev.yml change (PR body text, idempotent create) takes effect once it reaches main, since that workflow runs from main.
4. Afterwards, confirm gh api repos/decentraland/unity-explorer/compare/dev...main --jq .ahead_by returns 0 once the sync PR is merged.

Is it QA-able? No — CI-only change.

[github-actions]

New build in progress, come back later!

[decentraland-bot]

STEP 2 — Root-cause check: PASS ✅

The PR correctly identifies that squash-merging the chore/sync PR rewrites commit SHAs, preventing Git from recognizing dev as containing hotfixes applied directly to main. Enforcing merge commits preserves the original SHAs and eliminates the divergence. This addresses the root cause, not a symptom.

STEP 3 — Design & integration: N/A

This PR modifies only a CI workflow file (.github/workflows/auto-sync-main-to-dev.yml). No application code, ECS systems, or runtime components are changed. No lifecycle owner analysis required.

STEP 4 — Member audit: N/A

No public properties or accessors changed (workflow YAML only).

STEP 5 — Line-level review

P1 — PR title uses invalid type ci:

The semantic / title-matches-convention CI check is failing because ci is not in the repo's allowed type list:

break, chore, docs, feat, fix, refactor, revert, style, test, release

The branch name is chore/enforce-merge-commit-on-main-dev-sync, so the title should match:

chore: force merge commit on main→dev sync PR

This blocks merge — please update the PR title.

P2 — Missing trailing newline at EOF

See inline comment on line 35.

Security review

No security issues found. The workflow uses ${{ secrets.GITHUB_TOKEN }} (standard GitHub Actions practice), no hardcoded secrets, and no injection vectors — all gh CLI arguments are fixed literals with no user-controlled interpolation.

STEP 6 — Complexity: SIMPLE

Workflow YAML change, 1 file, +7/−2 lines. No runtime code, no ECS, no async patterns.

STEP 7 — QA assessment: NO

CI-only change — no user-facing runtime behavior affected.

STEP 8 — Non-blocking warnings: None

STEP 9 — Verdict

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single CI workflow file with a small YAML change — no runtime code touched
QA_REQUIRED: NO

---

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

[decentraland-bot]

PR Review — ci: force merge commit on main→dev sync PR

STEP 2 — Root-cause check: ✅ PASS

The PR correctly identifies the root cause: squash-merging the chore/sync PR rewrites commit SHAs on dev, so Git no longer recognizes that dev already contains hotfixes from main. The fix — forcing merge commits via gh pr merge --merge --auto — preserves shared commit ancestry and prevents future release conflicts. Evidence cited in #9490 (duplicate SHAs from squashed sync) confirms this is the cause, not a symptom.

STEP 3 — Design & integration: N/A

CI workflow file only — no runtime code, no ECS systems, no new long-lived units.

STEP 4 — Member audit: N/A

No code members introduced or modified.

STEP 5 — Line-level review

#	Sev	File	Issue
1	P2	auto-sync-main-to-dev.yml:30	gh pr create failure blocks auto-merge enablement (see inline)
2	P2	auto-sync-main-to-dev.yml:35	Missing newline at end of file

Security review

No new security issues introduced:

• GITHUB_TOKEN is correctly passed via env: block (no script injection risk)
• No untrusted input flows into run: blocks — all values are static strings
• Workflow triggers (push to main, workflow_dispatch) are trusted

Pre-existing items (out of scope for this PR):

• actions/checkout@v6 uses a mutable tag instead of a pinned SHA
• No explicit permissions block (inherits repo defaults)

STEP 6 — Complexity: SIMPLE

STEP 7 — QA: NO

CI-only change — no runtime code affected, nothing ships in the Unity player.

STEP 8 — Non-blocking warnings: None

---

REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single CI workflow file change (+7/−2) with no runtime code impact
QA_REQUIRED: NO

---

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

[dalkia]

Thanks for the deep dive — the GITHUB_TOKEN anti-recursion behavior is real, but the "head SHA with no check runs → auto-merge waits forever" inference doesn't hold for this repo. History disproves it.

dev's required status checks (via branch protection API): enforce-approvals, Build (macos), Build (windows64), Test (editmode), Test (playmode), plus 1 required approval.

The last three sync PRs were all authored by app/github-actions (i.e. opened via GITHUB_TOKEN), and every one of those required checks ran and reported green:

• chore: sync main to dev #9479 — enforce-approvals ✅, Build (macos) ✅, Build (windows64) ✅, Test (editmode) ✅, Test (playmode) ✅
• chore: sync main to dev #9458 and chore: sync main to dev #9433 — same pattern.

So on these PRs the pull_request checks do fire and do post results — the suppression isn't producing the "zero check runs" state described. And this PR doesn't touch how the branch is pushed or how the PR is created (same checkout, same gh pr create, same GITHUB_TOKEN); it only edits the body and adds the --auto step. So check-triggering behavior is identical to those three PRs.

Net: approve → required checks go green (as they always have) → auto-merge merges as a merge commit. The "require approval before merge" behavior is exactly what --auto preserves.

Note on QA and DEV Approvals: it shows FAILURE on these PRs, but it's not in dev's required-checks list, so it never gated merge and won't gate auto-merge either.

I'll keep an eye on the first real run since auto-merge itself is new here, but no fix needed for this finding — leaving as-is.

[charly-bg]

Great catch on the root cause — I independently verified the diagnosis and it's correct: #9479 (the sync merged right before the #9490 conflicts) was squash-merged (its commit has a single parent), which rewrites the synced commits and makes main/dev diverge. Merge-commit syncs are the right strategy. 👍

That said, I'm requesting changes — as written the new step will fail on every run, because of a branch-protection rule the prerequisites don't account for.

🚫 Blocker: dev requires linear history, so the merge-commit method is unavailable

Current branch protection on dev has required_linear_history: true, and rebase-merge is disabled at repo level. Linear history means PRs into dev can only be squash- or rebase-merged — and rebase is off, so squash is the only merge method a non-admin has on dev. The past squashes likely weren't carelessness; squash was the only button available.

Consequences:

• gh pr merge --merge --auto run as github-actions[bot] (not an admin) will be rejected — the merge-commit method is blocked on this base branch. The new step fails, and every future run of the sync workflow goes red.
• Today's sync chore: sync main to dev #9523 did land as a true merge commit (2 parents) — but only because enforce_admins is off and the merger bypassed protection as an admin. The bot can't do that.

Required fix: remove "Require linear history" from dev's protection rule, and add it to this PR's prerequisites checklist. (The two prerequisites currently listed are already satisfied — "Allow auto-merge" is on, and dev requires 1 approval + 5 checks — the one that actually blocks is this one.)

⚠️ Major: GITHUB_TOKEN events don't trigger workflows — two concrete impacts

Events initiated with the workflow's GITHUB_TOKEN don't create workflow runs. This isn't theoretical — I traced sync PR #9523: created by github-actions[bot] on Jul 30 15:14, force-pushed again by the bot at 21:52, and zero check runs existed until a human triggered them at 08:50 the next morning (verified via triggering_actor on the runs).

1. Auto-merge will arm, then wait on checks that never start. The bot's own push/create can't trigger the required Build/Test checks, so a human still has to kick CI and approve. The change still adds value (locks the merge method), but "merges automatically once approved and checks pass" oversells what happens without human touch.
2. The auto-merge itself will be attributed to github-actions[bot], suppressing post-merge workflows. build-unitycloud.yml triggers on push: branches: [dev] — the dev Unity Cloud Build would silently not run after each auto-merged sync. pr-closure.yml (Unity Cloud target cleanup, pull_request: closed) would also be skipped.

Fix for both: arm auto-merge (ideally also do the branch push + PR creation) with a GitHub App installation token or a bot PAT instead of GITHUB_TOKEN — e.g. actions/create-github-app-token and pass that as GH_TOKEN. We already operate bot tokens for this repo's cloud builds, so there's precedent.

⚠️ Major: the auto-merge step is skipped whenever the PR already exists

gh pr create fails when a chore/sync PR is already open — which happens on every main push while a sync PR is pending. Empirically: the Jul 30 21:52 run of this exact workflow failed with "a pull request for branch "chore/sync" into branch "dev" already exists". When the create step fails, the new Enable auto-merge step never runs. Two scenarios where that bites:

• The first main push after this change lands, if a sync PR opened pre-change is still open — auto-merge is never armed on it.
• Anyone disabling auto-merge on the open PR — later runs won't re-arm it.

Fix: make creation idempotent and arm unconditionally:

run: |
  gh pr create ... || echo "sync PR already exists"
  gh pr merge chore/sync --merge --auto

Minor notes

• The workflow has no permissions: block; it relies on the repo's permissive default. The new step raises the token's needed scopes — declaring permissions: { contents: write, pull-requests: write } explicitly would be more robust (moot if we switch to an App token).
• dev has dismiss_stale_reviews: false, and auto-merge persists across pushes. An approval given before a later bot force-push of new main content survives — so once a human re-kicks checks, content nobody re-reviewed can auto-land. Given sync content already shipped through review on main, probably acceptable — but worth a conscious decision.
• Residual risk worth noting in the body: repo-level squash stays enabled (feature PRs need it), so a human can still manually squash the sync PR before auto-merge fires. The body warning is the only guard for that path.
• The $'...' quoting and the > [!IMPORTANT] alert render correctly, and the trailing-backslash/EOF cleanup is good hygiene.
• The new-dependency label on this PR looks like an auto-labeler misfire — worth removing.

[dalkia]

@charly-bg — you were right, and the PR has pivoted accordingly (see the updated description). Point-by-point:

🚫 Linear-history blocker — confirmed, and it killed the auto-merge approach. Removing required_linear_history from dev is not an option for us, and with it in place gh pr merge --merge --auto as the bot can never succeed. So instead of fighting for a bot merge commit, the PR now goes the other way: enforce-approvals (already a required check on dev) fails on chore/sync PRs until a repo admin approves. The admin then merges as a merge commit via the admin bypass — the same path #9523 took. This needs zero changes to branch protection.

⚠️ GITHUB_TOKEN doesn't trigger workflows — moot for the merge, unchanged for CI kick-off. With no auto-merge, both concrete impacts you traced disappear: the merge is performed by a human admin, so build-unitycloud.yml and pr-closure.yml fire normally post-merge. The "checks need a human kick after the bot's push" behavior you traced on #9523 is pre-existing and untouched by this PR — a human is now required in the loop anyway (admin review), so the marginal cost is low. Happy to tackle an App/PAT token for the sync push separately if it keeps annoying us.

⚠️ Auto-merge step skipped when the PR already exists — fixed at the root. The fragile step is gone entirely, and gh pr create is now idempotent (|| echo), so the workflow no longer goes red on every push during a hotfix wave.

Minor notes: permissions: block — the workflow no longer needs merge capability, so its token needs are back to what they were before this PR; I've left it for a separate hardening pass. Stale-approval semantics — unchanged from today's branch protection (dismiss_stale_reviews: false); the admin gate inherits the same behavior consciously. Residual squash risk — acknowledged and now stated in the PR body as the known limit: an admin in the loop is the mitigation, and if a squash still slips through, the escalation path (linear history in a ruleset + bot bypass actor + workflow-performed merge) is designed and ready. new-dependency label — agreed it's a mislabel, removing it.

[charly-bg]

Re-reviewed at f0624068ce. The pivot resolves both blockers I raised — the linear-history impossibility is gone with --auto, and gh pr create is now idempotent. Thanks for the point-by-point reply. One new finding changes my read of the admin gate, though, so I'm still requesting changes.

🚫 The admin gate would not have prevented #9479 — the incident it was built for

I ran the new logic against the last five sync PRs. It gates who approves, but the divergence was caused by which merge button was clicked — and those are independent:

sync PR	approved by	merged by	result	new gate verdict
#9433	(nobody)	anicalbano (admin)	merge commit ✅	❌ red — but merged anyway
#9458	(nobody)	anicalbano (admin)	merge commit ✅	❌ red — but merged anyway
#9479	mikhail-dcl (admin)	mikhail-dcl (admin)	squash ❌ — caused #9490	✅ green
#9504	mikhail-dcl (admin)	anicalbano (admin)	merge commit ✅	✅ green
#9523	mikhail-dcl (admin)	anicalbano (admin)	merge commit ✅	✅ green

Two things fall out of that table:

On #9479 the gate is a no-op. An admin approved it at 09:53 and the same admin merged it at 11:02, choosing Squash. enforce-approvals would have been green. The one data point we have for this failure mode is a case the gate does not touch.

#9433 and #9458 show the gate is bypassed by exactly the population it targets. Both landed with zero approvals, so dev's existing 1-approval requirement was already bypassed — enforce_admins: false means admins skip required checks, including this new one. So for admins the gate is advisory, and for the other 47 write-access collaborators (55 total, 8 admins, restrictions: none) it's a barrier only until any admin approves once — after which they can squash, since under required_linear_history: true Squash is the only button they get.

I don't think that makes the PR wrong — the accountability signal and the documented rule in the body are real improvements, and the idempotency fix is a genuine bug fix worth landing on its own. But the body currently reads as if the mechanism closes the hole, and it doesn't. I'd reframe it as a speed bump and move the escalation path off "designed and ready" onto the actual backlog.

💡 Cheap mechanical alternative: assert the invariant post-merge

Rather than trying to control the merge method pre-merge (which needs the ruleset + bypass-actor work), detect the actual invariant right after it happens — no branch-protection change, no bypass actor, no new token:

# on: push: branches: [dev]
- run: |
    AHEAD=$(gh api repos/${{ github.repository }}/compare/dev...main --jq '.ahead_by')
    if [ "$AHEAD" -ne 0 ]; then
      echo "::error::main has $AHEAD commit(s) not reachable from dev — sync diverged"
      exit 1
    fi

ahead_by == 0 on dev...main is precisely "every commit on main is an ancestor of dev" — the property a squashed sync destroys and a merge commit preserves. It's green right now (status: behind, ahead_by: 0, behind_by: 12), so you'd be locking in a known-good state. It would have gone red within minutes of #9479 instead of surfacing as six conflicts at the next release, and it catches every divergence cause, not just squash. Wire it to whatever alerting you prefer.

⚠️ Confirming the collaborators/…/permission abort (agreeing with the bot review)

enforce-group-approvals.yml:62 — this one is real and I'd fix it before merge. I confirmed the checkable half: neither workflow sets a defaults: block nor a per-step shell:, so these run: steps get Actions' default bash -e {0}, where VAR=$(cmd) takes the substitution's exit status. A 404 (outside collaborator, offboarded member, bot login) or 403 (token without push scope) on any reviewer iterated before the admin therefore kills the step, and because MISSING_MSG is never written the if: always() status step posts the generic Waiting on: QA approval, DEV approval — so an admin who did approve sees a red check blaming missing QA/DEV approval. Mitigating factor: I checked the last five sync PRs and every approver was a type=User collaborator, so this isn't firing today; it's a latent trip-wire.

PERMISSION=$(gh api "…/collaborators/$reviewer/permission" --jq '.permission' 2>/dev/null || echo "none")

Same class, same line-family: APPROVERS=$(gh pr view …) will also abort the step on a transient API failure with that same misleading status.

Also +1 on confirming ORG_ACCESS_TOKEN can actually read that endpoint (it needs push access, more than the /orgs/*/teams/*/members calls need) — a workflow_dispatch run before merge settles it.

🟡 Minor

• || echo swallows every failure. Agreed with the bot's narrowing suggestion — a bad token or a failed chore/sync push now also exits 0 and logs the reassuring "already exists", which is the same silent-drift class that produced release: 2026-07-27 #9490.
• Body wording: "stays red until an admin approves" — on a bot-created PR the context is usually absent/pending rather than red, since the bot's own create/force-push doesn't start a pull_request run. Blocks non-admins either way; just imprecise.
• Stale approvals: with dismiss_stale_reviews: false and require_last_push_approval: false (both confirmed), an admin approval from sync-wave N keeps the gate green for the force-pushed content of wave N+1. You've accepted this consciously — noting only that it interacts with the finding above: the gate can be green with no admin having seen the current content, and non-admins can then squash it.
• select(.state == "APPROVED") counts an approval later superseded by CHANGES_REQUESTED from the same person — consistent with the pre-existing logic below it, so consistency rather than a new defect.

✅ Verified good

• The gate sits before the auto-pr short-circuit, so the sync PR reaches it instead of being waved through — that ordering is the crux and it's right.
• HEAD_REF is only ever string-compared, and git refs can't contain newlines or spaces, so no GITHUB_ENV injection channel.
• A fork branch named chore/sync trips the gate into demanding admin approval — strictly stricter, never weaker.
• exit 0 after writing GITHUB_ENV is safe; the runner reads the env file after the step process exits, so APPROVAL_CHECK=passed still reaches the status step.
• With gh pr merge gone, the sync workflow's token needs shrank back to pre-PR levels — deferring the permissions: block is reasonable now.

Bottom line: land the gh pr create idempotency fix and the line-62 tolerance fix — both are clear wins. On the admin gate, I'd ask for the body to stop implying it prevents the squash, and I'd argue the post-merge invariant check above gets you actual detection for ~10 lines and zero policy change, which seems like a better use of the next iteration than a gate the #9479 timeline shows would have passed.

[dalkia]

@charly-bg — the five-PR table convinced us, especially #9479 being green under the gate. The PR has pivoted again (updated description, new commits): instead of gating who can approve and hoping the right button gets clicked, automation now performs the merge itself. Point by point:

The pivot: auto-merge-sync-pr.yml

When an admin approves the chore/sync PR, a new workflow verifies the approver's permission, waits for dev's required checks (gh pr checks --required --watch), and merges with gh pr merge --merge --admin. The merge method is hardcoded — squash is unreachable through this path.

The piece that was missing from the earlier auto-merge attempt: --auto respects branch protection, so required_linear_history rejects it forever — but --admin bypasses with the token owner's privilege (enforce_admins is off), which is the exact same bypass admins already use to merge sync PRs correctly by hand. ORG_ACCESS_TOKEN runs it; confirming its owner has admin on this repo is on the checklist before we rely on it.

Replayed against your #9479 timeline: approval at 09:53 → bot merges as a merge commit as soon as checks are green → the 11:02 squash never happens. The gate's no-op case is exactly the case the bot closes, because the approval is the merge trigger. It also moots the stale-approval interaction you flagged: an approval is consumed by an immediate merge, so it can't linger to green-light the next force-pushed wave.

Why we went prevention-first instead of the post-merge invariant check

We're not rejecting it — we're deferring it, for three reasons:

1. It's detection, and detection is only as good as its alerting. The red lands in the Actions tab and as a ✗ on the commit — exactly where the original squash hid. Its real value needs the paging wired first (transition-deduped Slack message + assigned tracking issue, not a red run per push while diverged). We'd rather land that properly in a follow-up than bundle a check here whose failure nobody is guaranteed to see.
2. The repair is post-hoc and needs an admin direct-push (git merge main into dev, bypassing protection). Cheap when caught in minutes, but still a manual, privileged recovery. The auto-merge removes the need for recovery on the one failure mode we've actually observed.
3. Same effort bought the mechanical fix. For roughly the ten lines the check costs, the bot closes the hole rather than reporting it.

That said, your point that ahead_by == 0 catches every divergence cause stands — including the one this PR consciously can't stop: the #9433/#9458 pattern of an admin merging manually with zero approvals (enforce_admins: false means everything here is bypassable by admins; that's now stated plainly in the description as a known limit, per your reframing ask). The invariant check is the right safety net behind the bot, and we'll take it as the follow-up, with alerting.

Your requested fixes — both in

• Line-62 abort: fixed as you suggested — the permission lookup tolerates 404/403 (2>/dev/null || echo "none"), the APPROVERS fetch tolerates transient failures, and the new workflow's permission lookup got the same treatment.
• || echo swallow: gh pr create is now guarded by an explicit gh pr list open-PR check; a bad token or failed create fails the run instead of logging "already exists".
• Body wording ("stays red" vs absent/pending): kept the simpler phrasing in the sync PR body since either state blocks non-admins, but fair point — the description no longer claims the gate prevents anything on its own.

[charly-bg]

Re-reviewed at a1cf79dc44. This is the right architecture — taking the button out of human hands is what closes #9479, and the two fixes I asked for are in (the gh pr list pre-check is better than the || echo I suggested, since a genuine gh failure now aborts loudly). Confirming your point about downstream workflows too: because ORG_ACCESS_TOKEN is a user PAT rather than GITHUB_TOKEN, the resulting push to dev does trigger build-unitycloud.yml and the close does trigger pr-closure.yml. And API-created merge commits are signed with GitHub's web-flow key, so dev's required_signatures ruleset is satisfied.

Still requesting changes — the checklist item you flagged comes back negative, and it's fatal to the merge step.

🚫 Blocker 1 — ORG_ACCESS_TOKEN's owner is not an admin, so --admin will be rejected

You wrote "confirming its owner has admin on this repo is on the checklist before we rely on it." I checked, and it doesn't:

• The QA and DEV Approvals commit status is posted with ORG_ACCESS_TOKEN, and its creator is decentraland-bot (type=User) — that identifies the token owner.
• GET /repos/decentraland/unity-explorer/collaborators/decentraland-bot/permission → permission=write, role_name=write.
• The repo's 8 admins are eordano, pentreathm, dalkia, nachomazzara, charly-bg, subscriptionsdcl, mikhail-dcl, anicalbano. decentraland-bot isn't among them.

--admin is documented as "Use administrator privileges to merge a pull request that does not meet requirements" — with write permission there are no such privileges to use, and without the bypass merge_method=merge is refused outright by required_linear_history on dev. So the final step fails every time.

The failure mode is worse than a red run: the else branch posts "most likely chore/sync conflicts with dev" and a four-step conflict-resolution runbook. A permissions error gets reported to the team as a merge conflict, and the runbook won't fix it. At minimum, surface the actual gh stderr in that comment instead of asserting a cause.

On the fix — I'd push back on simply granting decentraland-bot admin. --admin bypasses everything: linear history, required checks, required approvals. Handing that to a long-lived PAT that's already spread across many workflows in a public repo is a much larger relaxation than the one you ruled out (dropping required_linear_history, which would keep checks and approvals enforced for all 55 collaborators). The surgical option keeps both properties:

1. Create a ruleset on dev carrying required_linear_history, with a GitHub App (or decentraland-bot) as a bypass actor, and drop linear history from classic branch protection. Classic protection has no bypass-actor concept, which is exactly why you're stuck reaching for --admin; you already run one ruleset on dev (required_signatures), so the machinery is in place.
2. Then the bot merges with a narrowly-scoped App token and no blanket admin PAT.

Either way, please verify the merge actually succeeds before this lands — a workflow_dispatch-able dry run, or one supervised sync.

🚫 Blocker 2 — pin the merge to the approved SHA

Agreeing with the bot review here, with the concrete flag: the approval is the trigger but the merge happens after --watch returns, and chore/sync is force-pushed on every push to main. So the bot can merge content no admin ever saw. gh pr merge has the flag for exactly this:

gh pr merge "$PR_NUMBER" --repo "$REPO" --merge --admin \
  --match-head-commit "${{ github.event.review.commit_id }}"

That turns a silent unreviewed merge into a clean abort, and the existing "approve again" recovery path covers it.

⚠️ Major — --fail-fast races the very check the approval turns green

enforce-approvals is red before the admin approves — that's the design. Both enforce-group-approvals.yml and auto-merge-sync-pr.yml fire on the same pull_request_review: submitted event, so they start together. If auto-merge's first poll lands before the review-triggered enforce-approvals run registers a new check run, the newest entry for that name is still the stale pre-approval failure, and --fail-fast exits on the first poll. The bot then posts "a required check failed" (nothing failed), tells the admin to approve again, and the retry re-runs the same coin flip.

To be precise about severity, I tested the part I was unsure of: gh pr checks does dedupe by name, keeping the most recent run — on this PR the raw statusCheckRollup has two semantic / title-matches-convention entries while gh pr checks shows one (the later). So this is a startup race, not a permanent deadlock. Still worth fixing, since the failure is intermittent and self-perpetuating.

Cleanest fix: don't wait on enforce-approvals at all. Step 1 already verified independently that the approver is an admin, so waiting on the check that encodes the same fact is redundant and is what creates the race. Watch the build/test contexts only, or drop --fail-fast so a transient red doesn't abort the watch.

🟡 Minor

• A second approval produces a bogus conflict comment. concurrency has no cancel-in-progress, so a second admin approval queues a run behind the first; after the first merges, the queued run's if still passes (pull_request.merged is frozen in the event payload) and gh pr merge fails on an already-merged PR → the "conflicts with dev" comment fires again. cancel-in-progress: true, or re-read merge state before merging.
• On a .github-only sync, "wait for required checks" waits for almost nothing. I ran gh pr checks 9491 --required: only enforce-approvals reports. Build (macos), Build (windows64), Test (editmode), Test (playmode) are absent, because test.yml is paths: ['Explorer/**']. --admin then bypasses the missing contexts, so such a sync merges with no build validation. Fine in practice (a .github-only sync carries no game code) — just not what the step name promises.
• timeout-minutes: 90 is adequate but fails silently. Longest observed required check is ~40 min (Unity Cloud Build 08:50→09:29 on chore: sync main to dev #9523), so the headroom is right. But on timeout the job is cancelled and neither comment step runs, so the PR stalls with no signal. A if: cancelled() notice would close that.
• No permissions: block on the new workflow. It uses ORG_ACCESS_TOKEN throughout, so permissions: {} costs nothing and documents that GITHUB_TOKEN is unused.

✅ Verified good

• head.repo.full_name == github.repository closes the fork-spoof angle — relevant since this repo is public and anyone can submit a review.
• The admin check reads github.event.review.user.login from the event payload, so it isn't spoofable, and a login can't carry shell metacharacters into the [ … == … ] test.
• 2>/dev/null || echo "none" / || echo "" on both gh calls in enforce-group-approvals.yml — the bash -e abort is gone, and both paths fail closed.
• Hardcoding --merge does make squash unreachable through this path, which is the property that was missing from every earlier revision.

Bottom line: the design is right and I'd like it to land. Before it can, the bot needs an identity that can actually perform the merge — and I'd prefer a ruleset bypass actor over an admin PAT — plus --match-head-commit and the enforce-approvals race fix. The misleading "conflicts with dev" comment is worth fixing regardless, since with the current token that's the message the team will actually receive.

[dalkia]

Update — the auto-merge pivot announced above is reverted (2ab1a21b2), and the PR is back to its simple shape: admin-approval gate + explicit instructions in the sync PR body, plus the two fixes you requested (approver-scan API tolerance, idempotent pr create guard), which stay.

What changed our mind: we verified the token. ORG_ACCESS_TOKEN belongs to decentraland-bot with write permission — so gh pr merge --merge --admin would fail on every run (no admin privilege to invoke, and classic linear-history protection has no per-actor bypass). The workable alternatives were an admin-owned token (bypasses all of dev's protection — checks and reviews included — and reachable by any workflow in the repo) or the ruleset migration. Neither felt proportionate to apply preemptively.

So the strategy is: land the speed bump now — your reframing is adopted in the description, it's explicitly not claimed to be enforcement — and the mechanical fix is fully designed and parked in #9566 (ruleset with required_linear_history, bypass scoped to a one-member bot team in PR-only mode + repo admins, then re-land the reverted workflow minus --admin). Trigger for applying it: a squash slipping through again. The manual procedure kept the branches in sync for years when admins were aware of it; the sync PR body now makes that awareness hard to miss and links #9566 for context.

[charly-bg]

Re-reviewed at 7d89acf67a. The revert is the right call, and the rewritten description is the best artifact this PR has produced — it states the residual honestly, records why the auto-merge was backed out, and no longer claims enforcement it doesn't have. Approving.

First, though, a correction I owe you.

🔄 I was wrong that the gate is a no-op — it addresses 2 of the 3 recent incidents

My #9479 table was built from the five most recent sync PRs. I've now audited all 118 chore/sync PRs ever merged, and the picture is different from what either of us was working with:

Six squashes, all-time (1-parent merge commits): #2507 (2024-10-21), #2531 (2024-10-22), #2769 (2024-11-12), then #9014 (2026-06-19), #9060 (2026-06-30), #9479 (2026-07-27).

In all three recent cases the approver and the merger were the same person. The permission split is what matters:

PR	self-approved + merged by	permission	new gate verdict
#9014	davidejensen	write	❌ red — merge blocked
#9060	NickKhalow	write	❌ red — merge blocked
#9479	mikhail-dcl	admin	✅ green — not blocked

A non-admin's approval doesn't satisfy the new gate, and a non-admin can't merge past a red required check (restrictions: none, enforce_admins: false — so only admins bypass). So the gate would have blocked the exact merge action taken in #9014 and #9060. My "the gate can't constrain which button gets clicked" line was right about #9479 and wrong as a general claim. Apologies — the PR does more real work than I credited.

To keep it precise: the gate forces an admin into the loop, it doesn't make Squash unreachable. Once any admin approves, all 55 write-access collaborators can merge, and non-admins still see only the Squash button. So the residual is broader than "an admin squashes it": it's "an admin approves, then anyone squashes it."

⚠️ The deferral rationale rests on n=1, and the trigger has already fired three times

Both the description and #9566 cite only #9479 — "one lapse is enough," "what failed in #9479 was awareness in the moment." #9014 and #9060 appear nowhere. That matters for the specific decision being made, because #9566's trigger is "apply only if a sync PR gets squash-merged again despite the admin-approval gate."

The real rate: 19 months clean (2024-11-12 → 2026-06-19, ~75 sync PRs, zero squashes), then 3 squashes in 7 weeks. So "it kept main and dev in sync for years" is genuinely true of the middle stretch — I'm not disputing it — but "a failure mode the simple procedure historically prevented" describes the 2025 regime, not the current one. The procedure is degrading, and #9566 is parked at 3-low, unassigned.

I'd suggest two edits rather than reopening the design debate:

• Add chore: sync main to dev #9014 and chore: sync main to dev #9060 to [TECH DEBT] CI/Branch protection | Enforce merge-commit syncs via ruleset with bot bypass (if squash recurs) #9566's history so the next reader sees 3, not 1.
• Given the gate covers the chore: sync main to dev #9014/chore: sync main to dev #9060 shape but not chore: sync main to dev #9479's, the honest trigger isn't "if a squash slips through again" — it's "if an admin-approved squash slips through again." Worth stating that way, since it's a narrower and much less likely event to notice.

💡 Why two of the three squashes went unnoticed — and what that says about the safety net

Worth recording, because it explains the detection gap: a squash's divergence is self-healing at the next merge-commit sync. Merging main into dev makes main's tip — and therefore its whole history, including the pre-squash originals — an ancestor of dev again. Empirical proof: compare/dev...main is ahead_by: 0 right now despite six historical squashes.

So a squash is only damaging if a release → main merge lands inside the window before the next correct sync:

• chore: sync main to dev #9014 (Jun 19) and chore: sync main to dev #9060 (Jun 30) → healed by chore: sync main to dev #9103 (Jul 1, 2 parents). No release merge in the window. Nobody noticed.
• chore: sync main to dev #9479 (Jul 27) → the release: 2026-07-27 #9490 release merge landed Jul 28, inside the window. Six conflicts.

That reframes the risk as a time bomb whose fuse depends on release timing, not a guaranteed failure — which is exactly why the procedural approach has felt reliable. It also sharpens the case for the ahead_by == 0 check you've deferred twice: its value isn't catching a guaranteed break, it's catching the divergence during the self-healing window, when repair is one merge instead of a conflicted release. I'm not re-arguing the deferral — you've decided it twice with reasons — but I'd weight it higher than 3-low on the strength of "3 in 7 weeks, 2 of them invisible."

⚠️ The new instructions never reach an already-open sync PR

Agreeing with the bot review here. The idempotency guard exits before gh pr create, and the instruction text lives only in --body, so the body is written exactly once at creation. Any sync PR that stays open across subsequent main pushes keeps whatever body it was created with, even as the branch is force-pushed to new content.

To be fair, this isn't a regression from the fix I asked for — the old gh pr create failed on the already-exists path too and never updated the body either. What changed is that the body is now load-bearing: it's the entire mitigation for the admin path. Cheap close:

BODY=$'…'   # single source for both paths
if [ "$OPEN_PRS" -ne 0 ]; then
  gh pr edit "$(gh pr list --base dev --head chore/sync --state open --json number --jq '.[0].number')" --body "$BODY"
  echo "Sync PR already exists — refreshed its body."
  exit 0
fi

Timing note that makes this easy: there is no open sync PR right now (#9556 merged today at 10:15, ahead_by: 0). If this lands before the next main push, the transitional case never occurs and only the long-lived-PR case remains — worth merging while that window is open.

✅ Verified

• Diff is exactly two files, nothing outside .github/workflows/; the deleted auto-merge-sync-pr.yml never reached dev or main, so no orphaned required-check context.
• Both requested fixes survived the revert: 2>/dev/null || echo on the approver scan and permission lookup (fails closed, no bash -e abort), and the explicit open-PR pre-check (a genuine gh failure now aborts loudly).
• The gate sits before the auto-pr short-circuit, so sync PRs reach it while every other PR keeps the existing QA/DEV logic.
• dev is currently healthy: ahead_by: 0, behind_by: 14.
• The description's self-assessment is accurate on every point I can check, including that the gate would have been green on chore: sync main to dev #9479.

Bottom line: approving. It's a real improvement on 2 of the 3 recent incidents, it needs no branch-protection changes, and it's described without overclaiming — which is what I asked for last round. The two things I'd still do: refresh the body on the already-exists path, and correct #9566's incident history to three so the escalation isn't waiting on a fourth. The new-dependency label is still attached, by the way.

[dalkia]

@charly-bg on the already-open sync PR body refresh (gh pr edit --body-file on the already-exists path instead of exit 0) — I'm going to skip it, and I think the timing you flagged makes it unnecessary rather than just cheap.

There's no open chore/sync PR right now (#9556 merged, ahead_by: 0), and we're merging this before the next main push. So the next sync PR gets created fresh through gh pr create with the load-bearing body — the body is always current at creation. The already-exists path is only reachable by a sync PR that stays open across multiple main pushes, which won't happen in this merge window. Since we're landing it before the next branch appears, the body will always be up to date.

If a long-lived sync PR ever becomes a real pattern, adding the gh pr edit --body-file refresh at that point is a one-liner and we can revisit.

Separately, I've updated #9566 with the full picture per your audit: three squashes in seven weeks (#9014, #9060, #9479), the per-incident gate table, and the self-healing window note. I also reframed its trigger to "an admin-approved squash", since the gate already blocks the non-admin #9014/#9060 pattern. Thanks for the thorough review 🙏

[claude]

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

---

Post-merge re-review — 4b1e5b04b (PR now merged as 37b388f57)

• Determine what changed on this event
• Verify the merged diff matches what was reviewed
• Re-check previously-open findings against the merged state
• Re-run dependency + workflow security review
• Post verdict

---

Nothing reviewable changed on this event — and the PR has since merged

This synchronize added 4b1e5b04b, another dev merge only, bringing 44366d256 (#9471, the Afrikaans language-name .asset fix). It contributes nothing to this PR's effective diff.

Verified mechanically:

• git diff 7d89acf67..4b1e5b04b -- .github/ is empty — both workflow files are byte-identical to the revision @charly-bg approved, across both intervening dev merges.
• The PR merged into dev as 37b388f57, and git diff origin/chore/enforce-merge-commit-on-main-dev-sync HEAD -- .github/workflows/ is empty — the shipped state on dev is exactly what was reviewed. Nothing drifted in the merge.

On 37b388f57 having a single parent: it's a squash, and that is correct here — this is a feature PR into dev, where required_linear_history makes squash the only method. It is not an instance of the failure mode this PR is about; that one is specifically the chore/sync PR.

---

⚠️ One new finding — the gate goes live on dev now, the instructions don't ship until main

This is the deployment asymmetry behind test-instruction steps 2 and 3, and it changes the conclusion of the body-refresh discussion:

• enforce-group-approvals.yml is live immediately. It fires on pull_request / pull_request_review against dev, so the definition comes from dev (or the head↔base merge, where dev's newer copy wins since main didn't touch that file). The next chore/sync PR is gated.
• auto-sync-main-to-dev.yml is not. It fires on push: branches: [main], so GitHub runs the copy at main's tip. main still carries the pre-PR version until a release → main merge carries this over.

So the next sync PR will be created by the old workflow with the old body — no > [!IMPORTANT] block, no merge-commit instruction, no #9566 link — while the new required check is red demanding an admin approval. An admin lands on a red enforce-approvals with no explanation of what it wants or which button to press.

That matters for @dalkia's reasoning above:

the next sync PR gets created fresh through gh pr create with the load-bearing body — the body is always current at creation

Correct as a statement about gh pr create, but the create runs from main, not from dev — so the immediate next sync PR gets the old body regardless of whether a PR was already open. The window isn't "a long-lived sync PR"; it's every sync PR between now and this reaching main, which is exactly the window where the instruction text is the only mitigation for the admin path the gate can't block.

Nothing to change in the workflow for it — this resolves itself once the code reaches main. The cheap operational cover is to tell whoever merges the next sync PR what the red check wants, or to post the instruction text as a comment on that PR by hand. Worth a heads-up in the release channel rather than a code fix.

---

🟡 Minor — carried forward, all reasonable to defer

• Sync PRs get a factually wrong green status description. enforce-group-approvals.yml:177 posts QA and DEV approvals satisfied, but the sync path (:55-76) never evaluates QA or DEV. The commit status is the durable record of why the check went green; one if [ "$HEAD_REF" == "chore/sync" ] branch on DESC fixes it.
• No permissions: block on either workflow (W.10, MEDIUM).
• select(.state == "APPROVED") at :59 counts an approval later superseded by CHANGES_REQUESTED — consistent with the pre-existing logic at :117, so consistency rather than a new defect.
• :79/:106/:116/:121 still abort under bash -e on a transient gh failure and surface the misleading Waiting on: QA approval, DEV approval. Pre-existing; the new block is tolerant.
• workflow_dispatch from a non-main ref force-pushes that ref onto chore/sync while the new guard reports success — if: github.ref_name == 'main' on :16-19 closes it.
• The already-open-PR path still never refreshes the body (:26-29). Declined by @dalkia, and I agree it's low-value as a standalone fix — but see the finding above for why the timing argument it rested on doesn't hold.

✅ Verified good (re-confirmed against the shipped files on dev)

• Gate ordering is right — :55 sits before the auto-pr short-circuit at :81. The sync PR carries auto-pr, so a gate placed below it would be waved through. This is the crux and it holds.
• The gate cannot weaken anything. For chore/sync it replaces logic that previously exited 0 on the label, so that path is strictly stricter. A fork branch named chore/sync trips it into demanding admin approval. No branch name relaxes any requirement.
• No deadlock. Non-admins already needed 1 approval; they now need that approver to be an admin. Admins bypass, so the sync can always land.
• API-tolerance fixes shipped — 2>/dev/null || echo "none" at :62, || echo "" at :59, both failing closed.
• The gh pr list guard fails closed — an empty or non-integer OPEN_PRS makes [ … -ne 0 ] return non-zero and bash -e aborts, rather than falling through to a duplicate gh pr create. Strictly better than the || echo it replaced.
• $'…' quoting verified mechanically. The file contains 4 single quotes total: a self-contained pair in --jq 'length' on :25, and the two $'…' delimiters on :34. No interior apostrophe truncates the body; backticks, ", #, !, > are all literal inside ANSI-C quoting, so `main`, ("Merge pull request", …) and > [!IMPORTANT] render as intended.
• exit 0/exit 1 after writing $GITHUB_ENV is safe — the runner reads the env file after the step process exits, so APPROVAL_CHECK reaches the if: always() status step on both paths.
• HEAD_REF is only ever string-compared and git refs can't contain newlines or spaces — no GITHUB_ENV injection channel. for reviewer in $APPROVERS word-splitting is safe (logins are [A-Za-z0-9-]).
• W.1–W.10: no HIGH finding. No ${{ }} interpolation into any run: body (everything routes through env: EVENT_PAYLOAD: ${{ toJSON(github.event) }} + jq -r), no LLM invocation or allowedTools, no pull_request_target, no checkout of PR code, no third-party action added, no new secret consumer, no $GITHUB_OUTPUT heredoc. With --admin reverted, the shipped revision contains no branch-protection bypass primitive at all.
• No dangling reference to the reverted auto-merge-sync-pr.yml anywhere in the tree, and it never reached dev or main — so no orphaned required-check context.

---

Dependency review (STEP 1–3)

No dependency changes. Nothing under Packages/manifest.json, Packages/packages-lock.json, or Explorer/Assets/Plugins/; no binary, no native library, no new .asmdef, no editor/build/install hook. The .asset, C# and prefab files in the two dev merge commits arrive via dev (#9527, #9471) and are not part of this PR's diff. Nothing here ships in the desktop client, so STEP 1.5 registry lookups don't apply.

dependency / file	version	source	type	scope	risk	confidence
.github/workflows/enforce-group-approvals.yml	n/a	in-repo	CI config	CI-only	LOW	HIGH
.github/workflows/auto-sync-main-to-dev.yml	n/a	in-repo	CI config	CI-only	LOW	HIGH

The new-dependency label is still attached per the PR metadata, and is still a labeler misfire.

---

Summary

No diff change on this event, and the merged state on dev is byte-identical to the approved revision. Security posture unchanged and clean: no HIGH on W.1–W.10, no bypass primitive, no new secret consumer.

The one thing I'd act on is operational, not a code change: the gate is enforcing on dev now, but the explanatory body text won't reach a sync PR until this lands on main. Until then, whoever merges the next chore/sync PR sees a red required check with no instructions — worth a heads-up so the merge-commit procedure is followed in that gap.

Everything else is carried-forward minors, plus @charly-bg's tracking-side asks (which @dalkia has since applied to #9566: three squashes recorded, trigger narrowed to admin-approved) and dropping the new-dependency label.

DEPENDENCY_REVIEW: NEEDS_ATTENTION

I made no changes — GitHub App permissions don't allow modifying files under .github/workflows/, and there's nothing here I'd change in code anyway. gh is outside my tool allowlist in this environment, and the clone is shallow with fetch unavailable, so #9566, the 118-PR squash audit, dev's live branch protection, and main's current copy of auto-sync-main-to-dev.yml are reasoned from the review discussion and trigger semantics rather than fetched. Everything stated as "verified mechanically" was checked against local refs.

---

· Branch ·