feat: Project schema refactoring with API endpoint integration

[frankbria]

Summary

This PR brings the project schema refactoring work from branch 004-multi-agent-coordination to main, with the latest addition being the API endpoint integration for workspace management.

Latest Changes (commit 7b9be32)

API Endpoint Update for Workspace Management

• Updated POST /api/projects endpoint to support new project schema
• Integrated WorkspaceManager for automatic workspace creation
• Added comprehensive error handling with rollback on failure
• Includes 4 passing tests in tests/ui/test_project_api.py

API Changes

• Accepts: name, description, source_type, source_location, source_branch
• Creates project record and workspace automatically
• Updates project with workspace_path and git_initialized status
• Cleanup on failure (deletes project if workspace creation fails)

Previous Work Included

This PR also includes the complete project schema refactoring (from PR #4):

Database Schema

• Refactored projects table with source types (git_remote, local_path, upload, empty)
• Added workspace_path, source_location, source_branch fields
• Implemented proper migration system (migration_002)
• Added backward compatibility with default parameters

Workspace Management

• Created WorkspaceManager for project sandboxing
• Comprehensive error handling (network errors, timeouts, invalid URLs)
• Security: Path validation restricted to $HOME directory
• Timeout protection: 30s for empty init, 5min for git clone

Models

• Replaced ProjectType with SourceType enum
• Updated ProjectCreateRequest with source configuration fields
• Added validation for git_remote source type

Testing

All tests passing:

• ✅ 4/4 API endpoint tests (tests/ui/test_project_api.py)
• ✅ 24/24 database and workspace tests
• ✅ Sprint 4 E2E tests

Sprint 4 Work Included

This PR also includes completed Sprint 4 work:

• Multi-agent coordination
• Thread-safe WebSocket broadcasts
• PRD & Task Dashboard Display (cf-26)
• Real database integration in Dashboard
• Comprehensive E2E testing

Deployment Notes

• Database migration will run automatically on first init
• Workspace directory: .codeframe/workspaces/
• Security: Only HOME directory paths allowed for local_path source

Summary by CodeRabbit

• New Features

  • Guided speckit command suite for spec→plan→tasks workflows, plus checklist, clarify, analyze, implement flows
  • Project workspace management with multi-source support (git/local/upload/empty) and new project creation flow
  • Real-time chat UI and backend chat/history endpoints with WebSocket updates

• Improvements

  • Dashboard enhancements: live activity, agent monitoring, task status updates
  • New health endpoints, CI/CD workflow docs, templates for specs/plans/tasks/checklists

• Bug Fixes & Tests

  • Thread-safe WebSocket broadcasts, ordering/pagination fixes, datetime handling; expanded test coverage and CI-focused tests

[coderabbitai]

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "feat: Project schema refactoring with API endpoint integration" is concrete and specific, referring to real, substantial changes present in the changeset: the refactored projects table with new fields (source_type, source_location, workspace_path, etc.), the new SourceType enum replacing ProjectType, database migration (migration_002), WorkspaceManager implementation, and updated POST /api/projects endpoint with workspace creation logic. However, the changeset also includes significant Sprint 4 multi-agent coordination work (lead agent fixes, worker agent WebSocket updates, chat API implementation, comprehensive E2E testing, and related documentation), which represents a substantial portion of the changes but is not reflected in the title. The title captures one major aspect of the PR but does not encompass the full scope of what is being merged.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 004-multi-agent-coordination

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Pull Request Review: Project Schema Refactoring with API Integration

Summary

This PR brings significant architectural improvements to the project management system with a well-designed workspace management system. Overall, this is high-quality work with good error handling and security considerations.

Strengths

1. Excellent Database Migration Design

• Proper use of can_apply() to prevent duplicate migrations
• Clear documentation of schema changes
• Appropriate handling of SQLite limitations
• Good logging throughout

2. Strong Workspace Manager Implementation

• Security-first approach with path validation restricted to HOME directory
• Comprehensive error categorization for git operations
• Robust error handling with proper cleanup on failure
• Timeout protection (30s for init, 5min for clone)

3. API Integration

• Clean integration of WorkspaceManager into FastAPI endpoints
• Proper transaction-like behavior with rollback on failure (lines 287-290 in server.py)
• Good use of Pydantic validation

4. Test Coverage

• Basic happy path tests for API endpoints
• Workspace manager unit tests cover key scenarios
• Tests use proper fixtures with cleanup

CRITICAL Issues

1. Migration is Destructive (codeframe/persistence/migrations/migration_002_refactor_projects_schema.py:89)

This migration drops all existing project data without backup. Add explicit dev-only guards or implement proper data migration for production.

2. Database Race Condition (codeframe/ui/server.py:260-290)

Project is created before workspace, creating a window where another request could see invalid state. Use database transactions or create workspace first.

3. Missing Git Installation Validation (codeframe/workspace/manager.py:96, 145)

FileNotFoundError is caught but only after attempting operations. Add startup validation in init.

HIGH Priority Issues

4. Incomplete Upload Implementation (codeframe/workspace/manager.py:229-238)

No error handling, upload_filename unused. Either remove UPLOAD from enum or raise NotImplementedError.

5. Path Security: Symlink Attack Vector (codeframe/workspace/manager.py:180)

The _is_safe_path() check doesn't verify all parent directories are safe. Resolve symlinks first and verify all parent paths.

6. Inconsistent Error Handling in Tests (tests/ui/test_project_api.py:54-68)

Test accepts both 201 and 500 status codes. Use mock or known-good test repository.

MEDIUM Priority Issues

7. Missing database index on workspace_path (should be unique)
8. No validation of git branch names (injection risk)
9. Hardcoded timeout values (make configurable)
10. Missing exception documentation in docstrings

LOW Priority / Nitpicks

11. Logging import missing in server.py:453
12. Magic number for git clone depth (make configurable)
13. Inconsistent default branch name

Security Assessment

Good Practices:

• Path validation restricts to HOME directory
• symlinks=False prevents symlink attacks
• Proper subprocess timeout prevents DoS
• Input validation via Pydantic models

Concerns:

• Path traversal risk (issue 5)
• Branch name injection (issue 8)
• No rate limiting for workspace creation
• No repository size limits

Testing Assessment

Coverage is adequate for MVP but needs expansion.

Missing scenarios: concurrent creation, large repo cloning, invalid URLs, workspace cleanup verification, migration rollback, permission denied, disk exhaustion.

Performance Considerations

1. Synchronous git operations block event loop - use asyncio.to_thread()
2. No workspace size limits
3. No cleanup strategy for old workspaces

Code Quality

Positives: Excellent docstrings, consistent error handling, good separation of concerns, type hints, follows repo conventions.

Improvements: Add workspace cleanup strategy, size monitoring, structured logging.

Recommendations Summary

Before Merge (Critical):

1. Fix migration data loss or add dev-only guards
2. Implement proper error for unimplemented UPLOAD
3. Add logger import to server.py
4. Make git operations async

High Priority (Should address):
5. Add git availability validation at startup
6. Improve path security (symlink handling)
7. Add branch name validation
8. Expand test coverage
9. Add database constraints

Overall Assessment

Rating: 4/5 stars

This PR demonstrates strong software engineering practices. Main concerns are around production-readiness (destructive migration, blocking operations) and test coverage gaps.

Recommendation: Approve with required changes - Address critical issues (1-4) before merging.

Great work on this refactoring! The workspace management system is well-designed and will provide a solid foundation.

---

Review generated by Claude Code

[frankbria]

@claude The migration of the project table should be a one-time drop since there is no migration necessary. If that's the case, then there's no concern. However, it should not drop each and every migration. The drop is a one-time change.

[claude]

Claude finished @frankbria's task —— View job

---

Migration Behavior Analysis

I've analyzed the migration code and can confirm your understanding is correct - the migration will only run once and the drop is a one-time operation.

Todo List

• Read migration_002 file
• Read database.py migration system
• Analyze safeguards against re-running
• Provide answer with code references

---

How the Migration System Prevents Re-runs

The migration system has two layers of protection to ensure migrations run only once:

1. Migration Tracking Table

The MigrationRunner maintains a schema_migrations table that tracks which migrations have been applied (codeframe/persistence/migrations/init.py:50-56):

CREATE TABLE IF NOT EXISTS schema_migrations (
    version TEXT PRIMARY KEY,
    description TEXT,
    applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP
)

2. Double-Check Before Execution

Before running any migration, the system performs two checks (codeframe/persistence/migrations/init.py:61-67):

1. Already Applied Check: _is_applied() queries the tracking table
2. Can Apply Check: Calls migration's can_apply() method

if self._is_applied(conn, migration.version):
    logger.info(f"Migration {migration.version} already applied, skipping")
    continue

if not migration.can_apply(conn):
    logger.warning(f"Migration {migration.version} cannot be applied, skipping")
    continue

3. Migration 002's Additional Safeguard

The migration itself also implements can_apply() to verify the new schema isn't already in place (migration_002_refactor_projects_schema.py:39-64):

def can_apply(self, conn: sqlite3.Connection) -> bool:
    # Check if new fields already exist
    table_sql = row[0]
    has_new_fields = "source_type" in table_sql and "workspace_path" in table_sql
    
    if has_new_fields:
        logger.info("New schema already applied, skipping migration")
        return False
    
    return True

Conclusion

Your concern from the previous review is not valid. The migration:

1. Runs exactly once - tracked in schema_migrations table
2. Checks before dropping - verifies new schema isn't already applied
3. Logs the operation - warns when dropping data (line 84-87)
4. Is appropriate for dev - documented as "one-time destructive migration for dev" (line 89)

The DROP TABLE IF EXISTS projects on line 90 will only execute the first time migration 002 runs. All subsequent database initializations will skip this migration because:

• It's recorded in schema_migrations table
• _is_applied() returns True
• The migration is skipped entirely

Recommendation

No changes needed. The migration system is working as designed with proper safeguards against re-execution.

---

[coderabbitai]

Actionable comments posted: 22

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

codeframe/ui/server.py (1)

1-21: Define a module logger and use it for diagnostics.

logger is used later but never defined, causing NameError on error paths. Initialize a logger at module scope.

+import logging
...
-from codeframe.workspace import WorkspaceManager
+from codeframe.workspace import WorkspaceManager
+
+# Module logger
+logger = logging.getLogger("codeframe.ui.server")

codeframe/persistence/database.py (1)

46-71: Schema migration is destructive and contradicts system design promises.

The migration exists but implements a destructive approach that conflicts with the documented migration system:

Issue: The README promises "Data preservation during schema changes," but migration_002_refactor_projects_schema.py drops the old projects table without migrating data (line 89: DROP TABLE IF EXISTS projects). This is explicitly marked "for development purposes only" but breaks the system's documented contract.

Missing backward-compatibility handling:

• Line 86 logs a warning about destructive operation, but no fallback exists
• Existing projects are destroyed with no data migration
• root_path → workspace_path mapping not implemented
• No default values provided for existing rows

Required fixes:

1. Either refactor migration_002 to preserve and transform existing project data (migrate root_path to workspace_path, set default descriptions), OR
2. Update the README to explicitly document that early migrations are destructive and data loss should be expected during development

Clarify the project's stance: is destructive schema evolution acceptable, or should migrations guarantee data preservation?

🧹 Nitpick comments (62)

claudedocs/sprint4-p0-final-status.md (1)

31-31: Add language identifiers to fenced code blocks for better rendering.

The static analysis tool flagged several fenced code blocks without language identifiers. While this is a minor documentation formatting issue, adding identifiers improves syntax highlighting and readability.

Apply these changes to add language identifiers:

 ### Primary Issue: Event Loop Deadlock
 
 **Discovered by parallel Python expert subagents**:
 
 1. **Problem**: Worker agents (`execute_task`) are synchronous but call `_broadcast_async()` which tries to schedule tasks on the main event loop
 2. **Deadlock Scenario**:
-   ```
+   ```python
    Main Event Loop
    └─ await run_in_executor(None, agent.execute_task, task)
        └─ Thread

 ### Core Implementation
-```
+```plaintext
 codeframe/agents/lead_agent.py          | 185 lines modified
 tests/test_multi_agent_integration.py   | 53 lines added/modified

```diff
 ### Documentation
-```
+```plaintext
 claudedocs/sprint4-troubleshooting-plan.md        | NEW - 850 lines
 claudedocs/sprint4-p0-implementation-summary.md   | NEW - 450 lines
 claudedocs/sprint4-p0-final-status.md            | NEW (this file)

Also applies to: 255-255, 261-261

</blockquote></details>
<details>
<summary>claudedocs/sprint4-test-analysis.md (1)</summary><blockquote>

`357-357`: **Add language identifier to fenced code block.**

The code block at line 357 is missing a language identifier. Based on the content (file paths), this should be marked as `plaintext`.



Apply this fix:

```diff
 **Test File**: `tests/test_simple_assignment.py` (new)
 
 **Tests to Add**:
-```python
+```plaintext
 class TestAgentAssignment:
     def test_assign_frontend_keywords()

claudedocs/github-workflows-design.md (1)

38-60: Add language identifiers to code blocks for better documentation quality.

Multiple fenced code blocks are missing language identifiers. Most appear to be ASCII diagrams or plaintext content and should be marked with plaintext or text for proper rendering.

Example fixes:

 **File**: `.github/workflows/ci-tests.yml`
 **Triggers**: Push to any branch, Pull Request to main/staging/development
 
-```
+```text
 ┌─────────────────────────────────────────────────────┐
 │              CI Testing Pipeline                     │

 ### Required Secrets
 
 #### For Staging Deployment
-```
+```plaintext
 STAGING_SSH_KEY         - Private SSH key for staging server access
 STAGING_HOST            - Staging server hostname/IP

This improves syntax highlighting and documentation readability in rendered views.

Also applies to: 75-95, 112-141, 158-191, 200-214, 217-220, 357-369

codeframe/agents/lead_agent.py (2)

89-103: Defensive Git initialization improves reliability.

The defensive initialization of GitWorkflowManager is a good improvement that prevents startup failures when:

• Project has no root_path set
• Path exists but is not a Git repository
• Path doesn't exist

The graceful fallback (setting git_workflow = None) allows the Lead Agent to function for non-Git features while disabling Git-dependent operations.

Minor improvement suggestion: Line 102 catches a broad Exception. Consider whether this is intentional (to handle any unexpected error) or if specific exceptions should be caught.

If you want to be more specific about error handling, consider this refinement:

         try:
             project_root = Path(project_root_str)
             self.git_workflow = GitWorkflowManager(project_root, db)
         except (git.InvalidGitRepositoryError, git.NoSuchPathError) as e:
             logger.warning(f"Could not initialize GitWorkflowManager: {e}. Git features will be disabled.")
-        except Exception as e:
+        except (PermissionError, OSError) as e:
             logger.warning(f"Unexpected error initializing GitWorkflowManager: {e}. Git features will be disabled.")

However, the current broad catch may be intentional to ensure Lead Agent startup never fails due to Git issues, which is a valid design choice.

---

892-894: Consider extracting long error messages for better maintainability.

The static analysis tool flagged long error messages directly in raise statements. While this is a low-priority style issue, extracting these messages can improve readability and make them easier to maintain.

Example refactor:

     def start_issue_work(self, issue_id: int) -> Dict[str, Any]:
         """..."""
         # Check if git workflow is available
         if not self.git_workflow:
-            raise RuntimeError("Git workflow is not available. Please ensure project has a valid git repository.")
+            error_msg = (
+                "Git workflow is not available. "
+                "Please ensure project has a valid git repository."
+            )
+            raise RuntimeError(error_msg)

This is optional and you may prefer the current inline approach for simplicity.

Also applies to: 948-950

.specify/scripts/bash/setup-plan.sh (1)

31-31: Quote the command substitution to prevent word splitting.

The unquoted command substitution in the eval statement could cause issues if the output contains unexpected whitespace or special characters.

Apply this diff:

-eval $(get_feature_paths)
+eval "$(get_feature_paths)"

Based on static analysis.

tests/ui/test_models.py (1)

45-56: Consider more precise error field checking.

The string matching on line 56 could be fragile if Pydantic's error message format changes. Consider checking the error field name more precisely.

Apply this diff for a more robust assertion:

-    errors = exc_info.value.errors()
-    assert any("source_location" in str(e) for e in errors)
+    errors = exc_info.value.errors()
+    assert any(e.get("loc") == ("source_location",) or "source_location" in str(e.get("msg", "")) for e in errors)

Alternatively, if you want even more precision:

-    errors = exc_info.value.errors()
-    assert any("source_location" in str(e) for e in errors)
+    errors = exc_info.value.errors()
+    error_fields = [e.get("loc", ()) for e in errors]
+    assert ("source_location",) in error_fields or any("source_location" in str(e) for e in errors)

tests/ui/test_project_api.py (1)

54-68: Consider mocking git operations for more reliable testing.

Accepting both 201 and 500 status codes (line 68) is pragmatic but could mask actual bugs. The test can't distinguish between expected git failures and unexpected errors.

Consider one of these approaches:

Option 1: Mock the workspace creation

from unittest.mock import patch

def test_create_project_git_remote(test_client):
    """Test creating project from git repository."""
    with patch('codeframe.workspace.manager.WorkspaceManager.create_workspace') as mock_create:
        mock_create.return_value = Path("/fake/workspace")
        
        response = test_client.post(
            "/api/projects",
            json={
                "name": "Git Project",
                "description": "From git",
                "source_type": "git_remote",
                "source_location": "https://github.com/user/repo.git"
            }
        )
        
        assert response.status_code == 201
        assert mock_create.called

Option 2: Use a real public repository

def test_create_project_git_remote(test_client):
    """Test creating project from git repository."""
    response = test_client.post(
        "/api/projects",
        json={
            "name": "Git Project",
            "description": "From git",
            "source_type": "git_remote",
            "source_location": "https://github.com/octocat/Hello-World.git"  # Real public repo
        }
    )
    
    assert response.status_code == 201

.serena/memories/cf-14.2_chat_interface_implementation.md (1)

90-101: Add WS schema version note.

Consider documenting a version field (e.g., v: 1) for extensibility of chat_message.

.claude/commands/speckit.tasks.md (1)

64-95: Checklist format: enforce file path examples consistently.

Add one example that shows an absolute path (as Step 1 requires absolute paths).

claudedocs/sprint4-gui-checklist.md (2)

3-3: Replace bare URLs with Markdown links.

Address MD034 by wrapping URLs in . Example: Frontend.

Also applies to: 17-20, 410-412

---

377-381: Add language to shell code fences.

Use ```bash fences for curl blocks to satisfy MD040 and improve readability.

test_standalone.py (2)

50-53: Optional: use absolute git path or preflight check.

To avoid S607, ensure git is present: use shutil.which("git") or document the PATH assumption.

---

103-107: Minor: remove extraneous f-strings and narrow exception.

• f-string without placeholders → plain string.
• Prefer catching asyncio.TimeoutError and Exception separately with logging.

-                print(f"\n✅ EXECUTION COMPLETE!")
+                print("\n✅ EXECUTION COMPLETE!")
@@
-            except asyncio.TimeoutError:
-                print(f"\n❌ TIMEOUT after 5 seconds!")
+            except asyncio.TimeoutError:
+                print("\n❌ TIMEOUT after 5 seconds!")
                 print("The hang occurred inside start_multi_agent_execution")
-            except Exception as e:
+            except Exception as e:
                 print(f"\n❌ ERROR: {type(e).__name__}: {e}")

Also applies to: 108-112

claudedocs/sprint4-final-e2e-status.md (3)

61-61: Use a heading instead of bold for section title.

Change bold line to a heading to satisfy MD036.

---

290-297: Add language to fenced code blocks.

Mark commit file lists as ```text to satisfy MD040.

-```
+```text
 codeframe/agents/backend_worker_agent.py
 ...

---

297-306: Same: add language to fenced block.

Use ```text.

specs/004-multi-agent-coordination/checklists/release-gate.md (1)

21-22: Nit: clarify “updates timing” phrasing.

Consider “Can real-time dashboard update latency (<500ms) be objectively measured?” for clarity.

codeframe/persistence/migrations/migration_002_refactor_projects_schema.py (1)

124-155: Rollback should also be transactional.

Wrap rollback in a transaction to avoid partial states on failure.

-        cursor = conn.cursor()
+        cursor = conn.cursor()
+        conn.execute("BEGIN IMMEDIATE")
@@
-        conn.commit()
+        conn.commit()

claudedocs/sprint4-e2e-test-results.md (1)

110-114: Consider adding language specifiers to code blocks for better rendering.

The code blocks at lines 110, 154, and 180 lack language specifiers. Adding appropriate identifiers (text or log) would improve markdown rendering and readability.

Apply these changes:

-**Result**: ✅ **PASS**
-```
-✓ Compiled successfully
+**Result**: ✅ **PASS**
+```text
+✓ Compiled successfully

-**Backend Logs**:
-```
-INFO:     Started server process [989859]
+**Backend Logs**:
+```log
+INFO:     Started server process [989859]

-**Frontend Logs**:
-```
-⚠ You are using a non-standard "NODE_ENV" value in your environment.
+**Frontend Logs**:
+```log
+⚠ You are using a non-standard "NODE_ENV" value in your environment.

Also applies to: 154-160, 180-183

claudedocs/sprint4-p0-HANDOFF.md (1)

110-127: Consider adding language specifiers to code blocks for consistency.

The code blocks in the Files Modified section lack language specifiers. Adding text or appropriate language identifiers would improve consistency with other documentation and markdown rendering.

Apply this change:

 ### Core Implementation
-```
+```text
 codeframe/agents/lead_agent.py

 ### Documentation
-```
+```text
 claudedocs/sprint4-troubleshooting-plan.md         (NEW - 850 lines)

docs/plans/2025-10-27-project-schema-implementation.md (1)

773-803: Consider using parameterized queries for update_project to prevent SQL injection.

Lines 773-791 build dynamic SQL using f-strings for the update_project method. While the current implementation uses parameter placeholders (?) for values, the dynamic field names in the SET clause could be a security concern if field names are ever derived from untrusted input.

Consider adding field name validation:

 async def update_project(self, project_id: int, **kwargs) -> None:
     """Update project fields."""
     if not kwargs:
         return
 
+    # Validate field names against allowed columns
+    allowed_fields = {'workspace_path', 'git_initialized', 'description', 
+                     'source_type', 'source_location', 'source_branch', 
+                     'current_commit', 'status', 'phase'}
+    invalid_fields = set(kwargs.keys()) - allowed_fields
+    if invalid_fields:
+        raise ValueError(f"Invalid field names: {invalid_fields}")
+
     set_clause = ", ".join(f"{key} = ?" for key in kwargs.keys())
     values = list(kwargs.values()) + [project_id]

.specify/scripts/bash/check-prerequisites.sh (1)

143-149: Consider using jq for more robust JSON array construction.

Lines 143-149 build the JSON array using bash string manipulation. This works but could be more robust and maintainable using jq if available.

Alternative implementation with jq (if available):

# Build JSON array of documents
if command -v jq >/dev/null 2>&1; then
    json_docs=$(printf '%s\n' "${docs[@]}" | jq -R . | jq -s .)
else
    # Fallback to current implementation
    if [[ ${#docs[@]} -eq 0 ]]; then
        json_docs="[]"
    else
        json_docs=$(printf '"%s",' "${docs[@]}")
        json_docs="[${json_docs%,}]"
    fi
fi

claudedocs/sprint4-troubleshooting-plan.md (2)

57-57: Use proper headings instead of bold emphasis.

markdownlint (MD036) flags these lines. Convert bolded pseudo-headings to real headings (e.g., "### Solution", "### Step 1").

Also applies to: 99-99, 114-114, 126-126

---

374-374: Minor wording/hyphenation nits.

• “harder without full stack traces” → fine; optionally hyphenate compound modifiers when preceding nouns.
• “low risk, high value” → consider “low-risk, high‑value” for consistency.

Also applies to: 768-768

codeframe/ui/server.py (2)

388-395: Scope agents to project_id.

Endpoint path is project‑scoped but returns all agents. Filter to the project to avoid confusing UI.

---

278-285: Set git_initialized based on actual repo presence.

You unconditionally set git_initialized=True. Consider verifying workspace_path/.git exists (local_path may not init git).

codeframe/ui/models.py (4)

7-7: Remove unused import.

field_validator isn’t used. Safe to drop.

---

26-33: Make source_type non‑optional; it already has a default.

Typing it Optional allows null, which conflicts with your validator. Use a strict enum type.

-    source_type: Optional[SourceType] = Field(default=SourceType.EMPTY, description="Source type for project initialization")
+    source_type: SourceType = Field(default=SourceType.EMPTY, description="Source type for project initialization")

---

22-30: Add validation aliases to accept legacy keys (project_name, project_type).

Keeps seed scripts and older clients working during transition. Pydantic v2 supports validation_alias.

-from pydantic import BaseModel, Field, field_validator, model_validator, ConfigDict
+from pydantic import BaseModel, Field, model_validator, ConfigDict, AliasChoices
@@
-    name: str = Field(..., min_length=1, max_length=100, description="Project name")
+    name: str = Field(
+        ...,
+        min_length=1,
+        max_length=100,
+        description="Project name",
+        validation_alias=AliasChoices("name", "project_name"),
+    )
@@
-    source_type: SourceType = Field(default=SourceType.EMPTY, description="Source type for project initialization")
+    source_type: SourceType = Field(
+        default=SourceType.EMPTY,
+        description="Source type for project initialization",
+        validation_alias=AliasChoices("source_type", "project_type"),
+    )

---

34-39: Use ValidationError semantics but keep messages concise.

Pydantic will wrap ValueError; consider a shorter message to avoid long client‑visible errors.

.specify/scripts/bash/create-new-feature.sh (4)

3-3: Harden shell options.

Add -u and -o pipefail for safer failure semantics.

-set -e
+set -euo pipefail

---

106-121: SC2155: declare and assign separately inside function; also avoid UUOC.

Split assignments to keep return codes intact.

-    local clean_name=$(echo "$description" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9]/ /g')
+    local clean_name
+    clean_name=$(echo "$description" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9]/ /g')

---

177-181: Guard branch creation if it already exists; prefer git switch -c.

Avoid failing on reruns.

-if [ "$HAS_GIT" = true ]; then
-    git checkout -b "$BRANCH_NAME"
+if [ "$HAS_GIT" = true ]; then
+    if git rev-parse --verify --quiet "$BRANCH_NAME" >/dev/null; then
+        git switch "$BRANCH_NAME"
+    else
+        git switch -c "$BRANCH_NAME"
+    fi

---

190-200: Note: export only affects subshell; consider printing eval helper.

To persist in caller’s shell, echo an export line the user can eval.

.claude/commands/speckit.checklist.md (2)

98-98: Reduce “necessary requirements” wordiness.

Consider “all required” / “all needed” for brevity.

Also applies to: 105-105

---

236-236: Hyphenate compound modifier.

Use “rate‑limiting requirements” (hyphenated).

.specify/scripts/bash/update-agent-context.sh (3)

56-56: Quote the command substitution to prevent word splitting.

The eval statement should quote the command substitution to handle paths with spaces correctly.

Apply this diff:

-eval $(get_feature_paths)
+eval "$(get_feature_paths)"

Based on static analysis hints.

---

301-303: Separate declaration and assignment to avoid masking return values.

Combining local declaration with command substitution assignment can mask command failures. Separate them to ensure errors are caught properly.

Apply this diff:

-    local escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\[\.*^$()+{}|]/\\&/g')
-    local escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\[\.*^$()+{}|]/\\&/g')
-    local escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\[\.*^$()+{}|]/\\&/g')
+    local escaped_lang
+    local escaped_framework
+    local escaped_branch
+    escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\[\.*^$()+{}|]/\\&/g')
+    escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\[\.*^$()+{}|]/\\&/g')
+    escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\[\.*^$()+{}|]/\\&/g')

Based on static analysis hints.

---

433-433: Remove unused variable.

The changes_entries_added variable is set but never used in the function logic. Consider removing it unless you plan to use it for validation.

Apply this diff:

             if [[ -n "$new_change_entry" ]]; then
                 echo "$new_change_entry" >> "$temp_file"
             fi
             in_changes_section=true
-            changes_entries_added=true
             continue

Based on static analysis hints.

claudedocs/sprint4-p0-COMPLETE-STATUS.md (1)

48-48: Optional: Use consistent strong emphasis style.

For consistency with Markdown conventions, consider using asterisks instead of underscores for strong emphasis.

Change __init__ to **init** for consistent strong emphasis style throughout the document.

Based on static analysis hints.

claudedocs/sprint4-p0-implementation-summary.md (1)

34-34: Fix heading formatting.

The ATX-style heading is missing a space after the hash marks.

Apply this diff:

-###2. Comprehensive Logging
+### 2. Comprehensive Logging

Based on static analysis hints.

codeframe/persistence/database.py (1)

674-682: New methods provide essential functionality.

Three new methods added:

1. delete_project - Enables cleanup on workspace creation failure (used in server.py error handling)
2. get_blockers - Retrieves unresolved blockers with task info for UI
3. get_recent_activity - Formats changelog entries for activity feed

The implementations are clean with proper SQL joins and formatting. Minor improvement: add strict=True to zip() calls on lines 1759 and 1792 to catch length mismatches (Python 3.10+).

Apply this diff to add strict parameter to zip():

-        return [dict(zip(columns, row)) for row in cursor.fetchall()]
+        return [dict(zip(columns, row, strict=True)) for row in cursor.fetchall()]

Based on static analysis hints.

Also applies to: 1732-1802

codeframe/workspace/manager.py (2)

72-97: Improve error logging and remove unused variable.

The _init_empty method correctly initializes an empty Git repository with appropriate timeout. However:

1. Unused variable: The result variable is assigned but never used (line 80)
2. Error logging: Use logging.exception instead of logging.error to include stack traces for debugging (lines 90, 93, 96)

Apply these improvements:

         try:
-            result = subprocess.run(
+            subprocess.run(
                 ["git", "init"],
                 cwd=workspace_path,
                 check=True,
                 capture_output=True,
                 timeout=30,
                 text=True
             )
             logger.info(f"Initialized empty git repository at {workspace_path}")
         except subprocess.CalledProcessError as e:
-            logger.error(f"Git init failed: {e.stderr}")
+            logger.exception(f"Git init failed: {e.stderr}")
             raise RuntimeError(f"Failed to initialize git repository: {e.stderr}") from e
         except subprocess.TimeoutExpired as e:
-            logger.error("Git init timed out")
+            logger.exception("Git init timed out")
             raise RuntimeError("Git initialization timed out after 30 seconds") from e
         except FileNotFoundError as e:
-            logger.error("Git command not found")
+            logger.exception("Git command not found")
             raise RuntimeError("Git is not installed or not in PATH") from e

Based on static analysis hints.

---

99-145: Excellent error categorization for Git operations.

The _init_from_git method provides comprehensive error handling with specific detection for:

• Network errors
• Repository not found
• Branch not found
• Authentication failures

The 5-minute timeout and shallow clone (--depth 1) are appropriate for production use.

Apply the same improvements as _init_empty: remove unused result variable (line 115) and use logging.exception instead of logging.error for better debugging (lines 126, 129, 132, 135, 138, 141, 144).

.specify/scripts/bash/common.sh (7)

10-11: Avoid SC2155 masking and improve path resolution.

Declare then assign; also prefer realpath and safer cd fallback.

-        local script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-        (cd "$script_dir/../../.." && pwd)
+        local script_dir
+        script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+        local root_candidate
+        root_candidate="$(cd "$script_dir/../../.." && pwd)"
+        # Normalize symlinks; fall back to candidate if realpath unavailable
+        if command -v realpath >/dev/null 2>&1; then
+            realpath "$root_candidate"
+        else
+            echo "$root_candidate"
+        fi

Based on ShellCheck hints.

---

24-31: Reduce duplicate git calls; split declarations (SC2155).

Capture once, then echo; also split local declarations to avoid SC2155.

-    if git rev-parse --abbrev-ref HEAD >/dev/null 2>&1; then
-        git rev-parse --abbrev-ref HEAD
+    local git_branch
+    git_branch="$(git rev-parse --abbrev-ref HEAD 2>/dev/null)" || git_branch=""
+    if [[ -n "$git_branch" ]]; then
+        echo "$git_branch"
         return
     fi
-
-    local repo_root=$(get_repo_root)
-    local specs_dir="$repo_root/specs"
+    local repo_root
+    repo_root=$(get_repo_root)
+    local specs_dir
+    specs_dir="$repo_root/specs"

Based on ShellCheck hints.

---

37-49: Safer latest feature detection and readability.

Enable nullglob locally to avoid literal globs; split declarations (SC2155).

-        local latest_feature=""
-        local highest=0
+        local latest_feature=""
+        local highest=0
+        local _saved_shopt=$(shopt -p nullglob)
+        shopt -s nullglob
         for dir in "$specs_dir"/*; do
             if [[ -d "$dir" ]]; then
-                local dirname=$(basename "$dir")
+                local dirname
+                dirname=$(basename "$dir")
                 if [[ "$dirname" =~ ^([0-9]{3})- ]]; then
                     local number=${BASH_REMATCH[1]}
                     number=$((10#$number))
                     if [[ "$number" -gt "$highest" ]]; then
                         highest=$number
                         latest_feature=$dirname
                     fi
                 fi
             fi
         done
+        # restore
+        eval "$_saved_shopt"

Based on learnings.

---

61-63: Slightly faster repo check.

Use --is-inside-work-tree for intent; keep behavior.

-has_git() {
-    git rev-parse --show-toplevel >/dev/null 2>&1
-}
+has_git() {
+    git rev-parse --is-inside-work-tree >/dev/null 2>&1
+}
``` <!-- review_comment_end -->

---

`88-125`: **Handle no-match globs and multiple matches better.**

Guard with nullglob and avoid echoing a literal non-existent path inadvertently.


```diff
-    # Search for directories in specs/ that start with this prefix
-    local matches=()
-    if [[ -d "$specs_dir" ]]; then
-        for dir in "$specs_dir"/"$prefix"-*; do
+    # Search for directories in specs/ that start with this prefix
+    local matches=()
+    if [[ -d "$specs_dir" ]]; then
+        local _saved_shopt=$(shopt -p nullglob)
+        shopt -s nullglob
+        for dir in "$specs_dir"/"$prefix"-*; do
             if [[ -d "$dir" ]]; then
                 matches+=("$(basename "$dir")")
             fi
         done
+        eval "$_saved_shopt"
     fi

Based on ShellCheck-friendly patterns.

---

127-152: Avoid SC2155; ensure quoted heredoc values are robust.

Split declarations; also consider exporting via printf for easy eval by callers.

-    local repo_root=$(get_repo_root)
-    local current_branch=$(get_current_branch)
-    local has_git_repo="false"
+    local repo_root
+    repo_root=$(get_repo_root)
+    local current_branch
+    current_branch=$(get_current_branch)
+    local has_git_repo="false"
@@
-    local feature_dir=$(find_feature_dir_by_prefix "$repo_root" "$current_branch")
+    local feature_dir
+    feature_dir=$(find_feature_dir_by_prefix "$repo_root" "$current_branch")

Optional: switch to NUL-delimited output for robustness if paths can contain newlines.

---

154-156: Robust “non-empty directory” check; avoid parsing ls.

Use find for correctness and performance; also quote substitution.

-check_file() { [[ -f "$1" ]] && echo "  ✓ $2" || echo "  ✗ $2"; }
-check_dir() { [[ -d "$1" && -n $(ls -A "$1" 2>/dev/null) ]] && echo "  ✓ $2" || echo "  ✗ $2"; }
+check_file() { [[ -f "$1" ]] && echo "  ✓ $2" || echo "  ✗ $2"; }
+check_dir() {
+  if [[ -d "$1" ]] && find "$1" -mindepth 1 -print -quit >/dev/null 2>&1; then
+    echo "  ✓ $2"
+  else
+    echo "  ✗ $2"
+  fi
+}

Based on ShellCheck guidance.

docs/plans/2025-10-27-project-schema-refactoring.md (4)

303-315: Add language to fenced code block (MD040).

Specify a language or use text for pseudo-flow.

-```
+```text
 1. Create project (name, description, source)
    ↓
 2. Socratic conversation handles BOTH:
    - Discovery questions: "What's your tech stack? Monorepo structure?"
    - Requirements questions: "What features? Who are the users?"
    ↓
 3. Generate initial PRD from conversation
    ↓
 4. Populate config.discovery from same conversation
    ↓
 5. Transition to Planning phase

---

`322-334`: **Add language to fenced code block (MD040).**

Same as above.


```diff
-```
+```text
 1. Project exists (discovery already complete)
    ↓
 2. Trigger PRD regeneration
    ↓
 3. Use existing config.discovery context
    ↓
 4. Ask ONLY requirements questions (skip tech stack)
    ↓
 5. Generate PRD v2
    ↓
 6. Update config.prd.current_version

---

`491-491`: **Hyphenate compound adjective.**

Use “Backward-compatible” to satisfy grammar/lint.


```diff
-- ✅ Supports both deployment modes
-- ✅ Enables progressive discovery
-...
-- ✅ Tech stack evolution tracked
-- ✅ Backward compatible additions possible
+- ✅ Supports both deployment modes
+- ✅ Enables progressive discovery
+...
+- ✅ Tech stack evolution tracked
+- ✅ Backward-compatible additions possible

---

504-504: Prefer a heading instead of emphasis (MD036).

Promote to a heading for structure.

-**End of Design Document**
+## End of Design Document

codeframe/ui/server.py,cover (6)

105-166: Status persistence should be consistent (enum vs string); also actually log errors.

db.update_project(..., {"status": ProjectStatus.RUNNING}) may store a non-serializable enum while other code compares .value. Persist the string value and log exceptions.

-        db.update_project(project_id, {"status": ProjectStatus.RUNNING})
+        db.update_project(
+            project_id,
+            {"status": getattr(ProjectStatus, "RUNNING").value if hasattr(ProjectStatus, "RUNNING") else "running"}
+        )
@@
-    except Exception as e:
-        # Log error but let it propagate
-        raise
+    except Exception as e:
+        # Log error but let it propagate
+        import logging
+        logging.getLogger(__name__).exception("start_agent failed for project_id=%s", project_id)
+        raise

Verify how status is stored in the DB (string vs enum) so comparisons like Line 291 work consistently.

---

282-299: RUNNING check mixes enum/string; normalize.

Compare against the same representation you store (string recommended).

-    if project["status"] == ProjectStatus.RUNNING.value:
+    expected = getattr(ProjectStatus, "RUNNING").value if hasattr(ProjectStatus, "RUNNING") else "running"
+    if project["status"] == expected:
         return JSONResponse(

Confirm what Database.get_project returns for status.

---

340-347: Filter agents by project_id or remove param.

Endpoint ignores project_id; either filter or adjust route.

-    agents = app.state.db.list_agents()
-    return {"agents": agents}
+    agents = app.state.db.list_agents(project_id=project_id)
+    return {"agents": agents}

Ensure Database.list_agents supports filtering; otherwise add it.

---

593-617: WebSocket: handle JSON decode errors and unsubscribe; prune on failure.

Add basic validation to avoid 500s on invalid messages.

-            data = await websocket.receive_text()
-            message = json.loads(data)
+            data = await websocket.receive_text()
+            try:
+                message = json.loads(data)
+            except json.JSONDecodeError:
+                await websocket.send_json({"type": "error", "detail": "invalid_json"})
+                continue
@@
-            elif message.get("type") == "subscribe":
+            elif message.get("type") == "subscribe":
                 # Subscribe to specific project updates
                 project_id = message.get("project_id")
                 # TODO: Track subscriptions
                 await websocket.send_json({
                     "type": "subscribed",
                     "project_id": project_id
                 })
+            elif message.get("type") == "unsubscribe":
+                await websocket.send_json({"type": "unsubscribed"})

---

22-37: Start/stop periodic broadcaster in lifespan.

broadcast_updates() is never scheduled. Start it on startup and cancel on shutdown.

 async def lifespan(app: FastAPI):
     """Manage application lifespan - startup and shutdown."""
@@
-    app.state.db.initialize()
+    app.state.db.initialize()
+    # background updates
+    app.state._updates_task = asyncio.create_task(broadcast_updates())
@@
-    if hasattr(app.state, "db") and app.state.db:
+    if hasattr(app.state, "_updates_task"):
+        app.state._updates_task.cancel()
+        with contextlib.suppress(Exception):
+            await app.state._updates_task
+    if hasattr(app.state, "db") and app.state.db:
         app.state.db.close()

Import contextlib where needed.

---

47-53: Make CORS origins configurable for non-dev.

Read from env with a sane default to reduce risk in production.

 app.add_middleware(
     CORSMiddleware,
-    allow_origins=["http://localhost:3000", "http://localhost:5173"],  # React/Vite dev servers
+    allow_origins=os.environ.get("CORS_ALLOW_ORIGINS", "http://localhost:3000,http://localhost:5173").split(","),
     allow_credentials=True,
     allow_methods=["*"],
     allow_headers=["*"],
 )

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Update document dates and scope to match current PR context.

The document is dated 2025-10-25, but PR #5 was created on 2025-10-28. Additionally, the branch reference and content should reflect the current PR's actual work (project schema + workspace API integration) rather than prior Sprint 4 multi-agent coordination content.

🤖 Prompt for AI Agents

In claudedocs/SPRINT_4_FINAL_STATUS.md around lines 3 to 4, update the metadata
and scope to match the current PR: change the Date from 2025-10-25 to
2025-10-28, change the Branch from `004-multi-agent-coordination` to the branch
for PR #5 (the branch used for project schema + workspace API integration), and
revise the document header/body to reflect the actual work (project schema
changes and workspace API integration) instead of Sprint 4 multi-agent
coordination content so the file accurately describes the PR's scope and
changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Correct PR reference number.

Line 5 references PR #3, but this is PR #5 per the PR objectives.

-**PR**: #3 (MERGED)
+**PR**: #5

📝 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.

Suggested change

	**PR**: #3 (MERGED)
	**PR**: #5

🤖 Prompt for AI Agents

In claudedocs/SPRINT_4_FINAL_STATUS.md around line 5, the PR reference is
incorrect — it shows "PR: #3" but should be "PR: #5" per the PR objectives;
update the line to reference PR #5 (and verify surrounding references or
changelog entries for consistency) so the document correctly reflects the merged
PR number.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify all broadcast call sites use correct function signatures.

All call sites have been updated to use _broadcast_async, but I want to verify the arguments match the expected broadcast function signatures. The pattern looks consistent, but let's confirm the broadcast functions accept the provided arguments.

Run this verification script to check broadcast function signatures:

Also applies to: 517-528, 536-544, 675-683, 727-744, 760-769, 879-887

---

🏁 Script executed:

#!/bin/bash
# Verify broadcast function signatures match call sites in backend_worker_agent.py

echo "=== Checking broadcast_task_status signature ==="
ast-grep --pattern $'async def broadcast_task_status($$$) -> None:
  $$$'

echo ""
echo "=== Checking broadcast_test_result signature ==="
ast-grep --pattern $'async def broadcast_test_result($$$) -> None:
  $$$'

echo ""
echo "=== Checking broadcast_activity_update signature ==="
ast-grep --pattern $'async def broadcast_activity_update($$$) -> None:
  $$$'

echo ""
echo "=== Checking broadcast_correction_attempt signature ==="
ast-grep --pattern $'async def broadcast_correction_attempt($$$) -> None:
  $$$'

Length of output: 11851

---

🏁 Script executed:

#!/bin/bash
# Extract _broadcast_async call sites from backend_worker_agent.py for verification

echo "=== Call site at lines 446-453 ==="
sed -n '446,453p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 517-528 ==="
sed -n '517,528p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 536-544 ==="
sed -n '536,544p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 675-683 ==="
sed -n '675,683p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 727-744 ==="
sed -n '727,744p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 760-769 ==="
sed -n '760,769p' codeframe/agents/backend_worker_agent.py

echo ""
echo "=== Call site at lines 879-887 ==="
sed -n '879,887p' codeframe/agents/backend_worker_agent.py

Length of output: 3273

---

Fix broadcast call site argument order for broadcast_activity_update and broadcast_test_result.

The verification found critical signature mismatches:

1. Lines 536-544, 727-744 (second call), 879-887: broadcast_activity_update calls pass an extra positional argument "backend-worker" in the wrong position. The signature expects (manager, project_id, activity_type, message_text, task_id=None, agent_id=None), but these calls pass six positional arguments before the task_id keyword argument. Remove the "backend-worker" positional arguments; they don't map to any parameter.

2. Lines 517-528: broadcast_test_result passes test_result.total to the skipped parameter (8th positional arg). The signature expects (manager, project_id, task_id, status, passed, failed, errors, skipped, duration). Verify whether test_result.total represents skipped tests or if a different field should be used.

🤖 Prompt for AI Agents

In codeframe/agents/backend_worker_agent.py (lines ~446-453, 517-528, 536-544,
727-744, 879-887) the broadcast call sites have signature mismatches: remove the
extra positional "backend-worker" argument from all broadcast_activity_update
calls so they match (manager, project_id, activity_type, message_text,
task_id=None, agent_id=None) — either pass activity_type and message_text as
positional 3rd/4th args and supply task_id/agent_id as keywords or use explicit
keyword names; and for the broadcast_test_result call at ~517-528, stop passing
test_result.total into the skipped positional slot — instead pass the correct
skipped value (use test_result.skipped if that field exists, or compute skipped
= test_result.total - (test_result.passed + test_result.failed +
test_result.errors)) and pass it as the skipped argument (prefer named argument
skipped=...) so the call matches the signature (manager, project_id, task_id,
status, passed, failed, errors, skipped, duration).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Use new projects schema fields and stop writing root_path.

• create_project now requires a description; pass proper fields and set workspace_path.
• root_path column has been dropped; use workspace_path.

-        project_id = db.create_project("test-project", ProjectStatus.ACTIVE)
-        db.update_project(project_id, {"root_path": tmpdir})
+        project_id = db.create_project(
+            name="test-project",
+            description="Standalone test project",
+            source_type="local_path",
+            source_location=tmpdir,
+            source_branch="main",
+            workspace_path=tmpdir,
+        )
+        # Optionally mark active after init:
+        db.update_project(project_id, {"status": "active"})

📝 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.

Suggested change

	print("\n4. Creating project...")
	project_id = db.create_project("test-project", ProjectStatus.ACTIVE)
	db.update_project(project_id, {"root_path": tmpdir})
	print(f"✅ Project created: {project_id}")
	print("\n4. Creating project...")
	project_id = db.create_project(
	name="test-project",
	description="Standalone test project",
	source_type="local_path",
	source_location=tmpdir,
	source_branch="main",
	workspace_path=tmpdir,
	)
	# Optionally mark active after init:
	db.update_project(project_id, {"status": "active"})
	print(f"✅ Project created: {project_id}")

🤖 Prompt for AI Agents

In test_standalone.py around lines 56 to 59, update project creation to use the
new schema: call db.create_project with the required fields including a
description (e.g., "test project description") and any other required args
instead of only name and status, and then update_project should set
workspace_path rather than root_path (remove any writes to root_path). Ensure
the create_project invocation includes the description field and that subsequent
update_project call uses {"workspace_path": tmpdir} to persist the temporary
directory.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Sanity check: LeadAgent expects project workspace_path.

LeadAgent previously read root_path; with schema refactor ensure it now reads workspace_path or maps appropriately.

---

🏁 Script executed:

#!/bin/bash
rg -nP "get_project\(.*\).*root_path|workspace_path" codeframe/agents/lead_agent.py -C3

Length of output: 0

---

I need to broaden the search since the initial pattern didn't match. Let me verify the LeadAgent class definition and what it actually expects.

---

🏁 Script executed:

```shell
#!/bin/bash
# Find LeadAgent class definition and __init__ method
ast-grep --pattern 'class LeadAgent {
  $$$
  __init__($$$) {
    $$$
  }
  $$$
}'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Search for workspace_path usage in lead_agent.py
rg -n "workspace_path" codeframe/agents/lead_agent.py

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Search for root_path usage in lead_agent.py to see if it's been replaced
rg -n "root_path" codeframe/agents/lead_agent.py

Length of output: 207

---

🏁 Script executed:

#!/bin/bash
# Search for get_project calls in lead_agent.py
rg -n "get_project" codeframe/agents/lead_agent.py -C2

Length of output: 1576

---

I found that lead_agent.py is still using root_path (lines 92, 807), not workspace_path. Let me verify the project schema and database implementation to determine if this is intentional or a concern.

---

🏁 Script executed:

```shell
#!/bin/bash
# Find db.get_project implementation
rg -n "def get_project" --type=py -C5

Length of output: 4811

---

🏁 Script executed:

#!/bin/bash
# Check for project schema or model definition
fd -name "*.py" --exec rg -l "class.*Project|workspace_path.*root_path" {} \;

Length of output: 230

---

🏁 Script executed:

#!/bin/bash
# Look at test_standalone.py around lines 61-70 for context
cat test_standalone.py | sed -n '50,80p'

Length of output: 994

---

🏁 Script executed:

#!/bin/bash
# Search for workspace_path in entire codebase
rg -n "workspace_path" --type=py

Length of output: 4764

---

Update LeadAgent and test_standalone.py to use workspace_path instead of root_path.

The schema refactor replaced root_path with workspace_path in the projects table (confirmed in migration file and schema tests). However, lead_agent.py is still reading root_path on lines 92 and 807, and test_standalone.py is setting root_path on line 58. The server code correctly uses workspace_path, so these need to be updated for consistency.

🤖 Prompt for AI Agents

In test_standalone.py around lines 61-70 (and specifically line 58 where
root_path is set), replace the use of root_path with workspace_path when
creating the test project and when instantiating LeadAgent; in lead_agent.py at
lines ~92 and ~807 update code to read project.workspace_path (or the new
workspace_path field) instead of project.root_path, and if necessary add a safe
fallback to support older records (e.g., workspace_path = project.workspace_path
or project.root_path) so the tests and runtime use the new workspace_path field
consistently.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify mock target is correct for the task description.

The test mocks BackendWorkerAgent (line 98), but according to the Sprint 4 solution document in this PR, task descriptions containing "test" may be assigned to TestWorkerAgent instead. The task description on line 92 is "Test description".

Run the following to check which agent type gets assigned:

If the task is assigned to test-engineer, you may need to mock TestWorkerAgent instead of BackendWorkerAgent.

---

🏁 Script executed:

#!/bin/bash
# Check agent assignment logic for task descriptions

echo "=== Checking SimpleAgentAssigner logic ==="

# Find agent assignment logic
rg -n --type=py -A10 -B5 'class SimpleAgentAssigner|def assign_agent_type'

# Check for keyword matching in agent assignment
rg -n --type=py -C5 '"test"|"Test"' codeframe/agents/simple_agent_assigner.py

Length of output: 2357

---

---

🏁 Script executed:

#!/bin/bash
# Get full implementation of assign_agent_type method

echo "=== Full assign_agent_type implementation ==="
rg -n --type=py -A30 'def assign_agent_type' codeframe/agents/simple_assignment.py

echo ""
echo "=== Check for test keyword matching ==="
rg -n --type=py -C3 'test.*keyword|keyword.*test' codeframe/agents/simple_assignment.py

Length of output: 1564

---

---

🏁 Script executed:

#!/bin/bash
# Find AGENT_KEYWORDS definition

echo "=== AGENT_KEYWORDS definition ==="
rg -n --type=py -A20 'AGENT_KEYWORDS' codeframe/agents/simple_assignment.py

echo ""
echo "=== Check if test-engineer keywords exist ==="
rg -n --type=py -A5 'test.*engineer|test-engineer' codeframe/agents/simple_assignment.py

Length of output: 3362

---

---

🏁 Script executed:

#!/bin/bash
# Find how agent types map to actual agent classes

echo "=== Search for agent class mapping ==="
rg -n --type=py -C5 'test-engineer|TestWorkerAgent' codeframe/agents/

echo ""
echo "=== Check for agent factory or registry ==="
rg -n --type=py 'get_agent|create_agent|agent_class' codeframe/agents/

Length of output: 8080

---

Mock BackendWorkerAgent instead with TestWorkerAgent, or change task description to avoid test-engineer assignment.

The task description "Test description" triggers the "test-engineer" keyword match in SimpleAgentAssigner, which assigns the task to test-engineer agent type. The agent_pool_manager creates a TestWorkerAgent instance when agent_type == "test-engineer", not BackendWorkerAgent. Update the mock target on line 98 to mock TestWorkerAgent, or change the task description to something like "Sample task description" to avoid keyword matching.

🤖 Prompt for AI Agents

In tests/test_async_debug.py around lines 82-123, the task description triggers
SimpleAgentAssigner to assign "test-engineer" so the agent_pool_manager creates
a TestWorkerAgent, not BackendWorkerAgent; update the patch on line ~98 to patch
'codeframe.agents.agent_pool_manager.TestWorkerAgent' instead of
BackendWorkerAgent (or alternatively change the task description string from
"Test description" to something without the "test-engineer" keyword, e.g.,
"Sample task description") so the mock matches the actual created agent type.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix incorrect method signatures in test_both_fixtures.

The test has two critical issues:

1. Line 58: create_project() is called with ProjectStatus.ACTIVE as the description parameter, but the method expects a string description. According to the relevant code snippets, the signature is create_project(name: str, description: str, ...).

2. Line 60: update_project() is called with a dict as a positional argument, but the method signature from relevant snippets shows it accepts project_id and **kwargs, not a dict parameter.

Apply this diff to fix both issues:

 def test_both_fixtures(db_debug, temp_project_dir_debug):
     """Test using both fixtures."""
     print("\n⭐ TEST: test_both_fixtures started")
-    project_id = db_debug.create_project("test-project", ProjectStatus.ACTIVE)
+    project_id = db_debug.create_project(
+        name="test-project",
+        description="Debug test project"
+    )
     print(f"⭐ TEST: Created project {project_id}")
-    db_debug.update_project(project_id, {"root_path": temp_project_dir_debug})
+    db_debug.update_project(project_id, workspace_path=temp_project_dir_debug)
     print("⭐ TEST: Updated project root_path")
     print("⭐ TEST: test_both_fixtures passed!")

📝 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.

Suggested change

	def test_both_fixtures(db_debug, temp_project_dir_debug):
	"""Test using both fixtures."""
	print("\n⭐ TEST: test_both_fixtures started")
	project_id = db_debug.create_project("test-project", ProjectStatus.ACTIVE)
	print(f"⭐ TEST: Created project {project_id}")
	db_debug.update_project(project_id, {"root_path": temp_project_dir_debug})
	print("⭐ TEST: Updated project root_path")
	print("⭐ TEST: test_both_fixtures passed!")
	def test_both_fixtures(db_debug, temp_project_dir_debug):
	"""Test using both fixtures."""
	print("\n⭐ TEST: test_both_fixtures started")
	project_id = db_debug.create_project(
	name="test-project",
	description="Debug test project"
	)
	print(f"⭐ TEST: Created project {project_id}")
	db_debug.update_project(project_id, workspace_path=temp_project_dir_debug)
	print("⭐ TEST: Updated project root_path")
	print("⭐ TEST: test_both_fixtures passed!")

🤖 Prompt for AI Agents

In tests/test_fixture_debug.py around lines 55–62, the test calls create_project
and update_project with incorrect argument shapes: change create_project to pass
a string description (e.g., create_project("test-project", "Active project")
and, if status is required, pass status=ProjectStatus.ACTIVE as a keyword), and
change update_project to call with keyword args instead of a dict (e.g.,
update_project(project_id, root_path=temp_project_dir_debug)).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Fix the create_project API call - incorrect signature.

The create_project method signature has changed in the refactoring. According to the new schema (from codeframe/persistence/database.py), it requires name and description as the first two parameters:

def create_project(
    self,
    name: str,
    description: str,
    source_type: str = "empty",
    ...
) -> int:

However, this test is passing ProjectStatus.ACTIVE as the second argument, which will be interpreted as the description parameter. This is a type mismatch and will cause test failures.

Apply this diff to fix the API call:

 @pytest.fixture
 def project_id_debug(db_debug, temp_project_dir_debug):
     """Create test project."""
     print("🟢 FIXTURE: Creating project...")
-    project_id = db_debug.create_project("test-project", ProjectStatus.ACTIVE)
-    db_debug.update_project(project_id, {"root_path": temp_project_dir_debug})
+    project_id = db_debug.create_project(
+        name="test-project",
+        description="Test project for LeadAgent debug",
+        workspace_path=str(temp_project_dir_debug)
+    )
+    db_debug.update_project(project_id, {"git_initialized": True})
     print(f"🟢 FIXTURE: Project created: {project_id} ✅")
     return project_id

Note: I've also updated to use workspace_path instead of root_path and git_initialized instead of status, aligning with the new schema.

🤖 Prompt for AI Agents

In tests/test_lead_agent_debug.py around lines 37-44, the create_project call
uses the old signature and passes ProjectStatus.ACTIVE as the second argument
and later updates root_path; change the call to match the new
create_project(name: str, description: str, source_type: str = "empty", ...)
signature by passing a description string (e.g., "test project"), include the
git_initialized flag instead of status (e.g., git_initialized=True/False as
appropriate), and update db_debug.update_project to set "workspace_path" (not
"root_path") to temp_project_dir_debug; ensure the created project_id is
returned as before.

[claude]

Pull Request Review: Project Schema Refactoring

Overview

Substantial PR (14,986 additions, 247 deletions, 65 files) implementing major architectural change for multiple project source types and workspace management.

✅ Strengths

1. Well-Designed Migration System - Proper can_apply() detection, logging, rollback capability
2. Comprehensive Workspace Management - Strong error handling, security validation (/home/runner restriction), timeout protection, cleanup on failure
3. Robust API Design - Transactional rollback, proper HTTP codes, Pydantic validation
4. Good Test Coverage - 4 API tests, workspace tests, schema constraint tests

⚠️ Critical Issues

CRITICAL: Destructive Migration

Location: migration_002_refactor_projects_schema.py:89-90

• Migration DROPS projects table without data preservation
• Production deployment will lose all data
• Marked as dev-only but in main migration system
  Fix: Add production safety check OR implement data migration

HIGH: Incomplete Upload Source

Location: workspace/manager.py:229-238

• No timeout, no exception handling, no validation
• Does not extract archives (TODO)
  Fix: Remove SourceType.UPLOAD or add NotImplementedError

MEDIUM: Test Coverage Gaps

• No tests for LOCAL_PATH, cleanup on failure, path traversal, timeouts
• Git remote test accepts both 201 and 500 (does not validate behavior)
  Fix: Add mocking and security tests

MEDIUM: Schema Redundancy

• _create_schema() creates projects table, then migration drops and recreates
• Table created twice on fresh init
  Fix: Let migrations handle table creation

MEDIUM: Missing Validations

• No duplicate name check, URL validation, path existence check
  Fix: Add validation before project creation

🔒 Security

Good: Path validation, no shell injection, symlink protection

Concerns:

• TOCTOU in path traversal check (workspace/manager.py:161)
• No rate limiting on expensive operations

📊 Performance

Issue: Synchronous workspace creation blocks request for up to 5 minutes
Fix: Use BackgroundTasks for async creation, return 202 Accepted

📋 Summary

Code Quality: 4/5 - Well-structured, good error handling
Test Coverage: 3/5 - Happy paths covered, missing edge cases
Production Ready: NO - Destructive migration, blocking operations

Recommendation:

• ✅ Approve with required changes
• ❌ Block merge until migration fixed
• 📋 Create issues for async creation and testing

---

Reviewed with Claude Code

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (3)

codeframe/ui/server.py (3)

291-294: Avoid leaking internal errors; preserve traceback on workspace failures.

Current implementation exposes raw exception text in the 500 response and loses the original traceback. This is a security concern and hampers diagnostics.

Apply this diff to fix the exception handling:

     except Exception as e:
         # Cleanup: delete project if workspace creation fails
         app.state.db.delete_project(project_id)
-        raise HTTPException(status_code=500, detail=f"Workspace creation failed: {str(e)}")
+        logger.exception("Workspace creation failed for project %s", project_id)
+        raise HTTPException(status_code=500, detail="Workspace creation failed") from e

---

429-458: Fix logging, remove duplicate import, and chain exceptions.

Multiple issues in the blockers endpoint:

• Line 439: json is already imported at the module level (line 11); remove the inner import
• Lines 456-458: Exception handler doesn't chain exceptions and exposes internal error details

Apply this diff:

             # Parse blocking_agents from JSON if present
             blocking_agents = []
             if blocker.get("blocking_agents"):
-                import json
                 try:
                     blocking_agents = json.loads(blocker["blocking_agents"])
                 except (json.JSONDecodeError, TypeError):
                     blocking_agents = []

             formatted_blockers.append({
                 "id": blocker["id"],
                 "task_id": blocker["task_id"],
                 "severity": blocker["severity"],
                 "question": blocker["question"],
                 "reason": blocker["reason"],
                 "created_at": blocker["created_at"],
                 "blocking_agents": blocking_agents
             })

         return {"blockers": formatted_blockers}
     except Exception as e:
-        logger.error(f"Error fetching blockers: {e}", exc_info=True)
-        raise HTTPException(status_code=500, detail=f"Error fetching blockers: {str(e)}")
+        logger.exception("Error fetching blockers for project %s", project_id)
+        raise HTTPException(status_code=500, detail="Error fetching blockers") from e

---

484-491: Use exception chaining and generic error messages.

The exception handler should log the full traceback and return a generic error message to avoid leaking internal details.

Apply this diff:

     try:
         # Query changelog table for activity
         activity_items = app.state.db.get_recent_activity(project_id, limit=limit)

         return {"activity": activity_items}
     except Exception as e:
-        logger.error(f"Error fetching activity: {e}", exc_info=True)
-        raise HTTPException(status_code=500, detail=f"Error fetching activity: {str(e)}")
+        logger.exception("Error fetching activity for project %s", project_id)
+        raise HTTPException(status_code=500, detail="Error fetching activity") from e

🧹 Nitpick comments (2)

.specify/scripts/bash/create-new-feature.sh (1)

106-106: Separate declaration and assignment to comply with shellcheck SC2155.

Although set -e provides safety, assigning a pipeline result directly to a variable still masks individual command exit codes. This is flagged by shellcheck SC2155.

- local clean_name=$(echo "$description" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9]/ /g')
+ local clean_name
+ clean_name=$(echo "$description" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9]/ /g')

codeframe/ui/server.py (1)

37-39: Consider environment variable for workspace root path.

The workspace root is hardcoded relative to the current working directory, unlike db_path which supports an environment variable fallback. For consistency and deployment flexibility, consider adding a WORKSPACE_ROOT environment variable.

Apply this diff to add environment variable support:

-    # Initialize workspace manager
-    workspace_root = Path.cwd() / ".codeframe" / "workspaces"
-    app.state.workspace_manager = WorkspaceManager(workspace_root)
+    # Initialize workspace manager
+    workspace_root_str = os.environ.get("WORKSPACE_ROOT", ".codeframe/workspaces")
+    workspace_root = Path(workspace_root_str)
+    app.state.workspace_manager = WorkspaceManager(workspace_root)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b9be32 and a506a6b.

📒 Files selected for processing (3)

• .claude/commands/speckit.tasks.md (1 hunks)
• .specify/scripts/bash/create-new-feature.sh (1 hunks)
• codeframe/ui/server.py (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• .claude/commands/speckit.tasks.md

🧰 Additional context used

🧬 Code graph analysis (1)

codeframe/ui/server.py (3)

codeframe/persistence/database.py (6)

• create_project (333-378)
• update_project (640-672)
• delete_project (674-682)
• get_project (380-385)
• get_blockers (1732-1759)
• get_recent_activity (1761-1802)

codeframe/ui/models.py (3)

• ProjectCreateRequest (19-39)
• ProjectResponse (42-55)
• SourceType (11-16)

codeframe/workspace/manager.py (2)

• WorkspaceManager (13-238)
• create_workspace (25-70)

🪛 Ruff (0.14.1)

codeframe/ui/server.py

291-291: Do not catch blind exception: Exception

(BLE001)

---

294-294: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

294-294: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

455-455: Consider moving this statement to an else block

(TRY300)

---

458-458: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

458-458: Use explicit conversion flag

Replace with conversion flag

(RUF010)

---

488-488: Consider moving this statement to an else block

(TRY300)

---

491-491: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

491-491: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🪛 Shellcheck (0.11.0)

.specify/scripts/bash/create-new-feature.sh

[warning] 106-106: Declare and assign separately to avoid masking return values.

(SC2155)

⏰ 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)

• GitHub Check: claude-review

🔇 Additional comments (3)

.specify/scripts/bash/create-new-feature.sh (1)

8-41: Argument parsing has been correctly fixed.

The shift-based loop now properly handles all options and positional arguments. This resolves the critical indexing issue (${!i}) flagged in the past review. The implementation correctly validates option prerequisites (e.g., --short-name requires a value) and collects remaining arguments for the feature description.

codeframe/ui/server.py (2)

12-12: LGTM: Module logger properly initialized.

The addition of module-level logging correctly addresses the undefined logger issues flagged in previous reviews.

Also applies to: 23-24

---

21-21: LGTM: WorkspaceManager import supports workspace integration.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Critical: blocking_agents field not returned by database query.

The code attempts to parse blocking_agents from the blocker dict, but according to database.py (lines 1731-1758), get_blockers() only returns columns: id, task_id, severity, question, reason, created_at. The blocking_agents field is never present, so this code block never executes.

Verify the database schema and query to confirm whether blocking_agents should be included:

If blocking_agents should be included, update the SQL query in database.py. Otherwise, remove this dead code from the API endpoint.

---

🏁 Script executed:

#!/bin/bash
# Check if blocking_agents column exists in blockers table and is queried in get_blockers
rg -nP -A10 'def get_blockers' codeframe/persistence/database.py

# Check blockers table schema
rg -nP -B5 -A10 'CREATE TABLE.*blockers' codeframe/persistence/

Length of output: 2736

---

---

🏁 Script executed:

```shell
#!/bin/bash
# Get the complete get_blockers function implementation to see the SQL query
sed -n '1732,1800p' codeframe/persistence/database.py

Length of output: 76

---

🏁 Script executed:

#!/bin/bash
# Get the complete get_blockers function implementation to see the SQL query
sed -n '1732,1800p' codeframe/persistence/database.py

Length of output: 2237

---

🏁 Script executed:

#!/bin/bash
# Check json import at top of file and around lines 438-443
sed -n '1,15p' codeframe/ui/server.py | grep -n "import json"
echo "---"
sed -n '430,460p' codeframe/ui/server.py

Length of output: 1219

---

Remove dead code and duplicate import: blocking_agents is never returned by database query.

The get_blockers() function in database.py (lines 1743-1756) only selects id, task_id, severity, question, reason, and created_at. The blocking_agents field is not present in the blockers table schema and is never queried, so blocker.get("blocking_agents") always returns None. The entire code block at lines 438-443 is dead code.

Additionally, json is already imported at line 11; remove the redundant import at line 440 inside the loop.

🤖 Prompt for AI Agents

In codeframe/ui/server.py around lines 438 to 443, remove the dead code block
that checks blocker.get("blocking_agents") and the inner import json because the
blockers query never includes a blocking_agents field so this branch is never
executed; simply delete those lines (the conditional, import, try/except and
assignment) to eliminate unreachable code and the duplicate import.