fix(git): prevent reverting remote commits#2632
Merged
Merged
Conversation
…ranch git_signed_commit built the new commit on the remote tip while taking file contents from the local staged tree. When the remote branch had advanced (e.g. a CI bot auto-committing regenerated artifacts onto an open PR), committing from a stale checkout silently reverted those commits. Add assertNotBehindRemote: after fetching an existing branch, fail closed when the remote tip has commits the local checkout lacks, with an actionable recovery message (stash -> fetch -> reset --hard -> stash pop). Mirrors the existing merge-in-progress and base-leak refusals. New and up-to-date branches are unaffected (one rev-parse + merge-base, no extra fetch). Also document the failure mode in the agent git-workflow system prompt and the tool description, and cover the regression with tests. Generated-By: PostHog Code Task-Id: d562b81f-a6b5-425c-9981-b155fb52b612
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/git/src/signed-commit.test.ts:263-284
**Redundant call + missed parameterisation opportunity**
The two `behindRemoteError` tests call the same pure function twice on slightly different inputs, which violates OnceAndOnlyOnce. The "stays generic" checks (absence of `"tests-posthog"` and `"OpenAPI"`) don't depend on a different input — they could run alongside the presence assertions on the same call. Collapsing to a single test, or using `it.each` if more combinations are ever added, would express the idea once.
### Issue 2 of 2
packages/git/src/signed-commit.test.ts:399-464
**Setup duplication across the last two `assertNotBehindRemote` tests**
The "no-op" and "local ahead" tests each hand-build their own bare-remote + agent fixture (~20 lines each) that reproduces almost exactly what `setupBranchAdvancedByBot` already does — just without the bot step. Extracting a `setupBaseRepo` helper (or passing an option to skip the bot) would satisfy the OnceAndOnlyOnce rule and make each test's intent clearer at a glance.
Reviews (1): Last reviewed commit: "fix(git): refuse signed commits that wou..." | Re-trigger Greptile |
Address greptile review nits: extract setupBaseRepo/setupPushedBranch helpers so the no-op and local-ahead tests stop re-building the bare-remote fixture, and fold the two behindRemoteError assertions onto a single call. Generated-By: PostHog Code Task-Id: d562b81f-a6b5-425c-9981-b155fb52b612
Use it.each for the present/absent substring checks (building the message once) instead of a single flat test, matching the file's existing parameterised style and the CLAUDE.md preference. Generated-By: PostHog Code Task-Id: d562b81f-a6b5-425c-9981-b155fb52b612
charlesvien
approved these changes
Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Agent didn't check if there were new commits on remote, possibly overwriting them in the subsequent commit (for example leading to silently dropping CI artifacts).
Changes
packages/git—assertNotBehindRemote: after fetching an existing branch, fail closed when the remote tip has commits the local checkout lacks. The error outputs the divergence and gives a work-preserving recovery (stash --include-untracked→fetch→reset --hard→stash pop). Mirrors the existing merge-in-progress / base-leak refusals; new and up-to-date branches are unaffected.packages/agent— documents the failure mode in the agent's git-workflow prompt and the tool description.Tests
Real-git + bare-remote fixture reproducing the bug (commit → simulated bot push → second commit from a stale checkout → refused), plus up-to-date, local-ahead, and no-op (staged work preserved) cases. 39/39 pass.