mirror of
https://github.com/leonvanzyl/autocoder.git
synced 2026-01-30 06:12:06 +00:00
fix: address code review feedback from coderabbitai
- Add language specifier to fenced code block in expand-project.md - Remove detailed exception strings from WebSocket responses (security) - Make WebSocket "start" message idempotent to avoid session reset - Fix race condition in bulk feature creation with row-level lock - Add validation for starting_priority (must be >= 1) - Fix _query_claude to handle multiple feature blocks and deduplicate - Add FileReader error handling in ExpandProjectChat - Fix disconnect() to clear pending reconnect timeout - Enable sandbox mode and validate CLI path in expand_chat_session - Clean up temporary settings file on session close Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -144,7 +144,7 @@ Once the user approves, create features directly.
|
||||
|
||||
**Then output the features in this exact JSON format (the system will parse this):**
|
||||
|
||||
```
|
||||
```json
|
||||
<features_to_create>
|
||||
[
|
||||
{
|
||||
|
||||
@@ -161,12 +161,22 @@ async def expand_project_websocket(websocket: WebSocket, project_name: str):
|
||||
continue
|
||||
|
||||
elif msg_type == "start":
|
||||
# Create and start a new expansion session
|
||||
session = await create_expand_session(project_name, project_dir)
|
||||
# Check if session already exists (idempotent start)
|
||||
existing_session = get_expand_session(project_name)
|
||||
if existing_session:
|
||||
session = existing_session
|
||||
await websocket.send_json({
|
||||
"type": "text",
|
||||
"content": "Resuming existing expansion session. What would you like to add?"
|
||||
})
|
||||
await websocket.send_json({"type": "response_done"})
|
||||
else:
|
||||
# Create and start a new expansion session
|
||||
session = await create_expand_session(project_name, project_dir)
|
||||
|
||||
# Stream the initial greeting
|
||||
async for chunk in session.start():
|
||||
await websocket.send_json(chunk)
|
||||
# Stream the initial greeting
|
||||
async for chunk in session.start():
|
||||
await websocket.send_json(chunk)
|
||||
|
||||
elif msg_type == "message":
|
||||
# User sent a message
|
||||
@@ -192,7 +202,7 @@ async def expand_project_websocket(websocket: WebSocket, project_name: str):
|
||||
logger.warning(f"Invalid attachment data: {e}")
|
||||
await websocket.send_json({
|
||||
"type": "error",
|
||||
"content": f"Invalid attachment: {str(e)}"
|
||||
"content": "Invalid attachment format"
|
||||
})
|
||||
continue
|
||||
|
||||
@@ -236,7 +246,7 @@ async def expand_project_websocket(websocket: WebSocket, project_name: str):
|
||||
try:
|
||||
await websocket.send_json({
|
||||
"type": "error",
|
||||
"content": f"Server error: {str(e)}"
|
||||
"content": "Internal server error"
|
||||
})
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
@@ -305,7 +305,7 @@ async def create_features_bulk(project_name: str, bulk: FeatureBulkCreate):
|
||||
Create multiple features at once.
|
||||
|
||||
Features are assigned sequential priorities starting from:
|
||||
- starting_priority if specified
|
||||
- starting_priority if specified (must be >= 1)
|
||||
- max(existing priorities) + 1 if not specified
|
||||
|
||||
This is useful for:
|
||||
@@ -328,18 +328,28 @@ async def create_features_bulk(project_name: str, bulk: FeatureBulkCreate):
|
||||
if not bulk.features:
|
||||
return FeatureBulkCreateResponse(created=0, features=[])
|
||||
|
||||
# Validate starting_priority if provided
|
||||
if bulk.starting_priority is not None and bulk.starting_priority < 1:
|
||||
raise HTTPException(status_code=400, detail="starting_priority must be >= 1")
|
||||
|
||||
_, Feature = _get_db_classes()
|
||||
|
||||
try:
|
||||
with get_db_session(project_dir) as session:
|
||||
# Determine starting priority
|
||||
# Determine starting priority with row-level lock to prevent race conditions
|
||||
if bulk.starting_priority is not None:
|
||||
current_priority = bulk.starting_priority
|
||||
else:
|
||||
max_priority_feature = session.query(Feature).order_by(Feature.priority.desc()).first()
|
||||
# Lock the max priority row to prevent concurrent inserts from getting same priority
|
||||
max_priority_feature = (
|
||||
session.query(Feature)
|
||||
.order_by(Feature.priority.desc())
|
||||
.with_for_update()
|
||||
.first()
|
||||
)
|
||||
current_priority = (max_priority_feature.priority + 1) if max_priority_feature else 1
|
||||
|
||||
created_features = []
|
||||
created_ids = []
|
||||
|
||||
for feature_data in bulk.features:
|
||||
db_feature = Feature(
|
||||
@@ -351,20 +361,16 @@ async def create_features_bulk(project_name: str, bulk: FeatureBulkCreate):
|
||||
passes=False,
|
||||
)
|
||||
session.add(db_feature)
|
||||
session.flush() # Flush to get the ID immediately
|
||||
created_ids.append(db_feature.id)
|
||||
current_priority += 1
|
||||
|
||||
session.commit()
|
||||
|
||||
# Refresh to get IDs and return responses
|
||||
for db_feature in session.query(Feature).order_by(Feature.priority.desc()).limit(len(bulk.features)).all():
|
||||
created_features.insert(0, feature_to_response(db_feature))
|
||||
|
||||
# Re-query to get the actual created features in order
|
||||
# Query created features by their IDs (avoids relying on priority range)
|
||||
created_features = []
|
||||
start_priority = current_priority - len(bulk.features)
|
||||
for db_feature in session.query(Feature).filter(
|
||||
Feature.priority >= start_priority,
|
||||
Feature.priority < current_priority
|
||||
Feature.id.in_(created_ids)
|
||||
).order_by(Feature.priority).all():
|
||||
created_features.append(feature_to_response(db_feature))
|
||||
|
||||
|
||||
@@ -67,6 +67,7 @@ class ExpandChatSession:
|
||||
self._client_entered: bool = False
|
||||
self.features_created: int = 0
|
||||
self.created_feature_ids: list[int] = []
|
||||
self._settings_file: Optional[Path] = None
|
||||
|
||||
async def close(self) -> None:
|
||||
"""Clean up resources and close the Claude client."""
|
||||
@@ -79,6 +80,13 @@ class ExpandChatSession:
|
||||
self._client_entered = False
|
||||
self.client = None
|
||||
|
||||
# Clean up temporary settings file
|
||||
if self._settings_file and self._settings_file.exists():
|
||||
try:
|
||||
self._settings_file.unlink()
|
||||
except Exception as e:
|
||||
logger.warning(f"Error removing settings file: {e}")
|
||||
|
||||
async def start(self) -> AsyncGenerator[dict, None]:
|
||||
"""
|
||||
Initialize session and get initial greeting from Claude.
|
||||
@@ -111,7 +119,7 @@ class ExpandChatSession:
|
||||
|
||||
# Create security settings file
|
||||
security_settings = {
|
||||
"sandbox": {"enabled": False},
|
||||
"sandbox": {"enabled": True},
|
||||
"permissions": {
|
||||
"defaultMode": "acceptEdits",
|
||||
"allow": [
|
||||
@@ -121,6 +129,7 @@ class ExpandChatSession:
|
||||
},
|
||||
}
|
||||
settings_file = self.project_dir / ".claude_settings.json"
|
||||
self._settings_file = settings_file
|
||||
with open(settings_file, "w") as f:
|
||||
json.dump(security_settings, f, indent=2)
|
||||
|
||||
@@ -128,8 +137,14 @@ class ExpandChatSession:
|
||||
project_path = str(self.project_dir.resolve())
|
||||
system_prompt = skill_content.replace("$ARGUMENTS", project_path)
|
||||
|
||||
# Create Claude SDK client
|
||||
# Find and validate Claude CLI
|
||||
system_cli = shutil.which("claude")
|
||||
if not system_cli:
|
||||
yield {
|
||||
"type": "error",
|
||||
"content": "Claude CLI not found. Please install Claude Code."
|
||||
}
|
||||
return
|
||||
try:
|
||||
self.client = ClaudeSDKClient(
|
||||
options=ClaudeAgentOptions(
|
||||
@@ -268,20 +283,35 @@ class ExpandChatSession:
|
||||
"timestamp": datetime.now().isoformat()
|
||||
})
|
||||
|
||||
# Check for feature creation block in full response
|
||||
features_match = re.search(
|
||||
# Check for feature creation blocks in full response (handle multiple blocks)
|
||||
features_matches = re.findall(
|
||||
r'<features_to_create>\s*(\[[\s\S]*?\])\s*</features_to_create>',
|
||||
full_response
|
||||
)
|
||||
|
||||
if features_match:
|
||||
try:
|
||||
features_json = features_match.group(1)
|
||||
features_data = json.loads(features_json)
|
||||
if features_matches:
|
||||
# Collect all features from all blocks, deduplicating by name
|
||||
all_features: list[dict] = []
|
||||
seen_names: set[str] = set()
|
||||
|
||||
if features_data and isinstance(features_data, list):
|
||||
# Create features via REST API
|
||||
created = await self._create_features_bulk(features_data)
|
||||
for features_json in features_matches:
|
||||
try:
|
||||
features_data = json.loads(features_json)
|
||||
|
||||
if features_data and isinstance(features_data, list):
|
||||
for feature in features_data:
|
||||
name = feature.get("name", "")
|
||||
if name and name not in seen_names:
|
||||
seen_names.add(name)
|
||||
all_features.append(feature)
|
||||
except json.JSONDecodeError as e:
|
||||
logger.error(f"Failed to parse features JSON block: {e}")
|
||||
# Continue processing other blocks
|
||||
|
||||
if all_features:
|
||||
try:
|
||||
# Create all deduplicated features
|
||||
created = await self._create_features_bulk(all_features)
|
||||
|
||||
if created:
|
||||
self.features_created += len(created)
|
||||
@@ -294,18 +324,12 @@ class ExpandChatSession:
|
||||
}
|
||||
|
||||
logger.info(f"Created {len(created)} features for {self.project_name}")
|
||||
except json.JSONDecodeError as e:
|
||||
logger.error(f"Failed to parse features JSON: {e}")
|
||||
yield {
|
||||
"type": "error",
|
||||
"content": f"Failed to parse feature definitions: {str(e)}"
|
||||
}
|
||||
except Exception as e:
|
||||
logger.exception("Failed to create features")
|
||||
yield {
|
||||
"type": "error",
|
||||
"content": f"Failed to create features: {str(e)}"
|
||||
}
|
||||
except Exception as e:
|
||||
logger.exception("Failed to create features")
|
||||
yield {
|
||||
"type": "error",
|
||||
"content": "Failed to create features"
|
||||
}
|
||||
|
||||
async def _create_features_bulk(self, features: list[dict]) -> list[dict]:
|
||||
"""
|
||||
|
||||
@@ -121,6 +121,9 @@ export function ExpandProjectChat({
|
||||
|
||||
setPendingAttachments((prev) => [...prev, attachment])
|
||||
}
|
||||
reader.onerror = () => {
|
||||
setError(`Failed to read file: ${file.name}`)
|
||||
}
|
||||
reader.readAsDataURL(file)
|
||||
})
|
||||
}, [])
|
||||
|
||||
@@ -302,6 +302,10 @@ export function useExpandChat({
|
||||
clearInterval(pingIntervalRef.current)
|
||||
pingIntervalRef.current = null
|
||||
}
|
||||
if (reconnectTimeoutRef.current) {
|
||||
clearTimeout(reconnectTimeoutRef.current)
|
||||
reconnectTimeoutRef.current = null
|
||||
}
|
||||
if (wsRef.current) {
|
||||
wsRef.current.close()
|
||||
wsRef.current = null
|
||||
|
||||
Reference in New Issue
Block a user