fix: carry every ancestor's hunks into stacked descendants

[vitali87]

Summary

In _stacked_batch_args, a single-parent group merged only effective[parent]. Ancestor hunks therefore reached a child only through files the parent also edited.

Scenario: A edits f.py hunk 0 → B (depends on A) edits only g.py → C (depends on B) edits f.py hunk 1. C's effective assignments were f.py: [1]. C's branch is cut from B's (which contains A's hunk 0), but materialize_group_files rebuilds f.py from the merge base plus hunk 1 only, so C's commit silently deleted A's change. validate_no_conflicts allows this because A and C are related.

Fix: merge all of dag.ancestors(gid), as the multi-parent branch already does. Branch start point and PR base are unchanged.

Test plan

• New test_grandchild_carries_grandparent_hunks_in_shared_file (fails on main: {'f.py': [1]})
• uv run ruff check / ruff format --check clean
• uv run pytest -q — 445 passed

[coderabbitai]

Warning

Review limit reached

Next included review available in 42 minutes.

View limit details

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

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3a316671-0108-43b8-b24a-cd885f57c720

📥 Commits

Reviewing files that changed from the base of the PR and between 3f9581d and f4682b8.

📒 Files selected for processing (2)

• pr_split/cli.py
• tests/test_cli_helpers.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

The stacked-branch update now carries assignments from all ancestors when rebuilding descendant files, preventing inherited hunks from being reverted. Execution across root, linear-descendant, and merge-node branch shapes confirmed the new merging behavior. One cleanup remains in pr_split/cli.py: an obsolete cache is populated but never used.

Confidence Score: 4/5

The ancestor-merging behavior was exercised for root, linear-descendant, and merge-node cases, with focused helper tests passing.

The only finding is a verified maintainability issue: a local mapping is written but never read, so it obscures the active assignment-merging path without changing runtime output.

Files Needing Attention: pr_split/cli.py needs the unused effective mapping declaration and assignment removed.

T-Rex Logs

What T-Rex did

• T-Rex ran the executable stacked-batch mapping validation harness to exercise the posted P2 finding.
• The harness captured the Original stacked-batch behavior and the mapping-removed behavior to enable a side-by-side comparison.
• The harness produced an output comparison showing identical results between the original and mapping-removed paths.
• Targeted stacked branch helper tests were executed and documented.
• Contract validation completed without repository changes, and the original and in-memory variants produced identical results; maximum steps for this agent were reached.

_{Ran code and verified through T-Rex}

Comments Outside Diff (2)

1. pr_split/cli.py, line 239-272 (link)

   Effective cache is write-only state

   effective is initialized and populated for every group, but _stacked_batch_args never reads it after the branch arguments began using merged directly. Removing the declaration and assignment leaves root, linear-descendant, and merge-node outputs unchanged. Remove this obsolete state so the ancestor-merging flow is not obscured.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

   Artifacts

   
   

   Executable stacked-batch mapping validation harness

   • Authored Python harness parses and disassembles `_stacked_batch_args`, then executes original and in-memory mapping-removed variants across three DAG shapes, proving the mapping has no read path or behavioral effect—the takeaway.

   Original stacked-batch behavior capture

   • Captured successful execution of the checked-in `_stacked_batch_args` for root, linear-grandchild, and merge-node inputs, including empty `LOAD_FAST effective` offsets—the takeaway.

   Mapping-removed stacked-batch behavior capture

   • Captured successful execution of an in-memory `_stacked_batch_args` variant with only lines 239 and 272 removed, yielding the same normalized case outputs as the original—the takeaway.

   Original and mapping-removed output comparison

   • Captured the successful assertion that original and mapping-removed outputs are identical for all three cases and that original bytecode never loads `effective`—the takeaway.

   Targeted stacked branch helper tests

   • Captured successful targeted pytest execution for grandchild assignment carry-forward and merge-node stacked-batch behavior, with all selected tests passing—the takeaway.

   

   _{Ran code and verified through T-Rex}

2. General comment

   effective mapping is dead write-only state in _stacked_batch_args

   • Bug
     • pr_split/cli.py initializes effective at line 239 and assigns effective[gid] = merged at line 272, but no compiled path reads the mapping. Removing both lines in memory leaves all exercised stacked-batch results unchanged.
   • Cause
     • The mapping was retained after the output path was changed to append merged directly to batch_args.
   • Fix
     • Remove the effective declaration at line 239 and its assignment at line 272.

   _{Ran code and verified through T-Rex}

_{Reviews (1): Last reviewed commit: "fix: carry every ancestor's hunks into s..." | Re-trigger Greptile}

[vitali87]

Removed the now-unused effective mapping flagged by Greptile; ancestors are read straight from groups_by_id.