fix(NATSRS-010-2): CU-86akbh48z sync-upstream.yml exits 0 (success) instead of aborting when merge has conflicts, and force-pushes unconditionally - #115
Conversation
…rting when merge has conflicts, and force-pushes unconditionally
| fi | ||
| fi | ||
|
|
||
| git push -f origin "$BRANCH" |
There was a problem hiding this comment.
🦩 🔴 sync-upstream.yml exits 0 (success) instead of aborting when merge has conflicts, and force-pushes unconditionally
In the Sync step of the sync job (.github/workflows/sync-upstream.yml), changed the conflict path so that on merge failure the workflow now aborts the merge and exits with a non-zero status (exit 1) instead of silently exiting 0, making the job fail visibly. Additionally, before the unconditional git push -f origin "$BRANCH", added a divergence check: if the remote sync/upstream-main branch already exists, it is fetched and checked with git merge-base --is-ancestor against the freshly rebuilt local branch; if the remote branch is not an ancestor (i.e. it contains commits not in the newly computed merge, meaning a maintainer pushed manual fixups), the workflow errors out and refuses to force-push, preventing silent clobbering of manual work. This is a heuristic fastest-safe check based on ancestry rather than a full semantic diff of PR content, so there is residual risk: a maintainer amending/rebasing the branch without adding new distinguishable commits, or corner cases in git ref state on the runner, could still evade detection. A more complete fix might also compare against the exact PR head SHA via gh pr view, but this in-file change directly addresses both cited issues (unguarded exit 0 on conflict, unconditional force-push) with a minimal, self-contained mechanism.
🤖 Prompt for AI agents
In .github/workflows/sync-upstream.yml around line 45, review and complete this code-review fix: sync-upstream.yml exits 0 (success) instead of aborting when merge has conflicts, and force-pushes unconditionally.
What the draft fix changed: In the `Sync` step of the `sync` job (`.github/workflows/sync-upstream.yml`), changed the conflict path so that on merge failure the workflow now aborts the merge and exits with a non-zero status (`exit 1`) instead of silently exiting 0, making the job fail visibly. Additionally, before the unconditional `git push -f origin "$BRANCH"`, added a divergence check: if the remote `sync/upstream-main` branch already exists, it is fetched and checked with `git merge-base --is-ancestor` against the freshly rebuilt local branch; if the remote branch is not an ancestor (i.e. it contains commits not in the newly computed merge, meaning a maintainer pushed manual fixups), the workflow errors out and refuses to force-push, preventing silent clobbering of manual work. This is a heuristic fastest-safe check based on ancestry rather than a full semantic diff of PR content, so there is residual risk: a maintainer amending/rebasing the branch without adding new distinguishable commits, or corner cases in git ref state on the runner, could still evade detection. A more complete fix might also compare against the exact PR head SHA via `gh pr view`, but this in-file change directly addresses both cited issues (unguarded exit 0 on conflict, unconditional force-push) with a minimal, self-contained mechanism.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
Closes findings from rule NATSRS-010-2 — sync-upstream.yml exits 0 (success) instead of aborting when merge has conflicts, and force-pushes unconditionally.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Warning
This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.
.github/workflows/sync-upstream.yml:45What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
06b1d724-91f9-455e-9eef-4cda91351dbaMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akbh48z NATS async-nats and CI review findings (10 PRs)