Merge Sprint 4 Multi-Agent Coordination work into main - #9
Conversation
- Add symlink resolution with strict=True to prevent symlink attacks - Blacklist sensitive directories (.ssh, .aws, .gnupg, .config) - Improve exception handling to catch all path-related errors - Update docstring with comprehensive security policy Co-authored-by: Frank Bria <frankbria@users.noreply.github.com>
Project Schema Refactoring - Flexible Source Types & Deployment Modes
WalkthroughSprint 4.5 implements a comprehensive project schema refactoring, introducing deployment mode validation to restrict local filesystem access in hosted environments, enhancing workspace path safety with symlink resolution and sensitive directory blacklisting, and adding integration tests to validate the complete project creation flow with rollback capabilities. Changes
Sequence DiagramsequenceDiagram
actor User
participant API as create_project endpoint
participant DeployMode as Deployment Mode Guard
participant DB as Database
participant WorkspaceManager as Workspace Manager
participant SafetyCheck as Path Safety Check
User->>API: POST create_project (source_type, source_location)
API->>DeployMode: is_hosted_mode()?
alt Hosted Mode + local_path
DeployMode-->>API: true
API-->>User: HTTP 403 (local_path forbidden)
else Allowed Configuration
DeployMode-->>API: false OR non-local_path
API->>DB: Create project record
DB-->>API: project_id
API->>WorkspaceManager: create_workspace(workspace_path)
WorkspaceManager->>SafetyCheck: _is_safe_path(workspace_path)?
alt Path is Safe
SafetyCheck-->>WorkspaceManager: true
WorkspaceManager->>WorkspaceManager: resolve symlinks, setup .git
WorkspaceManager-->>API: workspace_path
API->>DB: Update project with workspace_path
DB-->>API: success
API-->>User: HTTP 200 (project created)
else Path is Unsafe
SafetyCheck-->>WorkspaceManager: false
WorkspaceManager-->>API: error
API->>DB: Delete project (rollback)
API-->>User: HTTP 400/403 (failed)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
tests/ui/test_deployment_mode.py (1)
12-67: Consider parametrizing fixtures to reduce duplication.The two fixtures
test_client_hostedandtest_client_self_hostedhave nearly identical code. Consider using pytest's@pytest.fixture(params=[...])to reduce duplication:@pytest.fixture(params=["hosted", "self_hosted"]) def test_client(request): """Test client with configurable deployment mode.""" temp_dir = Path(tempfile.mkdtemp()) db_path = temp_dir / "test.db" workspace_root = temp_dir / "workspaces" db = Database(db_path) db.initialize() app.state.db = db app.state.workspace_root = workspace_root from codeframe.workspace import WorkspaceManager app.state.workspace_manager = WorkspaceManager(workspace_root) os.environ["CODEFRAME_DEPLOYMENT_MODE"] = request.param client = TestClient(app) yield client, request.param del os.environ["CODEFRAME_DEPLOYMENT_MODE"] db.close() shutil.rmtree(temp_dir)However, the current explicit approach is also clear and acceptable, especially if you prefer test clarity over DRYness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
AGILE_SPRINTS.md(1 hunks)claudedocs/project-schema-test-results.md(1 hunks)codeframe/ui/server.py(3 hunks)codeframe/workspace/manager.py(2 hunks)tests/integration/test_project_creation_flow.py(1 hunks)tests/ui/test_deployment_mode.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T01:37:34.924Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-10-26T01:37:34.924Z
Learning: Applies to docs/**/AGILE_SPRINTS.md : Update AGILE_SPRINTS.md with each commit to reflect true codebase state
Applied to files:
AGILE_SPRINTS.md
🧬 Code graph analysis (3)
tests/ui/test_deployment_mode.py (2)
codeframe/persistence/database.py (2)
initialize(19-39)close(560-564)codeframe/workspace/manager.py (1)
WorkspaceManager(13-249)
tests/integration/test_project_creation_flow.py (3)
codeframe/persistence/database.py (5)
initialize(19-39)close(560-564)update_project(640-672)get_project(380-385)delete_project(674-682)codeframe/workspace/manager.py (2)
WorkspaceManager(13-249)create_workspace(25-70)codeframe/ui/models.py (1)
SourceType(11-16)
codeframe/ui/server.py (1)
codeframe/ui/models.py (1)
SourceType(11-16)
🪛 LanguageTool
claudedocs/project-schema-test-results.md
[duplication] ~92-~92: Possible typo: you repeated a word.
Context: ...pi/projectsnow requiresdescriptionfield -project_typefield renamed tosource_type` with new value...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~96-~96: Possible typo: you repeated a word.
Context: ...ace_path` (managed internally) ### For Tests - Tests using old database schema need updates ...
(ENGLISH_WORD_REPEAT_RULE)
🪛 Ruff (0.14.3)
tests/ui/test_deployment_mode.py
111-111: Probable insecure usage of temporary file or directory: "/tmp/test"
(S108)
tests/integration/test_project_creation_flow.py
90-90: Do not assert blind exception: Exception
(B017)
codeframe/workspace/manager.py
235-235: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (7)
codeframe/ui/server.py (2)
25-50: LGTM! Clean deployment mode implementation.The
DeploymentModeenum and helper functions provide a clear, maintainable way to distinguish between self-hosted and hosted SaaS deployments. The environment variable approach with a sensible default ensures backward compatibility.
289-294: LGTM! Effective security guard for hosted deployments.The hosted mode check is correctly placed before any database operations, preventing local filesystem access in hosted SaaS environments. The HTTP 403 response with a clear error message provides good user feedback.
tests/ui/test_deployment_mode.py (1)
102-116: Test coverage is appropriate.The test correctly verifies that self-hosted mode allows
local_pathsource type. The static analysis warning about/tmp/test(line 111) is a false positive—it's just a test value that won't actually be accessed since the test only verifies the HTTP status code.AGILE_SPRINTS.md (1)
1964-2047: Excellent sprint documentation.Sprint 4.5 documentation is comprehensive and follows the established format. It clearly tracks all implementation tasks, schema changes, and test results. The completion status and commit references make it easy to trace the work done.
Based on learnings
tests/integration/test_project_creation_flow.py (1)
11-30: Well-structured integration test fixture.The
integration_envfixture properly sets up all necessary components (database, workspace manager) and ensures comprehensive cleanup. Good use of a temporary directory to isolate tests.codeframe/workspace/manager.py (1)
206-238: Excellent security improvements to path validation.The enhanced
_is_safe_pathmethod significantly strengthens the security posture:
- Symlink resolution (
strict=True) prevents path traversal attacks by requiring the path to exist and resolving all symbolic links.- Sensitive directory blacklist prevents access to credential stores (
.ssh,.aws,.gnupg,.config).- Broader exception handling catches additional edge cases like permission errors and non-existent paths.
These changes are essential for the hosted deployment mode where filesystem access must be carefully controlled.
Note: The static analysis suggestion to move line 235 to an else block can be safely ignored—the current structure with early returns is clear and idiomatic.
claudedocs/project-schema-test-results.md (1)
1-131: Comprehensive test results documentation.This document provides excellent visibility into the project schema refactoring effort:
- Clear breakdown of all 21 new tests by category
- Comprehensive schema changes summary
- Breaking changes documentation for developers
- Security implications explained
The LanguageTool warnings about word repetition (lines 92, 96) are false positives from the markdown formatting and can be safely ignored.
| # Try to create workspace (should fail) | ||
| with pytest.raises(Exception): | ||
| workspace_manager.create_workspace( | ||
| project_id=project_id, | ||
| source_type=SourceType.GIT_REMOTE, | ||
| source_location="invalid-url" | ||
| ) |
There was a problem hiding this comment.
Use more specific exception type in assertion.
The test catches a blind Exception, which is too broad and could mask unexpected failures. Based on the WorkspaceManager implementation, invalid git URLs raise RuntimeError.
Apply this diff to make the assertion more specific:
- # Try to create workspace (should fail)
- with pytest.raises(Exception):
+ # Try to create workspace (should fail with RuntimeError)
+ with pytest.raises(RuntimeError, match="Failed to create workspace"):
workspace_manager.create_workspace(
project_id=project_id,
source_type=SourceType.GIT_REMOTE,
source_location="invalid-url"
)This makes the test more precise and helps catch unexpected exception types.
🧰 Tools
🪛 Ruff (0.14.3)
90-90: Do not assert blind exception: Exception
(B017)
🤖 Prompt for AI Agents
In tests/integration/test_project_creation_flow.py around lines 89 to 95, the
test currently uses pytest.raises(Exception) which is too broad; replace that
with pytest.raises(RuntimeError) so the test asserts the specific exception type
thrown for invalid git URLs by WorkspaceManager.
Merge Sprint 4 Multi-Agent Coordination work into main
1 participant
Summary
This PR merges the 004-multi-agent-coordination branch back into main, bringing in substantial completed work that was developed but not fully merged.
What's Included (26 commits, ~3,000+ lines)
Sprint 4 Multi-Agent Coordination - Complete Implementation
Bug Fixes & Improvements
Speckit Integration (~1,850 lines)
Project Schema Refactoring Enhancements
CI/CD Planning Documentation
Why This Merge?
The 004 branch contains completed Sprint 4 work including UI components, comprehensive documentation, bug fixes, and the entire Speckit integration framework. Main has evolved independently with CI/CD workflows implemented. This merge brings both together.
Conflicts Expected
tests/test_health_endpoint.py- Both branches added this file with different implementationsTesting
All tests were passing on the 004 branch before this PR. Will need to verify:
Related Issues: Sprint 4 completion (cf-f03 through cf-k01 now closed in beads)
Summary by CodeRabbit
New Features
Tests
Documentation