Skip to content

fix: add worktree cleanup handling to merge-pr command#1248

Open
dyoshikawa-claw wants to merge 6 commits intomainfrom
fix/issue-1165
Open

fix: add worktree cleanup handling to merge-pr command#1248
dyoshikawa-claw wants to merge 6 commits intomainfrom
fix/issue-1165

Conversation

@dyoshikawa-claw
Copy link
Collaborator

Summary

  • update .rulesync/commands/merge-pr.md Step 6 to support worktree-aware cleanup
  • add guidance to detect branch worktree mapping via git worktree list --porcelain
  • prefer git gtr rm <worktree-name> when a worktree exists, and keep existing branch cleanup fallback

Fixes #1165

@dyoshikawa-claw

This comment has been minimized.

@github-actions

This comment has been minimized.

@dyoshikawa-claw
Copy link
Collaborator Author

Addressed in latest commit: 9a121ba1

Updated .rulesync/commands/merge-pr.md Step 6 to:

  • clarify how to parse git worktree list --porcelain and map branch to worktree <path>
  • add explicit quoting for placeholders (e.g. "<worktree-name>", "<branch-name>")
  • make fallback cleanup command explicit for non-main worktree context using git -C "<worktree-path>" ...

Also confirmed pnpm cicheck passes locally before push.

@dyoshikawa-claw

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dyoshikawa

This comment has been minimized.

@github-actions

This comment has been minimized.

@dyoshikawa-claw

This comment has been minimized.

@github-actions

This comment has been minimized.

@dyoshikawa-claw
Copy link
Collaborator Author

Addressed in latest commit: 5aea99ff

Updated .rulesync/commands/merge-pr.md Step 6 to cover all requested high-severity points:

  • clarified porcelain parsing behavior including detached-HEAD blocks and branch-not-found handling
  • clarified <worktree-name> resolution via git gtr list --porcelain path match (no basename assumption)
  • added guidance for running from inside the target worktree (move to safe worktree first)
  • added explicit input validation guidance for <branch-name>, <base-branch>, and <worktree-path> to prevent unsafe expansion/substitution
  • added -- separator usage in destructive git/gtr commands

Also confirmed pnpm cicheck passes before push.

@dyoshikawa-claw

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2026

Overall Mergeability Verdict for PR #1248

NOT READY TO MERGE - One high severity and one medium severity issue identified that should be addressed before merging.


Code Review Results (PR #1248)

🔴 Critical Issues: 0

None identified.

🟠 High Severity Issues: 1

1. Undefined Variable Risk in Fallback Scenario (Code Quality)

  • Location: Step 2 (line 99) → Step 7 (line 122)
  • Issue: Step 2 instructs to "Capture a fallback <worktree-path> from the first worktree <path> entry." However, if no worktree entry exists (extremely rare but theoretically possible in a corrupted or unusual repo state), <worktree-path> would be undefined when Step 7's fallback command executes
  • Impact: Command would fail with an empty path argument
  • Recommendation: Add explicit handling: "If no worktree entry exists at all, report an error to the user and skip cleanup"

🟡 Medium Severity Issues: 7

2. Unclear Variable Scoping Between Steps (Code Quality)

  • Location: Steps 2, 4, 7 (lines 96-122)
  • Issue: The variable <worktree-path> is used in three different contexts without clear distinction
  • Recommendation: Use distinct variable names: <fallback-worktree-path> in Step 2, <branch-worktree-path> in Step 4

3. Missing Validation Guidance for <worktree-name> (Security)

  • Location: Lines 109-115
  • Issue: <worktree-name> is resolved from git gtr list --porcelain output and used in the destructive git gtr rm command, but there's no explicit validation guidance (unlike other inputs)
  • Recommendation: Add validation: "<worktree-name> must come from git gtr porcelain output. Reject it if it contains $, backticks, or newlines"

4. Missing Error Handling Guidance (Code Quality)

  • Location: Steps 6 and 7 (lines 111-122)
  • Issue: No guidance on handling command failures for cleanup operations
  • Recommendation: Add: "If any cleanup command fails, report the error to the user but do not block. The merge has already succeeded"

5. Incomplete Validation Failure Guidance (Code Quality)

  • Location: Step 3 (lines 100-102)
  • Issue: Says to "reject" invalid inputs but doesn't specify what action to take
  • Recommendation: Add explicit action: "If validation fails, report the security concern and skip automatic cleanup"

6. Undocumented git gtr Porcelain Format (Code Quality)

  • Location: Step 4 (line 109)
  • Issue: Assumes git gtr list --porcelain has name and path fields without documenting the format
  • Recommendation: Add format documentation similar to Step 2

7. Missing Handling for No Match in git gtr list (Code Quality)

  • Location: Step 4 (line 109)
  • Issue: If <worktree-path> doesn't match any path in git gtr list, <worktree-name> would be undefined
  • Recommendation: Add fallback guidance

8. Inconsistent Validation Strictness for <worktree-path> (Security)

  • Location: Line 102
  • Issue: Validation for <worktree-path> is less strict than for <branch-name> and <base-branch>
  • Recommendation: Either add more characters to reject list or document why validation is intentionally relaxed

🟢 Low Severity Issues: 6

9. Missing Guidance for Determining "Inside Worktree"

  • Location: Step 5 (line 111)
  • Recommendation: Add: "Check if the current working directory path starts with <worktree-path>"

10. Potential Branch Deletion Failure in Fallback

  • Location: Step 7 (line 122)
  • Recommendation: Consider using -D or note that this failure is acceptable

11. Incomplete Last Block Handling Guidance

  • Location: Step 2 (line 96)
  • Recommendation: Note that the last block may not have a trailing blank line

12. git -C Path Handling Without Explicit Dash Protection

  • Location: Line 122
  • Recommendation: For consistency, consider adding - to <worktree-path> rejection list

13. Reliance on AI Implementation of Validation

  • Location: Lines 100-102
  • Note: Security depends on AI correctly implementing validation logic

14. Missing Concrete Porcelain Output Example

  • Could add example output for clarity

Positive Aspects

Strong security posture - Comprehensive input validation guidance prevents injection attacks
Safe command construction - Use of -- separator in destructive commands
Clear porcelain parsing - Good explanation of block-based parsing and detached HEAD handling
Handles running from inside worktree - Step 5 addresses the edge case
Proper quoting - All variables are properly quoted in commands
Good defense-in-depth approach - Multiple layers of protection
Clear trust boundaries - Distinguishes between user input and porcelain output
Responsive to feedback - Author has iteratively addressed previous review concerns


Summary

Category Critical High Medium Low
Code Quality 0 1 5 3
Security 0 0 2 3
Total 0 1 7 6

Recommendation: Address the 1 high-severity issue (undefined fallback path handling) before merging. The medium and low severity issues could be addressed in a follow-up PR if needed, though addressing issue #3 (missing validation for <worktree-name>) would strengthen the security posture.

github run

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.

Add worktree cleanup to merge-pr command

2 participants