feat(api): add Git REST API for branch and commit operations (#270) #282
Conversation
Implement REST endpoints for git operations to unblock dependent tickets:
- POST /api/projects/{id}/git/branches - Create feature branch
- GET /api/projects/{id}/git/branches - List branches
- GET /api/projects/{id}/git/branches/{name} - Get branch details
- POST /api/projects/{id}/git/commit - Create commit
- GET /api/projects/{id}/git/commits - List commits
- GET /api/projects/{id}/git/status - Get working tree status
Add broadcast_branch_created WebSocket helper for real-time updates.
Include 46 comprehensive integration tests following TDD principles.
Unblocks: #271 (Git Commit UI), #272 (PR Creation API), #279 (Git Merge API)
WalkthroughAdds a new Git REST router under /api/projects/{project_id}/git/* for branch and commit management plus repo status, integrates GitWorkflowManager and DB checks, broadcasts branch_created WebSocket events, updates persistence with branch lookup, mounts router in the server, and adds comprehensive tests and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Git Router
participant DB as Database
participant GitMgr as GitWorkflowManager
participant WS as WebSocket Manager
Client->>Server: POST /api/projects/{id}/git/branches (issue_number, title)
Server->>DB: get_project_or_404(project_id)
Server->>DB: check_project_access(user, project_id)
DB-->>Server: project & access OK
Server->>GitMgr: create_feature_branch(params)
GitMgr-->>Server: branch created / error
Server->>DB: persist git_branches metadata
Server->>WS: broadcast_branch_created(project_id, branch_name, issue_number, issue_id?)
Server-->>Client: 201 BranchCreateResponse (branch metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 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)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)codeframe/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)tests/api/test_git_api.py (2)
⏰ 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)
🔇 Additional comments (18)
Comment |
Add Git REST API under
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Code ReviewOverall, this is a well-implemented PR with excellent test coverage (46 comprehensive tests). The code follows FastAPI patterns well and includes proper WebSocket broadcasts for real-time updates. ✅ Strengths
🔍 Issues to Address1. Security: Unvalidated
|
|
Review posted to PR #282 at #282 (comment) Summary: This is a high-quality PR with excellent test coverage (46 tests) and clean code structure. The implementation follows FastAPI patterns and includes proper WebSocket broadcasts for real-time updates. Key issues identified:
Minor suggestions:
Overall: Ready to merge after addressing the 4 main issues above. Test coverage is excellent and code follows project conventions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/git.py:
- Around line 313-315: The code treats an empty string status as falsy, causing
all_branches to be [] when callers pass status=""; change the conditional from
"if status" to explicitly check for None (e.g., "if status is not None") or
validate allowed status values before calling db.get_branches_by_status so that
an empty string is treated as an explicit value (or rejected) rather than
silently returning an empty list; update the logic around the all_branches
assignment and any upstream validation to ensure expected behavior when
status=="".
- Around line 538-540: The timestamp formatting assumes commits are already in
UTC; update the code that builds the commit dict (the lines using
commit.committed_datetime.isoformat().replace("+00:00", "Z")) to first convert
the datetime to UTC via commit.committed_datetime.astimezone(UTC) and then call
.isoformat().replace("+00:00", "Z") so all timestamps normalize to UTC; keep the
surrounding fields (message, author) unchanged and reuse the existing UTC
import.
- Around line 241-247: The code allows creating a git branch when no matching
issue exists (matching_issue is None), resulting in orphaned branches; stop
branch creation when the issue isn't found and avoid duplicate lookups by
passing project-scoped info to the workflow layer. Specifically, in the endpoint
that uses db.get_project_issues / matching_issue / issue_id and calls
workflow_manager.create_feature_branch, validate matching_issue and return an
error (e.g., 404 or 400) instead of proceeding when it's None; alternatively
change the call to workflow_manager.create_feature_branch to accept project_id +
issue_number (so the workflow can locate the issue only within that project) and
have create_feature_branch return the resolved issue_id which you then store.
Also remove the redundant cross-project issue lookup in
workflow_manager.create_feature_branch so the lookup is performed only once and
branches are always created with a valid issue_id.
🧹 Nitpick comments (3)
tests/api/test_git_api.py (1)
745-764: Consider using try/finally for header restoration to prevent test pollution.If an exception occurs between deleting the header (Line 750) and restoring it (Line 762), subsequent tests in the class may fail unexpectedly. Using try/finally ensures cleanup even on test failure.
♻️ Suggested safer approach
def test_create_branch_requires_auth(self, api_client, test_project_with_issue): """Test that branch creation requires authentication.""" project = test_project_with_issue - # Remove auth header - del api_client.headers["Authorization"] - - response = api_client.post( - f"/api/projects/{project['project_id']}/git/branches", - json={ - "issue_number": project["issue_number"], - "issue_title": project["issue_title"], - }, - ) - - # Restore auth header for cleanup + # Save and remove auth header from tests.api.conftest import create_test_jwt_token - api_client.headers["Authorization"] = f"Bearer {create_test_jwt_token()}" + original_auth = api_client.headers.get("Authorization") + try: + del api_client.headers["Authorization"] - assert response.status_code == 401 + response = api_client.post( + f"/api/projects/{project['project_id']}/git/branches", + json={ + "issue_number": project["issue_number"], + "issue_title": project["issue_title"], + }, + ) + + assert response.status_code == 401 + finally: + # Restore auth header for cleanup + api_client.headers["Authorization"] = original_auth or f"Bearer {create_test_jwt_token()}"codeframe/ui/routers/git.py (2)
370-385: Consider adding a direct branch lookup by name for better performance.The current implementation iterates through all branches for each status to find a match. This works but could be slow with many branches. A direct
db.get_branch_by_name(branch_name)method would be more efficient.#!/bin/bash # Check if a direct branch lookup method exists rg -n "def get_branch" --type py -C2
422-424: Minor: Redundant validation - already enforced by Pydantic.The
files_modifiedlist is already validated by Pydantic'smin_length=1constraint on Line 89. This explicit check is defensive but unnecessary.♻️ Can be removed if desired
- # Validate files_modified is not empty - if not request.files_modified: - raise HTTPException(status_code=400, detail="No files to commit") - # Get task and verify it belongs to this project
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
codeframe/ui/routers/git.pycodeframe/ui/server.pycodeframe/ui/websocket_broadcasts.pytests/api/conftest.pytests/api/test_git_api.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development
Use FastAPI framework for backend API development
Use SQLite with async support (aiosqlite) for database operations
Use tiktoken for token counting
Run linting with ruff check .
Import and use domain-specific repositories from codeframe/persistence/repositories/ for data access instead of direct database calls
Files:
codeframe/ui/websocket_broadcasts.pycodeframe/ui/routers/git.pycodeframe/ui/server.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
🧬 Code graph analysis (2)
codeframe/ui/websocket_broadcasts.py (2)
codeframe/core/models.py (2)
project_id(234-235)issue_number(238-239)codeframe/ui/shared.py (1)
broadcast(154-185)
tests/api/test_git_api.py (4)
tests/api/conftest.py (2)
api_client(67-177)create_test_jwt_token(43-63)codeframe/core/models.py (2)
issue_number(238-239)title(242-243)codeframe/cli/api_client.py (1)
post(230-240)tests/integration/conftest.py (1)
create_response(261-265)
⏰ 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). (5)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: Macroscope - Correctness Check
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
🔇 Additional comments (11)
tests/api/conftest.py (1)
215-218: LGTM! Cleanup order correctly handles foreign key constraints.The additions properly account for the new
git_branchestable andchangelogreferences. The comment on Line 217 correctly documents the FK dependency on issues.codeframe/ui/websocket_broadcasts.py (1)
239-271: LGTM! Follows established broadcast patterns.The
broadcast_branch_createdfunction is consistent with other broadcast helpers in the module. Message structure, timestamp formatting, optional field handling, and error logging all align with existing patterns likebroadcast_commit_created.codeframe/ui/server.py (2)
27-27: LGTM! Router import follows alphabetical ordering.
334-334: LGTM! Router mounting follows existing conventions.tests/api/test_git_api.py (4)
29-66: LGTM! Well-structured fixture with proper git initialization.The fixture correctly initializes a git repository with an initial commit, which is required for branch operations. The use of
tempfile.TemporaryDirectorycontext manager ensures cleanup.
143-283: LGTM! Comprehensive test coverage for branch creation.Tests cover success paths, validation errors (422), not found (404), and duplicate handling (400). The test at Line 212-227 correctly verifies the branch actually exists in the git repository.
431-587: LGTM! Commit tests properly create files before testing.Tests correctly create actual files in the workspace before making commit requests, ensuring realistic test scenarios.
660-734: LGTM! Status tests verify working tree states effectively.Tests cover clean repo, untracked files, and response field validation. The
test_status_with_untracked_filetest (Lines 716-729) correctly creates a file and verifies it appears inuntracked_files.codeframe/ui/routers/git.py (3)
42-135: LGTM! Well-structured Pydantic models with appropriate validation.Field validators handle whitespace stripping,
min_lengthconstraints prevent empty inputs, and response models useOptionalfor nullable fields.
142-202: LGTM! Helper functions properly encapsulate validation and error handling.Clear separation of concerns: project lookup, access control, and workflow manager initialization. Error handling maps to appropriate HTTP status codes (400, 403, 404, 500).
560-620: LGTM! Correct git status implementation.The endpoint properly uses:
repo.is_dirty(untracked_files=True)for dirty staterepo.index.diff(None)for modified (unstaged) filesrepo.index.diff("HEAD")for staged filesrepo.untracked_filesfor untracked files
- Security: Add branch name validation with regex pattern to prevent injection attacks via special git characters (~, ^, :, ?, *, etc.) - Performance: Add single-query get_branch_by_name_and_issues() method replacing 3 sequential DB queries in get_branch endpoint - Bug: Retrieve commit message from specific commit_hash instead of repo.head.commit which may have moved after creation - Error handling: Replace broad Exception catches with specific types (git.BadName, git.InvalidGitRepositoryError, ValueError, KeyError)
Code ReviewLooking at the latest code, I can see that most issues from previous reviews have been addressed. However, I found a few areas that need attention: ✅ Previously Addressed IssuesThe following issues from earlier reviews appear to be fixed:
🚨 Critical Issue: Branch Creation Without Issue ValidationLocation: The endpoint creates a git branch even when no matching issue exists: matching_issue = next(
(i for i in issues if i.issue_number == request.issue_number),
None
)
issue_id = matching_issue.id if matching_issue else NoneProblems:
Recommendation: Return 404 when if not matching_issue:
raise HTTPException(
status_code=404,
detail=f"Issue {request.issue_number} not found in project"
)🐛 Minor Bug: Empty Status String HandlingLocation: The all_branches = db.get_branches_by_status(status) if status else []If a client passes Recommendation: Use explicit None check: all_branches = db.get_branches_by_status(status) if status is not None else []Or validate status values upfront and reject invalid ones.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/git.py:
- Around line 650-665: The staged-files call repo.index.diff("HEAD") can raise
if the repo has no commits (no HEAD); update the logic that builds staged_files
(used for GitStatusResponse) to first detect or handle a missing HEAD (e.g.,
check repo.head.is_valid() or repo.head.is_detached/exists) and if HEAD is
absent return an empty staged_files list, otherwise call
repo.index.diff("HEAD"); alternatively wrap repo.index.diff("HEAD") in a
try/except catching git.BadName and set staged_files = [] on that error so the
GitStatusResponse always receives a safe list.
- Around line 376-426: The route decorator for get_branch currently uses
"{branch_name}" which cannot match branch names containing slashes; update the
router path in the @router.get decorator to use the path converter
"{branch_name:path}" so FastAPI captures names with '/' (keep the function name
get_branch and existing parameter branch_name and validation via
validate_branch_name), and ensure any related docs/tests or client code that
call this endpoint expect slash-containing branch names to be URL-encoded as
appropriate.
🧹 Nitpick comments (2)
codeframe/ui/routers/git.py (2)
38-66: Consider adding validation for consecutive slashes and leading/trailing slashes.Git rejects branch names with consecutive slashes (
//), leading slashes (/foo), or trailing slashes (foo/). The current regex doesn't explicitly prevent these patterns.🔧 Suggested enhancement
# Check for dangerous patterns - dangerous_patterns = ['..', '@{', '~', '^', ':', '?', '*', '[', '\\', ' '] + dangerous_patterns = ['..', '@{', '~', '^', ':', '?', '*', '[', '\\', ' ', '//'] for pattern in dangerous_patterns: if pattern in branch_name: raise ValueError(f"Branch name contains invalid character sequence: {pattern}") + + # Check for leading/trailing slashes + if branch_name.startswith('/') or branch_name.endswith('/'): + raise ValueError("Branch name cannot start or end with a slash")
463-466: Redundant validation for empty files_modified.The
CommitRequestmodel already validates thatfiles_modifiedcannot be empty viaField(..., min_length=1)and thevalidate_filesfield validator. This explicit check is defensive but redundant.Could remove this check since Pydantic will reject requests with empty
files_modifiedbefore reaching the endpoint handler.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codeframe/persistence/database.pycodeframe/persistence/repositories/git_repository.pycodeframe/ui/routers/git.py
🧰 Additional context used
📓 Path-based instructions (2)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development
Use FastAPI framework for backend API development
Use SQLite with async support (aiosqlite) for database operations
Use tiktoken for token counting
Run linting with ruff check .
Import and use domain-specific repositories from codeframe/persistence/repositories/ for data access instead of direct database calls
Files:
codeframe/persistence/repositories/git_repository.pycodeframe/persistence/database.pycodeframe/ui/routers/git.py
codeframe/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/persistence/**/*.py: Use Repository pattern for data access with domain-specific repositories in codeframe/persistence/repositories/
Use pre-production flattened v1.0 database schema with direct table creation (no migration system)
Maintain backward compatibility with existing imports and method signatures in Database repository migration
Files:
codeframe/persistence/repositories/git_repository.pycodeframe/persistence/database.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to codeframe/persistence/**/*.py : Maintain backward compatibility with existing imports and method signatures in Database repository migration
Applied to files:
codeframe/persistence/database.py
🧬 Code graph analysis (2)
codeframe/persistence/repositories/git_repository.py (1)
codeframe/persistence/database.py (1)
get_branch_by_name_and_issues(634-636)
codeframe/persistence/database.py (1)
codeframe/persistence/repositories/git_repository.py (1)
get_branch_by_name_and_issues(211-239)
⏰ 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). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: opencode-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (7)
codeframe/persistence/database.py (1)
634-637: LGTM!The delegation method follows the established pattern used by other git branch methods in this facade class, maintaining backward compatibility. Based on learnings, this maintains backward compatibility with existing imports and method signatures.
codeframe/persistence/repositories/git_repository.py (1)
211-239: LGTM!The implementation is clean and addresses performance optimization by using a single-query lookup with an
INclause. Key points:
- Empty
issue_idsguard prevents invalid SQL and unnecessary queries- Proper parameterization protects against SQL injection (placeholders are only
?characters, branch_name is parameterized)LIMIT 1is appropriate for this lookup patterncodeframe/ui/routers/git.py (5)
32-36: Branch name regex may reject valid single-character alphanumeric branches.The regex
^[a-zA-Z0-9][-a-zA-Z0-9_./]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$correctly handles single-character branch names via the alternation. However, the pattern requires at least 2 characters for the first alternative (start and end alphanumeric with optional middle characters), which is handled by the|^[a-zA-Z0-9]$branch. This looks correct.
79-172: LGTM!The Pydantic models are well-structured with proper Pydantic v2 syntax (
field_validatorwith@classmethod). Input validation covers empty/whitespace strings and empty file lists.
179-239: LGTM!Helper functions are well-factored with appropriate error handling. The specific exception handling for
git.InvalidGitRepositoryErrorandgit.NoSuchPathErroraligns with the PR's objective to replace broadExceptioncatches with specific exceptions.
566-604: LGTM!The implementation efficiently uses
iter_commitswithmax_countto avoid loading all commits. Error handling is comprehensive with specific exceptions (git.BadNamefor invalid branch references) as noted in the PR objectives. The defensive try/except forfiles_changedstats is appropriate since not all commits may have accessible stats.
278-300: The concern about None issue_id is unfounded—broadcast_branch_created is explicitly designed to handle Optional[int].The
broadcast_branch_createdfunction signature declaresissue_id: Optional[int] = None, and its implementation conditionally includes theissue_idin the message only when it's not None (if issue_id is not None). The broadcast operation itself is a websocket notification that gracefully handles the None case. No issues are expected when proceeding without a matching issue.
- Add 404 response when issue not found in create_branch endpoint
- Add VALID_BRANCH_STATUSES validation for status query parameter
- Use {branch_name:path} to support branch names with slashes
- Handle empty repos safely in get_git_status (no HEAD reference)
- Add test for issue-not-found scenario
Addresses code review feedback for PR #282.
Code ReviewOverall, this is a high-quality PR with excellent test coverage (50 comprehensive tests) and clean implementation. The code follows FastAPI patterns well and includes proper WebSocket broadcasts for real-time updates. ✅ Strengths
🔍 Issues to Address1. Timestamp normalization in list_commitsLocation: The timestamp formatting assumes commits are already in UTC but doesn't normalize: timestamp=commit.committed_datetime.isoformat().replace("+00:00", "Z"),If a commit was made in a different timezone (e.g., Recommendation: timestamp=commit.committed_datetime.astimezone(UTC).isoformat().replace("+00:00", "Z"),2. Redundant validation for files_modifiedLocation: The Recommendation: Remove lines 480-482 as Pydantic already enforces this validation. 3. Branch name regex doesn't prevent consecutive slashesLocation: The regex allows forward slashes but doesn't explicitly prevent consecutive slashes ( Recommendation: Add to dangerous_patterns check: dangerous_patterns = ['..', '@{', '~', '^', ':', '?', '*', '[', '\\', ' ', '//']
# Check for leading/trailing slashes
if branch_name.startswith('/') or branch_name.endswith('/'):
raise ValueError("Branch name cannot start or end with a slash")📝 Minor Suggestions
🎯 Test CoverageThe test suite is excellent with 50 tests covering all endpoints and error cases:
SummaryThis PR is ready to merge after addressing the 3 minor issues above. The code quality is high, tests are comprehensive, and it follows project conventions. The critical issue validation (commit f8407a9) was properly addressed. Key improvement from previous reviews: All critical issues have been fixed in recent commits. Only minor polishing items remain. |
Review CompletePosted comprehensive review to PR #282: #282 (comment) Overall Assessment: High-quality PR ready to merge after addressing 3 minor issues. Key FindingsStrengths:
Minor Issues to Address:
The PR successfully addresses all critical issues from previous reviews. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/git.py:
- Around line 674-682: The code uses repo.index.entries.keys() directly which
yields (path, stage) tuples, causing staged_files to contain tuples instead of
strings expected by GitStatusResponse; update the else branch where staged_files
is set (inside the try when repo.head.is_valid() is false) to extract only the
path component from each key returned by repo.index.entries (e.g., map or list
comprehension pulling key[0] or using tuple unpacking) so staged_files becomes
List[str]; keep the existing logic for repo.index.diff("HEAD") and the
git.BadName except block unchanged.
🧹 Nitpick comments (4)
codeframe/ui/routers/git.py (3)
363-374: Consider performance implications of filtering in-memory.The current approach fetches all branches by status and filters in Python. For projects with many issues, this could be optimized with a single database query that joins branches with project issues. However, this is acceptable for now given typical usage patterns.
480-482: Redundant validation, but harmless.This check duplicates the Pydantic
min_length=1constraint onfiles_modified. It does provide a clearer error message ("No files to commit" vs generic validation error), so keeping it is acceptable.
594-597: Bareexcept Exceptioncould mask unexpected errors.While the intent is to handle cases where
commit.statsmight fail, catching all exceptions could hide bugs. Consider logging the exception at DEBUG level for diagnosability.♻️ Suggested improvement
try: files_changed = commit.stats.total.get("files", 0) if commit.stats else None - except Exception: + except Exception as e: + logger.debug(f"Could not get files_changed for commit {commit.hexsha[:7]}: {e}") files_changed = Nonetests/api/test_git_api.py (1)
755-777: Consider adding authorization tests for other endpoints.Only
create_branchis tested for authorization. While the auth middleware likely applies uniformly to all endpoints, adding at least one read endpoint (e.g.,list_branches) to the auth test class would strengthen confidence that all Git endpoints are protected.♻️ Suggested addition
def test_list_branches_requires_auth(self, api_client, test_project_with_git): """Test that branch listing requires authentication.""" project = test_project_with_git # Remove auth header del api_client.headers["Authorization"] response = api_client.get( f"/api/projects/{project['id']}/git/branches" ) # Restore auth header for cleanup from tests.api.conftest import create_test_jwt_token api_client.headers["Authorization"] = f"Bearer {create_test_jwt_token()}" assert response.status_code == 401
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/ui/routers/git.pytests/api/test_git_api.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development
Use FastAPI framework for backend API development
Use SQLite with async support (aiosqlite) for database operations
Use tiktoken for token counting
Run linting with ruff check .
Import and use domain-specific repositories from codeframe/persistence/repositories/ for data access instead of direct database calls
Files:
codeframe/ui/routers/git.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
🧬 Code graph analysis (1)
tests/api/test_git_api.py (2)
tests/api/conftest.py (2)
api_client(67-177)create_test_jwt_token(43-63)codeframe/core/models.py (2)
issue_number(238-239)title(242-243)
⏰ 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). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: opencode-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (11)
codeframe/ui/routers/git.py (6)
1-31: LGTM!Imports are well-organized and appropriate for the module's functionality. Using
datetimewithUTCfrom Python 3.11+ is correct per coding guidelines.
32-67: LGTM!The branch name validation implements defense-in-depth by checking dangerous patterns before the regex match. This follows Git's own branch name restrictions appropriately.
79-93: LGTM!Proper use of Pydantic v2
@field_validatorwith@classmethoddecorator order. Whitespace stripping prevents empty-but-whitespace inputs.
179-239: LGTM!Helper functions properly encapsulate common patterns (project lookup, access check, workflow manager instantiation) with appropriate HTTP exceptions.
246-322: LGTM!The create_branch endpoint properly validates issue existence before creating the branch, handles errors appropriately, and broadcasts the event after successful creation.
393-444: LGTM!Using
{branch_name:path}correctly supports slashes in branch names. The single-query lookup viaget_branch_by_name_and_issuesis efficient.tests/api/test_git_api.py (5)
18-27: LGTM!The
get_app()helper correctly imports the app locally to ensure it gets the reloaded instance after theapi_clientfixture reloads the server module.
29-67: LGTM!The
test_project_with_gitfixture properly initializes a git repository with an initial commit, which is required for branch operations. Usingtempfile.TemporaryDirectoryensures cleanup.
143-297: LGTM!Comprehensive test coverage for branch creation including happy path, validation errors (422), not found errors (404), and duplicate branch handling (400).
444-601: LGTM!Commit tests properly create files in the workspace before attempting commits, ensuring realistic test scenarios. Good coverage of validation and error cases.
673-748: LGTM!Git status tests cover both clean and dirty repository states, verifying all expected fields in the response.
…zation - Extract only path component from repo.index.entries.keys() tuples (keys are (path, stage) pairs, not plain strings) - Use .astimezone(UTC) before formatting commit timestamps for proper UTC normalization Addresses additional code review feedback for PR #282.
Code ReviewThis PR is in excellent condition with all critical issues from previous reviews addressed. The code is production-ready. ✅ All Previously Identified Issues Fixed
📝 Minor Suggestions (Non-Blocking)
🎯 Overall AssessmentExcellent test coverage (50 tests) with comprehensive coverage of:
The code follows FastAPI patterns, includes proper WebSocket broadcasts, and maintains clean separation of concerns. SummaryReady to merge ✅ All critical and most minor issues have been addressed. The remaining suggestions are optional optimizations that don't block merge. The implementation is solid, well-tested, and follows project conventions. |
Code ReviewThis PR is in excellent condition with all critical issues from previous reviews addressed. The code is production-ready. ✅ All Previously Identified Issues Fixed
📝 Minor Suggestions (Non-Blocking)
🎯 Overall AssessmentExcellent test coverage (50 tests) with comprehensive coverage of:
The code follows FastAPI patterns, includes proper WebSocket broadcasts, and maintains clean separation of concerns. SummaryReady to merge ✅ All critical and most minor issues have been addressed. The remaining suggestions are optional optimizations that don't block merge. The implementation is solid, well-tested, and follows project conventions. |
|
Review posted to PR #282: #282 (comment) SummaryExcellent PR - Production Ready ✅ All critical issues from previous reviews have been addressed: Fixed Issues
Minor Suggestions (Non-blocking)
Test CoverageExcellent with 50 tests covering all endpoints, auth, and error cases. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/git.py:
- Around line 504-509: The commit_task_changes flow currently passes
request.files_modified directly to repo.index.add(), so add path validation
inside commit_task_changes: for each path in files_modified reject any that
contain '..' segments or are absolute (os.path.isabs), resolve the candidate
against the repository/project root (use repo.working_tree_dir or the project
root variable and os.path.realpath) and ensure the resolved path has the project
root as its commonpath (os.path.commonpath) to prevent escaping the workspace;
on validation failure raise or return an explicit error and do not call
repo.index.add() with unsafe paths.
- Around line 246-252: The create_branch handler has a race where both requests
read self.repo.branches before either calls self.repo.create_head, causing a
GitCommandError and 500; fix by enforcing atomicity: add a UNIQUE constraint on
branch name scoped to project in the Branch DB model (project_id + name), wrap
branch creation in a DB transaction and attempt to insert the branch row first,
catching IntegrityError to return 409, then create the git branch
(repo.create_head) — or alternatively serialize git operations using a per-repo
lock around self.repo.create_head to prevent concurrent create_head calls;
update create_branch to handle GitCommandError/IntegrityError and return a
proper 409 conflict instead of 500.
🧹 Nitpick comments (2)
codeframe/ui/routers/git.py (2)
32-66: Branch name validation is solid, consider adding.locksuffix check.The validation correctly blocks common dangerous patterns like
..,@{, and special characters. Git also disallows branch names ending with.lock(e.g.,refs/heads/foo.lock). Consider adding this check for completeness.Optional enhancement
# Check for dangerous patterns dangerous_patterns = ['..', '@{', '~', '^', ':', '?', '*', '[', '\\', ' '] for pattern in dangerous_patterns: if pattern in branch_name: raise ValueError(f"Branch name contains invalid character sequence: {pattern}") + + # Git disallows .lock suffix + if branch_name.endswith('.lock'): + raise ValueError("Branch name cannot end with '.lock'")
367-374: Clarify thestatus=Nonebranch handling.Line 368 returns an empty list when
status is None, but withdefault="active"on the query param, this case shouldn't occur in practice. However, this creates an inconsistent API behavior: omitting the parameter gives "active" branches, but there's no way to get all branches regardless of status.Consider whether you want to support fetching all branches (e.g., when
statusis explicitly omitted or set to a sentinel value like"all").Optional: Support fetching all statuses
@router.get("/{project_id}/git/branches", response_model=BranchListResponse) async def list_branches( project_id: int, - status: Optional[str] = Query(default="active", description="Filter by status (active, merged, abandoned)"), + status: Optional[str] = Query(default=None, description="Filter by status (active, merged, abandoned). Omit to get all."), db: Database = Depends(get_db), current_user: User = Depends(get_current_user), ):Then adjust the query logic to fetch all branches when
statusis None.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codeframe/ui/routers/git.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development
Use FastAPI framework for backend API development
Use SQLite with async support (aiosqlite) for database operations
Use tiktoken for token counting
Run linting with ruff check .
Import and use domain-specific repositories from codeframe/persistence/repositories/ for data access instead of direct database calls
Files:
codeframe/ui/routers/git.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
🧬 Code graph analysis (1)
codeframe/ui/routers/git.py (4)
codeframe/ui/dependencies.py (1)
get_db(14-29)codeframe/auth/dependencies.py (1)
get_current_user(17-121)codeframe/ui/websocket_broadcasts.py (2)
broadcast_branch_created(239-271)broadcast_commit_created(201-236)codeframe/persistence/repositories/git_repository.py (2)
get_branches_by_status(157-172)get_branch_by_name_and_issues(211-239)
⏰ 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). (5)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: opencode-review
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (6)
codeframe/ui/routers/git.py (6)
79-171: LGTM!The Pydantic models are well-structured with appropriate validation. The
strip_whitespacefield validator provides good input sanitization, and the response models cover all necessary fields.
179-238: LGTM!The helper functions cleanly separate concerns: project lookup, access control, and workflow manager instantiation. Error handling maps git exceptions to appropriate HTTP status codes.
393-443: LGTM!Good use of the
:pathconverter to handle branch names containing slashes. The single-query lookup viaget_branch_by_name_and_issuesis an efficient approach that avoids N+1 queries.
583-610: LGTM!The commit listing is well-implemented with proper pagination limits, defensive error handling for commit stats, and correct UTC timestamp normalization. Handling
git.BadNameseparately provides clearer error messages for invalid branch references.
673-691: LGTM!Good handling of edge cases for repositories without commits (empty HEAD). The fix to extract paths from
(path, stage)tuples inrepo.index.entries.keys()correctly addresses the GitPython API behavior.
291-296: No action required — branch name sanitization is already properly implemented.The
issue_titleis sanitized by the_sanitize_branch_namemethod (called on line 67 ofcreate_feature_branch), which removes special characters, converts to lowercase, collapses consecutive hyphens, and strips leading/trailing hyphens. The implementation is secure and tested.Likely an incorrect or invalid review comment.
…ndition Security fixes: - Add validate_file_paths() to prevent directory traversal attacks in commit endpoint (rejects absolute paths, '..' segments, and paths that escape the workspace via symlink resolution) - Handle branch creation race condition by returning 409 Conflict instead of 500 when concurrent requests create the same branch Changes: - Add os import for path validation - Add validate_file_paths() function with commonpath check - Call path validation before commit_task_changes() - Update create_branch to return 409 for "already exists" errors - Update test to expect 409 for duplicate branch creation - Add 3 security tests for path validation (absolute, traversal, escape) Test coverage: 50 tests passing
Code ReviewThis PR is in excellent condition with comprehensive test coverage (50 tests). The code follows FastAPI patterns and includes proper WebSocket broadcasts for real-time updates. ✅ All Previously Identified Issues Fixed
📝 Minor Suggestions (Non-Blocking)
🎯 Overall AssessmentExcellent test coverage with 50 tests covering:
The code follows FastAPI patterns, includes proper WebSocket broadcasts, and maintains clean separation of concerns. The path traversal validation in SummaryReady to merge ✅ All critical and most minor issues have been addressed. The remaining suggestions are optional polish items that don't block merge. The implementation is solid, well-tested, and follows project conventions. |
Code ReviewAfter reviewing the latest code changes and previous reviews, I'm impressed with the quality of this PR. ✅ All Critical Issues AddressedThe following issues from earlier reviews have been properly fixed:
📝 Minor Suggestions (Non-Blocking)
🎯 Overall AssessmentExcellent code quality with:
SummaryReady to merge ✅ All critical and security issues have been addressed. The remaining suggestions are optional polish items. The implementation is solid, well-tested, and follows project conventions. Reviewer Context: This review was performed after reviewing all previous comments on the PR to ensure consistency and avoid duplicate feedback. |
| # Get workflow manager | ||
| workflow_manager = get_git_workflow_manager(project, db) | ||
|
|
||
| # Validate file paths to prevent directory traversal attacks |
There was a problem hiding this comment.
workflow_manager.repo.working_tree_dir can be None (bare repo). Then validate_file_paths(...) resolves against CWD, weakening path checks. Consider guarding for a missing working tree and fail early.
| # Validate file paths to prevent directory traversal attacks | |
| # Ensure repo has working tree (non-bare) | |
| if not workflow_manager.repo.working_tree_dir: | |
| raise HTTPException(status_code=500, detail="Project workspace has no working tree") |
🚀 Want me to fix this? Reply ex: "fix it for me".
| except git.GitCommandError as e: | ||
| logger.error(f"Git error creating commit: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Git operation failed: {e}") |
There was a problem hiding this comment.
workflow_manager.repo.commit(commit_hash) can raise git.BadName/git.BadObject, which aren’t caught and will bubble unhandled. Consider catching them like git.GitCommandError so the endpoint returns a consistent 500.
- except git.GitCommandError as e:
+ except (git.GitCommandError, git.BadName, git.BadObject) as e:
logger.error(f"Git error creating commit: {e}")
raise HTTPException(status_code=500, detail=f"Git operation failed: {e}")🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review SummaryI've reviewed PR #282 and posted a comprehensive review comment. The PR is in excellent condition and ready to merge. ✅ All Critical Issues AddressedThe PR has successfully addressed all issues from previous reviews:
📝 Minor Suggestions (Non-Blocking)Only 3 minor suggestions were identified:
🎯 Overall Assessment
Result: Ready to merge ✅ All critical issues have been addressed. The remaining suggestions are optional polish items that don't block merge. |
Code ReviewAfter reviewing the latest code changes and previous reviews, I'm impressed with the quality of this PR. ✅ All Critical Issues AddressedThe following issues from earlier reviews have been properly fixed:
📝 Minor Suggestions (Non-Blocking)
🎯 Overall AssessmentExcellent code quality with:
SummaryReady to merge ✅ All critical and security issues have been addressed. The remaining suggestions are optional polish items. The implementation is solid, well-tested, and follows project conventions. Reviewer Context: This review was performed after reviewing all previous comments on the PR to ensure consistency and avoid duplicate feedback. |
Review CompletePosted comprehensive review to PR #282: #282 (comment) Overall Assessment: Ready to merge ✅ Summary of FindingsAll Critical Issues Fixed:
Minor Suggestions (Non-blocking):
Strengths:
The code follows FastAPI patterns, includes proper error handling, and maintains clean separation of concerns. All critical and security issues have been addressed. |
Summary
Endpoints
/api/projects/{id}/git/branches/api/projects/{id}/git/branches/api/projects/{id}/git/branches/{name}/api/projects/{id}/git/commit/api/projects/{id}/git/commits/api/projects/{id}/git/statusFiles Changed
codeframe/ui/routers/git.py- New router module (620 lines)codeframe/ui/server.py- Register git routercodeframe/ui/websocket_broadcasts.py- Addbroadcast_branch_createdhelpertests/api/test_git_api.py- Integration tests (764 lines)tests/api/conftest.py- Add cleanup for git_branches and changelog tablesUnblocks
This PR unblocks the following dependent tickets:
Test plan
Summary by CodeRabbit
New Features
Realtime
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.