feat(pipeline): add configurable pre_push_check before updating an existing PR branch - #865
feat(pipeline): add configurable pre_push_check before updating an existing PR branch#865nbost130 wants to merge 4 commits into
Conversation
…isting PR branch The push step had no notion that a pull request might already be owned by an external merge process. When a gate round produces a fix, rebase, or lint commit for a branch whose PR is currently held by a merge queue, a batching merge bot, or a release train, the push changes the PR head SHA and invalidates whatever that process has in flight. Where such a process batches several approved PRs onto one scratch branch, a single new commit throws away the CI cycle for every PR in the batch, not just the one that moved. no-mistakes cannot know which repositories have such a process or how it signals a hold, so this adds the seam rather than the knowledge: a new repo-level `pre_push_check` shell command, run immediately before the push step moves a branch that ALREADY exists on the push remote. A non-zero exit refuses the push before any object moves and reports the pull request, the commit range, the exit code, and the check's own output. Scope is deliberately narrow. The hook does not run when the field is unset (no subprocess, no forge lookup, no behavior change), when the push creates the remote branch for the first time (nothing can own a branch that does not exist yet, so opening a brand-new PR is never gated), or when the remote already points at the pushed head. The command receives the decision context in the environment: NO_MISTAKES_PR_URL, NO_MISTAKES_PR_NUMBER, NO_MISTAKES_REF, NO_MISTAKES_BRANCH, NO_MISTAKES_BASE_BRANCH, NO_MISTAKES_HEAD_SHA, and NO_MISTAKES_REMOTE_SHA. The PR identity comes from the run's recorded PR when it has one and otherwise from a best-effort forge lookup by branch, so it is available on the first push of a run against an already-open PR. A forge that is unreachable, unauthenticated, or unsupported leaves those two variables empty rather than silently disabling a guard the repository asked for. `pre_push_check` is honored only from the trusted default-branch copy of .no-mistakes.yaml, regardless of allow_repo_commands, for two independent reasons: it runs shell on the daemon host like commands.* do, and it is the guard standing in front of the very push a contributor's branch is asking for, so a pushed branch must be able neither to inject it nor to delete it.
Confidence Score: 4/5The PR is not yet safe to merge because restarted pushes on non-GitHub providers can still give pre_push_check a stale PR target branch. A CI auto-fix restart reruns Push with Run.PRURL already populated; the durable-URL fast path bypasses FindPR, and the affected providers cannot refresh the base through PRBaseBranchReader, leaving the configured fallback in NO_MISTAKES_BASE_BRANCH. Files Needing Attention: internal/pipeline/steps/prepush.go Reviews (3): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if sctx.Run != nil && sctx.Run.PRURL != nil { | ||
| if recorded := strings.TrimSpace(*sctx.Run.PRURL); recorded != "" { | ||
| return recorded, prNumberFromURL(recorded), "" | ||
| } | ||
| } |
There was a problem hiding this comment.
When a CI auto-fix restarts the pipeline on Gitea, Forgejo, GitLab, Bitbucket, or Azure DevOps, the recorded Run.PRURL makes this branch return before FindPR can obtain the live base, while livePRBaseBranch cannot refresh it on those providers. The check therefore receives the configured fallback in NO_MISTAKES_BASE_BRANCH, causing target-specific policy to incorrectly allow or block the push after a PR is retargeted.
|
Speaking as Kun's firstmate: first-time fork CI is approved after reviewing the diff. This is opt-in ( Not merging yet — waiting on you, not the captain. Greptile still has an open P1: on non-GitHub forges, a CI auto-fix restart with |
2 participants
What Changed
pre_push_checkrepo config field: an optional shell command the Push step runs immediately before moving an already-existing remote branch, so a repository can veto a push that would land underneath a branch an external merge process (merge queue, batching bot, release train) already owns. The check is skipped for new-branch creation and no-op (up-to-date) pushes, receivesNO_MISTAKES_REF/BRANCH/BASE_BRANCH/HEAD_SHA/REMOTE_SHA/PR_URL/PR_NUMBERenv vars (PR identity resolved from the run record or the forge, best-effort), and a non-zero exit refuses the push with aprePushCheckBlockedErrorwhose output is clamped and redacted (credential URLs, home paths) before being embedded in the error.PrePushCheckthroughRepoConfig, its YAML unmarshaling,Config,EffectiveRepoConfig(trusted-default-branch-only regardless ofallow_repo_commands), andMerge.common_exec.go'smergeEnvto extract a reusableoverrideEnv(base, extra)helper (applies overrides onto an arbitrary base env slice, not justos.Environ()), used to layer the pre-push decision context onto the step's existing environment.AGENTS.md,docs/.../environment.md,docs/.../pipeline-steps.md, anddocs/.../repo-config.mdto document the new field and its trust/security semantics.Risk Assessment
✅ Low: The change is additive and opt-in (empty pre_push_check is a complete no-op), correctly follows the repo's established trusted-default-branch-only security pattern for gate-control config fields, runs strictly before any object moves in the push step, reuses existing fork-aware PR-lookup and env-merge helpers rather than reimplementing them, and is covered by real subprocess/git-fixture tests exercising both the block and pass paths plus the skip matrix.
Testing
This is a backend CLI/pipeline feature (configurable pre_push_check shell hook before updating an existing PR branch) with no UI surface, so I validated it with the existing targeted test suite, which is genuinely product-level rather than mock-based: it spins up a real bare git repo as the push remote, configures pre_push_check to a real shell script, and asserts via git rev-parse/git ls-remote against that real remote that a refusing check (exit 3) leaves the remote branch and DB push-state completely untouched while a passing check (exit 0) lets the push land, that the check observes the correct pre-push remote SHA (proving true pre-push ordering), that the correct NO_MISTAKES_* env vars (PR URL/number, ref, branch, base branch, head/remote SHA) are passed, and that first-time branch publication and already-up-to-date pushes correctly skip the check. I also reran the config trust-boundary test (pushed vs trusted pre_push_check resolution through EffectiveRepoConfig/Merge, including allow_repo_commands and no-trusted-copy cases) and the full neighboring force-push/push regression set to confirm the new call site and the env-merge refactor introduced no regressions. All tests passed; no issues found.
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
go build ./...go test ./internal/pipeline/steps/... -run 'PrePush|PRNumberFromURL' -vgo test ./internal/config/... -run 'PrePushCheck' -vgo test ./internal/pipeline/steps/... -run 'TestOverrideEnv|TestMergeEnv|Env' -vgo test ./internal/pipeline/steps/... -run 'TestPushStep|TestForcePush|TestResolveForcePushDecision' -v✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.