Skip to content

Commit 121dc59

Browse files
author
Test User
committed
fix(#247): address code review feedback from CodeRabbit
Fixes: - Fix critical bug: use self.answer_capture (not self._answer_capture) in full_reset_discovery - Add clear() method to AnswerCapture class for proper state reset - Add thread safety lock (_sync_lock) in delete_project_tasks_and_issues - Update Dialog onOpenChange handler to call handleCancelRestart when closed - Update phase_manager.py docstring to document discovery restart transitions - Remove unused api_client fixture from TestPhaseManagerTransitions tests
1 parent 7f739f9 commit 121dc59

File tree

6 files changed

+48
-39
lines changed

6 files changed

+48
-39
lines changed

codeframe/agents/lead_agent.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -751,9 +751,9 @@ def full_reset_discovery(self) -> dict:
751751
self._category_coverage = {}
752752
self._coverage_turn_count = 0
753753

754-
# Clear DiscoveryAnswerCapture state if it exists
755-
if hasattr(self, "_answer_capture") and self._answer_capture:
756-
self._answer_capture.clear()
754+
# Clear AnswerCapture state if it exists
755+
if hasattr(self, "answer_capture") and self.answer_capture:
756+
self.answer_capture.clear()
757757

758758
# Reset state in database
759759
self.db.upsert_memory(

codeframe/core/phase_manager.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ class PhaseManager:
4444
Phase Transition Rules:
4545
discovery → planning (forward only)
4646
planning → active, discovery (forward or back)
47-
active → review, planning (forward or back)
48-
review → complete, active (forward or back)
47+
active → review, planning, discovery (forward or back to discovery for restart)
48+
review → complete, active, discovery (forward or back to discovery for restart)
4949
complete → (terminal state)
5050
5151
Example:

codeframe/discovery/answers.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,14 @@ def __init__(self) -> None:
2222
"""Initialize the AnswerCapture instance."""
2323
self.answers: dict[str, dict[str, Any]] = {}
2424

25+
def clear(self) -> None:
26+
"""Clear all captured answers.
27+
28+
Resets the internal state to an empty dictionary, effectively
29+
clearing all previously captured answers.
30+
"""
31+
self.answers = {}
32+
2533
def capture_answer(self, question_id: str, answer_text: str) -> bool:
2634
"""Capture an answer for a specific question.
2735

codeframe/persistence/database.py

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -730,36 +730,37 @@ def delete_project_tasks_and_issues(self, project_id: int) -> dict:
730730
Returns:
731731
Dictionary with counts: {"tasks": int, "issues": int}
732732
"""
733-
cursor = self.conn.cursor()
734-
735-
# Count tasks before deletion
736-
cursor.execute(
737-
"SELECT COUNT(*) FROM tasks WHERE project_id = ?",
738-
(project_id,),
739-
)
740-
task_count = cursor.fetchone()[0]
741-
742-
# Count issues before deletion
743-
cursor.execute(
744-
"SELECT COUNT(*) FROM issues WHERE project_id = ?",
745-
(project_id,),
746-
)
747-
issue_count = cursor.fetchone()[0]
748-
749-
# Delete tasks first (due to FK constraint on issue_id)
750-
cursor.execute(
751-
"DELETE FROM tasks WHERE project_id = ?",
752-
(project_id,),
753-
)
754-
755-
# Then delete issues
756-
cursor.execute(
757-
"DELETE FROM issues WHERE project_id = ?",
758-
(project_id,),
759-
)
760-
761-
self.conn.commit()
762-
return {"tasks": task_count, "issues": issue_count}
733+
with self._sync_lock:
734+
cursor = self.conn.cursor()
735+
736+
# Count tasks before deletion
737+
cursor.execute(
738+
"SELECT COUNT(*) FROM tasks WHERE project_id = ?",
739+
(project_id,),
740+
)
741+
task_count = cursor.fetchone()[0]
742+
743+
# Count issues before deletion
744+
cursor.execute(
745+
"SELECT COUNT(*) FROM issues WHERE project_id = ?",
746+
(project_id,),
747+
)
748+
issue_count = cursor.fetchone()[0]
749+
750+
# Delete tasks first (due to FK constraint on issue_id)
751+
cursor.execute(
752+
"DELETE FROM tasks WHERE project_id = ?",
753+
(project_id,),
754+
)
755+
756+
# Then delete issues
757+
cursor.execute(
758+
"DELETE FROM issues WHERE project_id = ?",
759+
(project_id,),
760+
)
761+
762+
self.conn.commit()
763+
return {"tasks": task_count, "issues": issue_count}
763764

764765
def create_audit_log(self, *args, **kwargs):
765766
"""Delegate to audit_logs.create_audit_log()."""

tests/api/test_discovery_restart.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -471,19 +471,19 @@ def test_full_reset_resets_answer_capture_state(self, mock_provider_class, api_c
471471
class TestPhaseManagerTransitions:
472472
"""Test phase manager allows transitions back to discovery from any phase."""
473473

474-
def test_active_to_discovery_transition_allowed(self, api_client):
474+
def test_active_to_discovery_transition_allowed(self):
475475
"""Test transition from active to discovery is allowed."""
476476
from codeframe.core.phase_manager import PhaseManager
477477

478478
assert PhaseManager.can_transition("active", "discovery") is True
479479

480-
def test_review_to_discovery_transition_allowed(self, api_client):
480+
def test_review_to_discovery_transition_allowed(self):
481481
"""Test transition from review to discovery is allowed."""
482482
from codeframe.core.phase_manager import PhaseManager
483483

484484
assert PhaseManager.can_transition("review", "discovery") is True
485485

486-
def test_planning_to_discovery_transition_already_allowed(self, api_client):
486+
def test_planning_to_discovery_transition_already_allowed(self):
487487
"""Test transition from planning to discovery is already allowed."""
488488
from codeframe.core.phase_manager import PhaseManager
489489

web-ui/src/components/DiscoveryProgress.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1104,7 +1104,7 @@ const DiscoveryProgress = memo(function DiscoveryProgress({ projectId, onViewPRD
11041104
)}
11051105

11061106
{/* Restart Discovery Confirmation Dialog (Issue #247) */}
1107-
<Dialog open={showRestartConfirmation} onOpenChange={setShowRestartConfirmation}>
1107+
<Dialog open={showRestartConfirmation} onOpenChange={(open) => { if (!open) handleCancelRestart(); }}>
11081108
<DialogContent className="sm:max-w-[425px]">
11091109
<DialogHeader>
11101110
<div className="flex items-center gap-2">

0 commit comments

Comments
 (0)