chore: implement agent-review session improvements#27
Conversation
Improvements identified from session review: - worktree-helper.sh: Detect squash-merged branches by checking for deleted remote branches (fixes cleanup not detecting squash merges) - pr.md: Add 'Handling Contradictory AI Feedback' section with verification and resolution process - worktree.md: Document squash merge detection and manual cleanup - postflight.md: Add worktree cleanup reminder after PR merge - AGENTS.md: Clarify when direct-to-main (option 4) is acceptable (docs-only changes) vs not acceptable (code changes)
WalkthroughThese changes enhance the git worktree management system by adding improved merge detection logic to the helper script and documenting new cleanup workflows. Documentation is updated to clarify conditional acceptance criteria and provide guidance on contradictory feedback handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the development workflow by introducing smarter worktree management and clearer guidelines for interacting with automated review systems. The core improvement is the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sun Jan 11 00:44:20 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
🤖 Augment PR SummarySummary: This PR improves agent workflow guidance and enhances worktree cleanup to better handle GitHub’s common squash-merge pattern. Changes:
Technical Notes: Squash-merge detection relies on the remote branch being deleted (post-prune), which is typical for 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| is_merged=true | ||
| merge_type="merged" | ||
| # Check 2: Remote branch deleted (indicates squash merge or PR closed) | ||
| elif ! git show-ref --verify --quiet "refs/remotes/origin/$worktree_branch" 2>/dev/null; then |
There was a problem hiding this comment.
The “remote deleted” heuristic (git show-ref ... refs/remotes/origin/$worktree_branch) will also be true for branches that were never pushed (or repos without an origin remote), which could mark active/unmerged worktrees as merged and delete them after confirmation. Consider gating this check on the branch having an origin/<branch> (or upstream) to avoid false positives.
🤖 Was this useful? React with 👍 or 👎
| git worktree remove "$worktree_path" 2>/dev/null || true | ||
| git worktree remove "$worktree_path" 2>/dev/null || git worktree remove --force "$worktree_path" 2>/dev/null || true | ||
| # Also delete the local branch | ||
| git branch -D "$worktree_branch" 2>/dev/null || true |
There was a problem hiding this comment.
cmd_clean now runs git branch -D (and may worktree remove --force), which can delete the branch and potentially discard uncommitted changes—this is a behavioral shift from the prior “cleanup” semantics. It may be worth explicitly documenting this in the script/help/docs so users don’t lose work unexpectedly.
🤖 Was this useful? React with 👍 or 👎
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements, particularly enhancing the worktree-helper.sh script to detect squash-merged branches, which is a significant enhancement for workflows using that merge strategy. The accompanying documentation updates in pr.md, worktree.md, and postflight.md are clear and helpful.
My review includes a few suggestions for the worktree-helper.sh script to improve maintainability by refactoring duplicated code and simplifying a command. I also noted a small correction for the documentation in postflight.md to ensure the example command for auto-cleaning worktrees is accurate. Overall, these are solid improvements.
| local is_merged=false | ||
| local merge_type="" | ||
|
|
||
| # Check 1: Traditional merge detection | ||
| if git branch --merged "$default_branch" 2>/dev/null | grep -q "^\s*$worktree_branch$"; then | ||
| is_merged=true | ||
| merge_type="merged" | ||
| # Check 2: Remote branch deleted (indicates squash merge or PR closed) | ||
| elif ! git show-ref --verify --quiet "refs/remotes/origin/$worktree_branch" 2>/dev/null; then | ||
| is_merged=true | ||
| merge_type="remote deleted" | ||
| fi |
There was a problem hiding this comment.
This logic to determine if a worktree is ready for cleaning is duplicated in the auto-clean section around line 423. To improve maintainability and avoid the two sections diverging in the future, consider extracting this logic into a shared helper function.
For example, you could create a function that checks for both merge conditions and returns a status.
# Determines if a branch is cleanable and returns the reason.
# @param $1 branch_name
# @param $2 default_branch
# @return 0 on success (cleanable), 1 on failure.
# @output The merge type ('merged' or 'remote deleted') if cleanable.
_get_cleanable_merge_type() {
local branch_name="$1"
local default_branch="$2"
if git branch --merged "$default_branch" 2>/dev/null | grep -q "^\s*${branch_name}$"; then
echo "merged"
return 0
elif ! git show-ref --verify --quiet "refs/remotes/origin/${branch_name}" 2>/dev/null; then
echo "remote deleted"
return 0
fi
return 1
}You could then call it in both places like this:
if merge_type=$(_get_cleanable_merge_type "$worktree_branch" "$default_branch"); then
# ... logic for listing or removing ...
fiThis would make the code DRY (Don't Repeat Yourself).
| if [[ "$should_remove" == "true" ]]; then | ||
| echo -e "${BLUE}Removing $worktree_branch...${NC}" | ||
| git worktree remove "$worktree_path" 2>/dev/null || true | ||
| git worktree remove "$worktree_path" 2>/dev/null || git worktree remove --force "$worktree_path" 2>/dev/null || true |
There was a problem hiding this comment.
The command to remove the worktree can be simplified. git worktree remove --force will remove the worktree regardless of whether it has uncommitted changes or not, making the current chain of commands redundant. You can simplify this line to just use --force.
| git worktree remove "$worktree_path" 2>/dev/null || git worktree remove --force "$worktree_path" 2>/dev/null || true | |
| git worktree remove --force "$worktree_path" 2>/dev/null || true |
| ~/.aidevops/agents/scripts/worktree-helper.sh list | ||
|
|
||
| # Auto-clean merged worktrees (detects squash merges too) | ||
| ~/.aidevops/agents/scripts/worktree-helper.sh clean |
There was a problem hiding this comment.
The comment # Auto-clean merged worktrees suggests a non-interactive operation, but worktree-helper.sh clean runs interactively by default. To match the comment's intent and provide a true auto-clean command, you should add the --auto-yes flag.
| ~/.aidevops/agents/scripts/worktree-helper.sh clean | |
| ~/.aidevops/agents/scripts/worktree-helper.sh clean --auto-yes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.agent/workflows/pr.md:
- Around line 515-519: The markdown fenced code block after the list item "4.
**Proceed with merge**" is missing a blank line before the opening ``` and thus
violates MD031; add a blank line between the paragraph and the code fence and
ensure the fence uses the language tag (bash) and has a matching closing ``` so
the snippet `gh pr merge 123 --squash --delete-branch` is properly rendered as a
code block.
🧹 Nitpick comments (2)
.agent/scripts/worktree-helper.sh (1)
369-370: Consider adding feedback for fetch operation.The
git fetch --prunesilently continues on failure, which could lead to stale remote state being used for merge detection. While the|| trueprevents script failure (good for robustness), users have no visibility into whether the fetch succeeded.🔍 Optional improvement for visibility
- # Fetch to get current remote branch state (detects deleted branches) - git fetch --prune origin 2>/dev/null || true + # Fetch to get current remote branch state (detects deleted branches) + echo "Fetching remote branch state..." + if git fetch --prune origin 2>/dev/null; then + echo "Remote state updated" + else + echo -e "${YELLOW}Warning: Could not fetch remote state (using local cache)${NC}" + fiBased on coding guidelines emphasizing clear logging and feedback.
.agent/workflows/worktree.md (1)
325-342: Clear documentation of squash merge detection.The new subsection accurately documents the enhanced merge detection behavior and provides practical manual cleanup commands for edge cases. The two-method approach (traditional + remote deletion) is clearly explained.
The manual cleanup examples appropriately use
--forceand-Dflags, though you might consider adding a brief note that-Dis destructive and should only be used for truly abandoned branches.📋 Optional safety note
Consider adding a brief warning before the manual commands:
**Manual cleanup** if needed (use with caution): ```bash # Force remove worktree (only if normal removal fails) git worktree remove --force ~/Git/myrepo-feature-old # Delete local branch (WARNING: -D force-deletes without checking for unmerged work) git branch -D feature/old</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 61ed0572da9059d2484555455b67b130c465fe51 and 39ff3833b35301c38c1c81a45128e21a6ad19642. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `.agent/AGENTS.md` * `.agent/scripts/worktree-helper.sh` * `.agent/workflows/postflight.md` * `.agent/workflows/pr.md` * `.agent/workflows/worktree.md` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>.agent/scripts/*.sh</summary> **⚙️ CodeRabbit configuration file** > `.agent/scripts/*.sh`: Automation scripts - focus on: > - Reliability and robustness > - Clear logging and feedback > - Proper exit codes > - Error recovery mechanisms > Files: - `.agent/scripts/worktree-helper.sh` </details> </details><details> <summary>🧠 Learnings (9)</summary> <details> <summary>📚 Learning: 2025-12-22T01:24:53.937Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: templates/home/AGENTS.md:0-0
Timestamp: 2025-12-22T01:24:53.937Z
Learning: Follow all security protocols and working directory specifications defined in ~/Git/aidevops/AGENTS.md**Applied to files:** - `.agent/workflows/postflight.md` - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-11-29T04:34:42.033Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-29T04:34:42.033Z
Learning: Reference AGENTS.md (authoritative) instead of AGENT.md for AI assistant guidance**Applied to files:** - `.agent/workflows/pr.md` - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-11-29T04:34:42.033Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: AGENT.md:0-0
Timestamp: 2025-11-29T04:34:42.033Z
Learning: Maintain all AI assistant instructions, documentation, and operational guidance in AGENTS.md as the single source of truth**Applied to files:** - `.agent/workflows/pr.md` - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-11-29T04:34:27.158Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:34:27.158Z
Learning: All instructions, documentation, and operational guidance should be maintained in AGENTS.md as the single source of truth**Applied to files:** - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-11-29T04:34:30.742Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-29T04:34:30.742Z
Learning: Maintain all instructions, documentation, and operational guidance in AGENTS.md as the single source of truth**Applied to files:** - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2026-01-06T15:57:56.008Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T15:57:56.008Z
Learning: Applies to **/AGENTS.md : Limit root AGENTS.md to ~50-100 max instructions with universal applicability to >80% of tasks**Applied to files:** - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2026-01-06T15:57:56.008Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T15:57:56.008Z
Learning: Applies to **/AGENTS.md : Use progressive disclosure in AGENTS.md with pointers to subagents rather than inline content**Applied to files:** - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-11-29T04:34:30.742Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-29T04:34:30.742Z
Learning: Reference AGENTS.md for authoritative AI assistant guidance instead of GEMINI.md**Applied to files:** - `.agent/AGENTS.md` </details> <details> <summary>📚 Learning: 2025-12-22T01:24:53.937Z</summary>Learnt from: CR
Repo: marcusquinn/aidevops PR: 0
File: templates/home/AGENTS.md:0-0
Timestamp: 2025-12-22T01:24:53.937Z
Learning: Reference the authoritative repository at ~/Git/aidevops/ for all detailed AI assistant instructions and configurations**Applied to files:** - `.agent/AGENTS.md` </details> </details><details> <summary>🪛 GitHub Check: Codacy Static Code Analysis</summary> <details> <summary>.agent/workflows/pr.md</summary> [notice] 516-516: .agent/workflows/pr.md#L516 Fenced code blocks should be surrounded by blank lines </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>.agent/workflows/pr.md</summary> 516-516: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Codacy Static Code Analysis </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>.agent/AGENTS.md (1)</summary><blockquote> `34-39`: **Clear documentation improvement for branch workflow.** The explicit guidance on when direct-to-main commits are acceptable (docs-only) vs. not acceptable (code changes) helps AI assistants and developers make better decisions. This reduces the risk of accidental code commits bypassing PR review. </blockquote></details> <details> <summary>.agent/scripts/worktree-helper.sh (2)</summary><blockquote> `380-391`: **Well-structured squash merge detection logic.** The two-condition approach correctly addresses the PR objective of detecting squash merges via remote branch deletion. The logic is clear and defensive: 1. Traditional merge detection first (authoritative) 2. Remote deletion fallback (squash merge indicator) 3. Distinct labeling for user clarity The remote deletion check could theoretically flag force-deleted or never-merged branches, but the subsequent user confirmation prompt (`[y/N]`) provides a safety gate. --- `395-395`: **Excellent feedback improvement.** Displaying the specific merge detection reason (`"merged"` vs `"remote deleted"`) helps users understand why a worktree is being flagged for cleanup. This transparency aids troubleshooting and builds trust in the automation. </blockquote></details> <details> <summary>.agent/workflows/postflight.md (1)</summary><blockquote> `584-604`: **Helpful postflight workflow addition.** The new "Worktree Cleanup" section logically extends the postflight checklist with practical cleanup guidance. The placement after success criteria is appropriate, and the cross-reference to `worktree.md` helps users find detailed documentation. The documentation accurately reflects the enhanced detection behavior implemented in `worktree-helper.sh`. </blockquote></details> <details> <summary>.agent/workflows/pr.md (1)</summary><blockquote> `488-528`: **Pragmatic guidance for handling AI reviewer limitations.** This section provides a balanced approach to contradictory AI feedback: - Emphasizes verification and documentation (steps 1-3) - Requires checking authoritative sources - Mandates explaining decisions in PR comments - Only suggests proceeding with merge after demonstrating correctness The guidance correctly acknowledges that AI reviewers can be wrong while maintaining accountability through documentation requirements. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| local should_remove=false | ||
|
|
||
| # Check 1: Traditional merge | ||
| if git branch --merged "$default_branch" 2>/dev/null | grep -q "^\s*$worktree_branch$"; then | ||
| should_remove=true | ||
| # Check 2: Remote branch deleted | ||
| elif ! git show-ref --verify --quiet "refs/remotes/origin/$worktree_branch" 2>/dev/null; then | ||
| should_remove=true | ||
| fi | ||
|
|
||
| if [[ "$should_remove" == "true" ]]; then | ||
| echo -e "${BLUE}Removing $worktree_branch...${NC}" | ||
| git worktree remove "$worktree_path" 2>/dev/null || true | ||
| git worktree remove "$worktree_path" 2>/dev/null || git worktree remove --force "$worktree_path" 2>/dev/null || true | ||
| # Also delete the local branch | ||
| git branch -D "$worktree_branch" 2>/dev/null || true |
There was a problem hiding this comment.
Improve error handling and user feedback in removal logic.
The removal logic has several reliability concerns:
- Silent multi-level fallback (line 435): The triple-chained command with full error suppression provides no feedback about which removal method succeeded or if all failed
- Unconditional force delete (line 437):
git branch -Ddestroys the local branch without checking for unmerged commits, potentially losing work - Inconsistent state risk: If worktree removal fails but branch deletion succeeds, the worktree directory remains but the branch reference is lost
🛡️ Proposed fix with logging and safety checks
if [[ "$should_remove" == "true" ]]; then
echo -e "${BLUE}Removing $worktree_branch...${NC}"
- git worktree remove "$worktree_path" 2>/dev/null || git worktree remove --force "$worktree_path" 2>/dev/null || true
- # Also delete the local branch
- git branch -D "$worktree_branch" 2>/dev/null || true
+
+ # Try normal removal first
+ if git worktree remove "$worktree_path" 2>/dev/null; then
+ echo " Worktree removed"
+ elif git worktree remove --force "$worktree_path" 2>/dev/null; then
+ echo -e " ${YELLOW}Worktree force-removed${NC}"
+ else
+ echo -e " ${RED}Failed to remove worktree${NC}"
+ # Skip branch deletion if worktree removal failed
+ worktree_path=""
+ worktree_branch=""
+ continue
+ fi
+
+ # Delete local branch only after successful worktree removal
+ if git branch -d "$worktree_branch" 2>/dev/null; then
+ echo " Local branch deleted"
+ elif git branch -D "$worktree_branch" 2>/dev/null; then
+ echo -e " ${YELLOW}Local branch force-deleted${NC}"
+ else
+ echo -e " ${YELLOW}Could not delete local branch${NC}"
+ fi
fiKey improvements:
- Clear feedback at each removal step
- Try soft delete (
-d) before force delete (-D) for branches - Skip branch deletion if worktree removal fails (maintains consistency)
- User can see if force removal was necessary
Based on coding guidelines emphasizing "Clear logging and feedback" and "Error recovery mechanisms".
| 4. **Proceed with merge**: If feedback is demonstrably incorrect, merge despite CHANGES_REQUESTED | ||
| ```bash | ||
| gh pr merge 123 --squash --delete-branch | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Fix markdown formatting for code block.
Missing blank line before the fenced code block on line 516 violates markdown formatting standards (MD031).
📝 Proposed fix
4. **Proceed with merge**: If feedback is demonstrably incorrect, merge despite CHANGES_REQUESTED
+
```bash
gh pr merge 123 --squash --delete-branch
```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 4. **Proceed with merge**: If feedback is demonstrably incorrect, merge despite CHANGES_REQUESTED | |
| ```bash | |
| gh pr merge 123 --squash --delete-branch | |
| ``` | |
| 4. **Proceed with merge**: If feedback is demonstrably incorrect, merge despite CHANGES_REQUESTED |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 516-516: .agent/workflows/pr.md#L516
Fenced code blocks should be surrounded by blank lines
🪛 markdownlint-cli2 (0.18.1)
516-516: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In @.agent/workflows/pr.md around lines 515 - 519, The markdown fenced code
block after the list item "4. **Proceed with merge**" is missing a blank line
before the opening ``` and thus violates MD031; add a blank line between the
paragraph and the code fence and ensure the fence uses the language tag (bash)
and has a matching closing ``` so the snippet `gh pr merge 123 --squash
--delete-branch` is properly rendered as a code block.
1 participant
Summary
Improvements identified from @agent-review session analysis:
Changes
worktree-helper.shcmd_clean()to detect squash merges viagit show-refpr.mdworktree.mdpostflight.mdAGENTS.mdProblem Solved
The
worktree-helper.sh cleancommand previously only detected traditional merges viagit branch --merged. Squash merges (common in GitHub PRs) weren't detected because they create new commits. Now the script also checks if the remote branch was deleted, which reliably indicates the PR was merged.Summary by CodeRabbit
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.