fix: sync-main-to-experimental action correct base branch

[pravusjif]

Problem

The sync job reset chore/sync to main's HEAD instead of merging into experimental, so the branch — and the @dcl/protocol package built from it — carried none of experimental's content. Regenerating in the Explorer against that package deleted experimental-only schema, e.g. public enum AvatarEmoteMask in AvatarShape.gen.cs.

Squashing the sync PR breaks it a second way: main stops being an ancestor of experimental, the merge base freezes, and later syncs conflict on content experimental already has.

Changes

sync-main-to-experimental.yml — builds chore/sync as experimental + main

• fetch-depth: 0 and a git identity, so the merges work at all
• branches from experimental, or reuses an in-flight chore/sync so a manual conflict resolution survives later pushes to main
• pushes whenever the recomputed branch differs from the remote; opens the PR only when something is actually ahead of experimental
• a conflict aborts, pushes nothing, and fails with recovery instructions
• the PR body warns against the Squash button

merge-sync-to-experimental.yml — new, manual (workflow_dispatch), lands the sync as a merge commit

allow_merge_commit=false means the PR button can only squash. That setting governs the button, not git push, and experimental is unprotected — so the workflow merges chore/sync into experimental with --no-ff, pushes, and deletes the branch. The repo setting stays false; no more flipping it on and off to land these.

Full cycle

1. Someone merges a PR into main.
2. The sync workflow builds chore/sync and opens the sync PR into experimental. build-deploy publishes the tarball and comments the install URL; validate-compatibility runs.
3. More commits land on main → they are merged onto the same chore/sync, so the open PR just accumulates them.
4. Test the tarball in the Explorer, review the PR.
5. When it's good: Actions → Merge sync PR into experimental → Run workflow. Not the green button.
6. The workflow pushes the merge commit. GitHub closes the PR as Merged once its commits are reachable from experimental, then the workflow deletes chore/sync.
7. The next push to main starts a fresh cycle from experimental.

Step 6's deletion matters: the sync workflow reads "chore/sync exists" as "a sync is still in flight". GitHub's delete_branch_on_merge only fires for button merges, not for a PR closed by a push, so the workflow does it explicitly.

If it gets squashed anyway

Nothing blocks the button. Restore the ancestry with a no-content merge that records main as a parent:

git checkout experimental
git merge -s ours origin/main -m "chore: record main ancestry"
git push origin experimental

[github-actions]

Test this pull request

• The @dcl/protocol package can be tested in scenes by running

  npm install "https://sdk-team-cdn.decentraland.org/@dcl/protocol/branch//dcl-protocol-1.0.0-33929112256.commit-097fbbe.tgz"

[decentraland-bot]

Review: fix: sync-main-to-experimental action correct base branch

Files changed: 1 (+67 −5) — .github/workflows/sync-main-to-experimental.yml
CI: All checks passing ✅
ADR-6: PR title (fix: …) and branch (fix/sync-main-to-experimental-action) follow semantic conventions ✅

---

Problem & Fix Assessment

The old workflow was fundamentally broken: git checkout -B chore/sync without specifying a start point created chore/sync at main's HEAD, discarding all experimental-only content. The new approach correctly builds chore/sync as experimental + main — experimental-only protos survive the sync.

Analysis

✅ Architecture & Merge Strategy

• Correct base: branching from origin/experimental (or reusing origin/chore/sync when a PR is already open) ensures experimental-only content is preserved.
• Uniform merge loop: the for REF in origin/experimental origin/main loop handles both fresh and reuse paths cleanly. When starting from origin/experimental, the first merge is a no-op ("Already up to date") — correct and harmless.
• Conflict handling: merge --abort || true + ::error:: + exit 1 fails cleanly instead of pushing broken state.
• No-op detection: git diff --quiet origin/experimental HEAD correctly skips the push and PR creation when experimental already contains everything in main.

✅ Race Condition Prevention

• Concurrency group (cancel-in-progress: false) ensures two pushes to main queue instead of racing on the push to chore/sync.
• --force-with-lease instead of --force guards against overwriting manual conflict resolutions pushed between the fetch and the push. Significant safety improvement over the old --force.

✅ Edge Cases Verified

Scenario	Behavior
First run, no existing PR	Branches from experimental, merges main, creates PR
Existing PR open, chore/sync exists	Reuses branch, preserves manual conflict resolution
PR was closed but chore/sync still exists	PR_NUMBER is empty → falls back to origin/experimental (fresh start)
Nothing to sync	Prints message, sets changed=false, skips push + PR
Merge conflict	Aborts merge, emits ::error::, exits 1
Manual push to chore/sync during run	--force-with-lease rejects the push (correct — protects manual work)
workflow_dispatch trigger	Works identically to push trigger

✅ Security

• No secrets exposure: only GITHUB_TOKEN (automatically provisioned).
• Minimal permissions: contents: write (push), pull-requests: write (PR create/comment), issues: write (needed by gh pr comment/--label which internally hit the Issues API). All justified.
• No injection vectors: no untrusted user input flows into shell commands. The workflow triggers are push (to main) and workflow_dispatch — both trusted.
• No hardcoded credentials or sensitive data in logs.

✅ Shell Scripting Quality

• set -euo pipefail on all run blocks.
• Variables properly quoted ("$PR_NUMBER", "$BASE").
• // empty in jq correctly produces empty string (not null) when no PR exists.
• git rev-parse --verify --quiet safely checks branch existence without error output.
• Step outputs via $GITHUB_OUTPUT (modern GitHub Actions pattern, not deprecated set-output).

ℹ️ Why the git identity is required

The old workflow never created commits — it only did checkout -B and push. The new workflow creates merge commits (git merge --no-edit -m "...") and git requires an author identity for that. The github-actions[bot] user with email 41898282+github-actions[bot]@users.noreply.github.com is the standard GitHub Actions bot identity (41898282 is the bot's numeric user ID). This is the correct and conventional approach.

✅ Consumer Impact

This change modifies an internal CI workflow only. No public API surfaces, exported packages, or schemas are affected. No downstream consumer impact.

Verdict: APPROVE ✅

No P0 or P1 issues found. The fix is well-designed, handles edge cases properly, and includes multiple safety improvements over the original (--force-with-lease, concurrency group, conflict detection, no-op skip). The PR description is thorough and accurately explains both the problem and the solution.

---

Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack

[decentraland-bot]

✅ Approved by Claude, approved by Codex — fixes the sync-main-to-experimental workflow to build chore/sync as experimental + main instead of force-pushing at main's HEAD, preserving experimental-only schema.

Checked: merge logic for both fresh-branch and existing-PR paths converges to the correct experimental+main tree; --force-with-lease guarded by the wildcard-refspec fetch immediately before it; concurrency group (cancel-in-progress: false) serializes concurrent pushes to main; set -euo pipefail + git merge --abort || true + exit 1 fails safely on conflicts; git diff --quiet origin/experimental HEAD correctly compares final tree state; PR_NUMBER is jq-derived and double-quoted everywhere; permissions block is minimal (contents, pull-requests, issues: write); step outputs (pr_number, changed) are correctly written and consumed; changed left unset on sync-step failure correctly skips the PR step; no shell injection vectors; CI green (Deployment Notification, Validate compatibility, check_and_build).

---

Cross-model review by Jarvis 🤖 · head 5ece7fe · Claude + Codex · Requested by Charly (<@U0747ARK5TM>) via Slack

[charly-bg]

The diagnosis is right and I confirmed the two git facts it rests on: checkout -B chore/sync with no start point did sit at main's HEAD, and bot-created chore/sync PRs do get build-deploy runs (10 historical pull_request runs on headBranch=chore/sync), so the broken package is real. The core fix — fetch-depth: 0, branch from origin/experimental, merge origin/main — produces the same content shape as the manual PRs #475/#480, and validate-compatibility passes on that shape (verified via #480's run).

Three things to settle before merging.

P1 — after a conflict the workflow can never open the PR

Create or update PR is gated on changed == 'true', so a conflict's exit 1 skips it. The reuse path (L55) needs an already-open PR. First conflict is therefore a closed loop: a human must create chore/sync, resolve, push, and open the PR by hand before the job can self-heal — and the ::error:: names a branch that doesn't exist on the remote at that point, because nothing was pushed. main stays red until then (the job runs on push: main).

Either push the conflicted state to a side ref, or spell out all four recovery steps in the annotation.

P1 — the no-op gate suppresses the push, not just the create (L71-75)

git diff --quiet origin/experimental HEAD decides whether to push and whether to touch the PR. Sequence: run 1 pushes chore/sync and opens the PR → main reverts the change → run 2 rebuilds from origin/chore/sync, merges the revert in, tree now matches experimental → changed=false → the correct branch is discarded and the stale remote chore/sync stays as the PR head, still proposing the reverted content. Merging it re-introduces the revert into experimental. No comment, no failure, nothing clears it.

The check exists to dodge gh pr create's "No commits between…", so gate the create on it. If you want a push gate too, the comparand is origin/chore/sync vs HEAD.

P1 (precondition, not a defect in this diff) — the repo is squash-only as of today

allow_merge_commit=false, allow_rebase_merge=false, allow_squash_merge=true right now — but experimental's tip is 4f4e0ab4, a 2-parent Merge pull request #480 merged at 12:52 today, so the flip is hours old. No ruleset or required_linear_history is forcing it.

Real merge commits are what keep main an ancestor of experimental (merge_base == main today) and keep the merge base advancing. Under squash-only that linkage breaks and git merge origin/main starts conflicting on content experimental already has from main — within about two syncs. The old job had the same broken linkage but never merged locally, so it failed silently; this PR turns it into a hard red. Worth confirming merge commits are coming back before this lands.

P3 — non-blocking

• L42-46 are dead code, and the comment's premise is false. actions/checkout@v4 does git remote add origin (which writes the wildcard remote.origin.fetch) and, with fetch-depth: 0, fetches +refs/heads/*:refs/remotes/origin/* via getRefSpecForAllHistory; nothing in checkout ever narrows remote.origin.fetch. The narrowing only happens at the default fetch-depth: 1, which this diff removes. The stale info failure is real (I reproduced it with a narrowed refspec) but unreachable once fetch-depth: 0 is set. Delete both lines and the comment.
• --force-with-lease doesn't protect what L78-79 claims. The lease resolves through the tracking ref that L46 refreshes moments earlier, so it's satisfied by whatever is on the remote. Reproduced: human commit on chore/sync, PR closed → BASE=origin/experimental → + f5c9690...445f29b (forced update), human work gone. The thing that actually preserves manual work is the BASE=origin/chore/sync reuse at L56. Keep the lease, but drop the claim.
• issues: write is unnecessary and its comment is wrong: gh applies --label via GraphQL updatePullRequest{labelIds} and comments via addComment(subjectId: <PullRequest>) — both under pull-requests. contents: write + pull-requests: write cover everything the job does.
• --no-edit is redundant with -m (L64). --set-upstream (L80) is pointless — the workspace is discarded each run and it has no bearing on the lease.
• A real lease race rejects with a bare (stale info) and a red job, no ::error::. Same for a transient gh pr list failure under set -e — the sync is silently skipped until the next push.
• PR_NUMBER is read at L48 and consumed at L90. If the PR merges in between (delete_branch_on_merge=true makes merge-then-push-to-main the normal sequence) the comment lands on a closed PR and no new PR is opened that run. Narrow, self-heals.
• The ::error:: says "Conflict" for any merge failure, including unrelated histories or a dirty tree.
• The base version's --jq '.[0].number' never emitted "null" in practice (run 33861349214's log shows the empty-PR path working), so // empty is hardening rather than a fix.

Checked and cleared

concurrency (GitHub keeps one pending run per group, but each run recomputes from origin/main's tip, so nothing is dropped) · set -euo pipefail interactions (if ! git merge and git rev-parse --quiet are in condition context) · git merge --abort || true after a non-conflict failure (exit 128, correctly swallowed; post-abort state clean, nothing pushed) · --force-with-lease on a brand-new remote branch with no tracking ref (pushes as a create — which is the repo's state right now, chore/sync is 404) · auto-pr label exists · fetch-depth: 0 cost (repo is 1.3 MB) · push permissions (no rulesets, chore/sync and experimental unprotected, token can push) · the new permissions: block only narrows the repo default · output plumbing (pr_number written before every early exit, passed via env: so no injection surface) · the current changed=false first-run case is correct (behind_by=0 today, so the next run legitimately no-ops).

Simplicity

Over-built by roughly ten lines, and the excess is where the bugs cluster — the reuse-existing-branch machinery accounts for most of the findings above. Keying "rebuild vs. keep" on whether the remote branch already contains origin/main's tip, rather than on whether a PR happens to be open, would remove the PR lookup, the two-ref loop and the cross-step PR_NUMBER hand-off at once. Dropping the sync branch entirely for gh pr create --base experimental --head main is not viable — conflict resolutions would have to land on main.

[decentraland-bot]

Re-review: fix: sync-main-to-experimental action correct base branch

Files changed: 2 — .github/workflows/sync-main-to-experimental.yml (+83 −6), .github/workflows/merge-sync-to-experimental.yml (new, +67)
CI: Pending (check_and_build, Validate compatibility — both in progress)
ADR-6: PR title (fix: …) and branch (fix/sync-main-to-experimental-action) follow semantic conventions ✅
Head: a364e95 (includes "mini refactor based on PR feedback")

---

What changed since the last review (5ece7fe → a364e95)

The latest commit addresses the two code-level P1s from @charly-bg's review:

charly-bg finding	Status
P1 — conflict can never open the PR (step-output gating + incomplete error message)	✅ Fixed — step-output gating removed; PR step always runs; ::error:: now includes full recovery instructions
P1 — no-op gate suppresses push, not just PR create (git diff --quiet gated both)	✅ Fixed — push gate now compares HEAD vs origin/chore/sync; PR-create gate uses merge-base --is-ancestor HEAD origin/experimental
P1 — repo is squash-only (precondition)	ℹ️ Precondition, not a code defect — PR body now includes explicit squash warning + recovery instructions

Additional cleanup: issues: write removed, --no-edit removed, --set-upstream removed, --force-with-lease claim dropped, branch-reuse decoupled from PR existence (just checks branch existence).

---

Analysis of current state

✅ Core merge logic

The fundamental fix is correct: branching from origin/experimental (or reusing origin/chore/sync) and merging origin/main into it preserves experimental-only content. The for-loop approach handles both fresh and reuse paths uniformly.

✅ Concurrency & race protection

• concurrency groups with cancel-in-progress: false serialize pushes to main correctly.
• --force-with-lease correctly rejects the push if a human pushed to chore/sync between fetch and push (verified: the tracking ref from the fetch is the lease comparand).

✅ Push & PR gate separation

• Push decision: HEAD vs REMOTE_HEAD — pushes whenever the recomputed branch differs from the remote. This fixes charly's scenario (main reverts → stale remote stays). ✅
• PR-create decision: merge-base --is-ancestor HEAD origin/experimental — skips PR creation when experimental already contains everything. ✅

✅ Security

• No injection vectors: All dynamic values are either hardcoded strings or integers from gh API. No untrusted input flows into shell commands.
• Permissions minimal: contents: write + pull-requests: write (sync), contents: write + pull-requests: read (merge). All justified.
• Triggers safe: push on main (requires repo write access) and workflow_dispatch (same). No pull_request_target or other external-triggerable events.
• No secrets exposure.

✅ Conflict handling

merge --abort || true + expanded ::error:: with four recovery steps + exit 1. Clean, nothing pushed on conflict. The recovery instructions correctly cover both the "branch exists" and "branch doesn't exist" cases.

---

Remaining findings (all P2)

[P2] Merge workflow no-op path skips cleanup

merge-sync-to-experimental.yml: when --is-ancestor origin/chore/sync origin/experimental is true (already merged), the workflow exits 0 without deleting chore/sync or affecting the PR. This is reachable after a partial failure (experimental push succeeds, branch delete fails in a prior run) or an external merge. Re-running the merge workflow always hits the same no-op exit.

Self-heals on next meaningful push to main (sync workflow recomputes and pushes). Manual fix is trivial (git push origin --delete chore/sync). Consider adding git push origin --delete chore/sync || true to the no-op path for robustness.

[P2] Redundant git config --replace-all in both workflows

With fetch-depth: 0, actions/checkout@v4 already sets remote.origin.fetch to the wildcard refspec via getRefSpecForAllHistory. The git config --replace-all line is dead code (as charly noted). The git fetch that follows is useful (refreshes refs), but the config line can be removed. Harmless, but noise.

[P2] Empty-diff PR lingers after net-zero main changes

If main adds then reverts a change while a sync PR is open, chore/sync accumulates merge commits not in experimental's graph even though the tree is identical. --is-ancestor (commit-graph-based) returns false, so the PR stays open with 0 files changed. Merging it creates an empty merge commit on experimental — harmless but noisy. A supplementary git diff --quiet origin/experimental HEAD check could catch this and skip PR maintenance.

---

Verdict: APPROVE ✅

No P0 or P1 issues. The latest commit substantively addresses both code-level P1s from the prior review. The push/PR gate separation is correct, conflict recovery is well-documented, and the two-workflow design (sync creates the branch, merge lands it as a real merge commit) is sound for working around the squash-only constraint. The P2 items above are worth addressing but do not block merge.

Note: charly's third P1 (squash-only repo setting) is a precondition — worth confirming merge commits are re-enabled or that the team is committed to using the merge workflow exclusively, but not a defect in this diff.

---

Reviewed by Jarvis 🤖 · Requested by Pravus (<@UDJQDQC0Z>) via Slack

[decentraland-bot]

[P2] When --is-ancestor is true (chore/sync already in experimental), the workflow exits without deleting the stale chore/sync branch. Consider:

Suggested change

	exit 0
	if git merge-base --is-ancestor origin/chore/sync origin/experimental; then
	echo "experimental already contains chore/sync; cleaning up."
	git push origin --delete chore/sync || true
	exit 0

This handles the partial-failure recovery case (prior run pushed experimental but failed on branch delete).

[decentraland-bot]

[P2] With fetch-depth: 0, actions/checkout@v4 already sets remote.origin.fetch to the wildcard refspec. This git config line is redundant (as noted by @charly-bg). The git fetch below is still useful for refreshing refs, but this config line can be removed.

[decentraland-bot]

[P2] Same as the sync workflow — git config --replace-all is redundant with fetch-depth: 0.