Skip to content

AIP-20: Build create-review-worktree skill - #266

Draft
jodavis-claude wants to merge 3 commits into
dev/claude/AIP-12-specfrom
dev/claude/AIP-20
Draft

jodavis-claude wants to merge 3 commits into
dev/claude/AIP-12-specfrom
dev/claude/AIP-20

Conversation

@jodavis-claude

Copy link
Copy Markdown
Collaborator

Work item

AIP-20: Build create-review-worktree, a skill that, given a resolved PR reference (from resolve-pr-reference's JSON output), stands up an isolated working copy checked out to that PR's head — an EnterWorktree worktree for a same-repo PR, a git worktree add from a sibling clone or a scratch gh repo clone for a cross-repo PR — so every later Code Review Helper deliverable can operate on the PR's code without touching the reviewer's own checkout.

Changes

  • plugins/dev-team/skills/create-review-worktree/SKILL.md (new) — the skill itself. Classifies which isolation mechanism a resolved PR needs, then either (a) same-repo: calls the native EnterWorktree tool, runs gh pr checkout <number> -R <owner>/<repo>, and returns isolation_kind: enterworktree, or (b) cross-repo: delegates to the script's finish-cross-repo command, returning isolation_kind: sibling-worktree or isolation_kind: scratch-clone. Documents an explicit return contract (isolation_kind, worktree_path, head_ref) since the spec's prose didn't fix one.
  • plugins/dev-team/skills/create-review-worktree/scripts/create_review_worktree.py (new) — deterministic classification and cross-repo execution logic: parse_owner_repo, get_current_repo, get_repo_root, sibling_matches, classify_isolation, finish_cross_repo, and a main() CLI (classify / finish-cross-repo subcommands, JSON in, JSON out).
  • plugins/dev-team/skills/create-review-worktree/scripts/test_create_review_worktree.py (new) — 35 pytest tests covering every deterministic branch, including one real (non-mocked) local git integration test for the sibling-worktree path.

Design decisions

  • Split a testable script out of the Orchestrator: classify_isolation/finish_cross_repo get full branch-coverage pytest (mirroring decide_cleanup_action.py's precedent), while the untestable EnterWorktree call stays as thin, manually-verified SKILL.md prose.
  • Sibling-directory match is verified by comparing the sibling's own git remote get-url origin against the target owner/repo, case-insensitively — resolves an ambiguity the spec left open.
  • git worktree add / gh repo clone / gh pr checkout failures propagate as RuntimeError with the underlying stderr — no structured failure taxonomy invented for this component.
  • Both cross-repo mechanisms (sibling-worktree, scratch-clone) share the same target directory naming convention: .claude/worktrees/review-<owner>-<repo>-<number>/.
  • No Gherkin/E2E scenarios were written — no .feature/Gherkin harness exists anywhere in this repo (confirmed via search), so per missing-test-harness none was invented. The real-git integration test is the closest existing analog for exercising the Orchestrator's primary wiring end-to-end.

Testing completed

  • plugins/dev-team/skills/create-review-worktree/scripts/test_create_review_worktree.py: 35 unit/integration tests, all passing (python3 -m pytest test_create_review_worktree.py -q35 passed).
  • Repo build/test validation scripts run clean (see linked build log in workflow context).

Implements same-repo isolation via EnterWorktree + gh pr checkout, cross-repo isolation via a sibling-clone git worktree add when one exists, and a scratch gh repo clone fallback otherwise. The deterministic classify/finish-cross-repo logic lives in create_review_worktree.py, pytest-tested (mocked subprocess + one real, non-mocked git integration test) mirroring decide_cleanup_action.py's precedent; the native EnterWorktree call itself stays as thin SKILL.md prose, since it is only callable from the agent's own tool-use turn.

@jodavis-claude jodavis-claude left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

First-pass review of AIP-20 (create-review-worktree). Exit criteria are met and the deterministic classification/cross-repo logic is thoroughly tested (35 passing pytest tests, including one real non-mocked git integration test for the sibling-worktree path). One correctness/fault-tolerance issue found in the sibling-worktree branch handling (inline comment below) that I'd like addressed before sign-off; one minor documentation clarity note as well.

Comment thread plugins/dev-team/skills/create-review-worktree/scripts/create_review_worktree.py Outdated
Comment thread plugins/dev-team/skills/create-review-worktree/SKILL.md Outdated
…olation path

git worktree add -b <branch> ... created a named branch in the sibling clone's own repo that the immediately-following gh pr checkout never reused (it creates/switches to the PR's own real head branch instead). This orphaned the transient branch permanently, and made a second review of the same owner/repo/number fail outright since -b refuses to reuse an existing branch name. Switched to git worktree add --detach, since gh pr checkout creates/switches the real branch on its own right after. Added a real-git integration test that asserts no review-<owner>-<repo>-<number> branch is left behind in the sibling clone.
'Skip step 3 entirely' sat right after step 2's own internal numbered list (which has its own item 3), reading as if step 2's own item 3 were optional. It actually referred to the top-level Cross-repo section that follows. Reworded per review feedback.

@jodavis-claude jodavis-claude left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sign-off review for AIP-20 (create-review-worktree).

Prior review threads (2/2 resolved):

  1. Stray branch left in sibling clone (Priority 2, correctness/fault-tolerance) — fixed in commit c75849a. finish_cross_repo()'s sibling-worktree path now uses git worktree add --detach <target_dir> HEAD instead of -b <branch>, so no orphaned review-<owner>-<repo>-<number> branch is left behind in the sibling clone, and a second review of the same PR no longer fails with "branch already exists". Verified: new regression test test_finish_cross_repo_sibling_worktree_does_not_leave_a_stray_branch_in_sibling_clone is a real (non-mocked) git integration test that fails against the pre-fix code and passes against the fix. Thread resolved.
  2. SKILL.md "skip step 3" wording ambiguity (Priority 5, documentation clarity) — fixed in commit 67f3921. Reworded to "The Cross-repo section below does not apply to the same-repo path," matching the reviewer's suggested wording and removing the ambiguity with step 2's own internal item 3. Thread resolved.

New issues in modified files (commits c75849a, 67f3921): none found. Reviewed create_review_worktree.py's changed lines, SKILL.md's changed line, and the new test's mocking pattern (matches the existing _passthrough_except_gh real-git precedent already used by the sibling class in this file, not a new deviation).

Verification: all 36 tests pass (python3 -m pytest test_create_review_worktree.py -q36 passed).

Approved — no further changes requested.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants