diff --git a/README.md b/README.md index 124af7e..261a160 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,14 @@ Works for same-repo and fork PRs: the PR's head (`refs/pull//head`) is fetche pr-split split '#42' --base main ``` +A bare number (with or without `#`) is always treated as a PR number, even if a +local branch or abbreviated commit hash with the same digits exists. To split a +local branch literally named like a number, pass its full ref: + +```bash +pr-split split refs/heads/1006 --base main +``` + ### Split a fork PR by user:branch ```bash @@ -101,7 +109,7 @@ pr-split split feature-branch --base main --dry-run pr-split split feature-branch --base main --stack ``` -A group with a single parent has its branch cut from the parent group's branch, carries the parent's hunks for shared files, and targets the parent's branch, with or without `--stack`. Its PR shows only its own diff, compiles standalone, and GitHub retargets it automatically as the parent merges. A group with several parents is cut from the merge base, carries every ancestor's changes, and targets the base branch, so its diff also includes those ancestors' changes (its PR description lists them). Groups with no dependencies are cut from the merge base and target the base branch. `--stack` additionally registers the chains as native GitHub stacks. +A group with a single parent has its branch cut from the parent group's branch, carries the parent's hunks for shared files, and targets the parent's branch, with or without `--stack`. Its PR shows only its own diff and compiles standalone. When `pr-split merge` reaches such a child it retargets the PR at the base branch (`gh pr edit --base`) right before merging it, since GitHub only does that itself for native stacks or when the parent's head branch is deleted (which `gh pr merge --auto` skips). A group with several parents is cut from the merge base, carries every ancestor's changes, and targets the base branch, so its diff also includes those ancestors' changes (its PR description lists them). Groups with no dependencies are cut from the merge base and target the base branch. `--stack` additionally registers the chains as native GitHub stacks. A new file larger than `--max-loc` cannot fit in any one sub-PR. With `--stack` it is cut into pieces between top-level definitions: Python files at top-level statements, and other languages at unindented lines after a blank line, outside brackets, strings and comments. Each piece is appended by a PR stacked on the one holding the piece before it. Every layer holds a valid prefix of the file, and the top of the chain holds the whole file. Without `--stack` a new file is never cut. @@ -121,7 +129,7 @@ Shows a table with each sub-PR's ID, title, branch, PR number, live state (OPEN/ pr-split merge ``` -Walks the dependency DAG and merges each PR in topological order. Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed or any PR was left blocked, so re-run once the blocking PRs are ready. +Walks the dependency DAG and merges each PR in topological order. Each child PR that targets its parent's branch is retargeted at the base branch right before it is merged (native-stack PRs are left to GitHub). Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed, any PR was left blocked, or a PR's state could not be fetched from GitHub (check `gh auth status`), so re-run once the cause is resolved. Use `--auto` to queue merges behind CI checks (uses `gh pr merge --auto`): @@ -129,9 +137,9 @@ Use `--auto` to queue merges behind CI checks (uses `gh pr merge --auto`): pr-split merge --auto ``` -`--auto` is not fire-and-forget: after queueing a batch, `merge` waits for every PR in it to reach `MERGED` before moving on to the dependent batch, polling GitHub every 10 seconds for up to 10 minutes per batch. If a PR is still unmerged when the timeout expires, or gets closed while waiting, the command stops before the dependent batch and exits 1 (webhook `exit_reason: incomplete_batch`); re-run `pr-split merge --auto` once CI has caught up to continue from where it left off. +`--auto` is not fire-and-forget: after queueing a batch, `merge` waits for every PR in it to reach `MERGED` before moving on to the dependent batch, polling GitHub every 10 seconds for up to 10 minutes per batch. If a PR is still unmerged when the timeout expires, the command stops before the dependent batch and exits 1 (webhook `exit_reason: incomplete_batch`). A PR closed while waiting is listed as ` (CLOSED)` among the skipped PRs, its dependants are skipped as blocked, and the run exits 1 (webhook `exit_reason: pr_closed`). Either way, re-run `pr-split merge --auto` once CI has caught up to continue from where it left off. -Use `--notify` to POST merge results to a webhook URL (e.g. Slack, Discord): +Use `--notify` to POST merge results to an http(s) webhook URL (e.g. Slack, Discord); other schemes are rejected up front, and a delivery failure (timeout, non-2xx) is logged as a warning without changing the exit code: ```bash pr-split merge --notify https://hooks.slack.com/... @@ -253,7 +261,7 @@ commit_message = "chore: release notes for {title}" # {id}, {title}, {index} ### Re-split with different parameters -Running `split` again when a plan already exists will prompt you to clean up existing branches and PRs before re-planning. Dry-run plans are silently overwritten. +Running `split` again when a plan already exists will prompt you to clean up existing branches and PRs before re-planning. Dry-run plans are silently overwritten. With `--dry-run`, `split` never closes PRs or deletes branches: if the saved plan has already been executed it exits 1 and asks you to run `pr-split clean` first. ### Clean up @@ -302,6 +310,7 @@ on: branches: [main] permissions: + contents: read pull-requests: write jobs: @@ -337,7 +346,7 @@ jobs: |--------|-------------| | `total-loc` | Total lines of code in the PR diff | | `total-groups` | Number of suggested groups | -| `objective` | Plan objective score (lower is better) | +| `objective` | Simplified plan score, `overflow × 1000 + file_scatter × 50 + groups` (lower is better; `0` when no split plan was scored; not the full `score_plan` objective described in METHODOLOGY.md) | | `should-split` | Whether the PR should be split (`true`/`false`) | ## Planning backends diff --git a/action.yml b/action.yml index 6ff063f..3141470 100644 --- a/action.yml +++ b/action.yml @@ -35,7 +35,7 @@ outputs: description: "Number of suggested groups" value: ${{ steps.score.outputs.total_groups }} objective: - description: "Plan objective score (lower is better)" + description: "Simplified plan score: overflow*1000 + file_scatter*50 + groups (lower is better; 0 when no split plan was scored)" value: ${{ steps.score.outputs.objective }} should-split: description: "Whether the PR should be split (true/false)" @@ -79,11 +79,17 @@ runs: run: python "${{ github.action_path }}/scripts/score_pr.py" - name: Post comment - if: inputs.post-comment == 'true' && steps.score.outputs.should_split == 'true' + if: inputs.post-comment == 'true' && steps.score.outputs.comment_path != '' uses: actions/github-script@v7 + env: + SHOULD_SPLIT: ${{ steps.score.outputs.should_split }} with: script: | const fs = require('fs'); + if (!context.payload.pull_request) { + core.info('Not a pull_request event; no comment to post.'); + return; + } const body = fs.readFileSync('${{ steps.score.outputs.comment_path }}', 'utf8'); const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, @@ -92,13 +98,15 @@ runs: }); const existing = comments.find(c => c.body.includes('')); if (existing) { + // Always refresh an existing comment so a PR that shrank below + // the threshold no longer shows a stale split plan. await github.rest.issues.updateComment({ owner: context.repo.owner, repo: context.repo.repo, comment_id: existing.id, body, }); - } else { + } else if (process.env.SHOULD_SPLIT === 'true') { await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/pr_split/cli.py b/pr_split/cli.py index ad690a4..31f1094 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -2,8 +2,12 @@ import json as json_mod import shutil +import stat +import sys import tempfile import time +import urllib.error +import urllib.parse import urllib.request from collections.abc import Callable, Generator from concurrent.futures import ThreadPoolExecutor, as_completed @@ -42,8 +46,10 @@ materialize_group_files, merge_chain_assignments, parse_diff, + target_file_modes, ) from .exceptions import ( + DiffParseError, ErrorMsg, GitOperationError, PlanValidationError, @@ -64,10 +70,11 @@ fetch_fork_pr, is_worktree_clean, merge_base, + prune_remote_tracking_refs, push_branch, remove_worktree, ) -from .git_ops.branches import commit_exists, run_git +from .git_ops.branches import commit_exists, require_tools, run_git from .git_ops.prs import ( branch_has_merged, close_pr, @@ -77,6 +84,7 @@ get_pr_state, link_stack, merge_pr, + retarget_pr, set_pr_base, stack_numbers_for, unstack, @@ -126,19 +134,64 @@ def _choose_plan( select_plan(branch) +def _reachable_from_roots(groups: list[Group]) -> set[str]: + """Ids that a root-first walk will actually visit. + + A group that is neither a root nor a descendant of one (unknown + dependency chain, cycle) is never drawn, so it must not count towards + the number of parents a child expects to be visited from. + """ + children: dict[str, list[str]] = {g.id: [] for g in groups} + for g in groups: + for dep in g.depends_on: + if dep in children: + children[dep].append(g.id) + reachable: set[str] = set() + stack = [g.id for g in groups if not g.depends_on] + while stack: + current = stack.pop() + if current in reachable: + continue + reachable.add(current) + stack.extend(children[current]) + return reachable + + +def _expected_visits(groups: list[Group]) -> dict[str, int]: + """How many times each group will be reached from a drawn parent. + + Every reachable parent is drawn in full exactly once, so a child is + visited once per reachable parent. It is rendered as a stub until its + final visit, which guarantees the full node (with its subtree and the + "<-- this PR" marker) appears exactly once and every stub precedes it. + """ + reachable = _reachable_from_roots(groups) + # A set, not a count: the walk visits a child once per distinct parent, + # so a duplicated depends_on entry must not raise the expected total. + return {g.id: len({d for d in g.depends_on if d in reachable}) for g in groups} + + def _render_dag(groups: list[Group]) -> str: roots = [g for g in groups if not g.depends_on] tree = Tree("Split Plan") + expected = _expected_visits(groups) + seen: dict[str, int] = {g.id: 0 for g in groups} def _add_children(parent_tree: Tree, parent_id: str) -> None: children = [g for g in groups if parent_id in g.depends_on] for child in children: - deps_label = ", ".join(child.depends_on) - branch = parent_tree.add(f"{child.id}: {child.title} (depends on: {deps_label})") + seen[child.id] += 1 + if seen[child.id] < expected[child.id]: + parent_tree.add(escape(f"{child.id}: {child.title} (see below)")) + continue + deps_label = ", ".join(dict.fromkeys(child.depends_on)) + branch = parent_tree.add( + escape(f"{child.id}: {child.title} (depends on: {deps_label})") + ) _add_children(branch, child.id) for root in roots: - root_branch = tree.add(f"{root.id}: {root.title}") + root_branch = tree.add(escape(f"{root.id}: {root.title}")) _add_children(root_branch, root.id) with console.capture() as capture: @@ -149,14 +202,26 @@ def _add_children(parent_tree: Tree, parent_id: str) -> None: def _render_dag_markdown(groups: list[Group], current_id: str) -> str: roots = [g for g in groups if not g.depends_on] lines: list[str] = [] + expected = _expected_visits(groups) + seen: dict[str, int] = {g.id: 0 for g in groups} def _add_children(parent_id: str, prefix: str) -> None: children = [g for g in groups if parent_id in g.depends_on] for i, child in enumerate(children): is_last = i == len(children) - 1 connector = "\u2514\u2500\u2500" if is_last else "\u251c\u2500\u2500" + seen[child.id] += 1 + if seen[child.id] < expected[child.id]: + # Another parent will visit this node later; the full node + # (with its subtree and the "<-- this PR" marker) follows there. + lines.append(f"{prefix}{connector} {child.id}: {child.title} (see below)") + continue marker = " <-- this PR" if child.id == current_id else "" - lines.append(f"{prefix}{connector} {child.id}: {child.title}{marker}") + also = "" + others = [d for d in dict.fromkeys(child.depends_on) if d != parent_id] + if others: + also = f" (also depends on: {', '.join(others)})" + lines.append(f"{prefix}{connector} {child.id}: {child.title}{also}{marker}") extension = " " if is_last else "\u2502 " _add_children(child.id, prefix + extension) @@ -169,6 +234,21 @@ def _add_children(parent_id: str, prefix: str) -> None: return f"## Dependency graph\n\nMerge in this order:\n\n```\n{tree_block}\n```" +def _check_pr_template() -> None: + try: + validate_pr_template() + except PRSplitError as exc: + console.print(f"[red]{exc}[/red]") + raise typer.Exit(1) from exc + + +def _require_cli_tools(*tools: str) -> None: + missing = require_tools(*tools) + if missing is not None: + console.print(f"[red]{ErrorMsg.TOOL_NOT_FOUND(tool=missing)}[/red]") + raise typer.Exit(1) + + def _require_local_branch(base: str) -> None: """Exit unless ``base`` names a local branch head. @@ -226,6 +306,8 @@ def _validate_inputs( if not dry_run and not check_gh_auth(): console.print(f"[red]{ErrorMsg.GH_AUTH_FAILED()}[/red]") raise typer.Exit(1) + if not dry_run: + _check_pr_template() if not dry_run and stacked: _require_gh_stack() @@ -242,7 +324,7 @@ def _handle_loc_bound_warnings(warnings: list[str], *, strict_loc_bounds: bool) if strict_loc_bounds and warnings: console.print(f"[red]{ErrorMsg.LOC_BOUNDS_STRICT_FAILED()}[/red]") for warning in warnings: - console.print(f"[red]- {warning}[/red]") + console.print(f"[red]- {escape(warning)}[/red]") raise typer.Exit(1) for warning in warnings: @@ -313,12 +395,13 @@ def _present_plan(groups: list[Group]) -> None: files = ", ".join(a.file_path for a in group.assignments) deps = ", ".join(group.depends_on) if group.depends_on else "" diff_str = f"+{group.estimated_added}/-{group.estimated_removed}" + # Plan text is LLM/user-written; "[...]" in it is Rich markup unless escaped. table.add_row( - group.id, - group.title, + escape(group.id), + escape(group.title), diff_str, - deps, - files, + escape(deps), + escape(files), ) console.print(table) @@ -357,14 +440,33 @@ def _create_single_branch_and_commit( add_worktree(worktree_path, branch_name, start_point or merge_base_ref) try: materialized = materialize_group_files(parsed_diff, group, merge_base_ref) + modes = target_file_modes(parsed_diff, group) for file_path, content in materialized.items(): p = Path(worktree_path) / file_path - if content is not None: - p.parent.mkdir(parents=True, exist_ok=True) - # newline="" keeps CRLF from the reconstructed content intact. - p.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") - elif p.exists(): + mode = modes.get(file_path) + if content is None: + # A dangling symlink is not "exists()" but must still go. + if p.is_symlink() or p.exists(): + p.unlink() + continue + p.parent.mkdir(parents=True, exist_ok=True) + if p.is_symlink(): + # Never write through an existing link (it would modify the + # target file); replace the link itself. p.unlink() + if mode is not None and stat.S_ISLNK(mode): + # Git stores a symlink's target as the blob content. A regular + # file being converted to a link is still on disk here. + if p.is_symlink() or p.exists(): + p.unlink() + p.symlink_to(content.rstrip("\n")) + continue + # newline="" keeps CRLF from the reconstructed content intact. + p.write_text(content, encoding="utf-8", errors="surrogateescape", newline="") + if mode is not None: + # Git only tracks the executable bit; apply the diff's target + # mode so chmod changes reach the sub-PR. + p.chmod(mode & 0o777) logger.info(logs.COMMITTING_GROUP.format(group=group.id, title=group.title)) commit_sha = commit_files_in_dir( @@ -551,13 +653,72 @@ def _pr_template_path() -> Path: return plan_dir() / "template.md" +# GitHub rejects PR bodies over 65 536 characters; a group holding thousands +# of files (mass rename, formatter run) blows past that with its file list +# alone, and the failure only surfaced after every branch had been pushed. +_PR_BODY_FILE_LIST_LIMIT = 200 + + +def _format_file_list(files: list[str]) -> str: + shown = files[:_PR_BODY_FILE_LIST_LIMIT] + lines = [f"- `{f}`" for f in shown] + if len(files) > len(shown): + lines.append(f"- … and {len(files) - len(shown)} more files") + return "\n".join(lines) + + +_PR_TEMPLATE_PLACEHOLDERS = ( + "description", + "files", + "added", + "removed", + "loc", + "dependencies", + "dag", + "id", + "title", +) + + +def _render_pr_template(template_vars: dict[str, object]) -> str: + template_path = _pr_template_path() + try: + template = template_path.read_text(encoding="utf-8") + return template.format(**template_vars) + except (KeyError, ValueError, IndexError, AttributeError, TypeError) as exc: + # AttributeError/TypeError cover "{title.nope}" and "{added[0]}", + # which str.format raises as plain attribute/subscript errors. + available = ", ".join(f"{{{k}}}" for k in sorted(template_vars)) + raise PRSplitError( + f"Invalid PR template at {template_path}: {exc}. " + f"Available placeholders: {available}. " + "Escape literal braces with {{ and }}." + ) from exc + except OSError as exc: + raise PRSplitError(f"Could not read PR template at {template_path}: {exc}") from exc + + +def validate_pr_template() -> None: + """Render the custom template against placeholder values. + + The template is otherwise first used when the PRs are opened, i.e. after + every branch has been created and pushed; a typo in it must fail before + that. Raises PRSplitError with the same message the real render would. + """ + if not _pr_template_path().exists(): + return + sample: dict[str, object] = {k: k for k in _PR_TEMPLATE_PLACEHOLDERS} + sample.update({"added": 0, "removed": 0, "loc": 0}) + _render_pr_template(sample) + + def _build_pr_body(group: Group, all_groups: list[Group]) -> str: template_path = _pr_template_path() if template_path.exists(): files = [a.file_path for a in group.assignments] - template_vars = { + template_vars: dict[str, object] = { "description": group.description, - "files": "\n".join(f"- `{f}`" for f in files), + "files": _format_file_list(files), "added": group.estimated_added, "removed": group.estimated_removed, "loc": group.estimated_loc, @@ -566,24 +727,12 @@ def _build_pr_body(group: Group, all_groups: list[Group]) -> str: "id": group.id, "title": group.title, } - try: - template = template_path.read_text(encoding="utf-8") - return template.format(**template_vars) - except (KeyError, ValueError, IndexError) as exc: - available = ", ".join(f"{{{k}}}" for k in sorted(template_vars)) - raise PRSplitError( - f"Invalid PR template at {template_path}: {exc}. " - f"Available placeholders: {available}. " - "Escape literal braces with {{ and }}." - ) from exc - except OSError as exc: - raise PRSplitError(f"Could not read PR template at {template_path}: {exc}") from exc + return _render_pr_template(template_vars) files = [a.file_path for a in group.assignments] sections = [group.description] if files: - file_list = "\n".join(f"- `{f}`" for f in files) - sections.append(f"## Files changed\n\n{file_list}") + sections.append(f"## Files changed\n\n{_format_file_list(files)}") sections.append( f"## Diff stats\n\n" f"**+{group.estimated_added}** additions, " @@ -643,6 +792,11 @@ def _push_and_create_prs( record_map = {r.group_id: r for r in branch_records} errors: list[tuple[str, Exception]] = [] + # Branch names are reused across runs; drop tracking refs left by an + # earlier split whose remote branch was merged or deleted since, or + # force-with-lease rejects every push as stale. + prune_remote_tracking_refs() + # Children target parent branches, so every branch is pushed before any PR opens. with ThreadPoolExecutor(max_workers=_PUSH_MAX_WORKERS) as executor: push_futures = { @@ -679,22 +833,53 @@ def _base_pushed(group: Group) -> bool: return False gid = owner + # Open PRs in base-chain order: a child's PR targets its parent's + # branch, so if the parent's PR could not be opened the child must be + # skipped too, or the saved plan ends up holding orphans whose stack + # root does not exist. + pending = [g for g in groups if g.id not in done and g.id in pushed and _base_pushed(g)] + results: dict[str, PRRecord] = dict(done) + failed_prs: set[str] = set() with ThreadPoolExecutor(max_workers=_PUSH_MAX_WORKERS) as executor: - future_to_group_id = { - executor.submit( - _create_single_pr, group, record_map[group.id], groups, draft=draft - ): group.id - for group in groups - if group.id not in done and group.id in pushed and _base_pushed(group) - } - results: dict[str, PRRecord] = dict(done) - for future in as_completed(future_to_group_id): - group_id = future_to_group_id[future] - try: - results[group_id] = future.result() - except Exception as exc: - logger.error(f"Failed to create PR for {group_id}: {exc}") - errors.append((group_id, exc)) + while pending: + ready: list[Group] = [] + for group in pending: + owner = branch_owner.get(record_map[group.id].base_branch) + if owner is None or owner in results: + ready.append(group) + elif owner in failed_prs: + logger.warning( + logs.PR_SKIPPED_BASE_PR_FAILED.format( + group=group.id, base=record_map[group.id].base_branch + ) + ) + failed_prs.add(group.id) + pending = [g for g in pending if g not in ready and g.id not in failed_prs] + if not ready: + # Whatever is left descends (possibly several levels, and in + # any listing order) from a failed PR; say so for each one. + for group in pending: + logger.warning( + logs.PR_SKIPPED_BASE_PR_FAILED.format( + group=group.id, base=record_map[group.id].base_branch + ) + ) + failed_prs.add(group.id) + break + future_to_group_id = { + executor.submit( + _create_single_pr, group, record_map[group.id], groups, draft=draft + ): group.id + for group in ready + } + for future in as_completed(future_to_group_id): + group_id = future_to_group_id[future] + try: + results[group_id] = future.result() + except Exception as exc: + logger.error(f"Failed to create PR for {group_id}: {exc}") + errors.append((group_id, exc)) + failed_prs.add(group_id) if errors: error_details = "\n".join([f"- {gid}: {exc}" for gid, exc in errors]) @@ -728,7 +913,7 @@ def _move_assignment( if from_id == to_id: console.print( f"[yellow]Source and destination are the same" - f" ('{from_id}'). No move performed.[/yellow]" + f" ('{escape(from_id)}'). No move performed.[/yellow]" ) return False @@ -736,7 +921,7 @@ def _move_assignment( src = group_map.get(from_id) dst = group_map.get(to_id) if not src or not dst: - console.print(f"[red]Group '{from_id}' or '{to_id}' not found.[/red]") + console.print(f"[red]Group '{escape(from_id)}' or '{escape(to_id)}' not found.[/red]") return False pf_map = {pf.path: pf for pf in parsed_diff.patch_set} @@ -763,7 +948,9 @@ def _move_assignment( break if not found: - console.print(f"[red]Hunk {file_path}:{hunk_index} not found in {from_id}.[/red]") + console.print( + f"[red]Hunk {escape(file_path)}:{hunk_index} not found in {escape(from_id)}.[/red]" + ) return False dst_assignment = next((a for a in dst.assignments if a.file_path == file_path), None) @@ -787,7 +974,10 @@ def _move_assignment( refresh_generated_description(src) refresh_generated_description(dst) - console.print(f"[green]Moved {file_path}:{hunk_index} from {from_id} to {to_id}[/green]") + console.print( + f"[green]Moved {escape(file_path)}:{hunk_index} from {escape(from_id)} " + f"to {escape(to_id)}[/green]" + ) return True @@ -795,11 +985,13 @@ def _show_group_detail(groups: list[Group], group_id: str) -> None: group_map = {g.id: g for g in groups} group = group_map.get(group_id) if not group: - console.print(f"[red]Group '{group_id}' not found.[/red]") + console.print(f"[red]Group '{escape(group_id)}' not found.[/red]") return - console.print(f"\n[bold]{group.id}[/bold]: {group.title}") - console.print(f" Description: {group.description}") - console.print(f" Depends on: {', '.join(group.depends_on) or 'none'}") + # Titles, descriptions and paths come from the plan (LLM-written); any + # "[...]" in them would be swallowed as Rich markup unless escaped. + console.print(f"\n[bold]{escape(group.id)}[/bold]: {escape(group.title)}") + console.print(f" Description: {escape(group.description)}") + console.print(f" Depends on: {escape(', '.join(group.depends_on) or 'none')}") console.print( f" Estimated: +{group.estimated_added}/-{group.estimated_removed}" f" ({group.estimated_loc} LOC)" @@ -809,7 +1001,9 @@ def _show_group_detail(groups: list[Group], group_id: str) -> None: hunks_str = "all" else: hunks_str = ", ".join(str(i) for i in a.hunk_indices) - console.print(f" {a.file_path} [{a.assignment_type}] hunks: [{hunks_str}]") + # Square brackets are Rich markup; without escaping "[whole_file]" + # is treated as a style tag and silently dropped from the output. + console.print(escape(f" {a.file_path} [{a.assignment_type.value}] hunks: [{hunks_str}]")) console.print() @@ -1074,7 +1268,17 @@ def _merge_groups( return True +def _stdin_is_interactive() -> bool: + return sys.stdin.isatty() + + def _interactive_edit(groups: list[Group], parsed_diff: ParsedDiff) -> list[Group]: + if not _stdin_is_interactive(): + # Scripts and CI (`split --dry-run < /dev/null`) have no one to answer + # the editor prompt; typer.prompt would turn the EOF into an Abort + # before the plan is ever saved. Accept the plan as-is instead. + console.print("[yellow]Non-interactive stdin; accepting the plan as-is.[/yellow]") + return groups console.print( "\n[cyan]Interactive editor. Commands:[/cyan]\n" " [bold]move[/bold] : \n" @@ -1175,6 +1379,11 @@ def _interactive_edit(groups: list[Group], parsed_diff: ParsedDiff) -> list[Grou ) +def _is_fork_ref(dev_branch: str) -> bool: + """True for the PR-number (`#42`) and `user:branch` argument forms.""" + return dev_branch.lstrip("#").isdigit() or ":" in dev_branch + + def _split_settings( partition_strategy: PartitionStrategy | None, build: Callable[[PartitionStrategy], Settings], @@ -1215,7 +1424,14 @@ def _resolve_fork_ref(dev_branch: str) -> ForkPRInfo | None: @app.command(help="Split a large PR into smaller dependency-ordered PRs.") def split( - dev_branch: Annotated[str, typer.Argument(help="Branch name, PR number, or user:branch")], + dev_branch: Annotated[ + str, + typer.Argument( + help="Branch name, PR number, or user:branch. A bare number is always " + "treated as a PR number; for a local branch literally named like a " + "number, use its full ref, e.g. refs/heads/1006." + ), + ], base: Annotated[str, typer.Option(help="Base branch")] = "main", min_loc: Annotated[ int | None, @@ -1304,13 +1520,30 @@ def split( fork_info: ForkPRInfo | None = None select_plan(dev_branch_arg) + # Say "git/gh is not installed" before any helper turns that into a + # misleading "branch not found" or "authentication failed". + _require_cli_tools("git", *(() if dry_run else ("gh",))) + # A branch that exists only as origin/ (fresh clone or worktree) # is adopted as a local branch, as `git checkout ` would. for name in (dev_branch, base): - if not (name.lstrip("#").isdigit() or ":" in name): + if not _is_fork_ref(name): adopt_remote_branch(name) - if not branch_exists(dev_branch): + # Decide the argument form before consulting git: `git rev-parse` accepts + # abbreviated SHAs, so a bare PR number such as 1006 would otherwise be + # taken for a commit whose hash starts with those digits and the wrong + # diff would be split without any warning. + if _is_fork_ref(dev_branch) or not branch_exists(dev_branch): + # Only a PR number or user:branch can be fetched from GitHub; a plain + # branch name that does not exist is just a typo, so say so before + # touching gh (which --dry-run must not require). + if not _is_fork_ref(dev_branch): + console.print(f"[red]{ErrorMsg.BRANCH_NOT_FOUND(branch=dev_branch)}[/red]") + raise typer.Exit(1) + # A PR number or user:branch has to be fetched with gh even for a + # dry run. + _require_cli_tools("gh") if not check_gh_auth(): console.print(f"[red]{ErrorMsg.GH_AUTH_FAILED()}[/red]") raise typer.Exit(1) @@ -1331,14 +1564,46 @@ def split( _validate_inputs(dev_branch, base, dry_run=dry_run, stacked=stack) + # Validate settings before anything destructive: the existing-plan + # cleanup below closes PRs and deletes branches, and a bad --min-loc or + # a missing API key must not be discovered only after that. + try: + settings = _split_settings( + partition_strategy, + lambda strategy: Settings( + min_loc=min_loc, + max_loc=max_loc, + strict_loc_bounds=strict_loc_bounds, + max_refinement_iterations=max_refinement_iterations, + cp_sat_timeout=cp_sat_timeout, + priority=priority, + chunk_strategy=chunk_strategy, + partition_strategy=strategy, + ), + ) + except (ValidationError, ValueError) as exc: + console.print(f"[red]{escape(str(exc))}[/red]") + raise typer.Exit(1) from exc + if plan_exists(): existing = _load_plan_or_exit() has_git_state = existing.git_state.branches or existing.git_state.prs + if has_git_state and dry_run: + # A preview must never close PRs or delete branches, and saving + # the new plan would drop the record of the existing ones. + console.print("[yellow]An existing split plan with branches/PRs was found.[/yellow]") + console.print( + "[red]--dry-run would overwrite the record of those branches/PRs without " + "cleaning them up. Run 'pr-split clean' first, or run 'pr-split split' " + "without --dry-run to clean up and re-split.[/red]" + ) + raise typer.Exit(1) if has_git_state: console.print("[yellow]An existing split plan with branches/PRs was found.[/yellow]") console.print( "[red]Warning: this will permanently close PRs and delete remote branches.[/red]" ) + _require_cleanup_tools(existing.git_state) if typer.confirm("Clean up and proceed with re-splitting?"): closed_prs, deleted_branches = _cleanup_git_state(existing.git_state) logger.success( @@ -1355,7 +1620,16 @@ def split( raw_diff = extract_diff(dev_branch, diff_base) # Only a stacked child builds on the PR holding a file's earlier pieces. split_new_files_over = max_loc if stack else None - parsed_diff = parse_diff(raw_diff, split_new_files_over=split_new_files_over) + try: + parsed_diff = parse_diff(raw_diff, split_new_files_over=split_new_files_over) + except DiffParseError as exc: + console.print(f"[red]{exc}[/red]") + raise typer.Exit(1) from exc + if not parsed_diff.patch_set: + # Planning an empty diff "succeeds" with zero groups and then + # execute rejects the plan as missing its diff; stop here instead. + console.print(f"[red]{ErrorMsg.NO_CHANGES(base=base, dev=dev_branch_arg)}[/red]") + raise typer.Exit(1) stats = parsed_diff.stats logger.info( logs.DIFF_STATS.format( @@ -1366,23 +1640,6 @@ def split( ) ) - try: - settings = _split_settings( - partition_strategy, - lambda strategy: Settings( - min_loc=min_loc, - max_loc=max_loc, - strict_loc_bounds=strict_loc_bounds, - max_refinement_iterations=max_refinement_iterations, - cp_sat_timeout=cp_sat_timeout, - priority=priority, - chunk_strategy=chunk_strategy, - partition_strategy=strategy, - ), - ) - except (ValidationError, ValueError) as exc: - console.print(f"[red]{exc}[/red]") - raise typer.Exit(1) from exc try: validate_no_binary_files(parsed_diff) except PlanValidationError as exc: @@ -1433,7 +1690,7 @@ def split( logger.success("Edited plan validation passed") _report_oversized_groups(groups, settings.max_loc, hunk_counts) except PRSplitError as exc: - console.print(f"[red]Edited plan is invalid: {exc}[/red]") + console.print(f"[red]Edited plan is invalid: {escape(str(exc))}[/red]") raise typer.Exit(1) from exc merge_base_ref = merge_base(diff_base, dev_branch) @@ -1559,7 +1816,9 @@ def status() -> None: # so showing it would claim OPEN for a PR that may be gone. pr_state = "UNKNOWN" unverified.append(pr_record.pr_number) - table.add_row(group.id, group.title, branch_name, pr_info, pr_state, review) + table.add_row( + escape(group.id), escape(group.title), escape(branch_name), pr_info, pr_state, review + ) console.print(table) for gid, branch, parent in stale_layers(plan_file): @@ -1572,6 +1831,14 @@ def status() -> None: ) +def _require_cleanup_tools(git_state: GitState) -> None: + # Every close/delete failure inside _cleanup_git_state is swallowed as a + # warning, so a missing binary would otherwise look like a successful + # cleanup that then deletes the plan. Both callers (clean and the + # re-split prompt in split) go through here before asking to proceed. + _require_cli_tools("git", *(("gh",) if git_state.prs else ())) + + def _cleanup_git_state(git_state: GitState) -> tuple[int, int]: closed_prs = 0 for pr_record in git_state.prs: @@ -1812,6 +2079,8 @@ def clean() -> None: plan_file = _load_plan_or_exit() git_state = plan_file.git_state + _require_cleanup_tools(git_state) + typer.confirm("Delete all pr-split branches and close PRs?", abort=True) closed_prs, deleted_branches = _cleanup_git_state(git_state) @@ -1923,12 +2192,16 @@ def execute( " (the push or PR creation failed). Recreating them and retrying.[/yellow]" ) - if not plan.raw_diff: + if plan.raw_diff is None: console.print( "[red]Plan is missing saved diff data." " Re-run 'pr-split split --dry-run' to regenerate.[/red]" ) raise typer.Exit(1) + if not plan.raw_diff.strip() or not plan.groups: + # A plan saved by an older version from an empty diff. + console.print(f"[red]{ErrorMsg.PLAN_HAS_NO_CHANGES()}[/red]") + raise typer.Exit(1) if not plan.merge_base_sha: console.print( @@ -1936,6 +2209,7 @@ def execute( " Re-run 'pr-split split --dry-run' to regenerate.[/red]" ) raise typer.Exit(1) + _require_cli_tools("git", "gh") if not commit_exists(plan.merge_base_sha): console.print( f"[red]Plan's merge base {plan.merge_base_sha} is not in this repository " @@ -1955,10 +2229,15 @@ def execute( if not check_gh_auth(): console.print(f"[red]{ErrorMsg.GH_AUTH_FAILED()}[/red]") raise typer.Exit(1) + _check_pr_template() if plan.stacked: _require_gh_stack() - parsed_diff = parse_diff(plan.raw_diff, split_new_files_over=plan.split_new_files_over) + try: + parsed_diff = parse_diff(plan.raw_diff, split_new_files_over=plan.split_new_files_over) + except DiffParseError as exc: + console.print(f"[red]{exc}[/red]") + raise typer.Exit(1) from exc try: validate_no_binary_files(parsed_diff) @@ -1969,7 +2248,7 @@ def execute( validate_coverage(plan.groups, parsed_diff) validate_new_file_pieces(plan.groups, parsed_diff, dag) except PlanValidationError as exc: - console.print(f"[red]{exc}[/red]") + console.print(f"[red]{escape(str(exc))}[/red]") raise typer.Exit(1) from exc # Any plan with dependency edges is laid out along its DAG (stacked or not), @@ -2034,7 +2313,12 @@ def execute( _AUTO_MERGE_POLL_TIMEOUT = 600 -def _poll_for_merged(group_ids: list[str], pr_map: dict[str, PRRecord]) -> set[str]: +def _poll_for_merged( + group_ids: list[str], + pr_map: dict[str, PRRecord], + fetch_errors: list[str] | None = None, + closed: list[str] | None = None, +) -> set[str]: pending = set(group_ids) actually_merged: set[str] = set() deadline = time.monotonic() + _AUTO_MERGE_POLL_TIMEOUT @@ -2053,6 +2337,10 @@ def _poll_for_merged(group_ids: list[str], pr_map: dict[str, PRRecord]) -> set[s logger.warning( f"PR #{pr_record.pr_number} ({gid}) {reason} while polling, aborting wait" ) + if state == "" and fetch_errors is not None: + fetch_errors.append(gid) + if state == "CLOSED" and closed is not None: + closed.append(gid) pending.discard(gid) if pending: remaining = ", ".join(pending) @@ -2060,11 +2348,44 @@ def _poll_for_merged(group_ids: list[str], pr_map: dict[str, PRRecord]) -> set[s return actually_merged +def _validate_webhook_url(value: str | None) -> str | None: + """Accept only http(s) webhook URLs. + + urllib happily opens file:// (and reports "Webhook notification sent"), + and a bare hostname fails only after the merges with an obscure + "unknown url type" warning. + """ + if value is None: + return None + parts = urllib.parse.urlsplit(value) + if parts.scheme not in ("http", "https") or not parts.netloc: + raise typer.BadParameter(f"must be an http(s) URL, got '{value}'") + return value + + +class _NoRedirect(urllib.request.HTTPRedirectHandler): + """Refuse to follow redirects. + + urllib re-issues a redirected POST as a body-less GET, so a moved + webhook URL would receive an empty request while the tool reports the + notification as sent. Surface the redirect instead so the user can + update the URL. + """ + + def redirect_request(self, req, fp, code, msg, headers, newurl): + raise urllib.error.HTTPError( + req.full_url, code, f"redirected to {newurl}; update the webhook URL", headers, fp + ) + + +_webhook_opener = urllib.request.build_opener(_NoRedirect) + + def _send_webhook(url: str, payload: dict[str, object]) -> None: try: data = json_mod.dumps(payload).encode("utf-8") req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) - with urllib.request.urlopen(req, timeout=10) as resp: + with _webhook_opener.open(req, timeout=10) as resp: resp.read() logger.info(f"Webhook notification sent to {url}") except Exception as exc: @@ -2090,8 +2411,9 @@ def merge_all( str | None, typer.Option( "--notify", - help="Webhook URL to POST merge results to", + help="Webhook URL (http/https) to POST merge results to", envvar="PR_SPLIT_WEBHOOK_URL", + callback=_validate_webhook_url, ), ] = None, ) -> None: @@ -2103,11 +2425,19 @@ def merge_all( plan = plan_file.plan git_state = plan_file.git_state pr_map = {r.group_id: r for r in git_state.prs} + # Stacked children were opened against their parent's split branch. + stacked_children = { + r.group_id for r in git_state.branches if r.base_branch != plan.base_branch + } if not pr_map: console.print("[yellow]No PRs found in plan. Nothing to merge.[/yellow]") raise typer.Exit(0) + # Without gh every PR would be skipped as a "fetch error" and the run + # would still report success. + _require_cli_tools("gh") + if _base_has_merged(plan.base_branch): console.print( f"[red]{logs.BASE_ALREADY_MERGED.format(base=escape(plan.base_branch))}[/red]" @@ -2126,6 +2456,8 @@ def merge_all( skipped: list[str] = [] skipped_ids: set[str] = set() blocked: list[str] = [] + fetch_errors: list[str] = [] + closed_while_waiting: list[str] = [] failed: list[str] = [] stopped = False @@ -2144,6 +2476,7 @@ def merge_all( f"PR #{pr_record.pr_number} ({group_id}) state could not be fetched, skipping" ) skipped_ids.add(group_id) + fetch_errors.append(group_id) skipped.append(f"{group_id} (fetch error)") continue @@ -2190,6 +2523,13 @@ def merge_all( continue try: + if group_id in stacked_children: + # Its parent has merged by now (iter_ready + the guard + # above), but the PR still targets the parent's branch. + # Merging there -- which `--auto` does silently, since gh + # skips deleting the head branch in auto mode -- would + # never reach the base branch. + retarget_pr(pr_record.pr_number, plan.base_branch) merge_pr(pr_record.pr_number, auto=auto) if not auto: merged.append(group_id) @@ -2203,8 +2543,21 @@ def merge_all( queued = [gid for gid in batch if gid not in merged and gid not in skipped_ids] if queued: logger.info(f"Waiting for auto-merge to complete: {', '.join(queued)}") - actually_merged = _poll_for_merged(queued, pr_map) + poll_fetch_errors: list[str] = [] + poll_closed: list[str] = [] + actually_merged = _poll_for_merged(queued, pr_map, poll_fetch_errors, poll_closed) merged.extend(actually_merged) + for gid in poll_fetch_errors: + fetch_errors.append(gid) + skipped_ids.add(gid) + skipped.append(f"{gid} (fetch error)") + for gid in poll_closed: + # Closed while we waited: name it like a closed PR found up + # front, but keep it a failure -- we queued it for merging + # and it never merged. + closed_while_waiting.append(gid) + skipped_ids.add(gid) + skipped.append(f"{gid} (CLOSED)") if stopped or any(gid not in merged and gid not in skipped_ids for gid in batch): if not stopped: @@ -2222,15 +2575,25 @@ def merge_all( console.print() if merged: - console.print(f"[green]Merged ({len(merged)}): {', '.join(merged)}[/green]") + console.print(f"[green]Merged ({len(merged)}): {escape(', '.join(merged))}[/green]") if skipped: - console.print(f"[yellow]Skipped ({len(skipped)}): {', '.join(skipped)}[/yellow]") + console.print(f"[yellow]Skipped ({len(skipped)}): {escape(', '.join(skipped))}[/yellow]") if failed: - console.print(f"[red]Failed ({len(failed)}): {', '.join(failed)}[/red]") + console.print(f"[red]Failed ({len(failed)}): {escape(', '.join(failed))}[/red]") if blocked: console.print( f"[yellow]Blocked by unmerged dependencies ({len(blocked)}): " - f"{', '.join(blocked)}. Re-run once those PRs are merged.[/yellow]" + f"{escape(', '.join(blocked))}. Re-run once those PRs are merged.[/yellow]" + ) + if fetch_errors: + console.print( + f"[red]Could not fetch PR state for ({len(fetch_errors)}): " + f"{', '.join(fetch_errors)}. Check 'gh auth status' and re-run.[/red]" + ) + if closed_while_waiting: + console.print( + f"[red]Closed before auto-merge completed ({len(closed_while_waiting)}): " + f"{', '.join(closed_while_waiting)}. Reopen or recreate them and re-run.[/red]" ) if notify: exit_reason = ( @@ -2238,6 +2601,10 @@ def merge_all( if stopped else "incomplete_batch" if exited_early + else "fetch_error" + if fetch_errors + else "pr_closed" + if closed_while_waiting else "unmerged_dependency" if blocked else "success" @@ -2250,12 +2617,19 @@ def merge_all( "merged": merged, "skipped": skipped_structured, "failed": failed, - "success": not (failed or stopped or exited_early or blocked), + "success": not ( + failed + or stopped + or exited_early + or blocked + or fetch_errors + or closed_while_waiting + ), "exit_reason": exit_reason, }, ) - if failed or stopped or exited_early or blocked: + if failed or stopped or exited_early or blocked or fetch_errors or closed_while_waiting: raise typer.Exit(1) logger.success(f"Merge complete: {len(merged)} PRs merged") diff --git a/pr_split/config.py b/pr_split/config.py index dddd4fe..71ac2fa 100644 --- a/pr_split/config.py +++ b/pr_split/config.py @@ -47,7 +47,7 @@ class Settings(BaseSettings): max_loc: int = Field(default=DEFAULT_MAX_LOC, gt=0) strict_loc_bounds: bool = DEFAULT_STRICT_LOC_BOUNDS max_refinement_iterations: int = Field(default=DEFAULT_MAX_REFINEMENT_ITERATIONS, ge=0) - cp_sat_timeout: float = DEFAULT_CP_SAT_TIMEOUT_SECONDS + cp_sat_timeout: float = Field(default=DEFAULT_CP_SAT_TIMEOUT_SECONDS, gt=0) priority: Priority = Priority.ORTHOGONAL chunk_strategy: ChunkStrategy = DEFAULT_CHUNK_STRATEGY partition_strategy: PartitionStrategy = DEFAULT_PARTITION_STRATEGY diff --git a/pr_split/diff_ops/__init__.py b/pr_split/diff_ops/__init__.py index d57fc55..42794c1 100644 --- a/pr_split/diff_ops/__init__.py +++ b/pr_split/diff_ops/__init__.py @@ -1,5 +1,5 @@ from .parser import ParsedDiff, extract_diff, parse_diff -from .reconstructor import materialize_group_files, merge_chain_assignments +from .reconstructor import materialize_group_files, merge_chain_assignments, target_file_modes __all__ = [ "ParsedDiff", @@ -7,4 +7,5 @@ "materialize_group_files", "merge_chain_assignments", "parse_diff", + "target_file_modes", ] diff --git a/pr_split/diff_ops/parser.py b/pr_split/diff_ops/parser.py index dbfbd28..0f6bc9f 100644 --- a/pr_split/diff_ops/parser.py +++ b/pr_split/diff_ops/parser.py @@ -9,7 +9,7 @@ from unidiff import PatchSet from .. import logs -from ..exceptions import DiffParseError, GitOperationError +from ..exceptions import DiffParseError, ErrorMsg, GitOperationError from ..types_defs import DiffStats, FileSummary, HunkInfo from .new_file_split import split_new_file @@ -23,6 +23,10 @@ # reported under its new name would fail # -U3 and fixed prefixes override diff.context and diff.mnemonicPrefix, # which would shift hunk positions and paths +# --submodule=short with diff.submodule=log|diff a gitlink change +# has no "index ... 160000" header and would slip +# past the submodule check in parse_diff (or show +# up as a bogus vendor/ diff) DIFF_ARGS: tuple[str, ...] = ( "--no-color", "--no-ext-diff", @@ -31,6 +35,7 @@ "-U3", "--src-prefix=a/", "--dst-prefix=b/", + "--submodule=short", ) @@ -39,13 +44,23 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: # Capture bytes: text mode applies universal-newline translation, which # turns CRLF into LF and would make every sub-PR rewrite the file's # line endings. - result = subprocess.run( - # core.quotePath=false: otherwise git octal-escapes and quotes any - # non-ASCII path ("b/caf\303\251.txt"), which unidiff cannot parse for - # new/deleted files and mis-reports as a literal quoted path otherwise. - ["git", "-c", "core.quotePath=false", "diff", *DIFF_ARGS, f"{base_branch}...{dev_branch}"], - capture_output=True, - ) + try: + result = subprocess.run( + # core.quotePath=false: otherwise git octal-escapes and quotes any + # non-ASCII path ("b/caf\303\251.txt"), which unidiff cannot parse for + # new/deleted files and mis-reports as a literal quoted path otherwise. + [ + "git", + "-c", + "core.quotePath=false", + "diff", + *DIFF_ARGS, + f"{base_branch}...{dev_branch}", + ], + capture_output=True, + ) + except FileNotFoundError as exc: + raise GitOperationError(ErrorMsg.TOOL_NOT_FOUND(tool="git")) from exc if result.returncode != 0: raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) # Git diffs any NUL-free file as text, so legacy latin-1 sources reach us @@ -54,6 +69,21 @@ def extract_diff(dev_branch: str, base_branch: str) -> str: return result.stdout.decode("utf-8", errors="surrogateescape") +_GITLINK_MODE = "160000" + + +def _submodule_paths(patch_set: PatchSet) -> list[str]: + """Files whose diff header carries the gitlink mode (a submodule pointer).""" + paths: list[str] = [] + for patch_file in patch_set: + header = "\n".join(patch_file.patch_info or []) + if re.search( + rf"^index [0-9a-f]+\.\.[0-9a-f]+ {_GITLINK_MODE}$", header, re.M + ) or re.search(rf"^(?:new|deleted) file mode {_GITLINK_MODE}$", header, re.M): + paths.append(patch_file.path) + return paths + + _C_ESCAPES = { "a": b"\a", "b": b"\b", @@ -152,6 +182,12 @@ def parse_diff(raw_diff: str, *, split_new_files_over: int | None = None) -> Par file=patch_file.path, loc=patch_file.added, pieces=len(patch_file) ) ) + # A submodule bump looks like a one-hunk text change ("-Subproject + # commit …"), so it would plan and validate fine and only fail with + # "bad object" while materializing, after the user confirmed. + submodules = _submodule_paths(patch_set) + if submodules: + raise DiffParseError(ErrorMsg.SUBMODULE_UNSUPPORTED(paths=", ".join(submodules))) return ParsedDiff(patch_set=patch_set, raw_diff=raw_diff) @@ -192,6 +228,7 @@ def stats(self) -> DiffStats: is_deleted=pf.is_removed_file, is_renamed=pf.is_rename, hunk_count=len(pf), + hunk_indices=list(range(len(pf))), ) ) return DiffStats( diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index b8df46c..82fea99 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -1,5 +1,7 @@ from __future__ import annotations +import re +import stat import subprocess from loguru import logger @@ -7,7 +9,7 @@ from .. import logs from ..constants import AssignmentType -from ..exceptions import GitOperationError +from ..exceptions import ErrorMsg, GitOperationError from ..schemas import Group, GroupAssignment from .parser import ParsedDiff @@ -58,10 +60,13 @@ def merge_chain_assignments( def _get_base_file_content(file_path: str, ref: str) -> str: # Bytes, not text mode: universal newlines would strip the CR from CRLF # files and the reconstructed file would come out with LF endings. - result = subprocess.run( - ["git", "show", f"{ref}:{file_path}"], - capture_output=True, - ) + try: + result = subprocess.run( + ["git", "show", f"{ref}:{file_path}"], + capture_output=True, + ) + except FileNotFoundError as exc: + raise GitOperationError(ErrorMsg.TOOL_NOT_FOUND(tool="git")) from exc if result.returncode != 0: raise GitOperationError(result.stderr.decode("utf-8", errors="replace").strip()) return result.stdout.decode("utf-8", errors="surrogateescape") @@ -149,7 +154,10 @@ def materialize_group_files( patch_file = pf_map.get(file_path) if patch_file is None: continue - if patch_file.is_removed_file: + # unidiff's is_removed_file is a heuristic that is also true for a + # file truncated to empty (single hunk with target length 0); only a + # /dev/null target is a real deletion. + if patch_file.target_file == "/dev/null": result[file_path] = None continue indices = _assigned_hunk_indices(patch_file, assignments) @@ -161,3 +169,37 @@ def materialize_group_files( base_content = _get_base_file_content(file_path, ref) result[file_path] = apply_hunks(base_content, patch_file, indices) return result + + +# "new file mode"/"new mode" appear when the mode changes; an unchanged mode +# is only visible on the "index .. " line. +_TARGET_MODE_RE = re.compile( + r"^(?:new file mode|new mode) (\d{6})$|^index [0-9a-f]+\.\.[0-9a-f]+ (\d{6})$", + re.MULTILINE, +) + + +def target_file_modes(parsed_diff: ParsedDiff, group: Group) -> dict[str, int]: + """Map each file the group materializes to the mode the diff gives it. + + unidiff parses ``old mode``/``new mode``/``new file mode`` and ``index`` + headers into ``patch_info`` only; nothing else applies them, so without + this a ``chmod +x`` that comes with a content change silently loses the + bit and a symlink is written as a regular file. Regular files (100644, + 100755) and symlinks (120000) are reported. + """ + wanted = {assignment.file_path for assignment in group.assignments} + modes: dict[str, int] = {} + for patch_file in parsed_diff.patch_set: + # target_file, not is_removed_file: the latter is also true for a file + # truncated to empty, which still needs its mode applied. + if patch_file.path not in wanted or patch_file.target_file == "/dev/null": + continue + header = "".join(str(line) for line in patch_file.patch_info or []) + match = _TARGET_MODE_RE.search(header) + if not match: + continue + mode = int(match.group(1) or match.group(2), 8) + if stat.S_ISREG(mode) or stat.S_ISLNK(mode): + modes[patch_file.path] = mode + return modes diff --git a/pr_split/exceptions.py b/pr_split/exceptions.py index 55ac79c..189e310 100644 --- a/pr_split/exceptions.py +++ b/pr_split/exceptions.py @@ -15,9 +15,11 @@ class ErrorMsg(StrEnum): ) DIRTY_WORKTREE = "Working tree has uncommitted changes; commit or stash first" GH_AUTH_FAILED = "GitHub CLI authentication failed; run 'gh auth login'" + TOOL_NOT_FOUND = "'{tool}' is not installed or not on PATH" CYCLE_DETECTED = "Dependency cycle detected in split plan" COVERAGE_GAP = "Hunk {file}[{index}] not assigned to any group" COVERAGE_OVERLAP = "Hunk {file}[{index}] assigned to multiple groups: {groups}" + COVERAGE_DUPLICATE = "Hunk {file}[{index}] listed more than once in group '{group}'" UNKNOWN_HUNK = "Hunk {file}[{index}] assigned to group '{group}' does not exist in the diff" UNKNOWN_FILE = "File '{file}' assigned to group '{group}' does not exist in the diff" UNKNOWN_DEPENDENCY = "Group '{group}' depends on unknown group '{dep}'" @@ -30,6 +32,8 @@ class ErrorMsg(StrEnum): LOC_MISMATCH = "Total LOC {actual} does not match diff LOC {expected}" MERGE_CONFLICT = "Groups '{a}' and '{b}' modify overlapping regions in '{file}'" NO_PLAN = "No split plan found; run 'pr-split split' first" + NO_CHANGES = "No changes between '{base}' and '{dev}'; nothing to split" + PLAN_HAS_NO_CHANGES = "Saved plan has an empty diff; nothing to execute" PLAN_AMBIGUOUS = ( "Several split plans are saved ({branches}); pick one with" " 'pr-split --branch '" @@ -117,15 +121,28 @@ class ErrorMsg(StrEnum): HUNK_TOO_LARGE = "Hunk {file}[{index}] has ~{tokens} estimated tokens, exceeds budget {budget}" MIN_LOC_GE_MAX_LOC = "min_loc {min_loc} must be less than max_loc {max_loc}" LOC_BOUNDS_STRICT_FAILED = "Plan violates configured LOC bounds" + CP_SAT_TIMED_OUT = ( + "CP-SAT found no hunk assignment within {timeout:g}s ({units} units); " + "raise --cp-sat-timeout or use --partition-strategy graph" + ) + CP_SAT_INFEASIBLE = "CP-SAT partitioning failed to find a feasible hunk assignment" BINARY_FILES_UNSUPPORTED = ( "Diff contains binary files, which cannot be split into hunks: {files}." " Commit them separately and re-run" ) + HUNKLESS_FILES_UNSUPPORTED = ( + "Diff contains files with no text hunks (mode-only or empty changes), which" + " cannot be assigned to a sub-PR: {files}. Commit them separately and re-run" + ) GH_STACK_MISSING = ( "The gh-stack extension is required for stacked PRs;" " run 'gh extension install github/gh-stack'" ) STACK_LINK_FAILED = "Failed to link stack for PRs {prs}: {detail}" + SUBMODULE_UNSUPPORTED = ( + "Submodule changes are not supported (pointer bump at {paths}); " + "split them out of the branch first" + ) def __call__(self, **kwargs: object) -> str: return self.value.format(**kwargs) if kwargs else self.value diff --git a/pr_split/git_ops/__init__.py b/pr_split/git_ops/__init__.py index 2aae56d..0e076c0 100644 --- a/pr_split/git_ops/__init__.py +++ b/pr_split/git_ops/__init__.py @@ -25,6 +25,9 @@ from .branches import ( merge_base as merge_base, ) +from .branches import ( + prune_remote_tracking_refs as prune_remote_tracking_refs, +) from .branches import ( push_branch as push_branch, ) diff --git a/pr_split/git_ops/branches.py b/pr_split/git_ops/branches.py index 6a86bc9..8fa03e0 100644 --- a/pr_split/git_ops/branches.py +++ b/pr_split/git_ops/branches.py @@ -1,15 +1,21 @@ from __future__ import annotations +import atexit +import contextlib +import functools +import hashlib import os import re +import shutil import subprocess +import tempfile import time from loguru import logger from .. import logs from ..constants import PLAN_DIR -from ..exceptions import GitOperationError +from ..exceptions import ErrorMsg, GitOperationError def _git_env() -> dict[str, str]: @@ -18,29 +24,36 @@ def _git_env() -> dict[str, str]: return {**os.environ, "LC_ALL": "C", "LANGUAGE": "C"} -def run_git(*args: str) -> str: - result = subprocess.run( - ["git", *args], - capture_output=True, - text=True, - env=_git_env(), - ) +def _git(args: tuple[str, ...], cwd: str | None = None) -> str: + try: + result = subprocess.run( + ["git", *args], + capture_output=True, + text=True, + cwd=cwd, + env=_git_env(), + ) + except FileNotFoundError as exc: + raise GitOperationError(ErrorMsg.TOOL_NOT_FOUND(tool="git")) from exc if result.returncode != 0: raise GitOperationError(result.stderr.strip()) return result.stdout.strip() +def run_git(*args: str) -> str: + return _git(args) + + def run_git_in_dir(cwd: str, *args: str) -> str: - result = subprocess.run( - ["git", *args], - capture_output=True, - text=True, - cwd=cwd, - env=_git_env(), - ) - if result.returncode != 0: - raise GitOperationError(result.stderr.strip()) - return result.stdout.strip() + return _git(args, cwd=cwd) + + +def require_tools(*tools: str) -> str | None: + """Return the first of ``tools`` that is not on PATH, or None.""" + for tool in tools: + if shutil.which(tool) is None: + return tool + return None def commit_exists(ref: str) -> bool: @@ -171,10 +184,36 @@ def delete_branch(branch: str, *, remote: bool = False) -> None: if _REMOTE_REF_MISSING not in str(exc): raise logger.info(logs.BRANCH_ALREADY_GONE.format(branch=branch, where=" on origin")) + # Either way the remote branch is gone; drop the local tracking ref + # too, or the next `push --force-with-lease` of a reused branch name + # is rejected as "stale info" against a ref origin no longer has. + forget_remote_tracking_ref(branch) if local_error is not None: raise local_error +def forget_remote_tracking_ref(branch: str) -> None: + with contextlib.suppress(GitOperationError): + run_git("update-ref", "-d", f"refs/remotes/origin/{branch}") + + +def prune_remote_tracking_refs() -> None: + """Drop tracking refs whose remote branch no longer exists. + + Split branch names are reused across runs; after a merge or `clean` the + remote branch is gone but `refs/remotes/origin/pr-split/...` may still + point at the old head, and every `push --force-with-lease` would be + rejected as stale. Only *gone* refs are dropped (`git remote prune`, no + fetch): refreshing the refs that still exist would make the lease + compare against origin's current tip and silently force over commits + someone else pushed to a reused branch. + """ + try: + run_git("remote", "prune", "origin") + except GitOperationError as exc: + logger.warning(logs.PRUNE_FAILED.format(error=exc)) + + def merge_base(ref_a: str, ref_b: str) -> str: return run_git("merge-base", ref_a, ref_b) @@ -218,24 +257,116 @@ def diff_base_ref(base: str) -> str: def derive_split_namespace(dev_branch_arg: str) -> str: - raw = dev_branch_arg.split(":", 1)[1] if ":" in dev_branch_arg else dev_branch_arg.lstrip("#") + """Derive the branch namespace for a split from its dev-branch argument. + + The result must be a valid git ref component and distinct per source: + ``user:branch`` keeps the user so a fork branch cannot collide with a + local branch of the same name, ``..`` is never emitted, and an argument + that sanitises to nothing falls back to a short hash rather than an + empty component (``pr-split//pr-1`` is not a valid ref). + """ + if ":" in dev_branch_arg: + user, branch = dev_branch_arg.split(":", 1) + raw = f"{user}-{branch}" + else: + raw = dev_branch_arg.lstrip("#") sanitized = re.sub(r"[^a-zA-Z0-9._-]", "-", raw) - return sanitized.strip("-") + sanitized = re.sub(r"\.{2,}", "-", sanitized).strip("-.") + if not sanitized: + return hashlib.sha1(dev_branch_arg.encode("utf-8")).hexdigest()[:8] + return sanitized + + +def local_branch_exists(branch: str) -> bool: + """True only for a local branch head (``branch_exists`` also matches tags/SHAs).""" + return branch_exists(f"refs/heads/{branch}") + + +def _registered_worktrees() -> list[tuple[str, str | None]]: + """(realpath, branch ref or None) for every registered worktree, main first.""" + try: + listing = run_git("worktree", "list", "--porcelain") + except GitOperationError: + return [] + entries: list[tuple[str, str | None]] = [] + path: str | None = None + branch: str | None = None + for line in [*listing.splitlines(), ""]: + if line.startswith("worktree "): + path = line[len("worktree ") :] + elif line.startswith("branch "): + branch = line[len("branch ") :] + elif not line and path is not None: + entries.append((os.path.realpath(path), branch)) + path, branch = None, None + return entries + + +def _forget_stale_worktrees(path: str, branch_name: str) -> None: + """Drop worktree registrations a killed run left behind. + + ``git branch -D`` refuses a branch that a registered worktree still has + checked out. A run killed mid-way leaves its temporary worktree (and its + registration) in place, so the next run could never reuse the branch + without a manual ``git worktree prune``/``remove``. Registrations that + hold this branch, or sit at the requested path, are removed; the main + worktree is never touched. + """ + run_git("worktree", "prune") + entries = _registered_worktrees() + if not entries: + return + main_path = entries[0][0] + wanted_path = os.path.realpath(path) + wanted_ref = f"refs/heads/{branch_name}" + for registered_path, ref in entries[1:]: + if registered_path == main_path: + continue + if ref == wanted_ref or registered_path == wanted_path: + # Double --force also overrides the "initializing" lock a run + # killed in the middle of `git worktree add` leaves behind. + run_git("worktree", "remove", "--force", "--force", registered_path) def add_worktree(path: str, branch_name: str, start_point: str) -> None: + _forget_stale_worktrees(path, branch_name) prev_sha: str | None = None - if branch_exists(branch_name): - prev_sha = run_git("rev-parse", branch_name) + if local_branch_exists(branch_name): + prev_sha = run_git("rev-parse", f"refs/heads/{branch_name}") run_git("branch", "-D", branch_name) try: - run_git("worktree", "add", "-b", branch_name, path, start_point) + # A repository post-checkout hook (husky, lint-staged installers) + # runs inside the throwaway worktree, where it has no toolchain and + # can only fail; pointing hooksPath at an empty directory for this + # one command disables it. + run_git( + "-c", + f"core.hooksPath={_no_hooks_dir()}", + "worktree", + "add", + "-b", + branch_name, + path, + start_point, + ) except GitOperationError: if prev_sha is not None: run_git("branch", branch_name, prev_sha) raise +@functools.cache +def _no_hooks_dir() -> str: + """An empty directory to use as core.hooksPath (no hooks run). + + mkdtemp creates it private to this user, so nobody else can plant hooks + in it the way they could in a fixed, shared path under the temp dir. + """ + path = tempfile.mkdtemp(prefix="pr-split-no-hooks-") + atexit.register(shutil.rmtree, path, ignore_errors=True) + return path + + def remove_worktree(path: str) -> None: run_git("worktree", "remove", "--force", path) @@ -245,7 +376,11 @@ def commit_files_in_dir( ) -> str: if not file_paths: raise GitOperationError("commit_files_in_dir called with no file paths") - run_git_in_dir(cwd, "add", "-A", "--", *file_paths) + # -f: the dev branch may track a file that matches .gitignore (added + # with `git add -f`); the diff materialises it, and without -f `git add` + # refuses the path and the whole group fails. The path list is explicit, + # so -f cannot pull in anything unintended; -A still stages deletions. + run_git_in_dir(cwd, "add", "-A", "-f", "--", *file_paths) author_args = ("--author", author) if author else () # The content is a subset of commits already accepted on the dev # branch; a pre-commit/commit-msg hook (husky, pre-commit, lint-staged) diff --git a/pr_split/git_ops/prs.py b/pr_split/git_ops/prs.py index f784173..9ccab01 100644 --- a/pr_split/git_ops/prs.py +++ b/pr_split/git_ops/prs.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import os import re import subprocess from collections.abc import Sequence @@ -11,22 +12,92 @@ from ..constants import FORK_REF_PREFIX, PR_REF_PREFIX from ..exceptions import ErrorMsg, GitOperationError from ..types_defs import ForkPRInfo +from .branches import run_git def _run_gh(*args: str) -> str: - result = subprocess.run( - ["gh", *args], - capture_output=True, - text=True, - ) + try: + result = subprocess.run( + ["gh", *args], + capture_output=True, + text=True, + ) + except FileNotFoundError as exc: + # Callers only expect GitOperationError; a missing binary must not + # escape as a raw traceback (or make check_gh_auth raise). + raise GitOperationError(ErrorMsg.TOOL_NOT_FOUND(tool="gh")) from exc if result.returncode != 0: raise GitOperationError(result.stderr.strip()) return result.stdout.strip() +PR_BODY_MAX_CHARS = 65_536 +_PR_BODY_TRUNCATION_NOTE = "\n\n_(body truncated: GitHub's limit is 65,536 characters)_" + + +def _truncate_pr_body(body: str) -> str: + """Keep the body under GitHub's hard limit so PR creation cannot fail on it. + + The cut lands on a line boundary and closes an open code fence (the + dependency graph is the last section of every generated body), so the + truncation note renders as a note rather than as code text. + """ + if len(body) <= PR_BODY_MAX_CHARS: + return body + fence_close = "\n```" + keep = PR_BODY_MAX_CHARS - len(_PR_BODY_TRUNCATION_NOTE) - len(fence_close) + cut = body[:keep] + newline = cut.rfind("\n") + if newline > 0: + cut = cut[:newline] + if cut.count("```") % 2 == 1: + cut += fence_close + return cut + _PR_BODY_TRUNCATION_NOTE + + +# Matches the host of an https/ssh/git URL or an scp-like ``user@host:path``. +# Local paths and file:// URLs deliberately do not match: they have no host +# for gh to authenticate against. A dot is required only in the bare +# ``host:path`` form, where it separates real hosts from Windows drives and +# relative paths; scheme and ``user@`` forms accept single-label hosts +# (``git@ghe:org/repo``), which gh itself supports. +_REMOTE_HOST_RE = re.compile( + r"^(?:" + r"(?:https?|ssh|git|git\+ssh|ssh\+git)://(?:[^@/]+@)?(?P[^.:/@][^:/@]*)" + r"|[^@/:]+@(?P[^.:/@][^:/@]*)" + r"|(?P[^.:/@][^:/@]*\.[^:/@]+)" + r")(?::\d+)?[:/]" +) + + +def gh_host() -> str: + """The GitHub host pr-split talks to, resolved the way gh does. + + gh targets the host of the repository's remote and only treats GH_HOST + as an override, so an enterprise-only checkout must be checked against + its own host, not github.com. + """ + override = os.environ.get("GH_HOST") + if override: + return override.lower() + try: + remote = run_git("remote", "get-url", "origin") + except GitOperationError: + return "github.com" + match = _REMOTE_HOST_RE.match(remote.strip()) + if not match: + return "github.com" + host = match.group("scheme_host") or match.group("scp_host") or match.group("bare_host") + # gh lowercases hosts it parses from remotes; `--hostname` is case-sensitive. + return host.lower() + + def check_gh_auth() -> bool: + # Without --hostname, `gh auth status` exits 1 when *any* configured + # host is unauthenticated (a stale enterprise token), even though every + # call pr-split makes targets one host. try: - _run_gh("auth", "status") + _run_gh("auth", "status", "--hostname", gh_host()) except GitOperationError: return False return True @@ -77,6 +148,7 @@ def find_open_pr(branch: str) -> tuple[int, str] | None: def create_pr( head: str, base: str, title: str, body: str, *, draft: bool = False ) -> tuple[int, str]: + body = _truncate_pr_body(body) args = [ "pr", "create", @@ -148,6 +220,26 @@ def merge_pr(pr_number: int, *, auto: bool = False) -> None: logger.info(f"{'Queued' if auto else 'Merged'} PR #{pr_number}") +_STACKED_PR_EDIT_REFUSED = "part of a stack" + + +def retarget_pr(pr_number: int, base: str) -> bool: + """Point a stacked child PR at ``base`` once its parent has merged. + + Returns False when GitHub refuses because the PR is in a native stack; + GitHub retargets those itself when the parent PR merges. + """ + try: + set_pr_base(pr_number, base) + except GitOperationError as exc: + if _STACKED_PR_EDIT_REFUSED in str(exc).lower(): + logger.info(logs.PR_RETARGET_NATIVE_STACK.format(number=pr_number)) + return False + raise + logger.info(logs.PR_RETARGETED.format(number=pr_number, base=base)) + return True + + def close_pr(pr_number: int) -> None: _run_gh("pr", "close", str(pr_number)) logger.info(logs.PR_CLOSED.format(number=pr_number)) @@ -264,7 +356,7 @@ def fetch_fork_branch(user: str, branch: str) -> ForkPRInfo: logger.info(logs.FETCHING_FORK_BRANCH.format(branch=branch, fork=fork_full_name)) try: - run_git("fetch", clone_url, f"{branch}:{local_ref}") + run_git("fetch", clone_url, f"+{branch}:{local_ref}") except GitOperationError as exc: raise GitOperationError( ErrorMsg.FORK_FETCH_FAILED(user=user, branch=branch, detail=str(exc)) diff --git a/pr_split/logs.py b/pr_split/logs.py index a338046..f767a24 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -103,6 +103,7 @@ " the diff will be sent to that host" ) LLM_OUTPUT_INCOMPLETE = "LLM output incomplete (status: {status}, reason: {reason})" +LLM_UNEXPECTED_STOP = "LLM stopped with stop_reason={stop_reason} (keys in tool input: {keys})" CHUNK_RETRY = "Chunk {index}/{total} failed (attempt {attempt}), retrying: {error}" INVALID_HUNK_INDEX = ( "Group '{group}': invalid hunk index {index} for {file} (max: {max}), skipping" @@ -144,6 +145,9 @@ PR_SKIPPED_BASE_NOT_PUSHED = ( "Skipping PR for group '{group}': its base branch '{base}' was not pushed" ) +PR_SKIPPED_BASE_PR_FAILED = ( + "Skipping PR for {group}: no PR could be opened for its base branch {base}" +) CP_SAT_NOT_OPTIMAL = ( "CP-SAT stopped at the {timeout:g}s limit with a feasible but unproven-optimal plan " "({units} units, {groups} groups); raise --cp-sat-timeout for a better partition" @@ -162,3 +166,11 @@ " its sub-PR will not compile or import on its own" ) ADOPTED_REMOTE_BRANCH = "Created local branch {branch} from {remote_ref}" +PRUNE_FAILED = ( + "Could not prune origin's tracking refs before pushing ({error}); " + "a reused branch name may be rejected as stale" +) +PR_RETARGETED = "Retargeted PR #{number} at {base} before merging" +PR_RETARGET_NATIVE_STACK = ( + "PR #{number} is in a native stack; GitHub retargets it when its parent merges" +) diff --git a/pr_split/planner/chunker.py b/pr_split/planner/chunker.py index 734f42a..93c118f 100644 --- a/pr_split/planner/chunker.py +++ b/pr_split/planner/chunker.py @@ -62,13 +62,23 @@ def _chunk_slack_penalty(used_tokens: int, token_budget: int) -> float: return (slack * slack) / max(1, token_budget) -def _chunk_boundary_penalty(hunk_sequence: list[HunkRef], boundary_index: int) -> float: +def _same_file_boundary_penalty(token_budget: int) -> float: + # The slack penalty can reach token_budget per chunk, so a fixed penalty + # is dwarfed at real budgets (hundreds of thousands of tokens) and the + # DP happily cuts a file in half to balance chunk sizes. Make cutting a + # file always cost more than any slack it could save. + return max(_DP_SAME_FILE_BOUNDARY_PENALTY, 2.0 * token_budget) + + +def _chunk_boundary_penalty( + hunk_sequence: list[HunkRef], boundary_index: int, token_budget: int +) -> float: if boundary_index >= len(hunk_sequence) - 1: return 0.0 left = hunk_sequence[boundary_index] right = hunk_sequence[boundary_index + 1] if left.file_path == right.file_path: - return _DP_SAME_FILE_BOUNDARY_PENALTY + return _same_file_boundary_penalty(token_budget) return 0.0 @@ -109,7 +119,7 @@ def chunk_hunks_dynamic_programming( _DP_CHUNK_BASE_COST + _chunk_slack_penalty(used_tokens, token_budget) + file_mix_penalty - + _chunk_boundary_penalty(hunk_sequence, end - 1) + + _chunk_boundary_penalty(hunk_sequence, end - 1, token_budget) ) candidate_cost = best_cost[start - 1] + chunk_cost if candidate_cost < best_cost[end]: @@ -184,6 +194,7 @@ def build_chunk_stats_from_hunks(parsed_diff: ParsedDiff, hunk_refs: list[HunkRe is_deleted=pf.is_removed_file, is_renamed=pf.is_rename, hunk_count=len(indices), + hunk_indices=sorted(indices), ) ) return DiffStats( diff --git a/pr_split/planner/client.py b/pr_split/planner/client.py index 0288b4d..2b1ccd4 100644 --- a/pr_split/planner/client.py +++ b/pr_split/planner/client.py @@ -181,9 +181,12 @@ def _call_anthropic(system: str, user: str, *, settings: Settings) -> RawToolOut if isinstance(block, BetaToolUseBlock) and isinstance(block.input, dict): keys = list(block.input.keys()) break - logger.warning(logs.LLM_OUTPUT_TRUNCATED.format(stop_reason=stop_reason, keys=keys)) if stop_reason == "max_tokens": + logger.warning(logs.LLM_OUTPUT_TRUNCATED.format(stop_reason=stop_reason, keys=keys)) raise LLMError(ErrorMsg.LLM_OUTPUT_TRUNCATED(detail=f"stop_reason={stop_reason}")) + # Any other stop reason is not a truncation; if a tool block is + # present the plan is complete, otherwise the loop below reports it. + logger.debug(logs.LLM_UNEXPECTED_STOP.format(stop_reason=stop_reason, keys=keys)) for block in response.content: if isinstance(block, BetaToolUseBlock) and block.name == SPLIT_TOOL_NAME: return RawToolOutput(groups=_extract_raw_output(block.input)) @@ -203,6 +206,10 @@ def _call_openai(system: str, user: str, *, settings: Settings) -> RawToolOutput except openai.APIError as exc: raise LLMError(ErrorMsg.LLM_PARSE_ERROR(detail=str(exc))) from exc status = getattr(response, "status", None) + if status == "failed": + error = getattr(response, "error", None) + detail = getattr(error, "message", None) or getattr(error, "code", None) or "unknown error" + raise LLMError(ErrorMsg.LLM_PARSE_ERROR(detail=f"response failed: {detail}")) if status == "incomplete": details = getattr(response, "incomplete_details", None) reason = getattr(details, "reason", None) or "unknown" @@ -685,7 +692,7 @@ def _plan_split_with_llm( ) -> list[Group]: diff_stats = parsed_diff.stats - system = build_system_prompt(settings.priority, settings.max_loc) + system = build_system_prompt(settings.priority, settings.max_loc, settings.min_loc) user = build_user_prompt(diff_stats, parsed_diff.labeled_diff) logger.info(logs.COUNTING_TOKENS.format(model=settings.model)) diff --git a/pr_split/planner/partitioning.py b/pr_split/planner/partitioning.py index d7bbc82..4248baa 100644 --- a/pr_split/planner/partitioning.py +++ b/pr_split/planner/partitioning.py @@ -12,7 +12,7 @@ from .. import logs from ..constants import AssignmentType, PartitionStrategy, Priority -from ..exceptions import PRSplitError +from ..exceptions import ErrorMsg, PRSplitError from ..graph import PlanDAG from ..schemas import Group, GroupAssignment from .chunker import recompute_estimated_loc @@ -481,8 +481,14 @@ def _group_units_cp_sat( solver = cp_model.CpSolver() solver.parameters.max_time_in_seconds = settings.cp_sat_timeout status = solver.Solve(model) + if status == cp_model.UNKNOWN: + # The time limit expired before any solution was found; that is not + # infeasibility, and the fix (more time or another backend) differs. + raise PRSplitError( + ErrorMsg.CP_SAT_TIMED_OUT(timeout=settings.cp_sat_timeout, units=len(units)) + ) if status not in (cp_model.OPTIMAL, cp_model.FEASIBLE): - raise PRSplitError("CP-SAT partitioning failed to find a feasible hunk assignment") + raise PRSplitError(ErrorMsg.CP_SAT_INFEASIBLE()) assignments: dict[int, list[PartitionUnit]] = defaultdict(list) for unit_idx, unit in enumerate(units): diff --git a/pr_split/planner/prompts.py b/pr_split/planner/prompts.py index 0e75283..68b9dea 100644 --- a/pr_split/planner/prompts.py +++ b/pr_split/planner/prompts.py @@ -77,7 +77,7 @@ No cycles are allowed. 3. Each group should stay within approximately {max_loc} lines of code \ (added + removed). Exceeding this is acceptable only when a logical unit cannot \ -be split further. +be split further.{min_loc_rule} 4. Use the propose_split_plan tool to return your plan. 5. PR titles MUST follow conventional commits format: \ type(optional-scope): description. Allowed types: feat, fix, refactor, test, \ @@ -209,7 +209,17 @@ def _format_file_summary(diff_stats: DiffStats) -> str: ] flag_str = f" [{', '.join(flags)}]" if flags else "" hunk_count = fs["hunk_count"] - idx_range = f"indices 0..{hunk_count - 1}" if hunk_count > 0 else "no hunks" + indices = fs["hunk_indices"] + # In chunked mode a file's hunks in this chunk keep their per-file + # global indices, which need not start at 0 or be contiguous; list + # them explicitly so the model does not re-use indices from earlier + # chunks. + if not indices: + idx_range = "no hunks" + elif indices == list(range(len(indices))): + idx_range = f"indices 0..{indices[-1]}" + else: + idx_range = f"indices {', '.join(str(i) for i in indices)} (this chunk only)" lines.append( f" {fs['path']}: +{fs['added']}/-{fs['removed']}" f" ({hunk_count} hunks, {idx_range}){flag_str}" @@ -228,9 +238,20 @@ def _format_file_summary(diff_stats: DiffStats) -> str: } -def build_system_prompt(priority: Priority, max_loc: int) -> str: +_MIN_LOC_RULE = ( + " Groups should also not be smaller than about {min_loc} lines: merge " + "closely related small changes rather than creating tiny groups, unless " + "a change genuinely stands alone." +) + + +def build_system_prompt(priority: Priority, max_loc: int, min_loc: int | None = None) -> str: + # --min-loc was documented as applying to every backend, but the LLM only + # ever heard about it via the (off by default) refinement prompt. + min_loc_rule = _MIN_LOC_RULE.format(min_loc=min_loc) if min_loc else "" return _SYSTEM_PROMPT_TEMPLATE.format( max_loc=max_loc, + min_loc_rule=min_loc_rule, priority_instructions=_PRIORITY_MAP[priority], ) diff --git a/pr_split/planner/validator.py b/pr_split/planner/validator.py index bd4c3b2..aaab5fa 100644 --- a/pr_split/planner/validator.py +++ b/pr_split/planner/validator.py @@ -13,15 +13,20 @@ def validate_no_binary_files(parsed_diff: ParsedDiff) -> None: - """Refuse diffs with binary files. + """Refuse diffs with binary files or hunk-less entries. - Binary patches carry no hunks, so coverage validation would pass - without any group claiming them and the change would silently be - dropped from every sub-PR. + Binary patches and mode-only changes carry no hunks, so coverage + validation would pass without any group claiming them and the change + would silently be dropped from every sub-PR. """ binary = [pf.path for pf in parsed_diff.patch_set if pf.is_binary_file] if binary: raise PlanValidationError(ErrorMsg.BINARY_FILES_UNSUPPORTED(files=", ".join(binary))) + # Mode-only changes (chmod +x) and other hunk-less entries have the same + # failure mode: nothing for a group to claim, so the change vanishes. + hunkless = [pf.path for pf in parsed_diff.patch_set if len(pf) == 0] + if hunkless: + raise PlanValidationError(ErrorMsg.HUNKLESS_FILES_UNSUPPORTED(files=", ".join(hunkless))) def validate_coverage(groups: list[Group], parsed_diff: ParsedDiff) -> None: @@ -41,7 +46,14 @@ def validate_coverage(groups: list[Group], parsed_diff: ParsedDiff) -> None: indices = assignment.hunk_indices for idx in indices: key = (assignment.file_path, idx) - assigned.setdefault(key, []).append(group.id) + owners = assigned.setdefault(key, []) + if group.id in owners: + # The same group claiming a hunk twice is a malformed + # plan, not an overlap between groups. + raise PlanValidationError( + ErrorMsg.COVERAGE_DUPLICATE(file=key[0], index=key[1], group=group.id) + ) + owners.append(group.id) all_hunks = {(pf.path, i) for pf in parsed_diff.patch_set for i in range(len(pf))} diff --git a/pr_split/types_defs.py b/pr_split/types_defs.py index aa61a66..a4f3134 100644 --- a/pr_split/types_defs.py +++ b/pr_split/types_defs.py @@ -23,6 +23,7 @@ class FileSummary(TypedDict): is_deleted: bool is_renamed: bool hunk_count: int + hunk_indices: list[int] class ForkPRInfo(TypedDict): diff --git a/scripts/score_pr.py b/scripts/score_pr.py index f415751..ef44242 100644 --- a/scripts/score_pr.py +++ b/scripts/score_pr.py @@ -8,6 +8,7 @@ import sys import tempfile from pathlib import Path +from urllib.parse import quote def _run(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess[str]: @@ -24,11 +25,26 @@ def _set_output(name: str, value: str) -> None: f.write(f"{name}={value}\n") -def _skip(reason: str) -> None: +def _skip(reason: str, *, within_limits: bool = False) -> None: print(reason) _set_output("total_groups", "1") _set_output("objective", "0") _set_output("should_split", "false") + if within_limits: + # Emit a comment body so an earlier "please split" comment on this + # PR is refreshed once it shrinks under the threshold (the action + # only updates an existing marker comment when should_split is + # false; it never creates one). + _write_comment( + [ + "", + "## pr-split analysis", + "", + reason, + "", + "This PR is within acceptable size limits.", + ] + ) def _write_comment(lines: list[str]) -> None: @@ -70,7 +86,8 @@ def _oversized_without_plan( def _md_escape(s: str) -> str: - return s.replace("|", "\\|") + # A table cell must stay on one line and must not contain a bare pipe. + return " ".join(s.splitlines()).replace("|", "\\|") def load_plan_groups(plan_path: str) -> list[dict]: @@ -85,6 +102,20 @@ def load_plan_groups(plan_path: str) -> list[dict]: return plan.get("groups", []) +def _saved_plan_path(dev_branch: str) -> str | None: + """The plan file ``pr-split split `` saved, if any. + + pr-split keeps one plan per dev branch under ``.pr-split/plans/``, named + by the branch percent-encoded (``pr_split.plan_store.plan_key``); older + versions wrote a single ``.pr-split/plan.json``. + """ + per_branch = os.path.join(".pr-split", "plans", f"{quote(dev_branch, safe='')}.json") + for path in (per_branch, os.path.join(".pr-split", "plan.json")): + if os.path.exists(path): + return path + return None + + def _parse_int_env(name: str, default: int) -> int: raw = os.environ.get(name, str(default)) try: @@ -97,6 +128,10 @@ def _parse_int_env(name: str, default: int) -> int: def main() -> None: max_loc = _parse_int_env("MAX_LOC", 400) min_loc_raw = os.environ.get("MIN_LOC", "") + if min_loc_raw: + # Validate like MAX_LOC so a typo is reported as such instead of as + # a generic "pr-split failed to generate a plan". + min_loc_raw = str(_parse_int_env("MIN_LOC", 0)) strategy = os.environ.get("PARTITION_STRATEGY", "graph") priority = os.environ.get("PRIORITY", "orthogonal") threshold = _parse_int_env("THRESHOLD_GROUPS", 2) @@ -139,18 +174,24 @@ def main() -> None: _set_output("total_loc", str(total_loc)) if total_loc <= max_loc: - _skip(f"PR has {total_loc} LOC — under the {max_loc} threshold, no split needed.") + _skip( + f"PR has {total_loc} LOC — under the {max_loc} threshold, no split needed.", + within_limits=True, + ) return - # Create local branch refs for pr-split + # Create local branch refs for pr-split. `split` needs the base as a local + # branch, but the head is passed as the fetched ref: materialising it as + # a branch named after head_branch is unsafe, since a fork PR opened from + # the fork's "main" has head_branch == base_branch and would clobber the + # base ref, leaving an empty diff. _run(["git", "branch", "-f", base_branch, f"origin/{base_branch}"]) - _run(["git", "branch", "-f", head_branch, local_head]) # Run pr-split in dry-run mode cmd = [ "pr-split", "split", - head_branch, + local_head, "--base", base_branch, "--partition-strategy", @@ -175,8 +216,8 @@ def main() -> None: ) return - plan_path = ".pr-split/plan.json" - if not os.path.exists(plan_path): + plan_path = _saved_plan_path(local_head) + if plan_path is None: _oversized_without_plan( "no split plan could be generated: pr-split wrote no plan file.", total_loc, @@ -240,7 +281,7 @@ def main() -> None: lines.append("|-------|-------|------|------------|-------|") for g in groups: files = ", ".join(f"`{_md_escape(a['file_path'])}`" for a in g.get("assignments", [])) - deps = ", ".join(g.get("depends_on", [])) or "—" + deps = ", ".join(_md_escape(d) for d in g.get("depends_on", [])) or "—" diff_str = f"+{g.get('estimated_added', 0)}/-{g.get('estimated_removed', 0)}" title = _md_escape(g["title"]) gid = _md_escape(g["id"]) diff --git a/tests/conftest.py b/tests/conftest.py index 02acbe4..7c8ef40 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,3 +8,11 @@ def _no_live_base_merge_lookup(monkeypatch: pytest.MonkeyPatch) -> None: """status and merge ask GitHub whether the plan's base branch has merged; keep that off the network in tests. Tests that exercise it patch it again.""" monkeypatch.setattr("pr_split.cli._base_has_merged", lambda base: False) + + +@pytest.fixture(autouse=True) +def _cli_tools_present(monkeypatch: pytest.MonkeyPatch) -> None: + """The CLI checks that git and gh are on PATH before doing anything; the + tests mock every git/gh call, so they must not depend on the machine + having gh installed. Tests of the missing-tool path patch it again.""" + monkeypatch.setattr("pr_split.cli.require_tools", lambda *tools: None) diff --git a/tests/network_guards.py b/tests/network_guards.py index 15df9ca..54b7fc7 100644 --- a/tests/network_guards.py +++ b/tests/network_guards.py @@ -13,6 +13,14 @@ def _no_base_fetch() -> Iterator[None]: yield +@pytest.fixture(autouse=True) +def _no_remote_prune() -> Iterator[None]: + """Pushing prunes origin's tracking refs first; keep that off the network + (and off this checkout's refs). Tests that exercise it patch it again.""" + with patch("pr_split.cli.prune_remote_tracking_refs"): + yield + + @pytest.fixture(autouse=True) def _no_selected_plan() -> Iterator[None]: """A command's plan selection is process-wide; start each test without one.""" diff --git a/tests/test_chunker.py b/tests/test_chunker.py index 4b1b461..445428d 100644 --- a/tests/test_chunker.py +++ b/tests/test_chunker.py @@ -162,6 +162,29 @@ def test_prefers_not_to_split_same_file(self) -> None: assert [ref.file_path for ref in chunks[0]] == ["a.py"] assert [ref.file_path for ref in chunks[1]] == ["b.py", "b.py"] + def test_large_budget_still_keeps_files_whole_when_possible(self) -> None: + # At a realistic budget the slack term is hundreds of thousands of + # tokens; a fixed same-file penalty used to lose to it and b.py was + # cut in half although a two-chunk plan without any cut exists. + budget = 538_000 + refs = [HunkRef("a.py", 0, 10_000)] + refs += [HunkRef("b.py", i, 50_000) for i in range(10)] + refs += [HunkRef("c.py", i, 50_000) for i in range(2)] + chunks = chunk_hunks_dynamic_programming(refs, budget) + assert len(chunks) == 2 + for chunk in chunks: + assert sum(r.token_estimate for r in chunk) <= budget + # no file appears in more than one chunk + seen: dict[str, int] = {} + for idx, chunk in enumerate(chunks): + for ref in chunk: + assert seen.setdefault(ref.file_path, idx) == idx + + def test_file_is_still_cut_when_it_cannot_fit_one_chunk(self) -> None: + refs = [HunkRef("big.py", i, 60) for i in range(3)] + chunks = chunk_hunks_dynamic_programming(refs, 100) + assert [len(c) for c in chunks] == [1, 1, 1] + def test_greedy_helper_retains_previous_behavior(self) -> None: refs = [ HunkRef("a.py", 0, 30), @@ -362,3 +385,22 @@ def test_oversized_hunk_is_pr_split_error_for_both_strategies(self) -> None: for strategy in (ChunkStrategy.GREEDY, ChunkStrategy.DYNAMIC_PROGRAMMING): with pytest.raises(PRSplitError, match=r"a\.py\[0\].*exceeds budget 100"): chunk_hunks(refs, 100, strategy) + + +class TestChunkStatsKeepGlobalIndices: + def test_second_chunk_summary_uses_per_file_global_indices(self) -> None: + from pr_split.diff_ops.parser import parse_diff + from pr_split.planner.chunker import build_chunk_stats_from_hunks, build_hunk_sequence + from pr_split.planner.prompts import build_chunk_continuation_prompt + + diff = "diff --git a/f.py b/f.py\n--- a/f.py\n+++ b/f.py\n" + "".join( + f"@@ -{10 * i + 1},1 +{10 * i + 1},2 @@\n x\n+y{i}\n" for i in range(4) + ) + parsed = parse_diff(diff) + refs = build_hunk_sequence(parsed, 0.25) + second_chunk = refs[2:] + stats = build_chunk_stats_from_hunks(parsed, second_chunk) + assert stats["file_summaries"][0]["hunk_indices"] == [2, 3] + prompt = build_chunk_continuation_prompt(stats, "diff", 2, 2, "catalog") + assert "indices 2, 3 (this chunk only)" in prompt + assert "indices 0..1" not in prompt diff --git a/tests/test_chunker_exhaustive.py b/tests/test_chunker_exhaustive.py index c830704..cfcb6e5 100644 --- a/tests/test_chunker_exhaustive.py +++ b/tests/test_chunker_exhaustive.py @@ -25,7 +25,7 @@ def _chunk_plan_cost( 100.0 + _chunk_slack_penalty(used_tokens, token_budget) + file_mix_penalty - + _chunk_boundary_penalty(hunk_sequence, end_index - 1) + + _chunk_boundary_penalty(hunk_sequence, end_index - 1, token_budget) ) return cost diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index cb5bfe0..2805e6a 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -7,6 +7,7 @@ from __future__ import annotations +import re import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -340,11 +341,100 @@ def test_template_os_error(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch _build_pr_body(group, [group]) +class TestPrTemplateEarlyValidation: + def _use_template(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, text: str) -> None: + template_file = tmp_path / "template.md" + template_file.write_text(text, encoding="utf-8") + monkeypatch.setattr("pr_split.cli._pr_template_path", lambda: template_file) + + @pytest.mark.parametrize( + "text", + ["{title.nope}", "{added[0]}", "{unknown}", "{title", "{0}"], + ) + def test_bad_placeholders_are_friendly_errors( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, text: str + ) -> None: + from pr_split.cli import validate_pr_template + + self._use_template(tmp_path, monkeypatch, text) + with pytest.raises(PRSplitError, match="Invalid PR template"): + validate_pr_template() + group = _group("pr-1", "t", files=["a.py"]) + with pytest.raises(PRSplitError, match="Invalid PR template"): + _build_pr_body(group, [group]) + + def test_valid_template_passes_and_renders( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.cli import validate_pr_template + + self._use_template(tmp_path, monkeypatch, "# {title}\n\n{files}\n\n{{literal}} {loc}") + validate_pr_template() + group = _group("pr-1", "t", files=["a.py"]) + body = _build_pr_body(group, [group]) + assert body.startswith("# t\n\n- `a.py`") + assert "{literal} 15" in body + + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + def test_split_inputs_reject_a_bad_template_before_planning( + self, + mock_be: MagicMock, + mock_clean: MagicMock, + mock_auth: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + from pr_split.cli import console + + self._use_template(tmp_path, monkeypatch, "{title.nope}") + with console.capture() as capture, pytest.raises(typer.Exit): + _validate_inputs("feature", "main") + assert "Invalid PR template" in " ".join(capture.get().split()) + # a dry run never opens PRs, so the template is not consulted + _validate_inputs("feature", "main", dry_run=True) + + @patch("pr_split.cli._create_branches_and_commits") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_execute_rejects_a_bad_template_before_creating_branches( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_commit: MagicMock, + mock_clean: MagicMock, + mock_auth: MagicMock, + mock_confirm: MagicMock, + mock_create: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + self._use_template(tmp_path, monkeypatch, "{added[0]}") + mock_plan_file = MagicMock() + mock_plan_file.git_state.branches = [] + mock_plan_file.git_state.prs = [] + mock_plan_file.plan.raw_diff = "some diff" + mock_plan_file.plan.merge_base_sha = "abc123" + mock_load.return_value = mock_plan_file + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert "Invalid PR template" in " ".join(result.output.split()) + mock_confirm.assert_not_called() + mock_create.assert_not_called() + + # --------------------------------------------------------------------------- # _send_webhook # --------------------------------------------------------------------------- class TestSendWebhook: - @patch("pr_split.cli.urllib.request.urlopen") + @patch("pr_split.cli._webhook_opener.open") def test_successful_webhook(self, mock_urlopen: MagicMock) -> None: mock_resp = MagicMock() mock_resp.read.return_value = b"" @@ -355,7 +445,7 @@ def test_successful_webhook(self, mock_urlopen: MagicMock) -> None: _send_webhook("https://example.com/hook", {"event": "test"}) mock_urlopen.assert_called_once() - @patch("pr_split.cli.urllib.request.urlopen", side_effect=Exception("timeout")) + @patch("pr_split.cli._webhook_opener.open", side_effect=Exception("timeout")) def test_failed_webhook_logs_warning(self, mock_urlopen: MagicMock) -> None: # Should not raise _send_webhook("https://example.com/hook", {"event": "test"}) @@ -374,6 +464,26 @@ def test_known_group_renders(self) -> None: g = _group("pr-1", "root", files=["a.py"], depends_on=["pr-0"]) _show_group_detail([g], "pr-1") + def test_assignment_type_is_printed_not_eaten_as_markup(self) -> None: + from pr_split.cli import console + + g = _group("pr-1", "Support list[str] [WIP]", files=["a.py"]) + g.description = "uses the [red] tag literally" + g.assignments.append( + GroupAssignment( + file_path="src/[id].py", + assignment_type=AssignmentType.PARTIAL_HUNKS, + hunk_indices=[0, 2], + ) + ) + with console.capture() as capture: + _show_group_detail([g], "pr-1") + out = capture.get() + assert "pr-1: Support list[str] [WIP]" in out + assert "Description: uses the [red] tag literally" in out + assert "a.py [whole_file] hunks: [all]" in out + assert "src/[id].py [partial_hunks] hunks: [0, 2]" in out + def test_partial_hunks_rendering(self) -> None: g = Group( id="pr-1", @@ -395,6 +505,11 @@ def test_partial_hunks_rendering(self) -> None: # _interactive_edit # --------------------------------------------------------------------------- class TestInteractiveEditRecomputesLoc: + @pytest.fixture(autouse=True) + def _interactive_stdin(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The editor only prompts on a TTY; these tests drive it via typer.prompt. + monkeypatch.setattr("pr_split.cli._stdin_is_interactive", lambda: True) + @patch("pr_split.cli.recompute_estimated_loc") @patch("pr_split.cli._move_assignment", return_value=True) @patch("pr_split.cli.typer.prompt", side_effect=["move a.py:0 pr-1 pr-2", "done"]) @@ -697,6 +812,176 @@ def test_split_fork_ref_not_found( result = runner.invoke(app, ["split", "unknown-ref", "--dry-run"]) assert result.exit_code != 0 + @patch("pr_split.cli._resolve_fork_ref") + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=False) + @patch("pr_split.cli.branch_exists", return_value=False) + def test_split_missing_plain_branch_reports_branch_not_found( + self, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_resolve: MagicMock, + ) -> None: + result = runner.invoke(app, ["split", "nosuchbranch", "--dry-run"]) + assert result.exit_code == 1 + assert "Branch 'nosuchbranch' does not exist" in result.output + assert "GitHub CLI authentication failed" not in result.output + mock_auth.assert_not_called() + mock_resolve.assert_not_called() + + @patch("pr_split.cli._resolve_fork_ref", return_value=None) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + def test_pr_number_is_never_taken_for_an_abbreviated_sha( + self, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_resolve: MagicMock, + ) -> None: + # branch_exists says True because `git rev-parse 1006` resolves a + # commit whose hash starts with 1006; the argument is still PR #1006. + result = runner.invoke(app, ["split", "1006", "--dry-run"]) + assert result.exit_code == 1 + mock_resolve.assert_called_once_with("1006") + assert "Branch '1006' does not exist" in result.output + + def test_full_ref_is_not_a_fork_ref(self) -> None: + from pr_split.cli import _is_fork_ref + + # Documented escape hatch: a local branch literally named like a + # number is reachable through its full ref. + assert _is_fork_ref("refs/heads/1006") is False + assert _is_fork_ref("1006") is True + assert _is_fork_ref("#1006") is True + + @patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}) + @patch("pr_split.cli.extract_diff", side_effect=RuntimeError("stop here")) + @patch("pr_split.cli._resolve_fork_ref", return_value=None) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + def test_full_ref_escape_hatch_takes_the_branch_path( + self, + mock_be: MagicMock, + mock_auth: MagicMock, + mock_clean: MagicMock, + mock_resolve: MagicMock, + mock_extract: MagicMock, + ) -> None: + result = runner.invoke(app, ["split", "refs/heads/1006", "--dry-run"]) + mock_resolve.assert_not_called() + mock_extract.assert_called_once() + assert result.exit_code != 0 # stopped by the stubbed extract_diff + + @patch("pr_split.cli.check_gh_auth", return_value=False) + @patch("pr_split.cli.branch_exists", return_value=False) + def test_split_fork_shaped_ref_still_checks_gh_auth( + self, + mock_be: MagicMock, + mock_auth: MagicMock, + ) -> None: + result = runner.invoke(app, ["split", "someone:feature", "--dry-run"]) + assert result.exit_code == 1 + assert "GitHub CLI authentication failed" in result.output + mock_auth.assert_called_once() + + +class TestSplitValidatesSettingsBeforeCleanup: + @patch("pr_split.cli.extract_diff") + @patch("pr_split.cli._cleanup_git_state") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + def test_bad_settings_never_reach_the_destructive_cleanup( + self, + mock_be: MagicMock, + mock_validate: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_confirm: MagicMock, + mock_cleanup: MagicMock, + mock_extract: MagicMock, + ) -> None: + existing = MagicMock() + existing.git_state.branches = [MagicMock()] + existing.git_state.prs = [MagicMock()] + mock_load.return_value = existing + + result = runner.invoke( + app, + ["split", "feature", "--min-loc", "500", "--max-loc", "400"], + env={"ANTHROPIC_API_KEY": "sk-test"}, + ) + + assert result.exit_code == 1 + assert "min_loc 500 must be less than max_loc 400" in " ".join(result.output.split()) + mock_confirm.assert_not_called() + mock_cleanup.assert_not_called() + mock_extract.assert_not_called() + + +class TestSplitDryRunWithExecutedPlan: + @patch("pr_split.cli._cleanup_git_state") + @patch("pr_split.cli.typer.confirm") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + def test_dry_run_refuses_instead_of_offering_cleanup( + self, + mock_be: MagicMock, + mock_validate: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_confirm: MagicMock, + mock_cleanup: MagicMock, + ) -> None: + existing = MagicMock() + existing.git_state.branches = [] + existing.git_state.prs = [MagicMock()] + mock_load.return_value = existing + + result = runner.invoke( + app, ["split", "feature", "--dry-run"], env={"ANTHROPIC_API_KEY": "sk-test"} + ) + + assert result.exit_code == 1 + assert "Run 'pr-split clean' first" in result.output + mock_confirm.assert_not_called() + mock_cleanup.assert_not_called() + + @patch("pr_split.cli.extract_diff", side_effect=PRSplitError("stop here")) + @patch("pr_split.cli._cleanup_git_state", return_value=(1, 1)) + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + def test_real_split_still_offers_cleanup( + self, + mock_be: MagicMock, + mock_validate: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_confirm: MagicMock, + mock_cleanup: MagicMock, + mock_extract: MagicMock, + ) -> None: + existing = MagicMock() + existing.git_state.branches = [] + existing.git_state.prs = [MagicMock()] + mock_load.return_value = existing + + runner.invoke(app, ["split", "feature"], env={"ANTHROPIC_API_KEY": "sk-test"}) + + mock_confirm.assert_called_once() + mock_cleanup.assert_called_once_with(existing.git_state) + # --------------------------------------------------------------------------- # execute command @@ -724,10 +1009,27 @@ def test_execute_missing_raw_diff(self, mock_pe: MagicMock, mock_load: MagicMock mock_plan_file = MagicMock() mock_plan_file.git_state.branches = [] mock_plan_file.git_state.prs = [] - mock_plan_file.plan.raw_diff = "" + mock_plan_file.plan.raw_diff = None mock_load.return_value = mock_plan_file result = runner.invoke(app, ["execute"]) assert result.exit_code != 0 + assert "missing saved diff data" in result.output + + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_execute_empty_saved_diff_is_refused( + self, mock_pe: MagicMock, mock_load: MagicMock + ) -> None: + mock_plan_file = MagicMock() + mock_plan_file.git_state.branches = [] + mock_plan_file.git_state.prs = [] + mock_plan_file.plan.raw_diff = "" + mock_plan_file.plan.groups = [] + mock_plan_file.plan.merge_base_sha = "abc123" + mock_load.return_value = mock_plan_file + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert "Saved plan has an empty diff; nothing to execute" in result.output @patch("pr_split.cli.commit_exists", return_value=False) @patch("pr_split.cli.load_plan") @@ -823,6 +1125,47 @@ def test_execute_rejects_a_remote_tracking_base_from_an_old_plan( mock_confirm.assert_not_called() mock_create.assert_not_called() + @patch("pr_split.cli.validate_coverage") + @patch("pr_split.cli.commit_exists", return_value=True) + @patch("pr_split.cli.check_gh_auth", return_value=True) + @patch("pr_split.cli.is_worktree_clean", return_value=True) + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_execute_rejects_a_saved_submodule_plan( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_be: MagicMock, + mock_clean: MagicMock, + mock_auth: MagicMock, + mock_commit: MagicMock, + mock_validate: MagicMock, + ) -> None: + mock_plan_file = MagicMock() + mock_plan_file.git_state.branches = [] + mock_plan_file.git_state.prs = [] + mock_plan_file.plan.merge_base_sha = "abc123" + mock_plan_file.plan.stacked = False + mock_plan_file.plan.split_new_files_over = None + mock_plan_file.plan.raw_diff = ( + "diff --git a/vendor b/vendor\n" + "index 0ebfaa8..2f687ea 160000\n" + "--- a/vendor\n" + "+++ b/vendor\n" + "@@ -1 +1 @@\n" + "-Subproject commit 0ebfaa8000000000000000000000000000000000\n" + "+Subproject commit 2f687ea000000000000000000000000000000000\n" + ) + mock_load.return_value = mock_plan_file + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Submodule changes are not supported (pointer bump at vendor)" in " ".join( + result.output.split() + ) + mock_validate.assert_not_called() + @patch("pr_split.cli.load_plan") @patch("pr_split.cli.plan_exists", return_value=True) def test_execute_missing_merge_base_sha( @@ -974,6 +1317,295 @@ def test_clean_no_plan(self, mock_pe: MagicMock) -> None: assert result.exit_code == 0 +class TestRichEscapingOfPlanText: + def test_present_plan_keeps_brackets_in_title_and_paths(self) -> None: + from pr_split.cli import _present_plan, console + + g = _group("pr-1", "Add [core] typing", files=["src/[id].py"]) + # _render_dag captures on the same console, which would reset an + # enclosing capture; it has its own test below. + with patch("pr_split.cli._render_dag", return_value=""), console.capture() as capture: + _present_plan([g]) + out = capture.get() + assert "Add [core] typing" in out + assert "src/[id].py" in out + + def test_render_dag_keeps_brackets(self) -> None: + from pr_split.cli import _render_dag + + root = _group("pr-1", "Root [v2]") + child = _group("pr-2", "Child [x]", depends_on=["pr-1"]) + out = _render_dag([root, child]) + assert "pr-1: Root [v2]" in out + assert "pr-2: Child [x] (depends on: pr-1)" in out + + def test_move_messages_keep_bracketed_paths(self) -> None: + from pr_split.cli import console + + g1 = _group("pr-1", "src", files=["src/[id].py"]) + g2 = _group("pr-2", "dst") + pf = MagicMock() + pf.path = "src/[id].py" + pf.__len__ = MagicMock(return_value=2) + parsed = MagicMock() + parsed.patch_set = [pf] + with console.capture() as capture: + assert _move_assignment([g1, g2], parsed, "src/[id].py", 1, "pr-1", "pr-2") + assert not _move_assignment([g1, g2], parsed, "src/[id].py", 9, "pr-1", "pr-2") + assert not _move_assignment([g1, g2], parsed, "a.py", 0, "[pr-1]", "pr-2") + _show_group_detail([g1], "[pr-9]") + out = capture.get() + assert "Moved src/[id].py:1 from pr-1 to pr-2" in out + assert "Hunk src/[id].py:9 not found in pr-1." in out + assert "Group '[pr-1]' or 'pr-2' not found." in out + assert "Group '[pr-9]' not found." in out + + +class TestRichEscapingOfStatusAndSummaries: + @patch("pr_split.cli.get_pr_state", return_value={"state": "OPEN", "reviewDecision": None}) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_status_table_keeps_bracketed_title( + self, mock_pe: MagicMock, mock_load: MagicMock, mock_state: MagicMock + ) -> None: + from pr_split.constants import Priority + from pr_split.schemas import BranchRecord, GitState, PlanFile, PRRecord, SplitPlan + + plan = SplitPlan( + dev_branch="feature", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + groups=[_group("pr-1", "Add [core] typing")], + ) + git_state = GitState( + branches=[BranchRecord(group_id="pr-1", branch_name="b[1]", base_branch="main")], + prs=[PRRecord(group_id="pr-1", pr_number=7, pr_url="u")], + ) + mock_load.return_value = PlanFile(plan=plan, git_state=git_state) + result = runner.invoke(app, ["status"]) + assert result.exit_code == 0 + assert "Add [core] typing" in result.output + assert "b[1]" in result.output + + def test_error_echo_keeps_bracketed_path(self) -> None: + from pr_split.cli import _handle_loc_bound_warnings, console + + # The strict path prints each warning in red (the non-strict path only logs). + with console.capture() as capture, pytest.raises(typer.Exit): + _handle_loc_bound_warnings( + ["Group 'pr-1' touches src/[id].py"], strict_loc_bounds=True + ) + assert "src/[id].py" in capture.get() + + def test_editor_same_group_message_keeps_bracketed_id(self) -> None: + from pr_split.cli import console + + with console.capture() as capture: + assert not _move_assignment([], MagicMock(), "a.py", 0, "[pr-1]", "[pr-1]") + assert "Source and destination are the same ('[pr-1]')" in capture.get() + + +class TestRichEscapingOfMergeSummary: + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_summary_lines_keep_bracketed_group_ids( + self, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + ) -> None: + from pr_split.constants import Priority + from pr_split.exceptions import GitOperationError + from pr_split.schemas import GitState, PlanFile, PRRecord, SplitPlan + + groups = [_group("[pr-1]", "a"), _group("[pr-2]", "b"), _group("[pr-3]", "c")] + plan = SplitPlan( + dev_branch="feature", + base_branch="main", + max_loc=400, + priority=Priority.ORTHOGONAL, + groups=groups, + ) + prs = [PRRecord(group_id=g.id, pr_number=i + 1, pr_url="u") for i, g in enumerate(groups)] + mock_load.return_value = PlanFile(plan=plan, git_state=GitState(prs=prs)) + states = { + 1: {"state": "OPEN", "isDraft": False, "reviewDecision": None}, + 2: {"state": "OPEN", "isDraft": True, "reviewDecision": None}, + 3: {"state": "OPEN", "isDraft": False, "reviewDecision": None}, + } + mock_state.side_effect = lambda n: states[n] + + def merge(n: int, *, auto: bool = False) -> None: + if n == 3: + raise GitOperationError("conflict") + + mock_merge.side_effect = merge + + result = runner.invoke(app, ["merge"]) + + flat = " ".join(result.output.split()) + assert "Merged (1): [pr-1]" in flat + assert "Skipped (1): [pr-2] (draft)" in flat + assert "Failed (1): [pr-3]" in flat + + +class TestPrBodyFileListCap: + def test_long_file_lists_are_summarised(self) -> None: + from pr_split.cli import _PR_BODY_FILE_LIST_LIMIT, _build_pr_body + + n = 2000 + g = _group("pr-1", "mass rename", files=[f"pkg/mod_{i}/handler.py" for i in range(n)]) + body = _build_pr_body(g, [g]) + assert body.count("- `pkg/") == _PR_BODY_FILE_LIST_LIMIT + assert f"… and {n - _PR_BODY_FILE_LIST_LIMIT} more files" in body + assert len(body) < 65_536 + + def test_short_file_lists_are_complete(self) -> None: + from pr_split.cli import _build_pr_body + + g = _group("pr-1", "t", files=["a.py", "b.py"]) + body = _build_pr_body(g, [g]) + assert "- `a.py`\n- `b.py`" in body + assert "more files" not in body + + +class TestMissingCliToolsAreReportedFirst: + @patch("pr_split.cli.branch_exists", return_value=False) + @patch("pr_split.cli.require_tools", return_value="git") + def test_split_reports_missing_git_before_anything_else( + self, mock_req: MagicMock, mock_be: MagicMock + ) -> None: + result = runner.invoke(app, ["split", "feature", "--dry-run"]) + assert result.exit_code == 1 + assert "'git' is not installed or not on PATH" in result.output + mock_be.assert_not_called() + + @patch("pr_split.cli.require_tools") + def test_dry_run_does_not_require_gh(self, mock_req: MagicMock) -> None: + mock_req.return_value = None + with ( + patch("pr_split.cli.branch_exists", return_value=True), + patch("pr_split.cli._validate_inputs", side_effect=typer.Exit(0)), + ): + runner.invoke(app, ["split", "feature", "--dry-run"]) + assert [c.args for c in mock_req.call_args_list] == [("git",)] + + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli.require_tools", return_value="gh") + def test_merge_reports_missing_gh_instead_of_fetch_errors( + self, + mock_req: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + ) -> None: + from pr_split.schemas import PRRecord + + plan_file = MagicMock() + plan_file.git_state.prs = [PRRecord(group_id="pr-1", pr_number=1, pr_url="u")] + mock_load.return_value = plan_file + result = runner.invoke(app, ["merge"]) + assert result.exit_code == 1 + assert "'gh' is not installed or not on PATH" in result.output + assert "fetch error" not in result.output + mock_state.assert_not_called() + + @patch("pr_split.cli._cleanup_git_state") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli.require_tools", return_value="gh") + def test_clean_reports_missing_gh_and_keeps_the_plan( + self, + mock_req: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_confirm: MagicMock, + mock_cleanup: MagicMock, + ) -> None: + from pr_split.schemas import PRRecord + + plan_file = MagicMock() + plan_file.git_state.prs = [PRRecord(group_id="pr-1", pr_number=1, pr_url="u")] + mock_load.return_value = plan_file + result = runner.invoke(app, ["clean"]) + assert result.exit_code == 1 + assert "'gh' is not installed or not on PATH" in result.output + mock_confirm.assert_not_called() + mock_cleanup.assert_not_called() + assert mock_req.call_args.args == ("git", "gh") + + @patch("pr_split.cli._cleanup_git_state") + @patch("pr_split.cli.typer.confirm", return_value=True) + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.require_tools") + def test_resplit_in_dry_run_refuses_before_any_cleanup( + self, + mock_req: MagicMock, + mock_be: MagicMock, + mock_validate: MagicMock, + mock_pe: MagicMock, + mock_load: MagicMock, + mock_confirm: MagicMock, + mock_cleanup: MagicMock, + ) -> None: + from pr_split.schemas import PRRecord + + mock_req.side_effect = lambda *tools: "gh" if "gh" in tools else None + existing = MagicMock() + existing.git_state.branches = [MagicMock()] + existing.git_state.prs = [PRRecord(group_id="pr-1", pr_number=7, pr_url="u")] + mock_load.return_value = existing + + result = runner.invoke( + app, ["split", "feature", "--dry-run"], env={"ANTHROPIC_API_KEY": "sk-test"} + ) + + assert result.exit_code == 1 + # A dry run never cleans up, so it refuses before gh would be needed. + assert "--dry-run would overwrite" in result.output + mock_confirm.assert_not_called() + mock_cleanup.assert_not_called() + + @patch("pr_split.cli.check_gh_auth", return_value=False) + @patch("pr_split.cli.branch_exists", return_value=False) + @patch("pr_split.cli.require_tools") + def test_dry_run_of_a_pr_number_still_needs_gh( + self, mock_req: MagicMock, mock_be: MagicMock, mock_auth: MagicMock + ) -> None: + mock_req.side_effect = lambda *tools: "gh" if "gh" in tools else None + result = runner.invoke(app, ["split", "#123", "--dry-run"]) + assert result.exit_code == 1 + assert "'gh' is not installed or not on PATH" in result.output + assert "authentication failed" not in result.output + mock_auth.assert_not_called() + + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + @patch("pr_split.cli.require_tools", return_value="gh") + def test_execute_reports_missing_gh( + self, mock_req: MagicMock, mock_pe: MagicMock, mock_load: MagicMock + ) -> None: + mock_plan_file = MagicMock() + mock_plan_file.git_state.branches = [] + mock_plan_file.git_state.prs = [] + mock_plan_file.plan.raw_diff = "some diff" + mock_plan_file.plan.merge_base_sha = "abc123" + mock_load.return_value = mock_plan_file + result = runner.invoke(app, ["execute"]) + assert result.exit_code == 1 + assert "'gh' is not installed or not on PATH" in result.output + + class TestExecuteRetriesAfterFailedPush: """Branches without PRs mean a failed push; execute must retry, not refuse. @@ -1561,6 +2193,11 @@ def test_crossed_moves_are_refused(self) -> None: class TestEditorEmptiedGroupEndToEnd: + @pytest.fixture(autouse=True) + def _interactive_stdin(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The editor only prompts on a TTY; these tests drive it via typer.prompt. + monkeypatch.setattr("pr_split.cli._stdin_is_interactive", lambda: True) + @patch("pr_split.cli.typer.prompt") def test_moving_the_last_hunk_out_yields_a_valid_one_group_plan( self, mock_prompt: MagicMock @@ -1822,6 +2459,11 @@ def test_merge_refuses_when_the_base_has_merged( class TestEditorPlanCommands: + @pytest.fixture(autouse=True) + def _interactive_stdin(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The editor only prompts on a TTY; these tests drive it via typer.prompt. + monkeypatch.setattr("pr_split.cli._stdin_is_interactive", lambda: True) + def _partial(self, file_path: str, indices: list[int]) -> GroupAssignment: return GroupAssignment( file_path=file_path, @@ -1949,3 +2591,169 @@ def test_yes_skips_the_confirmation_prompt( mock_confirm.assert_not_called() mock_create.assert_called_once() assert "stop here" in result.output + + +class TestDryRunWithClosedStdin: + """`split --dry-run < /dev/null` must save the plan, not abort. + + The interactive editor is entered unconditionally; with a closed stdin + (scripts, CI) `typer.prompt` raises EOFError, which used to become + typer.Abort before the plan was ever saved. + """ + + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli.merge_base", return_value="abc123") + @patch("pr_split.cli._present_plan") + @patch("pr_split.cli.validate_plan", return_value=[]) + @patch("pr_split.cli.plan_split") + @patch("pr_split.cli.parse_diff") + @patch("pr_split.cli.extract_diff", return_value="diff --git a/a.py b/a.py\n") + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + @patch("pr_split.cli.plan_exists", return_value=False) + def test_closed_stdin_accepts_the_plan_and_saves_it( + self, + mock_plan_exists: MagicMock, + mock_branch_exists: MagicMock, + mock_validate_inputs: MagicMock, + mock_extract_diff: MagicMock, + mock_parse_diff: MagicMock, + mock_plan_split: MagicMock, + mock_validate_plan: MagicMock, + mock_present_plan: MagicMock, + mock_merge_base: MagicMock, + mock_save_plan: MagicMock, + ) -> None: + parsed_diff = MagicMock() + parsed_diff.stats = { + "total_files": 1, + "total_added": 10, + "total_removed": 5, + "total_loc": 15, + } + mock_parse_diff.return_value = parsed_diff + group = _group("pr-1", "feat: auth", files=["a.py"]) + mock_plan_split.return_value = [group] + + # No `input=` and no patched _interactive_edit: the editor really runs + # and hits EOF on the runner's empty stdin. + result = runner.invoke( + app, + ["split", "feature-branch", "--dry-run"], + env={"ANTHROPIC_API_KEY": "sk-test"}, + ) + + assert result.exit_code == 0 + assert "accepting the plan as-is" in result.output.replace("\n", " ") + mock_save_plan.assert_called_once() + + +def _flat(output: str) -> str: + # Rich draws the error in a box that wraps text (and, on CI, colours it + # with ANSI codes); strip both and collapse whitespace so a phrase can be + # matched regardless of wrapping. + plain = re.sub(r"\x1b\[[0-9;]*m", "", output) + return " ".join(re.sub(r"[│╭╮╰╯─]", " ", plain).split()) + + +class TestNotifyUrlValidation: + @pytest.mark.parametrize("bad", ["file:///tmp/wh.txt", "not-a-url", "ftp://x/y", "https://"]) + def test_non_http_urls_are_rejected_before_anything_runs(self, bad: str) -> None: + with patch("pr_split.cli.plan_exists") as mock_pe: + result = runner.invoke(app, ["merge", "--notify", bad]) + assert result.exit_code == 2 + assert "must be an http(s) URL" in _flat(result.output) + mock_pe.assert_not_called() + + def test_env_var_is_validated_too(self) -> None: + with patch("pr_split.cli.plan_exists") as mock_pe: + result = runner.invoke(app, ["merge"], env={"PR_SPLIT_WEBHOOK_URL": "file:///x"}) + assert result.exit_code == 2 + mock_pe.assert_not_called() + + @patch("pr_split.cli.plan_exists", return_value=False) + def test_http_urls_pass(self, mock_pe: MagicMock) -> None: + result = runner.invoke(app, ["merge", "--notify", "https://hooks.example/abc"]) + assert result.exit_code == 0 + mock_pe.assert_called_once() + + +class TestWebhookRedirects: + def _serve(self, handler_cls: type) -> tuple[object, int]: + import threading + from http.server import HTTPServer + + server = HTTPServer(("127.0.0.1", 0), handler_cls) + threading.Thread(target=server.serve_forever, daemon=True).start() + return server, server.server_address[1] + + def test_redirect_is_reported_instead_of_a_body_less_get(self) -> None: + from http.server import BaseHTTPRequestHandler + + from pr_split.cli import _send_webhook + + received: list[tuple[str, int]] = [] + + class Target(BaseHTTPRequestHandler): + def do_GET(self) -> None: + received.append(("GET", int(self.headers.get("Content-Length") or 0))) + self.send_response(200) + self.end_headers() + + def do_POST(self) -> None: + received.append(("POST", int(self.headers.get("Content-Length") or 0))) + self.send_response(200) + self.end_headers() + + def log_message(self, *args: object) -> None: + pass + + target, target_port = self._serve(Target) + + class Mover(BaseHTTPRequestHandler): + def do_POST(self) -> None: + self.send_response(301) + self.send_header("Location", f"http://127.0.0.1:{target_port}/hook") + self.end_headers() + + def log_message(self, *args: object) -> None: + pass + + mover, mover_port = self._serve(Mover) + try: + with patch("pr_split.cli.logger") as mock_logger: + _send_webhook(f"http://127.0.0.1:{mover_port}/old", {"event": "merge_complete"}) + finally: + mover.shutdown() # type: ignore[attr-defined] + target.shutdown() # type: ignore[attr-defined] + + assert received == [] + warning = str(mock_logger.warning.call_args) + assert "redirected to" in warning and f"127.0.0.1:{target_port}/hook" in warning + mock_logger.info.assert_not_called() + + def test_direct_post_still_delivers_the_payload(self) -> None: + from http.server import BaseHTTPRequestHandler + + from pr_split.cli import _send_webhook + + bodies: list[bytes] = [] + + class Sink(BaseHTTPRequestHandler): + def do_POST(self) -> None: + bodies.append(self.rfile.read(int(self.headers["Content-Length"]))) + self.send_response(200) + self.end_headers() + + def log_message(self, *args: object) -> None: + pass + + sink, port = self._serve(Sink) + try: + with patch("pr_split.cli.logger") as mock_logger: + _send_webhook(f"http://127.0.0.1:{port}/hook", {"event": "merge_complete"}) + finally: + sink.shutdown() # type: ignore[attr-defined] + + assert bodies == [b'{"event": "merge_complete"}'] + mock_logger.warning.assert_not_called() diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 69c5820..3e12c05 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -725,6 +725,131 @@ def test_grandchild_materializes_with_all_ancestor_hunks( assert child_calls[0].args[1].assignments[0].hunk_indices == [0, 1, 2] +class TestParentPrFailureGating: + def _chain(self) -> tuple[list[Group], list[BranchRecord]]: + groups = [ + _group("pr-1", "feat: a"), + _group("pr-2", "feat: b", ["pr-1"]), + _group("pr-3", "feat: c", ["pr-2"]), + ] + records = [ + _branch_record("pr-1", "pr-split/ns/pr-1"), + BranchRecord( + group_id="pr-2", + branch_name="pr-split/ns/pr-2", + base_branch="pr-split/ns/pr-1", + commit_sha="abc123", + ), + BranchRecord( + group_id="pr-3", + branch_name="pr-split/ns/pr-3", + base_branch="pr-split/ns/pr-2", + commit_sha="abc123", + ), + ] + return groups, records + + @patch("pr_split.cli.create_pr") + @patch("pr_split.cli.push_branch") + def test_children_are_skipped_when_the_root_pr_fails( + self, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + from pr_split.exceptions import PRCreationError + + groups, records = self._chain() + + def create( + *, head: str, base: str, title: str, body: str, draft: bool = False + ) -> tuple[int, str]: + if head == "pr-split/ns/pr-1": + raise GitOperationError("boom") + return (11, "https://github.com/pr/11") + + mock_create.side_effect = create + with pytest.raises(PRCreationError) as excinfo: + _push_and_create_prs(groups, records) + + assert excinfo.value.pr_records == [] + assert [c.kwargs["head"] for c in mock_create.call_args_list] == ["pr-split/ns/pr-1"] + + @patch("pr_split.cli.create_pr") + @patch("pr_split.cli.push_branch") + def test_every_descendant_is_warned_even_when_listed_before_its_parent( + self, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + from pr_split.exceptions import PRCreationError + + groups, records = self._chain() + groups.reverse() + records.reverse() + + def create( + *, head: str, base: str, title: str, body: str, draft: bool = False + ) -> tuple[int, str]: + if head == "pr-split/ns/pr-1": + raise GitOperationError("boom") + return (11, "https://github.com/pr/11") + + mock_create.side_effect = create + with patch("pr_split.cli.logger") as mock_logger, pytest.raises(PRCreationError): + _push_and_create_prs(groups, records) + + warned = " ".join(str(c.args[0]) for c in mock_logger.warning.call_args_list) + assert "Skipping PR for pr-2" in warned + assert "Skipping PR for pr-3" in warned + + @patch("pr_split.cli.create_pr") + @patch("pr_split.cli.push_branch") + def test_only_the_failed_subtree_is_skipped( + self, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + from pr_split.exceptions import PRCreationError + + groups, records = self._chain() + groups.append(_group("pr-4", "feat: d")) + records.append(_branch_record("pr-4", "pr-split/ns/pr-4")) + numbers = iter(range(20, 30)) + + def create( + *, head: str, base: str, title: str, body: str, draft: bool = False + ) -> tuple[int, str]: + if head == "pr-split/ns/pr-2": + raise GitOperationError("boom") + n = next(numbers) + return (n, f"https://github.com/pr/{n}") + + mock_create.side_effect = create + with pytest.raises(PRCreationError) as excinfo: + _push_and_create_prs(groups, records) + + created = sorted(r.group_id for r in excinfo.value.pr_records) + assert created == ["pr-1", "pr-4"] + heads = {c.kwargs["head"] for c in mock_create.call_args_list} + assert "pr-split/ns/pr-3" not in heads + + @patch("pr_split.cli.create_pr") + @patch("pr_split.cli.push_branch") + def test_parent_pr_is_opened_before_its_child( + self, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + groups, records = self._chain() + order: list[str] = [] + numbers = iter(range(1, 10)) + + def create( + *, head: str, base: str, title: str, body: str, draft: bool = False + ) -> tuple[int, str]: + order.append(head) + n = next(numbers) + return (n, f"https://github.com/pr/{n}") + + mock_create.side_effect = create + result = _push_and_create_prs(groups, records) + + assert [r.group_id for r in result] == ["pr-1", "pr-2", "pr-3"] + assert order == ["pr-split/ns/pr-1", "pr-split/ns/pr-2", "pr-split/ns/pr-3"] + + class TestTransitivePushFailureGating: @patch("pr_split.cli.create_pr", return_value=(1, "https://github.com/pr/1")) @patch("pr_split.cli.push_branch") @@ -762,6 +887,22 @@ def push(branch: str) -> None: assert mock_create.call_count == 0 +class TestPushPrunesFirst: + @patch("pr_split.cli.create_pr", return_value=(1, "https://github.com/pr/1")) + @patch("pr_split.cli.push_branch") + @patch("pr_split.cli.prune_remote_tracking_refs") + def test_tracking_refs_are_refreshed_before_any_push( + self, mock_prune: MagicMock, mock_push: MagicMock, mock_create: MagicMock + ) -> None: + order: list[str] = [] + mock_prune.side_effect = lambda: order.append("prune") + mock_push.side_effect = lambda branch: order.append("push") + _push_and_create_prs( + [_group("pr-1", "feat: a")], [_branch_record("pr-1", "pr-split/ns/pr-1")] + ) + assert order == ["prune", "push"] + + class TestWorktreeWritesPreserveCrlf: @patch("pr_split.cli.commit_files_in_dir", return_value="sha1") @patch("pr_split.cli.materialize_group_files", return_value={"c.txt": "a\r\nb\r\n"}) @@ -892,3 +1033,221 @@ def test_oversized_ids_are_recorded(self) -> None: groups = [self._sized("pr-1", 876), self._sized("pr-2", 400)] assert _oversized_group_ids(groups, 400) == ["pr-1"] + + +class TestRenderDagMultiParent: + def _diamond(self) -> list[Group]: + return [ + _group("pr-1", "base"), + _group("pr-2", "left", depends_on=["pr-1"]), + _group("pr-3", "right", depends_on=["pr-1"]), + _group("pr-4", "merge", depends_on=["pr-2", "pr-3"]), + _group("pr-5", "after merge", depends_on=["pr-4"]), + ] + + def test_markdown_renders_merge_node_and_subtree_once(self) -> None: + result = _render_dag_markdown(self._diamond(), "pr-4") + assert result.count("<-- this PR") == 1 + assert result.count("pr-4: merge (also depends on: pr-2)") == 1 + assert result.count("pr-4: merge (see below)") == 1 + assert result.count("pr-5: after merge") == 1 + + def test_markdown_marks_descendant_of_merge_once(self) -> None: + result = _render_dag_markdown(self._diamond(), "pr-5") + assert result.count("<-- this PR") == 1 + + def test_tree_renders_merge_node_once(self) -> None: + result = _render_dag(self._diamond()) + assert result.count("pr-4: merge (depends on: pr-2, pr-3)") == 1 + assert result.count("pr-4: merge (see below)") == 1 + assert result.count("pr-5: after merge") == 1 + + def test_stub_always_precedes_full_node(self) -> None: + # Traversal visits pr-4's branch before pr-2's even though pr-2 is the + # last-listed dependency of pr-5; the stub must still point downward. + groups = [ + _group("pr-1", "root"), + _group("pr-4", "short", depends_on=["pr-1"]), + _group("pr-6", "mid", depends_on=["pr-1"]), + _group("pr-2", "long", depends_on=["pr-6"]), + _group("pr-5", "merge", depends_on=["pr-2", "pr-4"]), + ] + for text, full_label in ( + (_render_dag_markdown(groups, "pr-5"), "pr-5: merge (also depends on: pr-4)"), + (_render_dag(groups), "pr-5: merge (depends on: pr-2, pr-4)"), + ): + assert text.index("pr-5: merge (see below)") < text.index(full_label) + assert text.count("pr-5: merge (see below)") == 1 + assert text.count(full_label) == 1 + + def test_unknown_dependency_does_not_hide_node(self) -> None: + groups = [ + _group("pr-1", "root"), + _group("pr-2", "child", depends_on=["pr-1", "ghost"]), + ] + text = _render_dag_markdown(groups, "pr-2") + assert "pr-2: child (also depends on: ghost) <-- this PR" in text + + def test_unreachable_parent_does_not_hide_node(self) -> None: + # pr-9 depends on an unknown group, so it is never visited; pr-2 must + # still be drawn in full (with its subtree and marker) under pr-1. + groups = [ + _group("pr-1", "root"), + _group("pr-9", "orphan", depends_on=["ghost"]), + _group("pr-2", "leaf", depends_on=["pr-1", "pr-9"]), + _group("pr-5", "after", depends_on=["pr-2"]), + ] + md = _render_dag_markdown(groups, "pr-2") + assert md.count("<-- this PR") == 1 + assert "pr-2: leaf (also depends on: pr-9) <-- this PR" in md + assert "pr-5: after" in md + assert "(see below)" not in md + tree = _render_dag(groups) + assert "pr-2: leaf (depends on: pr-1, pr-9)" in tree + assert "pr-5: after" in tree + + def test_parent_still_being_walked_is_not_pending(self) -> None: + # pr-3 depends on pr-1 (its grandparent, mid-walk) and pr-2 (a sibling + # listed after it); it must be drawn in full under pr-2. + groups = [ + _group("pr-1", "root"), + _group("pr-3", "merge", depends_on=["pr-1", "pr-2"]), + _group("pr-2", "mid", depends_on=["pr-1"]), + _group("pr-4", "after", depends_on=["pr-3"]), + ] + md = _render_dag_markdown(groups, "pr-4") + assert md.count("<-- this PR") == 1 + assert md.count("pr-3: merge (see below)") == 1 + assert "pr-3: merge (also depends on: pr-1)" in md + assert "pr-4: after <-- this PR" in md + tree = _render_dag(groups) + assert "pr-3: merge (depends on: pr-1, pr-2)" in tree + assert "pr-4: after" in tree + + def test_topological_order_draws_merge_node_once(self) -> None: + # The natural planner order: pr-2 listed before pr-3. pr-3 is reached + # from pr-1 (still mid-walk) and from pr-2; it must be drawn once. + groups = [ + _group("pr-1", "root"), + _group("pr-2", "mid", depends_on=["pr-1"]), + _group("pr-3", "merge", depends_on=["pr-1", "pr-2"]), + _group("pr-4", "after", depends_on=["pr-3"]), + ] + md = _render_dag_markdown(groups, "pr-4") + assert md.count("<-- this PR") == 1 + assert md.count("pr-3: merge (see below)") == 1 + assert md.count("pr-3: merge (also depends on:") == 1 + assert md.count("pr-4: after") == 1 + assert md.index("pr-3: merge (see below)") < md.index("pr-3: merge (also depends on:") + tree = _render_dag(groups) + assert tree.count("pr-3: merge (depends on: pr-1, pr-2)") == 1 + assert tree.count("pr-3: merge (see below)") == 1 + assert tree.count("pr-4: after") == 1 + + def test_duplicate_dependency_entries_do_not_hide_node(self) -> None: + groups = [ + _group("pr-1", "root"), + _group("pr-2", "child", depends_on=["pr-1", "pr-1"]), + _group("pr-3", "leaf", depends_on=["pr-2"]), + ] + md = _render_dag_markdown(groups, "pr-3") + assert "pr-2: child" in md + assert "(see below)" not in md + assert "also depends on" not in md + assert "pr-3: leaf <-- this PR" in md + tree = _render_dag(groups) + assert "pr-2: child (depends on: pr-1)" in tree + assert "pr-3: leaf" in tree + + +class TestWorktreeAppliesFileModes: + @patch("pr_split.cli.commit_files_in_dir", return_value="sha1") + @patch("pr_split.cli.target_file_modes", return_value={"run.sh": 0o100755}) + @patch("pr_split.cli.materialize_group_files", return_value={"run.sh": "x\n", "a.py": "y\n"}) + @patch("pr_split.cli.remove_worktree") + @patch("pr_split.cli.add_worktree") + def test_executable_bit_applied_only_where_diff_says( + self, + mock_add: MagicMock, + mock_remove: MagicMock, + mock_mat: MagicMock, + mock_modes: MagicMock, + mock_commit: MagicMock, + ) -> None: + import os + import stat + from pathlib import Path + + modes: dict[str, int] = {} + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + for file_path in file_paths: + modes[file_path] = stat.S_IMODE(os.stat(Path(cwd) / file_path).st_mode) + return "sha1" + + mock_commit.side_effect = capture + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + assert modes["run.sh"] & stat.S_IXUSR + assert not modes["a.py"] & stat.S_IXUSR + + +class TestWorktreeMaterializesSymlinks: + def _run( + self, materialized: dict[str, str | None], modes: dict[str, int] + ) -> dict[str, object]: + import os + from pathlib import Path + + seen: dict[str, object] = {} + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + for file_path in file_paths: + p = Path(cwd) / file_path + seen[file_path] = ( + os.readlink(p) if p.is_symlink() else ("missing" if not p.exists() else "file") + ) + return "sha1" + + with ( + patch("pr_split.cli.add_worktree"), + patch("pr_split.cli.remove_worktree"), + patch("pr_split.cli.materialize_group_files", return_value=materialized), + patch("pr_split.cli.target_file_modes", return_value=modes), + patch("pr_split.cli.commit_files_in_dir", side_effect=capture), + ): + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + return seen + + def test_new_symlink_is_created_as_a_link(self) -> None: + seen = self._run({"link": "other.py\n", "other.py": "x\n"}, {"link": 0o120000}) + assert seen["link"] == "other.py" + assert seen["other.py"] == "file" + + def test_symlink_without_mode_info_stays_a_plain_file(self) -> None: + seen = self._run({"link": "other.py\n"}, {}) + assert seen["link"] == "file" + + def test_regular_file_converted_to_symlink(self) -> None: + import os + from pathlib import Path + + seen: dict[str, object] = {} + + def pre_populate(path: str, branch: str, start: str) -> None: + Path(path).mkdir(parents=True, exist_ok=True) + (Path(path) / "toref").write_text("old regular content\n") + + def capture(cwd: str, file_paths: list[str], message: str, **kwargs: object) -> str: + p = Path(cwd) / "toref" + seen["toref"] = os.readlink(p) if p.is_symlink() else "file" + return "sha1" + + with ( + patch("pr_split.cli.add_worktree", side_effect=pre_populate), + patch("pr_split.cli.remove_worktree"), + patch("pr_split.cli.materialize_group_files", return_value={"toref": "a.py\n"}), + patch("pr_split.cli.target_file_modes", return_value={"toref": 0o120000}), + patch("pr_split.cli.commit_files_in_dir", side_effect=capture), + ): + _create_branches_and_commits([_group("pr-1", "t")], MagicMock(), "main", "sha", "ns") + assert seen["toref"] == "a.py" diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 4987110..0631a72 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -426,6 +426,31 @@ def test_all_groups_emptied_is_an_error( mock_save_plan.assert_not_called() +class TestSplitEmptyDiff: + @patch("pr_split.cli.save_plan") + @patch("pr_split.cli.plan_split") + @patch("pr_split.cli.extract_diff", return_value="") + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + def test_no_changes_is_refused_before_planning( + self, + mock_branch_exists: MagicMock, + mock_validate_inputs: MagicMock, + mock_extract_diff: MagicMock, + mock_plan_split: MagicMock, + mock_save_plan: MagicMock, + ) -> None: + result = runner.invoke( + app, + ["split", "feature-branch", "--base", "main", "--dry-run"], + env={"ANTHROPIC_API_KEY": "sk-test"}, + ) + assert result.exit_code == 1 + assert "No changes between 'main' and 'feature-branch'; nothing to split" in result.output + mock_plan_split.assert_not_called() + mock_save_plan.assert_not_called() + + class TestSplitCliEnvVars: @patch("pr_split.cli.save_plan") @patch("pr_split.cli.merge_base", return_value="abc123") @@ -681,6 +706,7 @@ def test_close_failure_warning_includes_the_reason( class TestCleanupAfterMergeDeletedBranches: + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.cli.get_pr_state", return_value={"state": "MERGED"}) @patch("pr_split.cli.shutil.rmtree") @patch("pr_split.cli.plan_path") @@ -693,6 +719,7 @@ def test_fully_merged_split_cleans_up_completely( mock_path: MagicMock, mock_rmtree: MagicMock, mock_state: MagicMock, + mock_forget: MagicMock, ) -> None: mock_path.return_value.exists.return_value = True # merge --delete-branch already removed both local and remote branches @@ -863,6 +890,260 @@ def test_child_of_merged_parent_merges( assert result.exit_code == 0 +class TestMergeFetchErrors: + @patch("pr_split.cli._send_webhook") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_unfetchable_pr_state_fails_the_run( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + ) -> None: + groups = [ + _group("pr-1", "ok", files=["a.py"]), + _group("pr-2", "unfetchable", files=["b.py"]), + ] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = lambda n: ( + {"state": "OPEN", "isDraft": False, "reviewDecision": None} if n == 1 else {} + ) + + result = runner.invoke(app, ["merge", "--notify", "https://example.invalid/hook"]) + + mock_merge.assert_called_once_with(1, auto=False) + assert result.exit_code == 1 + assert "Could not fetch PR state" in result.output + assert "Merge complete" not in result.output + payload = mock_webhook.call_args[0][1] + assert payload["success"] is False + assert payload["exit_reason"] == "fetch_error" + + @patch("pr_split.cli._send_webhook") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_fetch_error_on_parent_is_reported_as_root_cause( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + ) -> None: + groups = [ + _group("pr-1", "unfetchable", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = lambda n: ( + {} if n == 1 else {"state": "OPEN", "isDraft": False, "reviewDecision": None} + ) + + result = runner.invoke(app, ["merge", "--notify", "https://example.invalid/hook"]) + + assert result.exit_code == 1 + assert mock_webhook.call_args[0][1]["exit_reason"] == "fetch_error" + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli._send_webhook") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_fetch_error_while_polling_auto_merge_is_reported( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + mock_sleep: MagicMock, + ) -> None: + groups = [_group("pr-1", "only", files=["a.py"])] + mock_load.return_value = _plan_with_prs(groups) + mock_state.side_effect = [{"state": "OPEN", "isDraft": False, "reviewDecision": None}, {}] + + result = runner.invoke( + app, ["merge", "--auto", "--notify", "https://example.invalid/hook"] + ) + + assert result.exit_code == 1 + assert "Could not fetch PR state" in result.output + payload = mock_webhook.call_args[0][1] + assert payload["exit_reason"] == "fetch_error" + assert [s["id"] for s in payload["skipped"]] == ["pr-1"] + + +class TestMergeClosedWhilePolling: + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli._send_webhook") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_closed_during_auto_wait_is_listed_as_skipped( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + mock_sleep: MagicMock, + ) -> None: + groups = [ + _group("pr-1", "closed later", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + mock_load.return_value = _plan_with_prs(groups) + open_state = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + mock_state.side_effect = [open_state, {"state": "CLOSED"}, open_state] + + result = runner.invoke( + app, ["merge", "--auto", "--notify", "https://example.invalid/hook"] + ) + + assert result.exit_code == 1 + assert "pr-1 (CLOSED)" in result.output + assert "pr-2 (dependency pr-1 not merged)" in result.output + payload = mock_webhook.call_args[0][1] + assert [s["id"] for s in payload["skipped"]] == ["pr-1", "pr-2"] + assert payload["exit_reason"] == "pr_closed" + assert payload["success"] is False + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli._send_webhook") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_closed_leaf_during_auto_wait_exits_nonzero( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_webhook: MagicMock, + mock_sleep: MagicMock, + ) -> None: + groups = [_group("pr-1", "closed later", files=["a.py"])] + mock_load.return_value = _plan_with_prs(groups) + open_state = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + mock_state.side_effect = [open_state, {"state": "CLOSED"}] + + result = runner.invoke( + app, ["merge", "--auto", "--notify", "https://example.invalid/hook"] + ) + + assert result.exit_code == 1 + assert "pr-1 (CLOSED)" in result.output + assert "Closed before auto-merge completed (1): pr-1" in result.output + assert "Merge complete" not in result.output + payload = mock_webhook.call_args[0][1] + assert payload["exit_reason"] == "pr_closed" + assert payload["success"] is False + assert payload["merged"] == [] + + +class TestStackedMergeRetargetsBase: + def _stacked_plan(self) -> MagicMock: + groups = [ + _group("pr-1", "root", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + plan_file = _plan_with_prs(groups) + plan_file.plan.base_branch = "main" + plan_file.git_state.branches = [ + BranchRecord(group_id="pr-1", branch_name="pr-split/ns/pr-1", base_branch="main"), + BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="pr-split/ns/pr-1" + ), + ] + return plan_file + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_child_is_retargeted_at_the_plan_base_before_merging( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n} -> {base}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"merge {n}") + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + assert calls == ["merge 1", "retarget 2 -> main", "merge 2"] + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_unstacked_prs_are_not_retargeted( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + plan_file = self._stacked_plan() + plan_file.git_state.branches[1] = BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="main" + ) + mock_load.return_value = plan_file + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + mock_retarget.assert_not_called() + assert mock_merge.call_count == 2 + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_auto_mode_retargets_before_queueing( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + mock_sleep: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + open_state = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + mock_state.side_effect = [open_state, {"state": "MERGED"}, open_state, {"state": "MERGED"}] + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"queue {n}") + + result = runner.invoke(app, ["merge", "--auto"]) + + assert result.exit_code == 0, result.output + assert calls == ["queue 1", "retarget 2", "queue 2"] + + class TestMergeRejectsMalformedSavedPlan: def _plan_file(self, groups: list[Group]) -> PlanFile: from pr_split.constants import Priority @@ -1034,3 +1315,35 @@ def test_coverage_gap_is_reported_not_raised( assert result.exception is None or isinstance(result.exception, SystemExit) assert "Hunk f.py[1] not assigned to any group" in result.output mock_save_plan.assert_not_called() + + +class TestSplitRejectsSubmoduleDiffs: + @patch("pr_split.cli.plan_split") + @patch("pr_split.cli.extract_diff") + @patch("pr_split.cli._validate_inputs") + @patch("pr_split.cli.branch_exists", return_value=True) + def test_submodule_diff_is_a_clean_error( + self, + mock_branch_exists: MagicMock, + mock_validate_inputs: MagicMock, + mock_extract_diff: MagicMock, + mock_plan_split: MagicMock, + ) -> None: + mock_extract_diff.return_value = ( + "diff --git a/vendor b/vendor\n" + "index 0ebfaa8..2f687ea 160000\n" + "--- a/vendor\n" + "+++ b/vendor\n" + "@@ -1 +1 @@\n" + "-Subproject commit 0ebfaa8000000000000000000000000000000000\n" + "+Subproject commit 2f687ea000000000000000000000000000000000\n" + ) + result = runner.invoke( + app, ["split", "feature-branch", "--dry-run"], env={"ANTHROPIC_API_KEY": "sk-test"} + ) + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Submodule changes are not supported (pointer bump at vendor)" in " ".join( + result.output.split() + ) + mock_plan_split.assert_not_called() diff --git a/tests/test_client.py b/tests/test_client.py index c705628..8440485 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1204,6 +1204,29 @@ def test_small_diff_direct_call( assert len(result) == 1 mock_llm.assert_called_once() + @patch("pr_split.planner.client._refine_plan_with_llm") + @patch("pr_split.planner.client.recompute_estimated_loc") + @patch("pr_split.planner.client._call_llm") + @patch("pr_split.planner.client._count_tokens", return_value=100) + def test_min_loc_reaches_the_system_prompt( + self, + mock_count: MagicMock, + mock_llm: MagicMock, + mock_recompute: MagicMock, + mock_refine: MagicMock, + ) -> None: + parsed = parse_diff(_TWO_FILE_DIFF) + settings = _make_settings( + Provider.ANTHROPIC, partition_strategy=PartitionStrategy.LLM, min_loc=50, max_loc=400 + ) + mock_llm.return_value = RawToolOutput(groups=_SAMPLE_RAW_GROUPS) + mock_refine.side_effect = lambda groups, *a, **kw: groups + + _plan_split_with_llm(parsed, settings) + + system = mock_llm.call_args.kwargs["system"] + assert "not be smaller than about 50 lines" in system + @patch("pr_split.planner.client._refine_plan_with_llm") @patch("pr_split.planner.client._plan_split_chunked") @patch("pr_split.planner.client._count_tokens", return_value=999_999_999) @@ -1305,6 +1328,57 @@ def test_chunk_retry_recovers_from_truncated_first_attempt(self, mock_call: Magi assert mock_call.call_count == 2 +class TestOpenAIFailedResponse: + @patch("pr_split.planner.client.openai.OpenAI") + def test_failed_status_raises_with_error_message(self, mock_cls: MagicMock) -> None: + mock_response = SimpleNamespace( + status="failed", + error=SimpleNamespace(code="server_error", message="upstream exploded"), + output=[], + ) + mock_cls.return_value.responses.create.return_value = mock_response + settings = _make_settings(Provider.OPENAI) + with pytest.raises(LLMError, match="response failed: upstream exploded"): + _call_openai("sys", "usr", settings=settings) + + @patch("pr_split.planner.client.openai.OpenAI") + def test_failed_status_without_error_object(self, mock_cls: MagicMock) -> None: + mock_cls.return_value.responses.create.return_value = SimpleNamespace( + status="failed", error=None, output=[] + ) + with pytest.raises(LLMError, match="response failed: unknown error"): + _call_openai("sys", "usr", settings=_make_settings(Provider.OPENAI)) + + @patch("pr_split.planner.client.openai.OpenAI") + def test_failed_status_falls_back_to_error_code(self, mock_cls: MagicMock) -> None: + mock_cls.return_value.responses.create.return_value = SimpleNamespace( + status="failed", + error=SimpleNamespace(code="rate_limit_exceeded", message=None), + output=[], + ) + with pytest.raises(LLMError, match="response failed: rate_limit_exceeded"): + _call_openai("sys", "usr", settings=_make_settings(Provider.OPENAI)) + + +class TestTruncationWarningScope: + @patch("pr_split.planner.client.logger") + @patch("pr_split.planner.client.anthropic.Anthropic") + def test_end_turn_with_tool_block_does_not_warn_truncated( + self, mock_cls: MagicMock, mock_logger: MagicMock + ) -> None: + from anthropic.types.beta import BetaToolUseBlock + + block = BetaToolUseBlock( + id="tu_1", type="tool_use", name=SPLIT_TOOL_NAME, input={"groups": _SAMPLE_RAW_GROUPS} + ) + mock_cls.return_value.beta.messages.create.return_value = SimpleNamespace( + stop_reason="end_turn", content=[block] + ) + result = _call_anthropic("sys", "usr", settings=_make_settings(Provider.ANTHROPIC)) + assert result["groups"] == _SAMPLE_RAW_GROUPS + assert not any("truncated" in str(c.args[0]) for c in mock_logger.warning.call_args_list) + + class TestMergeChunkGroupsSameFile: def test_same_file_in_later_chunk_is_merged_into_one_assignment(self) -> None: first = Group( diff --git a/tests/test_config.py b/tests/test_config.py index 1ea04f0..7f24af1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -184,6 +184,21 @@ def test_empty_string_key_raises_openai(self, monkeypatch: pytest.MonkeyPatch) - Settings(provider=Provider.OPENAI) +class TestCpSatTimeoutBound: + @pytest.mark.parametrize("timeout", [0.0, -1.0, -5]) + def test_non_positive_timeout_is_rejected( + self, monkeypatch: pytest.MonkeyPatch, timeout: float + ) -> None: + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test") + with pytest.raises(ValidationError, match="cp_sat_timeout"): + Settings(partition_strategy=PartitionStrategy.CP_SAT, cp_sat_timeout=timeout) + + def test_positive_timeout_is_accepted(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test") + settings = Settings(partition_strategy=PartitionStrategy.CP_SAT, cp_sat_timeout=0.25) + assert settings.cp_sat_timeout == 0.25 + + class TestDerivedMinLocFloor: @pytest.mark.parametrize("max_loc", [2, 3]) def test_tiny_max_loc_derives_min_loc_of_one( diff --git a/tests/test_diff_parser_extended.py b/tests/test_diff_parser_extended.py index be71784..443c108 100644 --- a/tests/test_diff_parser_extended.py +++ b/tests/test_diff_parser_extended.py @@ -121,6 +121,7 @@ def test_extract_diff_success(self, mock_run: MagicMock) -> None: "-U3", "--src-prefix=a/", "--dst-prefix=b/", + "--submodule=short", "main...feature", ], capture_output=True, @@ -413,3 +414,126 @@ def git(*args: str) -> str: [(modified, 1), (deleted, 1), ("plain name.md", 1), ('new "file".txt', 1)] ) assert parsed.stats["total_files"] == 4 + + +SUBMODULE_DIFF = """\ +diff --git a/app.py b/app.py +--- a/app.py ++++ b/app.py +@@ -1 +1 @@ +-x ++y +diff --git a/vendor b/vendor +index 0ebfaa8..2f687ea 160000 +--- a/vendor ++++ b/vendor +@@ -1 +1 @@ +-Subproject commit 0ebfaa8000000000000000000000000000000000 ++Subproject commit 2f687ea000000000000000000000000000000000 +""" + +NEW_SUBMODULE_DIFF = """\ +diff --git a/lib b/lib +new file mode 160000 +index 0000000..2f687ea +--- /dev/null ++++ b/lib +@@ -0,0 +1 @@ ++Subproject commit 2f687ea000000000000000000000000000000000 +""" + + +DELETED_SUBMODULE_DIFF = """\ +diff --git a/vendor b/vendor +deleted file mode 160000 +index d075b20..0000000 +--- a/vendor ++++ /dev/null +@@ -1 +0,0 @@ +-Subproject commit d075b20000000000000000000000000000000000 +""" + + +class TestSubmoduleChangesAreRejected: + def test_deleted_submodule_is_rejected(self) -> None: + from pr_split.exceptions import DiffParseError + + with pytest.raises(DiffParseError, match="pointer bump at vendor"): + parse_diff(DELETED_SUBMODULE_DIFF) + + def test_pointer_bump_is_rejected_by_name(self) -> None: + from pr_split.exceptions import DiffParseError + + with pytest.raises(DiffParseError, match="pointer bump at vendor"): + parse_diff(SUBMODULE_DIFF) + + def test_added_submodule_is_rejected(self) -> None: + from pr_split.exceptions import DiffParseError + + with pytest.raises(DiffParseError, match="Submodule changes are not supported"): + parse_diff(NEW_SUBMODULE_DIFF) + + def test_regular_files_with_similar_content_are_fine(self) -> None: + raw = ( + "diff --git a/notes.txt b/notes.txt\n" + "index 0ebfaa8..2f687ea 100644\n" + "--- a/notes.txt\n" + "+++ b/notes.txt\n" + "@@ -1 +1 @@\n" + "-Subproject commit 0ebfaa8000000000000000000000000000000000\n" + "+Subproject commit 2f687ea000000000000000000000000000000000\n" + ) + assert [pf.path for pf in parse_diff(raw).patch_set] == ["notes.txt"] + + def test_real_submodule_bump_is_rejected_before_planning(self, tmp_path: Path) -> None: + from pr_split.exceptions import DiffParseError + + def git(cwd: Path, *args: str) -> str: + return subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "-c", + "protocol.file.allow=always", + *args, + ], + cwd=cwd, + capture_output=True, + text=True, + check=True, + ).stdout + + sub = tmp_path / "sub" + sub.mkdir() + git(sub, "init", "-q", "-b", "main") + (sub / "f").write_text("1\n") + git(sub, "add", "-A") + git(sub, "commit", "-qm", "one") + repo = tmp_path / "repo" + repo.mkdir() + git(repo, "init", "-q", "-b", "main") + (repo / "app.py").write_text("x\n") + git(repo, "add", "-A") + git(repo, "submodule", "add", "-q", str(sub), "vendor") + git(repo, "commit", "-qm", "base") + git(repo, "checkout", "-qb", "dev") + (sub / "f").write_text("2\n") + git(sub, "commit", "-qam", "two") + git(repo / "vendor", "pull", "-q", "origin", "main") + (repo / "app.py").write_text("y\n") + git(repo, "add", "-A") + git(repo, "commit", "-qm", "bump") + + cwd = os.getcwd() + os.chdir(repo) + try: + for mode in ("short", "log", "diff"): + # A user-level diff.submodule setting must not hide the bump. + git(repo, "config", "diff.submodule", mode) + with pytest.raises(DiffParseError, match="vendor"): + parse_diff(extract_diff("dev", "main")) + finally: + os.chdir(cwd) diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 4775772..231cbc8 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -34,6 +34,10 @@ def test_coverage_overlap(self) -> None: assert "bar.py" in result assert "g1, g2" in result + def test_coverage_duplicate(self) -> None: + result = ErrorMsg.COVERAGE_DUPLICATE(file="bar.py", index=1, group="g1") + assert result == "Hunk bar.py[1] listed more than once in group 'g1'" + def test_loc_mismatch(self) -> None: result = ErrorMsg.LOC_MISMATCH(actual=10, expected=20) assert "10" in result diff --git a/tests/test_git_branches.py b/tests/test_git_branches.py index e2c4a81..bf93eba 100644 --- a/tests/test_git_branches.py +++ b/tests/test_git_branches.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -9,6 +10,7 @@ from pr_split.constants import PLAN_DIR from pr_split.exceptions import GitOperationError from pr_split.git_ops.branches import ( + _no_hooks_dir, add_worktree, adopt_remote_branch, branch_exists, @@ -18,6 +20,7 @@ derive_split_namespace, diff_base_ref, is_worktree_clean, + local_branch_exists, merge_base, push_branch, remove_worktree, @@ -181,14 +184,18 @@ def test_local_only(self, mock_git: MagicMock) -> None: delete_branch("pr-split/pr-1") mock_git.assert_called_once_with("branch", "-D", "pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_with_remote(self, mock_git: MagicMock) -> None: + def test_with_remote(self, mock_git: MagicMock, mock_forget: MagicMock) -> None: mock_git.return_value = "" delete_branch("pr-split/pr-1", remote=True) assert mock_git.call_count == 2 + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_local_failure_still_deletes_remote(self, mock_git: MagicMock) -> None: + def test_local_failure_still_deletes_remote( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [GitOperationError("checked out"), ""] with pytest.raises(GitOperationError, match="checked out"): delete_branch("pr-split/pr-1", remote=True) @@ -201,15 +208,21 @@ def test_local_failure_without_remote_raises_immediately(self, mock_git: MagicMo delete_branch("pr-split/pr-1") mock_git.assert_called_once() + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_already_deleted_locally_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_already_deleted_locally_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: # `pr-split merge` deletes the local branch; cleanup must not fail on it. mock_git.side_effect = [GitOperationError("error: branch 'pr-split/pr-1' not found."), ""] delete_branch("pr-split/pr-1", remote=True) mock_git.assert_any_call("push", "origin", "--delete", "pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_already_deleted_on_origin_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_already_deleted_on_origin_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [ "", GitOperationError( @@ -218,8 +231,11 @@ def test_branch_already_deleted_on_origin_counts_as_deleted(self, mock_git: Magi ] delete_branch("pr-split/pr-1", remote=True) + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_branch_gone_everywhere_counts_as_deleted(self, mock_git: MagicMock) -> None: + def test_branch_gone_everywhere_counts_as_deleted( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [ GitOperationError("error: branch 'pr-split/pr-1' not found."), GitOperationError( @@ -233,8 +249,11 @@ def test_missing_local_branch_without_remote_is_fine(self, mock_git: MagicMock) mock_git.side_effect = GitOperationError("error: branch 'pr-split/pr-1' not found.") delete_branch("pr-split/pr-1") + @patch("pr_split.git_ops.branches.forget_remote_tracking_ref") @patch("pr_split.git_ops.branches.run_git") - def test_other_remote_failure_still_raises(self, mock_git: MagicMock) -> None: + def test_other_remote_failure_still_raises( + self, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = ["", GitOperationError("fatal: could not read from remote")] with pytest.raises(GitOperationError, match="could not read from remote"): delete_branch("pr-split/pr-1", remote=True) @@ -323,28 +342,44 @@ def test_failure_raises(self, mock_run: MagicMock) -> None: class TestAddWorktree: + @patch("pr_split.git_ops.branches._forget_stale_worktrees") @patch("pr_split.git_ops.branches.run_git") - @patch("pr_split.git_ops.branches.branch_exists", return_value=False) - def test_adds_worktree(self, mock_exists: MagicMock, mock_git: MagicMock) -> None: + @patch("pr_split.git_ops.branches.local_branch_exists", return_value=False) + def test_adds_worktree( + self, mock_exists: MagicMock, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.return_value = "" add_worktree("/tmp/wt", "pr-split/ns/pr-1", "abc123") - mock_git.assert_called_once_with( - "worktree", "add", "-b", "pr-split/ns/pr-1", "/tmp/wt", "abc123" - ) - + args = mock_git.call_args.args + assert args[0] == "-c" and args[1].startswith("core.hooksPath=") + assert args[2:] == ("worktree", "add", "-b", "pr-split/ns/pr-1", "/tmp/wt", "abc123") + mock_forget.assert_called_once_with("/tmp/wt", "pr-split/ns/pr-1") + + def test_hooks_dir_is_private_and_empty(self) -> None: + path = Path(_no_hooks_dir()) + assert path.is_dir() + assert list(path.iterdir()) == [] + # A fixed shared path would let another local user plant hooks there. + assert path.stat().st_mode & 0o077 == 0 + + @patch("pr_split.git_ops.branches._forget_stale_worktrees") @patch("pr_split.git_ops.branches.run_git") - @patch("pr_split.git_ops.branches.branch_exists", return_value=True) + @patch("pr_split.git_ops.branches.local_branch_exists", return_value=True) def test_deletes_existing_branch_first( - self, mock_exists: MagicMock, mock_git: MagicMock + self, mock_exists: MagicMock, mock_git: MagicMock, mock_forget: MagicMock ) -> None: mock_git.side_effect = ["oldsha", "", ""] add_worktree("/tmp/wt", "pr-split/ns/pr-1", "abc123") assert mock_git.call_count == 3 + mock_git.assert_any_call("rev-parse", "refs/heads/pr-split/ns/pr-1") mock_git.assert_any_call("branch", "-D", "pr-split/ns/pr-1") + @patch("pr_split.git_ops.branches._forget_stale_worktrees") @patch("pr_split.git_ops.branches.run_git") - @patch("pr_split.git_ops.branches.branch_exists", return_value=True) - def test_restores_branch_on_failure(self, mock_exists: MagicMock, mock_git: MagicMock) -> None: + @patch("pr_split.git_ops.branches.local_branch_exists", return_value=True) + def test_restores_branch_on_failure( + self, mock_exists: MagicMock, mock_git: MagicMock, mock_forget: MagicMock + ) -> None: mock_git.side_effect = [ "oldsha", "", @@ -370,7 +405,7 @@ def test_basic_commit(self, mock_git: MagicMock) -> None: mock_git.side_effect = ["", "", "abc123"] sha = commit_files_in_dir("/tmp/wt", ["file.py"], "test commit") assert sha == "abc123" - assert mock_git.call_args_list[0].args == ("/tmp/wt", "add", "-A", "--", "file.py") + assert mock_git.call_args_list[0].args == ("/tmp/wt", "add", "-A", "-f", "--", "file.py") @patch("pr_split.git_ops.branches.run_git_in_dir") def test_commit_with_author(self, mock_git: MagicMock) -> None: @@ -385,6 +420,150 @@ def test_empty_file_paths_raises(self) -> None: commit_files_in_dir("/tmp/wt", [], "msg") +class TestAddWorktreeStaleEntries: + def _repo(self, tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=repo, check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "base", + ], + cwd=repo, + check=True, + ) + return repo + + def test_killed_run_with_directory_still_on_disk_can_be_rerun( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + first = tmp_path / "run1" / "g2" + first.parent.mkdir() + add_worktree(str(first), "pr-split/ns/g2", "main") + # A SIGKILL leaves the old temporary worktree directory in place and + # the next run uses a fresh temporary directory. + assert first.exists() + + second = tmp_path / "run2" / "g2" + second.parent.mkdir() + add_worktree(str(second), "pr-split/ns/g2", "main") + + assert (second / ".git").exists() + assert not first.exists() + + def test_registration_locked_by_an_interrupted_add_is_cleared( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + first = tmp_path / "run1" / "g4" + first.parent.mkdir() + add_worktree(str(first), "pr-split/ns/g4", "main") + # git leaves this lock in place when a `worktree add` is killed. + subprocess.run( + ["git", "worktree", "lock", "--reason", "initializing", str(first)], + cwd=repo, + check=True, + ) + + second = tmp_path / "run2" / "g4" + second.parent.mkdir() + add_worktree(str(second), "pr-split/ns/g4", "main") + + assert (second / ".git").exists() + + def test_killed_run_whose_directory_vanished_can_be_rerun( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + import shutil + + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + first = str(tmp_path / "wt1") + add_worktree(first, "pr-split/ns/g2", "main") + shutil.rmtree(first) + + add_worktree(str(tmp_path / "wt2"), "pr-split/ns/g2", "main") + + assert (tmp_path / "wt2" / ".git").exists() + + def test_symlinked_path_is_matched_by_realpath( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + add_worktree(str(link / "wt"), "pr-split/ns/g3", "main") + + add_worktree(str(link / "wt"), "pr-split/ns/g3", "main") + + assert (real / "wt" / ".git").exists() + + def test_other_live_worktrees_are_left_alone( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + other = tmp_path / "other" + add_worktree(str(other), "pr-split/ns/other", "main") + + add_worktree(str(tmp_path / "mine"), "pr-split/ns/mine", "main") + + assert (other / ".git").exists() + assert (repo / ".git").exists() + + def test_registered_path_that_still_exists_is_replaced( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + path = str(tmp_path / "wt") + add_worktree(path, "pr-split/ns/g1", "main") + + add_worktree(path, "pr-split/ns/g1", "main") + + assert (tmp_path / "wt" / ".git").exists() + + def test_tag_with_the_branch_name_does_not_confuse_the_check( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + repo = self._repo(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "tag", "pr-split/ns/g5"], cwd=repo, check=True) + assert branch_exists("pr-split/ns/g5") + assert not local_branch_exists("pr-split/ns/g5") + + add_worktree(str(tmp_path / "wt"), "pr-split/ns/g5", "main") + + assert (tmp_path / "wt" / ".git").exists() + + +class TestRequireTools: + def test_returns_first_missing_tool(self) -> None: + from pr_split.git_ops.branches import require_tools + + with patch( + "pr_split.git_ops.branches.shutil.which", + side_effect=lambda t: None if t == "gh" else "/usr/bin/x", + ): + assert require_tools("git", "gh") == "gh" + assert require_tools("git") is None + + class TestGitLocaleIsPinned: @patch("pr_split.git_ops.branches.subprocess.run") def test_run_git_forces_the_c_locale(self, mock_run: MagicMock) -> None: @@ -417,6 +596,162 @@ def test_localised_git_still_reports_already_deleted( delete_branch("pr-split/ns/never") +class TestStaleRemoteTrackingRefs: + def _repo_with_origin(self, tmp_path: Path) -> Path: + origin = tmp_path / "origin.git" + subprocess.run(["git", "init", "-q", "--bare", str(origin)], check=True) + repo = tmp_path / "repo" + subprocess.run(["git", "clone", "-q", str(origin), str(repo)], check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "base", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "push", "-q", "-u", "origin", "HEAD:main"], cwd=repo, check=True) + return repo + + def test_reused_branch_name_pushes_again_after_remote_deletion( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import prune_remote_tracking_refs, push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-1"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-1") + # Simulate `gh pr merge --delete-branch` / GitHub deleting the head + # branch behind our back: the remote branch goes away but the local + # tracking ref still points at the old head. + subprocess.run( + ["git", "update-ref", "-d", "refs/heads/pr-split/ns/pr-1"], + cwd=tmp_path / "origin.git", + check=True, + ) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "v2", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "branch", "-f", "pr-split/ns/pr-1", "HEAD"], cwd=repo, check=True) + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/ns/pr-1") + + prune_remote_tracking_refs() + push_branch("pr-split/ns/pr-1") + + def test_prune_keeps_the_lease_against_third_party_pushes( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import prune_remote_tracking_refs, push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-9"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-9") + # A reviewer pushes a fixup to the split branch from another clone. + other = tmp_path / "other" + subprocess.run( + ["git", "clone", "-q", str(tmp_path / "origin.git"), str(other)], check=True + ) + subprocess.run(["git", "checkout", "-q", "pr-split/ns/pr-9"], cwd=other, check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=r", + "-c", + "user.email=r@x", + "commit", + "-q", + "--allow-empty", + "-m", + "fixup", + ], + cwd=other, + check=True, + ) + subprocess.run(["git", "push", "-q", "origin", "pr-split/ns/pr-9"], cwd=other, check=True) + subprocess.run( + [ + "git", + "-c", + "user.name=t", + "-c", + "user.email=t@x", + "commit", + "-q", + "--allow-empty", + "-m", + "resplit", + ], + cwd=repo, + check=True, + ) + subprocess.run(["git", "branch", "-f", "pr-split/ns/pr-9", "HEAD"], cwd=repo, check=True) + + prune_remote_tracking_refs() + + # The lease must still protect the reviewer's commit. + with pytest.raises(GitOperationError, match="stale info"): + push_branch("pr-split/ns/pr-9") + + def test_remote_delete_drops_the_tracking_ref( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-2"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-2") + assert branch_exists("refs/remotes/origin/pr-split/ns/pr-2") + + delete_branch("pr-split/ns/pr-2", remote=True) + + assert not branch_exists("refs/remotes/origin/pr-split/ns/pr-2") + + def test_already_deleted_remote_still_drops_the_tracking_ref( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.branches import push_branch + + repo = self._repo_with_origin(tmp_path) + monkeypatch.chdir(repo) + subprocess.run(["git", "branch", "pr-split/ns/pr-3"], cwd=repo, check=True) + push_branch("pr-split/ns/pr-3") + subprocess.run( + ["git", "update-ref", "-d", "refs/heads/pr-split/ns/pr-3"], + cwd=tmp_path / "origin.git", + check=True, + ) + + delete_branch("pr-split/ns/pr-3", remote=True) + + assert not branch_exists("refs/remotes/origin/pr-split/ns/pr-3") + + class TestIsWorktreeCleanIgnoresPlanDir: """The check must not trip on pr-split's own `.pr-split/plan.json`.""" @@ -512,6 +847,85 @@ def test_failing_pre_commit_hook_does_not_block_the_sub_pr_commit( assert log.strip() == "feat: a" +class TestIgnoredPathsAreStillCommitted: + def test_file_tracked_on_dev_despite_gitignore_is_committed( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _git_identity(monkeypatch) + repo = tmp_path / "repo" + (repo / "build").mkdir(parents=True) + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=repo, check=True) + (repo / ".gitignore").write_text("build/\n") + (repo / "build" / "out.txt").write_text("artifact\n") + + commit_files_in_dir(str(repo), [".gitignore", "build/out.txt"], "feat: artifact") + + tracked = subprocess.run( + ["git", "ls-files"], cwd=repo, capture_output=True, text=True, check=True + ).stdout.split() + assert "build/out.txt" in tracked + + def test_deleted_files_are_still_staged( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _git_identity(monkeypatch) + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=repo, check=True) + (repo / "a.py").write_text("x\n") + commit_files_in_dir(str(repo), ["a.py"], "add") + (repo / "a.py").unlink() + + commit_files_in_dir(str(repo), ["a.py"], "remove") + + tracked = subprocess.run( + ["git", "ls-files"], cwd=repo, capture_output=True, text=True, check=True + ).stdout.split() + assert tracked == [] + + +class TestWorktreeAddSkipsHooks: + def test_failing_post_checkout_hook_does_not_block_worktree_creation( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _git_identity(monkeypatch) + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=repo, check=True) + subprocess.run( + ["git", "commit", "-q", "--allow-empty", "-m", "base"], cwd=repo, check=True + ) + hook = repo / ".git" / "hooks" / "post-checkout" + hook.write_text("#!/bin/sh\necho 'post-checkout failing' >&2\nexit 1\n") + hook.chmod(0o755) + monkeypatch.chdir(repo) + + add_worktree(str(tmp_path / "wt"), "pr-split/ns/g1", "main") + + assert (tmp_path / "wt" / ".git").exists() + + def test_hooks_path_from_config_is_also_bypassed( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _git_identity(monkeypatch) + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=repo, check=True) + subprocess.run( + ["git", "commit", "-q", "--allow-empty", "-m", "base"], cwd=repo, check=True + ) + hooks = tmp_path / "myhooks" + hooks.mkdir() + (hooks / "post-checkout").write_text("#!/bin/sh\nexit 1\n") + (hooks / "post-checkout").chmod(0o755) + subprocess.run(["git", "config", "core.hooksPath", str(hooks)], cwd=repo, check=True) + monkeypatch.chdir(repo) + + add_worktree(str(tmp_path / "wt"), "pr-split/ns/g2", "main") + + assert (tmp_path / "wt" / ".git").exists() + + class TestCommitExists: @patch("pr_split.git_ops.branches.run_git", return_value="") def test_true_when_object_resolves(self, mock_git: MagicMock) -> None: @@ -574,3 +988,29 @@ def test_existing_local_branch_is_untouched( before = run_git("rev-parse", "refs/heads/feat/move-op") assert adopt_remote_branch("feat/move-op") is False assert run_git("rev-parse", "refs/heads/feat/move-op") == before + + +class TestDeriveSplitNamespaceRefSafety: + def test_fork_namespace_includes_user(self) -> None: + assert derive_split_namespace("user:feat/x") == "user-feat-x" + assert derive_split_namespace("user:feat/x") != derive_split_namespace("feat/x") + + def test_all_invalid_characters_fall_back_to_hash(self) -> None: + result = derive_split_namespace("---") + assert re.fullmatch(r"[0-9a-f]{8}", result) + assert derive_split_namespace("---") == result + assert derive_split_namespace("user:---") == "user" + + def test_double_dots_and_edge_dots_are_removed(self) -> None: + result = derive_split_namespace(".feat..x.") + assert ".." not in result + assert not result.startswith(".") + assert not result.endswith(".") + + def test_namespace_is_a_valid_ref_component(self, tmp_path: Path) -> None: + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + for arg in ("---", "user:---", ".feat..x.", "#42", "a b/c", "weird@chars!"): + ref = f"refs/heads/pr-split/{derive_split_namespace(arg)}/pr-1" + subprocess.run( + ["git", "check-ref-format", ref], cwd=tmp_path, check=True, capture_output=True + ) diff --git a/tests/test_git_prs.py b/tests/test_git_prs.py index 2043ef9..d8faa0a 100644 --- a/tests/test_git_prs.py +++ b/tests/test_git_prs.py @@ -160,3 +160,137 @@ def test_ready_by_default(self, mock_gh: MagicMock) -> None: mock_gh.return_value = "https://github.com/org/repo/pull/7" create_pr("head", "main", "Title", "Body") assert "--draft" not in mock_gh.call_args.args + + +class TestCheckGhAuthHost: + @patch("pr_split.git_ops.prs.run_git", return_value="git@github.com:org/repo.git") + @patch("pr_split.git_ops.prs._run_gh", return_value="") + def test_checks_only_the_target_host( + self, mock_gh: MagicMock, mock_git: MagicMock, monkeypatch: pytest.MonkeyPatch + ) -> None: + from pr_split.git_ops.prs import check_gh_auth + + monkeypatch.delenv("GH_HOST", raising=False) + assert check_gh_auth() is True + mock_gh.assert_called_once_with("auth", "status", "--hostname", "github.com") + + @pytest.mark.parametrize( + ("remote", "host"), + [ + ("https://ghe.example.com/org/repo.git", "ghe.example.com"), + ("ssh://git@ghe.example.com:2222/org/repo.git", "ghe.example.com"), + ("git@ghe.example.com:org/repo.git", "ghe.example.com"), + ("https://user:tok@ghe.example.com/org/repo", "ghe.example.com"), + ("git@github.com:org/repo.git", "github.com"), + # gh lowercases remote hosts; `--hostname` is case-sensitive. + ("https://GHE.Example.COM/org/repo.git", "ghe.example.com"), + # Local remotes have no host to authenticate against. + ("../other", "github.com"), + ("/srv/repo.git", "github.com"), + ("file:///srv/repo.git", "github.com"), + # Single-label GHE hosts are valid in scheme and scp forms. + ("git@ghe:org/repo.git", "ghe"), + ("ssh://git@ghe/org/repo.git", "ghe"), + # Bare scp form still needs a dot to rule out local paths. + ("ghe.example.com:org/repo.git", "ghe.example.com"), + ], + ) + def test_host_is_derived_from_the_origin_remote( + self, monkeypatch: pytest.MonkeyPatch, remote: str, host: str + ) -> None: + from pr_split.git_ops.prs import gh_host + + monkeypatch.delenv("GH_HOST", raising=False) + with patch("pr_split.git_ops.prs.run_git", return_value=remote): + assert gh_host() == host + + def test_no_remote_falls_back_to_github(self, monkeypatch: pytest.MonkeyPatch) -> None: + from pr_split.git_ops.prs import gh_host + + monkeypatch.delenv("GH_HOST", raising=False) + with patch("pr_split.git_ops.prs.run_git", side_effect=GitOperationError("no origin")): + assert gh_host() == "github.com" + + @patch("pr_split.git_ops.prs._run_gh", return_value="") + def test_honours_gh_host(self, mock_gh: MagicMock, monkeypatch: pytest.MonkeyPatch) -> None: + from pr_split.git_ops.prs import check_gh_auth + + monkeypatch.setenv("GH_HOST", "GHE.example.com") + with patch("pr_split.git_ops.prs.run_git") as mock_git: + check_gh_auth() + mock_gh.assert_called_once_with("auth", "status", "--hostname", "ghe.example.com") + mock_git.assert_not_called() + + @patch("pr_split.git_ops.prs._run_gh", side_effect=GitOperationError("not logged in")) + def test_unauthenticated_target_host_is_false(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import check_gh_auth + + assert check_gh_auth() is False + + +class TestRetargetPr: + @patch("pr_split.git_ops.prs._run_gh", return_value="") + def test_edits_the_base(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is True + mock_gh.assert_called_once_with("pr", "edit", "7", "--base", "main") + + @patch( + "pr_split.git_ops.prs._run_gh", + side_effect=GitOperationError( + "Cannot change the base branch because the pull request is part of a stack" + ), + ) + def test_native_stack_refusal_is_tolerated(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is False + + @patch("pr_split.git_ops.prs._run_gh", side_effect=GitOperationError("rate limited")) + def test_other_failures_raise(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + with pytest.raises(GitOperationError, match="rate limited"): + retarget_pr(7, "main") + + +class TestCreatePrBodyLimit: + @patch("pr_split.git_ops.prs._run_gh", return_value="https://github.com/o/r/pull/9") + def test_oversized_body_is_truncated_below_the_limit(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import PR_BODY_MAX_CHARS + + create_pr(head="h", base="b", title="t", body="x" * 100_000) + args = mock_gh.call_args[0] + sent = args[args.index("--body") + 1] + assert len(sent) <= PR_BODY_MAX_CHARS + assert sent.endswith("_(body truncated: GitHub's limit is 65,536 characters)_") + + @patch("pr_split.git_ops.prs._run_gh", return_value="https://github.com/o/r/pull/9") + def test_truncation_closes_an_open_code_fence_on_a_line_boundary( + self, mock_gh: MagicMock + ) -> None: + from pr_split.git_ops.prs import PR_BODY_MAX_CHARS + + lines = "\n".join(f"pr-{i}: group {i}" for i in range(6000)) + body = f"intro\n\n```\n{lines}\n```" + create_pr(head="h", base="b", title="t", body=body) + args = mock_gh.call_args[0] + sent = args[args.index("--body") + 1] + assert len(sent) <= PR_BODY_MAX_CHARS + assert sent.count("```") % 2 == 0 + assert sent.endswith("```\n\n_(body truncated: GitHub's limit is 65,536 characters)_") + # cut on a line boundary: the line before the closing fence is intact + before_close = sent.rsplit("\n```", 1)[0] + last_line = before_close.rsplit("\n", 1)[-1] + assert last_line.startswith("pr-") + assert ( + last_line + == f"{last_line.split(':')[0]}: group {last_line.split('-')[1].split(':')[0]}" + ) + + @patch("pr_split.git_ops.prs._run_gh", return_value="https://github.com/o/r/pull/9") + def test_normal_body_is_sent_verbatim(self, mock_gh: MagicMock) -> None: + create_pr(head="h", base="b", title="t", body="hello") + args = mock_gh.call_args[0] + assert args[args.index("--body") + 1] == "hello" diff --git a/tests/test_git_prs_coverage.py b/tests/test_git_prs_coverage.py index 6278283..84f1824 100644 --- a/tests/test_git_prs_coverage.py +++ b/tests/test_git_prs_coverage.py @@ -7,6 +7,8 @@ from __future__ import annotations import json +import subprocess +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -167,6 +169,32 @@ def test_git_fetch_failure(self, mock_gh: MagicMock, mock_git: MagicMock) -> Non fetch_fork_branch("user", "branch") +class TestMissingCliTools: + @patch( + "pr_split.git_ops.prs.subprocess.run", + side_effect=FileNotFoundError(2, "No such file", "gh"), + ) + def test_missing_gh_is_a_git_operation_error(self, mock_run: MagicMock) -> None: + from pr_split.git_ops.prs import _run_gh, check_gh_auth + + with pytest.raises(GitOperationError, match="'gh' is not installed or not on PATH"): + _run_gh("auth", "status") + assert check_gh_auth() is False + assert get_pr_state(1) == {} + + @patch( + "pr_split.git_ops.branches.subprocess.run", + side_effect=FileNotFoundError(2, "No such file", "git"), + ) + def test_missing_git_is_a_git_operation_error(self, mock_run: MagicMock) -> None: + from pr_split.git_ops.branches import run_git, run_git_in_dir + + with pytest.raises(GitOperationError, match="'git' is not installed or not on PATH"): + run_git("status") + with pytest.raises(GitOperationError, match="'git' is not installed"): + run_git_in_dir("/tmp", "status") + + def _pr_data(head_repo: str, base_repo: str, *, fork: bool) -> str: def repo(full_name: str, *, is_fork: bool) -> dict[str, object]: return { @@ -284,3 +312,74 @@ def test_no_exact_match_is_none(self, mock_gh: MagicMock) -> None: [{"number": 1, "url": "u1", "headRefName": "feat/x-2", "isCrossRepository": False}] ) assert find_open_pr("feat/x") is None + + +class TestForkFetchRefspecIsForced: + @patch("pr_split.git_ops.branches.run_git") + @patch("pr_split.git_ops.prs._run_gh") + def test_fetch_fork_pr_uses_forced_refspec( + self, mock_gh: MagicMock, mock_git: MagicMock + ) -> None: + mock_gh.return_value = json.dumps( + { + "head": { + "ref": "feature", + "repo": {"fork": True, "clone_url": "https://x/f.git", "full_name": "u/f"}, + }, + "base": {"ref": "main"}, + } + ) + mock_git.side_effect = ["", "A "] + fetch_fork_pr(42) + fetch_call = mock_git.call_args_list[0].args + assert fetch_call[0] == "fetch" + assert fetch_call[2] == "+refs/heads/feature:refs/pr-split/pr-42" + + @patch("pr_split.git_ops.branches.run_git") + @patch("pr_split.git_ops.prs._run_gh") + def test_fetch_fork_branch_uses_forced_refspec( + self, mock_gh: MagicMock, mock_git: MagicMock + ) -> None: + mock_gh.side_effect = [ + "repo", + json.dumps({"clone_url": "https://x/f.git", "full_name": "u/repo"}), + "main", + ] + mock_git.side_effect = ["", "A "] + fetch_fork_branch("u", "feature") + fetch_call = mock_git.call_args_list[0].args + assert fetch_call[2].startswith("+feature:") + + def test_forced_refspec_survives_amended_fork_head(self, tmp_path: Path) -> None: + def git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@x", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + fork = tmp_path / "fork" + fork.mkdir() + git(fork, "init", "-q", "-b", "feature") + (fork / "f.txt").write_text("one\n") + git(fork, "add", "f.txt") + git(fork, "commit", "-qm", "first") + + local = tmp_path / "local" + local.mkdir() + git(local, "init", "-q", "-b", "main") + git(local, "fetch", "-q", str(fork), "+feature:refs/pr-split/pr-1") + + # Rewrite the commit the local ref already points at: non-fast-forward. + (fork / "f.txt").write_text("two\n") + git(fork, "add", "f.txt") + git(fork, "commit", "-q", "--amend", "-m", "rewritten") + new_head = git(fork, "rev-parse", "HEAD") + + # Without "+" git refuses the non-fast-forward update. + with pytest.raises(subprocess.CalledProcessError): + git(local, "fetch", "-q", str(fork), "feature:refs/pr-split/pr-1") + git(local, "fetch", "-q", str(fork), "+feature:refs/pr-split/pr-1") + assert git(local, "rev-parse", "refs/pr-split/pr-1") == new_head diff --git a/tests/test_partitioning.py b/tests/test_partitioning.py index 9a9c85e..5f2d631 100644 --- a/tests/test_partitioning.py +++ b/tests/test_partitioning.py @@ -149,6 +149,32 @@ def solve_feasible(self: object, model: object) -> int: assert "0.5s limit" in warning assert "raise --cp-sat-timeout" in warning + @pytest.mark.parametrize( + ("status_name", "expected"), + [("UNKNOWN", "found no hunk assignment within 0.5s"), ("INFEASIBLE", "feasible hunk")], + ) + def test_solver_status_is_reported_distinctly( + self, monkeypatch: pytest.MonkeyPatch, status_name: str, expected: str + ) -> None: + cp_model = pytest.importorskip("ortools.sat.python.cp_model") + from unittest.mock import patch + + from pr_split.exceptions import PRSplitError + + parsed = parse_diff(SAMPLE_DIFF) + settings = _settings( + monkeypatch, + max_loc=10, + partition_strategy=PartitionStrategy.CP_SAT, + cp_sat_timeout=0.5, + ) + forced = getattr(cp_model, status_name) + with ( + patch.object(cp_model.CpSolver, "Solve", lambda self, model: forced), + pytest.raises(PRSplitError, match=expected), + ): + partition_diff(parsed, settings) + def test_sub_decisecond_timeout_is_reported_exactly( self, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 6fb6d3a..f1bbc51 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -27,6 +27,7 @@ def _make_diff_stats() -> DiffStats: is_deleted=False, is_renamed=False, hunk_count=2, + hunk_indices=[0, 1], ), FileSummary( path="bar.py", @@ -36,6 +37,7 @@ def _make_diff_stats() -> DiffStats: is_deleted=False, is_renamed=False, hunk_count=1, + hunk_indices=[0], ), ], ) @@ -60,6 +62,17 @@ def test_logical_includes_logical(self) -> None: assert "LOGICAL" in result assert "200" in result + def test_min_loc_adds_a_size_floor_rule(self) -> None: + result = build_system_prompt(Priority.ORTHOGONAL, 400, min_loc=100) + assert "not be smaller than about 100 lines" in result + assert "within approximately 400 lines" in result + + def test_no_min_loc_keeps_the_prompt_unchanged(self) -> None: + assert build_system_prompt(Priority.ORTHOGONAL, 400) == build_system_prompt( + Priority.ORTHOGONAL, 400, min_loc=None + ) + assert "smaller than" not in build_system_prompt(Priority.ORTHOGONAL, 400) + class TestBuildUserPrompt: def test_contains_file_summary(self) -> None: @@ -101,6 +114,7 @@ def test_zero_hunk_file(self) -> None: is_deleted=False, is_renamed=False, hunk_count=0, + hunk_indices=[], ), ], ) @@ -130,3 +144,28 @@ class TestBuildSystemPromptExtended: def test_returns_nonempty_string(self) -> None: result = build_system_prompt(Priority.ORTHOGONAL, 400) assert len(result) > 100 + + +class TestChunkSummaryIndices: + def test_later_chunk_lists_global_indices(self) -> None: + stats = DiffStats( + total_files=1, + total_added=1, + total_removed=0, + total_loc=1, + file_summaries=[ + FileSummary( + path="f.py", + added=1, + removed=0, + is_new=False, + is_deleted=False, + is_renamed=False, + hunk_count=2, + hunk_indices=[1, 3], + ) + ], + ) + result = build_user_prompt(stats, "diff") + assert "indices 1, 3 (this chunk only)" in result + assert "indices 0..1" not in result diff --git a/tests/test_reconstructor.py b/tests/test_reconstructor.py index a9d2303..fd13eef 100644 --- a/tests/test_reconstructor.py +++ b/tests/test_reconstructor.py @@ -13,6 +13,7 @@ materialize_group_files, merge_chain_assignments, split_git_lines, + target_file_modes, ) from pr_split.exceptions import GitOperationError from pr_split.schemas import Group, GroupAssignment @@ -617,3 +618,125 @@ def test_bare_carriage_return_does_not_shift_hunks(self, mock_run: MagicMock) -> ], ) assert materialize_group_files(parsed, group, "main")["c.txt"] == "x\ry\nZ\n" + + +class TestTruncatedFileIsNotDeleted: + @patch("pr_split.diff_ops.reconstructor._get_base_file_content", return_value="x\ny\n") + def test_file_emptied_on_dev_is_written_empty(self, mock_base: MagicMock) -> None: + parsed = parse_diff( + "diff --git a/a.txt b/a.txt\nindex 1111111..e69de29 100644\n" + "--- a/a.txt\n+++ b/a.txt\n@@ -1,2 +0,0 @@\n-x\n-y\n" + ) + assert parsed.patch_set[0].is_removed_file # the heuristic that misled us + group = Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path="a.txt", + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=[0], + ) + ], + ) + assert materialize_group_files(parsed, group, "main") == {"a.txt": ""} + + def test_real_deletion_still_removes(self) -> None: + parsed = parse_diff( + "diff --git a/a.txt b/a.txt\ndeleted file mode 100644\nindex 1111111..0000000\n" + "--- a/a.txt\n+++ /dev/null\n@@ -1,2 +0,0 @@\n-x\n-y\n" + ) + group = Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path="a.txt", + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=[0], + ) + ], + ) + assert materialize_group_files(parsed, group, "main") == {"a.txt": None} + + +class TestTargetFileModes: + def _group(self, *paths: str) -> Group: + return Group( + id="pr-1", + title="t", + description="d", + assignments=[ + GroupAssignment( + file_path=p, assignment_type=AssignmentType.WHOLE_FILE, hunk_indices=[0] + ) + for p in paths + ], + ) + + def test_mode_change_with_content_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/run.sh b/run.sh\nold mode 100644\nnew mode 100755\n" + "index 422c2b7..55dce13\n--- a/run.sh\n+++ b/run.sh\n@@ -1,2 +1,2 @@\n a\n-b\n+B\n" + ) + assert target_file_modes(parsed, self._group("run.sh")) == {"run.sh": 0o100755} + + def test_new_executable_file_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/n.sh b/n.sh\nnew file mode 100755\n--- /dev/null\n+++ b/n.sh\n" + "@@ -0,0 +1 @@\n+x\n" + ) + assert target_file_modes(parsed, self._group("n.sh")) == {"n.sh": 0o100755} + + def test_plain_change_and_unassigned_files_are_ignored(self) -> None: + parsed = parse_diff( + "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -1 +1 @@\n-x\n+y\n" + "diff --git a/b.sh b/b.sh\nold mode 100644\nnew mode 100755\n" + "--- a/b.sh\n+++ b/b.sh\n@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("a.py")) == {} + + def test_removed_file_is_ignored(self) -> None: + parsed = parse_diff( + "diff --git a/d.sh b/d.sh\ndeleted file mode 100755\n--- a/d.sh\n+++ /dev/null\n" + "@@ -1 +0,0 @@\n-x\n" + ) + assert target_file_modes(parsed, self._group("d.sh")) == {} + + def test_new_symlink_mode_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/link b/link\nnew file mode 120000\n--- /dev/null\n+++ b/link\n" + "@@ -0,0 +1 @@\n+other.py\n\\ No newline at end of file\n" + ) + assert target_file_modes(parsed, self._group("link")) == {"link": 0o120000} + + def test_retargeted_symlink_mode_comes_from_index_line(self) -> None: + parsed = parse_diff( + "diff --git a/link b/link\nindex d90d485..5f35558 120000\n--- a/link\n+++ b/link\n" + "@@ -1 +1 @@\n-other.py\n\\ No newline at end of file\n" + "+missing.py\n\\ No newline at end of file\n" + ) + assert target_file_modes(parsed, self._group("link")) == {"link": 0o120000} + + def test_unchanged_regular_mode_from_index_line(self) -> None: + parsed = parse_diff( + "diff --git a/a.py b/a.py\nindex 1111111..2222222 100644\n--- a/a.py\n+++ b/a.py\n" + "@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("a.py")) == {"a.py": 0o100644} + + def test_mode_change_back_to_non_executable_is_reported(self) -> None: + parsed = parse_diff( + "diff --git a/run.sh b/run.sh\nold mode 100755\nnew mode 100644\n" + "--- a/run.sh\n+++ b/run.sh\n@@ -1 +1 @@\n-x\n+y\n" + ) + assert target_file_modes(parsed, self._group("run.sh")) == {"run.sh": 0o100644} + + def test_file_truncated_to_empty_keeps_its_new_mode(self) -> None: + parsed = parse_diff( + "diff --git a/a.sh b/a.sh\nold mode 100644\nnew mode 100755\n" + "--- a/a.sh\n+++ b/a.sh\n@@ -1,2 +0,0 @@\n-x\n-y\n" + ) + assert target_file_modes(parsed, self._group("a.sh")) == {"a.sh": 0o100755} diff --git a/tests/test_score_pr_comment.py b/tests/test_score_pr_comment.py new file mode 100644 index 0000000..eefd1fb --- /dev/null +++ b/tests/test_score_pr_comment.py @@ -0,0 +1,84 @@ +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from types import ModuleType +from unittest.mock import patch + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "score_pr.py" +MARKER = "" + + +def _load_script() -> ModuleType: + spec = importlib.util.spec_from_file_location("score_pr_comment", SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules["score_pr_comment"] = module + spec.loader.exec_module(module) + return module + + +def _outputs(path: Path) -> dict[str, str]: + return dict(line.split("=", 1) for line in path.read_text().splitlines() if line) + + +class TestSkipWritesRefreshableComment: + def _prepare(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> tuple[ModuleType, Path]: + output_file = tmp_path / "output.txt" + output_file.touch() + monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) + return _load_script(), output_file + + def test_under_threshold_skip_writes_marker_comment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + module, output_file = self._prepare(tmp_path, monkeypatch) + + module._skip("PR has 12 LOC — under the 400 threshold.", within_limits=True) + + outputs = _outputs(output_file) + assert outputs["should_split"] == "false" + body = Path(outputs["comment_path"]).read_text() + assert body.startswith(MARKER) + assert "within acceptable size limits" in body + assert "under the 400 threshold" in body + + def test_plain_skip_writes_no_comment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + module, output_file = self._prepare(tmp_path, monkeypatch) + + module._skip("Not a pull_request event; skipping.") + + outputs = _outputs(output_file) + assert outputs["should_split"] == "false" + assert "comment_path" not in outputs + + def test_main_under_threshold_writes_within_limits_comment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + module, output_file = self._prepare(tmp_path, monkeypatch) + monkeypatch.setenv("BASE_BRANCH", "main") + monkeypatch.setenv("HEAD_BRANCH", "feature") + monkeypatch.setenv("PR_NUMBER", "7") + monkeypatch.setenv("MAX_LOC", "400") + + with ( + patch.object(module, "_run") as mock_run, + patch.object(module.subprocess, "run") as mock_planner, + ): + mock_run.return_value.stdout = "10\t2\ta.py\n" + module.main() + + mock_planner.assert_not_called() + outputs = _outputs(output_file) + assert outputs["total_loc"] == "12" + assert outputs["should_split"] == "false" + body = Path(outputs["comment_path"]).read_text() + assert body.startswith(MARKER) + assert "PR has 12 LOC" in body + assert "within acceptable size limits" in body diff --git a/tests/test_score_pr_events.py b/tests/test_score_pr_events.py index 38b10aa..3784cdb 100644 --- a/tests/test_score_pr_events.py +++ b/tests/test_score_pr_events.py @@ -10,6 +10,8 @@ import pytest +from pr_split.plan_store import plan_key + SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "score_pr.py" @@ -69,6 +71,7 @@ def test_pull_request_event_still_fetches( monkeypatch.setenv("HEAD_BRANCH", "feature") monkeypatch.setenv("PR_NUMBER", "7") monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) module = _load_script() with patch.object(module, "_run") as mock_run: @@ -104,8 +107,10 @@ def _run_main( def fake_planner(cmd: list[str], **kwargs: object) -> object: if plan is not None: - (tmp_path / ".pr-split").mkdir(exist_ok=True) - (tmp_path / ".pr-split" / "plan.json").write_text(json.dumps(plan)) + # Where pr-split saves the plan for dev branch pr-split/head-7. + plans = tmp_path / ".pr-split" / "plans" + plans.mkdir(parents=True, exist_ok=True) + (plans / f"{plan_key('pr-split/head-7')}.json").write_text(json.dumps(plan)) return subprocess.CompletedProcess(cmd, returncode, "", "boom") with ( diff --git a/tests/test_score_pr_refs.py b/tests/test_score_pr_refs.py new file mode 100644 index 0000000..e483b78 --- /dev/null +++ b/tests/test_score_pr_refs.py @@ -0,0 +1,99 @@ +from __future__ import annotations + +import importlib.util +import subprocess +import sys +from pathlib import Path +from types import ModuleType +from unittest.mock import patch + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "score_pr.py" + + +def _load_script() -> ModuleType: + spec = importlib.util.spec_from_file_location("score_pr_refs", SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules["score_pr_refs"] = module + spec.loader.exec_module(module) + return module + + +class TestScoreUsesFetchedRefsDirectly: + def test_head_named_like_base_does_not_clobber_base( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + output_file = tmp_path / "output.txt" + output_file.touch() + monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) + monkeypatch.setenv("BASE_BRANCH", "main") + monkeypatch.setenv("HEAD_BRANCH", "main") # fork PR opened from the fork's main + monkeypatch.setenv("PR_NUMBER", "7") + monkeypatch.setenv("MAX_LOC", "1") + module = _load_script() + + git_calls: list[list[str]] = [] + + def fake_run(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess[str]: + git_calls.append(cmd) + stdout = "5\t3\ta.py\n" if cmd[:2] == ["git", "diff"] else "" + return subprocess.CompletedProcess(cmd, 0, stdout=stdout, stderr="") + + split_cmds: list[list[str]] = [] + + def fake_subprocess_run( + cmd: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + split_cmds.append(cmd) + return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="stub") + + with ( + patch.object(module, "_run", side_effect=fake_run), + patch.object(module.subprocess, "run", side_effect=fake_subprocess_run), + ): + module.main() + + branch_calls = [c for c in git_calls if c[:2] == ["git", "branch"]] + assert branch_calls == [["git", "branch", "-f", "main", "origin/main"]] + assert len(split_cmds) == 1 + split = split_cmds[0] + assert split[:3] == ["pr-split", "split", "pr-split/head-7"] + assert split[split.index("--base") + 1] == "main" + + def test_branch_event_without_pr_number_uses_origin_head( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + output_file = tmp_path / "output.txt" + output_file.touch() + monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) + monkeypatch.setenv("BASE_BRANCH", "main") + monkeypatch.setenv("HEAD_BRANCH", "feature") + monkeypatch.delenv("PR_NUMBER", raising=False) + monkeypatch.setenv("MAX_LOC", "1") + module = _load_script() + + def fake_run(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess[str]: + stdout = "5\t3\ta.py\n" if cmd[:2] == ["git", "diff"] else "" + return subprocess.CompletedProcess(cmd, 0, stdout=stdout, stderr="") + + split_cmds: list[list[str]] = [] + + def fake_subprocess_run( + cmd: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + split_cmds.append(cmd) + return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="stub") + + with ( + patch.object(module, "_run", side_effect=fake_run), + patch.object(module.subprocess, "run", side_effect=fake_subprocess_run), + ): + module.main() + + split = split_cmds[0] + assert split[2] == "origin/feature" + assert split[split.index("--base") + 1] == "main" diff --git a/tests/test_score_pr_table.py b/tests/test_score_pr_table.py new file mode 100644 index 0000000..e0c2835 --- /dev/null +++ b/tests/test_score_pr_table.py @@ -0,0 +1,113 @@ +from __future__ import annotations + +import importlib.util +import json +import re +import subprocess +import sys +from pathlib import Path +from types import ModuleType +from unittest.mock import patch + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "score_pr.py" + + +def _load_script() -> ModuleType: + spec = importlib.util.spec_from_file_location("score_pr_table", SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules["score_pr_table"] = module + spec.loader.exec_module(module) + return module + + +def _outputs(path: Path) -> dict[str, str]: + return dict(line.split("=", 1) for line in path.read_text().splitlines() if line) + + +class TestMdEscape: + def test_pipes_and_newlines_are_neutralised(self) -> None: + module = _load_script() + assert module._md_escape("a | b") == "a \\| b" + assert module._md_escape("first\nsecond\r\nthird") == "first second third" + + +class TestCommentTable: + def _prepare(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> tuple[ModuleType, Path]: + output_file = tmp_path / "output.txt" + output_file.touch() + monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) + monkeypatch.setenv("BASE_BRANCH", "main") + monkeypatch.setenv("HEAD_BRANCH", "feature") + monkeypatch.setenv("MAX_LOC", "10") + monkeypatch.chdir(tmp_path) + return _load_script(), output_file + + def test_dependency_ids_and_multiline_titles_stay_in_their_cells( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + module, output_file = self._prepare(tmp_path, monkeypatch) + groups = [ + { + "id": "pr-1|x", + "title": "a | b\nsecond line", + "depends_on": [], + "assignments": [{"file_path": "a.py"}], + "estimated_loc": 300, + "estimated_added": 300, + "estimated_removed": 0, + }, + { + "id": "pr-2", + "title": "t", + "depends_on": ["pr-1|x"], + "assignments": [{"file_path": "b.py"}], + "estimated_loc": 300, + "estimated_added": 300, + "estimated_removed": 0, + }, + ] + + def fake_run(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess[str]: + stdout = "600\t0\ta.py\n" if cmd[:2] == ["git", "diff"] else "" + return subprocess.CompletedProcess(cmd, 0, stdout=stdout, stderr="") + + def fake_pr_split(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + plan_dir = tmp_path / ".pr-split" + plan_dir.mkdir(exist_ok=True) + (plan_dir / "plan.json").write_text(json.dumps({"groups": groups})) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + with ( + patch.object(module, "_run", side_effect=fake_run), + patch.object(module.subprocess, "run", side_effect=fake_pr_split), + ): + module.main() + + comment = Path(_outputs(output_file)["comment_path"]).read_text() + rows = [line for line in comment.splitlines() if line.startswith("| pr-")] + assert len(rows) == 2 + assert rows[0].startswith("| pr-1\\|x | a \\| b second line |") + assert "| pr-1\\|x |" in rows[1] + # five columns => exactly six unescaped cell separators per row + assert all(len(re.findall(r"(? None: + output_file = tmp_path / "output.txt" + output_file.touch() + monkeypatch.setenv("GITHUB_OUTPUT", str(output_file)) + monkeypatch.setenv("BASE_BRANCH", "main") + monkeypatch.setenv("HEAD_BRANCH", "feature") + monkeypatch.setenv("MIN_LOC", "abc") + module = _load_script() + with patch.object(module, "_run") as mock_run, pytest.raises(SystemExit): + module.main() + assert "MIN_LOC must be an integer, got 'abc'" in capsys.readouterr().err + mock_run.assert_not_called() diff --git a/tests/test_types_defs.py b/tests/test_types_defs.py index 9bce6b2..797b3ca 100644 --- a/tests/test_types_defs.py +++ b/tests/test_types_defs.py @@ -55,6 +55,7 @@ def test_dict_access(self) -> None: "is_deleted": False, "is_renamed": False, "hunk_count": 2, + "hunk_indices": [0, 1], } assert fs["path"] == "test.py" assert fs["added"] == 10 diff --git a/tests/test_validator.py b/tests/test_validator.py index eef8222..b935a9a 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -121,7 +121,31 @@ def test_duplicate_assignment_raises(self) -> None: 7, ), ] - with pytest.raises(PlanValidationError, match="multiple groups"): + with pytest.raises(PlanValidationError, match="multiple groups: g1, g2"): + validate_coverage(groups, parsed) + + def test_same_group_listing_hunk_twice_is_reported_as_duplicate(self) -> None: + parsed = parse_diff(SAMPLE_DIFF) + groups = [ + _make_group( + "g1", + [_ga("a.py", PARTIAL, [0]), _ga("a.py", WHOLE, [])], + 3, + ), + _make_group("g2", [_ga("b.py", WHOLE, [0])], 4), + ] + with pytest.raises(PlanValidationError) as exc_info: + validate_coverage(groups, parsed) + assert str(exc_info.value) == "Hunk a.py[0] listed more than once in group 'g1'" + assert "multiple groups" not in str(exc_info.value) + + def test_repeated_index_in_one_assignment_is_reported_as_duplicate(self) -> None: + parsed = parse_diff(SAMPLE_DIFF) + groups = [ + _make_group("g1", [_ga("a.py", PARTIAL, [0, 0])], 3), + _make_group("g2", [_ga("b.py", WHOLE, [0])], 4), + ] + with pytest.raises(PlanValidationError, match=r"more than once in group 'g1'"): validate_coverage(groups, parsed) @@ -275,3 +299,29 @@ def test_stale_whole_file_index_does_not_create_a_phantom_conflict(self) -> None _make_group("g2", [_ga("b.py", WHOLE, [0]), _ga("a.py", PARTIAL, [99])], 4), ] validate_no_conflicts(groups, PlanDAG(groups), {"a.py": 1, "b.py": 1}) + + +MODE_ONLY_DIFF = """\ +diff --git a/run.sh b/run.sh +old mode 100644 +new mode 100755 +diff --git a/a.py b/a.py +--- a/a.py ++++ b/a.py +@@ -1 +1 @@ +-x ++y +""" + + +class TestValidateNoHunklessFiles: + def test_mode_only_change_raises(self) -> None: + parsed = parse_diff(MODE_ONLY_DIFF) + with pytest.raises(PlanValidationError, match=r"no text hunks.*run\.sh"): + validate_no_binary_files(parsed) + + def test_validate_plan_rejects_mode_only_even_when_text_hunks_are_covered(self) -> None: + parsed = parse_diff(MODE_ONLY_DIFF) + groups = [_make_group("g1", [_ga("a.py", WHOLE, [0])], 2)] + with pytest.raises(PlanValidationError, match="no text hunks"): + validate_plan(groups, parsed, PlanDAG(groups), max_loc=400)