feat(deployment): Add auto-push to origin after worktree merge #473
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds deployment configuration and a deployment handler: new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as DeploymentHandler
participant Lock as FileLock
participant Worktree as WorktreeMonitor
participant Git as Git (git)
participant GH as GitHub CLI (gh)
participant Notify as Notifier
Client->>Handler: deploy(task_id, task_title, branch)
rect rgb(248,249,251)
Handler->>Lock: acquire()
Lock-->>Handler: locked
alt wait_for_all_worktrees
Handler->>Worktree: _all_worktrees_complete()
Worktree-->>Handler: complete / timeout
end
end
rect rgb(240,255,240)
alt mode = LOCAL_ONLY
Handler-->>Client: DeploymentResult(success=true, message="local only")
else mode = AUTO_PUSH
loop retries
Handler->>Git: _git_push_with_auth(...)
Git-->>Handler: ok / error
end
Handler->>Git: _get_head_sha()
Git-->>Handler: sha
Handler-->>Client: DeploymentResult(success, commit_sha)
else mode = AUTO_PR
Handler->>Git: push branch
Git-->>Handler: pushed
Handler->>GH: _create_pull_request(...)
GH-->>Handler: pr_url / error
Handler-->>Client: DeploymentResult(success, pr_url)
end
end
rect rgb(255,250,240)
alt notify_on_push or notify_on_failure
Handler->>Notify: _send_notification(message, success)
Notify-->>Handler: notified
end
end
Handler->>Lock: release()
Lock-->>Handler: released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 @abe238, 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 introduces a new, configurable automated deployment system that activates upon task completion and merging to the main branch. It provides flexible deployment options, including local-only merges, direct pushes to the remote origin, or automatic pull request creation, enhancing the workflow for task completion. The system is designed with security in mind, using Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 a valuable configurable deployment feature for handling completed tasks, with modes for local-only merges, auto-pushing, and auto-PR creation. The implementation shows strong consideration for security, using the GIT_ASKPASS pattern to avoid exposing tokens, and for preventing race conditions with file-based locking. However, there are several areas that need improvement. The current implementation has significant portability issues, using fcntl and osascript, which will limit its use to Unix-like systems and macOS. There is also notable code duplication in the git push logic that should be refactored. Additionally, error handling and configuration loading could be made more robust to prevent silent failures and inconsistencies. My review includes specific suggestions to address these points.
| """ | ||
|
|
||
| import asyncio | ||
| import fcntl |
There was a problem hiding this comment.
The use of fcntl for file locking makes this feature specific to Unix-like systems and will cause the application to crash on Windows. For cross-platform compatibility, a library like filelock is recommended as it provides a platform-independent API for file locking.
| async def _execute_push(self) -> subprocess.CompletedProcess: | ||
| """Execute git push with secure token handling via GIT_ASKPASS.""" | ||
| token = await self._get_github_token() | ||
| env = os.environ.copy() | ||
| askpass_path = None | ||
|
|
||
| if token: | ||
| with tempfile.NamedTemporaryFile( | ||
| mode="w", suffix=".sh", delete=False | ||
| ) as f: | ||
| f.write(f'#!/bin/bash\necho "{token}"') | ||
| askpass_path = f.name | ||
|
|
||
| os.chmod(askpass_path, 0o700) | ||
| env["GIT_ASKPASS"] = askpass_path | ||
|
|
||
| try: | ||
| branch = self.config.target_branch | ||
| return await asyncio.wait_for( | ||
| asyncio.get_event_loop().run_in_executor( | ||
| None, | ||
| lambda: subprocess.run( | ||
| ["git", "push", "origin", f"{branch}:{branch}"], | ||
| cwd=self.project_root, | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| ), | ||
| timeout=self.config.push_timeout | ||
| ) | ||
| finally: | ||
| if askpass_path and os.path.exists(askpass_path): | ||
| os.unlink(askpass_path) |
There was a problem hiding this comment.
The methods _execute_push and _execute_branch_push are almost identical. The only significant difference is the git push command arguments. This duplication makes the code harder to maintain and more prone to bugs if one is updated and the other is not. You should refactor this into a single private method that takes the git arguments as a parameter.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The try...except Exception: pass block will silently ignore any errors when reading a worktree's status file, including JSON decoding errors or missing keys. This is dangerous as it could lead to a premature push if a status file is corrupted. The exception should be caught more specifically, logged, and the function should return False to be safe.
| except Exception: | |
| pass | |
| except (json.JSONDecodeError, KeyError) as e: | |
| logger.warning(f"Could not parse status file {status_file}: {e}. Assuming not complete.") | |
| return False | |
| except Exception as e: | |
| logger.error(f"Unexpected error reading status file {status_file}: {e}. Assuming not complete.") | |
| return False |
| async def _send_notification(self, message: str, success: bool) -> None: | ||
| """Send desktop notification.""" | ||
| try: | ||
| icon = "checkmark.circle" if success else "xmark.circle" | ||
| await asyncio.get_event_loop().run_in_executor( | ||
| None, | ||
| lambda: subprocess.run( | ||
| [ | ||
| "osascript", "-e", | ||
| f'display notification "{message}" with title "Auto-Claude"' | ||
| ], | ||
| capture_output=True, | ||
| ) | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The _send_notification method uses osascript, which is only available on macOS. This will fail on other operating systems. To make notifications work on other platforms, you should either use a cross-platform notification library or implement platform-specific logic based on sys.platform. Additionally, the icon variable is defined but not used in the osascript command.
async def _send_notification(self, message: str, success: bool) -> None:
"""Send desktop notification."""
import sys
if sys.platform != "darwin":
logger.info(f"Skipping notification on non-macOS platform: {message}")
return
try:
await asyncio.get_event_loop().run_in_executor(
None,
lambda: subprocess.run(
[
"osascript", "-e",
f'display notification "{message}" with title "Auto-Claude"'
],
capture_output=True,
check=False, # Avoid raising exception on non-zero exit
)
)
except Exception as e:
logger.warning(f"Failed to send notification: {e}")
apps/backend/config/deployment.py
Outdated
| push_retries=int(os.getenv("DEPLOYMENT_PUSH_RETRIES", "3")), | ||
| push_retry_delay=int(os.getenv("DEPLOYMENT_PUSH_RETRY_DELAY", "5")), | ||
| push_timeout=int(os.getenv("DEPLOYMENT_PUSH_TIMEOUT", "300")), |
There was a problem hiding this comment.
The int() conversions for DEPLOYMENT_PUSH_RETRIES, DEPLOYMENT_PUSH_RETRY_DELAY, and DEPLOYMENT_PUSH_TIMEOUT will raise a ValueError if the environment variables are set to non-integer values, causing the application to crash on startup. It's safer to handle this gracefully by falling back to the default values, similar to how an invalid DEPLOYMENT_MODE is handled.
apps/backend/config/deployment.py
Outdated
| deployment = project_config.get("deployment", {}) | ||
| if "mode" in deployment: | ||
| config.mode = DeploymentMode(deployment["mode"]) | ||
| if "target_branch" in deployment: | ||
| config.target_branch = deployment["target_branch"] | ||
| if "wait_for_all_worktrees" in deployment: | ||
| config.wait_for_all_worktrees = deployment["wait_for_all_worktrees"] | ||
| except (json.JSONDecodeError, ValueError): | ||
| pass |
There was a problem hiding this comment.
The from_project method only allows overriding a subset of configuration options (mode, target_branch, wait_for_all_worktrees) from the config.json file. Other important settings like retry logic and notification preferences are not overridable at the project level. This is inconsistent and limits the flexibility of project-specific configurations. All settings defined in DeploymentConfig should be overridable for consistency.
deployment = project_config.get("deployment", {})
if "mode" in deployment:
config.mode = DeploymentMode(deployment["mode"])
if "target_branch" in deployment:
config.target_branch = deployment["target_branch"]
if "wait_for_all_worktrees" in deployment:
config.wait_for_all_worktrees = bool(deployment["wait_for_all_worktrees"])
if "push_retries" in deployment:
config.push_retries = int(deployment["push_retries"])
if "push_retry_delay" in deployment:
config.push_retry_delay = int(deployment["push_retry_delay"])
if "push_timeout" in deployment:
config.push_timeout = int(deployment["push_timeout"])
if "notify_on_push" in deployment:
config.notify_on_push = bool(deployment["notify_on_push"])
if "notify_on_failure" in deployment:
config.notify_on_failure = bool(deployment["notify_on_failure"])
except (json.JSONDecodeError, ValueError, TypeError):
pass| try: | ||
| fcntl.flock(self.fd, fcntl.LOCK_UN) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The __aexit__ method in AsyncFileLock uses a broad except Exception: pass. This can hide important errors during the lock release process. It's better to at least log the exception for debugging purposes, even if you can't do much to recover from it.
| try: | |
| fcntl.flock(self.fd, fcntl.LOCK_UN) | |
| except Exception: | |
| pass | |
| try: | |
| fcntl.flock(self.fd, fcntl.LOCK_UN) | |
| except Exception as e: | |
| logger.warning(f"Failed to release deployment lock: {e}") |
| def _validate_remote(self, url: str) -> bool: | ||
| """Validate remote URL is a known git host.""" | ||
| known_hosts = ["github.com", "gitlab.com", "bitbucket.org"] | ||
| return any(host in url for host in known_hosts) |
There was a problem hiding this comment.
The _validate_remote method uses any(host in url for host in known_hosts), which can be easily bypassed with a crafted URL (e.g., https://evil.com/github.com). A more secure approach is to parse the URL and check the hostname directly.
| def _validate_remote(self, url: str) -> bool: | |
| """Validate remote URL is a known git host.""" | |
| known_hosts = ["github.com", "gitlab.com", "bitbucket.org"] | |
| return any(host in url for host in known_hosts) | |
| def _validate_remote(self, url: str) -> bool: | |
| """Validate remote URL is a known git host.""" | |
| from urllib.parse import urlparse | |
| known_hosts = ["github.com", "gitlab.com", "bitbucket.org"] | |
| try: | |
| # Handle SSH URLs like [email protected]:user/repo.git | |
| if "@" in url and ":" in url and not url.startswith(("http", "https CURLEncode")): | |
| host = url.split("@")[1].split(":")[0] | |
| return host in known_hosts | |
| # Handle HTTP/HTTPS URLs | |
| parsed_url = urlparse(url) | |
| return parsed_url.hostname in known_hosts | |
| except Exception: | |
| return False |
| status_file = wt / ".auto-claude-status" | ||
| if status_file.exists(): | ||
| try: | ||
| import json |
There was a problem hiding this comment.
The import json statement is inside the _all_worktrees_complete method. According to PEP 8, imports should be at the top of the file to have a clear view of the module's dependencies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/backend/.env.exampleapps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/.env*
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/.env*: Enable Electron MCP for E2E testing by settingELECTRON_MCP_ENABLED=truein.envand starting the Electron app withnpm run dev
Configure memory system credentials inapps/backend/.envand validate withgraphiti_config.py
Files:
apps/backend/.env.example
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.276Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
🪛 GitHub Actions: Lint
apps/backend/config/deployment.py
[error] 10-10: Ruff I001: Import block is un-sorted or un-formatted.
🪛 GitHub Check: python
apps/backend/config/deployment.py
[failure] 13-13: Ruff (F401)
apps/backend/config/deployment.py:13:20: F401 typing.Optional imported but unused
[failure] 10-15: Ruff (I001)
apps/backend/config/deployment.py:10:1: I001 Import block is un-sorted or un-formatted
apps/backend/handlers/deployment_handler.py
[failure] 294-294: Ruff (UP045)
apps/backend/handlers/deployment_handler.py:294:42: UP045 Use X | None for type annotations
[failure] 30-30: Ruff (UP045)
apps/backend/handlers/deployment_handler.py:30:13: UP045 Use X | None for type annotations
[failure] 29-29: Ruff (UP045)
apps/backend/handlers/deployment_handler.py:29:17: UP045 Use X | None for type annotations
[failure] 8-18: Ruff (I001)
apps/backend/handlers/deployment_handler.py:8:1: I001 Import block is un-sorted or un-formatted
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (2)
apps/backend/.env.example (1)
49-76: LGTM!The deployment settings section is well-documented with clear descriptions for each variable, sensible defaults, and follows the existing format of the file. The commented-out examples make the configuration discoverable without changing default behavior.
apps/backend/handlers/deployment_handler.py (1)
24-31: LGTM!The
DeploymentResultdataclass is well-structured with appropriate optional fields for different deployment outcomes.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
6 issue(s) must be addressed (0 required, 6 recommended), 4 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 6 issue(s)
- Low: 4 issue(s)
Generated by Auto Claude PR Review
Findings (10 selected of 10 total)
🟡 [MEDIUM] Remote URL validation can be bypassed
📁 apps/backend/handlers/deployment_handler.py:318
The _validate_remote() method uses a simple substring check: any(host in url for host in known_hosts). This can be bypassed with malicious URLs like: (1) https://evil.com?github.com (query parameter), (2) https://github.com.evil.com (subdomain), (3) https://evil.com/github.com/ (path). An attacker could trick the system into pushing to a malicious remote.
Suggested fix:
Use proper URL parsing: `from urllib.parse import urlparse; parsed = urlparse(url); return any(parsed.netloc == host or parsed.netloc.endswith('.' + host) for host in known_hosts)`
🟡 [MEDIUM] GitHub token written to temporary file on disk
📁 apps/backend/handlers/deployment_handler.py:198
The token is written to a temporary shell script file for GIT_ASKPASS authentication. While file permissions are set to 0o700 and cleanup is in a finally block, if the process is killed (SIGKILL) or crashes, the token file may remain on disk. Additionally, the token is briefly visible in the filesystem.
Suggested fix:
Consider using GIT_CREDENTIAL_HELPER with stdin-based credential input, or ensure cleanup with atexit handler: `import atexit; atexit.register(lambda: os.unlink(askpass_path) if os.path.exists(askpass_path) else None)`
🟡 [MEDIUM] Empty except block silently swallows exceptions
📁 apps/backend/handlers/deployment_handler.py:125
Multiple empty except blocks in the file (lines 125, 284, 347, 371) silently catch and ignore exceptions with just pass. This makes debugging difficult and can hide real problems. GitHub Advanced Security also flagged this issue.
Suggested fix:
At minimum, log the exception: `except Exception as e: logger.debug(f'Non-critical error ignored: {e}')`. For the lock release at line 125, consider: `except Exception: pass # Lock release failure is non-fatal`
🟡 [MEDIUM] Empty except block in config loading
📁 apps/backend/config/deployment.py:78
The project config loading uses except (json.JSONDecodeError, ValueError): pass which silently ignores config parsing errors. Users won't know if their config file has syntax errors.
Suggested fix:
Log a warning: `except (json.JSONDecodeError, ValueError) as e: logger.warning(f'Failed to parse project config, using defaults: {e}')`
🟡 [MEDIUM] Duplicated code in _execute_push and _execute_branch_push
📁 apps/backend/handlers/deployment_handler.py:197
Methods _execute_push (lines 197-229) and _execute_branch_push (lines 267-290) have nearly identical logic for: (1) getting token, (2) creating temp askpass file, (3) setting permissions, (4) running git push, (5) cleanup. This violates DRY and makes maintenance harder.
Suggested fix:
Extract common logic into a private method: `async def _git_push_with_auth(self, *args) -> subprocess.CompletedProcess` that handles token, askpass file, and cleanup. Both methods call this helper with different git arguments.
🔵 [LOW] Unused import 'Optional'
📁 apps/backend/config/deployment.py:13
The Optional type is imported from typing but never used in the file. GitHub Advanced Security flagged this.
Suggested fix:
Remove the unused import: change `from typing import Optional` to just remove the line, or if other typing imports exist, remove Optional from them.
🔵 [LOW] Unused variable 'icon'
📁 apps/backend/handlers/deployment_handler.py:360
The variable icon is assigned based on success status but never used. It appears to be leftover from planned but unimplemented functionality.
Suggested fix:
Either remove the unused variable, or implement its intended use (perhaps for macOS notification icon).
🔵 [LOW] Potential file descriptor leak in AsyncFileLock
📁 apps/backend/handlers/deployment_handler.py:108
In AsyncFileLock.aenter, the file is opened at line 108 before the lock acquisition loop. If an exception occurs during the loop (other than BlockingIOError or TimeoutError), the file descriptor may not be properly closed.
Suggested fix:
Wrap the entire __aenter__ logic in try-except-finally or use a context manager pattern: `try: self.fd = open(...); while True: ... except Exception: if self.fd: self.fd.close(); raise`
🔵 [LOW] TOCTOU race condition in worktree status check
📁 apps/backend/handlers/deployment_handler.py:340
In _all_worktrees_complete(), there's a time-of-check-to-time-of-use race: the status file could be modified or deleted between status_file.exists() check (line 340) and open(status_file) (line 343). While the exception is caught, this could lead to inconsistent state detection.
Suggested fix:
Remove the exists() check and handle FileNotFoundError directly: `try: with open(status_file) as f: ... except FileNotFoundError: continue`
🟡 [MEDIUM] New directory structure deviates from codebase patterns
📁 apps/backend/config/deployment.py:1
This PR introduces new directories config/ and handlers/ that don't exist in the current codebase. Existing patterns show: configs in core/ (e.g., core/model_config.py), handlers/runners in runners/ or services/. This architectural deviation could cause confusion.
Suggested fix:
Consider moving files to match existing patterns: (1) `apps/backend/core/deployment_config.py` for config, (2) `apps/backend/services/deployment.py` or `apps/backend/runners/deployment.py` for handler.
This review was generated by Auto Claude.
Fixes based on CodeRabbit, Gemini, and GitHub Advanced Security reviews: Critical: - Replace fcntl with cross-platform FileLock from runners/github/file_lock.py - Fix shell injection vulnerability by escaping single quotes in token High Priority: - Extract common _git_push_with_auth() to eliminate code duplication - Fix silent exception handling in worktree status checks - Make notifications platform-aware (macOS only, graceful skip on others) - Fix remote URL validation to properly parse URLs and prevent bypass Medium Priority: - Remove unused Optional import, sort imports per Ruff - Use modern X | None syntax instead of Optional[X] for Python 3.12+ - Replace deprecated get_event_loop() with get_running_loop() - Add _safe_int() helper for robust env var parsing - Allow all 8 config settings to be overridden from project config - Add logging for config parse errors instead of silent pass - Add timeout to PR creation to prevent infinite hangs - Move json import to top of file per PEP 8 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/backend/handlers/deployment_handler.py (1)
300-319: Remote validation blocks legitimate self-hosted Git servers.The hardcoded
known_hostsset only allows github.com, gitlab.com, and bitbucket.org. Users with self-hosted GitLab, Gitea, or other Git servers will fail validation. Consider making the allowed hosts configurable via environment variable.🔎 Proposed fix
def _validate_remote(self, url: str) -> bool: """ Validate remote URL is a known git host. Handles both HTTPS URLs and SSH URLs (git@host:user/repo.git). """ known_hosts = {"github.com", "gitlab.com", "bitbucket.org"} + + # Support custom hosts via environment variable + custom_hosts = os.getenv("DEPLOYMENT_ALLOWED_HOSTS", "") + if custom_hosts: + known_hosts.update(h.strip() for h in custom_hosts.split(",")) try:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
🧰 Additional context used
📓 Path-based instructions (1)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/config/deployment.pyapps/backend/handlers/deployment_handler.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.276Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
🧬 Code graph analysis (1)
apps/backend/handlers/deployment_handler.py (2)
apps/backend/config/deployment.py (2)
DeploymentConfig(39-120)DeploymentMode(31-35)apps/backend/runners/github/file_lock.py (2)
FileLock(104-211)FileLockTimeout(98-101)
🪛 GitHub Actions: Lint
apps/backend/config/deployment.py
[error] 1-1: Ruff formatting check failed. 2 files would be reformatted by 'ruff format apps/backend/ --check --diff'.
apps/backend/handlers/deployment_handler.py
[error] 1-1: Ruff formatting check failed. 2 files would be reformatted by 'ruff format apps/backend/ --check --diff'.
⏰ 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). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (5)
apps/backend/handlers/deployment_handler.py (5)
34-50: Well-structured class with clear security documentation.The class docstring clearly documents the security features (GIT_ASKPASS for token handling, file locking, remote validation). The use of the cross-platform
FileLockfromrunners/github/file_lock.pyaddresses cross-platform compatibility concerns.
153-202: Secure token handling via GIT_ASKPASS.The implementation properly escapes single quotes in the token (line 177) to prevent shell injection, uses a temporary file with restricted permissions (0o700), and cleans up the askpass script in the
finallyblock. This is a solid security-hardened approach.
335-363: Robust error handling for worktree status checks.The exception handling properly distinguishes between expected parsing errors (
json.JSONDecodeError,KeyError) and unexpected errors, logging appropriate warning/error messages and safely returningFalseto prevent premature deployment.
365-385: Platform-aware notification implementation.The platform check at line 367 properly skips macOS-specific
osascripton non-Darwin platforms with debug logging, addressing cross-platform compatibility.
1-385: Fix Ruff formatting violations in deployment_handler.py.Run
ruff format apps/backend/to apply the necessary formatting changes, including adjusting line breaks, trailing commas, and blank line placement.
|
@coderabbitai please review this PR |
|
@abe238: I'll review this PR for you. Let me analyze the deployment automation implementation, including the configuration system, deployment handler, security considerations, and concurrency controls. 🧠 Learnings used✅ Actions performedReview triggered.
|
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔄 Follow-up Review: Needs Revision
Resolution Status
- ✅ Resolved: 7 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
Verdict
Progress Made: 8 of 10 previous findings have been fully addressed with good fixes (URL validation rewritten, empty except blocks now log, duplicated code consolidated, unused imports/variables removed, FileLock properly implemented).
Blocking Issues:
- NEW-002 [HIGH]: Command injection in _send_notification - osascript message not sanitized. Must be fixed before merge.
Remaining Concerns (non-blocking):
- Token written to temp file (0d829aa357c0) - inherent GIT_ASKPASS limitation, acceptable risk with existing cleanup
- TOCTOU race (ccc7992b5496) - low severity, mitigated by exception handling
- Custom hosts validation (NEW-003) - medium severity suggestion
- Behavior change in worktree check (NEW-005) - medium severity, could cause blocking
Required Fix: Sanitize the message parameter in _send_notification() before passing to osascript by escaping double quotes and backslashes.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🟡 [MEDIUM] [UNRESOLVED] GitHub token written to temporary file on disk
📁 apps/backend/handlers/deployment_handler.py:198
The token is written to a temporary shell script file for GIT_ASKPASS authentication. While file permissions are set to 0o700 and cleanup is in a finally block, if the process is killed (SIGKILL) or crashes, the token file may remain on disk. Additionally, the token is briefly visible in the filesystem.
Resolution note: Token still written to temp file via tempfile.NamedTemporaryFile. Cleanup in finally block, but SIGKILL could leave file on disk.
Suggested fix:
Consider using GIT_CREDENTIAL_HELPER with stdin-based credential input, or ensure cleanup with atexit handler: `import atexit; atexit.register(lambda: os.unlink(askpass_path) if os.path.exists(askpass_path) else None)`
🟡 [MEDIUM] [UNRESOLVED] New directory structure deviates from codebase patterns
📁 apps/backend/config/deployment.py:1
This PR introduces new directories config/ and handlers/ that don't exist in the current codebase. Existing patterns show: configs in core/ (e.g., core/model_config.py), handlers/runners in runners/ or services/. This architectural deviation could cause confusion.
Resolution note: New directories are being introduced. Pattern consistency with existing codebase cannot be verified from diff alone.
Suggested fix:
Consider moving files to match existing patterns: (1) `apps/backend/core/deployment_config.py` for config, (2) `apps/backend/services/deployment.py` or `apps/backend/runners/deployment.py` for handler.
🟠 [HIGH] Command injection via notification message
📁 apps/backend/handlers/deployment_handler.py:369
The _send_notification method passes the message parameter directly into osascript without sanitization. A message containing double quotes or AppleScript escape sequences could break out of the string context. While messages are mostly internal, external data (PR URLs, branch names) could be controlled by attackers.
Suggested fix:
Sanitize message by escaping double quotes and backslashes: safe_message = message.replace('\\', '\\\\').replace('"', '\\"')
🟡 [MEDIUM] Custom allowed hosts can bypass security validation
📁 apps/backend/handlers/deployment_handler.py:300
DEPLOYMENT_ALLOWED_HOSTS env var allows arbitrary hosts without format validation. Empty entries after comma splitting could add empty string to allowed set.
Suggested fix:
Add hostname format validation regex before adding custom hosts to the set.
🟡 [MEDIUM] Behavior change in _all_worktrees_complete may block deployments
📁 apps/backend/handlers/deployment_handler.py:341
Previous implementation continued on JSON parse errors. New implementation returns False, which could block deployments indefinitely if status files become corrupted.
Suggested fix:
Consider adding stale file detection (e.g., skip files older than 24 hours) or cleanup mechanism.
This review was generated by Auto Claude.
Add configurable deployment behavior when tasks complete: - local_only: Just merge, no push (default, backward compatible) - auto_push: Push to origin immediately after merge - auto_pr: Create a pull request instead of direct push Security features: - Token never appears in command line (uses GIT_ASKPASS) - File lock prevents concurrent push race conditions - Remote URL validation before push New files: - apps/backend/config/deployment.py: Configuration dataclass - apps/backend/handlers/deployment_handler.py: Push/PR logic Updated: - apps/backend/.env.example: New DEPLOYMENT_* variables Closes companion issue to AndyMik90#243 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Fixes based on CodeRabbit, Gemini, and GitHub Advanced Security reviews: Critical: - Replace fcntl with cross-platform FileLock from runners/github/file_lock.py - Fix shell injection vulnerability by escaping single quotes in token High Priority: - Extract common _git_push_with_auth() to eliminate code duplication - Fix silent exception handling in worktree status checks - Make notifications platform-aware (macOS only, graceful skip on others) - Fix remote URL validation to properly parse URLs and prevent bypass Medium Priority: - Remove unused Optional import, sort imports per Ruff - Use modern X | None syntax instead of Optional[X] for Python 3.12+ - Replace deprecated get_event_loop() with get_running_loop() - Add _safe_int() helper for robust env var parsing - Allow all 8 config settings to be overridden from project config - Add logging for config parse errors instead of silent pass - Add timeout to PR creation to prevent infinite hangs - Move json import to top of file per PEP 8 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add encoding="utf-8" to all file open() calls - Use _safe_int() in from_project() for robust integer parsing - Rename _acquire_lock() to _create_lock() for clarity - Add DEPLOYMENT_ALLOWED_HOSTS env var for custom git hosts - Run ruff format to fix line length violations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
abe238
force-pushed
the
feat/auto-push-on-completion
branch
from
6e3535c to
376b204
Compare
|
All review feedback has been addressed in commits 0a42f07 and 6e3535c (now rebased as 376b204): Issues Fixed:
CI Status: All 11 checks passing ✅ Ready for approval and merge when you're satisfied with the changes. @AndyMik90 |
- Fix HIGH: Command injection in _send_notification (sanitize message) - Fix TOCTOU race: Remove exists() check, catch FileNotFoundError - Add atexit handler for token file cleanup on abnormal termination 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
| if askpass_path and os.path.exists(askpass_path): | ||
| try: | ||
| os.unlink(askpass_path) | ||
| except OSError: |
Check notice
Code scanning / CodeQL
Empty except Note
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/backend/handlers/deployment_handler.py (1)
8-21: Fix import sorting to pass CI.The import block is unsorted, causing the Ruff linter to fail in CI. This blocks the PR from merging.
🔎 Proposed fix
-import atexit import asyncio +import atexit import json import logging import os import subprocess import sys import tempfile from dataclasses import dataclass from pathlib import Path from urllib.parse import urlparse from config.deployment import DeploymentConfig, DeploymentMode from runners.github.file_lock import FileLock, FileLockTimeout
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/handlers/deployment_handler.py
🧰 Additional context used
📓 Path-based instructions (1)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/handlers/deployment_handler.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/handlers/deployment_handler.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
🪛 GitHub Actions: Lint
apps/backend/handlers/deployment_handler.py
[error] 8-8: Ruff: Import block is un-sorted or un-formatted. (ruff check apps/backend/ --output-format=github)
🪛 GitHub Check: CodeQL
apps/backend/handlers/deployment_handler.py
[notice] 180-180: Empty except
'except' clause does nothing but pass and there is no explanatory comment.
🪛 GitHub Check: python
apps/backend/handlers/deployment_handler.py
[failure] 8-21: Ruff (I001)
apps/backend/handlers/deployment_handler.py:8:1: I001 Import block is un-sorted or un-formatted
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
apps/backend/handlers/deployment_handler.py (4)
393-395: Good sanitization for AppleScript injection prevention.The message sanitization properly escapes backslashes and double quotes to prevent AppleScript injection. This addresses previous security concerns.
157-216: Secure token handling implementation looks solid.The GIT_ASKPASS pattern with temporary file, single-quote escaping, and atexit cleanup properly addresses the security concerns from previous reviews. The token never appears on the command line.
311-339: URL validation properly implemented.The remote validation now correctly parses URLs using
urlparse, extracts the hostname, and supports custom hosts viaDEPLOYMENT_ALLOWED_HOSTS. This addresses the previous security concern about substring matching and supports self-hosted Git servers.
355-385: Excellent exception handling in worktree status checks.The error handling properly distinguishes between different failure modes:
FileNotFoundErroris expected and continues (worktree not started yet)JSONDecodeErrorandKeyErrorare logged as warnings and returnFalse(safe default)- Unexpected exceptions are logged as errors and return
FalseThis prevents premature pushes if status files are corrupted while providing good diagnostics.
| def cleanup_askpass() -> None: | ||
| """Remove askpass file if it exists.""" | ||
| if askpass_path and os.path.exists(askpass_path): | ||
| try: | ||
| os.unlink(askpass_path) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
Add explanatory comment or log warning for exception handling.
The empty except OSError: pass was flagged by CodeQL. In cleanup code, silently ignoring errors is often acceptable, but should be documented or logged at debug level for troubleshooting.
🔎 Proposed fix
def cleanup_askpass() -> None:
"""Remove askpass file if it exists."""
if askpass_path and os.path.exists(askpass_path):
try:
os.unlink(askpass_path)
- except OSError:
- pass
+ except OSError as e:
+ # Ignore cleanup failures (e.g., file already removed, permission denied)
+ logger.debug(f"Could not remove askpass file: {e}")🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 180-180: Empty except
'except' clause does nothing but pass and there is no explanatory comment.
🤖 Prompt for AI Agents
In apps/backend/handlers/deployment_handler.py around lines 175 to 181, the
cleanup_askpass() function currently swallows OSError with an empty except which
CodeQL flagged; update the handler to either add a short explanatory comment
documenting why errors are safe to ignore, or change the except to capture the
exception (except OSError as e) and emit a debug-level log containing context
and the exception (using the module logger or processLogger) so failures are
recorded for troubleshooting while still not raising in cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/backend/handlers/deployment_handler.py (1)
180-181: Add explanatory comment for empty except block.The empty
except OSError: passblock was flagged by static analysis. While the behavior is intentional (the file may already be deleted by thefinallyblock when the atexit handler runs), the lack of documentation makes it unclear. Add a brief comment explaining why the exception is safely ignored, or log at debug level.🔎 Proposed fix
try: os.unlink(askpass_path) - except OSError: - pass + except OSError: + # File may already be removed by finally block; safe to ignore + passOr with debug logging:
try: os.unlink(askpass_path) - except OSError: - pass + except OSError as e: + # File may already be removed by finally block + logger.debug(f"Askpass cleanup: {e}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/handlers/deployment_handler.py
🧰 Additional context used
📓 Path-based instructions (1)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/handlers/deployment_handler.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/handlers/deployment_handler.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
⏰ 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). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
apps/backend/handlers/deployment_handler.py (1)
1-410: Excellent implementation with comprehensive security and error handling.The deployment handler is well-architected with strong security measures:
- Secure token handling via GIT_ASKPASS pattern prevents command-line exposure
- Cross-platform file locking prevents race conditions
- Robust URL validation with support for custom hosts
- Retry logic with exponential backoff for reliability
- Platform-aware notifications that gracefully degrade
- Comprehensive exception handling with appropriate logging
The code follows Python best practices (modern type hints, async patterns, proper encoding), and nearly all previous review feedback has been incorporated. The only remaining minor issue is documentation of the intentional exception handling in the cleanup routine.
| message="Could not acquire deployment lock (another push in progress)", | ||
| ) | ||
|
|
||
| return DeploymentResult(success=False, message="Unknown deployment mode") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider raising an exception for impossible deployment mode.
If DeploymentMode is an enum with only three values (LOCAL_ONLY, AUTO_PUSH, AUTO_PR), reaching this line represents a programming error rather than a runtime error. Consider raising ValueError or AssertionError instead of returning an error result, to fail fast and surface the issue during development.
🔎 Proposed fix
- return DeploymentResult(success=False, message="Unknown deployment mode")
+ raise ValueError(f"Unknown deployment mode: {self.config.mode}")🤖 Prompt for AI Agents
In apps/backend/handlers/deployment_handler.py around line 92, the code returns
DeploymentResult(success=False, message="Unknown deployment mode") for an
impossible DeploymentMode; instead raise an exception to fail fast. Replace the
return with an explicit exception (e.g., raise AssertionError(f"Unhandled
DeploymentMode: {mode}") or raise ValueError(f"Unhandled DeploymentMode:
{mode}")) so programming errors surface during development; keep the message
descriptive and include the offending mode variable.
MikeeBuilds
added
feature
3 participants
Summary
local_only(default),auto_push,auto_prChanges
apps/backend/config/deployment.py- Configuration with env + project overrideapps/backend/handlers/deployment_handler.py- Push/PR logic with retryapps/backend/.env.example- Added DEPLOYMENT_* variablesTest plan
DEPLOYMENT_MODE=local_only→ verify no push after mergeDEPLOYMENT_MODE=auto_push→ verify push to originDEPLOYMENT_MODE=auto_pr→ verify PR created via gh CLI🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.