Skip to content

fix: resolve merge-base refs that exist only on the remote - #532

Merged
shenxianpeng merged 6 commits into
mainfrom
claude/fix-merge-base-602anc
Aug 7, 2026
Merged

fix: resolve merge-base refs that exist only on the remote#532
shenxianpeng merged 6 commits into
mainfrom
claude/fix-merge-base-602anc

Conversation

@shenxianpeng

@shenxianpeng shenxianpeng commented Aug 6, 2026

Copy link
Copy Markdown
Member

Why this exists

Turning on the commit-check workflow in #531 made CC202 fail immediately — Current branch is not rebased onto target branch — on a branch that was correctly based on main the whole time. The failure reproduces in every CI checkout of a pull request, so it was split out of #531 per review: this is an engine behavior change affecting all users, and #531 stays CI-only.

The bug — three ways one mistake shows up

Git exits 128 when it cannot resolve a name. Both call sites in MergeBaseValidator read any non-zero as "not an ancestor".

The target. _find_target_branch verifies refs/heads/main, falls back to verifying refs/remotes/origin/main, then returns the bare name either way. A PR checkout has only the remote-tracking ref, so the caller ran:

git merge-base --is-ancestor main HEAD
fatal: Not a valid object name main   (exit 128)

Measured in a clone shaped like the runner's checkout:

_find_target_branch('main')           -> 'main'
git_merge_base('main', 'HEAD')        -> 128    <- reported as "not rebased"
git_merge_base('origin/main', 'HEAD') -> 0      <- the truth

The remote fallback now returns origin/ — the ref it just verified. Note require_rebase_target = "origin/main" in config is not a workaround: the resolver tries refs/heads/origin/main and refs/remotes/origin/origin/main, finds neither, returns None, and the check silently passes without checking anything.

The branch. get_branch_name() falls back to GITHUB_HEAD_REF, so a detached CI checkout reports a branch name that exists on no local ref. Same 128, same misreading. An unresolvable branch now resolves through origin/ first.

The last resort. Order matters here, and two rounds of review each caught a false pass in it. On a pull_request event HEAD is GitHub's synthetic merge commit, whose first parent is the target tip — so answering from HEAD passes every branch, rebased or not. That is why origin/<branch> must be tried before HEAD; and when the remote ref is missing too, HEAD still cannot be trusted. Its second parent is the pull request head, the commit actually under review. Measured in that shape on a genuinely diverged branch:

git_merge_base('origin/main', 'feat/work')        -> 128
git_merge_base('origin/main', 'origin/feat/work') -> 128
git_merge_base('origin/main', 'HEAD')             -> 0    <- false pass
git_merge_base('origin/main', 'HEAD^2')           -> 1    <- the truth

So the fallback asks about HEAD^2 whenever HEAD is a merge, which answers rather than giving up. Where HEAD has a single parent it is the branch commit itself (push event, or a branch never pushed) and answers for itself. A real ancestry failure (exit 1) fails at every step.

Why no test ever caught it

None of the existing merge-base tests ran the code they named:

  • Two patched commit_check.util.git_merge_base, but the engine imports the name directly — the mock never bound and real git ran against whatever checkout pytest happened to be in. One passed only because real git returned 128 and 128 was misread as FAIL — the very confusion this PR removes.
  • A third built its rule with no regex, so validate() returned PASS at the "no target" exit. The patched function's call count was 0.

All three now patch/assert against the engine's own reference, and four integration tests drive real git in PR-shaped clones: remote-only target (PASS), detached HEAD with no ref anywhere (PASS), a diverged branch on a merge-ref checkout (FAIL), and a diverged branch with no remote ref at all (FAIL) — the last two pin the fallback order and the HEAD^2 step. Reverting any fix fails its test; verified by restoring the plain HEAD fallback and watching the new test go red.

CodSpeed note — the flagged "regression" is the fix working

CodSpeed flags test_merge_base_validator_valid at 3 ms → 4.5 ms. That benchmark was measuring a test that did nothing: with no regex configured, validate() returned at the early exit and the mocked git_merge_base was never called. The test now exercises the path it is named after, and the extra 1.5 ms is that work. Making the benchmark fast again would mean making the test vacuous again — please acknowledge it on CodSpeed rather than expecting a code change. The same run reports two improvements (test_empty_message_passes ×4, test_merge_base_validator_invalid +20%): those are the tests whose mocks now actually bind, so they stopped fork-ing real git. CodSpeed also flags "different runtime environments detected" for this comparison.

After merge

Once released and picked up by commit-check-action, CC202 on #531 (and every downstream PR checkout) stops falsely failing — and starts genuinely failing branches that are not rebased, which it has never done in CI before.

🤖 Generated with Claude Code

https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn

CC202 reported "not rebased onto target branch" for a branch that was
correctly based on main the whole time — in every CI checkout of a pull
request. Two halves, same mistake: exit 128 from git means "could not
resolve that name", and both call sites read it as "not an ancestor".

_find_target_branch verifies refs/heads/main, falls back to verifying
refs/remotes/origin/main, then returns the bare name either way. A pull
request checkout has only the remote-tracking ref, so the caller ran

    git merge-base --is-ancestor main HEAD
    fatal: Not a valid object name main   (exit 128)

and the failure was reported as a rebase problem. The remote fallback now
returns origin/<name> — the ref that was just verified. Note that writing
require_rebase_target = "origin/main" in config is not a workaround:
_find_target_branch tries refs/heads/origin/main and
refs/remotes/origin/origin/main, finds neither, returns None, and the
check silently passes without checking anything.

The second half: get_branch_name() falls back to GITHUB_HEAD_REF, so a
detached CI checkout reports a branch name that exists on no local ref.
Same 128, same misreading. validate() now retries against HEAD — the same
commit, always resolvable — and only a real non-zero ancestry answer
fails.

Measured in a clone shaped like the runner's checkout:

    _find_target_branch('main')           -> 'main'      (before fix)
    git_merge_base('main', 'HEAD')        -> 128
    git_merge_base('origin/main', 'HEAD') -> 0

The existing tests never caught this because none of them ran the code
they named: two patched commit_check.util.git_merge_base while the engine
imports the name directly, so the mock never bound and real git ran
against whatever checkout pytest was in — one of them passed only because
128 was misread as FAIL. A third built its rule with no regex, so
validate() returned PASS before reaching the mocked call (call count: 0).
All three now assert against the engine's own reference, and two new
tests drive real git in pull-request-shaped clones. Reverting either fix
fails its test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
@shenxianpeng
shenxianpeng requested a review from a team as a code owner August 6, 2026 15:08
@github-actions github-actions Bot added the bug Something isn't working label Aug 6, 2026
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The engine now resolves missing target branches through qualified remote references. Merge-base validation retries with the remote current branch and then HEAD. Tests cover local, detached, rebased, and divergent pull-request checkouts.

Changes

Merge-base resolution

Layer / File(s) Summary
Qualified target branch resolution
commit_check/engine.py, tests/engine_test.py
_find_target_branch returns origin/<branch> when only the remote target branch exists. Tests assert the qualified reference.
Merge-base fallback validation
commit_check/engine.py, tests/engine_test.py
Merge-base checks retry with the remote-tracking current branch and then HEAD. Unit and integration tests cover detached, missing-local-branch, rebased, and divergent checkout states.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: tests

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: resolving merge-base references that exist only on the remote.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-merge-base-602anc

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The build job died before reaching any code: GitHub's runner could not
download its own actions ("Failed to resolve action download info.
Error: Service Unavailable", three attempts). The workflow token is
read-only, so a re-run cannot be requested through the API — an empty
commit re-triggers everything and disappears in the squash merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.71%. Comparing base (7315edf) to head (b5a5577).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
+ Coverage   97.61%   97.71%   +0.09%     
==========================================
  Files          12       12              
  Lines        1258     1269      +11     
==========================================
+ Hits         1228     1240      +12     
+ Misses         30       29       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq

codspeed-hq Bot commented Aug 6, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
❌ 1 regressed benchmark
✅ 436 untouched benchmarks
⏩ 121 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_merge_base_validator_valid 3 ms 4.5 ms -34.67%
test_empty_message_passes 9.3 ms 2.3 ms ×4
test_merge_base_validator_invalid 5.5 ms 4.6 ms +19.66%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/fix-merge-base-602anc (b5a5577) with main (7315edf)2

Open in CodSpeed

Footnotes

  1. 121 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (e2edc16) during the generation of this report, so 7315edf was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

shenxianpeng and others added 2 commits August 6, 2026 15:34
Re-review caught this before merge: the HEAD fallback traded the false
failure for a false pass. On a pull_request event the runner checks out
GitHub's synthetic merge commit, whose first parent IS the target tip —
so is-ancestor(target, HEAD) is true by construction, for every branch,
rebased or not. Measured on a diverged branch in that shape:

    git_merge_base('origin/main', 'feat/work')        -> 128
    git_merge_base('origin/main', 'HEAD')             -> 0   <- wrong
    git_merge_base('origin/main', 'origin/feat/work') -> 1   <- the truth

An unresolvable branch name now resolves through its remote-tracking ref
first; HEAD remains only as the last resort, where it still gives a real
answer on checkouts whose HEAD is the branch commit itself (push events,
or a branch that was never pushed). A new test builds the merge-ref shape
with a genuinely diverged branch and asserts FAIL — disabling the
origin/<branch> step fails it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
The CodSpeed run for 47cd595 has sat in "queued" for nine hours after
yesterday's GitHub Actions incident and can no longer be cancelled
("Cannot cancel a workflow re-run that has not yet queued"), so its
check never reports. CodeQL's Analyze (python) on the same SHA cannot
be re-run through the API either — it answers 403 "cannot be retried".

An empty commit is the only lever that reaches both: a new head SHA
starts fresh check runs for the whole suite. No file changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 491-498: Update the fallback around git_merge_base in
commit_check/engine.py lines 491-498 so HEAD is used only after verifying it
represents the actual source-branch commit; otherwise return a resolution
failure without evaluating ancestry. In tests/engine_test.py lines 1259-1297,
remove refs/remotes/origin/feat/work before validation and assert the diverged
branch still returns FAIL.

In `@tests/engine_test.py`:
- Around line 50-51: Update the subprocess call in the test helper using the
fixed git executable to resolve Ruff S603 and S607: either add a narrowly scoped
suppression with an audit comment explaining that the executable is fixed and
arguments are test-controlled, or update the project security-lint configuration
to allow this intentional usage.
🪄 Autofix

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 Plus

Run ID: 3eefdfc4-4d10-49cc-9b96-4240228b01bf

📥 Commits

Reviewing files that changed from the base of the PR and between e2edc16 and 5e98643.

📒 Files selected for processing (2)
  • commit_check/engine.py
  • tests/engine_test.py

Comment thread commit_check/engine.py Outdated
Comment thread tests/engine_test.py
shenxianpeng and others added 2 commits August 7, 2026 05:42
Review caught a residual false pass in the fallback chain. When the
branch is unresolvable under both its own name and origin/<branch>,
the last resort asked about HEAD -- but on a pull_request event HEAD
is GitHub's synthetic merge commit, whose first parent IS the target
tip, so it passes any branch. Measured in that shape with the remote
ref removed, on a branch that is genuinely behind:

    git_merge_base('origin/main', 'feat/work')        -> 128
    git_merge_base('origin/main', 'origin/feat/work') -> 128
    git_merge_base('origin/main', 'HEAD')             -> 0   <- wrong
    git_merge_base('origin/main', 'HEAD^2')           -> 1   <- the truth

HEAD's second parent is the pull request head, the commit actually
under review, so the fallback now asks about that whenever HEAD is a
merge. This answers rather than giving up: where HEAD has a single
parent it is the branch commit itself and still answers for itself,
so the rebased detached-checkout case keeps passing.

Adds git_rev_parse_verify to test for the second parent, and a test
that builds the merge-ref shape with no remote ref and asserts FAIL --
restoring the plain HEAD fallback fails it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
SonarCloud failed the quality gate at 14.2% duplication on new code
(limit 3%). The two diverged-branch tests repeated the same twenty
lines of setup and the same chdir/patch/validate dance.

Extracts _diverged_merge_ref_clone for the shape and
_validate_merge_base for the invocation, leaving each test as its
distinguishing step plus an assertion. Net 13 lines lighter, and the
regression test still fails when the plain HEAD fallback is restored.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2026

Copy link
Copy Markdown

@shenxianpeng
shenxianpeng merged commit 98fb97c into main Aug 7, 2026
27 of 28 checks passed
@shenxianpeng
shenxianpeng deleted the claude/fix-merge-base-602anc branch August 7, 2026 07:01
shenxianpeng added a commit that referenced this pull request Aug 7, 2026
* fix: treat an empty commit message as supplied, not absent

main went red on the push run right after #532 merged, on a test that
had been green on the pull request. Nothing regressed -- the merge was
the first time the test met a real commit.

_get_commit_body tested stdin_text for truth, so an empty string read
as "not provided" and the check fell through to get_commit_info("b"),
the repository's HEAD commit. test_empty_message_passes therefore never
measured an empty message: on a pull_request run HEAD is GitHub's
synthetic merge commit, whose body is empty, so it passed for the wrong
reason; on main HEAD became the squashed commit carrying a
Co-authored-by trailer, CC013 detected it, and the test failed. Measured
on this checkout, the "empty" message resolved to 6694 characters.

The same looseness reaches the public API: validate_message("") answers
about the last commit rather than the empty message it was given.

The skip logic in this file already draws the line at None
(_should_skip_validation, _resolve_current_author); _get_commit_body now
follows it. The CLI is unaffected -- _resolve_commit_message_source
already normalises empty stdin to None.

Adds a hermetic regression test: the existing one only holds while the
checkout's own HEAD carries no AI trailers, which is what made it fragile
in the first place. The new one patches get_commit_info and asserts it is
never consulted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn

* fix: distinguish an absent commit message from an empty one

Follows the one-line fix on _get_commit_body by applying the same rule
to the readers that were still testing stdin_text for truth, so an
empty string is no longer read as "the caller said nothing".

_get_commit_message, _get_subject, _get_author_value and BranchValidator
now split on None, matching _should_skip_validation and
_resolve_current_author, which already did. api.validate_author draws
the same line with `name is not None`, so the intent was there; only
these readers had not followed it. ForcePushValidator deliberately keeps
a truth test: its stdin_text carries a *list* of refs, where empty
genuinely means nothing to check rather than a value to judge.

That surfaced a rule that could never fire. _is_empty_commit_allowed
exists to reject an empty message under allow_empty_commits = false, but
CommitTypeValidator returned PASS on a falsy message before ever
reaching it, so the rejecting branch was dead code. A supplied message
now reaches the rule even when empty; one git never gave us still
returns early. Measured after the change:

    validate_message("")                          -> pass  (default)
    validate_message("", allow_empty_commits=off) -> fail  CC008

The other validators keep their early return: BodyValidator documents
whitespace-only input as "no commit message at all", and
allow_empty_commits is the rule that owns that judgement.

Adds two tests pinning both directions, each patching get_commit_info to
prove the verdict comes from the supplied message rather than the
repository's HEAD. Restoring the early return fails them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn

* docs: record why an unreadable commit file still counts as named

Review asked whether _message_was_supplied should drop to False when a
commit_file cannot be read, since the text then comes from git. Measured
the only reachable case: a HEAD commit whose message is genuinely empty,
where allow_empty_commits = false makes CC008 the correct verdict.
Deriving the flag from successful resolution would restore the miss this
branch fixes, so the behaviour stands and the docstring now says why.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
shenxianpeng added a commit that referenced this pull request Aug 7, 2026
The pin was v2.13.0, whose requirements.txt installs commit-check 2.13.1.
That engine cannot resolve a rebase target existing only as origin/main in
a pull request checkout, so it reported "not rebased" for a branch that
was -- the false CC202 this workflow has carried since its first run.

v2.13.1 of the action installs commit-check 2.13.4, which carries the
merge-base fix from #532. Confirmed by reading requirements.txt at the tag
rather than assuming the action version tracks the engine version:

    v2.13.0 (124de73) -> commit-check==2.13.1
    v2.13.1 (562a184) -> commit-check==2.13.4

The branch was already rebased before this change -- git merge-base
--is-ancestor origin/main HEAD returned true while CC202 still failed --
so the failure was the engine, not the branch, and rebasing again could
never have fixed it.

Two things ride along on the newer engine. Skipped checks now report as
skipped rather than as passes (#537), so a run bypassed by ignore_authors
says so instead of showing green ticks over nothing. And the imperative
whitelist goes from 396 verbs to 529, retiring a class of false CC003 --
2.13.1 rejected "treat", which #527 had added three releases earlier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant