diff --git a/backend/apps/skill_app.py b/backend/apps/skill_app.py index 204325046..dc3adf77a 100644 --- a/backend/apps/skill_app.py +++ b/backend/apps/skill_app.py @@ -17,6 +17,7 @@ stream_skill_creation, update_skill_list, get_official_skills_with_status, + install_skills_from_zip_for_tenant, ) from consts.model import SkillInstanceInfoRequest, SkillCreateRequest, SkillCreateInteractiveRequest, SkillUpdateRequest, SkillResponse from utils.auth_utils import get_current_user_id, get_current_user_info @@ -46,6 +47,8 @@ def _build_skill_update_data(request: SkillUpdateRequest) -> Dict[str, Any]: "content", "tags", "source", + "group_ids", + "ingroup_permission", "config_schemas", "config_values", ): @@ -66,11 +69,14 @@ async def list_skills( ) -> JSONResponse: """List all available skills for the current tenant (or a specific tenant for super admin).""" try: - _, current_tenant_id = get_current_user_id(authorization) + user_id, current_tenant_id = get_current_user_id(authorization) # Super admin can query a specific tenant's skills; otherwise use current user's tenant effective_tenant_id = tenant_id if tenant_id else current_tenant_id service = SkillService(tenant_id=effective_tenant_id) - skills = service.list_skills(tenant_id=effective_tenant_id) + skills = service.list_visible_skills( + tenant_id=effective_tenant_id, + user_id=user_id, + ) return JSONResponse(content={"skills": skills}) except SkillException as e: raise HTTPException(status_code=500, detail=str(e)) @@ -122,8 +128,6 @@ async def install_skills( """ try: user_id, current_tenant_id = get_current_user_id(authorization) - from services.skill_service import install_skills_from_zip_for_tenant - effective_tenant_id = tenant_id if tenant_id else current_tenant_id installed_names = install_skills_from_zip_for_tenant( skill_names=request.skill_names, @@ -165,6 +169,8 @@ async def create_skill( "tool_ids": tool_ids, "tags": request.tags, "source": request.source, + "group_ids": request.group_ids, + "ingroup_permission": request.ingroup_permission, "config_schemas": request.config_schemas, "config_values": request.config_values, "files": request.files if request.files else [], @@ -310,7 +316,10 @@ async def get_skill_file_content( raise HTTPException(status_code=500, detail="Internal server error") -@router.put("/{skill_name}/upload") +@router.put( + "/{skill_name}/upload", + responses={403: {"description": "Not authorized to update this skill"}}, +) async def update_skill_from_file( skill_name: str, file: UploadFile = File(..., description="SKILL.md file or ZIP archive"), @@ -343,6 +352,8 @@ async def update_skill_from_file( return JSONResponse(content=skill) except UnauthorizedError as e: raise HTTPException(status_code=401, detail=str(e)) + except ForbiddenError as e: + raise HTTPException(status_code=403, detail=str(e)) except SkillException as e: if _NOT_FOUND_TEXT in str(e).lower(): raise HTTPException(status_code=404, detail=str(e)) @@ -603,7 +614,10 @@ async def get_skill(skill_name: str, authorization: Optional[str] = Header(None) raise HTTPException(status_code=500, detail="Internal server error") -@router.put("/{skill_name}") +@router.put( + "/{skill_name}", + responses={403: {"description": "Not authorized to update this skill"}}, +) async def update_skill( skill_name: str, request: SkillUpdateRequest, @@ -644,6 +658,8 @@ async def update_skill( return JSONResponse(content=skill) except UnauthorizedError as e: raise HTTPException(status_code=401, detail=str(e)) + except ForbiddenError as e: + raise HTTPException(status_code=403, detail=str(e)) except SkillException as e: if _NOT_FOUND_TEXT in str(e).lower(): raise HTTPException(status_code=404, detail=str(e)) diff --git a/backend/apps/skill_repository_app.py b/backend/apps/skill_repository_app.py index 108642f6a..704630c8e 100644 --- a/backend/apps/skill_repository_app.py +++ b/backend/apps/skill_repository_app.py @@ -46,9 +46,10 @@ async def list_skill_repository_listings_api( ): """List all skill marketplace repository listings with optional filters.""" try: - _, tenant_id = get_current_user_id(authorization) + user_id, tenant_id = get_current_user_id(authorization) result = list_skill_repository_listings_impl( tenant_id, + user_id=user_id, status=status, skill_id=skill_id, category_id=category_id, diff --git a/backend/consts/model.py b/backend/consts/model.py index f7a8997e7..f4a555d7e 100644 --- a/backend/consts/model.py +++ b/backend/consts/model.py @@ -1336,6 +1336,8 @@ class SkillCreateRequest(BaseModel): tool_names: Optional[List[str]] = [] tags: Optional[List[str]] = [] source: Optional[str] = "custom" + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[Dict[str, str]]] = Field( @@ -1361,6 +1363,8 @@ class SkillUpdateRequest(BaseModel): tool_names: Optional[List[str]] = None tags: Optional[List[str]] = None source: Optional[str] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[SkillFileData]] = Field( @@ -1379,6 +1383,8 @@ class SkillResponse(BaseModel): tool_ids: List[int] tags: List[str] source: str + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None created_by: Optional[str] = None diff --git a/backend/database/db_models.py b/backend/database/db_models.py index 15fa1aa7c..8de6466f1 100644 --- a/backend/database/db_models.py +++ b/backend/database/db_models.py @@ -16,6 +16,7 @@ _PUBLISHER_TENANT_ID_DOC = "Publisher tenant ID" _PUBLISHER_USER_ID_DOC = "Publisher user ID" _MCP_NAME_DOC = "MCP name" +_INGROUP_PERMISSION_DOC = "In-group permission: EDIT, READ_ONLY, PRIVATE" # Base class for tables without audit fields @@ -611,7 +612,7 @@ class AgentInfo(TableBase): group_ids = Column(String, doc="Agent group IDs list") is_new = Column(Boolean, default=False, doc="Whether this agent is marked as new for the user") current_version_no = Column(Integer, nullable=True, doc="Current published version number. NULL means no version published yet") - ingroup_permission = Column(String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") + ingroup_permission = Column(String(30), doc=_INGROUP_PERMISSION_DOC) requested_output_tokens = Column( Integer, doc=( @@ -708,7 +709,7 @@ class KnowledgeRecord(TableBase): tenant_id = Column(String(100), doc="Tenant ID") group_ids = Column(String, doc="Knowledge base group IDs list") ingroup_permission = Column( - String(30), doc="In-group permission: EDIT, READ_ONLY, PRIVATE") + String(30), doc=_INGROUP_PERMISSION_DOC) summary_frequency = Column(String(10), nullable=True, doc="Auto-summary frequency: '3h', '5h', '1d', '1w', or NULL (disabled)") last_summary_time = Column(TIMESTAMP(timezone=False), nullable=True, @@ -1202,6 +1203,8 @@ class SkillInfo(TableBase): JSON, doc="Runtime parameter values from config/config.yaml") source = Column(String(30), nullable=False, default="official", doc="Skill source: official, custom, etc.") + group_ids = Column(String, doc="Skill group IDs list") + ingroup_permission = Column(String(30), doc=_INGROUP_PERMISSION_DOC) class SkillToolRelation(TableBase): diff --git a/backend/database/skill_db.py b/backend/database/skill_db.py index 66fa0d6e3..66b162d1d 100644 --- a/backend/database/skill_db.py +++ b/backend/database/skill_db.py @@ -10,6 +10,7 @@ from database.client import get_db_session, filter_property, as_dict from database.db_models import SkillInfo, SkillToolRelation, SkillInstance, ToolInfo from utils.skill_params_utils import strip_params_comments_for_db +from utils.str_utils import convert_list_to_string, convert_string_to_list logger = logging.getLogger(__name__) @@ -231,10 +232,16 @@ def _build_skill_update_values( "content": "skill_content", "tags": "skill_tags", "source": "source", + "ingroup_permission": "ingroup_permission", } for input_field, model_field in field_mapping.items(): if input_field in skill_data: row_values[model_field] = skill_data[input_field] + if "group_ids" in skill_data: + group_ids = skill_data["group_ids"] + row_values["group_ids"] = ( + convert_list_to_string(group_ids) if isinstance(group_ids, list) else group_ids + ) for field in ("config_schemas", "config_values"): if field in skill_data: @@ -270,6 +277,8 @@ def _to_dict(skill: SkillInfo) -> Dict[str, Any]: "config_schemas": skill.config_schemas, "config_values": skill.config_values, "source": skill.source, + "group_ids": convert_string_to_list(skill.group_ids), + "ingroup_permission": skill.ingroup_permission, "created_by": skill.created_by, "create_time": skill.create_time.isoformat() if skill.create_time else None, "updated_by": skill.updated_by, @@ -287,6 +296,9 @@ def list_skills(tenant_id: str) -> List[Dict[str, Any]]: skills = session.query(SkillInfo).filter( SkillInfo.tenant_id == tenant_id, SkillInfo.delete_flag != 'Y' + ).order_by( + SkillInfo.create_time.desc(), + SkillInfo.skill_id.desc(), ).all() results = [] for s in skills: @@ -396,6 +408,12 @@ def create_skill(skill_data: Dict[str, Any], tenant_id: str) -> Dict[str, Any]: config_values=_params_value_for_db( skill_data.get("config_values")), source=skill_data.get("source", "custom"), + group_ids=( + convert_list_to_string(skill_data.get("group_ids")) + if isinstance(skill_data.get("group_ids"), list) + else skill_data.get("group_ids") + ), + ingroup_permission=skill_data.get("ingroup_permission"), created_by=skill_data.get("created_by"), create_time=datetime.now(), updated_by=skill_data.get("updated_by"), diff --git a/backend/database/skill_repository_db.py b/backend/database/skill_repository_db.py index e99997476..fff9b28ae 100644 --- a/backend/database/skill_repository_db.py +++ b/backend/database/skill_repository_db.py @@ -66,6 +66,7 @@ def get_skill_repository_by_skill_id( skill_id: int, *, publisher_tenant_id: Optional[str] = None, + statuses: Optional[Collection[str]] = None, ) -> Optional[dict]: """Fetch an active repository listing by source skill_id.""" with get_db_session() as session: @@ -77,7 +78,9 @@ def get_skill_repository_by_skill_id( query = query.filter( SkillRepository.publisher_tenant_id == publisher_tenant_id, ) - record = query.first() + if statuses is not None: + query = query.filter(SkillRepository.status.in_(list(statuses))) + record = query.order_by(SkillRepository.update_time.desc()).first() return as_dict(record) if record else None @@ -133,6 +136,7 @@ def list_skill_repository_summaries( query = session.query( SkillRepository.skill_repository_id, SkillRepository.skill_id, + SkillRepository.publisher_user_id, SkillRepository.submitted_by, SkillRepository.name, SkillRepository.description, @@ -171,6 +175,7 @@ def list_skill_repository_summaries( { "skill_repository_id": row.skill_repository_id, "skill_id": row.skill_id, + "publisher_user_id": row.publisher_user_id, "submitted_by": row.submitted_by, "name": row.name, "description": row.description, @@ -265,6 +270,29 @@ def update_skill_repository_status_by_id( return int(result.rowcount or 0) +def reset_skill_repository_status( + *, + repository_id: int, + skill_id: int, + status: str, + publisher_tenant_id: str, +) -> int: + """Set other active listings with the same skill and status to not_shared.""" + with get_db_session() as session: + result = session.execute( + update(SkillRepository) + .where( + SkillRepository.skill_id == skill_id, + SkillRepository.status == status, + SkillRepository.skill_repository_id != repository_id, + SkillRepository.publisher_tenant_id == publisher_tenant_id, + SkillRepository.delete_flag != "Y", + ) + .values(status=STATUS_NOT_SHARED) + ) + return int(result.rowcount or 0) + + def increment_skill_repository_downloads( *, repository_id: int, diff --git a/backend/services/skill_repository_service.py b/backend/services/skill_repository_service.py index 28f181c75..ef2becefd 100644 --- a/backend/services/skill_repository_service.py +++ b/backend/services/skill_repository_service.py @@ -15,7 +15,7 @@ VALID_OWNERSHIP_FILTERS, VALID_REPOSITORY_STATUSES, ) -from consts.const import CAN_EDIT_ALL_USER_ROLES, PERMISSION_EDIT, PERMISSION_READ +from consts.const import PERMISSION_PRIVATE, PERMISSION_READ from consts.exceptions import ForbiddenError, SkillDuplicateError, SkillException from database.skill_repository_db import ( get_skill_repository_by_id_and_publisher, @@ -24,6 +24,7 @@ insert_skill_repository_record, list_skill_repository_by_skill_ids, list_skill_repository_summaries, + reset_skill_repository_status, update_skill_repository_by_id, update_skill_repository_status_by_id, ) @@ -92,9 +93,13 @@ def _serialize_created_at(create_time: Any) -> Optional[str]: return str(create_time) -def _to_summary_item(record: Dict[str, Any]) -> Dict[str, Any]: +def _to_summary_item( + record: Dict[str, Any], + *, + can_take_down: Optional[bool] = None, +) -> Dict[str, Any]: """Map a DB record to a lightweight skill marketplace summary item.""" - return { + item = { "id": record.get("skill_repository_id"), "skill_repository_id": record.get("skill_repository_id"), "skill_id": record.get("skill_id"), @@ -110,6 +115,9 @@ def _to_summary_item(record: Dict[str, Any]) -> Dict[str, Any]: "created_at": record.get("created_at") or _serialize_created_at(record.get("create_time")), "updated_at": record.get("updated_at") or _serialize_created_at(record.get("update_time")), } + if can_take_down is not None: + item["can_take_down"] = can_take_down + return item def _to_detail_item( @@ -119,12 +127,16 @@ def _to_detail_item( ) -> Dict[str, Any]: """Map a DB record to a skill marketplace detail payload.""" snapshot = _as_dict(record.get("skill_info_json")) + creator_id = str(snapshot.get("created_by") or "").strip() + creator = get_user_tenant_by_user_id(creator_id) if creator_id else None + author = str((creator or {}).get("user_email") or "").strip() or None detail = { "skill_repository_id": record.get("skill_repository_id"), "skill_id": record.get("skill_id"), "name": record.get("name"), "description": record.get("description"), "source": record.get("source"), + "author": author, "submitted_by": record.get("submitted_by"), "icon": record.get("icon"), "status": record.get("status"), @@ -164,12 +176,14 @@ def _to_repository_info_item(record: Dict[str, Any]) -> Dict[str, Any]: def _matches_ownership(skill: Dict[str, Any], user_id: str, ownership_filter: str) -> bool: """Return whether a skill belongs to the requested ownership bucket.""" - created_by = skill.get("created_by") - if ownership_filter in (OWNERSHIP_ALL, OWNERSHIP_CREATED): - return created_by == user_id + if ownership_filter == OWNERSHIP_ALL: + return True + is_creator = str(skill.get("created_by")) == str(user_id) + if ownership_filter == OWNERSHIP_CREATED: + return is_creator if ownership_filter == OWNERSHIP_OTHERS: - return False - return created_by == user_id + return not is_creator + return True def _matches_search(skill: Dict[str, Any], search: Optional[str]) -> bool: @@ -190,11 +204,16 @@ def _matches_search(skill: Dict[str, Any], search: Optional[str]) -> bool: def _count_skills_by_ownership(skills: List[Dict[str, Any]], user_id: str) -> Dict[str, int]: """Count editable skills in each ownership bucket.""" - created = sum(1 for skill in skills if skill.get("created_by") == user_id) + created = sum( + 1 + for skill in skills + if str(skill.get("created_by")) == str(user_id) + ) + others = len(skills) - created return { - OWNERSHIP_ALL: created, + OWNERSHIP_ALL: len(skills), OWNERSHIP_CREATED: created, - OWNERSHIP_OTHERS: 0, + OWNERSHIP_OTHERS: others, } @@ -229,16 +248,19 @@ def _get_user_role(user_id: str) -> str: return str(user_tenant.get("user_role") or "USER") -def _resolve_mine_skill_permission( +def _can_publish_skill( *, skill: Dict[str, Any], user_id: str, user_role: str, -) -> str: - """Resolve list-item permission for skill repository mine view.""" - if user_role in CAN_EDIT_ALL_USER_ROLES: - return PERMISSION_EDIT - return PERMISSION_EDIT if skill.get("created_by") == user_id else PERMISSION_READ +) -> bool: + """Return whether the user may submit the skill to the repository.""" + if user_role == "ADMIN": + return True + return ( + user_role == "DEV" + and str(skill.get("created_by")) == str(user_id) + ) def _resolve_submitter_email(user_id: str) -> Optional[str]: @@ -255,9 +277,11 @@ def _validate_create_listing_permission( ) -> None: """Only ADMIN, or DEV who created the skill, may share to marketplace.""" user_role = _get_user_role(user_id) - if user_role == "ADMIN": - return - if user_role == "DEV" and skill_info.get("created_by") == user_id: + if _can_publish_skill( + skill=skill_info, + user_id=user_id, + user_role=user_role, + ): return raise ForbiddenError( f"User role {user_role} not authorized to create repository listing" @@ -322,6 +346,8 @@ def _build_skill_info_json(skill_info: Dict[str, Any]) -> Dict[str, Any]: "config_schemas": skill_info.get("config_schemas"), "config_values": skill_info.get("config_values"), "source": skill_info.get("source"), + "group_ids": skill_info.get("group_ids") or [], + "ingroup_permission": skill_info.get("ingroup_permission"), "tool_ids": skill_info.get("tool_ids") or [], "created_by": skill_info.get("created_by"), } @@ -386,6 +412,48 @@ def _build_repository_data_from_skill( return repository_data +def _find_resubmittable_repository_record( + skill_id: int, + tenant_id: str, +) -> Optional[Dict[str, Any]]: + """Find an existing review draft that can be refreshed by a new submission.""" + pending = get_skill_repository_by_skill_id( + skill_id, + publisher_tenant_id=tenant_id, + statuses=[STATUS_PENDING_REVIEW], + ) + if pending: + return pending + return get_skill_repository_by_skill_id( + skill_id, + publisher_tenant_id=tenant_id, + statuses=[STATUS_REJECTED], + ) + + +def _reset_repository_peer_statuses( + *, + skill_repository_id: int, + skill_id: int, + status: str, + publisher_tenant_id: str, +) -> None: + """Reset peer listings with the same status; also clear rejected when submitting.""" + reset_skill_repository_status( + repository_id=skill_repository_id, + skill_id=skill_id, + status=status, + publisher_tenant_id=publisher_tenant_id, + ) + if status == STATUS_PENDING_REVIEW: + reset_skill_repository_status( + repository_id=skill_repository_id, + skill_id=skill_id, + status=STATUS_REJECTED, + publisher_tenant_id=publisher_tenant_id, + ) + + def _validate_create_payload(repository_data: Dict[str, Any]) -> None: """Validate required fields before inserting a repository listing.""" required_fields = ( @@ -426,9 +494,9 @@ def create_skill_repository_listing_impl( ) _validate_create_payload(repository_data) - existing = get_skill_repository_by_skill_id( + existing = _find_resubmittable_repository_record( skill_id, - publisher_tenant_id=tenant_id, + tenant_id, ) if not existing: repository_id = insert_skill_repository_record( @@ -454,6 +522,13 @@ def create_skill_repository_listing_impl( raise ValueError("Failed to update repository listing") is_updated = True + _reset_repository_peer_statuses( + skill_repository_id=repository_id, + skill_id=skill_id, + status=STATUS_PENDING_REVIEW, + publisher_tenant_id=tenant_id, + ) + record = get_skill_repository_by_id_and_publisher( repository_id, tenant_id, @@ -591,6 +666,13 @@ def update_skill_repository_status_impl( if rows_affected == 0: raise ValueError(_REPOSITORY_LISTING_NOT_FOUND) + _reset_repository_peer_statuses( + skill_repository_id=skill_repository_id, + skill_id=record["skill_id"], + status=status, + publisher_tenant_id=tenant_id, + ) + updated = get_skill_repository_by_id_and_publisher( skill_repository_id, tenant_id, @@ -677,6 +759,7 @@ def install_skill_from_repository_impl( source="repository", user_id=user_id, tenant_id=tenant_id, + ingroup_permission=PERMISSION_READ, ) except SkillException as exc: message = str(exc) @@ -755,10 +838,13 @@ def _to_mine_skill_item( "description": skill.get("description"), "source": skill.get("source"), "tags": skill.get("tags") or [], + "group_ids": skill.get("group_ids") or [], + "ingroup_permission": skill.get("ingroup_permission"), "created_by": skill.get("created_by"), "created_at": skill.get("create_time"), "updated_at": skill.get("update_time"), - "permission": _resolve_mine_skill_permission( + "permission": skill.get("permission"), + "can_publish": _can_publish_skill( skill=skill, user_id=user_id, user_role=user_role, @@ -789,7 +875,10 @@ def list_my_editable_skills_impl( safe_page_size = max(int(page_size or 10), 1) user_role = _get_user_role(user_id) - skills = SkillService(tenant_id=tenant_id).list_skills(tenant_id=tenant_id) + skills = SkillService(tenant_id=tenant_id).list_visible_skills( + tenant_id=tenant_id, + user_id=user_id, + ) counts = _count_skills_by_ownership(skills, user_id) filtered_skills = [ @@ -838,6 +927,7 @@ def list_my_editable_skills_impl( def list_skill_repository_listings_impl( tenant_id: str, *, + user_id: str, status: Optional[str] = None, skill_id: Optional[int] = None, category_id: Optional[int] = None, @@ -863,8 +953,24 @@ def list_skill_repository_listings_impl( search=search, sort_by_update_time=sort_by_update_time, ) + user_role = _get_user_role(user_id) return { - "items": [_to_summary_item(record) for record in result.get("items", [])], + "items": [ + _to_summary_item( + record, + can_take_down=( + record.get("status") == STATUS_SHARED + and ( + user_role in ("ADMIN", "SU") + or ( + user_role == "DEV" + and str(record.get("publisher_user_id")) == str(user_id) + ) + ) + ), + ) + for record in result.get("items", []) + ], "pagination": result.get("pagination"), } diff --git a/backend/services/skill_service.py b/backend/services/skill_service.py index fbf5c980b..42f239b4c 100644 --- a/backend/services/skill_service.py +++ b/backend/services/skill_service.py @@ -21,18 +21,121 @@ from nexent.skills.skill_loader import SkillLoader from nexent.core.utils.observer import MessageObserver from nexent.core.agents.agent_model import ModelConfig -from consts.const import CONTAINER_SKILLS_PATH, OFFICIAL_SKILLS_ZIP_PATH, ROOT_DIR +from consts.const import ( + CAN_EDIT_ALL_USER_ROLES, + CONTAINER_SKILLS_PATH, + OFFICIAL_SKILLS_ZIP_PATH, + PERMISSION_EDIT, + PERMISSION_PRIVATE, + PERMISSION_READ, + ROOT_DIR, +) from consts.exceptions import ForbiddenError, SkillException from database import skill_db +from database.group_db import query_group_ids_by_user +from database.user_tenant_db import get_user_tenant_by_user_id from agents.skill_creation_agent import create_skill_from_request from utils.prompt_template_utils import get_skill_creation_simple_prompt_template from utils.content_classifier_utils import ContentClassifier +from utils.str_utils import convert_list_to_string logger = logging.getLogger(__name__) +_SKILL_UPDATE_FORBIDDEN_MESSAGE = "Not authorized to update this skill" _skill_manager: Optional[SkillManager] = None +def _to_group_id_set(group_ids: Any) -> set[int]: + if isinstance(group_ids, str): + return { + int(group_id.strip()) + for group_id in group_ids.split(",") + if group_id.strip().isdigit() + } + if isinstance(group_ids, list): + return { + int(group_id) + for group_id in group_ids + if str(group_id).strip().isdigit() + } + return set() + + +def can_view_skill( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, + user_group_ids: set[int], +) -> bool: + """Return whether a skill is available to the current user.""" + if user_role in CAN_EDIT_ALL_USER_ROLES: + return True + if str(skill.get("created_by")) == str(user_id): + return True + if skill.get("ingroup_permission") == PERMISSION_PRIVATE: + return False + return bool( + user_group_ids.intersection(_to_group_id_set(skill.get("group_ids"))) + ) + + +def resolve_skill_permission( + *, + skill: Dict[str, Any], + user_id: str, + user_role: str, + user_group_ids: set[int], +) -> str: + """Resolve whether the current user can edit or only use a visible skill.""" + if user_role in CAN_EDIT_ALL_USER_ROLES: + return PERMISSION_EDIT + if str(skill.get("created_by")) == str(user_id): + return PERMISSION_EDIT + if skill.get("ingroup_permission") != PERMISSION_EDIT: + return PERMISSION_READ + return ( + PERMISSION_EDIT + if user_group_ids.intersection(_to_group_id_set(skill.get("group_ids"))) + else PERMISSION_READ + ) + + +def _apply_default_skill_permission_fields( + skill_data: Dict[str, Any], + user_id: Optional[str], +) -> None: + """Default user-created skills to the creator's groups with edit permission.""" + if not user_id: + return + if skill_data.get("group_ids") is None: + skill_data["group_ids"] = convert_list_to_string(query_group_ids_by_user(user_id)) + if not skill_data.get("ingroup_permission"): + skill_data["ingroup_permission"] = PERMISSION_EDIT + + +def _get_user_role(user_id: Optional[str]) -> str: + if not user_id: + return "USER" + user_tenant = get_user_tenant_by_user_id(user_id) + if not user_tenant: + return "USER" + return str(user_tenant.get("user_role") or "USER") + + +def _can_edit_skill(skill: Dict[str, Any], user_id: Optional[str]) -> bool: + if not user_id: + return False + user_role = _get_user_role(user_id) + user_group_ids = set(query_group_ids_by_user(user_id) or []) + return resolve_skill_permission( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) == PERMISSION_EDIT + + def _normalize_zip_entry_path(name: str) -> str: """Normalize a ZIP member path for comparison (slashes, strip ./).""" norm = name.replace("\\", "/").strip() @@ -941,6 +1044,34 @@ def list_skills(self, tenant_id: Optional[str] = None) -> List[Dict[str, Any]]: logger.error(f"Error listing skills: {e}") raise SkillException(f"Failed to list skills: {str(e)}") from e + def list_visible_skills( + self, + *, + tenant_id: Optional[str] = None, + user_id: str, + ) -> List[Dict[str, Any]]: + """List skills visible to a user and attach the resolved permission.""" + user_role = _get_user_role(user_id) + user_group_ids = set(query_group_ids_by_user(user_id) or []) + visible_skills = [ + skill + for skill in self.list_skills(tenant_id=tenant_id) + if can_view_skill( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) + ] + for skill in visible_skills: + skill["permission"] = resolve_skill_permission( + skill=skill, + user_id=user_id, + user_role=user_role, + user_group_ids=user_group_ids, + ) + return visible_skills + def get_skill(self, skill_name: str, tenant_id: Optional[str] = None) -> Optional[Dict[str, Any]]: """Get a specific skill within a tenant. @@ -1028,6 +1159,7 @@ def create_skill( if user_id: skill_data["created_by"] = user_id skill_data["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_data, user_id) try: # Create database record first @@ -1156,6 +1288,7 @@ def _create_skill_from_md( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) @@ -1289,6 +1422,7 @@ def _create_skill_from_zip( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) @@ -1449,6 +1583,8 @@ def update_skill_from_file( existing = skill_db.get_skill_by_name(skill_name, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_name}") + if user_id is not None and not _can_edit_skill(existing, user_id): + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) content_bytes: bytes if isinstance(file_content, str): @@ -1621,6 +1757,8 @@ def update_skill( existing = skill_db.get_skill_by_name(skill_name, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_name}") + if user_id is not None and not _can_edit_skill(existing, user_id): + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) result = skill_db.update_skill( skill_name, skill_data, effective_tenant_id, updated_by=user_id or None @@ -1673,7 +1811,7 @@ def update_skill( ) return self._enrich_configs_from_yaml(result) - except SkillException: + except (ForbiddenError, SkillException): raise except Exception as e: logger.error(f"Error updating skill {skill_name}: {e}") @@ -1694,8 +1832,8 @@ def update_skill_by_id( existing = skill_db.get_skill_by_id(skill_id, effective_tenant_id) if not existing: raise SkillException(f"Skill not found: {skill_id}") - if not user_id or existing.get("created_by") != user_id: - raise ForbiddenError("Not authorized to update this skill") + if not _can_edit_skill(existing, user_id): + raise ForbiddenError(_SKILL_UPDATE_FORBIDDEN_MESSAGE) local_dir = self._resolve_local_skills_dir_for_overlay() if local_dir and "name" in skill_data: @@ -2128,7 +2266,8 @@ def create_skill_from_zip_bytes( source: str = "导入", user_id: Optional[str] = None, tenant_id: Optional[str] = None, - skip_duplicate_check: bool = False + skip_duplicate_check: bool = False, + ingroup_permission: Optional[str] = None, ) -> Dict[str, Any]: """Create a skill from ZIP bytes, optionally skipping the duplicate name check. @@ -2143,6 +2282,7 @@ def create_skill_from_zip_bytes( user_id: Creator user ID tenant_id: Tenant ID skip_duplicate_check: If True, skip the "skill already exists" check + ingroup_permission: Optional group permission override for the new skill Returns: Created skill dict @@ -2246,6 +2386,9 @@ def create_skill_from_zip_bytes( if user_id: skill_dict["created_by"] = user_id skill_dict["updated_by"] = user_id + if ingroup_permission is not None: + skill_dict["ingroup_permission"] = ingroup_permission + _apply_default_skill_permission_fields(skill_dict, user_id) result = skill_db.create_skill(skill_dict, tenant_id) diff --git a/deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql b/deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql new file mode 100644 index 000000000..e158a3103 --- /dev/null +++ b/deploy/sql/migrations/v2.4.0_0722_add_skill_permission_and_repository_snapshots.sql @@ -0,0 +1,44 @@ +-- Migration: Add skill group permissions and allow separate repository snapshots by status +-- Date: 2026-07-22 +-- Description: Align skill ownership and repository status behavior with agent repository semantics. + +SET search_path TO nexent; + +ALTER TABLE IF EXISTS nexent.ag_skill_info_t + ADD COLUMN IF NOT EXISTS group_ids VARCHAR, + ADD COLUMN IF NOT EXISTS ingroup_permission VARCHAR(30); + +COMMENT ON COLUMN nexent.ag_skill_info_t.group_ids IS 'Skill group IDs list'; +COMMENT ON COLUMN nexent.ag_skill_info_t.ingroup_permission IS 'In-group permission: EDIT, READ_ONLY, PRIVATE'; + +WITH tenant_groups AS ( + SELECT + tenant_id, + string_agg(group_id::text, ',' ORDER BY group_id) AS group_ids + FROM nexent.tenant_group_info_t + WHERE delete_flag = 'N' + GROUP BY tenant_id +) +UPDATE nexent.ag_skill_info_t skill +SET group_ids = tenant_groups.group_ids +FROM tenant_groups +WHERE skill.tenant_id = tenant_groups.tenant_id + AND skill.delete_flag = 'N' + AND skill.tenant_id IS NOT NULL + AND (skill.group_ids IS NULL OR skill.group_ids = ''); + +UPDATE nexent.ag_skill_info_t +SET ingroup_permission = 'EDIT' +WHERE delete_flag = 'N' + AND tenant_id IS NOT NULL + AND (ingroup_permission IS NULL OR ingroup_permission = ''); + +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_active; +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_shared_active; +DROP INDEX IF EXISTS nexent.uq_skill_repository_skill_pending_active; + +CREATE INDEX IF NOT EXISTS idx_skill_repository_skill_status_delete + ON nexent.ag_skill_repository_t (publisher_tenant_id, skill_id, status, delete_flag); + +COMMENT ON COLUMN nexent.ag_skill_repository_t.skill_id IS + 'Source skill ID from ag_skill_info_t; multiple active snapshots may exist across statuses'; diff --git a/frontend/app/[locale]/agents/components/AgentConfigComp.tsx b/frontend/app/[locale]/agents/components/AgentConfigComp.tsx index 4868bbfaf..7b2111692 100644 --- a/frontend/app/[locale]/agents/components/AgentConfigComp.tsx +++ b/frontend/app/[locale]/agents/components/AgentConfigComp.tsx @@ -17,6 +17,8 @@ import { useSkillList } from "@/hooks/agent/useSkillList"; import { useExternalAgents } from "@/hooks/agent/useExternalAgents"; import McpConfigModal from "./agentConfig/McpConfigModal"; import A2AAgentDiscoveryModal from "./a2a/A2AAgentDiscoveryModal"; +import type { Skill } from "@/types/agentConfig"; +import type { MyEditableSkillItem } from "@/types/skillRepository"; import { Wrench, RefreshCw, Lightbulb, Plug, BlocksIcon, Globe } from "lucide-react"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; @@ -36,6 +38,9 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { const [isMcpModalOpen, setIsMcpModalOpen] = useState(false); const [isSkillModalOpen, setIsSkillModalOpen] = useState(false); + const [editingSkill, setEditingSkill] = useState( + null + ); const [isRefreshing, setIsRefreshing] = useState(false); const [isRefreshingSkill, setIsRefreshingSkill] = useState(false); const [showA2ADiscovery, setShowA2ADiscovery] = useState(false); @@ -83,6 +88,11 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { invalidateSkills(); }, [invalidateSkills]); + const handleEditSkill = useCallback((skill: Skill) => { + setEditingSkill({ ...skill, repository_info: [] }); + setIsSkillModalOpen(true); + }, []); + return ( <> {/* Import handled by Ant Design Upload (no hidden input required) */} @@ -254,6 +264,7 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { isCreatingMode={isCreatingMode} currentAgentId={currentAgentId ?? undefined} isReadOnly={isReadOnly} + onEditSkill={handleEditSkill} /> @@ -279,7 +290,11 @@ export default function AgentConfigComp({}: AgentConfigCompProps) { setIsSkillModalOpen(false)} + editingSkill={editingSkill} + onCancel={() => { + setIsSkillModalOpen(false); + setEditingSkill(null); + }} onSuccess={handleSkillBuildSuccess} /> diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx index ca97c8ee1..28f18ce6e 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillBuildModal.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, useEffect, useRef, type ChangeEvent } from "react"; +import { useState, useEffect, useMemo, useRef, type ChangeEvent } from "react"; import { useTranslation } from "react-i18next"; import { Modal, @@ -44,10 +44,18 @@ import { type SkillListItem, type SkillData, } from "@/services/skillService"; -import { fetchSkillById } from "@/services/agentConfigService"; import type { MyEditableSkillItem } from "@/types/skillRepository"; +import { + fetchSkillById, + fetchSkillFileContent, + fetchSkillFiles, + type SkillFileNode, +} from "@/services/agentConfigService"; +import { normalizeSkillFiles } from "@/lib/skillFileUtils"; import { MarkdownRenderer } from "@/components/common/markdownRenderer"; import log from "@/lib/logger"; +import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; +import { useGroupDetails, useGroupList } from "@/hooks/group/useGroupList"; import SkillDraftPanel from "./SkillDraftPanel"; const { TextArea } = Input; @@ -110,6 +118,39 @@ function mergeGeneratedSkillTabs( return { updatedTabs, finalTabs }; } +function flattenSkillFiles( + nodes: SkillFileNode[], + skillName: string +): string[] { + const paths: string[] = []; + const walk = (items: SkillFileNode[], parentPath = "") => { + items.forEach((item) => { + const isRootSkillDirectory = + !parentPath && item.type === "directory" && item.name === skillName; + const path = isRootSkillDirectory + ? "" + : parentPath + ? `${parentPath}/${item.name}` + : item.name; + if (item.type === "file") { + paths.push(path); + } else if (item.children?.length) { + walk(item.children, path); + } + }); + }; + walk(nodes); + return paths; +} + +function sortSkillTabs(tabs: SkillFileContent[]): SkillFileContent[] { + return [...tabs].sort((a, b) => { + if (a.path === "SKILL.md") return -1; + if (b.path === "SKILL.md") return 1; + return a.path.localeCompare(b.path); + }); +} + export default function SkillBuildModal({ isOpen, onCancel, @@ -118,10 +159,33 @@ export default function SkillBuildModal({ onBeforeEditSave, }: SkillBuildModalProps) { const { t, i18n } = useTranslation("common"); + const { user, getAccessibleGroupIds } = useAuthorizationContext(); const [form] = Form.useForm(); const isEditMode = Boolean(editingSkill); + const { data: groupData } = useGroupList(user?.tenantId ?? null); + const accessibleGroupIds = useMemo( + () => getAccessibleGroupIds(), + [getAccessibleGroupIds] + ); + const { groups: filteredGroups } = useGroupDetails( + groupData?.groups ?? [], + accessibleGroupIds + ); + const groupSelectOptions = useMemo( + () => + filteredGroups.map((group) => ({ + label: group.group_name, + value: group.group_id, + })), + [filteredGroups] + ); const [activeTab, setActiveTab] = useState("interactive"); const [isSubmitting, setIsSubmitting] = useState(false); + const [isLoadingEditFiles, setIsLoadingEditFiles] = useState(false); + const [loadedEditSkillId, setLoadedEditSkillId] = useState( + null + ); + const [editFilesError, setEditFilesError] = useState(null); const [allSkills, setAllSkills] = useState([]); const [uploadFile, setUploadFile] = useState(null); const [uploadExtractedSkillName, setUploadExtractedSkillName] = @@ -228,6 +292,14 @@ export default function SkillBuildModal({ }; }, [isOpen]); + useEffect(() => { + if (!isOpen || isEditMode) return; + form.setFieldsValue({ + group_ids: accessibleGroupIds, + ingroup_permission: "READ_ONLY", + }); + }, [accessibleGroupIds, form, isEditMode, isOpen]); + useEffect(() => { if (!isOpen) { // Abort any ongoing streaming request @@ -254,6 +326,9 @@ export default function SkillBuildModal({ setSummaryContent(""); currentAssistantIdRef.current = ""; setAccumulatedDraft(null); + setLoadedEditSkillId(null); + setEditFilesError(null); + setIsLoadingEditFiles(false); } }, [isOpen]); @@ -307,24 +382,72 @@ export default function SkillBuildModal({ description: skill.description || "", source: skill.source || "custom", tags: Array.isArray(skill.tags) ? skill.tags : [], + group_ids: Array.isArray(skill.group_ids) ? skill.group_ids : [], + ingroup_permission: skill.ingroup_permission || "READ_ONLY", }); - setSkillTabs([{ path: "SKILL.md", content: skill.content || "" }]); - setActiveSkillTab("SKILL.md"); }; setActiveTab("interactive"); - applySkillInfo({ - name: skillName, - description: editingSkill.description || "", - source: editingSkill.source || "custom", - tags: editingSkill.tags || [], - }); + setEditFilesError(null); + setLoadedEditSkillId(null); + setIsLoadingEditFiles(true); - void fetchSkillById(editingSkill.skill_id).then((result) => { - if (result.success && result.data) { - applySkillInfo(result.data); + const loadEditFiles = async () => { + try { + const result = await fetchSkillById(editingSkill.skill_id); + const skillInfo = + result.success && result.data + ? result.data + : { + name: skillName, + description: editingSkill.description || "", + source: editingSkill.source || "custom", + tags: editingSkill.tags || [], + group_ids: editingSkill.group_ids || [], + ingroup_permission: + editingSkill.ingroup_permission || "READ_ONLY", + }; + const resolvedSkillName = skillInfo.name?.trim() || skillName; + const fileTree = await fetchSkillFiles(resolvedSkillName); + const filePaths = flattenSkillFiles( + normalizeSkillFiles(fileTree), + resolvedSkillName + ); + if (filePaths.length === 0) { + throw new Error("Skill file tree is empty"); + } + const tabs = await Promise.all( + filePaths.map(async (path) => { + const content = await fetchSkillFileContent( + resolvedSkillName, + path + ); + if (content === null) { + throw new Error(`Failed to load skill file: ${path}`); + } + return { path, content }; + }) + ); + if (!cancelled) { + const sortedTabs = sortSkillTabs(tabs); + applySkillInfo(skillInfo); + setSkillTabs(sortedTabs); + setActiveSkillTab(sortedTabs[0]?.path || "SKILL.md"); + setLoadedEditSkillId(editingSkill.skill_id); + } + } catch (error) { + log.error("Failed to load skill files for editing:", error); + if (!cancelled) { + setEditFilesError(t("skillManagement.message.loadFilesFailed")); + } + } finally { + if (!cancelled) { + setIsLoadingEditFiles(false); + } } - }); + }; + + void loadEditFiles(); return () => { cancelled = true; @@ -354,6 +477,12 @@ export default function SkillBuildModal({ const handleManualSubmit = async () => { try { + if (isEditMode && (isLoadingEditFiles || editFilesError)) { + message.error( + editFilesError || t("skillManagement.message.loadFilesFailed") + ); + return; + } const values = await form.validateFields(); if (isEditMode && editingSkill && onBeforeEditSave) { const shouldContinue = await onBeforeEditSave(editingSkill); @@ -600,7 +729,9 @@ export default function SkillBuildModal({ taskIdRef.current = taskId; }, onThinkingUpdate: (step, desc) => { - setThinkingDescription(desc || t("skillManagement.generatingSkill")); + setThinkingDescription( + desc || t("skillManagement.generatingSkill") + ); }, onThinkingVisible: (visible) => { setIsThinkingVisible(visible); @@ -775,6 +906,8 @@ export default function SkillBuildModal({ const modalBodyFrame = "min(92vh, 760px)"; const editingSkillName = editingSkill?.name?.trim() || interactiveSkillName.trim(); + const isEditContentReady = + !isEditMode || loadedEditSkillId === editingSkill?.skill_id; const renderUploadTab = () => { const existingSkill = allSkills.find( @@ -1071,6 +1204,7 @@ export default function SkillBuildModal({ textareaRefs={textareaRefs} shouldAutoScrollRef={shouldAutoScrollRef} onTextareaScroll={handleTextareaScroll} + groupSelectOptions={groupSelectOptions} /> ); @@ -1111,7 +1245,9 @@ export default function SkillBuildModal({ title={
- {isEditMode ? t("skillManagement.edit.title") : t("skillManagement.title")} + {isEditMode + ? t("skillManagement.edit.title") + : t("skillManagement.title")}
{isEditMode @@ -1143,6 +1279,9 @@ export default function SkillBuildModal({ type="primary" loading={isSubmitting} onClick={handleManualSubmit} + disabled={ + isEditMode && (isLoadingEditFiles || Boolean(editFilesError)) + } > {getConfirmButtonText()} @@ -1173,7 +1312,17 @@ export default function SkillBuildModal({ items={visibleTabItems} className="skill-build-tabs shrink-0" /> - {isEditMode || activeTab === "interactive" ? ( + {isEditMode && !isEditContentReady ? ( +
+ + {editFilesError ? ( +

{editFilesError}

+ ) : ( +
+ )} + +
+ ) : isEditMode || activeTab === "interactive" ? (
{renderChatPanel()} {renderDraftPanel()} diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx index c995ee898..39517b52a 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDetailModal.tsx @@ -11,6 +11,7 @@ import type { SkillFormData, } from "@/types/skill"; import { + fetchSkillById, fetchSkillFileContent, fetchSkillFiles, SkillFilesAccessDeniedError, @@ -27,6 +28,22 @@ interface SkillDetailModalProps { const OFFICIAL_SOURCES = new Set(["official", "\u5b98\u65b9"]); +async function loadSkillFileTabs(skillName: string): Promise { + const files = await fetchSkillFiles(skillName); + const paths = flattenSkillFiles(normalizeSkillFiles(files), skillName); + return Promise.all( + paths.map(async (path) => { + try { + const content = await fetchSkillFileContent(skillName, path); + return { path, content: content || "" }; + } catch (error) { + log.error("Failed to load skill file content:", error); + return { path, content: "" }; + } + }) + ); +} + export default function SkillDetailModal({ skill, open, @@ -59,30 +76,29 @@ export default function SkillDetailModal({ const loadFiles = async () => { setLoading(true); try { - const files = await fetchSkillFiles(skill.name); - const flatFiles = flattenSkillFiles( - normalizeSkillFiles(files), - skill.name + const detailResult = await fetchSkillById( + skill.skill_id, + skill.tenant_id ); - if (flatFiles.length === 0) { - return; + const detail = + detailResult.success && detailResult.data ? detailResult.data : skill; + const skillName = detail.name?.trim() || skill.name; + if (!cancelled) { + form.setFieldsValue({ + name: skillName, + description: detail.description || "", + source: formatSource(detail.source, t), + tags: Array.isArray(detail.tags) ? detail.tags : [], + content: detail.content || "", + }); } - const tabs = await Promise.all( - flatFiles.map(async (path) => { - try { - const content = await fetchSkillFileContent(skill.name, path); - return { path, content: content || "" }; - } catch (error) { - log.error("Failed to load skill file content:", error); - return { path, content: "" }; - } - }) - ); + const tabs = await loadSkillFileTabs(skillName); - if (!cancelled) { - setSkillTabs(sortSkillTabs(tabs)); - setActiveSkillTab(sortSkillTabs(tabs)[0]?.path || "SKILL.md"); + if (!cancelled && tabs.length > 0) { + const sortedTabs = sortSkillTabs(tabs); + setSkillTabs(sortedTabs); + setActiveSkillTab(sortedTabs[0]?.path || "SKILL.md"); } } catch (error) { if (cancelled) return; @@ -104,7 +120,7 @@ export default function SkillDetailModal({ return () => { cancelled = true; }; - }, [open, skill?.skill_id, form, t]); + }, [open, skill, form, t]); const handleClose = () => { setSkillTabs([{ path: "SKILL.md", content: "" }]); diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx index e6790b896..74d060c27 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillDraftPanel.tsx @@ -13,6 +13,7 @@ import { Button, Col, Form, Input, Modal, Row, Select, Tooltip } from "antd"; import { FileText, Folder, Maximize2, Pencil, Plus, X } from "lucide-react"; import { useTranslation } from "react-i18next"; +import { Can } from "@/components/permission/Can"; import type { SkillFileContent, SkillFormData } from "@/types/skill"; const { TextArea } = Input; @@ -31,6 +32,7 @@ interface SkillDraftPanelProps { textareaRefs?: MutableRefObject>; shouldAutoScrollRef?: MutableRefObject>; onTextareaScroll?: (tabPath: string) => void; + groupSelectOptions?: Array<{ label: string; value: number }>; className?: string; } @@ -46,6 +48,7 @@ export default function SkillDraftPanel({ textareaRefs, shouldAutoScrollRef, onTextareaScroll, + groupSelectOptions = [], className, }: SkillDraftPanelProps) { const { t } = useTranslation("common"); @@ -224,6 +227,58 @@ export default function SkillDraftPanel({ + {!readOnly ? ( + + + + + + + + + + ) : null} + { @@ -436,7 +493,8 @@ export default function SkillDraftPanel({ setExpandedEditorContent(""); }} centered - width={640} + width="min(960px, calc(100vw - 48px))" + className="expanded-file-editor" styles={{ body: { padding: 0 } }} footer={ readOnly @@ -480,7 +538,7 @@ export default function SkillDraftPanel({ if (readOnly) return; setExpandedEditorContent(e.target.value); }} - autoSize={{ minRows: 10, maxRows: 28 }} + autoSize={{ minRows: 16, maxRows: 32 }} className="rounded-none border-0 font-mono text-sm shadow-none focus:border-0 focus:shadow-none" style={{ resize: "none" }} /> diff --git a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx index a506b9224..8b70cb3eb 100644 --- a/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx +++ b/frontend/app/[locale]/agents/components/agentConfig/SkillManagement.tsx @@ -3,10 +3,10 @@ import { useState, useEffect } from "react"; import { useTranslation } from "react-i18next"; import { SkillGroup, Skill, SkillParam } from "@/types/agentConfig"; -import { Tabs, message, Tooltip, Badge } from "antd"; +import { Badge, Button, message, Tabs, Tooltip } from "antd"; import { useAgentConfigStore } from "@/stores/agentConfigStore"; import { useSkillList } from "@/hooks/agent/useSkillList"; -import { Info, Trash2, Settings } from "lucide-react"; +import { Eye, Pencil, Trash2, Settings } from "lucide-react"; import { useConfirmModal } from "@/hooks/useConfirmModal"; import { deleteSkill, fetchSkillInstances } from "@/services/agentConfigService"; import log from "@/lib/logger"; @@ -18,6 +18,7 @@ interface SkillManagementProps { isCreatingMode?: boolean; currentAgentId?: number | undefined; isReadOnly?: boolean; + onEditSkill?: (skill: Skill) => void; } export default function SkillManagement({ @@ -25,6 +26,7 @@ export default function SkillManagement({ isCreatingMode, currentAgentId, isReadOnly: isReadOnlyProp, + onEditSkill, }: SkillManagementProps) { const { t } = useTranslation("common"); const { confirm } = useConfirmModal(); @@ -136,6 +138,10 @@ export default function SkillManagement({ const handleInfoClick = (skill: Skill, e: React.MouseEvent) => { e.stopPropagation(); + if (!isReadOnly && skill.permission === "EDIT" && onEditSkill) { + onEditSkill(skill); + return; + } setSelectedSkill(skill); setIsDetailModalOpen(true); }; @@ -244,6 +250,9 @@ export default function SkillManagement({ > {group.skills.map((skill) => { const isSelected = originalSelectedSkillIdsSet.has(skill.skill_id); + const canEditSkill = + !isReadOnly && skill.permission === "EDIT" && onEditSkill != null; + const SkillActionIcon = canEditSkill ? Pencil : Eye; const hasConfigurableParams = Array.isArray(skill.config_schemas) && skill.config_schemas.length > 0; @@ -270,20 +279,27 @@ export default function SkillManagement({ onClick={isReadOnly ? undefined : (e) => handleConfigClick(skill, e)} /> )} - handleInfoClick(skill, e)} - /> - handleDeleteClick(skill, e)} + className="cursor-pointer text-gray-400 hover:text-gray-600 transition-colors" + onClick={(e) => handleInfoClick(skill, e)} /> + +
); diff --git a/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx b/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx index b5ad1af6c..8c22860e2 100644 --- a/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx +++ b/frontend/app/[locale]/skill-space/components/MineSkillsView.tsx @@ -1,7 +1,7 @@ "use client"; import { useState } from "react"; -import { App, Button, Dropdown, Input } from "antd"; +import { App, Button, Dropdown, Input, Tooltip } from "antd"; import type { MenuProps } from "antd"; import { useTranslation } from "react-i18next"; import { @@ -35,11 +35,18 @@ import type { MyEditableSkillItem, MyEditableSkillListItem, MySkillRepositoryInfoItem, + MineOwnershipFilter, NewSkillPaddingItem, SkillRepositoryListingCreatePayload, SkillRepositoryListingStatus, } from "@/types/skillRepository"; +const MINE_OWNERSHIP_FILTERS: MineOwnershipFilter[] = [ + "all", + "created", + "others", +]; + function isNewSkillPaddingItem( item: MyEditableSkillListItem ): item is NewSkillPaddingItem { @@ -49,6 +56,8 @@ function isNewSkillPaddingItem( export function MineSkillsView({ skills, counts, + ownership, + onOwnershipChange, searchQuery, onSearchChange, isLoading, @@ -61,6 +70,7 @@ export function MineSkillsView({ onRetry, onCreateSkill, onEditSkill, + onViewSkill, onDeleteSkill, onApplyListing, isUpdatingStatus, @@ -68,6 +78,8 @@ export function MineSkillsView({ }: { skills: MyEditableSkillListItem[]; counts: { all: number; created: number; others: number }; + ownership: MineOwnershipFilter; + onOwnershipChange: (ownership: MineOwnershipFilter) => void; searchQuery: string; onSearchChange: (value: string) => void; isLoading: boolean; @@ -80,6 +92,7 @@ export function MineSkillsView({ onRetry: () => void; onCreateSkill: () => void; onEditSkill: (skill: MyEditableSkillItem) => void; + onViewSkill: (skill: MyEditableSkillItem) => void; onDeleteSkill: (skill: MyEditableSkillItem) => Promise; onApplyListing: ( skill: MyEditableSkillItem, @@ -95,6 +108,11 @@ export function MineSkillsView({ useState(null); const [reviewModalInfo, setReviewModalInfo] = useState(null); + const ownershipLabelKey: Record = { + all: "agentRepository.mine.filter.all", + created: "agentRepository.mine.filter.created", + others: "agentRepository.mine.filter.others", + }; const openReviewModal = (skill: MyEditableSkillItem) => { const repositoryInfo = pickReviewDisplayRepositoryInfo( @@ -183,10 +201,16 @@ export function MineSkillsView({
- {}}> - {t("skillRepository.filter.all")} - {counts.all} - + {MINE_OWNERSHIP_FILTERS.map((filter) => ( + onOwnershipChange(filter)} + > + {t(ownershipLabelKey[filter])} + {counts[filter]} + + ))}

@@ -198,36 +222,38 @@ export function MineSkillsView({ isError={isError} isFetching={isFetching} onRetry={onRetry} - isEmpty={false} + isEmpty={skills.length === 0} emptyDescription={t("skillRepository.mine.empty")} > -

- {skills.map((skill) => - isNewSkillPaddingItem(skill) ? ( -
- -
- ) : ( - onEditSkill(skill)} - onDelete={() => handleDeleteSkill(skill)} - onApplyListing={() => handleEnableSkill(skill)} - onViewReview={() => openReviewModal(skill)} - /> - ) - )} -
+ <> +
+ {skills.map((skill) => + isNewSkillPaddingItem(skill) ? ( +
+ +
+ ) : ( + onEditSkill(skill)} + onView={() => onViewSkill(skill)} + onDelete={() => handleDeleteSkill(skill)} + onApplyListing={() => handleEnableSkill(skill)} + onViewReview={() => openReviewModal(skill)} + /> + ) + )} +
+ + - - = { "bg-emerald-50 text-emerald-700 dark:bg-emerald-500/10 dark:text-emerald-300", }; +function getApplyButtonLabel( + isPendingReview: boolean, + hasSharedRepository: boolean, + repositoryStatus: SkillRepositoryListingStatus, + t: (key: string) => string +) { + if (isPendingReview) { + return getSkillRepositoryStatusLabel(t, repositoryStatus); + } + return hasSharedRepository + ? t("skillRepository.mine.button.reapply") + : t("skillRepository.mine.button.apply"); +} + +function getMineSkillMenuItems({ + canPublish, + hasRepositoryInfo, + isPendingReview, + t, + onViewReview, + onDelete, +}: { + canPublish: boolean; + hasRepositoryInfo: boolean; + isPendingReview: boolean; + t: (key: string) => string; + onViewReview: () => void; + onDelete: () => void; +}): MenuProps["items"] { + const items: MenuProps["items"] = []; + if (canPublish && hasRepositoryInfo) { + items.push({ + key: "review", + label: t( + isPendingReview + ? "skillRepository.mine.viewReviewProgress" + : "skillRepository.mine.viewRepositoryStatus" + ), + icon: , + onClick: onViewReview, + }); + } + items.push({ + key: "delete", + label: t("common.delete"), + icon: , + danger: true, + onClick: onDelete, + }); + return items; +} + function MineSkillCard({ skill, onEdit, + onView, onDelete, onApplyListing, onViewReview, }: { skill: MyEditableSkillItem; onEdit: () => void; + onView: () => void; onDelete: () => void; onApplyListing: () => void; onViewReview: () => void; @@ -267,35 +347,34 @@ function MineSkillCard({ const latestRepository = pickReviewDisplayRepositoryInfo( skill.repository_info ?? [] ); + const repositoryInfo = skill.repository_info ?? []; + const hasRepositoryInfo = latestRepository != null; const repositoryStatus = latestRepository?.status ?? "not_shared"; - const hasRepositoryInfo = (skill.repository_info ?? []).length > 0; - const canApplyListing = - !latestRepository || latestRepository.status === "rejected"; - const isEnabled = latestRepository?.status === "shared"; - const canEdit = skill.permission !== "READ_ONLY"; + const hasSharedRepository = repositoryInfo.some( + (info) => info.status === "shared" + ); + const canEdit = + skill.permission !== "READ_ONLY" && skill.permission !== "PRIVATE"; + const canPublish = skill.can_publish === true; const updatedAt = formatRepositoryDate(skill.updated_at ?? skill.update_time); const sourceLabel = getSkillSourceLabel(skill.source, t); const tags = skill.tags?.filter((tag) => tag.trim()) ?? []; const isPendingReview = repositoryStatus === "pending_review"; - const menuItems: MenuProps["items"] = [ - ...(isPendingReview - ? [ - { - key: "review", - label: t("skillRepository.mine.viewReviewProgress"), - icon: , - onClick: onViewReview, - }, - ] - : []), - { - key: "delete", - label: t("common.delete"), - icon: , - danger: true, - onClick: onDelete, - }, - ]; + const canApplyListing = canPublish && !isPendingReview; + const applyButtonLabel = getApplyButtonLabel( + isPendingReview, + hasSharedRepository, + repositoryStatus, + t + ); + const menuItems = getMineSkillMenuItems({ + canPublish, + hasRepositoryInfo, + isPendingReview, + t, + onViewReview, + onDelete, + }); return (
@@ -309,7 +388,7 @@ function MineSkillCard({

{skill.name || t("skillRepository.common.untitled")}

- {hasRepositoryInfo ? ( + {hasSharedRepository ? ( Hub @@ -389,24 +468,28 @@ function MineSkillCard({ type="default" className="min-w-0 flex-1" icon={} - disabled + onClick={onView} > {t("skillRepository.common.view")} )} - + + + +
diff --git a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx index 9551a46e3..12320f305 100644 --- a/frontend/app/[locale]/skill-space/components/RepositoryView.tsx +++ b/frontend/app/[locale]/skill-space/components/RepositoryView.tsx @@ -71,51 +71,52 @@ export function RepositoryView({ isEmpty={listings.length === 0} emptyDescription={t("skillRepository.repository.empty")} > -
- {listings.map((listing) => ( - onDetailClick(listing)} - showAdminMenu={showAdminMenu} - isTakingDown={ - takingDownRepositoryId === listing.skill_repository_id - } - onTakeDown={() => onTakeDown(listing)} - action={ -
- - -
- } - /> - ))} -
+ <> +
+ {listings.map((listing) => ( + onDetailClick(listing)} + showAdminMenu={showAdminMenu || listing.can_take_down === true} + isTakingDown={ + takingDownRepositoryId === listing.skill_repository_id + } + onTakeDown={() => onTakeDown(listing)} + action={ +
+ + +
+ } + /> + ))} +
+ + - - ); } diff --git a/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx b/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx index 58b7f2d79..dca725697 100644 --- a/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx +++ b/frontend/app/[locale]/skill-space/components/ReviewSkillList.tsx @@ -73,103 +73,105 @@ export function ReviewSkillList({ isEmpty={listings.length === 0} emptyDescription={t("skillRepository.review.empty")} > -
-
-
- {t("skillRepository.review.skill")} - {t("skillRepository.review.submitter")} - {t("skillRepository.review.status")} - {t("skillRepository.review.actions")} -
+ <> +
+
+
+ {t("skillRepository.review.skill")} + {t("skillRepository.review.submitter")} + {t("skillRepository.review.status")} + {t("skillRepository.review.actions")} +
-
    - {listings.map((listing) => { - const isUpdating = - updatingRepositoryId === listing.skill_repository_id; - const isPendingReview = listing.status === "pending_review"; +
      + {listings.map((listing) => { + const isUpdating = + updatingRepositoryId === listing.skill_repository_id; + const isPendingReview = listing.status === "pending_review"; - return ( -
    • -
      -
      - -
      -
      -

      - {listing.name || t("skillRepository.common.untitled")} -

      - {listing.description ? ( -

      - {listing.description} -

      - ) : null} + return ( +
    • +
      +
      + +
      +
      +

      + {listing.name || + t("skillRepository.common.untitled")} +

      + {listing.description ? ( +

      + {listing.description} +

      + ) : null} +
      -
-
- {getSubmitterDisplay(listing.submitted_by, t)} -
+
+ {getSubmitterDisplay(listing.submitted_by, t)} +
-
- - {t(REVIEW_STATUS_LABEL_KEYS[listing.status])} - -
+
+ + {t(REVIEW_STATUS_LABEL_KEYS[listing.status])} + +
-
- - {isPendingReview ? ( - <> - - - - ) : null} -
- - ); - })} - +
+ + {isPendingReview ? ( + <> + + + + ) : null} +
+ + ); + })} + +
-
+ + - - ); } diff --git a/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx b/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx index 43d812c4a..fec41b927 100644 --- a/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx +++ b/frontend/app/[locale]/skill-space/components/SkillRepositoryControls.tsx @@ -67,26 +67,45 @@ export function PaginationBar({ onPageChange: (page: number) => void; }) { const { t } = useTranslation("common"); - const totalPages = Math.max(1, Math.ceil(total / pageSize)); - if (total <= pageSize) return null; + const totalPages = total > 0 ? Math.ceil(total / pageSize) : 0; + if (totalPages <= 1) return null; return ( -
+
+ {Array.from({ length: totalPages }, (_, index) => index + 1).map( + (pageNumber) => ( + + ) + )}
); } diff --git a/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx b/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx index cefc72fb5..45b08eacd 100644 --- a/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx +++ b/frontend/app/[locale]/skill-space/components/SkillRepositoryDetailModal.tsx @@ -28,8 +28,7 @@ export function SkillRepositoryDetailModal({ onRetry: () => void; }) { const { t } = useTranslation("common"); - const author = - detail?.submitted_by?.trim() || t("skillRepository.detail.unknownAuthor"); + const author = detail?.author?.trim(); const updatedAt = formatRepositoryDate(detail?.updated_at) || formatRepositoryDate(detail?.created_at) || @@ -72,10 +71,12 @@ export function SkillRepositoryDetailModal({
-

- - {author} -

+ {author ? ( +

+ + {author} +

+ ) : null} diff --git a/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts b/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts index eb0a0f9bf..124267c1b 100644 --- a/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts +++ b/frontend/app/[locale]/skill-space/components/skillRepositoryShared.ts @@ -72,11 +72,13 @@ export function pickReviewDisplayRepositoryInfo( const rejected = pickLatestRepositoryInfo( items.filter((item) => item.status === "rejected") ); - if (rejected) return rejected; - return pickLatestRepositoryInfo( + const shared = pickLatestRepositoryInfo( items.filter((item) => item.status === "shared") ); + if (shared) return shared; + + return rejected; } export function isCancelableRepositoryStatus( diff --git a/frontend/app/[locale]/skill-space/page.tsx b/frontend/app/[locale]/skill-space/page.tsx index ffaa3adb5..77c528cfa 100644 --- a/frontend/app/[locale]/skill-space/page.tsx +++ b/frontend/app/[locale]/skill-space/page.tsx @@ -22,11 +22,13 @@ import { ApiError } from "@/services/api"; import { deleteSkillByName } from "@/services/skillService"; import { cn } from "@/lib/utils"; import type { + MineOwnershipFilter, MyEditableSkillItem, MySkillRepositoryInfoItem, SkillRepositoryListingItem, SkillRepositoryListingStatus, } from "@/types/skillRepository"; +import type { Skill } from "@/types/agentConfig"; import { CountBadge } from "./components/SkillRepositoryControls"; import { SkillRepositoryDetailModal } from "./components/SkillRepositoryDetailModal"; import { MineSkillsView } from "./components/MineSkillsView"; @@ -37,6 +39,7 @@ import { STATUS_LABEL_KEYS, } from "./components/skillRepositoryShared"; import SkillBuildModal from "../agents/components/agentConfig/SkillBuildModal"; +import SkillDetailModal from "../agents/components/agentConfig/SkillDetailModal"; enum SkillRepositoryTab { REPOSITORY = "repository", @@ -47,7 +50,6 @@ enum SkillRepositoryTab { const REPOSITORY_PAGE_SIZE = 6; const MINE_PAGE_SIZE = 6; const REVIEW_PAGE_SIZE = 10; - const skillRepositoryTheme = { token: { colorPrimary: "#2563eb", colorInfo: "#3b82f6", borderRadius: 12 }, }; @@ -73,6 +75,8 @@ export default function SkillRepositoryPage() { const [repositoryPage, setRepositoryPage] = useState(1); const [repositorySearch, setRepositorySearch] = useState(""); const [minePage, setMinePage] = useState(1); + const [mineOwnership, setMineOwnership] = + useState("all"); const [mineSearch, setMineSearch] = useState(""); const [reviewPage, setReviewPage] = useState(1); const [detailRepositoryId, setDetailRepositoryId] = useState( @@ -82,6 +86,9 @@ export default function SkillRepositoryPage() { const [editingSkill, setEditingSkill] = useState( null ); + const [viewingSkill, setViewingSkill] = useState( + null + ); const [copyListing, setCopyListing] = useState(null); const [copyTargetName, setCopyTargetName] = useState(""); @@ -103,17 +110,20 @@ export default function SkillRepositoryPage() { const mineParams = useMemo( () => ({ - ownership: "all" as const, + ownership: mineOwnership, page: minePage, page_size: MINE_PAGE_SIZE, ...(mineSearch.trim() ? { search: mineSearch.trim() } : {}), - ...(!mineSearch.trim() ? { new_skill_padding: true } : {}), + ...(mineOwnership === "all" && !mineSearch.trim() + ? { new_skill_padding: true } + : {}), }), - [minePage, mineSearch] + [mineOwnership, minePage, mineSearch] ); const reviewParams = useMemo( () => ({ + status: "pending_review" as const, page: reviewPage, page_size: REVIEW_PAGE_SIZE, sort_by_update_time: true, @@ -232,7 +242,9 @@ export default function SkillRepositoryPage() { const handleInstall = (listing: SkillRepositoryListingItem) => { const baseName = listing.name?.trim() || "Skill"; setCopyListing(listing); - setCopyTargetName(t("skillRepository.copy.defaultName", { name: baseName })); + setCopyTargetName( + t("skillRepository.copy.defaultName", { name: baseName }) + ); setCopyNameError(null); }; @@ -271,7 +283,9 @@ export default function SkillRepositoryPage() { return; } message.error( - error instanceof Error ? error.message : t("skillRepository.copy.failed") + error instanceof Error + ? error.message + : t("skillRepository.copy.failed") ); } }; @@ -316,58 +330,6 @@ export default function SkillRepositoryPage() { ); }; - const getActiveRepositoryInfo = (skill?: MyEditableSkillItem | null) => - (skill?.repository_info ?? []).filter( - (info) => info.status === "shared" || info.status === "pending_review" - ); - - const confirmEditListedSkill = async ( - skill: MyEditableSkillItem - ): Promise => { - const activeInfo = getActiveRepositoryInfo(skill); - if (activeInfo.length === 0) { - return true; - } - - const hasShared = activeInfo.some((info) => info.status === "shared"); - const confirmed = await new Promise((resolve) => { - modal.confirm({ - title: hasShared - ? t("skillRepository.edit.confirmTakeDownTitle") - : t("skillRepository.edit.confirmWithdrawTitle"), - content: hasShared - ? t("skillRepository.edit.confirmTakeDownContent") - : t("skillRepository.edit.confirmWithdrawContent"), - okText: t("skillRepository.edit.continueSave"), - cancelText: t("common.cancel"), - onOk: () => resolve(true), - onCancel: () => resolve(false), - }); - }); - if (!confirmed) { - return false; - } - - try { - await Promise.all( - activeInfo.map((info) => - updateStatusMutation.mutateAsync({ - skillRepositoryId: info.skill_repository_id, - status: "not_shared", - }) - ) - ); - return true; - } catch (error) { - message.error( - error instanceof Error - ? error.message - : t("skillRepository.common.statusUpdateFailed") - ); - return false; - } - }; - const handleSkillBuildSuccess = async () => { await refetchMine().catch(() => {}); setEditingSkill(null); @@ -497,6 +459,11 @@ export default function SkillRepositoryPage() { { + setMineOwnership(ownership); + setMinePage(1); + }} searchQuery={mineSearch} onSearchChange={(value) => { setMineSearch(value); @@ -518,6 +485,7 @@ export default function SkillRepositoryPage() { setEditingSkill(skill); setSkillBuildOpen(true); }} + onViewSkill={(skill) => setViewingSkill(skill)} onDeleteSkill={async (skill) => { const name = skill.name?.trim(); if (!name) { @@ -643,7 +611,21 @@ export default function SkillRepositoryPage() { setEditingSkill(null); }} onSuccess={handleSkillBuildSuccess} - onBeforeEditSave={confirmEditListedSkill} + /> + setViewingSkill(null)} /> ); diff --git a/frontend/hooks/agent/useSkillList.ts b/frontend/hooks/agent/useSkillList.ts index b0b101a98..5f69e7259 100644 --- a/frontend/hooks/agent/useSkillList.ts +++ b/frontend/hooks/agent/useSkillList.ts @@ -2,7 +2,6 @@ import { useQuery, useQueryClient } from "@tanstack/react-query"; import { fetchSkills } from "@/services/agentConfigService"; import { useMemo } from "react"; import { Skill, SkillGroup } from "@/types/agentConfig"; -import { useAuthorizationContext } from "@/components/providers/AuthorizationProvider"; import { useTranslation } from "react-i18next"; const OFFICIAL_SKILL_SOURCES = new Set(["official", "官方"]); @@ -17,7 +16,6 @@ export function useSkillList(options?: { staleTime?: number; }) { const queryClient = useQueryClient(); - const { user } = useAuthorizationContext(); const { t } = useTranslation("common"); const query = useQuery({ @@ -34,14 +32,7 @@ export function useSkillList(options?: { }); const skills = query.data ?? []; - const currentUserId = user?.id ?? null; - - const availableSkills = useMemo(() => { - return skills.filter((skill: Skill) => { - if (isOfficialSkill(skill)) return true; - return Boolean(currentUserId && skill.created_by === currentUserId); - }); - }, [skills, currentUserId]); + const availableSkills = skills; const groupedSkills = useMemo(() => { const groups: SkillGroup[] = []; diff --git a/frontend/public/locales/en/common.json b/frontend/public/locales/en/common.json index 8049b0f68..3f4404262 100644 --- a/frontend/public/locales/en/common.json +++ b/frontend/public/locales/en/common.json @@ -1427,6 +1427,7 @@ "skillManagement.message.createSuccess": "Skill created successfully", "skillManagement.message.updateSuccess": "Skill updated successfully", "skillManagement.message.submitFailed": "Failed to submit skill", + "skillManagement.message.loadFilesFailed": "Failed to load Skill files. Saving is disabled to prevent data loss.", "skillManagement.message.pleaseSelectFile": "Please select a file to upload", "skillManagement.message.chatError": "Failed to generate skill, please try again", "skillManagement.message.nameExists": "Skill name already exists. Please change the name", @@ -1443,6 +1444,7 @@ "skillManagement.delete.confirmTitle": "Confirm Delete Skill", "skillManagement.delete.confirmContent": "Are you sure you want to delete skill 「{{skillName}}」? This action cannot be undone.", "skillManagement.delete.success": "Skill deleted successfully", + "skillManagement.noEditPermission": "No permission to edit this skill", "skillManagement.delete.failed": "Failed to delete skill", "mcpConfig.modal.title": "MCP Server Configuration", "mcpConfig.modal.close": "Close", @@ -2278,10 +2280,11 @@ "skillRepository.mine.applySuccess": "Listing request submitted", "skillRepository.mine.applyError": "Failed to submit review", "skillRepository.mine.button.apply": "List", - "skillRepository.mine.button.listed": "Listed", + "skillRepository.mine.button.reapply": "List again", "skillRepository.mine.deleteTitle": "Delete Skill", "skillRepository.mine.deleteContent": "Delete {{name}}?", "skillRepository.mine.viewReviewProgress": "View review progress", + "skillRepository.mine.viewRepositoryStatus": "View repository status", "skillRepository.mine.takeDownSuccess": "Taken down", "skillRepository.mine.withdrawSuccess": "Listing request withdrawn", "skillRepository.copy.defaultName": "{{name}} Copy", @@ -2299,9 +2302,7 @@ "skillRepository.delete.emptyName": "Skill name is empty and cannot be deleted", "skillRepository.delete.failed": "Delete failed", "skillRepository.delete.success": "Deleted successfully", - "skillRepository.edit.confirmTakeDownTitle": "Saving will take this Skill down", "skillRepository.edit.confirmWithdrawTitle": "Saving will withdraw review", - "skillRepository.edit.confirmTakeDownContent": "This Skill is listed. Saving changes will take it down, and you will need to submit it for review again.", "skillRepository.edit.confirmWithdrawContent": "This Skill is under review. Saving changes will withdraw the review, and you will need to submit it again.", "skillRepository.edit.continueSave": "Continue saving", "skillRepository.review.status.notShared": "Not listed", diff --git a/frontend/public/locales/zh/common.json b/frontend/public/locales/zh/common.json index 9ccdcae50..cd466fd02 100644 --- a/frontend/public/locales/zh/common.json +++ b/frontend/public/locales/zh/common.json @@ -1398,6 +1398,7 @@ "skillManagement.message.createSuccess": "技能创建成功", "skillManagement.message.updateSuccess": "技能更新成功", "skillManagement.message.submitFailed": "提交技能失败", + "skillManagement.message.loadFilesFailed": "加载 Skill 文件失败，无法保存以避免丢失文件", "skillManagement.message.pleaseSelectFile": "请选择要上传的文件", "skillManagement.message.chatError": "生成技能失败，请重试", "skillManagement.message.nameExists": "技能名称已存在，请修改名称", @@ -1414,6 +1415,7 @@ "skillManagement.delete.confirmTitle": "确认删除技能", "skillManagement.delete.confirmContent": "确定要删除技能「{{skillName}}」吗？删除后无法恢复。", "skillManagement.delete.success": "技能删除成功", + "skillManagement.noEditPermission": "无技能编辑权限", "skillManagement.delete.failed": "技能删除失败", "mcpConfig.modal.title": "MCP服务器配置", "mcpConfig.modal.close": "关闭", @@ -2404,10 +2406,11 @@ "skillRepository.mine.applySuccess": "已提交上架申请", "skillRepository.mine.applyError": "提交审批失败", "skillRepository.mine.button.apply": "上架", - "skillRepository.mine.button.listed": "已上架", + "skillRepository.mine.button.reapply": "重新上架", "skillRepository.mine.deleteTitle": "删除 Skill", "skillRepository.mine.deleteContent": "确认删除 {{name}}？", "skillRepository.mine.viewReviewProgress": "查看审批进度", + "skillRepository.mine.viewRepositoryStatus": "查看仓库状态", "skillRepository.mine.takeDownSuccess": "已下架", "skillRepository.mine.withdrawSuccess": "已撤回申请", "skillRepository.copy.defaultName": "{{name}} 副本", @@ -2425,9 +2428,7 @@ "skillRepository.delete.emptyName": "Skill 名称为空，无法删除", "skillRepository.delete.failed": "删除失败", "skillRepository.delete.success": "删除成功", - "skillRepository.edit.confirmTakeDownTitle": "保存后将自动下架", "skillRepository.edit.confirmWithdrawTitle": "保存后将撤回审核", - "skillRepository.edit.confirmTakeDownContent": "该 Skill 已上架，保存修改后将自动下架，需要重新提交审核。", "skillRepository.edit.confirmWithdrawContent": "该 Skill 正在审核中，保存修改后将撤回审核，需要重新提交。", "skillRepository.edit.continueSave": "继续保存", "skillRepository.review.status.notShared": "未上架", diff --git a/frontend/services/agentConfigService.ts b/frontend/services/agentConfigService.ts index d88ddf757..e2a712509 100644 --- a/frontend/services/agentConfigService.ts +++ b/frontend/services/agentConfigService.ts @@ -1097,6 +1097,11 @@ export const fetchSkills = async (tenantId?: string | null) => { config_schemas: skill.config_schemas ?? null, config_values: skill.config_values ?? null, tool_ids: Array.isArray(skill.tool_ids) ? skill.tool_ids.map(Number) : [], + group_ids: Array.isArray(skill.group_ids) + ? skill.group_ids.map(Number) + : [], + ingroup_permission: skill.ingroup_permission ?? null, + permission: skill.permission ?? "READ_ONLY", created_by: skill.created_by ?? null, updated_by: skill.updated_by ?? null, update_time: skill.update_time, @@ -1248,6 +1253,8 @@ export const createSkill = async (skillData: { source?: string; tags?: string[]; content?: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }) => { try { @@ -1261,6 +1268,12 @@ export const createSkill = async (skillData: { if (skillData.files && skillData.files.length > 0) { requestBody.files = skillData.files; } + if (skillData.group_ids !== undefined) { + requestBody.group_ids = skillData.group_ids; + } + if (skillData.ingroup_permission !== undefined) { + requestBody.ingroup_permission = skillData.ingroup_permission; + } const response = await fetch(API_ENDPOINTS.skills.create, { method: "POST", @@ -1308,6 +1321,8 @@ export const updateSkill = async ( tags?: string[]; content?: string; config_values?: Record; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }, tenantId?: string | null @@ -1323,6 +1338,10 @@ export const updateSkill = async ( requestBody.content = skillData.content; if (skillData.config_values !== undefined) requestBody.config_values = skillData.config_values; + if (skillData.group_ids !== undefined) + requestBody.group_ids = skillData.group_ids; + if (skillData.ingroup_permission !== undefined) + requestBody.ingroup_permission = skillData.ingroup_permission; if (skillData.files !== undefined) requestBody.files = skillData.files; const url = tenantId @@ -1369,6 +1388,8 @@ export const updateSkillById = async ( tags?: string[]; content?: string; config_values?: Record; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: Array<{ path: string; content: string }>; }, tenantId?: string | null @@ -1385,6 +1406,10 @@ export const updateSkillById = async ( requestBody.content = skillData.content; if (skillData.config_values !== undefined) requestBody.config_values = skillData.config_values; + if (skillData.group_ids !== undefined) + requestBody.group_ids = skillData.group_ids; + if (skillData.ingroup_permission !== undefined) + requestBody.ingroup_permission = skillData.ingroup_permission; if (skillData.files !== undefined) requestBody.files = skillData.files; const url = tenantId diff --git a/frontend/services/skillService.ts b/frontend/services/skillService.ts index ac050d8ca..ec3d94f71 100644 --- a/frontend/services/skillService.ts +++ b/frontend/services/skillService.ts @@ -30,6 +30,8 @@ export interface SkillData { source: string; tags: string[]; content: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; files?: SkillFileContent[]; } @@ -42,6 +44,8 @@ export interface SkillListItem { description?: string; tags: string[]; content?: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; config_values: Record | null; config_schemas: unknown[] | null; source: string; @@ -197,6 +201,10 @@ export async function fetchSkillsList( const toolIds = Array.isArray(rawToolIds) ? rawToolIds.map((id) => Number(id)).filter((n) => !Number.isNaN(n)) : []; + const rawGroupIds = s.group_ids; + const groupIds = Array.isArray(rawGroupIds) + ? rawGroupIds.map((id) => Number(id)).filter((n) => !Number.isNaN(n)) + : []; return { skill_id: Number.isNaN(skillId) ? 0 : skillId, name: String(s.name ?? ""), @@ -208,6 +216,11 @@ export async function fetchSkillsList( config_values, source: String(s.source ?? "custom"), tool_ids: toolIds, + group_ids: groupIds, + ingroup_permission: + s.ingroup_permission !== undefined + ? (s.ingroup_permission as "EDIT" | "READ_ONLY" | "PRIVATE" | null) + : undefined, created_by: s.created_by !== undefined ? (s.created_by as string | null) @@ -248,6 +261,8 @@ export const submitSkillForm = async ( source: values.source, tags: values.tags, content: values.content, + group_ids: values.group_ids, + ingroup_permission: values.ingroup_permission, files: values.files, }); } else { @@ -257,6 +272,8 @@ export const submitSkillForm = async ( source: values.source, tags: values.tags, content: values.content, + group_ids: values.group_ids, + ingroup_permission: values.ingroup_permission, files: values.files, }); } diff --git a/frontend/types/agentConfig.ts b/frontend/types/agentConfig.ts index c67556c85..effe5d39d 100644 --- a/frontend/types/agentConfig.ts +++ b/frontend/types/agentConfig.ts @@ -271,6 +271,9 @@ export interface Skill { config_schemas?: SkillParam[] | null; config_values?: Record | null; tool_ids?: number[]; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; + permission?: "EDIT" | "READ_ONLY"; created_by?: string | null; updated_by?: string | null; update_time?: string; diff --git a/frontend/types/skill.ts b/frontend/types/skill.ts index 54a4e2d40..73dc9345a 100644 --- a/frontend/types/skill.ts +++ b/frontend/types/skill.ts @@ -41,6 +41,8 @@ export interface SkillFormData { source: string; tags: string[]; content: string; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; } /** diff --git a/frontend/types/skillRepository.ts b/frontend/types/skillRepository.ts index 6440edf60..b8703685c 100644 --- a/frontend/types/skillRepository.ts +++ b/frontend/types/skillRepository.ts @@ -18,7 +18,9 @@ export interface SkillRepositoryListingItem { tags?: string[]; downloads?: number; category_id?: number | null; + author?: string | null; submitted_by?: string | null; + can_take_down?: boolean; } export interface SkillRepositoryListingPagination { @@ -66,13 +68,16 @@ export interface MyEditableSkillItem { description?: string | null; source?: string | null; tags?: string[]; + group_ids?: number[]; + ingroup_permission?: "EDIT" | "READ_ONLY" | "PRIVATE" | null; created_by?: string | null; updated_by?: string | null; create_time?: string | null; update_time?: string | null; updated_at?: string | null; downloads?: number; - permission?: "EDIT" | "READ_ONLY"; + permission?: "EDIT" | "READ_ONLY" | "PRIVATE"; + can_publish?: boolean; repository_info: MySkillRepositoryInfoItem[]; } diff --git a/test/backend/app/test_skill_app.py b/test/backend/app/test_skill_app.py index 35e585273..e5d66ac6e 100644 --- a/test/backend/app/test_skill_app.py +++ b/test/backend/app/test_skill_app.py @@ -142,6 +142,8 @@ class MockSkillCreateRequest(BaseModel): config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[Dict[str, str]]] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None class MockSkillFileData(BaseModel): path: str @@ -158,6 +160,8 @@ class MockSkillUpdateRequest(BaseModel): config_schemas: Optional[Dict[str, Any]] = None config_values: Optional[Dict[str, Any]] = None files: Optional[List[MockSkillFileData]] = None + group_ids: Optional[List[int]] = None + ingroup_permission: Optional[str] = None class MockSkillResponse(BaseModel): skill_id: Optional[int] = None @@ -198,6 +202,13 @@ def __init__(self): services_skill_service_mock.update_skill_list = MagicMock() services_skill_service_mock.get_official_skills_with_status = MagicMock(return_value=[]) services_skill_service_mock.install_skills_from_zip_for_tenant = MagicMock(return_value=[]) + + +def setup_function(): + """Restore module-level service stubs after tests that isolate imports.""" + sys.modules['services'] = services_mock + sys.modules['services.skill_service'] = services_skill_service_mock + sys.modules['services.asset_owner_visibility'] = services_asset_owner_visibility_mock services_asset_owner_visibility_mock.can_view_skill = MagicMock(return_value=True) # Mock utils @@ -267,7 +278,7 @@ def test_list_skills_success(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [ + mock_service.list_visible_skills.return_value = [ {"skill_id": 1, "name": "skill1", "description": "Desc1"}, {"skill_id": 2, "name": "skill2", "description": "Desc2"} ] @@ -290,7 +301,7 @@ def test_list_skills_empty(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [] + mock_service.list_visible_skills.return_value = [] app = FastAPI() app.include_router(skill_app.router) @@ -310,7 +321,7 @@ def test_list_skills_error(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.side_effect = SkillException("Database error") + mock_service.list_visible_skills.side_effect = SkillException("Database error") app = FastAPI() app.include_router(skill_app.router) @@ -327,7 +338,7 @@ def test_list_skills_super_admin_with_tenant_id(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.return_value = [ + mock_service.list_visible_skills.return_value = [ {"skill_id": 10, "name": "admin_skill", "description": "Admin desc"} ] @@ -345,7 +356,10 @@ def test_list_skills_super_admin_with_tenant_id(self, mocker): assert "skills" in data assert len(data["skills"]) == 1 # Verify the service was called with the target tenant_id, not super_tenant - mock_service.list_skills.assert_called_once_with(tenant_id="target_tenant") + mock_service.list_visible_skills.assert_called_once_with( + tenant_id="target_tenant", + user_id="super_user", + ) # ===== Create Skill Endpoint Tests ===== @@ -905,6 +919,29 @@ def test_update_skill_not_found(self, mocker): assert response.status_code == 404 + def test_update_skill_from_file_forbidden(self, mocker): + from backend.apps.skill_app import ForbiddenError + + with patch('backend.apps.skill_app.SkillService') as mock_service_class: + with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: + mock_auth.return_value = ("user123", "tenant123") + mock_service = MagicMock() + mock_service_class.return_value = mock_service + mock_service.update_skill_from_file.side_effect = ForbiddenError( + "Not authorized" + ) + + app = FastAPI() + app.include_router(skill_app.router) + client = TestClient(app) + response = client.put( + "/skills/test/upload", + files={"file": ("test.md", b"# Skill", "text/markdown")}, + headers={"Authorization": "Bearer token123"}, + ) + + assert response.status_code == 403 + # ===== Update Skill Instance Endpoint Tests ===== class TestUpdateSkillInstanceEndpoint: @@ -1095,7 +1132,7 @@ def test_unexpected_error_in_list_skills(self, mocker): with patch('backend.apps.skill_app.SkillService') as mock_service_class: mock_service = MagicMock() mock_service_class.return_value = mock_service - mock_service.list_skills.side_effect = Exception("Unexpected error") + mock_service.list_visible_skills.side_effect = Exception("Unexpected error") app = FastAPI() app.include_router(skill_app.router) @@ -2271,7 +2308,7 @@ def test_install_skills_success(self, mocker): """Test successful skill installation.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = ["skill1", "skill2"] app = FastAPI() @@ -2298,7 +2335,7 @@ def test_install_skills_empty_list(self, mocker): """Test installing empty skill list.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = [] app = FastAPI() @@ -2337,7 +2374,7 @@ def test_install_skills_super_admin_with_tenant_id(self, mocker): """Test super admin installing skills for a specific tenant via tenant_id query param.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("super_user", "super_tenant") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.return_value = ["skill1"] app = FastAPI() @@ -2364,7 +2401,7 @@ def test_install_skills_error(self, mocker): """Test installing skills with error.""" with patch('backend.apps.skill_app.get_current_user_id') as mock_auth: mock_auth.return_value = ("user123", "tenant123") - with patch('services.skill_service.install_skills_from_zip_for_tenant') as mock_install: + with patch('backend.apps.skill_app.install_skills_from_zip_for_tenant') as mock_install: mock_install.side_effect = Exception("Installation failed") app = FastAPI() @@ -2824,6 +2861,8 @@ def test_build_skill_update_data_with_all_fields(self, mocker): request.config_schemas = {"key": "value"} request.config_values = {"param": "val"} request.files = [mock_file_data] + request.group_ids = [10, 20] + request.ingroup_permission = "READ_ONLY" result = _build_skill_update_data(request) @@ -2835,6 +2874,8 @@ def test_build_skill_update_data_with_all_fields(self, mocker): assert result["config_schemas"] == {"key": "value"} assert result["config_values"] == {"param": "val"} assert result["files"] == [{"path": "test.py", "content": "code"}] + assert result["group_ids"] == [10, 20] + assert result["ingroup_permission"] == "READ_ONLY" def test_build_skill_update_data_with_partial_fields(self, mocker): """Test _build_skill_update_data with only some fields provided.""" @@ -2849,6 +2890,8 @@ def test_build_skill_update_data_with_partial_fields(self, mocker): request.config_schemas = None request.config_values = None request.files = None + request.group_ids = None + request.ingroup_permission = None result = _build_skill_update_data(request) @@ -2869,6 +2912,8 @@ def test_build_skill_update_data_with_empty_files(self, mocker): request.config_schemas = None request.config_values = None request.files = [] # Empty list is not None, so it gets included + request.group_ids = None + request.ingroup_permission = None result = _build_skill_update_data(request) diff --git a/test/backend/app/test_skill_repository_app.py b/test/backend/app/test_skill_repository_app.py index 8171bb05e..60f75a452 100644 --- a/test/backend/app/test_skill_repository_app.py +++ b/test/backend/app/test_skill_repository_app.py @@ -95,6 +95,7 @@ def test_list_skill_repository_listings_api_passes_filters(mocker, mock_auth_hea mock_get_user_id.assert_called_once_with(mock_auth_header["Authorization"]) mock_list.assert_called_once_with( "tenant-1", + user_id="user-1", status="pending_review", skill_id=3, category_id=2, diff --git a/test/backend/database/test_skill_db.py b/test/backend/database/test_skill_db.py index 8ff2806e9..a95131784 100644 --- a/test/backend/database/test_skill_db.py +++ b/test/backend/database/test_skill_db.py @@ -1,6 +1,7 @@ """Unit tests for backend.database.skill_db module.""" import sys import os +import types sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../backend")) sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../sdk")) @@ -45,11 +46,18 @@ utils_skill_params_mock = MagicMock() utils_skill_params_mock.strip_params_comments_for_db = lambda x: x +utils_str_utils_mock = types.ModuleType('utils.str_utils') +utils_str_utils_mock.convert_list_to_string = lambda items: "" if items is None else ",".join(str(item) for item in items) +utils_str_utils_mock.convert_string_to_list = ( + lambda items: [] if not items else [int(item.strip()) for item in items.split(",") if item.strip().isdigit()] +) sys.modules['utils'] = MagicMock() sys.modules['utils.auth_utils'] = MagicMock() sys.modules['utils.skill_params_utils'] = utils_skill_params_mock +sys.modules['utils.str_utils'] = utils_str_utils_mock sys.modules['backend.utils'] = MagicMock() sys.modules['backend.utils.skill_params_utils'] = utils_skill_params_mock +sys.modules['backend.utils.str_utils'] = utils_str_utils_mock from backend.database.skill_db import ( _params_value_for_db, @@ -115,6 +123,8 @@ def __init__(self, **kwargs): self.config_schemas = kwargs.get('config_schemas', {}) self.config_values = kwargs.get('config_values', {}) self.source = kwargs.get('source', 'custom') + self.group_ids = kwargs.get('group_ids', '') + self.ingroup_permission = kwargs.get('ingroup_permission') self.created_by = kwargs.get('created_by', 'creator1') self.create_time = kwargs.get('create_time', datetime.now()) self.updated_by = kwargs.get('updated_by', 'updater1') @@ -1053,7 +1063,7 @@ def test_list_skills_returns_all(self, monkeypatch, mock_session): mock_all = MagicMock() mock_all.return_value = [skill1, skill2] mock_filter = MagicMock() - mock_filter.all = mock_all + mock_filter.order_by.return_value.all = mock_all query.filter.return_value = mock_filter mock_ctx = MagicMock() @@ -1072,6 +1082,10 @@ def test_list_skills_returns_all(self, monkeypatch, mock_session): assert result[0]['name'] == 'skill1' assert result[0]['tool_ids'] == [1, 2] assert result[1]['tool_ids'] == [] + mock_filter.order_by.assert_called_once_with( + db_models_mock.SkillInfo.create_time.desc(), + db_models_mock.SkillInfo.skill_id.desc(), + ) def test_list_skills_empty(self, monkeypatch, mock_session): """Test listing when no skills exist.""" @@ -1080,7 +1094,7 @@ def test_list_skills_empty(self, monkeypatch, mock_session): mock_all = MagicMock() mock_all.return_value = [] mock_filter = MagicMock() - mock_filter.all = mock_all + mock_filter.order_by.return_value.all = mock_all query.filter.return_value = mock_filter mock_ctx = MagicMock() @@ -2274,6 +2288,20 @@ def test_config_fields(self): assert "config_schemas" in result assert "config_values" in result + def test_group_permission_fields(self): + result = _build_skill_update_values( + {"group_ids": [10, 20], "ingroup_permission": "READ_ONLY"}, + "admin", + ) + + assert result["group_ids"] == "10,20" + assert result["ingroup_permission"] == "READ_ONLY" + + def test_group_ids_string_is_preserved(self): + result = _build_skill_update_values({"group_ids": "10,20"}, "admin") + + assert result["group_ids"] == "10,20" + def test_empty_skill_data(self): result = _build_skill_update_values({}, "user1") assert "update_time" in result diff --git a/test/backend/database/test_skill_repository_db.py b/test/backend/database/test_skill_repository_db.py index d0129563b..419ca3d57 100644 --- a/test/backend/database/test_skill_repository_db.py +++ b/test/backend/database/test_skill_repository_db.py @@ -108,14 +108,45 @@ def test_get_repository_by_skill_id_with_optional_tenant(monkeypatch, mock_sessi session, query = mock_session record = MagicMock(payload={"skill_id": 8}) query.filter.return_value = query + query.order_by.return_value = query query.first.return_value = record _patch_session(monkeypatch, session) assert repo_db.get_skill_repository_by_skill_id( 8, publisher_tenant_id="tenant-1", + statuses={"pending_review", "rejected"}, ) == {"skill_id": 8} - assert query.filter.call_count == 2 + assert query.filter.call_count == 3 + query.order_by.assert_called_once_with(repo_db.SkillRepository.update_time.desc()) + + +def test_get_repository_by_skill_id_without_status_filter(monkeypatch, mock_session): + session, query = mock_session + query.filter.return_value = query + query.order_by.return_value = query + query.first.return_value = None + _patch_session(monkeypatch, session) + + assert repo_db.get_skill_repository_by_skill_id(8) is None + assert query.filter.call_count == 1 + + +def test_reset_repository_status_resets_matching_peer_records(monkeypatch, mock_session): + session, _ = mock_session + session.execute.return_value.rowcount = 2 + _patch_session(monkeypatch, session) + statement = _patch_update(monkeypatch) + + affected = repo_db.reset_skill_repository_status( + repository_id=5, + skill_id=8, + status="pending_review", + publisher_tenant_id="tenant-1", + ) + + assert affected == 2 + assert statement.values.call_args.kwargs == {"status": "not_shared"} def test_list_repository_summaries_with_filters(monkeypatch, mock_session): diff --git a/test/backend/services/test_skill_repository_service.py b/test/backend/services/test_skill_repository_service.py index 5fabc7063..95a3d7c78 100644 --- a/test/backend/services/test_skill_repository_service.py +++ b/test/backend/services/test_skill_repository_service.py @@ -16,6 +16,19 @@ if str(_BACKEND_ROOT) not in sys.path: sys.path.insert(0, str(_BACKEND_ROOT)) +_MOCKED_MODULE_NAMES = [ + "database.skill_repository_db", + "database.group_db", + "database.skill_db", + "database.user_tenant_db", + "services.skill_service", + "utils.str_utils", +] +_ORIGINAL_MODULES = { + name: sys.modules.get(name) + for name in _MOCKED_MODULE_NAMES +} + _consts_package = sys.modules.get("consts") if _consts_package is not None and not hasattr(_consts_package, "__path__"): _consts_package.__path__ = [] @@ -47,9 +60,11 @@ if not hasattr(consts_const_module, "CAN_EDIT_ALL_USER_ROLES"): consts_const_module.CAN_EDIT_ALL_USER_ROLES = {"ADMIN"} if not hasattr(consts_const_module, "PERMISSION_EDIT"): - consts_const_module.PERMISSION_EDIT = "edit" + consts_const_module.PERMISSION_EDIT = "EDIT" if not hasattr(consts_const_module, "PERMISSION_READ"): - consts_const_module.PERMISSION_READ = "read" + consts_const_module.PERMISSION_READ = "READ_ONLY" + if not hasattr(consts_const_module, "PERMISSION_PRIVATE"): + consts_const_module.PERMISSION_PRIVATE = "PRIVATE" _skill_repo_db_mock = MagicMock() _skill_repo_db_mock.get_skill_repository_by_id_and_publisher = MagicMock() @@ -58,10 +73,25 @@ _skill_repo_db_mock.insert_skill_repository_record = MagicMock(return_value=1) _skill_repo_db_mock.list_skill_repository_by_skill_ids = MagicMock(return_value=[]) _skill_repo_db_mock.list_skill_repository_summaries = MagicMock() +_skill_repo_db_mock.reset_skill_repository_status = MagicMock(return_value=0) _skill_repo_db_mock.update_skill_repository_by_id = MagicMock(return_value=1) _skill_repo_db_mock.update_skill_repository_status_by_id = MagicMock(return_value=1) sys.modules["database.skill_repository_db"] = _skill_repo_db_mock +_group_db_mock = MagicMock() +_group_db_mock.query_group_ids_by_user = MagicMock(return_value=[]) +sys.modules["database.group_db"] = _group_db_mock + +_utils_str_utils_mock = types.ModuleType("utils.str_utils") +_utils_str_utils_mock.convert_string_to_list = MagicMock( + side_effect=lambda value: [ + int(item) + for item in str(value or "").split(",") + if item.strip().isdigit() + ] +) +sys.modules["utils.str_utils"] = _utils_str_utils_mock + _skill_db_mock = MagicMock() _skill_db_mock.get_skill_by_name = MagicMock(return_value=None) sys.modules["database.skill_db"] = _skill_db_mock @@ -102,6 +132,29 @@ def create_skill_from_zip_bytes(self, **kwargs): def list_skills(self, tenant_id=None): return [] + def list_visible_skills(self, *, tenant_id=None, user_id): + user_tenant = _user_tenant_db_mock.get_user_tenant_by_user_id(user_id) or {} + user_role = str(user_tenant.get("user_role") or "USER") + user_group_ids = set(_group_db_mock.query_group_ids_by_user(user_id) or []) + skills = [ + skill + for skill in self.list_skills(tenant_id=tenant_id) + if user_role in {"ADMIN", "SUPER_ADMIN"} + or str(skill.get("created_by")) == str(user_id) + or ( + skill.get("ingroup_permission") != "PRIVATE" + and bool(user_group_ids.intersection(skill.get("group_ids") or [])) + ) + ] + for skill in skills: + skill["permission"] = ( + "EDIT" + if user_role in {"ADMIN", "SUPER_ADMIN"} + or str(skill.get("created_by")) == str(user_id) + else skill.get("ingroup_permission") or "READ_ONLY" + ) + return skills + _skill_service_module_mock = MagicMock() _skill_service_module_mock.SkillService = _SkillServiceMock @@ -136,6 +189,14 @@ def __init__(self, duplicate_names): from backend.services import skill_repository_service as srs +def teardown_module(): + for name, original in _ORIGINAL_MODULES.items(): + if original is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = original + + def setup_function(): _skill_repo_db_mock.reset_mock() _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.side_effect = None @@ -145,8 +206,11 @@ def setup_function(): _skill_repo_db_mock.increment_skill_repository_downloads.return_value = 1 _skill_repo_db_mock.insert_skill_repository_record.return_value = 1 _skill_repo_db_mock.list_skill_repository_by_skill_ids.return_value = [] + _skill_repo_db_mock.reset_skill_repository_status.return_value = 0 _skill_repo_db_mock.update_skill_repository_by_id.return_value = 1 _skill_repo_db_mock.update_skill_repository_status_by_id.return_value = 1 + _group_db_mock.reset_mock() + _group_db_mock.query_group_ids_by_user.return_value = [] _skill_db_mock.reset_mock() _skill_db_mock.get_skill_by_name.return_value = None _user_tenant_db_mock.reset_mock() @@ -169,7 +233,11 @@ def _repository_record(status="not_shared", publisher_user_id="user-1"): "submitted_by": "dev@example.com", "tags": ["tag"], "downloads": 0, - "skill_info_json": {"content": "content", "tags": ["tag"]}, + "skill_info_json": { + "content": "content", + "tags": ["tag"], + "created_by": "user-1", + }, "create_time": None, "update_time": None, } @@ -211,6 +279,28 @@ def test_create_skill_repository_listing_updates_existing_record(): _skill_repo_db_mock.update_skill_repository_by_id.assert_called_once() +def test_create_skill_repository_listing_does_not_overwrite_shared_record(): + _skill_repo_db_mock.get_skill_repository_by_skill_id.side_effect = [None, None] + _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = ( + _repository_record(status="pending_review") + ) + + result = srs.create_skill_repository_listing_impl( + skill_id=10, + tenant_id="tenant-1", + user_id="user-1", + ) + + assert result["is_updated"] is False + _skill_repo_db_mock.insert_skill_repository_record.assert_called_once() + _skill_repo_db_mock.update_skill_repository_by_id.assert_not_called() + requested_statuses = [ + call.kwargs["statuses"] + for call in _skill_repo_db_mock.get_skill_repository_by_skill_id.call_args_list + ] + assert requested_statuses == [["pending_review"], ["rejected"]] + + def test_create_skill_repository_listing_rejects_non_owner_dev(): class SkillServiceNonOwner(_SkillServiceMock): def get_skill_by_id(self, skill_id, tenant_id=None): @@ -248,6 +338,12 @@ def test_update_status_admin_approves_pending_review(): assert result["status"] == "shared" _skill_repo_db_mock.update_skill_repository_status_by_id.assert_called_once() + _skill_repo_db_mock.reset_skill_repository_status.assert_called_once_with( + repository_id=1, + skill_id=10, + status="shared", + publisher_tenant_id="tenant-1", + ) def test_update_status_dev_cannot_approve_review(): @@ -279,21 +375,30 @@ def test_update_status_dev_cannot_update_other_users_listing(): def test_install_skill_from_repository_success_increments_downloads(): + create_kwargs = {} + + class CapturingSkillService(_SkillServiceMock): + def create_skill_from_zip_bytes(self, **kwargs): + create_kwargs.update(kwargs) + return super().create_skill_from_zip_bytes(**kwargs) + encoded_zip = base64.b64encode(b"zip").decode("ascii") _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = { **_repository_record(status="shared"), "skill_zip_base64": encoded_zip, } - result = srs.install_skill_from_repository_impl( - skill_repository_id=1, - tenant_id="tenant-1", - user_id="user-1", - target_name="Skill A Copy", - ) + with patch.object(srs, "SkillService", CapturingSkillService): + result = srs.install_skill_from_repository_impl( + skill_repository_id=1, + tenant_id="tenant-1", + user_id="user-1", + target_name="Skill A Copy", + ) assert result["name"] == "Skill A Copy" assert result["source"] == "repository" + assert create_kwargs["ingroup_permission"] == "READ_ONLY" _skill_repo_db_mock.increment_skill_repository_downloads.assert_called_once_with( repository_id=1, user_id="user-1", @@ -356,10 +461,59 @@ def list_skills(self, tenant_id=None): assert [item["name"] for item in result["items"]] == ["Excel Report"] +def test_mine_ownership_uses_creator_not_edit_permission(): + class ListSkillService(_SkillServiceMock): + def list_skills(self, tenant_id=None): + return [ + { + "skill_id": 1, + "name": "Created Skill", + "created_by": 100, + "group_ids": [1], + "ingroup_permission": "EDIT", + }, + { + "skill_id": 2, + "name": "Editable Skill", + "created_by": 200, + "group_ids": [1], + "ingroup_permission": "EDIT", + }, + ] + + with ( + patch.object(srs, "SkillService", ListSkillService), + patch.object( + srs, + "get_user_tenant_by_user_id", + return_value={"user_role": "DEV"}, + ), + patch.object(_group_db_mock, "query_group_ids_by_user", return_value=[1]), + ): + created_result = srs.list_my_editable_skills_impl( + tenant_id="tenant-1", + user_id="100", + ownership="created", + ) + others_result = srs.list_my_editable_skills_impl( + tenant_id="tenant-1", + user_id="100", + ownership="others", + ) + + assert created_result["counts"] == {"all": 2, "created": 1, "others": 1} + assert [item["name"] for item in created_result["items"]] == ["Created Skill"] + assert [item["name"] for item in others_result["items"]] == ["Editable Skill"] + assert others_result["items"][0]["permission"] == "EDIT" + assert created_result["items"][0]["can_publish"] is True + assert others_result["items"][0]["can_publish"] is False + + def test_list_repository_listings_validates_status(): with pytest.raises(ValueError): srs.list_skill_repository_listings_impl( "tenant-1", + user_id="user-1", status="bad_status", ) @@ -519,15 +673,55 @@ def test_repository_list_and_detail_success(): } result = srs.list_skill_repository_listings_impl( "tenant-1", + user_id="user-1", status="shared", ) assert result["items"][0]["status"] == "shared" + assert result["items"][0]["can_take_down"] is True + + result = srs.list_skill_repository_listings_impl( + "tenant-1", + user_id="user-2", + status="shared", + ) + assert result["items"][0]["can_take_down"] is False _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = ( _repository_record(status="shared") ) detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") assert detail["skill_repository_id"] == 1 + assert detail["author"] == "dev@example.com" + + _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = None + detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") + assert detail["author"] is None + + record_without_creator = _repository_record(status="shared") + record_without_creator["skill_info_json"]["created_by"] = None + _skill_repo_db_mock.get_skill_repository_by_id_and_publisher.return_value = record_without_creator + _user_tenant_db_mock.get_user_tenant_by_user_id.reset_mock() + detail = srs.get_skill_repository_listing_detail_impl(1, "tenant-1") + assert detail["author"] is None + _user_tenant_db_mock.get_user_tenant_by_user_id.assert_not_called() + + +def test_repository_list_does_not_grant_take_down_to_regular_user(): + _skill_repo_db_mock.list_skill_repository_summaries.return_value = { + "items": [_repository_record(status="shared")], + "pagination": {"total": 1}, + } + _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = { + "user_role": "USER" + } + + result = srs.list_skill_repository_listings_impl( + "tenant-1", + user_id="user-1", + status="shared", + ) + + assert result["items"][0]["can_take_down"] is False def test_mapping_and_filter_helpers_cover_edge_branches(): @@ -550,11 +744,6 @@ def test_mapping_and_filter_helpers_cover_edge_branches(): assert srs._paginate_mine_skills_with_optional_padding([], 1, 10, False) == ([], 0) _user_tenant_db_mock.get_user_tenant_by_user_id.return_value = None assert srs._get_user_role("missing-user") == "USER" - assert srs._resolve_mine_skill_permission( - skill={"created_by": "someone-else"}, - user_id="admin-1", - user_role="ADMIN", - ) == srs.PERMISSION_EDIT assert srs._normalize_listing_tags(None) == [] with pytest.raises(ValueError, match="icon is required"): srs._validate_card_fields({"icon": 123}) diff --git a/test/backend/services/test_skill_service.py b/test/backend/services/test_skill_service.py index bba58e41e..a322ede0b 100644 --- a/test/backend/services/test_skill_service.py +++ b/test/backend/services/test_skill_service.py @@ -162,6 +162,10 @@ def get_cached_message(self): consts_const_mock.CONTAINER_SKILLS_PATH = TEST_LOCAL_SKILLS_DIR consts_const_mock.OFFICIAL_SKILLS_ZIP_PATH = "/tmp/official-skills.zip" consts_const_mock.ROOT_DIR = "/tmp" +consts_const_mock.CAN_EDIT_ALL_USER_ROLES = {"ADMIN"} +consts_const_mock.PERMISSION_EDIT = "EDIT" +consts_const_mock.PERMISSION_PRIVATE = "PRIVATE" +consts_const_mock.PERMISSION_READ = "READ_ONLY" consts_exceptions_mock = types.ModuleType('consts.exceptions') class SkillException(Exception): @@ -231,6 +235,10 @@ def get_skill(self, skill_name, tenant_id=None): utils_prompt_template_utils_mock = types.ModuleType('utils.prompt_template_utils') utils_prompt_template_utils_mock.get_skill_creation_simple_prompt_template = MagicMock(return_value={"system_prompt": "", "user_prompt": ""}) utils_content_classifier_utils_mock = types.ModuleType('utils.content_classifier_utils') +utils_str_utils_mock = types.ModuleType('utils.str_utils') +utils_str_utils_mock.convert_list_to_string = MagicMock( + side_effect=lambda items: "" if items is None else ",".join(str(item) for item in items) +) class MockContentClassifier: def classify(self, content): @@ -241,6 +249,7 @@ def classify(self, content): sys.modules['utils.skill_params_utils'] = utils_skill_params_utils_mock sys.modules['utils.prompt_template_utils'] = utils_prompt_template_utils_mock sys.modules['utils.content_classifier_utils'] = utils_content_classifier_utils_mock +sys.modules['utils.str_utils'] = utils_str_utils_mock # Set up database mocks database_mock = types.ModuleType('database') @@ -251,6 +260,12 @@ def classify(self, content): database_db_models_mock = types.ModuleType('database.db_models') database_db_models_mock.SkillInfo = MagicMock() +database_group_db_mock = types.ModuleType('database.group_db') +database_group_db_mock.query_group_ids_by_user = MagicMock(return_value=[]) +database_user_tenant_db_mock = types.ModuleType('database.user_tenant_db') +database_user_tenant_db_mock.get_user_tenant_by_user_id = MagicMock( + return_value={"user_role": "DEV"} +) # Create mock skill_db module with functions database_skill_db_mock = types.ModuleType('database.skill_db') @@ -340,6 +355,8 @@ def mock_update_skill_by_id(skill_id, skill_data, tenant_id=None, updated_by=Non sys.modules['database.client'] = database_client_mock sys.modules['database.skill_db'] = database_skill_db_mock sys.modules['database.db_models'] = database_db_models_mock +sys.modules['database.group_db'] = database_group_db_mock +sys.modules['database.user_tenant_db'] = database_user_tenant_db_mock setattr(database_mock, 'skill_db', database_skill_db_mock) # Mock nexent.core.agents.run_agent for create_skill_from_request @@ -386,6 +403,148 @@ def create_test_service(tenant_id="test-tenant"): # ===== Helper Functions Tests ===== +class TestSkillGroupPermissions: + def test_group_permission_helpers_handle_edit_read_only_and_private(self): + group_skill = { + "created_by": "creator", + "group_ids": "10,invalid,20", + "ingroup_permission": "EDIT", + } + + assert skill_service._to_group_id_set(group_skill["group_ids"]) == {10, 20} + assert skill_service.can_view_skill( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) is True + assert skill_service.resolve_skill_permission( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) == "EDIT" + + group_skill["ingroup_permission"] = "READ_ONLY" + assert skill_service.resolve_skill_permission( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) == "READ_ONLY" + + group_skill["ingroup_permission"] = "PRIVATE" + assert skill_service.can_view_skill( + skill=group_skill, + user_id="member", + user_role="DEV", + user_group_ids={10}, + ) is False + + def test_group_permission_helpers_preserve_creator_and_admin_access(self): + private_skill = { + "created_by": "creator", + "group_ids": [], + "ingroup_permission": "PRIVATE", + } + + assert skill_service.can_view_skill( + skill=private_skill, + user_id="creator", + user_role="DEV", + user_group_ids=set(), + ) is True + assert skill_service.resolve_skill_permission( + skill=private_skill, + user_id="admin", + user_role="ADMIN", + user_group_ids=set(), + ) == "EDIT" + + def test_group_permission_helpers_reject_unmatched_groups_and_normalize_lists(self): + skill = { + "created_by": "creator", + "group_ids": [10, "20", "invalid"], + "ingroup_permission": "EDIT", + } + + assert skill_service._to_group_id_set(skill["group_ids"]) == {10, 20} + assert skill_service._to_group_id_set(None) == set() + assert skill_service.can_view_skill( + skill=skill, + user_id="outsider", + user_role="DEV", + user_group_ids={30}, + ) is False + assert skill_service.resolve_skill_permission( + skill=skill, + user_id="outsider", + user_role="DEV", + user_group_ids={30}, + ) == "READ_ONLY" + + def test_default_group_permission_does_not_override_explicit_values(self, mocker): + skill_data = { + "group_ids": [99], + "ingroup_permission": "READ_ONLY", + } + query_groups = mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10], + ) + + skill_service._apply_default_skill_permission_fields(skill_data, "user-1") + + assert skill_data == { + "group_ids": [99], + "ingroup_permission": "READ_ONLY", + } + query_groups.assert_not_called() + + def test_default_group_permission_uses_creator_groups(self, mocker): + skill_data = {} + mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10, 20], + ) + + skill_service._apply_default_skill_permission_fields(skill_data, "user-1") + + assert skill_data == { + "group_ids": "10,20", + "ingroup_permission": "EDIT", + } + + def test_default_group_permission_skips_anonymous_user(self, mocker): + query_groups = mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + ) + + skill_data = {} + skill_service._apply_default_skill_permission_fields(skill_data, None) + + assert skill_data == {} + query_groups.assert_not_called() + + def test_can_edit_skill_requires_user_and_allows_group_editor(self, mocker): + skill = { + "created_by": "owner", + "group_ids": [10], + "ingroup_permission": "EDIT", + } + mocker.patch( + "backend.services.skill_service.get_user_tenant_by_user_id", + return_value={"user_role": "DEV"}, + ) + mocker.patch( + "backend.services.skill_service.query_group_ids_by_user", + return_value=[10], + ) + + assert skill_service._can_edit_skill(skill, None) is False + assert skill_service._can_edit_skill(skill, "group-editor") is True + + class TestNormalizeZipEntryPath: """Test _normalize_zip_entry_path function.""" @@ -572,6 +731,96 @@ def test_list_skills_error(self, mocker): with pytest.raises(Exception): service.list_skills() + def test_list_skills_filters_by_creator_group_and_private_permission(self, mocker): + mocker.patch( + 'backend.services.skill_service.skill_db.list_skills', + return_value=[ + { + "skill_id": 1, + "name": "own-private", + "created_by": "user-1", + "group_ids": [], + "ingroup_permission": "PRIVATE", + }, + { + "skill_id": 2, + "name": "group-read-only", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "READ_ONLY", + }, + { + "skill_id": 3, + "name": "group-edit", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "EDIT", + }, + { + "skill_id": 4, + "name": "group-private", + "created_by": "user-2", + "group_ids": [10], + "ingroup_permission": "PRIVATE", + }, + { + "skill_id": 5, + "name": "different-group", + "created_by": "user-2", + "group_ids": [20], + "ingroup_permission": "EDIT", + }, + ], + ) + mocker.patch( + 'backend.services.skill_service.query_group_ids_by_user', + return_value=[10], + ) + mocker.patch( + 'backend.services.skill_service.get_user_tenant_by_user_id', + return_value={"user_role": "DEV"}, + ) + + result = create_test_service().list_visible_skills(user_id="user-1") + + assert [skill["name"] for skill in result] == [ + "own-private", + "group-read-only", + "group-edit", + ] + assert [skill["permission"] for skill in result] == [ + "EDIT", + "READ_ONLY", + "EDIT", + ] + + def test_list_skills_admin_can_view_all_tenant_skills(self, mocker): + mocker.patch( + 'backend.services.skill_service.skill_db.list_skills', + return_value=[ + { + "skill_id": 1, + "name": "private-skill", + "created_by": "user-2", + "group_ids": [], + "ingroup_permission": "PRIVATE", + } + ], + ) + mocker.patch( + 'backend.services.skill_service.query_group_ids_by_user', + return_value=[], + ) + mocker.patch( + 'backend.services.skill_service.get_user_tenant_by_user_id', + return_value={"user_role": "ADMIN"}, + ) + + result = create_test_service().list_visible_skills(user_id="admin-1") + + assert len(result) == 1 + assert result[0]["permission"] == "EDIT" + class TestSkillServiceGetSkill: """Test SkillService.get_skill method.""" @@ -858,6 +1107,26 @@ def test_update_skill_success(self, mocker): assert result["description"] == "updated" + def test_update_skill_rejects_user_without_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=False, + ) + + service = SkillService(tenant_id="test-tenant") + + with pytest.raises(skill_service.ForbiddenError): + service.update_skill( + "existing", + {"description": "updated"}, + tenant_id="test-tenant", + user_id="viewer", + ) + def test_update_skill_with_params(self, mocker): mocker.patch( 'backend.services.skill_service.skill_db.get_skill_by_name', @@ -1939,6 +2208,55 @@ def test_update_from_md_explicit_type(self, mocker): assert result["description"] == "updated" + def test_update_from_file_rejects_user_without_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=False, + ) + + service = SkillService(tenant_id="test-tenant") + + with pytest.raises(skill_service.ForbiddenError): + service.update_skill_from_file( + "existing", + b"---\nname: existing\n---", + tenant_id="test-tenant", + user_id="viewer", + ) + + def test_update_from_file_allows_user_with_edit_permission(self, mocker): + mocker.patch( + "backend.services.skill_service.skill_db.get_skill_by_name", + return_value={"skill_id": 1, "name": "existing", "created_by": "owner"}, + ) + can_edit = mocker.patch( + "backend.services.skill_service._can_edit_skill", + return_value=True, + ) + mocker.patch( + "backend.services.skill_service.skill_db.update_skill", + return_value={"skill_id": 1, "name": "existing"}, + ) + mocker.patch( + "backend.services.skill_service.skill_db.get_tool_ids_by_names", + return_value=[], + ) + service = SkillService(tenant_id="test-tenant") + service.skill_manager = MagicMock() + service._enrich_configs_from_yaml = lambda result: result + service.update_skill_from_file( + "existing", + b"---\nname: existing\n---\n# Content", + file_type="md", + user_id="group-editor", + ) + + can_edit.assert_called_once() + def test_update_from_zip(self, mocker): import zipfile zip_buffer = io.BytesIO() @@ -5808,4 +6126,3 @@ def test_non_dict_value(self): """Test with non-dict/cm value.""" result = skill_service._tooltip_for_commented_map_key("not a map", [], 0, "key") assert result is None - diff --git a/test/sdk/core/agents/test_nexent_agent.py b/test/sdk/core/agents/test_nexent_agent.py index ab54d347f..f0784a731 100644 --- a/test/sdk/core/agents/test_nexent_agent.py +++ b/test/sdk/core/agents/test_nexent_agent.py @@ -4065,19 +4065,23 @@ def test_create_builtin_tool_run_skill_script(self, nexent_agent_instance): }, ) - # Mock the skill script tool module - mock_run_skill_script = MagicMock() - mock_get_tool = MagicMock(return_value=mock_run_skill_script) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.run_skill_script_tool": MagicMock( - get_run_skill_script_tool=mock_get_tool, - run_skill_script=mock_run_skill_script + RunSkillScriptTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_run_skill_script - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + observer=nexent_agent_instance.observer, + ) def test_create_builtin_tool_read_skill_md(self, nexent_agent_instance): """Test create_builtin_tool with ReadSkillMdTool.""" @@ -4096,18 +4100,22 @@ def test_create_builtin_tool_read_skill_md(self, nexent_agent_instance): }, ) - mock_read_skill_md = MagicMock() - mock_get_tool = MagicMock(return_value=mock_read_skill_md) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.read_skill_md_tool": MagicMock( - get_read_skill_md_tool=mock_get_tool, - read_skill_md=mock_read_skill_md + ReadSkillMdTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_read_skill_md - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_write_skill_file(self, nexent_agent_instance): """Test create_builtin_tool with WriteSkillFileTool.""" @@ -4126,18 +4134,22 @@ def test_create_builtin_tool_write_skill_file(self, nexent_agent_instance): }, ) - mock_write_skill_file = MagicMock() - mock_get_tool = MagicMock(return_value=mock_write_skill_file) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.write_skill_file_tool": MagicMock( - get_write_skill_file_tool=mock_get_tool, - write_skill_file=mock_write_skill_file + WriteSkillFileTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_write_skill_file - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_read_skill_config(self, nexent_agent_instance): """Test create_builtin_tool with ReadSkillConfigTool.""" @@ -4156,18 +4168,22 @@ def test_create_builtin_tool_read_skill_config(self, nexent_agent_instance): }, ) - mock_read_skill_config = MagicMock() - mock_get_tool = MagicMock(return_value=mock_read_skill_config) + mock_tool_instance = MagicMock() + mock_tool_class = MagicMock(return_value=mock_tool_instance) with patch.dict("sys.modules", { "nexent.core.tools.read_skill_config_tool": MagicMock( - get_read_skill_config_tool=mock_get_tool, - read_skill_config=mock_read_skill_config + ReadSkillConfigTool=mock_tool_class, ) }): result = nexent_agent_instance.create_builtin_tool(tool_config) - assert result is mock_read_skill_config - mock_get_tool.assert_called_once() + assert result is mock_tool_instance + mock_tool_class.assert_called_once_with( + local_skills_dir="/tmp/skills", + agent_id="agent_123", + tenant_id="tenant_456", + version_no=1, + ) def test_create_builtin_tool_unknown_tool(self, nexent_agent_instance): """Test create_builtin_tool raises ValueError for unknown tool."""