Skip to content

fix: treat a bare PR number as a PR even when git resolves it as an abbreviated commit - #171

Closed
vitali87 wants to merge 2 commits into
fix/validate-settings-before-cleanupfrom
fix/pr-number-before-rev-parse
Closed

vitali87 wants to merge 2 commits into
fix/validate-settings-before-cleanupfrom
fix/pr-number-before-rev-parse

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Summary

split 1006 could silently split the wrong thing: the argument-form decision consulted branch_exists() (git rev-parse --verify) before _is_fork_ref(), and rev-parse happily resolves 1006 as an abbreviated commit SHA if any commit hash starts with those digits. The run then proceeded with "Extracting diff between main and 1006" against an arbitrary commit instead of fetching PR #1006 — reproduced against a real repo during review.

_is_fork_ref now wins unconditionally: a bare number (with or without #) is always a PR number. The precedence is documented in the argument help and README, including the escape hatch for a local branch literally named like a number — pass its full ref (refs/heads/1006), which flows through branch_exists/rev-parse and the whole pipeline (verified end to end, including the derived sub-branch namespace).

Stacked on #136 (stack = #111→#136→this).

Test plan

  • test_pr_number_is_never_taken_for_an_abbreviated_sha — fails on the base (_resolve_fork_ref never called there)
  • test_full_ref_is_not_a_fork_ref and test_full_ref_escape_hatch_takes_the_branch_path pin the documented escape hatch
  • Review reproduced the original bug on a real repo (brute-forced a commit whose hash starts with 1006) and verified the escape hatch on real git
  • 452 tests pass, ruff clean
  • Local review: 5/5

@coderabbitai

coderabbitai Bot commented Aug 30, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 56 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 0acbe554-f2a3-4541-92d1-c6f127e1c43d

📥 Commits

Reviewing files that changed from the base of the PR and between e8a1c7b and 568ccea.

📒 Files selected for processing (3)
  • README.md
  • pr_split/cli.py
  • tests/test_cli_coverage.py

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.

…before-rev-parse

Brings in the base PR (#136) as already updated with origin/main, so this
stacked PR is current with main and shares its base's conflict resolution.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w

Copy link
Copy Markdown
Owner Author

This change is already in main: it landed via the merge train #243, which includes this PR's head commit. GitHub can't mark a stacked PR merged when it lands that way, so I'm closing it as done.


Generated by Claude Code

@vitali87 vitali87 closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants