fix(oss-commit-sync): skip an export whose content OSS already holds

[sydorovdmytro]

The import checks external_is_benign before applying, with the reasoning in its comment: a commit whose post-image is already present "can only produce a no-op or a spurious conflict". The export had no equivalent — only the post-apply nothing_staged check, which never runs when git apply itself fails.

For this shape it does fail. A patch that deletes a path OSS no longer has cannot apply; there is nothing to 3-way against.

A release line lands in that state whenever its OSS branch is cut from the default branch after a change reached it, so the monorepo release branch carries a backport of that same change and the backport is the first thing replayed. On vcluster-pro v0.37 the anchor was #2321 on main, the first replayed commit was #2322 (its v0.37 backport), all 391 touched paths already matched its post-image, 47 of them deletions — and the run reported a conflict on a commit with nothing to contribute.

Closes DEVOPS-1545

Test plan

• Five new export.bats cases: already-mirrored commit skipped rather than called a conflict; a commit with something to add not skipped; a deletion OSS has not seen still mirrored; a mode-only change still mirrored; a rename decomposed and mirrored
• Mutation-tested individually: deletion-always-benign, comparing blobs instead of tree entries, and removing the pre-check each turn a specific test red. The over-skip guard passes either way by design
• Full suite 168/168, shellcheck clean
• Reproduced against the real repos: with this change the v0.37 export gets past 6a7b08b3 and proceeds. It then stops on a genuine pkg/platform/version.go conflict, which is a separate problem being handled by hand in chore: replay the pending v0.37 commits from vcluster-pro vcluster#4183

[loft-bot]

Panel review: 2 blocking, 5 quality notes, 8 lanes. Well-motivated fix with a real reproduction behind it; both blocking items were reproduced locally against the action's own bats fixture.

Blocking concerns

• .github/actions/oss-commit-sync/lib.sh:948-950 — the comparison reads blobs, not tree entries, so a mode-only commit is judged benign; it mirrored correctly at base and now hard-fails the run at the convergence assertion.
• .github/actions/oss-commit-sync/test/export.bats:1261 — the new helper's "deletion OSS still needs" branch is untested; a mutation making every deletion benign keeps all 52 cases green.

What was checked

Checked: correctness, architecture, reuse, test-quality, coverage gaps (analyzer plus independent verifier), operability, typos, PR metadata. Skipped: security (no injection sink, secret, authz or untrusted-input surface), dead-code (zero deletions in the diff), infra (no Terraform, Helm, manifest, workflow or Dockerfile), the four e2e lanes and entry-point fidelity (no e2e suite, and the linked ticket is an acceptance-criteria ticket rather than a customer bug), layout coherence (not a restructure-shaped diff). No sibling open PR touches this action, so no merge-order conflict.

Quality notes (non-blocking)

• consider — export.sh:435: $(git -C "$WT" rev-parse HEAD) sits in the if's tested command, so errexit is suspended and a failed read passes an empty argument; the helper then resolves ":${path}" against the monorepo index and answers benign for a delete-only commit (checked directly: rc 1 with the real tip, rc 0 with an empty argument). Unlikely to fire on a just-created worktree, but it contradicts the helper's own fail-closed comment — capture and check first, as the other producers here do.
• consider — export.bats:1261: this test stays green against base ea06a677 with the fix fully reverted, so it does not pin the change; the sibling at 1216 carries the pre-fix-red value. Worth correcting the PR body's "both verified red" claim. This becomes blocking if the sibling test is ever weakened or removed, leaving no red-without-the-fix guard.
• consider — export.bats:1216: the rename decomposition the helper's comment calls out is unexercised; re-adding -M to the diff-tree call leaves all 52 cases green while reproducing the original does not exist in index conflict. This becomes blocking if -M or rename handling in this helper is changed later.
• consider — export.sh:436: Skipping ... (content already on OSS) is a substring of the nothing_staged message below it, so neither the log nor the new test's assertion can tell the two skip paths apart. Wording unique to the pre-apply check would fix both.
• consider — lib.sh:929: monorepo_is_benign mirrors external_is_benign (lib.sh:547) closely enough that a future fix to one can easily miss the other — the mode issue above is an instance. A single parameterized helper taking the two (sha, prefix) pairs would keep them in step.

[loft-bot]

blocking — git rev-parse <rev>:<path> yields the blob, not the tree entry, so the file mode is outside this comparison: a mode-only commit (a chmod +x on a script under the prefix) matches on every path and is judged benign.

Reproduced on this suite's own fixture with npx bats@1.11.0, using one commit that only chmods the seeded pkg/app.go:

• at base ea06a677 it replays and pushes — OSS ends at 100755, exit 0;
• at this head it is skipped as content already on OSS, and the run then dies at the convergence assertion: ::error::OSS tree does not match the monorepo staging tree after replay: pkg/app.go | 0 → Re-run with align-tree=true, exit 1, OSS left at 100644.

So a case that mirrored correctly before this change becomes a hard export failure that blocks every later commit on the branch — the same stalled-mirror outage this PR exists to remove — and the only recovery offered is align-tree, which rewrites the whole tree. The helper's "a false benign cannot corrupt the mirror" reasoning does hold (nothing is corrupted), but here the assertion is the failure rather than a safety net behind one.

Comparing the tree entry keeps the mode in scope. Verified with exactly this patch: the mode-only commit replays again, and export.bats (52 cases) plus import.bats (26) stay green.

Suggested change

	blob_mono="$(git rev-parse --quiet --verify "${m}:${SUBTREE_PREFIX}/${path}" 2>/dev/null)" || return 1
	blob_oss="$(git rev-parse --quiet --verify "${oss}:${path}" 2>/dev/null)" || return 1
	[ "$blob_mono" = "$blob_oss" ] || return 1
	blob_mono="$(git ls-tree "$m" -- "${SUBTREE_PREFIX}/${path}")" || return 1
	blob_oss="$(git ls-tree "$oss" -- "$path")" || return 1
	[ -n "$blob_mono" ] && [ "${blob_mono%%$'\t'*}" = "${blob_oss%%$'\t'*}" ] || return 1

external_is_benign compares blobs the same way, but there the verdict only suppresses a guard complaint; here it decides whether a commit is replayed at all, so the two are not equivalent risks — worth fixing both if you touch the shared shape.

[loft-bot]

blocking — DEVOPS-1545's third criterion ("a commit that still has something to contribute is NOT skipped") is covered here only for the add/modify shape. The D-status branch is the one piece of bespoke logic in the new helper (git cat-file -e "${oss}:${path}" → not benign), and no spec drives a monorepo commit that deletes a path OSS still holds — git rm appears only in the sibling test at line 1216, where OSS has already lost the path.

Confirmed by mutation: replacing that branch's return 1 with a bare continue, so every deletion counts as benign, leaves all 52 export.bats cases green. A probe then shows the mutant skips the real deletion and dies later at the convergence assertion (OSS tree does not match the monorepo staging tree after replay: pkg/keep.go | 1 -), while this head replays it correctly.

Nothing is silently lost — the convergence assertion does catch it, so the ticket's claim on that point holds. But it converts every deletion-carrying export into a hard-failed run needing manual align-tree, which is the same outage class the ticket was filed for, and it would ship green.

Suggested case, mirroring the one at line 1216 but without the upstream deletion: export pkg/keep.go, git rm it in the monorepo only, then assert the run exits 0, the output does not contain content already on OSS, and git -C "$OSS_REMOTE" cat-file -e main:pkg/keep.go fails (the deletion was mirrored).

[sydorovdmytro]

Both blocking items were real and are fixed in f7e0742. I reproduced each before changing anything.

lib.sh:948 — mode-only commits judged benign. Confirmed: git rev-parse <rev>:<path> returns the same blob before and after a chmod +x, while ls-tree shows 100644 → 100755. The consequence you describe is the part that matters — a case that mirrored correctly at base becomes a hard export failure whose only offered recovery is align-tree, which is the outage class this PR exists to remove.

Took your point about fixing both. The comparison now lives in one benign_paths_match helper that both directions call with their own (rev, prefix) pair, so external_is_benign gets the same fix and the two cannot drift again. That also answers the duplication note at lib.sh:929.

export.bats:1261 — deletion branch untested. Correct, and your mutation is reproduced: replacing that return 1 with continue left every case green. Added the case you suggested, and it now fails under exactly that mutation.

On -M (export.bats:1216). Investigated and it turns out not to be a correctness risk here, so I did not add a test that would pin an irrelevant detail. -M yields R100\told\tnew; read -r status path takes path as the old name, whose source entry is empty, and the explicit [ -n "$entry_src" ] || return 1 guard then answers "not benign". So -M can only make the check more conservative, never wrongly benign. What was wrong was the comment promising more than the code enforced — I rewrote it to describe the guard that actually provides the property. The rename test stays, since it verifies end-to-end that a rename mirrors and the old path disappears.

export.sh:435 — errexit suspended in the tested condition. Fixed; the tip is read and checked first, and a failed read now dies rather than passing an empty rev. You were right that it contradicted the helper's own fail-closed comment.

export.sh:436 — indistinguishable skip messages. The pre-apply skip now reads already mirrored; OSS holds this commit's content, which shares no substring with the nothing_staged message, and the tests assert on that.

On the PR body's "both verified red". You were right to call that out, and I should have checked rather than asserted it. Only the first of the two pinned the change; the second guards against over-skipping and passes either way. I have since mutation-tested every new assertion individually — deletion-always-benign, blob-instead-of-entry, -M, and removing the pre-check entirely — and each of the first three plus the last turns a specific test red. The body is corrected.

171/171, shellcheck clean.