feat: block force pushes via pre-push hook - #410
Conversation
✅ Deploy Preview for commit-check ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements force-push detection and blocking via a new ChangesForce-Push Detection and Blocking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
==========================================
+ Coverage 95.51% 95.81% +0.29%
==========================================
Files 10 10
Lines 1004 1123 +119
==========================================
+ Hits 959 1076 +117
- Misses 45 47 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds a `check-no-force-push` pre-push hook that detects and blocks `git push --force` / `git push -f` by inspecting pushed ref ancestry via `git merge-base --is-ancestor`. ## Detection logic Reads git's pre-push stdin (<local ref> <local sha> <remote ref> <remote sha>) and evaluates: - Remote SHA is zero -> new branch push -> pass - merge-base returns 0 -> fast-forward -> pass - Returns 1 -> force push detected -> fail - Returns 128 -> git error, pass (safe default) ## Standalone mode When run without stdin, --no-force-push checks whether pushing HEAD to its configured upstream would require force, using git ls-remote and optional git fetch to resolve the remote commit. Closes #203
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@commit_check/engine.py`:
- Around line 565-583: The retry fetch logic in
_check_current_branch_against_upstream fails when get_upstream_remote_sha()
returns empty because target_ref == upstream_ref prevents fetch_on_128; change
the control so that when git_merge_base(target_ref, "HEAD") returns 128 you
always call fetch_upstream_ref(upstream_ref) and then re-run git_merge_base,
regardless of whether target_ref came from get_upstream_remote_sha; update the
block that checks returncode == 128 to remove the target_ref != upstream_ref
guard and ensure the retry path uses fetch_upstream_ref and reassigns returncode
before evaluating failure (use get_branch_name() and upstream_ref as before for
the failure message).
In `@docs/configuration.rst`:
- Around line 276-278: The table row for allow_force_push is inconsistent: it
currently maps `allow_force_push = true` to `CCHK_ALLOW_FORCE_PUSH=false` and
`--no-force-push`; update the env-var column to `CCHK_ALLOW_FORCE_PUSH=true` so
the value matches the TOML `allow_force_push = true` and the parity pattern, and
keep the `--no-force-push` flag as-is; move the guidance about "set false to
block" out of the value mapping into the descriptive text for `allow_force_push`
so the table only shows matching values.
In `@docs/example.rst`:
- Around line 156-176: The docs mention the check-no-force-push hook but omit
how to enable pre-push hooks; add a short instruction telling users to install
the pre-push hook type (for example by running pre-commit install --hook-type
pre-push) before configuring or relying on the check-no-force-push hook, and
update the Push Validation Examples section to include this installation step so
readers know to enable the pre-push hook type prior to using
check-no-force-push.
In `@README.rst`:
- Around line 207-210: The pre-commit config pins rev: v2.6.0 but declares the
hook id: check-no-force-push which was added after v2.6.0; update the rev value
in the same block (the lines containing rev: and id: check-no-force-push) to a
release that includes that hook (e.g., change rev: v2.6.0 to rev: v2.7.0 or a
newer compatible tag) so pre-commit can resolve the hook without errors.
In `@tests/api_test.py`:
- Line 273: The docstring for the validate_push() tests contains an ambiguous EN
DASH character – which triggers RUF002; open the docstring in tests/api_test.py
where the triple-quoted string reads "Tests for validate_push() – the
programmatic push safety API." and replace the EN DASH with a normal ASCII
hyphen so it reads "Tests for validate_push() - the programmatic push safety
API." to silence the lint warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e044e696-1acf-4479-ac62-104e6bfcec9e
📒 Files selected for processing (19)
.pre-commit-hooks.yamlREADME.rstcommit_check/__init__.pycommit_check/api.pycommit_check/config_merger.pycommit_check/engine.pycommit_check/main.pycommit_check/rule_builder.pycommit_check/rules_catalog.pycommit_check/util.pydocs/changelog.rstdocs/configuration.rstdocs/example.rstdocs/what-is-new.rsttests/api_test.pytests/engine_test.pytests/main_test.pytests/rule_builder_test.pytests/util_test.py
| def _check_current_branch_against_upstream(self) -> ValidationResult: | ||
| """Check whether pushing HEAD to its upstream would require force.""" | ||
| upstream_ref = get_upstream_branch() | ||
| if not upstream_ref: | ||
| return ValidationResult.PASS | ||
|
|
||
| target_ref = get_upstream_remote_sha(upstream_ref) or upstream_ref | ||
| returncode = git_merge_base(target_ref, "HEAD") | ||
| if ( | ||
| returncode == 128 | ||
| and target_ref != upstream_ref | ||
| and fetch_upstream_ref(upstream_ref) | ||
| ): | ||
| returncode = git_merge_base(target_ref, "HEAD") | ||
| if returncode == 1: | ||
| self._print_failure(f"{get_branch_name()} -> {upstream_ref}") | ||
| return ValidationResult.FAIL | ||
|
|
||
| return ValidationResult.PASS |
There was a problem hiding this comment.
Fetch retry skipped when remote SHA lookup fails.
When get_upstream_remote_sha() returns empty (e.g., no network, private repo), target_ref falls back to upstream_ref. If git_merge_base then returns 128 (ref unknown locally), the condition target_ref != upstream_ref is False, so fetch_upstream_ref is never called. This defeats the retry logic for shallow clones or CI environments where the upstream ref isn't available locally.
🐛 Proposed fix: fetch when returncode is 128 regardless of target_ref source
def _check_current_branch_against_upstream(self) -> ValidationResult:
"""Check whether pushing HEAD to its upstream would require force."""
upstream_ref = get_upstream_branch()
if not upstream_ref:
return ValidationResult.PASS
target_ref = get_upstream_remote_sha(upstream_ref) or upstream_ref
returncode = git_merge_base(target_ref, "HEAD")
- if (
- returncode == 128
- and target_ref != upstream_ref
- and fetch_upstream_ref(upstream_ref)
- ):
- returncode = git_merge_base(target_ref, "HEAD")
+ if returncode == 128 and fetch_upstream_ref(upstream_ref):
+ # Re-resolve target_ref after fetch if we didn't have a SHA
+ if target_ref == upstream_ref:
+ target_ref = get_upstream_remote_sha(upstream_ref) or upstream_ref
+ returncode = git_merge_base(target_ref, "HEAD")
if returncode == 1:
self._print_failure(f"{get_branch_name()} -> {upstream_ref}")
return ValidationResult.FAIL
return ValidationResult.PASS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@commit_check/engine.py` around lines 565 - 583, The retry fetch logic in
_check_current_branch_against_upstream fails when get_upstream_remote_sha()
returns empty because target_ref == upstream_ref prevents fetch_on_128; change
the control so that when git_merge_base(target_ref, "HEAD") returns 128 you
always call fetch_upstream_ref(upstream_ref) and then re-run git_merge_base,
regardless of whether target_ref came from get_upstream_remote_sha; update the
block that checks returncode == 128 to remove the target_ref != upstream_ref
guard and ensure the retry path uses fetch_upstream_ref and reassigns returncode
before evaluating failure (use get_branch_name() and upstream_ref as before for
the failure message).
| * - ``allow_force_push = true`` | ||
| - ``CCHK_ALLOW_FORCE_PUSH=false`` | ||
| - ``--no-force-push`` (enable via ``--no-force-push`` flag) |
There was a problem hiding this comment.
Fix inconsistent value mapping for allow_force_push across TOML and env var.
This row maps allow_force_push = true to CCHK_ALLOW_FORCE_PUSH=false, which conflicts with the rest of the table’s parity pattern and is likely to confuse users. Use matching values in the mapping row and move the “set false to block” guidance to description text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration.rst` around lines 276 - 278, The table row for
allow_force_push is inconsistent: it currently maps `allow_force_push = true` to
`CCHK_ALLOW_FORCE_PUSH=false` and `--no-force-push`; update the env-var column
to `CCHK_ALLOW_FORCE_PUSH=true` so the value matches the TOML `allow_force_push
= true` and the parity pattern, and keep the `--no-force-push` flag as-is; move
the guidance about "set false to block" out of the value mapping into the
descriptive text for `allow_force_push` so the table only shows matching values.
| Push Validation Examples | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| # Check whether pushing HEAD to its configured upstream would require force | ||
| commit-check --no-force-push | ||
|
|
||
| .. code-block:: yaml | ||
|
|
||
| # Configure the dedicated pre-push hook | ||
| - repo: https://github.com/commit-check/commit-check | ||
| rev: the tag or revision | ||
| hooks: | ||
| - id: check-no-force-push | ||
| stages: [pre-push] | ||
|
|
||
| ``git push | commit-check --no-force-push`` is not a prevention mechanism. The | ||
| push has already started, and normal ``git push`` output does not include the | ||
| pre-push ref lines that Git provides to hooks. | ||
|
|
There was a problem hiding this comment.
Add explicit pre-push hook installation step in this section.
This section introduces check-no-force-push, but it doesn’t mention that users must install the pre-push hook type (pre-commit install --hook-type pre-push). Without that, the hook won’t run.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/example.rst` around lines 156 - 176, The docs mention the
check-no-force-push hook but omit how to enable pre-push hooks; add a short
instruction telling users to install the pre-push hook type (for example by
running pre-commit install --hook-type pre-push) before configuring or relying
on the check-no-force-push hook, and update the Push Validation Examples section
to include this installation step so readers know to enable the pre-push hook
type prior to using check-no-force-push.
| rev: v2.6.0 | ||
| hooks: | ||
| - id: check-no-force-push | ||
| stages: [pre-push] |
There was a problem hiding this comment.
Use a release tag that actually contains check-no-force-push.
The snippet defines id: check-no-force-push but pins rev: v2.6.0. That version predates this hook, so copy/paste users can get pre-commit hook resolution errors. Update to the release that includes this feature (e.g., v2.7.0).
Suggested fix
- - repo: https://github.com/commit-check/commit-check
- rev: v2.6.0
+ - repo: https://github.com/commit-check/commit-check
+ rev: v2.7.0
hooks:
- id: check-no-force-push
stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.rst` around lines 207 - 210, The pre-commit config pins rev: v2.6.0
but declares the hook id: check-no-force-push which was added after v2.6.0;
update the rev value in the same block (the lines containing rev: and id:
check-no-force-push) to a release that includes that hook (e.g., change rev:
v2.6.0 to rev: v2.7.0 or a newer compatible tag) so pre-commit can resolve the
hook without errors.
|
|
||
|
|
||
| class TestValidatePush: | ||
| """Tests for validate_push() – the programmatic push safety API.""" |
There was a problem hiding this comment.
Replace ambiguous unicode dash in docstring.
Line 273 uses an EN DASH (–) that Ruff flags as ambiguous text (RUF002). Use a normal hyphen (-) to avoid lint noise/failures.
Suggested fix
-class TestValidatePush:
- """Tests for validate_push() – the programmatic push safety API."""
+class TestValidatePush:
+ """Tests for validate_push() - the programmatic push safety API."""🧰 Tools
🪛 Ruff (0.15.12)
[warning] 273-273: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/api_test.py` at line 273, The docstring for the validate_push() tests
contains an ambiguous EN DASH character – which triggers RUF002; open the
docstring in tests/api_test.py where the triple-quoted string reads "Tests for
validate_push() – the programmatic push safety API." and replace the EN DASH
with a normal ASCII hyphen so it reads "Tests for validate_push() - the
programmatic push safety API." to silence the lint warning.
|
|
Superseded by #412, which uses commit-check/commit-check:feat/block-force-pushes as the head branch instead of the fork branch. |
Summary
Adds a
check-no-force-pushpre-push hook that detects and blocksgit push --force/git push -fby inspecting pushed ref ancestry viagit merge-base --is-ancestor.Closes #203
Detection logic
Reads git's pre-push stdin (
<local ref> <local sha> <remote ref> <remote sha>) and evaluates:Standalone mode
When run outside a pre-push hook (no stdin),
--no-force-pushchecks whether pushing HEAD to its configured upstream would require force, usinggit ls-remoteand optionalgit fetchto resolve the remote commit.Usage
Config (
cchk.toml)CLI
pre-commit hook
Environment variable
export CCHK_ALLOW_FORCE_PUSH=falseChanges
commit_check/__init__.pyDEFAULT_PUSH_RULEScommit_check/rules_catalog.pyPUSH_RULEScommit_check/rule_builder.py_build_push_rules()commit_check/engine.pyForcePushValidatorcommit_check/config_merger.py[push]section supportcommit_check/main.py--no-force-pushCLI flagcommit_check/api.pyvalidate_push()APIcommit_check/util.py.pre-commit-hooks.yamlcheck-no-force-pushhooktests/Summary by CodeRabbit
Release Notes
New Features
--no-force-pushCLI flagcheck-no-force-pushpre-push hook for integration with pre-commitvalidate_push()Python API for programmatic push safety validationpush.allow_force_pushconfiguration option to control force push behaviorDocumentation