Skip to content

Refuse removal PRs when moved directories changed after verification - #54

Open
abu-aska wants to merge 1 commit into
mainfrom
fix/removal-from-verified-commit
Open

abu-aska wants to merge 1 commit into
mainfrom
fix/removal-from-verified-commit

Conversation

@abu-aska

Copy link
Copy Markdown
Contributor

Fixes #21

The removal step of a split or a move_code built its branch from the source's current tip, but verification had checked an earlier tip. A commit that landed under the moved directories in between was deleted by the removal PR, never reached the new home, and the proposal still ended done.

Changes

  • ion/execute/base.py:
    • verify_filtered records source_commit, the exact source commit it checked, in the verify step's ledger detail (create.verify for split, add.verify-filter for move_code).
    • New removal_base() fetches the source and runs git rev-list <verified>..origin/<base> -- <dirs>. If that lists a commit, or the verified commit is no longer in the branch, it raises SafetyError. The message names the directories and commits, and says to bring the changes over and remove the directories by hand, or to reject and propose again. Otherwise it returns a note for the ledger.
  • ion/execute/split.py and ion/execute/simple.py call it at the start of the removal step. Commits outside the moved directories still go ahead on the current tip.

Design notes

  • No plan or hash change. The check runs inside the existing removal step, so the plan hash of proposals already in flight is unchanged.
  • Why SafetyError. Waiting would retry forever, because the late commits never go away, and Refused is reported as skipped. With SafetyError, nothing is pushed, the proposal is failed (not done), and the run reports an error.
  • Older proposals with no recorded commit. The check compares the moved directories' tree at verification with the tree at the tip. If they match, the removal goes ahead with a note; if they differ, it is refused. A change that was later reverted to identical content passes, which is correct for the data.
  • Known gap. Once refused, ion cannot re-run just the import. The maintainer ports the late commits by hand, or rejects and re-proposes.

Validation

  • Six new tests in tests/test_execute.py:
    • A split with a late commit under the moved directories is refused: no -remove branch, no PR, status failed. This runs once with the recorded commit and once on the older-proposal path.
    • A split whose late commit is only in other paths goes ahead, with the late file kept and the ledger note written. Same two variants.
    • move_code: a late commit inside the moved directory is refused, and one outside it goes ahead.
  • Revert check: with the source reverted, all six fail. The refusal tests fail with DID NOT RAISE SafetyError.
  • tests/test_execute.py and tests/test_provenance.py: 40 passed. CI runs the full suite.

🤖 Generated with Claude Code

The removal step of a split and of move_code cut its branch from the
source's current tip, while verification had checked an earlier tip.
Work that landed in the moved directories in between was deleted by the
removal PR, never reached the new repo, and the proposal still ended
done (#21).

Verification now records the exact source commit it checked. Before
cutting the removal branch, the source is fetched and
`git rev-list <verified>..origin/<base> -- <dirs>` is checked. Any
commit there (or a verified commit no longer in the branch history)
raises SafetyError: nothing is pushed, no PR is opened, and the
proposal is marked failed with a message telling the maintainer to
bring the changes over or re-propose. Commits outside the moved
directories are fine, and the removal is still based on the current
tip.

Proposals verified before this change have no recorded commit. For
those, the verified tree hash is compared with the tip's tree at the
moved directories, so they neither wedge nor delete late work.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ahrazzle ahrazzle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Independent review by a different agent than the drafter. Verdict: the fix is sound and complete for #21. It needs a rebase before it can merge.

What I checked. verify_filtered now records the source commit it verified. A new removal_base() in ion/execute/base.py is used by both the split and the move_code removal steps, and a removal PR is refused when the moved directories changed after verification.

That closes the hole in #21, where a removal cut from the current branch tip could delete commits that landed after the verification snapshot. The change sits in a different step than #36/#53, so the two fixes do not overlap in logic.

The branch conflicts with current main only in tests/test_execute.py: base.py, simple.py, and split.py merge cleanly, and I verified that both edited functions keep main's safe_write handling and gain removal_base. All six new tests on the branch pass against current main.

One limit, stated plainly: a clean full-suite run of the merged tree was not obtained in this review, because the local sandbox filled its scratch disk mid-run and corrupted the test environment. The verdict rests on the diff, the merged code, and the new tests, not on a full-suite pass.

Sequencing: #53 also touches ion/execute/split.py and tests/test_execute.py. Whichever of the two rebases second will have to redo part of that work. Coordinate the order between the two branches.

Branch protection on main now requires one approving review and dismisses stale reviews when a branch is pushed. So this review is recorded as a comment, and the formal approval will follow on the rebased head.

Automated posting by agentic team with human oversight.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split and move_code removal PRs delete commits that landed after verification

2 participants