feat(planning): Add planning phase automation and task approval - #205
Conversation
Implements automated planning after PRD generation:
- Triggers generate_issues() and decompose_prd() after PRD completes
- Broadcasts WebSocket events at each stage (planning_started,
issues_generated, tasks_decomposed, tasks_ready, planning_failed)
Adds task approval endpoint (POST /api/projects/{id}/tasks/approve):
- Validates project is in planning phase before approval
- Updates approved task statuses to pending
- Transitions project phase to active via PhaseManager
- Broadcasts development_started event
Tests: 27 new tests covering automation flow, error handling, and
validation (all passing)
WalkthroughAdds planning-phase automation and task-approval endpoints: a background planner orchestrates LeadAgent issue generation and PRD decomposition with WebSocket progress/failure broadcasts; PRD completion spawns non-blocking planning; project-scoped task approval transitions phase to active and emits development_started; routers and tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Discovery API
participant LA as LeadAgent
participant DB as Database
participant WS as WebSocket Manager
Client->>API: POST /prd/complete (project_id, prd)
API->>DB: Save PRD
DB-->>API: OK
Note over API: schedule generate_planning_background (async background)
API->>WS: broadcast_planning_started(project_id)
WS-->>Client: planning_started
API->>LA: generate_issues(...) [via asyncio.to_thread]
LA-->>API: issues[] (count=N)
API->>WS: broadcast_issues_generated(project_id, issue_count=N)
WS-->>Client: issues_generated
API->>LA: decompose_prd(...) [via asyncio.to_thread]
LA-->>API: tasks[] (count=M)
API->>WS: broadcast_tasks_decomposed(project_id, task_count=M)
WS-->>Client: tasks_decomposed
API->>DB: Store tasks
DB-->>API: OK
API->>WS: broadcast_tasks_ready(project_id, total_tasks=M)
WS-->>Client: tasks_ready
alt error during planning
API->>WS: broadcast_planning_failed(project_id, error)
WS-->>Client: planning_failed
end
sequenceDiagram
participant Client
participant API as Tasks API
participant DB as Database
participant PM as PhaseManager
participant WS as WebSocket Manager
Client->>API: POST /api/projects/{id}/tasks/approve (approved_ids, excluded_ids)
API->>DB: Fetch project & verify phase == planning
DB-->>API: project
alt phase != planning
API-->>Client: 400 Bad Request
else
API->>DB: Update approved tasks -> status: pending
DB-->>API: OK
API->>PM: transition_phase(project_id, "active")
PM->>DB: Update project.phase -> active
DB-->>PM: OK
PM-->>API: OK
API->>WS: broadcast_development_started(project_id, approved_count, excluded_count)
WS-->>Client: development_started
API-->>Client: 200 OK (TaskApprovalResponse)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: Planning Phase AutomationI've reviewed PR #205 and have the following feedback: ✅ Strengths
🔴 Critical Issues1. Blocking Async Context (codeframe/ui/routers/discovery.py:131)# In generate_prd_background (async context)
await generate_planning_background(project_id, db, api_key)Problem: This blocks the PRD generation endpoint from returning until all planning automation completes (potentially minutes). The PRD endpoint should return immediately after saving the PRD. Fix: Use FastAPI's from fastapi import BackgroundTasks
async def generate_prd_background(..., background_tasks: BackgroundTasks):
# ... save PRD ...
# Broadcast PRD completion
# Schedule planning automation (non-blocking)
background_tasks.add_task(generate_planning_background, project_id, db, api_key)2. Synchronous Blocking in Async Function (discovery.py:183, 192)issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1)
decomposition_result = await asyncio.to_thread(agent.decompose_prd)Problem: Investigation Needed:
Risk: Silent failures or hangs if LeadAgent uses async I/O. 3. Missing Transaction Rollback on Partial Failure (tasks.py:206-220)# Update approved tasks to pending status
for task in approved_tasks:
db.update_task(task.id, {"status": "pending"})
# Transition project phase to active
PhaseManager.transition(project_id, "active", db)Problem: If Fix: Wrap in a transaction or reverse task updates on failure: try:
# Transition phase FIRST (fails early)
PhaseManager.transition(project_id, "active", db)
# Then update tasks (less likely to fail)
for task in approved_tasks:
db.update_task(task.id, {"status": "pending"})
except Exception:
# No rollback needed if phase transition fails first
raise
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codeframe/ui/routers/discovery.py (1)
147-206: Consider adding timeouts to LeadAgent calls.The
generate_prd_backgroundfunction has a 120-second timeout (line 85-89), butgenerate_planning_backgroundhas no timeout protection forgenerate_issuesanddecompose_prdcalls. These LLM-based operations could potentially hang indefinitely.🔎 Proposed fix to add timeout protection
+ PLANNING_TIMEOUT = 120 # seconds per operation + # Stage 2: Generate issues from PRD logger.info(f"Generating issues for project {project_id}") - issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1) + try: + issues = await asyncio.wait_for( + asyncio.to_thread(agent.generate_issues, sprint_number=1), + timeout=PLANNING_TIMEOUT + ) + except asyncio.TimeoutError: + logger.error(f"Issue generation timed out for project {project_id}") + await broadcast_planning_failed(manager, project_id, "Issue generation timed out") + return issue_count = len(issues) if issues else 0tests/ui/test_discovery_automation.py (1)
270-289: Test does not verify the claimed behavior.The test docstring states it verifies "planning automation is triggered after PRD completion," but it only inspects the function signature of
generate_prd_background. This doesn't actually test the integration between PRD completion and planning automation.Consider adding a test that mocks
generate_planning_backgroundand verifies it's called after PRD generation completes successfully.🔎 Proposed integration test
@pytest.mark.asyncio async def test_prd_completion_calls_planning_automation(self, mock_db, mock_manager): """Test that generate_planning_background is called after PRD completion.""" mock_agent = MagicMock() mock_agent.generate_prd.return_value = "# PRD Content" with patch("codeframe.ui.routers.discovery.manager", mock_manager), \ patch("codeframe.ui.routers.discovery.LeadAgent", return_value=mock_agent), \ patch("codeframe.ui.routers.discovery.generate_planning_background") as mock_planning: mock_planning.return_value = None # async function returns None from codeframe.ui.routers.discovery import generate_prd_background await generate_prd_background(project_id=1, db=mock_db, api_key="test-key") mock_planning.assert_called_once_with(1, mock_db, "test-key")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codeframe/ui/routers/discovery.pycodeframe/ui/routers/tasks.pycodeframe/ui/server.pycodeframe/ui/websocket_broadcasts.pytests/ui/test_discovery_automation.pytests/ui/test_task_approval.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/websocket_broadcasts.pycodeframe/ui/server.pycodeframe/ui/routers/discovery.pycodeframe/ui/routers/tasks.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: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/ui/test_task_approval.py
🧬 Code graph analysis (3)
codeframe/ui/websocket_broadcasts.py (2)
codeframe/core/models.py (1)
project_id(234-235)codeframe/ui/shared.py (1)
broadcast(154-185)
codeframe/ui/server.py (1)
codeframe/cli/project_commands.py (1)
tasks(255-317)
codeframe/ui/routers/discovery.py (2)
codeframe/ui/websocket_broadcasts.py (5)
broadcast_planning_started(794-813)broadcast_issues_generated(816-838)broadcast_tasks_decomposed(841-863)broadcast_tasks_ready(866-889)broadcast_planning_failed(892-915)codeframe/agents/lead_agent.py (1)
LeadAgent(28-2333)
⏰ 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: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (21)
codeframe/ui/server.py (1)
340-340: LGTM!The new
project_routeris correctly mounted alongside the existingtasks.router. This exposes the/api/projects/{project_id}/tasks/approveendpoint for the planning phase automation feature.tests/ui/test_task_approval.py (5)
19-57: LGTM!The fixtures are well-structured with appropriate mocks for the planning phase workflow. The mock tasks use
TaskStatus.PENDINGwhich correctly represents tasks awaiting approval.
60-192: LGTM!The
TestTaskApprovalEndpointclass provides comprehensive coverage of the approval workflow including success responses, exclusions, status updates, phase transitions, WebSocket broadcasts, and the rejection path.
194-289: LGTM!The validation tests cover all critical error paths: wrong phase (400), no tasks (404), project not found (404), and access denied (403). Error message assertions provide good regression protection.
292-319: LGTM!The broadcast message format test validates the essential fields and ensures the timestamp follows the expected ISO 8601 UTC format (ending with 'Z').
322-421: LGTM!Comprehensive tests for all planning-related broadcast functions with consistent validation of message types, project IDs, specific payload fields, and timestamps.
codeframe/ui/routers/discovery.py (1)
130-132: Verify error propagation behavior.The planning automation is triggered after PRD completion within the same try block. If
generate_planning_backgroundraises an exception, it will be caught by the outer exception handler (line 133) and broadcast aprd_generation_failedevent, which may be misleading since the PRD was actually generated successfully.Consider whether planning failures should be isolated from PRD completion status, or if this behavior is intentional (treating PRD + planning as an atomic operation).
tests/ui/test_discovery_automation.py (3)
17-49: LGTM!The fixtures correctly mock the LeadAgent's return values matching the expected interface, with
generate_issuesreturning a list anddecompose_prdreturning a dict withtaskscount.
52-193: LGTM!Excellent test coverage for the planning automation happy path. The event ordering test (lines 177-193) is particularly valuable for ensuring the correct broadcast sequence.
196-267: LGTM!Good error handling tests that verify failure broadcasts contain the error message and that no partial state changes occur (phase is not updated on error).
codeframe/ui/websocket_broadcasts.py (7)
787-792: LGTM!The section header follows the existing convention in this file for organizing broadcast functions by feature.
794-813: LGTM!
broadcast_planning_startedfollows the established pattern with appropriate message fields and error handling.
816-838: LGTM!
broadcast_issues_generatedcorrectly includes theissue_countfor real-time UI progress feedback.
841-863: LGTM!
broadcast_tasks_decomposedmaintains consistency with other count-based broadcasts.
866-889: LGTM!
broadcast_tasks_readyincludes astatus: "ready_for_review"field which provides a clear signal for the frontend to prompt user action.
892-915: LGTM!
broadcast_planning_failedappropriately includes the error message for debugging. The error originates from controlled sources (LeadAgent exceptions) so sensitive data leakage risk is low.
918-946: LGTM!
broadcast_development_startedprovides a complete summary of the approval action with counts and phase confirmation, enabling the frontend to update both progress indicators and phase displays.codeframe/ui/routers/tasks.py (4)
10-24: LGTM!The imports are well-organized and all are utilized by the new approval functionality.
29-30: LGTM!The
project_routerwith/api/projectsprefix correctly scopes the task approval endpoint under the project resource hierarchy.
120-132: LGTM!The models are well-designed:
TaskApprovalRequestusesdefault_factory=listcorrectly for the mutable default, andTaskApprovalResponseprovides a comprehensive summary of the approval action.
135-241: Well-structured endpoint with comprehensive validation.The approval flow is logically ordered with proper error handling. A few observations:
The individual task updates (lines 207-208) work correctly but could become slow with many tasks. Consider a batch update if performance becomes an issue.
Good handling of the rejection case with an early return (lines 176-183).
Critical fixes: - Use asyncio.create_task() for non-blocking planning automation (PRD completion now reports immediately while planning runs in background) - Move phase transition before task updates to prevent inconsistent state on partial failure Improvements: - Move imports to module level (PEP 8 compliance) - Add exc_info=True to error logging for full stack traces - Add docstring note about sync LeadAgent methods using asyncio.to_thread()
Updated Code Review: Planning Phase AutomationI've reviewed PR #205 in detail, including examining previous comments. Here's my comprehensive feedback: ✅ Strengths
🟡 Points to Address1. Phase-First Transaction Ordering is Good ✅The code correctly transitions phase FIRST (line 209), then updates tasks (line 220-221). This ensures atomicity - if phase transition fails, no tasks are modified. This is the right approach. However, consider adding a comment explaining why excluded tasks aren't deleted: # Separate approved and excluded tasks
# Note: Excluded tasks remain in database with original status for audit trail
excluded_ids = set(request.excluded_task_ids)2. Clarify Excluded Task Behavior (tasks.py:202-204)The code counts excluded tasks but doesn't modify them. Please clarify in PR description or code comment:
Recommendation: Add a comment explaining the intended behavior. 3. asyncio.to_thread() Usage is Correct ✅I verified that No changes needed - the docstring at line 166 correctly explains this. 4. Background Task Spawning ✅Line 139 uses Potential improvement: Consider adding error handling for unhandled task exceptions: task = asyncio.create_task(generate_planning_background(project_id, db, api_key))
task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None)This prevents silent failures if the background task crashes. 💡 Minor Suggestions5. Hardcoded Sprint Number (discovery.py:185)issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1)Question: Is 6. Test Gap: PhaseManager Call VerificationThe tests mock mock_phase_manager.transition.assert_called_once_with(1, "active", mock_db)7. Potential Race ConditionNo test covers concurrent approval attempts. Consider adding test for:
📊 Overall AssessmentThis is a well-structured feature with solid implementation patterns:
Main items to address:
Recommendation: Approve with minor suggestions. The critical issues from coderabbitai's review have been addressed correctly. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/tasks.py:
- Around line 202-221: The approved_tasks update loop lacks error handling so a
failure in db.update_task can leave the project phase as "active" while some
tasks remain non-pending; wrap the for loop that iterates over approved_tasks
and calls db.update_task(task.id, {"status": "pending"}) in a try/except block
that logs the exception (include task.id and project_id) and either rolls back
or raises an HTTPException with a 500 status, or pre-validate updateability by
checking each task's current status against TaskStatus (PENDING, ASSIGNED,
IN_PROGRESS, BLOCKED, COMPLETED, FAILED) before performing updates to avoid
partial state changes.
🧹 Nitpick comments (2)
codeframe/ui/routers/tasks.py (1)
224-229: Consider making WebSocket broadcast non-fatal.If
broadcast_development_startedraises an exception, the entire endpoint will fail with a 500 error, even though the core phase transition and task updates succeeded. This could confuse users who see their tasks approved but receive an error response.🔎 Proposed enhancement to make broadcast failures non-fatal
- # Broadcast development started event - await broadcast_development_started( - manager=manager, - project_id=project_id, - approved_count=len(approved_tasks), - excluded_count=len(excluded_tasks), - ) + # Broadcast development started event (non-fatal if it fails) + try: + await broadcast_development_started( + manager=manager, + project_id=project_id, + approved_count=len(approved_tasks), + excluded_count=len(excluded_tasks), + ) + except Exception as e: + logger.warning(f"Failed to broadcast development_started for project {project_id}: {e}") + # Non-fatal - continue with responseBased on learnings, other endpoints in the codebase (e.g.,
submit_discovery_answerat line 339 in discovery.py) handle broadcast failures as non-fatal warnings.codeframe/ui/routers/discovery.py (1)
192-208: LGTM: Complete planning flow with comprehensive error handling.The decomposition stage and completion broadcasts complete the planning automation workflow. Error handling correctly logs with stack traces and broadcasts
planning_failedto notify clients of issues.Minor observation: Line 195 defaults to 0 tasks if
decomposition_resultisNoneor lacks a "tasks" key. This means the workflow will complete successfully even if decomposition returns unexpected data. This may be intentional, but consider validating thattask_count > 0or at least logging a warning if decomposition produces no tasks.🔎 Optional: Add validation for empty task decomposition
# Stage 4: Decompose PRD into tasks logger.info(f"Decomposing PRD into tasks for project {project_id}") decomposition_result = await asyncio.to_thread(agent.decompose_prd) task_count = decomposition_result.get("tasks", 0) if decomposition_result else 0 + +# Log warning if no tasks were generated +if task_count == 0: + logger.warning(f"Decomposition produced 0 tasks for project {project_id}") # Stage 5: Broadcast tasks decomposed await broadcast_tasks_decomposed(manager, project_id, task_count)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/ui/routers/discovery.pycodeframe/ui/routers/tasks.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/tasks.pycodeframe/ui/routers/discovery.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: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/ui/routers/discovery.py
⏰ 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: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (6)
codeframe/ui/routers/tasks.py (3)
11-11: LGTM: Clean import additions and router separation.The new imports support phase management, WebSocket broadcasts, and the project-scoped approval endpoint. The separate
project_routerprovides good API organization for project-scoped operations.Also applies to: 17-17, 20-21, 29-30
120-133: LGTM: Well-structured approval models.The request and response models are clean and include appropriate validation. Using
Field(default_factory=list)forexcluded_task_idsis the correct Pydantic pattern.
163-199: LGTM: Comprehensive validation logic.The validation sequence correctly checks project existence, authorization, phase state, and task availability. The early return for rejection (lines 176-183) is a clean pattern that avoids unnecessary processing.
codeframe/ui/routers/discovery.py (3)
7-7: LGTM: Appropriate imports for planning automation.The
asyncioimport supports non-blocking task creation and thread-pool delegation for sync agent methods. The planning broadcast imports provide comprehensive event notifications throughout the planning workflow.Also applies to: 21-27
137-142: LGTM: Fire-and-forget planning automation with good error logging.The
asyncio.create_taskpattern allows PRD completion to respond immediately while planning runs asynchronously. The planning task has its own error handling (lines 205-207) that broadcastsplanning_failed, ensuring failures are communicated to clients. The addition ofexc_info=Trueto error logging is excellent for debugging.
155-190: LGTM: Well-structured planning automation with appropriate async patterns.The function correctly uses
asyncio.to_threadto run synchronousLeadAgentmethods without blocking the event loop. The sequential broadcast pattern (planning_started → issues_generated) provides clear progress tracking. Line 186 safely handles the case whereissuesmight beNone.
| excluded_ids = set(request.excluded_task_ids) | ||
| approved_tasks = [t for t in tasks if t.id not in excluded_ids] | ||
| excluded_tasks = [t for t in tasks if t.id in excluded_ids] | ||
|
|
||
| # Transition project phase to active FIRST (fails early before modifying tasks) | ||
| # This ensures we don't leave tasks in pending status if phase transition fails | ||
| try: | ||
| PhaseManager.transition(project_id, "active", db) | ||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to transition phase for project {project_id}: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="Failed to transition project to development phase" | ||
| ) | ||
|
|
||
| # Update approved tasks to pending status (after phase transition succeeds) | ||
| for task in approved_tasks: | ||
| db.update_task(task.id, {"status": "pending"}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check TaskStatus enum definition and status patterns
echo "=== Searching for TaskStatus enum ==="
ast-grep --pattern 'class TaskStatus'
echo -e "\n=== Searching for status string definitions ==="
rg -n 'status.*=.*["\']' codeframe/ --type py -A2 | head -50
echo -e "\n=== Checking for excluded/rejected status handling ==="
rg -nP -C3 '\b(excluded|rejected|skip)\b' codeframe/ --type py | head -80Repository: frankbria/codeframe
Length of output: 795
🏁 Script executed:
#!/bin/bash
# Check for transaction patterns and db.update_task usage
echo "=== Looking for transaction patterns ==="
rg -n 'transaction|atomic|begin|commit|rollback' codeframe/ --type py | head -30
echo -e "\n=== Check db.update_task implementation ==="
rg -n 'def update_task' codeframe/ --type py -A10Repository: frankbria/codeframe
Length of output: 7292
🏁 Script executed:
#!/bin/bash
# Check the full approve_tasks function to understand context
echo "=== Finding approve_tasks function ==="
ast-grep --pattern 'def approve_tasks'Repository: frankbria/codeframe
Length of output: 7500
🏁 Script executed:
#!/bin/bash
# Get full update_task implementation
echo "=== Full update_task implementation ==="
sed -n '97,130p' codeframe/persistence/repositories/task_repository.py
echo -e "\n=== Check how db uses SQLite connection ==="
rg -n 'self.conn.execute|cursor.execute' codeframe/persistence/repositories/task_repository.py | head -20Repository: frankbria/codeframe
Length of output: 2033
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling around task updates in approve_tasks or similar patterns
echo "=== Check for error handling patterns in task updates ==="
rg -B3 -A3 'for.*task.*in.*tasks' codeframe/ui/routers/tasks.py
echo -e "\n=== Check PhaseManager.transition to understand phase change ==="
ast-grep --pattern 'def transition'Repository: frankbria/codeframe
Length of output: 2980
No action needed for excluded task handling; add error handling to task status update loop.
The approach of transitioning the project phase before updating tasks is correct (deliberate fail-fast pattern). However, two clarifications:
-
Excluded tasks: No update is needed. The
TaskStatusenum has no "excluded" or "rejected" status—only PENDING, ASSIGNED, IN_PROGRESS, BLOCKED, COMPLETED, and FAILED. Excluded tasks intentionally retain their original status; they're tracked only for broadcast and response counts. -
Task update loop (lines 220-221): The loop lacks error handling. If any
update_taskcall fails, the project will be in "active" phase but some approved tasks won't have transitioned to "pending" status. Wrap the loop in a try-catch to handle failures gracefully or validate updateability before the loop.
🤖 Prompt for AI Agents
In @codeframe/ui/routers/tasks.py around lines 202 - 221, The approved_tasks
update loop lacks error handling so a failure in db.update_task can leave the
project phase as "active" while some tasks remain non-pending; wrap the for loop
that iterates over approved_tasks and calls db.update_task(task.id, {"status":
"pending"}) in a try/except block that logs the exception (include task.id and
project_id) and either rolls back or raises an HTTPException with a 500 status,
or pre-validate updateability by checking each task's current status against
TaskStatus (PENDING, ASSIGNED, IN_PROGRESS, BLOCKED, COMPLETED, FAILED) before
performing updates to avoid partial state changes.
Add comprehensive test coverage for missing scenarios: - End-to-end flow: planning phase → task approval → development phase - Tasks modified between generation and approval - Double approval (race condition) handling - Phase transition failure leaves tasks unchanged - Tasks deleted during approval process All 21 tests passing in test_task_approval.py
Final Code Review: Planning Phase AutomationI've thoroughly reviewed PR #205, including examining the diff and previous review comments. Here's my comprehensive assessment: ✅ Strengths
🟢 Implementation Patterns Verified as Correct1. Background Task Spawning ✅# discovery.py:139
asyncio.create_task(generate_planning_background(project_id, db, api_key))This correctly runs planning automation in the background without blocking PRD completion. PRD endpoint returns immediately while planning continues asynchronously. 2. asyncio.to_thread() Usage ✅# discovery.py:185, 192
issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1)
decomposition_result = await asyncio.to_thread(agent.decompose_prd)Verified that 3. Phase-First Transaction Pattern ✅# tasks.py:209-221
PhaseManager.transition(project_id, "active", db) # Fails early
for task in approved_tasks:
db.update_task(task.id, {"status": "pending"})If phase transition fails, no tasks are modified. This ensures data consistency. 💡 Minor Suggestions for Future Enhancement1. Hardcoded Sprint Number (discovery.py:185)issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1)Suggestion: Consider making sprint number configurable or deriving from project state. Currently always uses sprint 1, which may not be correct for all scenarios. 2. Excluded Task Behavior (tasks.py:202-204)excluded_ids = set(request.excluded_task_ids)
approved_tasks = [t for t in tasks if t.id not in excluded_ids]
excluded_tasks = [t for t in tasks if t.id in excluded_ids]Clarification Needed: Excluded tasks are counted but not modified. Should they:
Recommendation: Add a comment explaining the intended behavior for excluded tasks (e.g., "Excluded tasks remain unchanged for audit trail"). 3. Unhandled Background Task Exceptions# discovery.py:139
asyncio.create_task(generate_planning_background(project_id, db, api_key))Suggestion: Add done callback to prevent silent failures: task = asyncio.create_task(generate_planning_background(project_id, db, api_key))
task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None)4. Test Coverage GapsMissing integration tests for:
📊 Quality Metrics
✨ Overall AssessmentThis is a high-quality implementation with excellent test coverage and proper async patterns. The code demonstrates strong understanding of:
The minor suggestions above are optional enhancements for future consideration, not blocking issues. Recommendation: ✅ APPROVE - Ready to merge with optional follow-ups for excluded task behavior clarification. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/ui/test_task_approval.py:
- Around line 454-458: The test fixture's update_task uses
TaskStatus(updates["status"]) which fails because Enum construction expects
member names, not lowercase values; change the assignment to look up by name
using TaskStatus[updates["status"].upper()] (or implement a value-based lookup
in TaskStatus), e.g., inside update_task when "status" in updates set
task.status = TaskStatus[updates["status"].upper()] to handle the lowercase
"pending" input; locate update_task and TaskStatus references in the test and
replace the direct constructor usage accordingly.
🧹 Nitpick comments (1)
tests/ui/test_task_approval.py (1)
591-629: Consider verifying broadcast is also not called on failure.The test correctly verifies that
update_taskis not called when phase transition fails. For completeness, you could also assert that the WebSocket broadcast was not sent, ensuring the entire operation is atomic.🔎 Suggested enhancement
# Critical: update_task should NOT have been called since phase transition failed first mock_db.update_task.assert_not_called() + + # Also verify WebSocket broadcast was not sent + mock_manager.broadcast.assert_not_called()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/ui/test_task_approval.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/ui/test_task_approval.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
tests/ui/test_task_approval.py
🧬 Code graph analysis (1)
tests/ui/test_task_approval.py (3)
codeframe/ui/routers/tasks.py (2)
approve_tasks(136-242)TaskApprovalRequest(120-123)codeframe/core/phase_manager.py (1)
transition(87-122)codeframe/ui/websocket_broadcasts.py (6)
broadcast_development_started(918-946)broadcast_planning_started(794-813)broadcast_issues_generated(816-838)broadcast_tasks_decomposed(841-863)broadcast_tasks_ready(866-889)broadcast_planning_failed(892-915)
⏰ 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). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (5)
tests/ui/test_task_approval.py (5)
1-16: LGTM!Clear module docstring documenting the test scope and appropriate imports for mocking async functions and testing the approval endpoint.
19-57: LGTM!Well-designed fixtures with clear separation of concerns. The
mock_dbfixture provides a comprehensive setup for the common test scenarios, and theAsyncMockfor broadcast correctly handles async function mocking.
60-191: LGTM!Comprehensive test coverage for the approval endpoint's happy paths. Good use of
call_args_listfiltering to verify specific WebSocket broadcast messages, and clear assertions validating the response structure.
194-289: LGTM!Thorough validation testing covering all error paths. The tests correctly verify both the HTTP status codes and error message content.
292-421: LGTM!Good unit test coverage for all planning-related WebSocket broadcast functions. The timestamp format verification (
endswith("Z")) ensures ISO 8601 compliance for client consumption.
- Add comment explaining excluded tasks remain unchanged for audit trail - Add done_callback to planning background task to log unhandled exceptions (prevents silent failures in asyncio.create_task)
Follow-Up Review: Planning Phase AutomationI've reviewed the latest changes in commit a00ea39 (refactor(planning): Add clarifying comments and exception handling). ✅ Excellent ProgressThe PR has evolved significantly through iterative improvements:
🎯 Current State AssessmentBased on the current diff, the implementation demonstrates: ✅ Non-Blocking Execution: Uses asyncio.create_task() with done callback (discovery.py:147-148) 📊 Final Quality Metrics
💡 Minor Future Enhancements (Non-Blocking)These are optional improvements for future consideration, not required for this PR:
✨ Final VerdictThis PR represents excellent software engineering:
Recommendation: ✅ APPROVED - Ready to merge! The implementation correctly addresses all critical concerns from previous reviews:
Great work on the iterative improvements! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/discovery.py:
- Around line 164-217: The generate_planning_background task can hang because
agent.generate_issues and agent.decompose_prd are called without timeouts; wrap
the asyncio.to_thread(agent.generate_issues, ...) and
asyncio.to_thread(agent.decompose_prd, ...) calls with
asyncio.wait_for(timebound_call, timeout=120) (or a configurable timeout
variable) and handle asyncio.TimeoutError to log the timeout and call await
broadcast_planning_failed(manager, project_id, "timeout") so the function exits
cleanly; update the try/except to catch and distinguish TimeoutError from other
exceptions and ensure task_count/issue_count remain safe if the calls time out.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/ui/routers/discovery.pycodeframe/ui/routers/tasks.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/tasks.pycodeframe/ui/routers/discovery.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
codeframe/ui/routers/tasks.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/ui/routers/discovery.py
🧬 Code graph analysis (2)
codeframe/ui/routers/tasks.py (4)
codeframe/core/phase_manager.py (2)
PhaseManager(37-157)transition(87-122)codeframe/persistence/database.py (1)
Database(51-698)codeframe/ui/dependencies.py (1)
get_db(14-29)codeframe/ui/websocket_broadcasts.py (1)
broadcast_development_started(918-946)
codeframe/ui/routers/discovery.py (1)
codeframe/ui/websocket_broadcasts.py (5)
broadcast_planning_started(794-813)broadcast_issues_generated(816-838)broadcast_tasks_decomposed(841-863)broadcast_tasks_ready(866-889)broadcast_planning_failed(892-915)
⏰ 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: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (15)
codeframe/ui/routers/discovery.py (4)
7-7: LGTM! Clean import additions for planning automation.The asyncio import and planning broadcast functions are properly integrated and used by the new
generate_planning_backgroundfunction.Also applies to: 21-27
137-149: LGTM! Fire-and-forget pattern correctly implements non-blocking planning.The use of
asyncio.create_taskwith a done callback ensures PRD completion is reported immediately while planning runs in the background. The callback logs unhandled exceptions, andgenerate_planning_backgrounditself broadcastsplanning_failedon error, providing proper error visibility.
151-152: Good improvement to error logging.Adding
exc_info=Trueprovides full stack traces for debugging PRD generation failures.
194-194: Verify: Is hardcoded sprint_number=1 sufficient for all use cases?The
generate_issuescall uses a hardcodedsprint_number=1. Confirm this aligns with the product roadmap—if multi-sprint projects are planned, you may need a more flexible approach (e.g., reading from project config or incrementing sprint numbers).codeframe/ui/routers/tasks.py (11)
10-21: Imports are correctly added and used.All new imports (PhaseManager, manager, broadcast_development_started) are properly utilized in the approve_tasks endpoint.
29-30: Good RESTful design for project-scoped operations.The project_router correctly scopes the approval endpoint under
/api/projects/{project_id}/tasks/approve, which aligns with RESTful resource hierarchy.
120-132: Well-designed request/response models.The models use appropriate types and correctly employ
Field(default_factory=list)for the mutable default, following Pydantic best practices.
163-173: Proper project validation and authorization.The validation follows the same pattern as the create_task endpoint, ensuring consistency across the codebase.
175-183: Clear rejection handling with helpful user feedback.The early return pattern prevents unnecessary processing and provides a clear message guiding users on next steps.
185-191: Phase validation ensures correct workflow state.The check prevents task approval when the project isn't in the planning phase, providing idempotency protection and clear error messaging.
193-199: Appropriate task existence validation.The check prevents approval when no tasks exist, with a clear error message guiding users to generate tasks first.
202-206: Verify behavior when all tasks are excluded.If all tasks are excluded, the project will transition to "active" phase with zero pending tasks. While the comment suggests excluded tasks can be re-included later, confirm this is the intended workflow rather than requiring at least one approved task.
208-219: Excellent fail-fast pattern for phase transition.Transitioning the phase before updating tasks ensures atomicity - if the phase change fails, no task state is modified. The error handling correctly distinguishes between validation errors (HTTPException) and unexpected failures.
225-231: Proper WebSocket broadcast integration.The broadcast correctly uses
awaitand passes all required parameters (manager, project_id, approved_count, excluded_count) as defined in the broadcast function signature.
233-244: Good observability and comprehensive response.The structured logging provides clear audit trail, and the response gives the client complete information about the approval outcome.
- Wrap asyncio.to_thread calls with asyncio.wait_for(timeout=...) - Add PLANNING_OPERATION_TIMEOUT env var (default 120 seconds) - Handle asyncio.TimeoutError separately from other exceptions - Broadcast planning_failed with "timed out" message on timeout - Add tests for timeout scenarios on both generate_issues and decompose_prd
Code Review - PR #205: Planning Phase AutomationI've reviewed the latest changes and noticed several improvements have been made since the initial implementation. Here's my assessment: ✅ Strengths & Improvements Already Made
🟡 Minor Observations1. Hardcoded Sprint Number (discovery.py:198)issues = await asyncio.to_thread(agent.generate_issues, sprint_number=1)Note: This is already tracked in issue #210 ( 2. TaskApprovalRequest Model (tasks.py:122)class TaskApprovalRequest(BaseModel):
approved: bool
excluded_task_ids: List[int] = Field(default_factory=list)Question: Should this also accept Suggestion (optional): class TaskApprovalRequest(BaseModel):
approved: bool
approved_task_ids: Optional[List[int]] = None # If None, approve all except excluded
excluded_task_ids: List[int] = Field(default_factory=list)3. Test Coverage Gap: Integration TestsThe unit tests are excellent, but consider adding one end-to-end integration test that verifies:
This would catch integration issues between the different stages. 💡 Optional Enhancements (Future PRs)These are not blockers but could improve the implementation:
📊 Overall AssessmentThis is a well-implemented feature with:
The code follows project conventions from Recommendation: ✅ Approve with minor suggestions The minor observations above are not blockers. They're suggestions for future iterations or clarifications. The current implementation is production-ready. 🎯 Suggested Next Steps
Great work on this feature! The background task execution pattern is solid and the error handling is thorough. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/ui/test_discovery_automation.py (3)
270-299: Timeout test pattern may not accurately simulate real behavior.The
slow_generate_issueshelper is defined as an async function (line 278-280) but is used to patchasyncio.to_thread(line 284). In production,asyncio.to_threadwraps synchronous LeadAgent methods, not async functions.While this test will likely pass, it doesn't accurately simulate the actual execution pattern. Consider using a synchronous function with
time.sleep(10)instead:def slow_generate_issues(*args, **kwargs): import time time.sleep(10) # Blocks the thread return []This would more accurately test the timeout behavior of wrapping a blocking sync call with
asyncio.to_threadandasyncio.wait_for.
302-341: Same timeout test pattern issue in decompose_prd test.Similar to the previous timeout test,
conditional_slowis defined as async (lines 313-322) but used to patchasyncio.to_thread(line 326). Consider using synchronous functions withtime.sleep()for more accurate testing of the actual execution pattern.
343-362: Weak test coverage for PRD completion trigger.This test only verifies the function signature exists with correct parameters but doesn't test:
- That
generate_prd_backgroundactually spawns planning automation viaasyncio.create_task- That the done callback properly logs exceptions
- The integration between PRD completion and planning automation start
Consider adding an integration test that:
- Mocks the complete PRD generation flow
- Verifies that
asyncio.create_taskis called withgenerate_planning_background- Tests that the done callback handles exceptions
codeframe/ui/routers/discovery.py (1)
137-148: Consider using logger.exception() for better traceback capture.The done callback properly logs unhandled exceptions, but the
exc_info=task.exception()pattern (line 144) may not capture the full traceback. Consider this alternative:def _handle_planning_exception(task: asyncio.Task) -> None: """Log any unhandled exceptions from background planning task.""" if not task.cancelled() and task.exception(): exc = task.exception() logger.exception( f"Unhandled exception in planning background task: {exc}" )Using
logger.exception()automatically includes traceback information if available, providing better debugging context.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.beads/issues.jsonlcodeframe/ui/routers/discovery.pytests/ui/test_discovery_automation.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/discovery.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to sprints/sprint-[0-9][0-9]-*.md : Sprint summary files must reference and link to corresponding feature spec directories (specs/{feature}/) and include git commit references and beads issue links, rather than duplicating detailed content
Applied to files:
.beads/issues.jsonl
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
.beads/issues.jsonl
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/ui/routers/discovery.py
🧬 Code graph analysis (1)
tests/ui/test_discovery_automation.py (2)
codeframe/ui/routers/discovery.py (2)
generate_planning_background(164-233)generate_prd_background(35-161)codeframe/agents/lead_agent.py (1)
decompose_prd(1270-1389)
⏰ 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). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (7)
.beads/issues.jsonl (1)
15-15: LGTM! Properly documents known limitation.This issue correctly identifies that
sprint_number=1is hardcoded in the planning automation (actual location: line 198 in discovery.py). This is a valid enhancement to track for multi-sprint support.tests/ui/test_discovery_automation.py (3)
1-14: LGTM! Clear test structure.The module docstring clearly describes the test scope, and imports are appropriate for testing async planning automation with mocks.
17-49: LGTM! Well-structured test fixtures.The fixtures provide appropriate mocks for ConnectionManager, Database, and LeadAgent with consistent return values used throughout the tests.
80-90: Test verifies hardcoded sprint number.This test confirms that
generate_issuesis called withsprint_number=1(hardcoded). This aligns with issuecodeframe-1j32in issues.jsonl, which tracks the need to make sprint number configurable.Note: As per the documented issue, this hardcoded value should eventually be made configurable for multi-sprint support.
codeframe/ui/routers/discovery.py (3)
7-7: LGTM! Necessary imports for planning automation.The added
asyncioimport and planning-related websocket broadcast functions support the new planning automation workflow introduced in this PR.Also applies to: 21-27
164-234: Excellent timeout implementation!This function properly addresses the previous review concern about indefinite hangs:
✅ Configurable timeout via
PLANNING_OPERATION_TIMEOUTenvironment variable (line 184)
✅asyncio.wait_forwraps bothgenerate_issues(lines 197-200) anddecompose_prd(lines 209-212)
✅ Separateasyncio.TimeoutErrorhandling with appropriate error messages (lines 223-229)
✅ Planning failure broadcast on timeout with clear error messageThe implementation follows best practices for async timeout handling and provides good observability through WebSocket broadcasts at each stage.
Note: Line 198 hardcodes
sprint_number=1, which is documented as a known limitation in issuecodeframe-1j32.
195-200: Hardcoded sprint number is a known limitation.Line 198 hardcodes
sprint_number=1, which limits planning automation to single-sprint scenarios. This is documented in issuecodeframe-1j32and should be addressed in future work to support multi-sprint projects.For now, this is acceptable as it enables the initial planning automation workflow. The issue is properly tracked for future enhancement.
Note: Based on the documented issue, this limitation should eventually be resolved to derive sprint number from project state or make it configurable.
1 participant
Summary
generate_issues()anddecompose_prd())POST /api/projects/{id}/tasks/approve)Test plan
WebSocket Events Added
planning_startedissues_generatedLeadAgent.generate_issues()completestasks_decomposedLeadAgent.decompose_prd()completestasks_readyplanning_faileddevelopment_startedFiles Changed
codeframe/ui/routers/discovery.py- Addedgenerate_planning_background()functioncodeframe/ui/routers/tasks.py- Added task approval endpoint and modelscodeframe/ui/websocket_broadcasts.py- Added 6 broadcast functionscodeframe/ui/server.py- Registered project_routertests/ui/test_discovery_automation.py- 11 new teststests/ui/test_task_approval.py- 16 new testsSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.