Skip to content

Commit 7d5caab

Browse files
authored
fix: address claude-review feedback on blocker origins (#487)
## Summary Follow-up fixes after #530 was merged without reading claude-review output: - System guidance text generalized to cover stall-timeout + PRD discovery pause - `_REASON_STALL_DETECTED` constant extracted (eliminates 5 magic string usages) - Migration ALTER TABLE test added to cover the actual schema migration path - `BlockerResponse.created_by` typed as `BlockerOrigin` enum for Pydantic validation + OpenAPI docs - `blockers.create()` parameter typed as `Union[BlockerOrigin, str]` ## Validation - All CI checks green including claude-review and CodeRabbit - 9 Python tests, 20 React tests passing, no regressions - SQLite serialization confirmed: INSERT uses `origin.value` (stores plain string, not enum repr)
1 parent 787add1 commit 7d5caab

6 files changed

Lines changed: 61 additions & 12 deletions

File tree

codeframe/core/blockers.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from dataclasses import dataclass
1111
from datetime import datetime, timezone
1212
from enum import Enum
13-
from typing import Optional
13+
from typing import Optional, Union
1414

1515
from codeframe.core.workspace import Workspace, get_db_connection
1616
from codeframe.core import events, runtime, tasks
@@ -69,7 +69,7 @@ def create(
6969
workspace: Workspace,
7070
question: str,
7171
task_id: Optional[str] = None,
72-
created_by: str = "human",
72+
created_by: Union[BlockerOrigin, str] = BlockerOrigin.HUMAN,
7373
) -> Blocker:
7474
"""Create a new blocker.
7575

codeframe/core/react_agent.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@
5151
PRESERVE_RECENT_PAIRS = 5
5252
DEFAULT_CONTEXT_WINDOW = 200_000 # All Claude 4.x models
5353

54+
# Reason string emitted when a stall timeout triggers a blocker — used to
55+
# set the correct BlockerOrigin ("system") vs agent-generated blockers.
56+
_REASON_STALL_DETECTED = "stall_detected"
57+
5458
# Map tool names to agent phases for progress reporting.
5559
_TOOL_PHASE_MAP = {
5660
"read_file": AgentPhase.EXPLORING,
@@ -220,7 +224,7 @@ def run(self, task_id: str) -> AgentStatus:
220224
try:
221225
status = self._react_loop(system_prompt)
222226
if status == AgentStatus.FAILED:
223-
reason = "stall_detected" if self._stall_triggered.is_set() else "max_iterations_reached"
227+
reason = _REASON_STALL_DETECTED if self._stall_triggered.is_set() else "max_iterations_reached"
224228
self._emit(EventType.AGENT_FAILED, {
225229
"task_id": task_id,
226230
"reason": reason,
@@ -244,7 +248,7 @@ def run(self, task_id: str) -> AgentStatus:
244248
self._emit_stream_completion(task_id)
245249
return AgentStatus.COMPLETED
246250

247-
if reason == "escalated_to_blocker" or reason == "stall_detected":
251+
if reason == "escalated_to_blocker" or reason == _REASON_STALL_DETECTED:
248252
self._emit(EventType.AGENT_FAILED, {
249253
"task_id": task_id,
250254
"reason": "blocked",
@@ -433,7 +437,7 @@ def _react_loop(self, system_prompt: str) -> AgentStatus:
433437
# StallAction.BLOCKER (default)
434438
self._create_text_blocker(
435439
stall_ctx or "Agent stalled with no tool activity",
436-
"stall_detected",
440+
_REASON_STALL_DETECTED,
437441
)
438442
return AgentStatus.BLOCKED
439443

@@ -689,7 +693,7 @@ def _run_final_verification(
689693
)
690694
elif self._stall_action == StallAction.FAIL:
691695
return (False, "stall_failed")
692-
return (False, "stall_detected")
696+
return (False, _REASON_STALL_DETECTED)
693697

694698
self._verbose_print("[ReactAgent] Running final verification...")
695699
self._emit_progress(
@@ -1209,7 +1213,7 @@ def _create_text_blocker(self, text: str, reason: str) -> None:
12091213
f"Agent detected a blocker: {reason}\n\n"
12101214
f"Context:\n{text[:500]}"
12111215
)
1212-
origin = "system" if reason == "stall_detected" else "agent"
1216+
origin = "system" if reason == _REASON_STALL_DETECTED else "agent"
12131217
blocker = blockers.create(
12141218
workspace=self.workspace,
12151219
question=question,

codeframe/ui/routers/blockers_v2.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
from codeframe.core.workspace import Workspace
2121
from codeframe.lib.rate_limiter import rate_limit_standard
2222
from codeframe.core import blockers
23-
from codeframe.core.blockers import BlockerStatus
23+
from codeframe.core.blockers import BlockerStatus, BlockerOrigin
2424
from codeframe.ui.dependencies import get_v2_workspace
2525
from codeframe.ui.response_models import api_error, ErrorCodes
2626

@@ -45,7 +45,7 @@ class BlockerResponse(BaseModel):
4545
status: str
4646
created_at: str
4747
answered_at: Optional[str]
48-
created_by: str = "human"
48+
created_by: BlockerOrigin = BlockerOrigin.HUMAN
4949

5050

5151
class BlockerListResponse(BaseModel):

tests/core/test_blocker_origin.py

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def test_list_all_includes_created_by(self, workspace):
6161

6262
class TestExistingBlockersMigration:
6363
def test_blockers_without_created_by_default_to_human(self, workspace):
64-
"""Simulate a pre-migration blocker row with no created_by value."""
64+
"""COALESCE fallback: rows inserted without created_by read back as HUMAN."""
6565
from codeframe.core.workspace import get_db_connection
6666
import uuid
6767
from datetime import datetime, timezone
@@ -82,3 +82,48 @@ def test_blockers_without_created_by_default_to_human(self, workspace):
8282
fetched = blockers.get(workspace, old_id)
8383
assert fetched is not None
8484
assert fetched.created_by == BlockerOrigin.HUMAN
85+
86+
def test_alter_table_migration_adds_created_by_column(self, tmp_path: Path):
87+
"""ALTER TABLE migration: initializing a DB without created_by column adds it."""
88+
import sqlite3
89+
from codeframe.core.workspace import _init_database, CODEFRAME_DIR, STATE_DB_NAME
90+
91+
# Create a DB with the old blockers schema (no created_by column)
92+
repo = tmp_path / "old-repo"
93+
repo.mkdir()
94+
codeframe_dir = repo / CODEFRAME_DIR
95+
codeframe_dir.mkdir()
96+
db_path = codeframe_dir / STATE_DB_NAME
97+
98+
conn = sqlite3.connect(str(db_path))
99+
conn.execute("""
100+
CREATE TABLE IF NOT EXISTS blockers (
101+
id TEXT PRIMARY KEY,
102+
workspace_id TEXT NOT NULL,
103+
task_id TEXT,
104+
question TEXT NOT NULL,
105+
answer TEXT,
106+
status TEXT NOT NULL DEFAULT 'OPEN',
107+
created_at TEXT NOT NULL,
108+
answered_at TEXT
109+
)
110+
""")
111+
conn.commit()
112+
conn.close()
113+
114+
# Confirm column is absent before migration
115+
conn = sqlite3.connect(str(db_path))
116+
cursor = conn.execute("PRAGMA table_info(blockers)")
117+
columns_before = {row[1] for row in cursor.fetchall()}
118+
conn.close()
119+
assert "created_by" not in columns_before
120+
121+
# Run the full init (triggers the ALTER TABLE migration)
122+
_init_database(db_path)
123+
124+
# Confirm column is present after migration
125+
conn = sqlite3.connect(str(db_path))
126+
cursor = conn.execute("PRAGMA table_info(blockers)")
127+
columns_after = {row[1] for row in cursor.fetchall()}
128+
conn.close()
129+
assert "created_by" in columns_after

web-ui/__tests__/components/blockers/BlockerCard.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ describe('BlockerCard', () => {
259259
render(
260260
<BlockerCard blocker={makeBlocker({ created_by: 'system' })} workspacePath={workspacePath} onAnswered={onAnswered} />
261261
);
262-
expect(screen.getByText('Agent was inactive for too long. Answer to continue or retry execution.')).toBeInTheDocument();
262+
expect(screen.getByText('System-initiated pause. Review the context and answer to continue.')).toBeInTheDocument();
263263
});
264264
});
265265
});

web-ui/src/components/blockers/BlockerCard.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ const ORIGIN_CONFIG: Record<BlockerOrigin, {
2727
label: 'System',
2828
Icon: Settings01Icon,
2929
badgeClass: 'bg-blue-100 text-blue-800 dark:bg-blue-950/40 dark:text-blue-300',
30-
guidance: 'Agent was inactive for too long. Answer to continue or retry execution.',
30+
guidance: 'System-initiated pause. Review the context and answer to continue.',
3131
},
3232
agent: {
3333
label: 'Agent',

0 commit comments

Comments
 (0)