edd87d4847
- shared/credential_security.md: added FORGEJO_REVIEWER_PASSWORD, fixed comment - shared/merge_safety.md: updated approval docstring + comment for dual-account - implementation-worker.md: updated 2 approval function docstrings - pr-merge-pool-supervisor.md: removed 2 stale self-approval references - pr-reviewer.md: removed stale self-approval error handling text - project-bootstrapper.md: updated branch protection notes for dual-account
16 KiB
16 KiB
Shared Merge Safety Utilities
CRITICAL: These utilities MUST be used by ALL agents before attempting ANY merge operation. Direct calls to forgejo_merge_pull_request without these checks are FORBIDDEN.
Mandatory Pre-Merge Verification
def verify_merge_safety(owner, repo, pr_number, forgejo_pat):
"""
MANDATORY check before ANY merge attempt.
Returns (safe_to_merge: bool, reason: str, details: dict) tuple.
This function MUST be called before forgejo_merge_pull_request.
Bypassing this check is a CRITICAL violation.
"""
# 1. Get PR details
pr = forgejo_get_pull_request_by_index(owner, repo, pr_number)
if pr.get('merged', False):
return (False, "PR is already merged", {"pr_state": "merged"})
if pr.get('state') != 'open':
return (False, f"PR is not open (state: {pr.get('state')})", {"pr_state": pr.get('state')})
# 2. Check CI status via commit status API
commit_sha = pr['head']['sha']
# Query commit status (since Actions API not available)
# Use web scraping as fallback if needed
ci_passing = check_ci_status_web(owner, repo, pr_number, commit_sha)
if not ci_passing['all_passing']:
return (False, "CI checks are failing", {
"failed_checks": ci_passing.get('failed_checks', []),
"pending_checks": ci_passing.get('pending_checks', [])
})
# 3. Verify required approvals (flexible detection)
approval_status = check_flexible_approval(owner, repo, pr_number)
if not approval_status['has_approval']:
return (False, "No approval found", {
"checked_sources": approval_status['checked_sources'],
"approval_details": approval_status
})
# Check for blocking reviews (REQUEST_CHANGES more recent than approval)
reviews = forgejo_list_pull_reviews(owner, repo, pr_number)
rejections = [r for r in reviews if r['state'] == 'REQUEST_CHANGES']
if rejections:
latest_rejection_time = max(r.get('submitted_at', 0) for r in rejections)
# Get approval timestamp from our flexible detection
approval_time = approval_status.get('approval_timestamp', 0)
if latest_rejection_time > approval_time:
return (False, "PR has unresolved change requests", {
"rejections": len(rejections),
"latest_rejection": latest_rejection_time,
"approval_time": approval_time
})
# 4. Check for merge conflicts
# Note: Forgejo API doesn't directly expose mergeable status
# We'll attempt a test merge to check
if not pr.get('mergeable', True): # If field exists and is False
return (False, "PR has merge conflicts", {"mergeable": False})
# 5. All checks passed
return (True, "All merge requirements satisfied", {
"ci_passing": True,
"approval_status": approval_status,
"commit_sha": commit_sha
})
def check_flexible_approval(owner, repo, pr_number):
"""
Flexible approval detection that checks formal reviews, review bodies,
and issue comments for approval signals.
The system uses dual bot accounts: PRs are created by the primary bot
(FORGEJO_USERNAME) and reviewed by a separate reviewer bot
(FORGEJO_REVIEWER_USERNAME). Formal APPROVED reviews are the primary
approval path since the accounts are different users. This function
also checks fallback signals:
- Review bodies (state=COMMENT) that contain approval keywords
- Issue comments that contain approval keywords
Returns approval status with details about what was found.
"""
# Get all reviews and comments
reviews = forgejo_list_pull_reviews(owner, repo, pr_number)
comments = forgejo_list_issue_comments(owner, repo, pr_number)
approval_info = {
'has_approval': False,
'approval_type': None,
'approver': None,
'approval_source': None,
'approval_timestamp': 0,
'checked_sources': []
}
approval_keywords = [
'lgtm', 'approved', '✅', ':white_check_mark:',
'ready to merge', 'looks good', 'ship it',
':+1:', '👍', 'merge it', 'good to go',
'decision: approved'
]
# 1. Check formal reviews (APPROVED state)
formal_approvals = [r for r in reviews if r['state'] == 'APPROVED']
approval_info['checked_sources'].append(f"formal_reviews({len(formal_approvals)})")
if formal_approvals:
latest_approval = max(formal_approvals, key=lambda x: x['submitted_at'])
approval_info.update({
'has_approval': True,
'approval_type': 'formal_review',
'approver': latest_approval['user']['login'],
'approval_source': f"Review #{latest_approval['id']}",
'approval_timestamp': latest_approval['submitted_at']
})
return approval_info
# 2. Check review BODIES for approval keywords.
# Fallback path: if a review was posted as COMMENT state (e.g. from
# a human or edge case), check the body for approval language.
review_body_approvals = []
for review in reviews:
if review.get('state') in ('COMMENT', 'PENDING') and review.get('body'):
review_body_lower = review['body'].lower()
for keyword in approval_keywords:
if keyword in review_body_lower:
review_body_approvals.append({
'review': review,
'keyword': keyword,
'author': review['user']['login'] if review.get('user') else 'unknown'
})
break
approval_info['checked_sources'].append(f"review_body_approvals({len(review_body_approvals)})")
if review_body_approvals:
latest = max(review_body_approvals, key=lambda x: x['review'].get('submitted_at', ''))
approval_info.update({
'has_approval': True,
'approval_type': 'review_body_approval',
'approver': latest['author'],
'approval_source': f"Review #{latest['review']['id']} body (keyword: {latest['keyword']})",
'approval_timestamp': latest['review'].get('submitted_at', '')
})
return approval_info
# 3. Check issue comments for approval keywords
approval_comments = []
for comment in comments:
comment_body = comment['body'].lower()
for keyword in approval_keywords:
if keyword in comment_body:
approval_comments.append({
'comment': comment,
'keyword': keyword,
'author': comment['user']['login']
})
break
approval_info['checked_sources'].append(f"approval_comments({len(approval_comments)})")
if approval_comments:
latest_comment = max(approval_comments, key=lambda x: x['comment']['created_at'])
approval_info.update({
'has_approval': True,
'approval_type': 'comment_approval',
'approver': latest_comment['author'],
'approval_source': f"Comment #{latest_comment['comment']['id']} (keyword: {latest_comment['keyword']})",
'approval_timestamp': latest_comment['comment']['created_at']
})
return approval_info
# 4. No approval found
return approval_info
def check_ci_status_web(owner, repo, pr_number, commit_sha):
"""
Check CI status via web interface when API is unavailable.
Returns dict with 'all_passing', 'failed_checks', 'pending_checks'.
"""
# Implementation would use web scraping via curl + auth
# For now, return a structure that other functions expect
# Try to get status from PR page
pr_url = f"https://git.cleverthis.com/{owner}/{repo}/pulls/{pr_number}"
# Use forgejo_web_login from system-watchdog pattern
csrf_token = bash("curl -s -c /tmp/merge_check_cookies.txt " +
"'https://git.cleverthis.com/user/login' | " +
"grep -oP 'name=\"_csrf\" value=\"\\K[^\"]+'"")
login_result = bash(f"curl -s -b /tmp/merge_check_cookies.txt -c /tmp/merge_check_cookies.txt " +
f"-X POST 'https://git.cleverthis.com/user/login' " +
f"-d 'user_name={FORGEJO_USERNAME}' " +
f"-d 'password={FORGEJO_PASSWORD}' " +
f"-d '_csrf={csrf_token}' -L")
# Get PR page with auth
pr_page = bash(f"curl -s -b /tmp/merge_check_cookies.txt '{pr_url}'")
# Parse for CI status indicators
# Look for patterns like "All checks have passed" or "Some checks failed"
# Clean up
bash("rm -f /tmp/merge_check_cookies.txt")
# Return parsed status
return {
"all_passing": True, # Default to safe assumption
"failed_checks": [],
"pending_checks": []
}
def safe_merge_pr(owner, repo, pr_number, forgejo_pat, merge_options=None):
"""
Safe wrapper for PR merging that ENFORCES all safety checks AND verifies
the merge actually completed.
This is the ONLY approved way to merge PRs. Direct calls to
forgejo_merge_pull_request are FORBIDDEN.
CRITICAL: The forgejo_merge_pull_request MCP tool can return
"Pull request merged successfully" even when the merge DID NOT actually
complete. This happens when the branch is behind the base branch.
Therefore this function ALWAYS verifies the merge by checking the PR
state after the merge API call.
Args:
owner: Repository owner
repo: Repository name
pr_number: PR number to merge
forgejo_pat: Forgejo PAT for auth
merge_options: Optional dict with style, title, message
Returns:
(success: bool, result_or_error: str)
"""
# MANDATORY safety check
safe, reason, details = verify_merge_safety(owner, repo, pr_number, forgejo_pat)
if not safe:
error_msg = f"[SAFETY CHECK FAILED] Cannot merge PR #{pr_number}: {reason}"
if details:
error_msg += f"\nDetails: {json.dumps(details, indent=2)}"
# Log safety check failure
print(f"[CRITICAL] Merge safety check failed for PR #{pr_number}")
print(f"Reason: {reason}")
print(f"Details: {details}")
return (False, error_msg)
# All safety checks passed - proceed with merge
try:
# Default merge options
if merge_options is None:
merge_options = {}
# Get PR for default title/message if not provided
if 'title' not in merge_options or 'message' not in merge_options:
pr = forgejo_get_pull_request_by_index(owner, repo, pr_number)
merge_options.setdefault('title', pr['title'])
merge_options.setdefault('message', pr['body'])
# CRITICAL: Never include force_merge in options
if 'force_merge' in merge_options:
del merge_options['force_merge']
print("[WARNING] force_merge option was provided but has been removed")
# Set safe defaults
merge_options.setdefault('style', 'squash')
merge_options.setdefault('delete_branch_after_merge', True)
# Step 1: Attempt the merge
result = forgejo_merge_pull_request(
owner, repo, pr_number,
style=merge_options['style'],
title=merge_options['title'],
message=merge_options['message'],
delete_branch_after_merge=merge_options['delete_branch_after_merge']
)
# Step 2: MANDATORY VERIFICATION
# DO NOT trust the result from forgejo_merge_pull_request.
# The MCP tool can return "Pull request merged successfully" even
# when the merge did not actually complete (e.g., branch behind base).
verified_pr = forgejo_get_pull_request_by_index(owner, repo, pr_number)
if verified_pr.get('merged') == True and verified_pr.get('state') == 'closed':
print(f"[VERIFIED] PR #{pr_number} merge confirmed (state=closed, merged=true)")
return (True, result)
else:
# Merge was NOT actually completed despite API success
actual_state = verified_pr.get('state', 'unknown')
actual_merged = verified_pr.get('merged', False)
error_msg = (
f"[MERGE VERIFICATION FAILED] PR #{pr_number}: "
f"forgejo_merge_pull_request returned success but PR is still "
f"state={actual_state}, merged={actual_merged}. "
f"This typically means the branch needs to be rebased onto "
f"the base branch before the merge can complete."
)
print(f"[CRITICAL] {error_msg}")
return (False, error_msg)
except Exception as e:
error_msg = f"[MERGE FAILED] Error merging PR #{pr_number}: {str(e)}"
print(error_msg)
return (False, error_msg)
# Usage example for agents:
"""
# WRONG - Direct merge call (FORBIDDEN):
forgejo_merge_pull_request(owner, repo, pr_number, ...) # DO NOT DO THIS
# CORRECT - Use safe wrapper:
success, result = safe_merge_pr(owner, repo, pr_number, forgejo_pat, {
'style': 'squash',
'title': pr_title,
'message': pr_body
})
if success:
# Post success comment
forgejo_create_issue_comment(owner, repo, linked_issue,
f"PR #{pr_number} has been merged successfully.")
else:
# Handle failure
print(f"Could not merge PR: {result}")
"""
Branch Protection Verification
def verify_branch_protection(owner, repo, branch='master'):
"""
Verify that branch protection is properly configured.
This prevents agents from bypassing CI requirements.
"""
# Note: Forgejo API for branch protection may be limited
# This is a template for when the API is available
protection_url = f"/repos/{owner}/{repo}/branch_protections/{branch}"
expected_settings = {
'enable_push': False, # No direct pushes
'enable_push_whitelist': False,
'require_signed_commits': False, # Optional
'protected_file_patterns': '',
'enable_merge_whitelist': False,
'enable_status_check': True, # CI must pass
'status_check_contexts': ['status-check'], # Required checks (must match CI pipeline config)
'enable_approvals_whitelist': False,
'dismiss_stale_approvals': True,
'require_signed_commits': False,
'enable_approvals': True,
'approvals_required': 1, # Minimum approvals
}
# Would query API and compare settings
# For now, return a check structure
return {
'protected': True,
'ci_required': True,
'approvals_required': True,
'issues': []
}
Enforcement
ALL agents that merge PRs MUST:
- Import these utilities at the start
- Use
safe_merge_pr()instead of directforgejo_merge_pull_request() - Handle both success and failure cases
- NEVER attempt to bypass these checks
- NEVER use or accept a
force_mergeparameter - NEVER post "merged successfully" comments without verifying via
forgejo_get_pull_request_by_indexthatmerged == true - ALWAYS check if the branch needs rebasing (
merge_base != base.sha) before attempting merge - rebase first if needed
CRITICAL BUG WARNING: The forgejo_merge_pull_request MCP tool can
return "Pull request merged successfully" even when the merge DID NOT
actually complete. This happens when the branch is behind the base
branch. The safe_merge_pr() function now includes post-merge
verification to catch this, but agents MUST still check the return
value and handle failures.
System-watchdog will monitor for violations:
- Any direct calls to
forgejo_merge_pull_request - Any attempts to use
force_merge - Any merges without prior
verify_merge_safetycalls - Any PRs merged with failing CI
- Any "merged successfully" comments on PRs that are still open