Fix protected-paths guard to use merge-tree simulation (ADO 5347121) (microsoft-foundry#493)

brandom-msft · github-actions[bot] · Copilot · web-flow · commit 82ee65ffe469 · 2026-06-09T17:25:58.000-07:00
* Fix protected-paths guard to use merge-tree simulation (ADO 5347121) `guard_protected_paths()` in `.github/scripts/sync-core.sh` previously compared blobs at `sync_branch_tip:<path>` against `public_main:<path>`. Because `git fast-export --import-marks` combined with pathspec filters forces `--full-tree` mode, every emitted commit becomes a `deleteall` plus one `M` record per included file -- so excluded paths (`.github/`) are absent from the sync-branch-tip tree even when the eventual rebase-merge into public main would preserve them unchanged. The result: once a protected workflow file exists on public main, the guard hard-fails every incremental sync regardless of whether the merge would actually disturb the file. Scheduled sync has been paused since 2026-06-08. This change replaces the blob comparison with a `git merge-tree --write-tree` simulation. The guard now computes the prospective post-rebase-merge tree from public `origin/main` and the sync branch, then compares the public-main blob against the merged-tree blob for each protected path. Both topologies are correctly classified: * Normal incremental sync (`--import-marks` parent chain anchors to previous sync-import commits that also lacked `.github/`): the 3-way merge preserves `.github/` from public main, so the result tree has the unchanged blob and the guard passes. * Seed-marks recovery (parent chain anchors to public_sha that HAS `.github/`): the new sync commit's parent tree contains `.github/` while its own tree does not -- a real delete in the rebase-merge. The result tree omits the protected file and the guard correctly hard-fails with the existing "MISSING from sync branch" error. Non-zero exit from `merge-tree` is surfaced via a distinct conflict error so real prospective-merge conflicts are not conflated with protected-paths violations. For the synthetic true-orphan case (no common ancestor; only test fixtures hit this in practice), the guard falls back to the sync-branch tree directly, preserving orphan-wipe detection -- explicitly NOT using `--allow-unrelated-histories`, which empirically produces a UNION tree that would silently false-pass orphan wipes. Requires git >= 2.38 for `merge-tree --write-tree`. GitHub-hosted CI runners (ubuntu-latest) are at 2.43+. The guard validates the git version up front and fails loudly with a useful message if it is too old. GitHub's actual sync-PR merge strategy (`gh pr merge --rebase` with `--squash` fallback) produces the same protected-path blob set as a 3-way merge from `merge-tree`'s perspective, so the simulation is sound for either path. Tests (from PR microsoft-foundry#492 which landed the failing reproducer): T64-T68 guard unit tests on synthesized branches PASS (unchanged) T69 normal incremental + protected file RED -> GREEN T70 seed-recovery wipe (regression coverage) PASS (unchanged) Local test summary (WSL): Tests run: 75 | Passed: 66 | Failed: 9. The 9 failures (T29, T31, T32, T35, T36, T48, T50, T54, T55) are pre-existing environment-only issues (getcwd quirks, missing `jq`, verify-sync drift detection on the local fixture) that reproduce identically on `main` without this change. CI runners pass cleanly. Docs: `docs/repo-sync-automation.md` -- the "Known issue: guard incompatible with `fast-export --import-marks` + pathspec" section is removed; the Protected-paths guard intro and False-positive risk subsection are rewritten to describe the new merge-tree semantics; a 2026-06-09 Changelog entry records the fix. The 2026-06-08 historical entry is updated only to drop a now-dead anchor link. Re-enabling the scheduled cron in `.github/workflows/sync-to-public.yml` is tracked separately as ADO 5347122 (post-CI-confirmation follow-up). Bug: https://msdata.visualstudio.com/Vienna/_workitems/edit/5347121 Repro PR (tests-first): microsoft-foundry/foundry-samples-pr#492 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify divergent-content error message: "prospective merge result blob" The error message previously said "vs sync branch blob" but the blob being shown is from the prospective post-rebase-merge tree (`result_blob`), not the sync-branch-tip tree. Rephrase to match actual semantics and add an inline comment explaining the distinction. T67 only greps for the substring `divergent content` so the rename is safe. Addresses microsoft-foundry/foundry-samples-pr#493 (review). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
diff --git a/.github/scripts/sync-core.sh b/.github/scripts/sync-core.sh
@@ -271,11 +271,80 @@ guard_protected_paths() {
         return 1
     fi
 
+    # Compute the prospective post-rebase-merge tree (ADO 5347121). We do NOT
+    # compare blobs at the sync-branch tip directly: `git fast-export
+    # --import-marks` + pathspec forces `--full-tree` mode, so the sync
+    # branch's tree never contains excluded paths (`.github/`, etc.) — even
+    # when the eventual rebase-merge into public main would preserve them
+    # unchanged. Reading sync-branch-tip blobs as ground truth therefore
+    # hard-fails every incremental sync once a protected file exists on
+    # public main. Instead, we simulate the merge GitHub will perform and
+    # check protected paths against the resulting tree.
+    #
+    # GitHub merges sync PRs via `gh pr merge --rebase` (with `--squash`
+    # fallback). For the purpose of "which blobs end up on main", both
+    # strategies produce the same result as a 3-way merge from `merge-tree`'s
+    # perspective; the simulation is sound for either path.
+    #
+    # `git merge-tree --write-tree` (git >= 2.38) computes the merged tree
+    # OID without touching refs or the working tree — safe for dry-run.
+
+    local git_version_line git_major git_minor
+    git_version_line=$(git --version)
+    git_major=$(printf '%s' "$git_version_line" | sed -E 's/^git version ([0-9]+)\.([0-9]+).*/\1/')
+    git_minor=$(printf '%s' "$git_version_line" | sed -E 's/^git version ([0-9]+)\.([0-9]+).*/\2/')
+    if ! [[ "$git_major" =~ ^[0-9]+$ ]] || ! [[ "$git_minor" =~ ^[0-9]+$ ]]; then
+        log "ERROR: Protected-paths guard: could not parse git version from '$git_version_line'"
+        return 1
+    fi
+    if (( git_major < 2 )) || { (( git_major == 2 )) && (( git_minor < 38 )); }; then
+        log "ERROR: Protected-paths guard requires git >= 2.38 (for 'merge-tree --write-tree'); found $git_version_line"
+        return 1
+    fi
+
+    # Decide the prospective merge tree. In real sync topology, sync_branch
+    # always anchors back to a previous public state via --import-marks, so
+    # the merge base exists and merge-tree gives us the post-rebase tree.
+    # For a true orphan sync branch (no common ancestor — only synthetic test
+    # fixtures hit this in practice), fall back to the head tree directly:
+    # an orphan rebase-merge would replace public main wholesale, so the head
+    # tree IS the prospective result. Note: do NOT pass
+    # `--allow-unrelated-histories` to merge-tree — with an empty merge base
+    # it produces a UNION tree that would preserve protected files from base
+    # and silently false-pass orphan wipes.
+    local result_tree merge_output
+    if git -C "$PUBLIC_REPO" merge-base "$base_ref" "$head_ref" >/dev/null 2>&1; then
+        # Capture inside `if !` so `set -e` doesn't terminate the script on
+        # non-zero exit before our distinct conflict-error path runs.
+        if ! merge_output=$(git -C "$PUBLIC_REPO" merge-tree --write-tree "$base_ref" "$head_ref" 2>&1); then
+            log "ERROR: Protected-paths guard: prospective merge of $head_ref into $base_ref has conflicts or 'git merge-tree' failed. This is NOT a protected-paths violation — the sync cannot proceed because the eventual rebase-merge would not apply cleanly."
+            log "merge-tree output:"
+            while IFS= read -r line; do
+                log "  $line"
+            done <<< "$merge_output"
+            return 1
+        fi
+        # On success, stdout is exactly the result-tree OID on the first line.
+        result_tree=$(printf '%s\n' "$merge_output" | head -n 1)
+    else
+        log "Protected-paths guard: no common ancestor between $base_ref and $head_ref — treating sync branch tree as the prospective merge result (orphan-rebase semantics)"
+        result_tree=$(git -C "$PUBLIC_REPO" rev-parse --verify "$head_ref^{tree}" 2>/dev/null || echo "")
+        if [[ -z "$result_tree" ]]; then
+            log "ERROR: Protected-paths guard: could not resolve tree for $head_ref"
+            return 1
+        fi
+    fi
+
+    if ! git -C "$PUBLIC_REPO" rev-parse --verify "${result_tree}^{tree}" >/dev/null 2>&1; then
+        log "ERROR: Protected-paths guard: 'git merge-tree --write-tree' returned an unexpected value: '$result_tree'"
+        return 1
+    fi
+
     local failed=0
-    local path base_blob head_blob
+    local path base_blob result_blob
     for path in "${protected_paths[@]}"; do
         base_blob=$(git -C "$PUBLIC_REPO" rev-parse --verify "$base_ref:$path" 2>/dev/null || echo "")
-        head_blob=$(git -C "$PUBLIC_REPO" rev-parse --verify "$head_ref:$path" 2>/dev/null || echo "")
+        result_blob=$(git -C "$PUBLIC_REPO" rev-parse --verify "$result_tree:$path" 2>/dev/null || echo "")
 
         if [[ -z "$base_blob" ]]; then
             # Path is not on public main. Two interpretations: (a) it was
@@ -287,14 +356,21 @@ guard_protected_paths() {
             continue
         fi
 
-        if [[ -z "$head_blob" ]]; then
+        if [[ -z "$result_blob" ]]; then
+            # Phrased "MISSING from sync branch" for historical continuity
+            # (tests grep this string); semantically the path is missing from
+            # the prospective post-rebase-merge tree, which means a real
+            # rebase-merge of this sync branch would delete the protected file.
             log "ERROR: Protected-paths guard: $path is on public main (blob ${base_blob:0:8}) but MISSING from sync branch $SYNC_BRANCH"
             failed=1
             continue
         fi
 
-        if [[ "$base_blob" != "$head_blob" ]]; then
-            log "ERROR: Protected-paths guard: $path has divergent content — public main blob ${base_blob:0:8} vs sync branch blob ${head_blob:0:8}"
+        if [[ "$base_blob" != "$result_blob" ]]; then
+            # result_blob is the blob in the prospective post-rebase-merge tree
+            # (not the sync-branch-tip tree), which is what would actually land
+            # on public main after `gh pr merge --rebase`.
+            log "ERROR: Protected-paths guard: $path has divergent content — public main blob ${base_blob:0:8} vs prospective merge result blob ${result_blob:0:8}"
             failed=1
             continue
         fi
@@ -323,7 +399,7 @@ guard_protected_paths() {
         return 1
     fi
 
-    log "Protected-paths guard passed (${#protected_paths[@]} path(s) checked against $base_ref)"
+    log "Protected-paths guard passed (${#protected_paths[@]} path(s) checked against prospective merge of $head_ref into $base_ref)"
     return 0
 }
 
diff --git a/docs/repo-sync-automation.md b/docs/repo-sync-automation.md
@@ -230,8 +230,23 @@ ancestry, the guard fires before push and blocks the wipe.
 `sync-core.sh` runs a final invariant check after the sync branch is fully
 built (post-overlay, post-CODEOWNERS) and before emitting `has_changes=true`.
 For every path listed in `sync-config.json`'s `protected_paths` array, it
-compares the blob SHA on fresh `origin/main` against the blob SHA on the
-sync branch. Any deletion or content drift hard-fails the sync.
+simulates the prospective rebase-merge of the sync branch into public main
+(via `git merge-tree --write-tree`, requires git ≥ 2.38) and compares the
+blob SHA on fresh `origin/main` against the blob SHA on the *resulting*
+merged tree. Any deletion or content drift in the post-merge result
+hard-fails the sync.
+
+Why simulate the merge instead of inspecting the sync-branch tip directly:
+`git fast-export --import-marks` combined with pathspec filters forces
+`--full-tree` mode, so the sync branch's tree never contains files outside
+the include-set (e.g., `.github/`) — even when the eventual rebase-merge
+into public main would preserve those files unchanged. A blob comparison
+against the sync-branch tip would therefore hard-fail every incremental
+sync once a protected file exists on public main, regardless of whether
+the merge would actually disturb it. `merge-tree --write-tree` lets us
+inspect the post-rebase-merge tree itself, which is what GitHub will
+produce when the sync PR auto-merges. See ADO 5347121 for the full
+write-up.
 
 ### What's protected (as of 2026-06)
 
@@ -270,14 +285,18 @@ guard failure](#operator-recovery-from-a-guard-failure)).
 
 ### False-positive risk
 
-The guard fires whenever the sync branch's tree disagrees with public main
-on a protected path. The intended trigger is "orphan branch produced by
-discard-and-full-reexport." A secondary trigger: a human PR has modified
-the protected workflow on public main *after* this sync run's marks
-anchored. In that case the sync branch carries the older blob from the
-prior anchor, which the guard reads as drift. Recovery is the same
-seed-marks dispatch — re-anchoring the marks to current public HEAD picks
-up the human's edit.
+The guard inspects the prospective post-rebase-merge tree, not the
+sync-branch tip directly, so it correctly distinguishes "the eventual
+rebase-merge would actually delete or modify this protected path" from
+"the tree representations differ but the rebase-merge would preserve the
+file unchanged" (the topology produced by `fast-export --import-marks` +
+pathspec filtering on every incremental sync). The intended trigger is
+"orphan-style sync branch whose prospective merge tree would wipe a
+protected file." Secondary triggers — a human PR modifying a protected
+workflow on public main after this run's marks anchored, or a seed-marks
+recovery whose post-merge tree drops the file — are real wipes, not false
+positives. Recovery for either is the seed-marks dispatch documented
+below.
 
 ### Operator recovery from a guard failure
 
@@ -305,53 +324,6 @@ for completeness:
 See [Graft Synthesis Recovery](#graft-synthesis-recovery) for the deeper
 mechanics of `seed-marks-from-public.sh`.
 
-### Known issue: guard incompatible with `fast-export --import-marks` + pathspec
-
-**Status:** open as of 2026-06-08; scheduled sync paused.
-
-`git fast-export` source: when `--import-marks` and pathspec filters are
-both supplied, fast-export forces `--full-tree` mode. Each emitted commit
-becomes `from :parent` + `deleteall` + a full `M` record for every file
-in the filtered include-set. `fast-import` then constructs each new
-commit's tree solely from those `M` records — files OUTSIDE the
-include-set (e.g., everything under `.github/`) are absent from the
-sync-branch-tip tree even though they're present on public main.
-
-The guard's invariant ("sync branch tip blob must equal public main blob
-for every protected path") therefore cannot hold once protected files
-exist on public main, regardless of whether the prospective merge would
-actually delete them. In:
-
-- **Normal incremental sync:** `public.marks` from the cache anchors the
-  parent chain to previous sync-import commits that also lacked
-  `.github/`. The diff (`parent.tree → commit.tree`) shows no change to
-  `.github/`, so a rebase-merge of the sync PR preserves protected files
-  on public main. The guard's blob comparison flags this as drift, but
-  the actual merge is safe.
-- **Seed-marks recovery:** all marks map to current public main, which
-  HAS protected files. The first imported commit's parent tree includes
-  `.github/` while its own tree omits it — so the commit really
-  represents a delete, and a rebase-merge would wipe protected files.
-  Here the guard is correct, but it cannot distinguish the two
-  topologies because it inspects the sync-branch-tip tree, not the
-  prospective post-integration tree.
-
-**Proper fix (pending):** replace the blob comparison with a
-`git merge-tree --write-tree base_ref head_ref` simulation. Run the
-protected-path check against the *resulting* tree, not the head tree.
-This correctly classifies both topologies and lets normal incremental
-syncs pass while still catching seed-recovery wipes. Add integration
-tests covering the real fast-export/fast-import + pathspec + marks
-behavior (T64-T68 synthesize branch states and do not exercise the
-fast-import pipeline).
-
-**Workaround in place:** scheduled cron in `sync-to-public.yml` is
-commented out. Sync runs only via `workflow_dispatch` (and only when the
-operator has manually unblocked the protected paths via a seed-marks
-dispatch immediately preceded by a human PR that restores the path on
-public main). When the proper fix lands, uncomment the cron and re-test
-both topologies.
-
 ## Public→private mirror-back
 
 The public repo contains a public-overlay workflow at `.github/workflows/mirror-back.yml`.
@@ -612,7 +584,8 @@ Rollback affects public content. It does not rewrite private validation statuses
 
 | Date | Change |
 |------|--------|
-| 2026-06-08 | **Scheduled sync paused.** Commented out cron in `sync-to-public.yml` and documented a newly-discovered architectural bug in the protected-paths guard from PR #463. `git fast-export --import-marks` + pathspec filters force `--full-tree` mode, so the sync branch tip's tree never contains `.github/` entries; the guard's blob comparison cannot pass once protected files exist on public main. See [Known issue: guard incompatible with `fast-export --import-marks` + pathspec](#known-issue-guard-incompatible-with-fast-export---import-marks--pathspec). Proper fix (pending): replace blob comparison with `git merge-tree` simulation. Sync runs only via `workflow_dispatch` until the fix lands. |
+| 2026-06-09 | **Protected-paths guard fixed (ADO 5347121).** Replaced the sync-branch-tip blob comparison in `guard_protected_paths()` with a `git merge-tree --write-tree` simulation against the prospective post-rebase-merge tree. Normal incremental syncs (`fast-export --import-marks` + pathspec topology) now pass cleanly while seed-marks-recovery wipes still hard-fail. Requires git ≥ 2.38; CI runners are 2.43+. Test T69 (added in PR #492 as a RED reproducer) flips green; T70 (seed-marks wipe regression coverage) stays green. Re-enabling the scheduled cron is tracked separately as ADO 5347122. |
+| 2026-06-08 | **Scheduled sync paused.** Commented out cron in `sync-to-public.yml` and documented a newly-discovered architectural bug in the protected-paths guard from PR #463. `git fast-export --import-marks` + pathspec filters force `--full-tree` mode, so the sync branch tip's tree never contains `.github/` entries; the guard's blob comparison cannot pass once protected files exist on public main. Fix tracked as ADO 5347121; sync runs only via `workflow_dispatch` until the fix lands. |
 | 2026-06-08 | Added `seed_blocked_paths` `workflow_dispatch` input to `sync-to-public.yml` so the seed-marks tree-equivalence check can be run against a historical block-list. Exposed by the post-PR-#463 recovery: morning's scheduled sync wrote marks at a public SHA produced under a large validation block-list; later auto-recovery refused tree-equivalence on those historically-blocked paths. See [Historical block-list mismatch](#historical-block-list-mismatch). |
 | 2026-06-08 | Added protected-paths guard to `sync-core.sh` (PR #463). Public-only workflow files (`redirect-pull-requests.yml`, `mirror-back.yml`, `run-setup.yml`) are now listed in `sync-config.json`'s `protected_paths`; sync runs hard-fail if the sync branch would delete or modify them on public main. Backstops orphan-recovery wipes (the 2026-06 incidents that motivated this). See [Protected-paths guard](#protected-paths-guard). |
 | 2026-05-04 | Implemented Phase D4 + D4b atomically in PR-B: `compute-blocklist.sh` (shared entry point), `sync-to-public.yml` calls it before sync and passes `SYNC_BLOCKED_PATHS` into `sync-core.sh`, `verify-sync.yml` independently calls it and passes the same env into `verify-sync.sh`. Added `bypass_samples` / `bypass_reason` / `bypass_gate` workflow_dispatch inputs with mandatory loud surfacing (run-summary banner, PR body footer, auto-comment on `vars.BYPASS_LOG_ISSUE_NUMBER`). |
