feat(cli): add comprehensive GitHub PR workflow commands#305
Conversation
WalkthroughAdds a new "pr" Typer subcommand group implementing pull request management (create, list, get, merge, close, status), git helper utilities, tests, documentation, and CI workflow adjustments; registers Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as PR CLI Command
participant GitHelper as Git Helpers
participant GitHub as GitHubIntegration
User->>CLI: codeframe pr create --title "..."
CLI->>GitHelper: get_current_branch()
GitHelper-->>CLI: feature/branch-name
CLI->>GitHelper: get_commit_messages(base, head)
GitHelper-->>CLI: commit messages
CLI->>GitHub: create_pull_request(head=feature/branch-name, title, body, base)
GitHub-->>CLI: PR details (number, url)
CLI-->>User: Print PR metadata & URL
sequenceDiagram
actor User
participant CLI as PR CLI Command
participant GitHub as GitHubIntegration
User->>CLI: codeframe pr list --status open --format table
CLI->>GitHub: list_pull_requests(state='open')
GitHub-->>CLI: list of PRs
CLI->>CLI: format results (table or json)
CLI-->>User: Rendered output
sequenceDiagram
actor User
participant CLI as PR CLI Command
participant GitHub as GitHubIntegration
User->>CLI: codeframe pr merge --number 42 --strategy squash
CLI->>GitHub: get_pull_request(42)
GitHub-->>CLI: PR details (state)
CLI->>GitHub: merge_pull_request(42, method='squash')
GitHub-->>CLI: merge result (merged, sha)
CLI-->>User: Success/failure message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Based on my review of PR #305, here's my feedback: SummaryThis PR implements GitHub Pull Request CLI commands that are part of Phase 5 (Integrated Git Workflow & PR Management) in the Golden Path. The implementation follows v2 architecture patterns correctly: CLI-first, headless, no server required. ✅ Strengths
|
|
Review comment posted successfully. The PR is well-implemented with good test coverage and follows v2 architecture patterns. The main issue identified is a potential |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/cli/app.py`:
- Around line 4164-4167: The import of pr_app is placed after executable code
which triggers E402; either move the import of pr_app to the module-level
imports at the top of the file or suppress the check on that line by adding the
inline comment "# noqa: E402" to the import statement that precedes
app.add_typer(pr_app, name="pr"); update the same import line where pr_app is
referenced so linters stop failing while preserving the existing
app.add_typer(pr_app, name="pr") call.
🧹 Nitpick comments (1)
codeframe/cli/pr_commands.py (1)
245-399: Validate enum-like CLI options before hitting GitHub.
status,format, andstrategyaccept any string today, which can lead to confusing API errors despite the documented allowed values. Consider normalizing and validating these inputs locally.♻️ Suggested validation (local normalization + explicit allowed sets)
def list_prs( status: str = typer.Option( "open", "--status", "-s", help="Filter by status: open, closed, all" ), format: str = typer.Option( "table", "--format", "-f", help="Output format: table or json" ), ): @@ - try: + status = status.lower() + if status not in {"open", "closed", "all"}: + console.print(f"[red]Error:[/red] Invalid status '{status}'. Use open|closed|all.") + raise typer.Exit(1) + format = format.lower() + if format not in {"table", "json"}: + console.print(f"[red]Error:[/red] Invalid format '{format}'. Use table|json.") + raise typer.Exit(1) + try: token, repo = _get_github_config() @@ def get_pr( pr_number: int = typer.Argument(..., help="PR number"), format: str = typer.Option( "text", "--format", "-f", help="Output format: text or json" ), ): @@ - try: + format = format.lower() + if format not in {"text", "json"}: + console.print(f"[red]Error:[/red] Invalid format '{format}'. Use text|json.") + raise typer.Exit(1) + try: token, repo = _get_github_config() @@ def merge_pr( pr_number: int = typer.Argument(..., help="PR number to merge"), strategy: str = typer.Option( "squash", "--strategy", "-s", help="Merge strategy: squash, merge, rebase", ), ): @@ - try: + strategy = strategy.lower() + if strategy not in {"squash", "merge", "rebase"}: + console.print(f"[red]Error:[/red] Invalid strategy '{strategy}'. Use squash|merge|rebase.") + raise typer.Exit(1) + try: token, repo = _get_github_config()
Add `codeframe pr` command group for GitHub Pull Request management: - `pr create`: Create PRs with auto-generated descriptions from commits - `pr list`: List PRs with status filtering and JSON output - `pr get`: Get detailed PR information - `pr merge`: Merge PRs with squash/merge/rebase strategies - `pr close`: Close PRs without merging - `pr status`: Show PR status for current branch Implementation follows v2 CLI patterns (headless, no server required): - Uses existing GitHubIntegration async client - Wraps async calls with asyncio.run() for CLI context - Full test coverage (23 tests) using TDD approach - Environment-based configuration (GITHUB_TOKEN, GITHUB_REPO) Commands registered in both v1 CLI (__init__.py) and v2 CLI (app.py). Documentation updated in CLI_WIREFRAME.md with examples.
frankbria
force-pushed
the
feature/pr-cli-commands
branch
from
ebfc9d8 to
5aa3fe4
Compare
PR Review SummaryThis PR implements GitHub Pull Request CLI commands following v2 architecture patterns correctly. The implementation is well-structured with comprehensive test coverage and proper CLI-first, headless design. ✅ Strengths
|
|
Review posted successfully. The key findings are: Must Fix (blocking CI):
Recommended Fix:
Clarification:
The PR is otherwise well-implemented with excellent test coverage and follows v2 architecture patterns correctly. |
Address code review feedback: return None instead of True when PR is already closed to skip the success message, consistent with merge_pr handling of already-merged PRs.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@codeframe/cli/pr_commands.py`:
- Around line 45-68: The get_current_branch function currently only catches
subprocess.CalledProcessError, so if the git executable is missing
subprocess.run will raise FileNotFoundError and crash; update get_current_branch
to also catch FileNotFoundError (in addition to CalledProcessError) and raise a
RuntimeError with a clear, user-friendly message (e.g., "git not found; please
install git and ensure it's on PATH") so callers of get_current_branch receive a
consistent RuntimeError when git is unavailable.
- Around line 71-116: Both get_git_diff_stats and get_commit_messages can raise
FileNotFoundError when git is not installed; update each function (around the
subprocess.run call in get_git_diff_stats and get_commit_messages) to also catch
FileNotFoundError (in addition to subprocess.CalledProcessError) and return an
empty string instead of letting the exception bubble up, so pr create won't
abort if git is missing.
| def get_current_branch(repo_path: Optional[Path] = None) -> str: | ||
| """Get the current git branch name. | ||
|
|
||
| Args: | ||
| repo_path: Path to the git repository (defaults to cwd) | ||
|
|
||
| Returns: | ||
| Current branch name | ||
|
|
||
| Raises: | ||
| RuntimeError: If not in a git repository | ||
| """ | ||
| cwd = repo_path or Path.cwd() | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "rev-parse", "--abbrev-ref", "HEAD"], | ||
| cwd=cwd, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| return result.stdout.strip() | ||
| except subprocess.CalledProcessError as e: | ||
| raise RuntimeError(f"Failed to get current branch: {e.stderr}") |
There was a problem hiding this comment.
Handle missing git executable in get_current_branch.
If git isn’t installed, subprocess.run raises FileNotFoundError and the CLI will crash instead of showing a friendly error.
🧩 Proposed fix
def get_current_branch(repo_path: Optional[Path] = None) -> str:
@@
- except subprocess.CalledProcessError as e:
- raise RuntimeError(f"Failed to get current branch: {e.stderr}")
+ except FileNotFoundError as e:
+ raise RuntimeError("git executable not found. Please install Git.") from e
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to get current branch: {e.stderr}") from e🤖 Prompt for AI Agents
In `@codeframe/cli/pr_commands.py` around lines 45 - 68, The get_current_branch
function currently only catches subprocess.CalledProcessError, so if the git
executable is missing subprocess.run will raise FileNotFoundError and crash;
update get_current_branch to also catch FileNotFoundError (in addition to
CalledProcessError) and raise a RuntimeError with a clear, user-friendly message
(e.g., "git not found; please install git and ensure it's on PATH") so callers
of get_current_branch receive a consistent RuntimeError when git is unavailable.
| def get_git_diff_stats(repo_path: Path, base: str, head: str) -> str: | ||
| """Get diff statistics between two branches. | ||
|
|
||
| Args: | ||
| repo_path: Path to the git repository | ||
| base: Base branch name | ||
| head: Head branch name | ||
|
|
||
| Returns: | ||
| Diff statistics as string | ||
| """ | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "diff", "--stat", f"{base}...{head}"], | ||
| cwd=repo_path, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| return result.stdout.strip() | ||
| except subprocess.CalledProcessError: | ||
| return "" | ||
|
|
||
|
|
||
| def get_commit_messages(repo_path: Path, base: str, head: str) -> str: | ||
| """Get commit messages between two branches. | ||
|
|
||
| Args: | ||
| repo_path: Path to the git repository | ||
| base: Base branch name | ||
| head: Head branch name | ||
|
|
||
| Returns: | ||
| Commit messages as string | ||
| """ | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "log", "--oneline", f"{base}..{head}"], | ||
| cwd=repo_path, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| return result.stdout.strip() | ||
| except subprocess.CalledProcessError: | ||
| return "" |
There was a problem hiding this comment.
Prevent auto-description from crashing when git is missing.
Both helpers can raise FileNotFoundError, which currently bubbles up and aborts pr create.
🧩 Proposed fix
def get_git_diff_stats(repo_path: Path, base: str, head: str) -> str:
@@
- except subprocess.CalledProcessError:
+ except FileNotFoundError:
+ return ""
+ except subprocess.CalledProcessError:
return ""
@@
def get_commit_messages(repo_path: Path, base: str, head: str) -> str:
@@
- except subprocess.CalledProcessError:
+ except FileNotFoundError:
+ return ""
+ except subprocess.CalledProcessError:
return ""📝 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.
| def get_git_diff_stats(repo_path: Path, base: str, head: str) -> str: | |
| """Get diff statistics between two branches. | |
| Args: | |
| repo_path: Path to the git repository | |
| base: Base branch name | |
| head: Head branch name | |
| Returns: | |
| Diff statistics as string | |
| """ | |
| try: | |
| result = subprocess.run( | |
| ["git", "diff", "--stat", f"{base}...{head}"], | |
| cwd=repo_path, | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| ) | |
| return result.stdout.strip() | |
| except subprocess.CalledProcessError: | |
| return "" | |
| def get_commit_messages(repo_path: Path, base: str, head: str) -> str: | |
| """Get commit messages between two branches. | |
| Args: | |
| repo_path: Path to the git repository | |
| base: Base branch name | |
| head: Head branch name | |
| Returns: | |
| Commit messages as string | |
| """ | |
| try: | |
| result = subprocess.run( | |
| ["git", "log", "--oneline", f"{base}..{head}"], | |
| cwd=repo_path, | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| ) | |
| return result.stdout.strip() | |
| except subprocess.CalledProcessError: | |
| return "" | |
| def get_git_diff_stats(repo_path: Path, base: str, head: str) -> str: | |
| """Get diff statistics between two branches. | |
| Args: | |
| repo_path: Path to the git repository | |
| base: Base branch name | |
| head: Head branch name | |
| Returns: | |
| Diff statistics as string | |
| """ | |
| try: | |
| result = subprocess.run( | |
| ["git", "diff", "--stat", f"{base}...{head}"], | |
| cwd=repo_path, | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| ) | |
| return result.stdout.strip() | |
| except FileNotFoundError: | |
| return "" | |
| except subprocess.CalledProcessError: | |
| return "" | |
| def get_commit_messages(repo_path: Path, base: str, head: str) -> str: | |
| """Get commit messages between two branches. | |
| Args: | |
| repo_path: Path to the git repository | |
| base: Base branch name | |
| head: Head branch name | |
| Returns: | |
| Commit messages as string | |
| """ | |
| try: | |
| result = subprocess.run( | |
| ["git", "log", "--oneline", f"{base}..{head}"], | |
| cwd=repo_path, | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| ) | |
| return result.stdout.strip() | |
| except FileNotFoundError: | |
| return "" | |
| except subprocess.CalledProcessError: | |
| return "" |
🤖 Prompt for AI Agents
In `@codeframe/cli/pr_commands.py` around lines 71 - 116, Both get_git_diff_stats
and get_commit_messages can raise FileNotFoundError when git is not installed;
update each function (around the subprocess.run call in get_git_diff_stats and
get_commit_messages) to also catch FileNotFoundError (in addition to
subprocess.CalledProcessError) and return an empty string instead of letting the
exception bubble up, so pr create won't abort if git is missing.
PR Review SummaryThis PR implements GitHub Pull Request CLI commands following v2 architecture patterns correctly. The implementation is well-structured with comprehensive test coverage and proper CLI-first, headless design. ✅ Strengths
|
|
The review comment has been posted successfully at: #305 (comment) Summary of Review Findings:Must Fix (blocking CI):
Recommended Fix:
Clarification:
The PR is otherwise well-implemented with excellent test coverage and follows v2 architecture patterns correctly. |
Improvements from ralph-claude-code workflow: - Add git credentials clearing step to prevent auth conflicts - Pass PR context (title, body, number, repo) as environment variables - Include PR title directly in prompt to avoid gh pr view calls - Add pyproject.toml to paths-ignore to skip config-only changes - Remove gh pr view/diff instructions (info now passed directly) Kept existing: - Concurrency block to prevent duplicate reviews for same PR - 10-minute timeout - Substantial changes threshold (5+ files OR 20+ lines)
|
Based on my review of PR #305, here are my findings: SummaryThis PR implements comprehensive GitHub Pull Request CLI commands following v2 architecture patterns (CLI-first, headless, no server required). The implementation supports Phase 5 of the Golden Path and is well-structured with good test coverage. ✅ Strengths
|
|
Review comment posted successfully. The key findings are: Must Fix (blocking CI):
Recommended Fix:
Optional Enhancements:
Verified NOT issues:
The PR is otherwise well-implemented with excellent test coverage and follows v2 architecture patterns correctly. |
Add TestPRCommands class with 9 tests covering: - pr help - pr list (table and JSON formats) - pr get - pr create - pr merge - pr close - pr status - Missing token error handling Tests use monkeypatch to mock GitHubIntegration, following the existing pattern for AI integration tests with MockProvider. All tests are marked v2 via the module-level pytestmark.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/cli/pr_commands.py`:
- Around line 245-329: The PR list command (list_prs) and pr_status rely on
GitHubIntegration.list_pull_requests which currently fetches only the default
first page (30 items), causing missing PRs; update the call to
list_pull_requests to support pagination by passing per_page and page parameters
(or add a new CLI option like --limit/--page to list_prs that surfaces
per_page/page to the user), then loop/fetch subsequent pages until fewer than
per_page items are returned (or until the user-specified limit) and aggregate
results before rendering; adjust pr_status to also use the paginated
list_pull_requests so branch lookup can find PRs beyond the first page.
PR ReviewThis PR implements comprehensive GitHub PR CLI commands following v2 architecture patterns. The implementation is well-structured with excellent test coverage and follows the CLI-first, headless approach correctly. ✅ Strengths
|
|
Review comment posted successfully at #305 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/opencode-review.yml:
- Around line 33-36: The assignments to additions and deletions use unquoted
GitHub context variables and risk shell injection; update the workflow to assign
them safely by wrapping the expressions in quotes and validating/coercing to
integers before the arithmetic (e.g., set additions and deletions using quoted
expansions like additions="${{ github.event.pull_request.additions }}" and
deletions="${{ github.event.pull_request.deletions }}", then sanitize or default
non-numeric values to 0 before computing total), and ensure echo writes the
computed numeric total to GITHUB_OUTPUT.
- Around line 82-86: The workflow currently passes user-controlled env vars
(PR_TITLE, PR_BODY, PR_NUMBER, REPO_NAME) into the
anomalyco/opencode/github@latest action without sanitization; update the step
that calls the action so it first sanitizes/escapes these inputs before they are
interpolated into prompts or shell commands (e.g., implement or call a
sanitizeInput helper that strips/escapes control characters, backticks, shell
metacharacters, and prompt injection tokens, enforce max length, and validate
expected characters) and ensure the action invocation uses the sanitized values
rather than raw ${{ github.event.pull_request.* }} env vars; also verify the
action's input handling (e.g., any prompt-construction code inside the action)
performs the same sanitization/escaping for PR_TITLE and PR_BODY before passing
them to AI or to any shell.
🧹 Nitpick comments (3)
tests/cli/test_v2_cli_integration.py (3)
926-941: Mock setup is repeated across all test methods—consider extracting to a fixture.Each test method creates a nearly identical
AsyncMocksetup forGitHubIntegration. This repetition increases maintenance burden and makes tests harder to update.Consider a shared mock_github fixture
`@pytest.fixture` def mock_github(self, monkeypatch): """Create and configure mock GitHubIntegration.""" from unittest.mock import AsyncMock mock_gh = AsyncMock() mock_gh.close = AsyncMock() monkeypatch.setattr( "codeframe.cli.pr_commands.GitHubIntegration", lambda **kwargs: mock_gh, ) return mock_ghThen in tests:
def test_pr_list(self, mock_github_env, mock_pr_details, mock_github): mock_github.list_pull_requests = AsyncMock(return_value=[mock_pr_details]) result = runner.invoke(app, ["pr", "list"]) # ...
1003-1022: Missing verification that merge was called with correct parameters.The test verifies the command succeeds but doesn't assert that
merge_pull_requestwas called with the expected PR number and merge strategy.Add mock call verification
result = runner.invoke(app, ["pr", "merge", "42"]) assert result.exit_code == 0 assert "merged" in result.output.lower() + mock_gh.merge_pull_request.assert_called_once() + call_args = mock_gh.merge_pull_request.call_args + assert call_args[0][0] == 42 # PR number
886-1070: Consider adding tests for additional edge cases.The test coverage is solid for the happy path, but consider adding tests for:
- PR not found (404 scenario)
- Network/API errors
- Merge conflicts
- Already merged PR
1 participant
Summary
Adds a new
codeframe prcommand group for GitHub Pull Request management, following v2 CLI patterns (headless, no server required).New Commands
codeframe pr createcodeframe pr listcodeframe pr getcodeframe pr mergecodeframe pr closecodeframe pr statusImplementation Details
GitHubIntegrationasync client fromcodeframe/git/github_integration.pyasyncio.run()for CLI contextGITHUB_TOKEN,GITHUB_REPO)__init__.py) and v2 CLI (app.py)Files Changed
codeframe/cli/pr_commands.py- Core PR command implementation (545 lines)tests/cli/test_pr_commands.py- Comprehensive test suite (23 tests)codeframe/cli/app.py- Register PR commands in v2 CLIcodeframe/cli/__init__.py- Register PR commands in v1 CLIdocs/CLI_WIREFRAME.md- Updated documentation with examplesExample Usage
Test Plan
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.