feat: improve skill uploads and agent runtime stability

offline-backend-20260512/backend/app/gateway/routers/skills.py

@@ -272,6 +272,14 @@ class SkillUploadValidateResponse(BaseModel):
     allowed_tools: list[str] = Field(default_factory=list, description="Tools the skill may invoke")
     conflict: bool = Field(default=False, description="True if a skill with the same name already exists")
     conflict_kind: str | None = Field(default=None, description="'public' or 'custom' when conflict=true")
+    can_install_as_is: bool = Field(default=True, description="Whether the archive can be installed without overwrite")
+    can_overwrite: bool = Field(default=False, description="Whether the caller may overwrite the conflicting skill")
+    owned_by_me: bool = Field(default=False, description="True when the caller owns the conflicting custom skill")
+    owned_by_other: bool = Field(default=False, description="True when another user owns the conflicting custom skill")
+    builtin_conflict: bool = Field(default=False, description="True when the name conflicts with a built-in/public skill")
+    owner_username: str | None = Field(default=None, description="Friendly username for a conflicting skill owned by another user")
+    owner_user_id: str | None = Field(default=None, description="Owner user id for a conflicting custom skill")
+    message: str = Field(default="", description="Human-readable upload guidance")
     warnings: list[str] = Field(default_factory=list, description="Non-fatal warnings to surface to the user")
 
 
@@ -972,16 +980,55 @@ async def _persist_uploaded_skill(file: UploadFile) -> Path:
     "/skills/validate",
     response_model=SkillUploadValidateResponse,
     summary="Validate Skill Archive (No Install)",
-    description="Inspect an uploaded .zip/.skill archive and report parsed metadata + conflicts. Does NOT install or run the LLM security scan.",
+    description="Inspect an uploaded .zip/.skill archive and report parsed metadata plus ownership-aware conflicts. Does NOT install or run the LLM security scan.",
 )
 async def validate_skill_upload(
+    request: Request,
     file: UploadFile = File(..., description="The .zip or .skill archive to validate"),
+    target_category: SkillCategory = SkillCategory.CUSTOM,
     config: AppConfig = Depends(get_config),
+    skill_store: SkillStore = Depends(get_skill_store),
 ) -> SkillUploadValidateResponse:
     tmp_path = await _persist_uploaded_skill(file)
     try:
         result = get_or_new_skill_storage(app_config=config).validate_skill_archive(tmp_path)
-        return SkillUploadValidateResponse(**result)
+        (
+            can_install_as_is,
+            can_overwrite,
+            owned_by_me,
+            owned_by_other,
+            builtin_conflict,
+            owner_user_id,
+            owner_username,
+            message,
+        ) = await _preview_install_target(
+            request=request,
+            skill_store=skill_store,
+            skill_name=str(result.get("skill_name") or ""),
+            target_category=target_category,
+            conflict_kind=result.get("conflict_kind"),
+        )
+        effective_conflict_kind = result.get("conflict_kind")
+        if effective_conflict_kind is None:
+            if owned_by_me or owned_by_other:
+                effective_conflict_kind = "custom"
+            elif builtin_conflict:
+                effective_conflict_kind = "public"
+        return SkillUploadValidateResponse(
+            **{
+                **result,
+                "conflict": bool(result.get("conflict") or effective_conflict_kind),
+                "conflict_kind": effective_conflict_kind,
+                "can_install_as_is": can_install_as_is,
+                "can_overwrite": can_overwrite,
+                "owned_by_me": owned_by_me,
+                "owned_by_other": owned_by_other,
+                "builtin_conflict": builtin_conflict,
+                "owner_username": owner_username,
+                "owner_user_id": owner_user_id,
+                "message": message,
+            }
+        )
     except ValueError as e:
         raise HTTPException(status_code=400, detail=str(e))
     except FileNotFoundError as e:
@@ -999,7 +1046,7 @@ async def validate_skill_upload(
     "/skills/install-upload",
     response_model=SkillInstallResponse,
     summary="Install Skill from Upload",
-    description="Install a skill directly from an uploaded .zip/.skill archive. Set ?overwrite=true to replace an existing skill with the same name.",
+    description="Install a skill directly from an uploaded .zip/.skill archive. Set ?overwrite=true to replace the caller's own custom skill, or an existing public skill when installing as an admin.",
 )
 async def install_skill_upload(
     request: Request,
@@ -1017,22 +1064,30 @@ async def install_skill_upload(
     tmp_path = await _persist_uploaded_skill(file)
     try:
         storage = get_or_new_skill_storage(app_config=config)
-        # Pre-parse the archive to learn the skill name without writing anything,
-        # so we can reject the upload before touching the filesystem if another
-        # user already owns this name. Admins bypass the ownership check since
-        # they install to the shared public directory.
-        try:
-            validation = storage.validate_skill_archive(tmp_path)
-            preview_name = validation.get("skill_name") if isinstance(validation, dict) else None
-        except Exception:
-            preview_name = None
-        if preview_name and not is_admin:
-            existing = await skill_store.get_any(preview_name)
-            if existing and existing.get("owner_user_id") not in (None, user_id):
-                raise HTTPException(
-                    status_code=409,
-                    detail=f"Skill '{preview_name}' already exists and is owned by another user",
-                )
+        # Re-run the same ownership-aware preflight used by /skills/validate.
+        # This closes the time-of-check/time-of-use gap and prevents a crafted
+        # request (including an admin installing into custom/) from replacing a
+        # different user's skill.
+        validation = storage.validate_skill_archive(tmp_path)
+        preview_name = str(validation.get("skill_name") or "")
+        (
+            can_install_as_is,
+            can_overwrite,
+            _owned_by_me,
+            _owned_by_other,
+            _builtin_conflict,
+            _owner_user_id,
+            _owner_username,
+            conflict_message,
+        ) = await _preview_install_target(
+            request=request,
+            skill_store=skill_store,
+            skill_name=preview_name,
+            target_category=target_category,
+            conflict_kind=validation.get("conflict_kind"),
+        )
+        if (overwrite and not can_install_as_is and not can_overwrite) or (not overwrite and not can_install_as_is):
+            raise HTTPException(status_code=409, detail=conflict_message)
         result = await storage.ainstall_skill_from_archive(tmp_path, overwrite=overwrite, require_skill_suffix=False, target_category=target_category)
         try:
             from deerflow.skills.usage import mark_uploaded
@@ -1075,7 +1130,8 @@ async def _preview_install_target(
     skill_store: SkillStore,
     skill_name: str,
     target_category: SkillCategory,
-) -> tuple[bool, bool, bool, bool, str | None, str | None, str]:
+    conflict_kind: str | None = None,
+) -> tuple[bool, bool, bool, bool, bool, str | None, str | None, str]:
     """Return installability / ownership info for a parsed skill name."""
 
     user_id = _current_user_id(request)
@@ -1085,27 +1141,44 @@ async def _preview_install_target(
     owner_username: str | None = None
     if owner_user_id:
         try:
-            attached = await _attach_owner_usernames(
-                [SkillResponse(name=skill_name, description="", license=None, category=SkillCategory.CUSTOM, owner_user_id=owner_user_id)]
-            )
+            attached = await _attach_owner_usernames([SkillResponse(name=skill_name, description="", license=None, category=SkillCategory.CUSTOM, owner_user_id=owner_user_id)])
             owner_username = attached[0].owner_username if attached else None
         except Exception:
             owner_username = None
 
-    builtin_conflict = bool(existing and owner_user_id is None)
-    owned_by_me = bool(existing and owner_user_id == user_id)
-    owned_by_other = bool(existing and owner_user_id not in (None, user_id))
+    custom_conflict = conflict_kind == "custom" or bool(existing and owner_user_id is not None)
+    builtin_conflict = conflict_kind == "public" or bool(existing and owner_user_id is None and conflict_kind != "custom")
+    owned_by_me = bool(custom_conflict and owner_user_id == user_id)
+    owned_by_other = bool(custom_conflict and owner_user_id not in (None, user_id))
+    unattributed_custom_conflict = bool(custom_conflict and owner_user_id is None and not builtin_conflict)
 
     if target_category == SkillCategory.PUBLIC and not is_admin:
-        return False, owned_by_me, owned_by_other, builtin_conflict, owner_user_id, owner_username, "Only admin users can install skills to the public directory"
+        return False, False, owned_by_me, owned_by_other, builtin_conflict, owner_user_id, owner_username, "Only admin users can install skills to the public directory"
+
+    if target_category == SkillCategory.PUBLIC:
+        if custom_conflict:
+            who = owner_username or owner_user_id
+            if owned_by_other and who:
+                message = f"Skill '{skill_name}' has already been uploaded by user '{who}' and cannot be overwritten."
+            elif owned_by_me:
+                message = f"You already own custom skill '{skill_name}'. Switch the target to custom to overwrite it."
+            else:
+                message = f"Skill '{skill_name}' already exists as a custom skill and its owner could not be determined."
+            return False, False, owned_by_me, owned_by_other, builtin_conflict, owner_user_id, owner_username, message
+        if builtin_conflict:
+            return False, True, False, False, True, owner_user_id, owner_username, f"Built-in/public skill '{skill_name}' already exists and can be overwritten by an admin."
+        return True, False, False, False, False, owner_user_id, owner_username, f"Skill '{skill_name}' can be installed."
+
     if builtin_conflict:
-        return False, owned_by_me, owned_by_other, True, owner_user_id, owner_username, f"Skill '{skill_name}' already exists as a built-in/public skill. Please install it under a different name."
+        return False, False, owned_by_me, owned_by_other, True, owner_user_id, owner_username, f"Skill '{skill_name}' already exists as a built-in/public skill. Please install it under a different name."
     if owned_by_other:
-        who = owner_username or "another user"
-        return False, owned_by_me, True, builtin_conflict, owner_user_id, owner_username, f"Skill '{skill_name}' has already been installed by {who}. Please choose a different name."
+        who = owner_username or owner_user_id or "another user"
+        return False, False, False, True, False, owner_user_id, owner_username, f"Skill '{skill_name}' has already been uploaded by user '{who}' and cannot be overwritten."
     if owned_by_me:
-        return False, True, False, builtin_conflict, owner_user_id, owner_username, f"You have already installed skill '{skill_name}'. You can keep using it or install this copy under a different name."
-    return True, False, False, builtin_conflict, owner_user_id, owner_username, f"Skill '{skill_name}' can be installed."
+        return False, True, True, False, False, owner_user_id, owner_username, f"You have already uploaded skill '{skill_name}'. You may upload it again and overwrite your existing copy."
+    if unattributed_custom_conflict:
+        return False, False, False, False, False, owner_user_id, owner_username, f"Skill '{skill_name}' already exists, but its owner could not be determined. It cannot be overwritten safely."
+    return True, False, False, False, False, owner_user_id, owner_username, f"Skill '{skill_name}' can be installed."
 
 
 async def _build_renamed_skill_archive(
@@ -1193,11 +1266,12 @@ async def preview_skill_install(
         skill_file_path = await aresolve_thread_virtual_path(body.thread_id, body.path)
         validation = get_or_new_skill_storage(app_config=config).validate_skill_archive(skill_file_path, require_skill_suffix=False)
         skill_name = str(validation.get("skill_name") or "")
-        can_install_as_is, owned_by_me, owned_by_other, builtin_conflict, owner_user_id, owner_username, message = await _preview_install_target(
+        can_install_as_is, _can_overwrite, owned_by_me, owned_by_other, builtin_conflict, owner_user_id, owner_username, message = await _preview_install_target(
             request=request,
             skill_store=skill_store,
             skill_name=skill_name,
             target_category=body.target_category,
+            conflict_kind=validation.get("conflict_kind"),
         )
         return SkillInstallPreviewResponse(
             ok=True,
@@ -1248,11 +1322,12 @@ async def install_skill(
         storage = get_or_new_skill_storage(app_config=config)
         validation = storage.validate_skill_archive(skill_file_path, require_skill_suffix=False)
         preview_name = requested_name or str(validation.get("skill_name") or "")
-        can_install_as_is, owned_by_me, owned_by_other, builtin_conflict, _owner_user_id, _owner_username, message = await _preview_install_target(
+        can_install_as_is, _can_overwrite, owned_by_me, owned_by_other, builtin_conflict, _owner_user_id, _owner_username, message = await _preview_install_target(
             request=request,
             skill_store=skill_store,
             skill_name=preview_name,
             target_category=target_category,
+            conflict_kind=(validation.get("conflict_kind") if requested_name is None or requested_name == validation.get("skill_name") else None),
         )
         if not can_install_as_is:
             if requested_name and requested_name != validation.get("skill_name") and not owned_by_other and not builtin_conflict: